core, eth, trie: prepare trie sync for path based operation#21504
core, eth, trie: prepare trie sync for path based operation#21504karalabe merged 1 commit intoethereum:masterfrom karalabe:trie-path-sync
Conversation
eth/downloader/statesync.go
Outdated
There was a problem hiding this comment.
| trieTasks map[common.Hash]*trieTask // Trie node ownload tasks to track previous attempts | |
| trieTasks map[common.Hash]*trieTask // Trie node download tasks to track previous attempts |
trie/sync.go
Outdated
There was a problem hiding this comment.
This change looks like it's pretty substantial -- like this was previously wrong. What's the impact of changing this here?
There was a problem hiding this comment.
It was completely wrong before. It didn't pass in the full path to the leaf, rather omitted the suffix of the short node that ends in the leaf.
There was a problem hiding this comment.
So if we had a path like:
FullNode --0--> FullNode --1--> ShortNode --23456--> Account
Then the callback used the path 01 instead of 0123456 because the delivered request req was the ShortNode, but we're calling the callback on the value child of the short node.
There was a problem hiding this comment.
And now, does it use 23456 or 0123456 ?
There was a problem hiding this comment.
holiman
left a comment
holiman
left a comment
There was a problem hiding this comment.
Code LGTM, I've reached the limit of what I can review by eyes alone. Tests in production environment needed
karalabe
changed the title
karalabe
changed the title
| case *shortNode: | ||
| if len(path)-pos < len(n.Key) || !bytes.Equal(n.Key, path[pos:pos+len(n.Key)]) { | ||
| // Path branches off from short node | ||
| return nil, n, 0, nil |
There was a problem hiding this comment.
I'm not sure, but I have a hunch that this should be nil, n, 1, nil?
Because otherwise we don't set the root in line 157 properly.
Please disregard if I just didn't get it^^
There was a problem hiding this comment.
No, it's correct. That counter is meant to count how many hash nodes we've resolved (pulled up from the database). We only need to replace the root if we actually did expand the trie. This path doesn't expand the trie, it just reports a missing node.
3 participants
This is a small prep PR that separates trie node and byte code scheduling in
trie.Syncand also starts tracking the Merkle paths belonging to a specific trie request. The goal is to prepare the code forsnapsync, which has these two concepts separated and will also work on trie paths, not on hashes.The "paths" pointing to a specific trie node (returned by
trie.Sync.Missing) are either 1-tuple if the trie node belongs to the account trie; or a 2-tuple if it's a storage trie node. Partial paths (i.e. last items of both tuples) are stored in compact-encoded form, since they need to be able to represent odd nibbles. The first item of the 2-tuple (account hash) is simple binary encoding because we know it's always 32 bytes.The reason for using 2-tuple instead of 1 long concatenated path is because it's simpler to look it up by serving code, which doesn't need to interpret the compact-encoding, rather can delegate it to the trie (a concatenated path would cross multiple tries, so the
triepackage along is not meant to handle it). It also helps keep the downloader scheduler simpler and permits it (or snap) to group storage requests and not send the account hash for each one individually (not implemented in this PR).