Skip to content

execution: support long reorgs in non-finality - #23612

Open
taratorio wants to merge 6 commits into
mainfrom
long_reorgs_finalised_hash
Open

execution: support long reorgs in non-finality#23612
taratorio wants to merge 6 commits into
mainfrom
long_reorgs_finalised_hash

Conversation

@taratorio

Copy link
Copy Markdown
Member

fixes #17070

(warning: written by a human)

we add support for reorgs longer than MAX_REORG_DEPTH during periods of non-finality by:

  • holding off pruning of changesets based on finalised block hash's block num
  • holding off pruning of bals based on finalised block hash's block num
  • holding off e2 block retirement based on finalised block hash's block num
  • holding off e3 state retirement based on finalised block hash's block num

@AskAlexSharov

Copy link
Copy Markdown
Collaborator

Reviewed the current head. Bugs only. Two of these are the same root cause: finalisedBlockNum replaces a bound that used to be derived from local progress, but nothing caps it by local progress — and the finalised hash outlives the state it is being compared against, because kv.LastForkchoice survives restarts, unwinds and re-execution.

1. canRetire can propose retiring blocks the node does not have

db/snapshotsync/freezeblocks/block_snapshots.go

if finalisedBlockNum > 0 {
    blockTo = finalisedBlockNum
} else {
    keep := br.config.MaxReorgDepth
    if curBlockNum <= keep { return }
    blockTo = curBlockNum - keep
}

The finality branch drops both guards the other branch has: no curBlockNum ceiling and no curBlockNum <= keep early return. On main every path was bounded by curBlockNum.

Verified in-package — finalised hash at block 900,000, curBlockNum 10,000, MaxReorgDepth 96, mainnet snCfg:

proposed retiring up to 100000 with only 10000 blocks available

Same inputs on main give blockTo <= 9904. dbHasEnoughDataForBlocksRetire does not catch it — it only checks for a gap at the low end (nextBlockInSnapshots < firstInDB), so DumpBlocks(blockFrom, blockTo, ...) then runs over a range with no bodies. BuildFiles loops while ok, so it re-enters rather than settling.

2. PruneExecutionStage can prune every changeset and BAL the node holds

execution/stagedsync/stage_execute.go

if finalisedBlockNum > 0 {
    blockPruneTo = finalisedBlockNum
} else if s.ForwardProgress > cfg.syncCfg.MaxReorgDepth {
    blockPruneTo = s.ForwardProgress - cfg.syncCfg.MaxReorgDepth
}

Same missing ceiling, against s.ForwardProgress this time. When the stored finalised block is above execution progress, blockPruneTo exceeds everything executed, so PruneTable on kv.ChangeSets3 and kv.BlockAccessList deletes all of it — not "keep the last MaxReorgDepth", but "keep nothing". Unwind capability is gone until those tables refill.

blockPruneTo == 0 is fine, incidentally: PruneTable breaks on the first key, so dropping the s.ForwardProgress > MaxReorgDepth guard is harmless on its own.

Both sites want min(finalisedBlockNum, <local horizon>), keeping the existing floor as the lower bound rather than replacing it.

How the datadir reaches that state: the finalised hash is persisted, and this PR's finalizeHead in cmd/utils/app/import_cmd.go writes finalized = head at the end of an import. Any later unwind or re-execution on that datadir (integration stage_exec, which calls PruneExecutionStage through collateAndPrune) then runs with finalised above progress.

3. ReadForkchoiceFinalizedNum folds two different states into 0

db/rawdb/accessors_chain.go returns 0 both for "no finalised hash stored" and "hash stored but ReadHeaderNumber cannot resolve it". The three new callers read 0 as "no finality, fall back to MaxReorgDepth", which is the safe direction but silently. unwindIfNeeded treats the very same condition — finalised hash set, header number unresolvable — as ExecutionStatusInvalidForkchoice. One hard rejection and three invisible fallbacks off one predicate is worth separating.

4. Removed coverage

TestEngineApiUnwindBeyondRetainedChangesetsRejectedCleanly is deleted. It pinned that an unwind below what is still unwindable fails loudly instead of silently applying in part. Both new tests in exec_module_non_finality_test.go run with a finalised hash set, so the no-finality path — where changesets are still pruned at ForwardProgress - MaxReorgDepth and a deep FCU has nothing to unwind with — is now uncovered, in the PR that rewires that pruning. If the old assertion no longer holds, worth saying what replaces it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Extends reorg support during non-finality by retaining execution and snapshot data back to the finalized block.

Changes:

  • Uses finality as the pruning and retirement boundary.
  • Rejects reorgs crossing finalized history.
  • Adds retirement events, deterministic fork generation, import handling, and non-finality tests.

Reviewed changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
node/shards/events.go Adds state-retirement events.
node/shards/events_test.go Tests retirement notifications.
node/eth/backend.go Configures state reorg depth.
execution/tests/state_database_test.go Migrates test DB options.
execution/tests/blockgen/chain_makers.go Makes beacon roots deterministic.
execution/state/state_test.go Migrates test DB options.
execution/stagedsync/stageloop/stageloop.go Exposes retirement hooks.
execution/stagedsync/stage_execute.go Makes pruning finality-aware.
execution/stagedsync/stage_execute_unwind_test.go Migrates test DB options.
execution/stagedsync/stage_execute_unwind_routing_test.go Migrates test DB options.
execution/stagedsync/stage_execute_resume_test.go Migrates test DB options.
execution/stagedsync/exec3_parallel_test.go Migrates test DB options.
execution/stagedsync/committer_step_boundary_test.go Migrates test DB options.
execution/execmodule/forkchoice.go Enforces finality and emits retirement events.
execution/execmodule/execmoduletester/options.go Adds forkchoice test options.
execution/execmodule/execmoduletester/exec_module_tester.go Extends finality and retirement testing.
execution/execmodule/exec_module_test.go Tests deterministic divergent forks.
execution/execmodule/exec_module_non_finality_test.go Tests long-reorg behavior.
execution/exec/txtask_test.go Migrates test DB options.
execution/engineapi/engine_api_state_churn_prune_test.go Removes obsolete deep-unwind test.
execution/engineapi/engine_api_state_churn_files_test.go Updates reorg test configuration.
db/test/domains_restart_test.go Migrates test DB options.
db/test/domain_shared_bench_test.go Migrates benchmark DB options.
db/state/aggregator.go Gates state retirement by finality.
db/state/aggregator_bench_test.go Migrates benchmark DB options.
db/snapshotsync/freezeblocks/block_snapshots.go Caps block retirement at finality.
db/rawdb/accessors_chain.go Adds finalized-height lookup.
db/kv/temporal/temporaltest/kv_temporal_testdb.go Adds configurable test DB options.
db/kv/membatchwithdb/memory_mutation_test.go Migrates test DB options.
db/integrity/commitment_version_integration_test.go Migrates test DB options.
db/integrity/commitment_state_verify_test.go Migrates test DB options.
cmd/utils/app/import_reorg_test.go Verifies imported finality metadata.
cmd/utils/app/import_cmd.go Preserves and finalizes import forkchoice state.
cmd/integration/commands/stages.go Adapts aggregation return values.
cmd/integration/commands/dump_state_test.go Migrates test DB options.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

return nil, err
}
for !isCanonicalHash {
if currentParentNumber < finalisedBlockNum {
Comment on lines +288 to +297
func ReadForkchoiceFinalizedNum(db kv.Getter) uint64 {
h := ReadForkchoiceFinalized(db)
if h == (common.Hash{}) {
return 0
}
n := ReadHeaderNumber(db, h)
if n == nil {
return 0
}
return *n
@taratorio

Copy link
Copy Markdown
Member Author

Reviewed the current head. Bugs only. Two of these are the same root cause: finalisedBlockNum replaces a bound that used to be derived from local progress, but nothing caps it by local progress — and the finalised hash outlives the state it is being compared against, because kv.LastForkchoice survives restarts, unwinds and re-execution.

  1. can't happen - invalid fork choice
  2. can't happen - invalid fork choice
  3. not an issue - we fall back to max reorg depth in the callers of this func if finalisedBlockNum is 0
  4. the test was invalid

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[EL] max reorg depth and changesets may not be enough in non-finality conditions

3 participants