Use persistent data structures in fork choice #2059
Closed
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
michaelsproul
added
work-in-progress
|
Bugger, looks like I'll investigate further tomorrow |
|
Alternative solutions to consider, in roughly improving order:
Or, run on a forked + fixed version of |
|
It seems to be about 2x slower in the average case too, which is pretty bad. The graph below shows a Pyrmont node syncing with this branch (13:00-16:45), and keeping up with the head. At 18:30 I switched to v1.0.3 for comparison, and you can see average fork choice time dropped from around 40ms to 20ms. At 22:30 I switched back to this branch, and you can see the runtime doubled again. The peaks are also a bit higher.
I'm going to shelve this approach for now and investigate alternatives. |
bors bot
pushed a commit
that referenced
this pull request
Dec 9, 2020
## Issue Addressed Closes #2028 Replaces #2059 ## Proposed Changes If writing to the database fails while importing a block, revert fork choice to the last version stored on disk. This prevents fork choice from being ahead of the blocks on disk. Having fork choice ahead is particularly bad if it is later successfully written to disk, because it renders the database corrupt (see #2028). ## Additional Info * This mitigation might fail if the head+fork choice haven't been persisted yet, which can only happen at first startup (see #2067) * This relies on it being OK for the head tracker to be ahead of fork choice. I figure this is tolerable because blocks only get added to the head tracker after successfully being written on disk _and_ to fork choice, so even if fork choice reverts a little bit, when the pruning algorithm runs, those blocks will still be on disk and OK to prune. The pruning algorithm also doesn't rely on heads being unique, technically it's OK for multiple blocks from the same linear chain segment to be present in the head tracker. This begs the question of #1785 (i.e. things would be simpler with the head tracker out of the way). Alternatively, this PR could just revert the head tracker as well (I'll look into this tomorrow).
michaelsproul
added
the
consensus
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.
Issue Addressed
Closes #2028
Proposed Changes
This PR fixes issues with fork choice becoming out of sync with the database after failed writes (the root cause of #2028). It does this using quite a drastic approach: rather than mutating fork choice directly, we take a clone of it, mutate the clone, and then atomically update the original once the database write has succeeded. In order to make the clone cheap(er), we use persistent data structures from the
imcrate.In order to make this work, I've added SSZ implementations for
ArcandVector.Additional Info
I'm still assessing the performance and memory impact of this change. We should definitely test it for a substantial period of time on our Pyrmont nodes to ensure the performance penalty is tolerable.