-
Notifications
You must be signed in to change notification settings - Fork 20.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
eth: add debug_storageRangeAt #14350
Conversation
The key was constructed from nibbles, which isn't possible for all nodes. Remove the only use of Key in LightTrie by always retrying with the original key that was looked up.
5fb8474
to
e1678ce
Compare
// in the case of an odd number. All remaining nibbles (now an even number) fit properly | ||
// into the remaining bytes. Compact encoding is used for nodes stored on disk. | ||
|
||
func hexToCompact(hex []byte) []byte { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you define typedefs for hex
and compact
encoding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because both types of key can be stored in shortNode.Key
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could always use casts there. Typedefs would be helpful to avoid confusion elsewhere - though the function naming accomplishes some of that at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to change it in this PR. We can do it in another PR.
trie/iterator.go
Outdated
} | ||
|
||
// NewNodeIterator creates an post-order trie iterator. | ||
func NewNodeIterator(trie *Trie) NodeIterator { | ||
// newNodeIterator creates an post-order trie iterator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preorder. :)
trie/iterator.go
Outdated
// NewNodeIterator creates an post-order trie iterator. | ||
func NewNodeIterator(trie *Trie) NodeIterator { | ||
// newNodeIterator creates an post-order trie iterator. | ||
func newNodeIterator(trie *Trie, start []byte) NodeIterator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the motivation behind making this internal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The motivation was to have a single constructor for each iterator. Please see the commit message.
trie/iterator.go
Outdated
for parent.child++; parent.child < len(node.Children); parent.child++ { | ||
child := node.Children[parent.child] | ||
// Full node, move to the first non-nil child. | ||
i := parent.index + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't this part of the loop initializer any longer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accident
trie/iterator_test.go
Outdated
if err := checkIteratorOrder(testdata1[1:], it); err != nil { | ||
t.Fatal(err) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add tests for seeking to the empty string, and for seeking to a key past the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to test empty string, all other tests use NodeIterator(nil)
;). I've added one that seeks past the end.
69e1404
to
2f13ce1
Compare
eth/api_test.go
Outdated
state, _ = state.New(common.Hash{}, db) | ||
addr = common.Address{0x01} | ||
keys = []common.Hash{ | ||
common.HexToHash("340dd630ad21bf010b4e676dbfa9ba9a02175262d1fa356232cfde6cb5b47ef2"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume these are the hashes of the keys in the array below? Could you add a note to that effect for anyone coming across this later?
43de846
to
b22dfc8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor issues mostly. Please check and consider whether to fix or not.
One potentially bigger issue I saw was in the state object deep copy. Please check hat part out in detail and explain why it works if it works.
} | ||
|
||
// Continue iteration to the next child | ||
outer: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️ I hate these labels so much :P
node, err := it.trie.resolveHash(hash, nil, nil) | ||
if err != nil { | ||
return err | ||
return it.stack[len(it.stack)-1], &parent.index, it.path, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why return the previous "item" if peeking at the next one fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has to return something. The idea was that the iterator wouldn't advance if there was an error fetching the next item.
trie/encoding.go
Outdated
// KEYBYTES encoding contains the actual key and nothing else. This encoding is the | ||
// input to most API functions. | ||
// | ||
// HEX encoding contains on byte for each nibble of the key and an optional trailing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onE byte
// the second-lowest encoding whether the node at the key is a value node. The low nibble | ||
// of the first byte is zero in the case of an even number of nibbles and the first nibble | ||
// in the case of an odd number. All remaining nibbles (now an even number) fit properly | ||
// into the remaining bytes. Compact encoding is used for nodes stored on disk. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sweet, I've never seen this part of the code in 2 years :))
key: []byte{0x12, 0x34, 0x56}, | ||
hexIn: []byte{1, 2, 3, 4, 5, 6}, | ||
hexOut: []byte{1, 2, 3, 4, 5, 6, 16}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test for encoding/decoding empty keys? The previous code had explicit checks whereas the new code does this implicitly by relying on the calculation returning an empty slice. There's even a slight API change, where the old code returned nil
in hexToKeybytes
whereas the new returns []byte{}
. I'd be happy to see an explicit test that verifies this weird case too.
trie/encoding_test.go
Outdated
{hex: []byte{0, 15, 1, 12, 11, 8, 16 /*term*/}, compact: []byte{0x20, 0x0f, 0x1c, 0xb8}}, | ||
// odd length, terminator | ||
{hex: []byte{15, 1, 12, 11, 8, 16 /*term*/}, compact: []byte{0x3f, 0x1c, 0xb8}}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test for encoding/decoding empty keys?
if self.trie != nil { | ||
// A shallow copy makes the two tries independent. | ||
cpy := *self.trie | ||
stateObject.trie = &cpy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't stateObject.trie
a trie.SecureTrie
? Because that one is a much fancier construct that won't "deep copy" so easily https://github.com/ethereum/go-ethereum/blob/master/trie/secure_trie.go#L44
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shallow copy support for SecureTrie was added in 710435b51, by yours truly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't trust that guy.... he looks shady af ;)
core/state/statedb.go
Outdated
@@ -296,6 +296,17 @@ func (self *StateDB) GetState(a common.Address, b common.Hash) common.Hash { | |||
return common.Hash{} | |||
} | |||
|
|||
// StorageTrie returns an iterator over storage trie of an account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look like an iterator to me :P
}, nil | ||
case *ethapi.JavascriptTracer: | ||
return tracer.GetResult() | ||
return nil, vm.Context{}, nil, fmt.Errorf("tx %x failed: %v", tx.Hash(), err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this ever happen? If the tx is already in our chain, I think it's safe to say this won't ever fail. (Mentioning because you deleted a similar check at tx.AsMessage
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The around AsMessage is for deriving the sender. I don't want to remove this one because many more things can go wrong (including DB errors, etc.).
'encode' and 'decode' are meaningless because the code deals with three encodings. Document the encodings and give a name to each one.
Make it so each iterator has exactly one public constructor: - NodeIterators can be created through a method. - Iterators can be created through NewIterator on any NodeIterator.
The 'step' method is split into two parts, 'peek' and 'push'. peek returns the next state but doesn't make it current. The end of iteration was previously tracked by setting 'trie' to nil. End of iteration is now tracked using the 'iteratorEnd' error, which is slightly cleaner and requires less code.
b22dfc8
to
207bd7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The new RPC method allows downloading storage in chunks.
debug_storageRangeAt
takes parametersblockHash
,txIndex
,account
,startKey
,limit
. ThestartKey
applies to the hashed key (instead of the preimage as originally suggested in #3407) . The returned storage values contain the preimage if available. The returned object also contains thenextKey
field, which will be non-null if there are more keys after the range that was returned.Example:
We agreed on a preliminary spec in #3407. This supersedes that PR.
The earlier commits leading up to the new API add support for seeking in the trie iterator.