Repository navigation
Conversation
- Add skip_execution_validation parameter to on_block - Introduce OptimisticRootsTable for tracking optimistic blocks - Implement process_block_optimistic and handle_invalid_payload - Add is_optimistic_candidate_block helper (SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY = 128) - Extend BlockRangeSyncer with optimistic_mode and new_optimistic constructor - Wire --optimistic-sync CLI flag (default true) into ManagerConfig and service - Hook PayloadStatus::Invalid to handle_invalid_payload for chain backtracking
Bugs fixed:
- handle_invalid_payload: after removing blocks from DB, store.get_head()
called filter_block_tree() which still traversed parent_root_index_multimap
and hit bail!("failed to get block") for the just-removed block.
Fix:
- Track last_valid_root during the head-to-invalid traversal loop so the
new head is known without re-querying the fork choice tree post-removal.
- Also clean slot_index_provider, parent_root_index_multimap (new
remove_child()), unrealized_justifications_provider for each removed root
so the in-memory fork choice tree stays fully consistent.
- Add remove_child(parent_root, child_root) to ParentRootIndexMultimapTable.
- Fix unused-mut warning in BeaconBlockTable::remove().
Tests added:
- test_optimistic_root_storage: B256->bool REDB insert/remove verified
- test_invalid_payload_multi_descendant_rollback: A->B->C->D chain,
C invalid, C+D removed, B becomes head, optimistic roots cleaned
- Unit tests for is_optimistic_candidate_block safe-distance logic:
test_within_safe_distance_is_candidate,
test_beyond_safe_distance_not_candidate_by_distance,
test_safe_slots_constant (SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY == 128),
test_make_block_fields
All 3 beacon chain tests pass.
Related to ReamLabs#1395
|
@perfogic is this PR fine for me to review, I seen the other 2 are blocked on your concern. |
…nge target - Align is_optimistic_candidate_block with Ethereum spec: block must be >= 128 slots behind head to be a candidate based on distance. - Add safety checks to handle_invalid_payload so unimported invalid blocks do not purge the canonical chain. - Decouple BlockRangeSyncer from head-sync by targeting finalized_slot, avoiding conflict with EPF Cohort 7 head-sync work. - Fix clippy warnings across execution-engine, beacon-state, and syncer. - Add test_handle_invalid_payload_unimported_block_preserves_head.
|
Thanks @perfogic and @KolbyML! Exactly — with the changes in this PR:
Looking forward to your review @KolbyML, and happy to address any comments or adjustments! |
…nd resolve validated ancestors - Propagate PayloadStatus from state_transition to on_block in fork choice - Mark blocks as optimistic in OptimisticRootsTable when EL status is Syncing/Accepted or execution validation is skipped - Resolve optimistic roots recursively on Valid execution payload - Query db.optimistic_roots_provider dynamically in calculate_sync_status for Beacon Node RPC - Fix pattern match bug on PayloadStatusV1 in gossipsub beacon_block validation - Add comprehensive ancestor resolution unit test in ream-chain-beacon
Update: Implementation & Test VerificationHey, quick update on this PR. I've pushed the remaining optimistic sync spec changes Here's what's new:
TestsRan everything locally, and it's all passing:
Branch is rebased, tests are green, and this PR is ready for review and merge 🚀 |
|
Hey @KolbyML 👋 |
KolbyML
left a comment
There was a problem hiding this comment.
I left 4 comments, the same issues reoccur a lot in the pR, please fix them all then rerequest my review again
- Replace string error parsing with typed EngineError enum (InvalidPayload, InvalidBlockHash) - Remove redundant optimistic root insertion and dead comment in process_block_optimistic - Properly propagate database Results with '?' across fork choice, chain, and store instead of discarding with 'let _ =' or '.ok()' - Handle file deletion idempotently for missing blob and column sidecars - Clarify variable naming (rename 'b' to 'beacon_block', fix 'execution_enigne' typo) - Add comprehensive tests for EngineError downcast and latest_valid_hash: None rollback behavior
|
Thanks @KolbyML for the detailed review. I've addressed all four points in
Also added tests for the
Thanks again for the review. |
|
Hey @KolbyML, hope you're having a good week! Just wanted to share a gentle bump on PR #1576 (optimistic sync) whenever you have a moment. All feedback from the last review has been addressed and local tests are passing. Whenever convenient, a quick re-review and CI approval would be awesome. Thanks! |
|
Hey @KolbyML, just following up as you suggested. Whenever you get a chance, could you please take a look at the PR? Thanks! |
What was wrong?
Currently, the beacon node halts or rejects block imports if the execution layer is not yet synced or returns
SYNCING/ACCEPTED(onlyVALIDwas permitted). Furthermore, historic block range syncing eagerly called the execution engine without optimistic synchronization support, slowing down sync and lacking a rollback mechanism for invalid execution payloads.How was it fixed?
verify_and_notify_new_payloadto returnPayloadStatusV1and allow block imports onVALID,ACCEPTED, andSYNCING.--optimistic-syncsupport toBlockRangeSyncerwithis_optimistic_candidate_blocksafe distance logic (128 slots) andprocess_block_optimisticfor deferred EL verification.OptimisticRootsTabletoBeaconDBto track roots of optimistically imported blocks.handle_invalid_payloadto prune invalid blocks, dependent states, and column sidecars, back-tracking tolatest_valid_hashwithout deadlock.Closes #395
To-Do