eth/downloader: dynamically move pivot even during chain sync #21529
Conversation
| headers := packet.(*headerPack).headers | ||
|
|
||
| log.Warn("Pivot seemingly stale, moving", "old", pivot, "new", headers[0].Number) | ||
| pivot = headers[0].Number.Uint64() |
There was a problem hiding this comment.
At this point, any two headers received will cause us to do WriteLastPivotNumber to db. I'm thinking this might lead to some internal assumptions / corruption failing if, for example, a peer moves the pivot back to genesis, or block 5...?
If we become moved back to genesis, then we're done ..?
So shouldn't we also check that the returned headers match the numbers we requested?
There was a problem hiding this comment.
Hmm, yeah. Problem is that checking the number is not particularly useful. I could just give you the correct number and junk root hash. We could add at least PoW verification (beside number), but that will not work on Clique networks (acceptable?).
There was a problem hiding this comment.
Checked the header numbers in this PR for now. PoW checks would need to push the consensus engine into the downloader too.
85a9911 to
4f2942a
Compare
|
@holiman Issues addressed, PTAL |
9986463 to
ec54ac4
Compare
ec54ac4 to
fb835c0
Compare
When we've implemented fast sync, we picked a static pivot block and kept it indefinitely. This worked well because we didn't have state pruning, and any (recent) block we picked, it was sure to remain available until we finished syncing.
Eventually we've implemented in-memory GC, which caused stale state to gradually disappear from the network. Although the head of the state trie was garbage collected quote fast (15 mins), most of the trie remained constant throughout sync. Thus it was fine to pick a static pivot block, keep syncing that, and only dynamically bump it when we've approached the chain head. This could be done via an easy hack where the block collector pushes the pivot forward if it accumulates more than 64 blocks after the old pivot.
Unfortunately, with snap sync, the snapshot belonging to a pivot block completely disappears the instance it gets older than 128 blocks. The effect is that we start syncing a recent pivot, and when it goes stale, we suspend state sync until it gets updated, which won't happen until after we reach the end of the chain. This is a problem because we waste precious time.
This PR fixes the issue by adding an additional pivot staleness detection mechanism. When we are fast syncing the header skeleton, after every batch (~36K fast blocks (no exec)), we do an additional header retrieval, asking for the current pivot and the next one (+64 blocks). If there is a next one returned, we switch state sync to the new one. This way we can detect that the snapshots are imminently disappearing from the network and we should switch to a new state root before that.
Cost wise this extra header retrieval doesn't matter much, we're retrieving on average 1 header / 36K headers imported, so all in all waste about 150KB bandwidth during a full sync of mainnet.
Implementation wise the PR went with locks because there's no coordinator / runner in the downloader, so introducing it + channels would have been quite an invasive change. Unfortunately this meant that we needed to retain both the old mechanism that operated on the result queue and add a new mechanism that operates on header retrievals.
Security wise the dynamic pivot depends on our master peer (it cannot be attacked by non-master peers). If the master peer is malicious, it can feed us an arbitrary starting snapshot. This is at worse a grieving attack because after we're done importing the chain, we'll fast sync on top and at worse fall back to the current performance.