Summary
Follow-up to #21847 (and #21824). The in-RAM overlay prune in execution unwind keeps one extra txNum — the first (system) txNum of the first re-executed block — because both unwind paths pass Min(<first re-executed block>) to SharedDomains.Unwind, while TemporalMemBatch.Unwind keeps entries with txNum <= unwindToTxNum. The boundary should be the last committed txNum so that all re-executed blocks' overlay writes are dropped, and the two unwind paths should share a single boundary.
Surfaced by the Copilot review on #21847 (the second review batch, against stage_execute.go:523 and the regression test).
Detail
TemporalMemBatch.Unwind keeps e.txNum <= unwindToTxNum (db/state/temporal_mem_batch.go:541):
if e.txNum <= unwindToTxNum { kept = append(kept, e) }Both unwind paths compute the boundary as Min(<first re-executed block>) — i.e. that block's first/system txNum:
In both cases Min(resumeBlock) is the first system txNum of a block that gets re-executed, and the <= keep-condition leaves that single overlay entry in place.
Why this is hardening, not a live bug
The leaked entry has not caused observable issues (the disk-path convention has carried it since #20625), because:
- Re-execution shadows it — the re-executed block re-writes the same key at the same txNum;
getLatest returns the last-appended entry, so every downstream (higher-txNum) reader sees the fresh value.
- The retry reproduces the identical value — the no-op path retries the same canonical block, so its system tx writes the same bytes (
V_stale == V_new).
- System-tx writes are unconditional (EIP-4788 / EIP-2935 ring buffers) — no gas-sensitive
SLOAD-before-SSTORE, and they don't feed the user-tx gasUsed that triggers the unwind.
The only theoretical victim is a block-begin system op doing a gas-sensitive read-before-write of a slot it itself wrote in the same (failed) execution — which doesn't occur today. The point of this issue is to make the boundary enforce the invariant ("drop everything that will be re-executed") instead of relying on those three properties holding for every future system-level operation.
The inconsistency to resolve
Today both paths use Min(resumeBlock) — consistent with each other, but both leak the boundary txNum. The Copilot review on #21847 suggests tightening only the overlay-only path to Max(s.BlockNumber); doing that in isolation makes the two paths diverge (overlay-only = last committed txNum, disk = first re-executed txNum). So the resolution should pick one convention and apply it to both.
Proposed fix (recommended)
Use the last committed txNum as the prune boundary on both paths, factored into a single shared helper so they can't drift again:
- Overlay-only:
TxnumReader().Max(s.BlockNumber) (= Min(s.BlockNumber+1) - 1).
- Disk:
TxnumReader().Max(u.UnwindPoint) for the sd.Unwind boundary (re-execution still resumes at u.UnwindPoint+1).
TxNumsReader.Max already exists (db/kv/rawdbv3/txnum.go:204). Max(committedBlock) is also marginally more robust than Min(resumeBlock) — it references only the committed block, which is always present in the txnum index, whereas resumeBlock need not be.
Care on the disk path: txUnwindTo also feeds sd.SetTxNum(txUnwindTo) (stage_execute.go:343) and the non-nil-changeset unwindChangeset fallback inside TemporalMemBatch.Unwind. Verify both still behave with the tightened value; SetTxNum is overwritten by the trailing SeekCommitment regardless, and the existing TestSharedDomain_Unwind* regression tests cover the diffset fallback.
Acceptance criteria
- Both unwind paths prune the overlay to the last committed txNum, via one shared boundary.
TestUnwindExecutionStage_PrunesUncommittedOverlayWrite gains a case: an overlay write at exactly Min(committedBlock+1) (the first system tx of the first re-executed block) is pruned, while a write at/below the last committed txNum survives.
Links
Summary
Follow-up to #21847 (and #21824). The in-RAM overlay prune in execution unwind keeps one extra txNum — the first (system) txNum of the first re-executed block — because both unwind paths pass
Min(<first re-executed block>)toSharedDomains.Unwind, whileTemporalMemBatch.Unwindkeeps entries withtxNum <= unwindToTxNum. The boundary should be the last committed txNum so that all re-executed blocks' overlay writes are dropped, and the two unwind paths should share a single boundary.Surfaced by the Copilot review on #21847 (the second review batch, against
stage_execute.go:523and the regression test).Detail
TemporalMemBatch.Unwindkeepse.txNum <= unwindToTxNum(db/state/temporal_mem_batch.go:541):Both unwind paths compute the boundary as
Min(<first re-executed block>)— i.e. that block's first/system txNum:unwindExec3(stage_execute.go:155) computestxNum = TxnumReader().Min(u.UnwindPoint+1), passed tounwindExec3State→sd.Unwind(txUnwindTo)(stage_execute.go:342). Re-execution resumes atu.UnwindPoint+1.UnwindExecutionStage(added in execution/stagedsync: prune in-RAM overlay when execution unwind is a disk no-op #21824, refined in [r3.4] execution/stagedsync: prune overlay to committed boundary on no-op execution unwind #21847) usesMin(s.BlockNumber+1). Re-execution resumes ats.BlockNumber+1.In both cases
Min(resumeBlock)is the first system txNum of a block that gets re-executed, and the<=keep-condition leaves that single overlay entry in place.Why this is hardening, not a live bug
The leaked entry has not caused observable issues (the disk-path convention has carried it since #20625), because:
getLatestreturns the last-appended entry, so every downstream (higher-txNum) reader sees the fresh value.V_stale == V_new).SLOAD-before-SSTORE, and they don't feed the user-txgasUsedthat triggers the unwind.The only theoretical victim is a block-begin system op doing a gas-sensitive read-before-write of a slot it itself wrote in the same (failed) execution — which doesn't occur today. The point of this issue is to make the boundary enforce the invariant ("drop everything that will be re-executed") instead of relying on those three properties holding for every future system-level operation.
The inconsistency to resolve
Today both paths use
Min(resumeBlock)— consistent with each other, but both leak the boundary txNum. The Copilot review on #21847 suggests tightening only the overlay-only path toMax(s.BlockNumber); doing that in isolation makes the two paths diverge (overlay-only = last committed txNum, disk = first re-executed txNum). So the resolution should pick one convention and apply it to both.Proposed fix (recommended)
Use the last committed txNum as the prune boundary on both paths, factored into a single shared helper so they can't drift again:
TxnumReader().Max(s.BlockNumber)(=Min(s.BlockNumber+1) - 1).TxnumReader().Max(u.UnwindPoint)for thesd.Unwindboundary (re-execution still resumes atu.UnwindPoint+1).TxNumsReader.Maxalready exists (db/kv/rawdbv3/txnum.go:204).Max(committedBlock)is also marginally more robust thanMin(resumeBlock)— it references only the committed block, which is always present in the txnum index, whereasresumeBlockneed not be.Care on the disk path:
txUnwindToalso feedssd.SetTxNum(txUnwindTo)(stage_execute.go:343) and the non-nil-changesetunwindChangesetfallback insideTemporalMemBatch.Unwind. Verify both still behave with the tightened value;SetTxNumis overwritten by the trailingSeekCommitmentregardless, and the existingTestSharedDomain_Unwind*regression tests cover the diffset fallback.Acceptance criteria
TestUnwindExecutionStage_PrunesUncommittedOverlayWritegains a case: an overlay write at exactlyMin(committedBlock+1)(the first system tx of the first re-executed block) is pruned, while a write at/below the last committed txNum survives.Links
TemporalMemBatch.Unwind).gas used mismatch in 3.4.