-
Notifications
You must be signed in to change notification settings - Fork 745
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
Remove head tracker in favour of fork choice #1785
Comments
## 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).
Some notes for non-michael readers and myself HeadTracker is used for two things currently:
How to prune hot dataBlocks and states are persisted in the hot DB by root. To figure out what to prune you need to build a block DAG and compute what branches are non-viable / abandoned. Some ways to do it:
The pruning routine must be crash safe, options 2 and 3 risk "forgetting" data or causing inconsistencies like #4773 if not implemented properly. Brute-forceThe current implementation uses the Using fork-choiceUsing the fork-choice for pruning without holding its lock for too long may require persisting some prune helper data to the database. The desired order of operations is:
If the lock is not hold for this long and the node crashes during 3 then you lose track of pruned branches that will never be pruned. Another approach is to:
Then in a background thread, and in an atomic operation
Here you are sort of storing a HeadTracker but only for prune data that is decoupled from the fork-choice. HeadTracker but eagerly persistedWe can achieve better atomicity in the HeadTracker by persisting each head to the DB individually in separate keys. Heads are inserted in the HeadTracker in the existing atomic transaction during block import. Heads are pruned during the atomic operation of hot DB pruning. No action is necessary on shutdown. The pruning routine can stream all keys in that column range to iterate the HeadTracker. This option allows to keep the pruning logic the same but remove the potential inconsistencies on shutdown. |
I don't mind the brute-force approach, but it would only be viable with I also quite like the pruning-summary & per-block head tracker. Maybe the pruning summary is the simplest for now? Or we wait until tree-states and do the brute force. I like the simplicity of not worrying about lock ordering. |
More reasons to delete:
|
Description
The head tracker contains redundant information that is already present in the fork choice block DAG. This is undesirable as it means there isn't just "one source of truth", and the two data structures need to be kept in sync across concurrent executions, which is highly non-trivial (see #1771 for more)
Steps to resolve
HeadTracker
struct from the beacon chaindeleted
in fork choice, until they are cleaned up by fork choice's own pruning mechanismThe text was updated successfully, but these errors were encountered: