fix(l1): do not cap the interrupted-batch recovery at one execute batch - #7261
Conversation
When the consensus client has already delivered every block of the gap, full sync executes all of them in a single `add_blocks_in_batch` call (the "Executing N pending blocks ... no peer download needed" path), with no EXECUTE_BATCH_SIZE chunking and one `forkchoice_update` at the very end. Such a batch can be thousands of blocks long: on Plataberget a node that had just finished its initial sync was OOM-killed 1 895 blocks into one and came back with the head 1 767 blocks below the state on disk. The one-batch bound added at review would have declined exactly that recovery. A head moved down on purpose is indistinguishable from an interrupted batch by the data alone, so guard the one path that does it instead: skip the recovery when the `sync-test` `SYNC_BLOCK_NUM` rewind is active. The bound test becomes a test that a gap of 1 800 blocks is adopted.
|
🤖 Kimi Code ReviewI'll review this PR which modifies interrupted batch recovery logic in ethrex's state sync mechanism. SummaryThis PR removes the Issues Found1. Race condition / TOCTOU in environment variable check (
|
🤖 Codex Code ReviewFindings
No other correctness or Ethereum-rule issues stood out in this diff; the rest is documentation/test adjustment around the larger interrupted-batch case. I did not run tests. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Lines of code reportTotal lines added: Detailed view |
…ad rewinds The helper's doc claimed set_sync_block was the only path that moves the head down on purpose. debug_setHead is another: it rewinds the head marker through apply_fork_choice without touching the trie. A rewind at or above the committed root leaves no journal entry above the head, so the recovery does not fire; a rewind below the committed root is re-adopted on restart, where the state walk previously failed with "Unknown state found in DB".
…ch (#7261) **Motivation** Follow-up to #7260. That PR recovers a database whose canonical head lags the trie state committed by an interrupted full-sync batch, but it declines to recover when the journaled block is more than one execute batch (1024 blocks) above the head. That bound is too tight for a real case. When the consensus client has already delivered every block of the gap, the full syncer executes the whole gap in a single `add_blocks_in_batch` call with no per-chunk forkchoice update (`crates/networking/p2p/sync/full.rs`, "Executing N pending blocks for full sync"). On a glamsterdam-devnet-8 node the gap was 1895 blocks: the node finished its fresh full sync, ran one such batch, was OOM-killed 1895 blocks in, and came back with the head marker 1895 blocks below the committed state. With the bound in place the recovery does nothing and the node loops on "Unknown state found in DB" until the database is wiped. **Description** - Remove the one-execute-batch bound. The recovery is already gated by the checks that matter: the journaled block must chain down onto the current head and its state root must be present on disk. Neither depends on the size of the gap. - Keep `sync-test` (`SYNC_BLOCK_NUM`) working: that feature moves the head down on purpose right before `regenerate_head_state` runs, so the state above the head is not an interrupted batch and must not be re-adopted. The recovery is skipped when the variable is set under that feature. - Replace the "declines a gap wider than one batch" test with one that adopts a journaled block 1800 blocks above the head. - Document how the recovery relates to `debug_setHead`, the other path that moves the head down on purpose. That RPC only moves the head marker through `apply_fork_choice`: a rewind at or above the committed root leaves no journal entry above the head, so the recovery does not fire; a rewind below the committed root is re-adopted on restart, where the state walk previously failed with "Unknown state found in DB" (such a rewind never survived a restart). **How to Test** ```sh cargo test -p ethrex --lib interrupted_batch_recovery_tests cargo clippy -p ethrex --bin ethrex --all-targets -- -D warnings cargo clippy -p ethrex --bin ethrex --features sync-test --all-targets -- -D warnings ``` On a node bricked the same way (head marker below the last committed trie layer, gap > 1024), start the node with this build: it logs `Recovered from an interrupted full-sync batch: canonical head moved from <head> to <committed>` and resumes syncing instead of failing with "Unknown state found in DB". **Checklist** - [ ] Updated `STORE_SCHEMA_VERSION` (crates/storage/lib.rs) if the PR includes breaking changes to the `Store` requiring a re-sync. (Not needed: no schema change.) (cherry picked from commit 4857b8d)
2 participants
Motivation
Follow-up to #7260. That PR recovers a database whose canonical head lags the trie state committed by an interrupted full-sync batch, but it declines to recover when the journaled block is more than one execute batch (1024 blocks) above the head.
That bound is too tight for a real case. When the consensus client has already delivered every block of the gap, the full syncer executes the whole gap in a single
add_blocks_in_batchcall with no per-chunk forkchoice update (crates/networking/p2p/sync/full.rs, "Executing N pending blocks for full sync"). On a glamsterdam-devnet-8 node the gap was 1895 blocks: the node finished its fresh full sync, ran one such batch, was OOM-killed 1895 blocks in, and came back with the head marker 1895 blocks below the committed state. With the bound in place the recovery does nothing and the node loops on "Unknown state found in DB" until the database is wiped.Description
sync-test(SYNC_BLOCK_NUM) working: that feature moves the head down on purpose right beforeregenerate_head_stateruns, so the state above the head is not an interrupted batch and must not be re-adopted. The recovery is skipped when the variable is set under that feature.debug_setHead, the other path that moves the head down on purpose. That RPC only moves the head marker throughapply_fork_choice: a rewind at or above the committed root leaves no journal entry above the head, so the recovery does not fire; a rewind below the committed root is re-adopted on restart, where the state walk previously failed with "Unknown state found in DB" (such a rewind never survived a restart).How to Test
cargo test -p ethrex --lib interrupted_batch_recovery_tests cargo clippy -p ethrex --bin ethrex --all-targets -- -D warnings cargo clippy -p ethrex --bin ethrex --features sync-test --all-targets -- -D warningsOn a node bricked the same way (head marker below the last committed trie layer, gap > 1024), start the node with this build: it logs
Recovered from an interrupted full-sync batch: canonical head moved from <head> to <committed>and resumes syncing instead of failing with "Unknown state found in DB".Checklist
STORE_SCHEMA_VERSION(crates/storage/lib.rs) if the PR includes breaking changes to theStorerequiring a re-sync. (Not needed: no schema change.)