[SharovBot] fix Amsterdam signer support and BAL non-determinism#19434
[SharovBot] fix Amsterdam signer support and BAL non-determinism#19434
Conversation
Two issues caused the Amsterdam execution-spec tests to fail: 1. MakeSigner/LatestSigner did not handle AmsterdamTime independently of PragueTime. Test fixtures for the Amsterdam fork set AmsterdamTime but leave PragueTime nil, so the signer returned for Amsterdam blocks was missing setCode and (in LatestSigner) blob capabilities. Add an explicit Amsterdam case to MakeSigner and extend the LatestSigner checks to include AmsterdamTime. 2. During parallel block execution the finalization merge path in nextResult appended the complete post-finalization write set to the pre-existing writes, creating duplicate (Address, Path, Key) entries. Because IntraBlockState.VersionedWrites iterates a Go map whose order varies across processes, the duplicates were processed in non-deterministic order by CreateBAL, producing different balance- change lists and therefore different BAL hashes ~10% of the time. Replace instead of append since addWrites already contains all original writes (re-applied via ApplyVersionedWrites) plus the finalization changes. Fixes: https://github.com/erigontech/erigon/actions/runs/22296091141/job/64492938121 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
During parallel block execution, stateObject.Code() only reads from the state reader (e.g. BufferedReader), which does not see code written by prior transactions in the same block (only in the version map). This caused EIP-7702 delegation code set by tx0 to be invisible to tx1, producing incorrect BAL (Block Access List) entries. Three complementary fixes: - stateObject.Code(): fall back to version map when code is nil - versionedStateReader.ReadAccountCode/Size: check version map for CodePath entries (consistent with ReadAccountData) - exec3_parallel.go finalize: merge finalization writes with execution writes instead of replacing, preserving entries the finalization replay may drop due to stale state - bal_create.go: deterministic sort of writes for stable BAL output Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix EIP-7928 block access list hash non-determinism during parallel transaction execution by addressing several race conditions: 1. Synchronise CodeHash in getStateObject when code is loaded from the version map (written by a prior tx). refreshVersionedAccount may skip updating CodeHash because sdb.Version() makes the version check fail for entries from earlier transactions. The stale CodeHash caused SetCode's "revert to original" optimisation to incorrectly delete code writes when clearing an EIP-7702 delegation set by a prior tx. 2. Sort ApplyVersionedWrites entries by (Address, Path, Key) so that state object creation order is deterministic regardless of Go map iteration order. 3. Add WaitGroup synchronisation between the apply-loop goroutine and block finalization to ensure all per-tx state updates are visible before finalize reads state. 4. Flush merged writes (including fee-calc changes) to the version map so subsequent per-tx finalizations see full post-tx state. 5. Use versionedStateReader for coinbase balance reads during finalize to get deterministic values from the version map. 6. Enhance versionedStateReader to check the version map for accounts, code, and storage written by prior transactions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…alize() coinbase reader block
|
[SharovBot] 🔧 Fixed lint failure — |
…rallel execution FlushVersionedWrites previously wrote each entry under a separate lock, allowing concurrent workers to observe a partially-flushed state (e.g. seeing an AddressPath write but not the corresponding CodePath write from the same transaction). This caused intermittent BAL (EIP-7928) hash mismatches during parallel execution of EIP-7702 SET_CODE tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@mh0lt flaky test fix for amsterdam. some might be stupid and useless. especially the whole Amsterdam case part? |
… deadlocks test apply loop The applyWg mechanism called Add(1) per txResult and expected Done() to be called by the apply-loop consumer. The production apply loop does call Done(), but the test's simplified loop (runParallel) does not, causing TestAlternatingTx to hang until the 10-minute timeout. The versionedStateReader approach (cbReader) already provides deterministic coinbase reads from the version map without needing pe.rs to be fully applied first. Remove the applyWg.Wait() gate and the Add/Done machinery entirely.
|
[SharovBot] 🔧 Fixed Root cause: Fix: Removed the |
|
@mh0lt can you please either merge/review or incorporate in your PR so we do not have hive failures. |
yperbasis
left a comment
There was a problem hiding this comment.
merge latest main and double check if the changes still make sense
Amsterdam implies Prague, so: - MakeSigner: remove duplicate Amsterdam switch case (identical to Prague) - LatestSigner: remove redundant '|| config.AmsterdamTime != nil' checks Addresses review feedback from yperbasis.
There was a problem hiding this comment.
Pull request overview
This PR addresses two execution-layer correctness issues: (1) enabling Amsterdam signer selection when only AmsterdamTime is configured, and (2) eliminating non-deterministic EIP-7928 Block Access List (BAL) hashes during parallel transaction execution by making write application/flush and certain reads deterministic.
Changes:
- Make version-map flushing atomic and reuse lock acquisition to prevent partially-observed state during parallel execution.
- Ensure deterministic state application and BAL construction by sorting versioned writes consistently and improving intra-block visibility of prior-tx updates.
- Fix several parallel-finalization ordering/state-visibility gaps (coinbase reads, merging finalize writes, flushing merged writes) to stabilize BAL hashing.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
execution/state/versionmap.go |
Adds a locked internal write helper and makes FlushVersionedWrites flush all writes under a single lock for atomic visibility. |
execution/state/versionedio.go |
Extends versionedStateReader to fall back to the version map for account/storage/code reads when needed for same-block visibility. |
execution/state/intra_block_state.go |
Synchronizes CodeHash when code is loaded from the version map; sorts ApplyVersionedWrites input for deterministic processing order. |
execution/stagedsync/exec3_parallel.go |
Uses version-map-aware reader for deterministic coinbase balance, merges finalize writes with execution writes, and flushes merged writes for subsequent finalizations. |
execution/stagedsync/bal_create.go |
Sorts per-tx write sets before BAL construction to remove nondeterminism from iteration order. |
Comments suppressed due to low confidence (1)
execution/stagedsync/bal_create.go:59
- The comment about write slice order being non-deterministic is now misleading: this function explicitly sorts
writesabove, so the order is deterministic at this point. Please update the comment to reflect the actual reason for the two-pass handling (ensuring SelfDestructPath is applied before other paths), not Go map iteration order.
// First pass: apply SelfDestructPath writes so the selfDestructed flag
// is up-to-date before balance/nonce/code writes are processed.
// The write slice order is non-deterministic, and a SelfDestructPath=false
// (un-selfdestruct in a later tx) may appear after BalancePath in the slice.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // CodeHash causes the "revert to original" optimisation in SetCode | ||
| // to incorrectly delete code writes when clearing a delegation that | ||
| // was set by a prior transaction in the same block. | ||
| codeHash := accounts.InternCodeHash(crypto.Keccak256Hash(code)) |
There was a problem hiding this comment.
Computing Keccak256Hash(code) here can be expensive for large contract code, and this path may be hit frequently in parallel execution when prior-tx code updates are loaded from the version map. Consider using the version map’s existing CodeHashPath value (when present) to sync CodeHash instead of hashing the full bytecode, falling back to hashing only if the hash entry is unavailable.
| codeHash := accounts.InternCodeHash(crypto.Keccak256Hash(code)) | |
| // Prefer using the existing CodeHashPath value from the version map | |
| // (when present) to avoid recomputing Keccak256 on large code blobs. | |
| var rawCodeHash common.Hash | |
| hashFromVersionMap := false | |
| if sdb.versionMap != nil { | |
| if res := sdb.versionMap.Read(addr, CodeHashPath, accounts.NilKey, sdb.txIndex); res.Status() == MVReadResultDone { | |
| switch v := res.Value().(type) { | |
| case common.Hash: | |
| rawCodeHash = v | |
| hashFromVersionMap = true | |
| case [32]byte: | |
| rawCodeHash = common.BytesToHash(v[:]) | |
| hashFromVersionMap = true | |
| } | |
| } | |
| } | |
| if !hashFromVersionMap { | |
| rawCodeHash = crypto.Keccak256Hash(code) | |
| } | |
| codeHash := accounts.InternCodeHash(rawCodeHash) |
| // Sort writes by (Address, Path, Key) to ensure deterministic processing | ||
| // order. VersionedWrites come from WriteSet map iteration (Go maps have | ||
| // non-deterministic order). Processing order matters because some paths | ||
| // (CodePath, SelfDestructPath) call GetOrNewStateObject which triggers a | ||
| // read from the stateReader. If a BalancePath write for the same address | ||
| // has already been processed, the state object is already loaded and no | ||
| // read occurs; otherwise an extra read is recorded. Different reads | ||
| // produce different EIP-7928 BAL hashes. |
There was a problem hiding this comment.
This change fixes determinism by sorting versioned writes before applying them, but there’s no regression test covering the nondeterminism scenario described in the comment (different read recording affecting BAL hash). Since this file already has tests for ApplyVersionedWrites, please add a focused test that exercises the problematic ordering (e.g., CodePath/SelfDestructPath vs BalancePath) and asserts deterministic reads/BAL-related output regardless of original write ordering.
[SharovBot] Fix Amsterdam signer support and BAL non-determinism in parallel execution
Summary
MakeSignerandLatestSignerso thatsetCodeandblobtx types are supported when onlyAmsterdamTimeis configured (withoutPragueTime).ApplyVersionedWritesoutput by (Address, Path, Key) for deterministic application orderWaitGroupbefore block finalization to ensure all prior tx state is applied tope.rsversionedStateReader) rather than a stalepe.rssnapshotCodeHashingetStateObjectwhen code is loaded from the version map, preventing the "revert to original" optimization from incorrectly deleting code writesFlushVersionedWritesnow holds a single lock for all writes, preventing concurrent workers from observing a partially-flushed state (e.g. seeingAddressPathbut not the correspondingCodePathfrom the same transaction)Test plan
execution/testspackage compiles without errorsTestExecutionSpecBlockchainDevnet/amsterdampasses in a single runTestExecutionSpecBlockchainDevnet/amsterdampasses 30 consecutive times with-count=1TestExecutionSpecBlockchainDevnet/prague/eip7702passes (no regression)Fixes CI job: https://github.com/erigontech/erigon/actions/runs/22296091141/job/64492938121
🤖 Generated with Claude Code