eth/protocols/snap: make better use of delivered data #44
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Added a commit to make use of the storage estimator. Tested it out on goerli: EDIT: updated |
|
These are interesting: In many cases, this PR avoid the chunking totally (no healing required), and in other cases, heavily reduces the healing required. Plus, in cases like |
|
Running this on mannet now. Some preliminary data: Out of The largest partial delivery was |
|
From my mainnet run on a NUC: So snap |
|
This is how the parallelism was performed: I.e: |
karalabe
reviewed
Apr 26, 2021
eth/protocols/snap/sync.go
Outdated
| @@ -1803,30 +1804,46 @@ func (s *Syncer) processStorageResponse(res *storageResponse) { | |||
| // the subtasks for it within the main account task | |||
| if tasks, ok := res.mainTask.SubTasks[account]; !ok { | |||
| var ( | |||
| next common.Hash | |||
| keys = res.hashes[i] | |||
| lastKey = keys[len(keys)-1] | |||
There was a problem hiding this comment.
Out of curiosity, could someone send us an empty list of keys as the last batch and crash?
There was a problem hiding this comment.
Good question. Looks like it (but I wasn't able to trivially trigger it in a test). I'll fix it somehow
karalabe
reviewed
Apr 26, 2021
eth/protocols/snap/sync.go
Outdated
| // Somewhere on the order of 10K slots fits into a packet. We'd rather not chunk if | ||
| // each chunk is going to be only one single packet, so we use a factor of 2 to | ||
| // avoid chunking if we expect the remaining data to be filled with ~2 packets. | ||
| if n := estimate / (2 * 10000); n+1 < chunks { |
There was a problem hiding this comment.
This 10K hard coded will blow up the moment we change anything at the packet sizes. Perhaps lets use maxRequestSize / 64? That seems a bit saner.
holiman
commented
Apr 26, 2021
eth/protocols/snap/range.go
Outdated
| } | ||
|
|
||
| // newHashRange creates a new hashRange, initiated at the start position, | ||
| // and with the step set to fill the desired 'num' chunks | ||
| func newHashRange(start common.Hash, num uint64) *hashRange { | ||
| i := uint256.NewInt() | ||
| i.SetBytes32(start[:]) | ||
| left := new(big.Int).Sub(new(big.Int).Add(math.MaxBig256, common.Big1), start.Big()) |
There was a problem hiding this comment.
I don't get this.
new(big.Int).Add(math.MaxBig256, common.Big1) == 0 ?
Which makes this left := 0 - start ??
So step = (-start / num) which maybe works out in the end, but still I don't see how this is more correct than what was there previously?
There was a problem hiding this comment.
I used big ints, not uint256 for this calculation, so there's no overflow. The number of items in the hash range is 2^256, which cannot be represented by uint256. I needed the +1 for the correct calculation.
|
With the latest change, where we only stop the chunking at overflow, it sometimes gets into the position where the steps doesn't quite reach the end on the correct chunk, forcing one more tiny chunk to be created: Otherwise LGTM, but I'm not sure why you swapped to use |
|
The bug was that I rounded down the step, not up, so the last chunk got ever a bit smaller than needed to over the full range. Fixed now. |
|
LGTM! |
karalabe
added a commit
that referenced
this pull request
Apr 27, 2021
* eth/protocols/snap: make better use of delivered data * squashme * eth/protocols/snap: reduce chunking * squashme * eth/protocols/snap: reduce chunking further * eth/protocols/snap: break out hash range calculations * eth/protocols/snap: use sort.Search instead of looping * eth/protocols/snap: prevent crash on storage response with no keys * eth/protocols/snap: nitpicks all around * eth/protocols/snap: clear heal need on 1-chunk storage completion * eth/protocols/snap: fix range chunker, add tests Co-authored-by: Péter Szilágyi <peterke@gmail.com>
karalabe
added a commit
that referenced
this pull request
Apr 27, 2021
…thereum#22668) * eth/protocols/snap: generate storage trie from full dirty snap data * eth/protocols/snap: get rid of some more dead code * eth/protocols/snap: less frequent logs, also log during trie generation * eth/protocols/snap: implement dirty account range stack-hashing * eth/protocols/snap: don't loop on account trie generation * eth/protocols/snap: fix account format in trie * core, eth, ethdb: glue snap packets together, but not chunks * eth/protocols/snap: print completion log for snap phase * eth/protocols/snap: extended tests * eth/protocols/snap: make testcase pass * eth/protocols/snap: fix account stacktrie commit without defer * ethdb: fix key counts on reset * eth/protocols: fix typos * eth/protocols/snap: make better use of delivered data (#44) * eth/protocols/snap: make better use of delivered data * squashme * eth/protocols/snap: reduce chunking * squashme * eth/protocols/snap: reduce chunking further * eth/protocols/snap: break out hash range calculations * eth/protocols/snap: use sort.Search instead of looping * eth/protocols/snap: prevent crash on storage response with no keys * eth/protocols/snap: nitpicks all around * eth/protocols/snap: clear heal need on 1-chunk storage completion * eth/protocols/snap: fix range chunker, add tests Co-authored-by: Péter Szilágyi <peterke@gmail.com> * trie: fix test API error * eth/protocols/snap: fix some further liter issues * eth/protocols/snap: fix accidental batch reuse Co-authored-by: Martin Holst Swende <martin@swende.se>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a bit of a rough implementation, I haven't verifiied if it works totally correct.
Currently, when we retrieve a contract storage which is too large to fit in a single response, but otherwise "pretty small", then we discard data.
The received data might be
[0x0..,,,0x1...., 0x4..]. We divide the 'space' into 16 chunks, and immediately fill the first chunk with the data. Then we trim the edge, and thus keep[0x0..,]'but throw away[0x1...., 0x4..], which will instead be fetched separately in later chunks.This PR also adds the (so far not used) method
estimateRemainingSlots, which can be used to further tune the level of parallelism we choose.