feat(consensus): add batched forward epoch sync - #819
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d95dd942b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .filter(|boundary| { | ||
| boundary.certifying_position >= start && | ||
| boundary.certifying_position < end && | ||
| index.positions[&boundary.target_block_id] >= start && |
There was a problem hiding this comment.
Allow replay targets that precede the fetch cursor
When a node has already ordered the epoch-change target but has not committed it, fast_forward_sync_by_epoch_forward uses that ordered block as the fetch anchor and the older commit root as the replay anchor. The server then starts after the ordered block, but this predicate rejects the epoch-ending ledger-info boundary because its target position is before start, even though its certifying suffix QC is in the requested batch. The fetch returns BatchBoundaryNotFound, and the caller propagates that error rather than using the legacy fallback, so a validator/fullnode in this common non-blocking epoch-change state cannot finish the epoch transition. Reproduce by setting the ordered root to the epoch-change target, leaving the commit root behind, and requesting its certifying suffix; permit an authenticated replay target at or before the fetch cursor and validate it against the existing replay anchor/local block instead.
AGENTS.md reference: AGENTS.md:L5-L12
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efc06ea146
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| ensure!(record.randomness.is_none(), "Unnumbered forward block carries randomness"); |
There was a problem hiding this comment.
Require block numbers on non-suffix records
A Byzantine serving peer can mark an ordinary executable block as unnumbered, and this branch accepts it as long as randomness is also omitted, even though only reconfiguration suffixes may legitimately lack execution block numbers. persist_and_replay_forward_epoch_sync_batch then saves and inserts that block without a number, and recover_blocks_checked fails at block_number().ok_or(...); an honest retry repairs the DB mapping but insert_block returns the already-cached PipelinedBlock without copying the corrected number, so synchronization remains stuck until the block store is rebuilt or the node restarts. Validate None against is_reconfiguration_suffix() before persistence; this can be reproduced by changing one non-suffix record's number and randomness to None.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
| ); | ||
| anchor_block_number = block_number; | ||
| } else { | ||
| ensure!(record.randomness.is_none(), "Unnumbered forward block carries randomness"); |
There was a problem hiding this comment.
Authenticate fetched randomness before persisting it
For numbered records, the verifier accepts any record.randomness bytes without checking that they are the certified randomness for the block's epoch and round. The forward path persists those bytes before insertion, try_set_randomness_from_db installs them on the PipelinedBlock, and recovery feeds them into block metadata during execution; a Byzantine selected peer can therefore force an execution/hash mismatch and abort epoch sync. Retrying against an honest peer overwrites the DB value but does not repair the in-memory block because try_set_randomness_from_db refuses to replace randomness already present, leaving the node stuck until restart; the protocol needs an authenticated randomness proof or equivalent validation before persistence.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 913a733fe5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let Some(boundary) = boundary else { | ||
| return ForwardEpochSyncResponseV1::Error(ForwardEpochSyncError::BatchBoundaryNotFound); |
There was a problem hiding this comment.
Allow batches without a nearby commit boundary
When a valid epoch path has more than batch_size_blocks consensus blocks between replayable ledger infos—such as after a long sequence of certified but noncommitting rounds—select_forward_batch_end caps end at that size, this search finds no boundary, and the server permanently returns BatchBoundaryNotFound. The call path fast_forward_sync_by_epoch_forward → fetch_forward_epoch_sync_batch propagates that fetch error without invoking the legacy fallback, so the node cannot complete the epoch transition even though all records are valid. This can be reproduced by persisting batch_size_blocks + 1 chained certified records between two LedgerInfoSchema entries; support a persistence-only batch with an unchanged replay anchor, or otherwise continue fetching until a replay boundary without violating the server work bound.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
|
Opened #820: prepare timeout should be longer by default and configurable. |
Suggestion: lengthen prepare RPC timeout by default and make it configurableIn Duration::from_millis(1_000), // prepare — magic literal
Duration::from_millis(RPC_TIMEOUT_MSEC), // fetch — 5sWhy this mattersCold Recommendation
Happy to track this as a follow-up in this PR or a small follow-on change. |
Question for author:
|
|
@nekomoto911 Thanks — this is a real reachable window; we should not assume Addressed in
This keeps |
|
[High] Batch paging loop issues one more Fetch after consuming the last batch instead of waiting for the commit to land — the whole forward path then fails with ProblemIn
So the sync fails even though every block, QC, and ledger info of the epoch has already been fetched, verified, and persisted. Why "target not yet committed after the last batch" is the common case, not a raceCommit-root advancement is asynchronous with respect to batch processing: gravity-sdk/aptos-core/consensus/src/block_storage/block_store.rs Lines 621 to 634 in 1dad95a forward_epoch_sync_target_committed() == false and fires the doomed Fetch.
Notably, the sibling recovery branch already handles this exact situation correctly: ImpactNot a permanent stall, but a reliability regression on the new default path:
Suggested fixThe manifest intentionally carries no terminal/tail metadata (a certifying suffix past |
Summary
Compatibility
Validation