Reimport the checkpoint sync block #8417
Conversation
michaelsproul
force-pushed
the
reimport-checkpoint-sync-block
branch
from
8183e08 to
a6607e5
Compare
|
Retargeting at release-v8.0 so we can include this in the v8.0.1 release and prevent gappy DBs |
beacon_node/network/src/network_beacon_processor/sync_methods.rs
Outdated
Show resolved
Hide resolved
michaelsproul
force-pushed
the
reimport-checkpoint-sync-block
branch
from
aeefac0 to
e1adef7
Compare
|
The only downside to this change that I can see is that we end up reimporting blocks which do not need importing:
These logs are from two checkpoint syncs on Hoodi. We can see backfill reimporting the first anchor block at slot 1768544, and then continuing to re-import epoch boundary blocks as it backfills further. It even reimported slot 1760896 three times. The second sync was from slot 1768800 which we see getting reimported, and then again backfill reimports another block 1767424. I think this is probably fine for now, it might just have a small impact on disk I/O. It doesn't seem we are reimporting on every backfill batch, it must be something related to how batches are handled and what we get from peers. This is not straight-forward to fix because we don't have easy access to the slot of the anchor block. The |
|
Confirmed working in the skipped slot case as well, just needed to fix an unrelated bug in unaligned checkpoint sync (commits: 173b719 ebf151f). IDK why this wasn't caught by the unaligned checkpoint sync test, I've opened an issue for that here: #8426. Logs:
This was using |
michaelsproul
left a comment
michaelsproul
left a comment
There was a problem hiding this comment.
Happy to merge whenever
|
The unaligned checkpoint sync fix broke some tests, will fix. |
|
Some required checks have failed. Could you please take a look @dapplion? 🙏 |
mergify
bot
added
waiting-on-author
|
Re-tested again on latest commit, for both aligned+unaligned cases. All working well. |
michaelsproul
added
ready-for-review
jimmygchen
added
ready-for-merge
|
This pull request has been removed from the queue for the following reason: The merge conditions cannot be satisfied due to failing checks: You can check the last failing draft PR here: #8433. You may have to fix your CI before adding the pull request to the queue again. |
|
@mergify requeue |
✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically |
Verify that checkpoint sync works when blobs are not provided during initialization. Backfill should fetch blobs/columns via P2P as enabled by sigp#8417.
Since merging this PR, we don't need `--checkpoint-blobs`, even prior to Fulu: - #8417 This PR removes the mandatory check for blobs prior to Fulu, enabling simpler manual checkpoint sync. Co-Authored-By: Michael Sproul <[email protected]> Co-Authored-By: Jimmy Chen <[email protected]>
Issue Addressed
We want to not require checkpoint sync starts to include the required custody data columns, and instead fetch them from p2p.
Closes #6837
Proposed Changes
The checkpoint sync slot can:
In both cases backfill sync already fetches that epoch worth of blocks with current code. This PR modifies the backfill import filter function to allow to re-importing the oldest block slot in the DB.
I feel this solution is sufficient unless I'm missing something.
I have not tested this yet!Michael has tested this and it works.