-
Notifications
You must be signed in to change notification settings - Fork 992
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
Rewind head and header_head consistently. #2918
Conversation
68df947
to
c78ca14
Compare
shortcircuit "rewind and apply fork" for headers if next header
This has been tested pretty extensively against mainnet for a few weeks now. Planning to merge this today as it frees up some flexibility for a subsequent in-progress PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 . Though, haven't had the time to review this extensively. But overall if you feel confident about it + tested for a while, I'd say LGTM.
I hoped this patch my avoid the crash of latest gin build on floonet that last synced before HF.
|
@tromp Unfortunately this PR fixes some issues that we uncovered when investigating your one, but not that specific issue itself. |
This PR makes a cleaner separation between the "txhashset" and the standalone header extension.
We have 3 "heads" that track different chain tips.
head
- the chain headheader_head
- the header chain headsync_head
- the sync header chain head (used when syncing headers from a peer)Current behavior is to update
sync_head
as we receive chunks of headers from a peer during header sync. We then updateheader_head
if these increase our total known work on a validated chain of headers.We use
header_head
to know which blocks to request - effectively the blocks we have not yet seen betweenhead
andheader_head
.During the "private floonet" hardfork testing we discovered a couple of issues, specifically around serialization/deserialization of headers in the local db (where
version={1|2}
).Some investigation around that led to exploring using the header MMR to read header hashes (instead of needing to deserialize a full header from the db).
This identified some room for significant simplication and improvement with respect to how we maintain
head
andheader_head
alongside our header MMR and output|rangeproof|kernel MMRs.Intuitively
header_head
should match the last entry in the header MMR.But this is not always the case due to how the header MMR interacts with the output|rangeproof|kernel MMRs in a full txhashset.
We would extend the header MMR when syncing headers but then when processing a full block we would sync the header MMR to disk aligned with the full txhashset (i.e. at
head
and not the further alongheader_head
).Proposed Solution (This PR)
These changes allow us to simplify a bunch of code in various places -
set_txhashset_roots_forked
is no longer required for testing fork behavior.This makes our rewind behavior significantly more robust in the presence of forks and reorgs.
We can now maintain a consistent
header_head
and rely on it being aligned with our current header MMR, even when processing blocks on chain forks.I think (but this is really hard to actually verify) this plugs a couple of gaps where we can corrupt the MMR structures if the node is shutdown during fork processing.