diff --git a/db/state/execctx/domain_shared.go b/db/state/execctx/domain_shared.go index 3fbc856fbfd..e06e2c6273c 100644 --- a/db/state/execctx/domain_shared.go +++ b/db/state/execctx/domain_shared.go @@ -23,6 +23,7 @@ import ( "fmt" "math" "runtime" + "sync" "sync/atomic" "time" @@ -121,6 +122,42 @@ type SharedDomains struct { // stateCache is an optional cache for state data (accounts, storage, code) stateCache *cache.StateCache + + // changesetMu guards the global current-changeset-accumulator pointer + // against concurrent mutation while other writers are recording into it. + // + // Why this exists (the layering violation we are NOT fixing here): + // + // The "current accumulator" is unwind-side machinery: a sidecar that + // records per-block prev-value diffs so a later unwind can reconstruct + // the pre-block state. Execution should be forward-only and not be + // concerned with it at all. The proper architecture is to ignore the + // accumulator during execution and derive the per-block changesets + // post-hoc from sd entries (which are now tx-granular) at sd.Flush time. + // That decoupling is a larger refactor than this PR is taking on. + // + // The acute symptom that forces this band-aid: the parallel commitment + // calculator briefly swaps the global accumulator pointer to route its + // own per-block branch writes into block N's saved changeset (see + // committer.go computeWithBlockAccumulator). During that swap window, + // the apply goroutine continues calling DomainPut for block N+1's + // account/storage writes, and those land in block N's CS instead of + // block N+1's. On a later unwind, block N+1's CS lacks the prev-value + // for those writes and the executor reads stale state, producing wrong + // trie roots in reorg/fork tests (TestBlockchainHeaderchainReorgConsistency + // + the off-by-one cluster). + // + // Until the architectural fix lands, serialize the swap window: the + // calculator takes Lock around its swap+compute+restore, and DomainPut + // / DomainDel take Lock during the brief window they record into the + // accumulator. Functionally correct; perf-suboptimal. + // + // PERF FOLLOW-UP DRIVER: this lock is the concrete reason to move the + // accumulator out of the execution path. The goal is lock-free + // execution: derive per-block changesets post-hoc from sd entries + // (now tx-granular) at sd.Flush time, and delete this Mutex + the + // SetChangesetAccumulator/GetChangesetAccumulator API entirely. + changesetMu sync.Mutex } func NewSharedDomains(ctx context.Context, tx kv.TemporalTx, logger log.Logger) (*SharedDomains, error) { @@ -190,6 +227,12 @@ type changesetSwitcher interface { // GetChangesetByBlockNum returns the changeset for a given block number and // the block hash it is keyed under. GetChangesetByBlockNum(blockNumber uint64) (common.Hash, *changeset.StateChangeSet) + // GetChangesetByHash returns the changeset saved under (blockNumber, blockHash). + // Use in preference to GetChangesetByBlockNum when both are known — + // pastChangesAccumulator can hold multiple changesets per block number after + // a fork-bounce reorg, and number-only lookups are non-deterministic in that + // scenario. + GetChangesetByHash(blockNumber uint64, blockHash common.Hash) *changeset.StateChangeSet GetChangesetAccumulator() *changeset.StateChangeSet SetChangesetAccumulator(acc *changeset.StateChangeSet) SavePastChangesetAccumulator(blockHash common.Hash, blockNumber uint64, acc *changeset.StateChangeSet) @@ -232,7 +275,30 @@ func (sd *SharedDomains) ResetPendingUpdates() { // FlushPendingUpdates applies the pending deferred commitment update. // It sets the corresponding block's changeset as the accumulator // so writes go directly to the correct changeset. +// +// Concurrency contract: the inner swap (set cs_N → apply → restore prev) +// mutates the global accumulator pointer and per-domain diff fields that +// the apply goroutine's DomainPut/DomainDel writes through. Calls from +// inside the calculator's outer LockChangesetAccumulator window must hold +// that same Mutex; calls from end-of-stage Flush are single-threaded +// against apply but still need the lock for race-detector happens-before +// against any concurrent reads via DomainPut. Caller passes +// `lockHeld=true` when it already holds changesetMu (calc path); +// `false` when FlushPendingUpdates should acquire it itself +// (Flush / standalone callers). func (sd *SharedDomains) FlushPendingUpdates(ctx context.Context, tx kv.TemporalTx) error { + return sd.flushPendingUpdates(ctx, tx, false) +} + +// FlushPendingUpdatesLocked is the variant for callers that already hold +// changesetMu via LockChangesetAccumulator (the parallel calculator's +// per-block compute window). The public FlushPendingUpdates above +// acquires the lock itself. +func (sd *SharedDomains) FlushPendingUpdatesLocked(ctx context.Context, tx kv.TemporalTx) error { + return sd.flushPendingUpdates(ctx, tx, true) +} + +func (sd *SharedDomains) flushPendingUpdates(ctx context.Context, tx kv.TemporalTx, lockHeld bool) error { upd := sd.sdCtx.TakePendingUpdate() if upd == nil { return nil @@ -240,7 +306,16 @@ func (sd *SharedDomains) FlushPendingUpdates(ctx context.Context, tx kv.Temporal defer upd.Clear() putBranch := func(prefix, data, prevData []byte) error { - return sd.DomainPut(kv.CommitmentDomain, tx, prefix, data, upd.TxNum, prevData) + // Use the unlocked variant — we either hold the lock externally + // (lockHeld=true) or inside this function (locked below). Using + // the public DomainPut would re-acquire and self-deadlock for + // commitment-domain writes if the lock is held externally. + return sd.domainPutNoLock(kv.CommitmentDomain, tx, prefix, data, upd.TxNum, prevData) + } + + if !lockHeld { + sd.changesetMu.Lock() + defer sd.changesetMu.Unlock() } switcher, ok := sd.mem.(changesetSwitcher) @@ -249,11 +324,25 @@ func (sd *SharedDomains) FlushPendingUpdates(ctx context.Context, tx kv.Temporal return err } - blockHash, cs := switcher.GetChangesetByBlockNum(upd.BlockNum) + // Hash-aware lookup when the pending update carries a BlockHash. This + // disambiguates pastChangesAccumulator entries when multiple changesets + // exist for the same block number (canonical + fork during a reorg-bounce). + // Falls back to the legacy number-only lookup if the hash isn't set + // (zero hash) — preserves behavior for callers that don't yet thread + // the hash through. + var blockHash common.Hash + var cs *changeset.StateChangeSet + if upd.BlockHash != (common.Hash{}) { + blockHash = upd.BlockHash + cs = switcher.GetChangesetByHash(upd.BlockNum, blockHash) + } else { + blockHash, cs = switcher.GetChangesetByBlockNum(upd.BlockNum) + } if cs != nil { // Save current accumulator, switch to the pending update's block // changeset, apply deferred branch writes, save it back, then - // restore the original accumulator. + // restore the original accumulator. All accesses under + // changesetMu — see concurrency contract on the wrappers above. prev := switcher.GetChangesetAccumulator() switcher.SetChangesetAccumulator(cs) @@ -272,6 +361,20 @@ func (sd *SharedDomains) FlushPendingUpdates(ctx context.Context, tx kv.Temporal return err } +// domainPutNoLock is the lock-held variant of DomainPut for callers +// (FlushPendingUpdates) that already hold changesetMu externally. It +// shares DomainPut's body via domainPut(..., lockHeld=true). +// +// Today DomainPut(kv.CommitmentDomain, ...) happens to skip the lock +// anyway (see the CommitmentDomain exemption in domainPut), so calling +// DomainPut directly from FlushPendingUpdates wouldn't deadlock on the +// current code path. This variant is defensive: it stays correct even if +// a future change removes that exemption (e.g. the lock-free refactor in +// #21106 reshapes how CommitmentDomain writes are routed). +func (sd *SharedDomains) domainPutNoLock(domain kv.Domain, roTx kv.TemporalTx, k, v []byte, txNum uint64, prevVal []byte) error { + return sd.domainPut(domain, roTx, k, v, txNum, prevVal, true) +} + type temporalGetter struct { sd *SharedDomains tx kv.TemporalTx @@ -306,10 +409,89 @@ func (sd *SharedDomains) AsGetter(tx kv.TemporalTx) kv.TemporalGetter { return &temporalGetter{sd, tx} } +// LockChangesetAccumulator and UnlockChangesetAccumulator bracket a +// swap+use+restore sequence on the global accumulator pointer (see +// changesetMu doc on the SharedDomains struct for the layering rationale). +// Apply-side DomainPut/DomainDel take the same lock briefly so they +// cannot record into a swapped accumulator that does not belong to the +// block they are writing for. +// +// Holders MUST pair Lock with Unlock and MUST keep the critical section +// short — currently the calculator's per-block ComputeCommitment runs +// inside this lock, which serializes apply-side writes for the duration +// of compute. That cost goes away once the post-hoc-from-sd-entries +// derivation lands and this lock + the swap dance can both be deleted. +// +// Inside the locked window callers must use the *Locked variants +// (Set/GetChangesetAccumulatorLocked) — the public Set/Get acquire the +// same Mutex and would self-deadlock. +func (sd *SharedDomains) LockChangesetAccumulator() { sd.changesetMu.Lock() } +func (sd *SharedDomains) UnlockChangesetAccumulator() { sd.changesetMu.Unlock() } + +// SetChangesetAccumulator installs the given accumulator as the global +// "current" target for DomainPut/DomainDel diff recording. Locks +// changesetMu internally for the brief write — concurrent apply/calc +// paths cannot torn-write or torn-read this pointer. func (sd *SharedDomains) SetChangesetAccumulator(acc *changeset.StateChangeSet) { + sd.changesetMu.Lock() + sd.mem.(accHolder).SetChangesetAccumulator(acc) + sd.changesetMu.Unlock() +} + +// SetChangesetAccumulatorLocked is the unlocked variant for callers that +// already hold changesetMu via LockChangesetAccumulator (the calculator's +// per-block compute window). +func (sd *SharedDomains) SetChangesetAccumulatorLocked(acc *changeset.StateChangeSet) { sd.mem.(accHolder).SetChangesetAccumulator(acc) } +// GetChangesetAccumulator returns the currently-installed live changeset +// accumulator (the one DomainPut writes diff entries into). Returns nil if +// none is installed. Locks changesetMu internally — must NOT be called +// while already holding the lock (use GetChangesetAccumulatorLocked). +func (sd *SharedDomains) GetChangesetAccumulator() *changeset.StateChangeSet { + sd.changesetMu.Lock() + defer sd.changesetMu.Unlock() + if h, ok := sd.mem.(changesetSwitcher); ok { + return h.GetChangesetAccumulator() + } + return nil +} + +// GetChangesetAccumulatorLocked is the unlocked variant for callers that +// already hold changesetMu. +func (sd *SharedDomains) GetChangesetAccumulatorLocked() *changeset.StateChangeSet { + if h, ok := sd.mem.(changesetSwitcher); ok { + return h.GetChangesetAccumulator() + } + return nil +} + +// GetChangesetByBlockNum returns the saved changeset for a given block +// number (and the block hash it was saved under), or (zero hash, nil) if +// no such changeset has been saved via SavePastChangesetAccumulator. +// +// WARNING: ambiguous when pastChangesAccumulator holds multiple changesets +// for the same block number (e.g. canonical + fork during a reorg-bounce). +// Prefer GetChangesetByHash when the caller has the block hash available. +func (sd *SharedDomains) GetChangesetByBlockNum(blockNumber uint64) (common.Hash, *changeset.StateChangeSet) { + if h, ok := sd.mem.(changesetSwitcher); ok { + return h.GetChangesetByBlockNum(blockNumber) + } + return common.Hash{}, nil +} + +// GetChangesetByHash returns the saved changeset for an exact (blockNumber, +// blockHash) key, or nil if not found. Use this when the caller knows both — +// pastChangesAccumulator can hold multiple changesets per block number after +// a fork-bounce reorg, and number-only lookups are non-deterministic. +func (sd *SharedDomains) GetChangesetByHash(blockNumber uint64, blockHash common.Hash) *changeset.StateChangeSet { + if h, ok := sd.mem.(changesetSwitcher); ok { + return h.GetChangesetByHash(blockNumber, blockHash) + } + return nil +} + func (sd *SharedDomains) SavePastChangesetAccumulator(blockHash common.Hash, blockNumber uint64, acc *changeset.StateChangeSet) { sd.mem.(accHolder).SavePastChangesetAccumulator(blockHash, blockNumber, acc) } @@ -632,6 +814,14 @@ func (sd *SharedDomains) GetAsOf(domain kv.Domain, key []byte, ts uint64) (v []b // - user can append k2 into k1, then underlying methods will not preform append // - if `val == nil` it will call DomainDel func (sd *SharedDomains) DomainPut(domain kv.Domain, roTx kv.TemporalTx, k, v []byte, txNum uint64, prevVal []byte) error { + return sd.domainPut(domain, roTx, k, v, txNum, prevVal, false) +} + +// domainPut is the shared body for DomainPut (lockHeld=false) and +// domainPutNoLock (lockHeld=true). Factored so a new domain case or +// pre-check is written once. See changesetMu doc on the SharedDomains +// struct for the locking rationale. +func (sd *SharedDomains) domainPut(domain kv.Domain, roTx kv.TemporalTx, k, v []byte, txNum uint64, prevVal []byte, lockHeld bool) error { if v == nil { return fmt.Errorf("DomainPut: %s, trying to put nil value. not allowed", domain) } @@ -664,6 +854,18 @@ func (sd *SharedDomains) DomainPut(domain kv.Domain, roTx kv.TemporalTx, k, v [] sd.stateCache.Put(domain, k, v) } + // Serialize against the calculator's accumulator-swap window — see + // changesetMu doc on the SharedDomains struct. Skipped when the caller + // already holds changesetMu (lockHeld=true, the FlushPendingUpdates + // path), and currently also for CommitmentDomain — those writes + // originate exclusively from the calculator's compute, which holds + // changesetMu via LockChangesetAccumulator (re-acquiring would + // self-deadlock). All other domains are written by the apply goroutine + // and need to serialize against the swap. + if !lockHeld && domain != kv.CommitmentDomain { + sd.changesetMu.Lock() + defer sd.changesetMu.Unlock() + } return sd.mem.DomainPut(domain, ks, v, txNum, prevVal) } @@ -699,6 +901,9 @@ func (sd *SharedDomains) DomainDel(domain kv.Domain, tx kv.TemporalTx, k []byte, sd.stateCache.Delete(kv.AccountsDomain, k) sd.stateCache.Delete(kv.CodeDomain, k) } + // AccountsDomain — apply-side. Serialize against swap window. + sd.changesetMu.Lock() + defer sd.changesetMu.Unlock() return sd.mem.DomainDel(kv.AccountsDomain, ks, txNum, prevVal) case kv.StorageDomain: // Remove from state cache when storage is deleted @@ -716,6 +921,12 @@ func (sd *SharedDomains) DomainDel(domain kv.Domain, tx kv.TemporalTx, k []byte, default: //noop } + // Serialize against the calculator's swap window for non-commitment + // domains; CommitmentDomain skipped — see DomainPut comment. + if domain != kv.CommitmentDomain { + sd.changesetMu.Lock() + defer sd.changesetMu.Unlock() + } return sd.mem.DomainDel(domain, ks, txNum, prevVal) } @@ -783,11 +994,34 @@ func (sd *SharedDomains) SeekCommitment(ctx context.Context, tx kv.TemporalTx) ( // ComputeCommitment evaluates commitment for gathered updates. // If trieWarmup toggle was enabled via EnableTrieWarmup, pre-warms MDBX page cache by reading Branch data in parallel before processing. func (sd *SharedDomains) ComputeCommitment(ctx context.Context, tx kv.TemporalTx, saveStateAfter bool, blockNum, txNum uint64, logPrefix string, onProgress func(*commitment.CommitProgress)) (rootHash []byte, err error) { + return sd.computeCommitment(ctx, tx, saveStateAfter, blockNum, txNum, logPrefix, onProgress, false) +} + +// ComputeCommitmentLocked is the variant for callers (the parallel +// commitment calculator) that already hold changesetMu via +// LockChangesetAccumulator. The pending-updates flush uses the *Locked +// internal path so it doesn't self-deadlock on the outer mutex. +// +// Routes the deferred branch writes from the previous block into the +// correct block's changeset (via the hash-aware lookup in +// FlushPendingUpdatesLocked) without releasing the calculator's outer +// lock — closing the SetChangesetAccumulator-vs-SetChangesetAccumulator +// races between calc-internal swap and the apply-side SetChangesetAccumulator. +func (sd *SharedDomains) ComputeCommitmentLocked(ctx context.Context, tx kv.TemporalTx, saveStateAfter bool, blockNum, txNum uint64, logPrefix string, onProgress func(*commitment.CommitProgress)) (rootHash []byte, err error) { + return sd.computeCommitment(ctx, tx, saveStateAfter, blockNum, txNum, logPrefix, onProgress, true) +} + +func (sd *SharedDomains) computeCommitment(ctx context.Context, tx kv.TemporalTx, saveStateAfter bool, blockNum, txNum uint64, logPrefix string, onProgress func(*commitment.CommitProgress), lockHeld bool) (rootHash []byte, err error) { // Flush any pending deferred commitment updates from the previous block - // into the CORRECT block's changeset (via FlushPendingUpdates which uses - // GetChangesetByBlockNum). This ensures the branch writes are recorded in + // into the CORRECT block's changeset (via the hash-aware lookup in + // FlushPendingUpdates). This ensures the branch writes are recorded in // the original block's diffset so they can be properly reverted on unwind. - if err := sd.FlushPendingUpdates(ctx, tx); err != nil { + if lockHeld { + err = sd.FlushPendingUpdatesLocked(ctx, tx) + } else { + err = sd.FlushPendingUpdates(ctx, tx) + } + if err != nil { return nil, err } return sd.sdCtx.ComputeCommitment(ctx, tx, saveStateAfter, blockNum, txNum, logPrefix, onProgress) diff --git a/db/state/temporal_mem_batch.go b/db/state/temporal_mem_batch.go index 0aef35e42ea..863d5f61fdd 100644 --- a/db/state/temporal_mem_batch.go +++ b/db/state/temporal_mem_batch.go @@ -72,7 +72,13 @@ type TemporalMemBatch struct { pastForkableWriters map[kv.ForkableId][]kv.BufferedWriter currentChangesAccumulator *changeset.StateChangeSet - pastChangesAccumulator map[string]*changeset.StateChangeSet + // pastChangesAccumulator is read by the parallel commitment calculator + // goroutine (via SharedDomains.GetChangesetByBlockNum) while the exec + // loop writes to it (via SavePastChangesetAccumulator). pastChangesLock + // serializes those accesses so map-iteration during GetChangesetByBlockNum + // doesn't race with map-write during SavePastChangesetAccumulator. + pastChangesLock sync.RWMutex + pastChangesAccumulator map[string]*changeset.StateChangeSet unwindToTxNum uint64 // unwindChangeset is keyed by the pre-step portion of each entry's Key @@ -396,6 +402,8 @@ func (sd *TemporalMemBatch) SetChangesetAccumulator(acc *changeset.StateChangeSe } } func (sd *TemporalMemBatch) SavePastChangesetAccumulator(blockHash common.Hash, blockNumber uint64, acc *changeset.StateChangeSet) { + sd.pastChangesLock.Lock() + defer sd.pastChangesLock.Unlock() if sd.pastChangesAccumulator == nil { sd.pastChangesAccumulator = make(map[string]*changeset.StateChangeSet) } @@ -406,7 +414,14 @@ func (sd *TemporalMemBatch) SavePastChangesetAccumulator(blockHash common.Hash, } // GetChangesetByBlockNum returns the changeset for a given block number and its block hash. +// +// WARNING: ambiguous when pastChangesAccumulator holds multiple changesets for +// the same block number (e.g. canonical + fork during a reorg-bounce test). +// The first match in non-deterministic map iteration order is returned. +// Prefer GetChangesetByHash when the caller has the block hash available. func (sd *TemporalMemBatch) GetChangesetByBlockNum(blockNumber uint64) (common.Hash, *changeset.StateChangeSet) { + sd.pastChangesLock.RLock() + defer sd.pastChangesLock.RUnlock() for key, cs := range sd.pastChangesAccumulator { keyBytes := common.ToBytesZeroCopy(key) if binary.BigEndian.Uint64(keyBytes[:8]) == blockNumber { @@ -417,16 +432,34 @@ func (sd *TemporalMemBatch) GetChangesetByBlockNum(blockNumber uint64) (common.H return common.Hash{}, nil } +// GetChangesetByHash returns the changeset saved under the exact (blockNumber, +// blockHash) key. Returns nil if not found. Use this in preference to +// GetChangesetByBlockNum when both pieces of information are known — +// pastChangesAccumulator can hold multiple changesets per block number after +// a fork-bounce, and number-only lookups can return the wrong one +// non-deterministically. +func (sd *TemporalMemBatch) GetChangesetByHash(blockNumber uint64, blockHash common.Hash) *changeset.StateChangeSet { + var key [40]byte + binary.BigEndian.PutUint64(key[:8], blockNumber) + copy(key[8:], blockHash[:]) + sd.pastChangesLock.RLock() + defer sd.pastChangesLock.RUnlock() + return sd.pastChangesAccumulator[common.ToStringZeroCopy(key[:])] +} + func (sd *TemporalMemBatch) GetDiffset(tx kv.RwTx, blockHash common.Hash, blockNumber uint64) ([kv.DomainLen][]kv.DomainEntryDiff, bool, error) { var key [40]byte binary.BigEndian.PutUint64(key[:8], blockNumber) copy(key[8:], blockHash[:]) - if changeset, ok := sd.pastChangesAccumulator[common.ToStringZeroCopy(key[:])]; ok { + sd.pastChangesLock.RLock() + cs, ok := sd.pastChangesAccumulator[common.ToStringZeroCopy(key[:])] + sd.pastChangesLock.RUnlock() + if ok { return [kv.DomainLen][]kv.DomainEntryDiff{ - changeset.Diffs[kv.AccountsDomain].GetDiffSet(), - changeset.Diffs[kv.StorageDomain].GetDiffSet(), - changeset.Diffs[kv.CodeDomain].GetDiffSet(), - changeset.Diffs[kv.CommitmentDomain].GetDiffSet(), + cs.Diffs[kv.AccountsDomain].GetDiffSet(), + cs.Diffs[kv.StorageDomain].GetDiffSet(), + cs.Diffs[kv.CodeDomain].GetDiffSet(), + cs.Diffs[kv.CommitmentDomain].GetDiffSet(), }, true, nil } return changeset.ReadDiffSet(tx, blockNumber, blockHash) @@ -607,12 +640,22 @@ func (sd *TemporalMemBatch) Merge(o kv.TemporalMemBatch) error { return fmt.Errorf("can't merge from batch with non-nil currentChangesAccumulator") } + // Fixed lock order (receiver write-lock, then `other` read-lock). + // Callers must not interleave reciprocal Merge calls — i.e. never run + // a.Merge(b) and b.Merge(a) concurrently, which would deadlock here. + // In practice Merge runs single-threaded at stage commit / batch + // rollup, so this is a documented assumption rather than an enforced + // invariant. + sd.pastChangesLock.Lock() + other.pastChangesLock.RLock() for key, changeSet := range other.pastChangesAccumulator { if sd.pastChangesAccumulator == nil { sd.pastChangesAccumulator = map[string]*changeset.StateChangeSet{} } sd.pastChangesAccumulator[key] = changeSet } + other.pastChangesLock.RUnlock() + sd.pastChangesLock.Unlock() if other.unwindChangeset != nil { if sd.unwindChangeset == nil { diff --git a/execution/commitment/commitment.go b/execution/commitment/commitment.go index f1adc2bc243..ee730aee321 100644 --- a/execution/commitment/commitment.go +++ b/execution/commitment/commitment.go @@ -319,8 +319,16 @@ func putDeferredUpdate(upd *DeferredBranchUpdate) { // Used by the commitment context to defer branch update application until a later flush. type PendingCommitmentUpdate struct { BlockNum uint64 - TxNum uint64 - Deferred []*DeferredBranchUpdate + // BlockHash is the hash of the block these deferred updates were + // generated for. It disambiguates pastChangesAccumulator lookups when + // multiple changesets exist for the same block number (e.g. during a + // fork-bounce reorg test where canonical and fork chains both saved a + // block 1 changeset). Without the hash, GetChangesetByBlockNum returns + // the first match it iterates — non-deterministic and wrong in that + // scenario. + BlockHash common.Hash + TxNum uint64 + Deferred []*DeferredBranchUpdate } // Clear returns all deferred updates to the pool and nils the slice. diff --git a/execution/commitment/commitmentdb/commitment_context.go b/execution/commitment/commitmentdb/commitment_context.go index 8a241d2cc48..36836bd009b 100644 --- a/execution/commitment/commitmentdb/commitment_context.go +++ b/execution/commitment/commitmentdb/commitment_context.go @@ -103,6 +103,15 @@ func (sdc *SharedDomainsCommitmentContext) SetPendingUpdate(upd *commitment.Pend sdc.pendingUpdate = upd } +// PeekPendingUpdate returns the current pending update without taking ownership. +// Returns nil if no pending update is set. Used by the parallel commitment +// calculator to annotate the pending update with the block hash after a +// per-block ComputeCommitment, so the next FlushPendingUpdates can route to +// the exact (BlockNum, BlockHash) past changeset. +func (sdc *SharedDomainsCommitmentContext) PeekPendingUpdate() *commitment.PendingCommitmentUpdate { + return sdc.pendingUpdate +} + // ResetPendingUpdates clears the pending update, returning deferred updates to the pool. func (sdc *SharedDomainsCommitmentContext) ResetPendingUpdates() { if sdc.pendingUpdate != nil { diff --git a/execution/stagedsync/calc_state.go b/execution/stagedsync/calc_state.go index 7f6cc0e0146..b0833116c28 100644 --- a/execution/stagedsync/calc_state.go +++ b/execution/stagedsync/calc_state.go @@ -170,6 +170,46 @@ func (cs *calcState) ensureStorage(addr accounts.Address, key accounts.StorageKe } // ApplyWrites updates the local state with all writes from a TX result. +// +// Two semantic invariants matter for SD-of-pre-existing-contract: +// +// (1) IBS.Selfdestruct emits three versionWritten entries (IncarnationPath +// +// = preInc, SelfDestructPath=true, BalancePath=0). Without care, the +// trailing BalancePath=0 in the writeset would clobber Deleted=true +// via the unconditional `acc.Deleted = false` reset that +// BalancePath/NoncePath/CodeHashPath/CodePath cases use to reflect +// a re-creation. Fix: those cases only clear Deleted when the value +// is non-zero/non-empty. A zero-value write that arrives after SD is +// part of the SD's own emission and must not undo Deleted=true. +// +// (2) When SelfDestructPath=true is processed, zero Balance/Nonce/ +// +// CodeHash/Incarnation. Without zeroing, lazy-loaded pre-SD values +// survive in cs.accounts and FlushToUpdates routes into the default +// regular-UPDATE branch (emitting pre-SD nonce/codeHash) instead of +// the EIP-161 DeleteUpdate branch — which is what serial's +// DomainDel produces for a pure SD (leaf removed). Storage slots +// under the SD'd address must also zero out: vm.StorageKeys only +// returns slots written in the current tx's version map and SD via +// Selfdestruct() doesn't write storage explicitly, so without +// zeroing here, FlushToUpdates emits StorageUpdate with pre-SD +// values and leaves stale storage in the trie (TestRecreateAndRewind +// block 4 recreate sees stale storage). +// +// Ordering invariant that (1) relies on: when a single tx +// self-destructs an address and then re-creates it (CREATE2 to the same +// address — pre-Cancun pattern), IBS emits the SELFDESTRUCT-time writes +// (SelfDestructPath=true, BalancePath=0, IncarnationPath=preInc) BEFORE +// the recreate-time writes (BalancePath=newBal, NoncePath=1, CodeHash=…), +// because the EVM runs the opcodes in that order and versionWritten fires +// at opcode time. So the recreate's non-zero Balance/Nonce/CodeHash +// re-clear acc.Deleted after the SD case set it. For SD-only (no recreate) +// the post-SD BalancePath=0 arrives but is zero, so it does NOT re-clear. +// For fresh creates (no prior SD in this writeset) acc.Deleted starts +// false and the conditional is a no-op. (For per-block writesets the calc +// also relies on this — but blocks aren't single txs; the SD-then-recreate +// pattern there spans txs, and last-write-wins on acc.Deleted still holds.) func (cs *calcState) ApplyWrites(writes state.VersionedWrites) { for _, w := range writes { if w.Val == nil { @@ -181,51 +221,54 @@ func (cs *calcState) ApplyWrites(writes state.VersionedWrites) { acc := cs.ensureAccount(w.Address) acc.Balance = w.Val.(uint256.Int) acc.dirty = true - acc.Deleted = false + if !acc.Balance.IsZero() { + // Only a non-zero balance reflects real recreate or + // transfer-in. Zero is part of the SD emission (see + // invariant 1) and must not clear Deleted. + acc.Deleted = false + } case state.NoncePath: acc := cs.ensureAccount(w.Address) acc.Nonce = w.Val.(uint64) acc.dirty = true - acc.Deleted = false + if acc.Nonce != 0 { + acc.Deleted = false + } case state.CodeHashPath: acc := cs.ensureAccount(w.Address) v := w.Val.(accounts.CodeHash) acc.CodeHash = v.Value() acc.dirty = true - acc.Deleted = false + if v.Value() != empty.CodeHash { + acc.Deleted = false + } case state.CodePath: acc := cs.ensureAccount(w.Address) code := w.Val.([]byte) acc.CodeHash = crypto.Keccak256Hash(code) acc.dirty = true - acc.Deleted = false + if len(code) > 0 { + acc.Deleted = false + } case state.SelfDestructPath: if destructed, ok := w.Val.(bool); ok && destructed { acc := cs.ensureAccount(w.Address) acc.Deleted = true acc.dirty = true - // Mark every tracked storage slot dirty so - // FlushToUpdates emits a per-slot update. Serial's - // MakeWriteSet for an SD account emits storage writes - // (with their pre-SD values) alongside the account- - // reset, and the trie processes them in fold order. - // - // LOAD-BEARING INVARIANT: the marked slots emit as - // DeleteUpdate (not StorageUpdate with pre-SD values) - // because normalizeWriteSet at exec3_parallel.go's - // SelfDestructPath case adds StoragePath=0 entries for - // every key in vm.StorageKeys(addr). Those zeros - // arrive in this ApplyWrites loop AFTER the - // SelfDestructPath case, overwrite cs.storageState's - // pre-SD values, and FlushToUpdates emits DeleteUpdate - // per slot. Without that loop in normalizeWriteSet - // this code would silently leak pre-SD slot values - // into the trie. + // Invariant 2: zero account fields so FlushToUpdates + // routes into the EIP-161 DeleteUpdate branch. + acc.Balance = uint256.Int{} + acc.Nonce = 0 + acc.CodeHash = empty.CodeHash + acc.Incarnation = 0 + // Zero every tracked storage slot and mark dirty so + // FlushToUpdates emits DeleteUpdate per slot. if slots, ok := cs.storageState[w.Address]; ok { if cs.storageDirty[w.Address] == nil { cs.storageDirty[w.Address] = make(map[accounts.StorageKey]bool) } for key := range slots { + slots[key] = uint256.Int{} cs.storageDirty[w.Address][key] = true } } @@ -239,14 +282,6 @@ func (cs *calcState) ApplyWrites(writes state.VersionedWrites) { } cs.storageDirty[w.Address][w.Key] = true case state.IncarnationPath: - // Carries the pre-deletion incarnation when emitted by - // LightCollector.DeleteAccount alongside SelfDestructPath=true. - // Used by FlushToUpdates to differentiate self-destruct of a - // pre-existing contract from EIP-161 emptyRemoval. Direct - // type-assertion (panic on mismatch) matches the other cases - // in this function — silently zero-ing Incarnation here would - // route a real SD into the EIP-161 DeleteUpdate branch and - // reproduce the very wrong-root bug this PR fixes. acc := cs.ensureAccount(w.Address) acc.Incarnation = w.Val.(uint64) acc.dirty = true @@ -268,14 +303,17 @@ func (cs *calcState) FlushToUpdates(updates *commitment.Updates) { // Three flavours of "Deleted" writeset, distinguished by whether // the account fields actually became zero: - // 1. SD of a pre-existing contract: SD zeroes balance (sent to - // beneficiary) and incarnation > 0 in writeset. Serial's - // DomainDel emits the post-SD encoding (zero fields, leaf - // survives because incarnation is preserved in the - // serialised account). Emit zero-account UPDATE. - // 2. EIP-161 emptyRemoval of a touched-empty EOA-shaped account: - // all fields zero, incarnation also zero. Serial's DomainDel - // emits truly empty bytes. Emit DeleteUpdate. + // 1. (Currently unreachable from production writesets — defensive + // only.) SD-of-pre-existing-contract with incarnation > 0 + // retained: ApplyWrites' SelfDestructPath case now zeros + // Incarnation along with Balance/Nonce/CodeHash, so SD always + // lands in case 2 below. This branch stays as a safety net for + // hand-built writesets and future ApplyWrites changes that + // might preserve incarnation; if it fires, emit a zero-account + // UPDATE (matches serial's post-DomainDel encoding when the + // serialised account retains a non-zero incarnation). + // 2. SD / EIP-161 emptyRemoval: all fields zero (incarnation too). + // Serial's DomainDel removes the leaf. Emit DeleteUpdate. // 3. Deleted-but-not-empty (defense-in-depth): if the writeset // has SelfDestructPath=true but balance/nonce/code retain // non-zero values (e.g. OOG-during-CREATE2 with retained diff --git a/execution/stagedsync/calc_state_test.go b/execution/stagedsync/calc_state_test.go index cea782f56df..e4184fa0012 100644 --- a/execution/stagedsync/calc_state_test.go +++ b/execution/stagedsync/calc_state_test.go @@ -216,9 +216,16 @@ func TestFlushToUpdates_LiveAccount_EmitsFullUpdate(t *testing.T) { } // TestApplyWrites_IncarnationPath verifies that an IncarnationPath write -// from the apply pipeline is captured into the calcAccountState. This is -// the channel through which the executor signals "this account had a -// non-zero pre-block incarnation" alongside SelfDestructPath=true. +// captured before SelfDestructPath does NOT survive the SD: the SD case +// zeros all account fields (Balance/Nonce/CodeHash/Incarnation) so +// FlushToUpdates routes into the EIP-161 DeleteUpdate branch, matching +// serial's DomainDel behavior of removing the leaf for a pure SD. +// +// The previous expectation (Incarnation preserved → zero-account UPDATE +// flags) was based on a misreading of serial — empirically serial emits +// DeleteUpdate for a pure SD-of-pre-existing-contract, not a zero-account +// leaf with retained incarnation. TestRecreateAndRewind (block 3 SD) +// fails under the old expectation. func TestApplyWrites_IncarnationPath(t *testing.T) { cs := newTestCalcState() addr := accounts.InternAddress([20]byte{0xc1}) @@ -231,17 +238,20 @@ func TestApplyWrites_IncarnationPath(t *testing.T) { acc, ok := cs.accounts[addr] require.True(t, ok, "ensureAccount should have created an entry") - assert.Equal(t, uint64(1), acc.Incarnation, "IncarnationPath write must populate Incarnation") assert.True(t, acc.Deleted, "SelfDestructPath=true must set Deleted") + assert.Equal(t, uint64(0), acc.Incarnation, "SelfDestructPath must zero Incarnation (matches serial's DomainDel removing the leaf)") + assert.True(t, acc.Balance.IsZero(), "SelfDestructPath must zero Balance") + assert.Equal(t, uint64(0), acc.Nonce, "SelfDestructPath must zero Nonce") + assert.Equal(t, [32]byte(empty.CodeHash), acc.CodeHash, "SelfDestructPath must reset CodeHash") updates := newTestUpdates() cs.FlushToUpdates(updates) keyVal := addr.Value() got := lookupKeyUpdate(t, updates, string(keyVal[:])) assert.Equal(t, - commitment.BalanceUpdate|commitment.NonceUpdate|commitment.CodeUpdate, + commitment.DeleteUpdate, got.Flags, - "Deleted+Incarnation>0 routes through the zero-account UPDATE branch") + "Deleted+isAllZero routes through the EIP-161 DeleteUpdate branch (matches serial's DomainDel)") } // TestApplyWrites_BalancePathClearsDeleted verifies that a non-empty @@ -309,23 +319,28 @@ func (r *preBlockReader) TracePrefix() string // the prior version of this test had an empty vm and so the completion // loop's vm.Read fallback never fired, masking the actual production flow). // -// What it locks in: -// 1. normalizeWriteSet's completion loop reads BalancePath/NoncePath/ -// CodeHashPath from vm.Read first (where IBS already wrote BalancePath=0) -// then stateReader. So in production the trie sees BalancePath=0, -// NOT preBlockBalance. -// 2. calcState.ApplyWrites ends with acc.Deleted=false (because BalancePath=0 -// write fires acc.Deleted=false in the ApplyWrites BalancePath case), -// acc.Balance=0, acc.Nonce=preBlock, acc.CodeHash=preBlock, -// acc.Incarnation=preBlock. -// 3. FlushToUpdates default branch fires; the leaf survives with -// {Balance=0, Nonce=preBlockNonce, CodeHash=preBlockCodeHash} — -// matching what serial would write through the same path. +// What it locks in (post-#21088 corrected semantics): +// 1. normalizeWriteSet detects SD'd addresses by scanning for +// SelfDestructPath=true entries up front, and DROPS the raw +// IncarnationPath / BalancePath / NoncePath / CodeHashPath / CodePath +// writes for those addresses. The completion loop also skips them. +// The normalized writeset for the SD'd address contains ONLY +// SelfDestructPath=true (plus StoragePath=0 entries from vm.StorageKeys, +// none in this scenario). Without this, applyVersionedWrites takes the +// cleanup-before-recreate branch and writes the account back with +// {Balance=0, Inc=preInc} encoding instead of taking the pure-delete +// branch (DomainDel(Accounts)). +// 2. calcState.ApplyWrites ends with acc.Deleted=true, acc.Balance=0, +// acc.Nonce=0, acc.CodeHash=empty, acc.Incarnation=0 — the +// SelfDestructPath case zeros all account fields so FlushToUpdates +// routes into the EIP-161 DeleteUpdate branch. +// 3. FlushToUpdates emits DeleteUpdate, matching serial's DomainDel +// removing the leaf for a pure SD-of-pre-existing-contract. // -// The "Deleted && Incarnation>0 && isAllZero" branch in FlushToUpdates is -// therefore confirmed unreachable from real production writesets; it's -// defensive coverage (see docstring on -// TestFlushToUpdates_DeletedWithIncarnation_EmitsZeroAccountUpdate). +// The previous expectation (default-UPDATE branch, leaf survives with +// {Balance=0, Nonce=preBlock, CodeHash=preBlock}) was based on a stale +// reading of serial; empirically serial removes the leaf, and parallel +// must do the same to produce matching trie roots in TestRecreateAndRewind. func TestSDOfPreExistingContract_FullPipeline(t *testing.T) { addr := accounts.InternAddress([20]byte{0x40, 0x55, 0xca, 0xe5}) @@ -365,9 +380,9 @@ func TestSDOfPreExistingContract_FullPipeline(t *testing.T) { stateReader := &preBlockReader{addr: addr, acc: original} normalized := normalizeWriteSet(rawWrites, vm, 0, 0, stateReader) - // Sanity: completion loop should fill the missing NoncePath / CodeHashPath - // from stateReader (vm has nothing for those), and BalancePath should - // remain 0 (vm has the IBS-written zero value). + // SD-aware filtering: only SelfDestructPath survives in the normalized + // writeset for the SD'd address. The raw IncarnationPath/BalancePath + // writes are dropped, and the completion loop skips this address. pathSeen := map[state.AccountPath]any{} for _, w := range normalized { switch w.Path { @@ -375,15 +390,16 @@ func TestSDOfPreExistingContract_FullPipeline(t *testing.T) { pathSeen[w.Path] = w.Val } } - require.Contains(t, pathSeen, state.SelfDestructPath) - require.Contains(t, pathSeen, state.IncarnationPath) - require.Contains(t, pathSeen, state.BalancePath) - require.Contains(t, pathSeen, state.NoncePath) - require.Contains(t, pathSeen, state.CodeHashPath) - assert.Equal(t, uint256.Int{}, pathSeen[state.BalancePath].(uint256.Int), - "BalancePath must be 0 (vm.Read of IBS.Selfdestruct's versionWritten BalancePath=0), NOT preBlockBalance from stateReader fallback") - assert.Equal(t, preBlockNonce, pathSeen[state.NoncePath].(uint64)) - assert.Equal(t, preBlockCodeHash, pathSeen[state.CodeHashPath].(accounts.CodeHash)) + require.Contains(t, pathSeen, state.SelfDestructPath, + "SelfDestructPath=true must survive normalize for the pure-delete branch in applyVersionedWrites") + assert.NotContains(t, pathSeen, state.IncarnationPath, + "IncarnationPath must be filtered for SD'd address — otherwise applyVersionedWrites takes cleanup-before-recreate") + assert.NotContains(t, pathSeen, state.BalancePath, + "BalancePath must be filtered for SD'd address — same reason") + assert.NotContains(t, pathSeen, state.NoncePath, + "NoncePath must not be filled by completion-loop fallback for SD'd address") + assert.NotContains(t, pathSeen, state.CodeHashPath, + "CodeHashPath must not be filled by completion-loop fallback for SD'd address") // Drive ApplyWrites + FlushToUpdates. cs := newTestCalcState() @@ -391,30 +407,24 @@ func TestSDOfPreExistingContract_FullPipeline(t *testing.T) { acc, ok := cs.accounts[addr] require.True(t, ok) - assert.False(t, acc.Deleted, - "BalancePath=0 (and Nonce/CodeHash) written after SelfDestructPath reset Deleted=false") - assert.Equal(t, uint256.Int{}, acc.Balance, - "acc.Balance is 0 (post-SD value, not preBlockBalance)") - assert.Equal(t, preBlockNonce, acc.Nonce) - assert.Equal(t, [32]byte(preBlockCodeHash.Value()), acc.CodeHash) - assert.Equal(t, preBlockIncarnation, acc.Incarnation) + assert.True(t, acc.Deleted, + "SelfDestructPath=true must set acc.Deleted=true") + assert.True(t, acc.Balance.IsZero(), + "SelfDestructPath case zeros Balance") + assert.Equal(t, uint64(0), acc.Nonce, + "SelfDestructPath case zeros Nonce") + assert.Equal(t, [32]byte(empty.CodeHash), acc.CodeHash, + "SelfDestructPath case resets CodeHash to empty") + assert.Equal(t, uint64(0), acc.Incarnation, + "SelfDestructPath case zeros Incarnation so FlushToUpdates routes through DeleteUpdate (EIP-161 branch), matching serial's DomainDel") updates := newTestUpdates() cs.FlushToUpdates(updates) got := lookupKeyUpdate(t, updates, string(addr.Value().Bytes())) - // Default branch fires (acc.Deleted=false). Trie sees a leaf with - // {Balance=0, Nonce=preBlockNonce, CodeHash=preBlockCodeHash}. - assert.Equal(t, - commitment.BalanceUpdate|commitment.NonceUpdate|commitment.CodeUpdate, - got.Flags, - "production pipeline ends in the default FlushToUpdates branch") - assert.True(t, got.Balance.IsZero(), - "trie sees Balance=0 — IBS.Selfdestruct wrote it explicitly via versionWritten") - assert.Equal(t, preBlockNonce, got.Nonce, - "trie sees pre-block nonce (no NoncePath emit from IBS, completion-loop-from-stateReader fallback)") - assert.Equal(t, common.Hash(preBlockCodeHash.Value()), got.CodeHash, - "trie sees pre-block codeHash (same fallback as Nonce)") + // EIP-161-style DeleteUpdate (matches serial's DomainDel for a pure SD). + assert.Equal(t, commitment.DeleteUpdate, got.Flags, + "production pipeline ends in the EIP-161 DeleteUpdate branch (Deleted+isAllZero), matching serial's DomainDel removing the leaf") } // TestSDStorageCascade_EmitsPerSlotDeletes locks in the load-bearing diff --git a/execution/stagedsync/committer.go b/execution/stagedsync/committer.go index 5a5518c1461..fe2d0681807 100644 --- a/execution/stagedsync/committer.go +++ b/execution/stagedsync/committer.go @@ -96,6 +96,18 @@ type commitmentCalculator struct { // out publishes commitment roots. out chan commitmentResult + // forcePerBlockCompute overrides dbg.BatchCommitments and triggers a + // ComputeCommitment at every block boundary. Mirrors serial's + // `!dbg.BatchCommitments || shouldGenerateChangesets || KeepExecutionProofs` + // gate in [exec3_serial.go]: when changesets must be generated (reorg + // support) or execution proofs must be kept, per-block computation is + // required so that each block's changeset records the branch deltas + // attributable to that block alone. In batch mode, the trie folds + // multiple blocks together and the deferred buffer dedupes branch + // prefixes across the batch — those merged updates flush into the + // LAST block's changeset, which is wrong for per-block unwind. + forcePerBlockCompute bool + wg sync.WaitGroup done chan struct{} } @@ -106,6 +118,7 @@ func newCommitmentCalculator( db kv.TemporalRoDB, logPrefix string, logger log.Logger, + forcePerBlockCompute bool, in chan applyResult, out chan commitmentResult, ) (*commitmentCalculator, error) { @@ -137,17 +150,18 @@ func newCommitmentCalculator( asOfReader := &asOfStateReader{sd: doms, roTx: roTx, txNum: 0} return &commitmentCalculator{ - doms: doms, - db: db, - logPrefix: logPrefix, - logger: logger, - updates: calcUpdates, - state: newCalcState(asOfReader, logger, logPrefix), - asOfReader: asOfReader, - roTx: roTx, - in: in, - out: out, - done: make(chan struct{}), + doms: doms, + db: db, + logPrefix: logPrefix, + logger: logger, + updates: calcUpdates, + state: newCalcState(asOfReader, logger, logPrefix), + asOfReader: asOfReader, + roTx: roTx, + in: in, + out: out, + forcePerBlockCompute: forcePerBlockCompute, + done: make(chan struct{}), }, nil } @@ -213,7 +227,12 @@ func (cc *commitmentCalculator) handleMessage(ctx context.Context, msg applyResu // Break logic: in per-block mode, compute at every block boundary. // Skip the first block if it's a partial block (resumed mid-block). - if !dbg.BatchCommitments { + // `forcePerBlockCompute` overrides dbg.BatchCommitments to mirror + // serial's gate (exec3_serial.go around the `if !dbg.BatchCommitments + // || shouldGenerateChangesets || ...` check) — per-block compute is + // required when changesets must record per-block branch deltas + // (reorg support, KeepExecutionProofs). + if !dbg.BatchCommitments || cc.forcePerBlockCompute { if cc.lastComputedBlock == 0 && r.isPartial { // First block is partial (resumed mid-block). // Compute it (like serial does) to save trie state, then @@ -224,8 +243,9 @@ func (cc *commitmentCalculator) handleMessage(ctx context.Context, msg applyResu cc.computeAndCheck(ctx, r) } } - // In BatchCommitments mode: just accumulate — compute only on - // explicit commitComputeRequest from the apply loop. + // In BatchCommitments mode (without forcePerBlockCompute): just + // accumulate — compute only on explicit commitComputeRequest from + // the apply loop. case *commitComputeRequest: // Explicit compute signal from the apply loop at batch boundary. @@ -282,7 +302,9 @@ func (cc *commitmentCalculator) computeAndPublish(ctx context.Context, br *block cc.asOfReader.txNum = br.lastTxNum + 1 sdCtx.SetStateReader(cc.asOfReader) - rh, err := cc.doms.ComputeCommitment(ctx, cc.roTx, true, br.BlockNum, br.lastTxNum, cc.logPrefix, nil) + // Use hash-aware accumulator wrap — see computeWithBlockAccumulator + // docstring for why this is mandatory in reorg scenarios. + rh, err := cc.computeWithBlockAccumulator(ctx, br) if err != nil { cc.publish(ctx, commitmentResult{ blockNum: br.BlockNum, @@ -329,7 +351,12 @@ func (cc *commitmentCalculator) computeWithoutCheck(ctx context.Context, br *blo cc.asOfReader.txNum = br.lastTxNum + 1 sdCtx.SetStateReader(cc.asOfReader) - if _, err := cc.doms.ComputeCommitment(ctx, cc.roTx, true, br.BlockNum, br.lastTxNum, cc.logPrefix, nil); err != nil { + // Use the same hash-aware accumulator wrap as computeAndCheck — without + // it, the [state] write inside ComputeCommitment can land in a stale + // past-changeset entry chosen non-deterministically by GetChangesetByBlockNum + // when pastChangesAccumulator holds multiple changesets per block number + // (canonical + fork during reorg-bounce tests). + if _, err := cc.computeWithBlockAccumulator(ctx, br); err != nil { // Partial-block compute is intentionally not verified (no header root // to compare against), but a real ComputeCommitment failure leaves // later trie state suspect — log so the failure isn't silent. @@ -372,7 +399,15 @@ func (cc *commitmentCalculator) computeAndCheck(ctx context.Context, br *blockRe cc.asOfReader.txNum = br.lastTxNum + 1 sdCtx.SetStateReader(cc.asOfReader) - rh, err := cc.doms.ComputeCommitment(ctx, cc.roTx, true, br.BlockNum, br.lastTxNum, cc.logPrefix, nil) + // In per-block compute mode, the exec loop has (or is about to) + // swap the changeset accumulator to block N+1 by the time this runs. + // Wrap ComputeCommitment so any branch writes (mid-process inline + // flushes from `pendingPrefixes` collisions, plus any writes via + // putBranch) go into block N's saved changeset, not whatever the + // exec loop has set as current. Without this wrap, block N's + // branch deltas leak into block N+1's CS, producing a wrong-trie-root + // chain on subsequent blocks (see TestTxLookupUnwind reproducer). + rh, err := cc.computeWithBlockAccumulator(ctx, br) if err != nil { cc.publish(ctx, commitmentResult{ blockNum: br.BlockNum, @@ -408,6 +443,70 @@ func (cc *commitmentCalculator) publish(ctx context.Context, r commitmentResult) } } +// computeWithBlockAccumulator runs ComputeCommitment with the changeset +// accumulator switched to block N's saved changeset (looked up by hash) so +// that any branch writes during compute (mid-process inline flushes from +// `pendingPrefixes` collisions, plus the [state] write at end via +// encodeAndStoreCommitmentState) land in block N's CS rather than whatever +// the exec loop has installed as current. +// +// IMPORTANT: hash-aware lookup is mandatory here. pastChangesAccumulator +// can hold multiple changesets per block number after a fork-bounce +// (canonical block 1 + forks[i] block 1 with different hashes), and a +// number-only GetChangesetByBlockNum returns the first match in +// non-deterministic map iteration order. That non-determinism caused the +// calculator's [state] write for canonical block 1 to land in the fork's +// block 1 CS during the TestBlockchainHeaderchainReorgConsistency +// reproducer, leaving canonical block 1's CS without [state] and producing +// off-by-one wrong-trie-root chains on the next iteration's re-execution. +// +// If block N's CS hasn't been saved yet (rare race with the exec loop's +// SavePastChangesetAccumulator), falls through to whatever current is +// installed — same as the pre-fix behavior. +// +// Also annotates the pending deferred update (set inside ComputeCommitment +// when defer mode is on) with the block's hash, so the next call's +// FlushPendingUpdates uses the same hash-aware routing. +func (cc *commitmentCalculator) computeWithBlockAccumulator(ctx context.Context, br *blockResult) ([]byte, error) { + defer func() { + // Stamp the pending update (if any was set during ComputeCommitment) + // with this block's hash so FlushPendingUpdates on the next call + // routes to the exact (BlockNum, BlockHash) entry rather than + // guessing among ambiguous block-number-only matches. + if upd := cc.doms.GetCommitmentContext().PeekPendingUpdate(); upd != nil && upd.BlockNum == br.BlockNum { + upd.BlockHash = br.BlockHash + } + }() + + cs := cc.doms.GetChangesetByHash(br.BlockNum, br.BlockHash) + // Always take the lock around ComputeCommitment, even on the cs==nil + // fast path: the FlushPendingUpdates that ComputeCommitment runs + // internally still mutates the global accumulator pointer + per-domain + // diff fields, racing with the apply loop's SetChangesetAccumulator if + // we don't serialize. Without this, race detector flags ~73 SetDiff vs + // PutWithPrev hits on the cs==nil path (genesis, missing-CS edge cases). + cc.doms.LockChangesetAccumulator() + defer cc.doms.UnlockChangesetAccumulator() + if cs == nil { + return cc.doms.ComputeCommitmentLocked(ctx, cc.roTx, true, br.BlockNum, br.lastTxNum, cc.logPrefix, nil) + } + // LOAD-BEARING swap under the outer lock (already taken above). The + // Set/restore dance below mutates the global current-accumulator + // pointer; the deferred branch writes from block N-1 (flushed inside + // ComputeCommitmentLocked → FlushPendingUpdatesLocked) AND the [state] + // marker write at end of compute also touch that same global pointer + // and the per-domain diff fields. Holding changesetMu through all of + // it serializes against the apply goroutine's DomainPut/DomainDel. + // + // Inside the lock we must use the *Locked variants of Get/Set/Compute + // — the public counterparts re-acquire the same Mutex and would + // self-deadlock. + prev := cc.doms.GetChangesetAccumulatorLocked() + cc.doms.SetChangesetAccumulatorLocked(cs) + defer cc.doms.SetChangesetAccumulatorLocked(prev) + return cc.doms.ComputeCommitmentLocked(ctx, cc.roTx, true, br.BlockNum, br.lastTxNum, cc.logPrefix, nil) +} + // asOfStateReader reads account/storage/code at a specific txNum via // sd.GetAsOf (which checks sd.mem first, then falls through to files). // Commitment domain reads use GetLatest since branches are only written diff --git a/execution/stagedsync/exec3_parallel.go b/execution/stagedsync/exec3_parallel.go index 397287fee50..a88ba674563 100644 --- a/execution/stagedsync/exec3_parallel.go +++ b/execution/stagedsync/exec3_parallel.go @@ -193,8 +193,15 @@ func (pe *parallelExecutor) exec(ctx context.Context, execStage *StageState, u U pe.domains().SetChangesetAccumulator(pe.currentChangeSet) } - // Start the commitment calculator. - calculator, err := newCommitmentCalculator(executorContext, pe.rs.Domains(), pe.cfg.db, pe.logPrefix, pe.logger, commitResults, rootResults) + // Start the commitment calculator. forcePerBlockCompute mirrors serial's + // per-block gate (exec3_serial.go: `if !dbg.BatchCommitments || + // shouldGenerateChangesets || KeepExecutionProofs`). When changesets + // must be generated (for unwind/reorg) the calculator must compute + // per-block — otherwise batch-mode dedupes branch updates across the + // batch and flushes them all into the last block's changeset, which + // fails on subsequent reorgs. + forcePerBlockCompute := pe.shouldGenerateChangesets || pe.cfg.syncCfg.KeepExecutionProofs + calculator, err := newCommitmentCalculator(executorContext, pe.rs.Domains(), pe.cfg.db, pe.logPrefix, pe.logger, forcePerBlockCompute, commitResults, rootResults) if err != nil { return nil, nil, err } @@ -828,18 +835,30 @@ func (pe *parallelExecutor) execLoop(ctx context.Context) (err error) { pe.blockExecMetrics.Duration.Add(time.Since(blockExecutor.execStarted)) pe.blockExecMetrics.BlockCount.Add(1) } - if err := blockExecutor.sendResult(ctx, blockResult); err != nil { - return err - } - - // Snapshot the just-completed block's changeset and clear sd.mem's - // accumulator before any further sd.mem writes occur. This must - // happen here (in the exec loop) — not in the apply loop — so that - // it is serialized with the exec loop's other sd.mem writes - // (system calls, finalize, ApplyStateWrites for the next block). + // Snapshot the just-completed block's changeset BEFORE sending the + // blockResult, so that the commitment calculator (which consumes + // blockResults on a separate goroutine) can find this block's + // saved changeset via GetChangesetByBlockNum at compute time. + // In per-block compute mode (shouldGenerateChangesets), the + // calculator switches the accumulator to this saved CS for the + // duration of ComputeCommitment (committer.go:computeWithBlockAccumulator) + // so branch writes land in block N's CS rather than whatever the + // exec loop has installed as current. If we saved AFTER sendResult, + // the calculator could race ahead and look up an unsaved CS, + // causing branch deltas to leak into the next block's CS and + // produce wrong-trie-root chains on subsequent reorg-driven + // re-execution (see TestRecreateAndRewind reproducer). Clearing + // the live accumulator and the local pointer must still happen + // here (in the exec loop) so the rotation-to-next-block install + // at line 893-895 is serialized with the exec loop's other + // sd.mem writes (system calls, finalize, ApplyStateWrites for + // the next block). if pe.shouldGenerateChangesets && pe.currentChangeSet != nil { pe.domains().SavePastChangesetAccumulator(blockResult.BlockHash, blockResult.BlockNum, pe.currentChangeSet) } + if err := blockExecutor.sendResult(ctx, blockResult); err != nil { + return err + } pe.domains().SetChangesetAccumulator(nil) pe.currentChangeSet = nil @@ -2953,6 +2972,46 @@ func MergeVersionedWrites(prev, next state.VersionedWrites) state.VersionedWrite func normalizeWriteSet(writes state.VersionedWrites, vm *state.VersionMap, txIndex int, incarnation int, stateReader state.StateReader) state.VersionedWrites { filtered := make(state.VersionedWrites, 0, len(writes)) + // Pre-scan for SD'd addresses. IBS.Selfdestruct emits 3 writes for the + // SD'd account (IncarnationPath=preInc, SelfDestructPath=true, BalancePath=0). + // If we forward all 3 to applyVersionedWrites, it sees d.balance != nil || + // d.incarnation != nil and routes into the "cleanup-before-recreate" + // branch — which writes the account back with {Bal=0, Inc=preInc} encoding + // instead of taking the pure-delete branch (DomainDel(Accounts)). The + // account stays in sd.mem with non-zero incarnation, and a subsequent + // block's CREATE2 at the same address sees a phantom existing account, + // producing wrong execution / wrong trie root in TestRecreateAndRewind. + // Drop the BalancePath/NoncePath/IncarnationPath/CodeHashPath writes for + // SD'd addresses so applyVersionedWrites reaches the pure-delete branch. + // + // Two filters applied here: + // 1. Validated-incarnation: mirror the `w.Version.Incarnation != incarnation` + // skip the SelfDestructPath case uses below — a stale SelfDestructPath=true + // from a non-validated incarnation must not mark the address as SD'd. + // 2. Final-state: pre-Cancun a single tx can SELFDESTRUCT an address and then + // CREATE2-recreate at the same address; IBS emits SelfDestructPath=true + // (from Selfdestruct) followed later by SelfDestructPath=false (from + // CreateAccount, since the recreated object's selfdestructed flag is + // cleared). The address ends ALIVE, so its recreate-time account-field + // writes must survive — only mark sdSet when the LAST SelfDestructPath + // entry for the address (in emission order) is true. applyVersionedWrites + // already uses last-write-wins for d.selfDestruct, so this keeps the two + // in agreement. (EIP-6780 narrows this pattern post-Cancun but doesn't + // eliminate it; mainnet-rare, but cheap to get right.) + sdSet := make(map[accounts.Address]bool) + for _, w := range writes { + if w.Path == state.SelfDestructPath && w.Version.Incarnation == incarnation { + if v, ok := w.Val.(bool); ok { + sdSet[w.Address] = v + } + } + } + for addr, sd := range sdSet { + if !sd { + delete(sdSet, addr) + } + } + // Track which addresses have account-level writes vs storage-only writes. // Serial's MakeWriteSet calls UpdateAccountData for every dirty object, // including those with only storage changes. The commitment needs the @@ -2961,6 +3020,14 @@ func normalizeWriteSet(writes state.VersionedWrites, vm *state.VersionMap, txInd hasStorageWrite := make(map[accounts.Address]bool) for _, w := range writes { + // Drop account-field writes for SD'd addresses so applyVersionedWrites + // takes the pure-delete branch instead of cleanup-before-recreate. + if sdSet[w.Address] { + switch w.Path { + case state.BalancePath, state.NoncePath, state.IncarnationPath, state.CodeHashPath, state.CodePath: + continue + } + } switch w.Path { case state.StoragePath: // Only include writes from the current (validated) incarnation. @@ -3067,6 +3134,15 @@ func normalizeWriteSet(writes state.VersionedWrites, vm *state.VersionMap, txInd } for addr := range allAddresses { + if sdSet[addr] { + // Don't fill account fields for SD'd addresses — same rationale as + // the sdSet drop in the filter loop above. Without this, the + // stateReader fallback below would round-trip pre-SD account state + // (Nonce, CodeHash, Incarnation) back into the writeset and undo + // the SD when applyVersionedWrites picks the cleanup-then-recreate + // branch. + continue + } ver := state.Version{TxIndex: txIndex, Incarnation: incarnation} fields := addrFields[addr]