trie: more node iterator improvements #14615
Conversation
Set deadlocks immediately and isn't part of the Database interface.
fjl
force-pushed
the
trie-iterator-errtrack
branch
2 times, most recently
from
e06b8f5 to
262d96e
Compare
This is useful for testing because the underlying NodeIterator doesn't need to be kept in a separate variable just to get the error.
fjl
force-pushed
the
trie-iterator-errtrack
branch
3 times, most recently
from
699ed58 to
e8f756d
Compare
trie/encoding.go
Outdated
| func hexToKeybytes(hex []byte) []byte { | ||
| if hasTerm(hex) { | ||
| hex = hex[:len(hex)-1] | ||
| } | ||
| if len(hex)&1 != 0 { | ||
| panic("can't convert hex key of odd length") | ||
| // Odd length. This can happen if someone iterates a Trie |
There was a problem hiding this comment.
What's the purpose of this change?
There was a problem hiding this comment.
It's explained in the commit message:
This also removes the the panic for odd length because it is too subtle
to be useful. In a follow-up commit, using LeafKey in a core/state test
revealed that it iterates tries rooted in an intermediate node. This can
produce odd-length leaf keys and it's hard to debug. Removing the panic
removes risk of a rare production crash later.
There was a problem hiding this comment.
Isn't a 'rare production crash' the correct behaviour if we're iterating partial tries?
There was a problem hiding this comment.
Maybe. Not sure. Consider that the only use of hexToKeybytes is in LeafKey. With this change, LeafKey will return nil for leaves when iterating a subtrie rooted at odd level. Should it panic instead?
The odd-length panic is very new (added in f958d7d) and compared to one month ago, I no longer feel that it's strictly necessary.
There was a problem hiding this comment.
I still don't understand what the compelling reason to return nil here is, though. Is there any situation in which this would be expected?
| return it | ||
| } | ||
|
|
||
| // Hash returns the hash of the current node |
There was a problem hiding this comment.
Why delete these comments?
There was a problem hiding this comment.
They're not visible in GoDoc and other implementations of NodeIterator don't have comments either.
trie/iterator.go
Outdated
| state, parentIndex, path, err := it.peek(descend) | ||
| if err != nil { | ||
| it.err = err | ||
| if it.err = err; it.err != nil { |
There was a problem hiding this comment.
I don't think it makes sense to have the first statement here part of the if clause.
There was a problem hiding this comment.
hmm, I guess this is a style question that we can 🚲 🏠 over.
There was a problem hiding this comment.
I don't think it's bikeshedding. Would you agree this is bad style?
if it.err = err; err != nil {
they're functionally equivalent, but the latter demonstrates more clearly that the first part of the if statement is only being used for its side-effect.
There was a problem hiding this comment.
The optional first part of a condition is always used for side effect because it is a statement.
There was a problem hiding this comment.
Yes, but it's generally used to generate something; not just an assignment.
fjl
changed the title
LeafKey is useful for callers that can't interpret Path.
Instead of failing iteration irrecoverably, make it so Next retries the pending seek or peek every time. Smaller changes in this commit make this easier to test: * The iterator previously returned from Next on encountering a hash node. This caused it to visit the same path twice. * Path returned nibbles with terminator symbol for valueNode attached to fullNode, but removed it for valueNode attached to shortNode. Now the terminator is always present. This makes Path unique to each node and simplifies Leaf.
The light client trie iterator needs to know the path of the node that's missing so it can retrieve a proof for it. NodeIterator.Path is not sufficient because it is updated when the node is resolved and actually visited by the iterator. Also remove unused fields. They were added a long time ago before we knew which fields would be needed for the light client.
fjl
force-pushed
the
trie-iterator-errtrack
branch
from
fd46e3b to
3a2fcee
Compare
|
squashed |
Split out of #14589 to get review earlier.
The changes:
LeafKeymethod which returns the key of the current (leaf) node.MissingNodeErrorby inserting the node into the database before calling
Nextagain.Path.This makes
Pathunique.MissingNodeErrorcontains hex-encoded Path of the missing node. This is useful for the light client.