fix(state): omit no-op nonce change from block access list - #12830
fix(state): omit no-op nonce change from block access list#12830AnkushinDaniil wants to merge 1 commit into
Conversation
EIP-7928 records nonce changes from actual state transitions, so a SetNonce that writes the account's current nonce must not appear as a nonce_change. TracedAccessWorldState.SetNonce now records only when the value differs, matching the existing no-op guard for code changes.
|
Claude finished @AnkushinDaniil's task in 4m 16s —— View job Review:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ulong oldNonce = GetNonceInternal(address); | ||
| base.SetNonce(address, nonce); | ||
| _generatingBlockAccessList.AddNonceChange(address, nonce); | ||
| if (nonce != oldNonce) | ||
| { | ||
| _generatingBlockAccessList.AddNonceChange(address, nonce); | ||
| } |
There was a problem hiding this comment.
Low — touched-but-unchanged account is now dropped entirely.
The guard skips the AddNonceChange call, and AddNonceChange is what reaches GetOrAddAccountChanges. So for a no-op SetNonce on an address nothing else touched at this index, the address disappears from the BAL rather than appearing with an empty change set. That is the exact failure mode AddBalanceChange deliberately avoids (BlockAccessListAtIndex.cs:88-93): "Don't add zero balance transfers, but DO add empty account changes: EIP-7928 includes touched-but-unchanged accounts in the suggested BAL, so dropping them diverges from the hash."
Not reachable today — both production callers touch the address first (CreateAccountIfNotExists → AddAccountRead in PredeployInstaller.cs:82, GetNonce in TransactionProcessor.cs:1158) — and AddCodeChange already returns early the same way, so this is a consistency/robustness point rather than a live bug. Worth either an AddAccountRead(address) in the skip branch or a one-line comment recording that callers guarantee the touch.
| AccountChangesAtIndex? ac = tws.GetGeneratingBlockAccessList()!.GetAccountChanges(TestItem.AddressA); | ||
| using (Assert.EnterMultipleScope()) | ||
| { | ||
| Assert.That(ac?.NonceChange is not null, Is.EqualTo(expectRecorded)); | ||
| if (expectRecorded) | ||
| { | ||
| Assert.That(ac!.NonceChange!.Value.Value, Is.EqualTo(newNonce)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Low — the unchanged case tolerates two different outcomes.
ac?.NonceChange is not null is false both when the account entry exists with no nonce change and when the account entry is absent entirely. Today it is the latter (see the inline note on SetNonce), which is the behaviour actually worth pinning down. An explicit Assert.That(ac, ...) in the expectRecorded == false branch would make the intended BAL shape part of the contract rather than an accident.
Minor: the changed case (1 → 2) largely subsumes the existing NonceOp_RecordsNonceChange "SetNonce" case (0 → 5) — per AGENTS.md these could be one parameterized test, though keeping them separate is defensible given the different assertion shapes.
flcl42
left a comment
flcl42
left a comment
There was a problem hiding this comment.
Found 1 low-severity issue in src/Nethermind/Nethermind.State.Test/TracedAccessWorldStateTests.cs.
| Assert.That(ac?.NonceChange is not null, Is.EqualTo(expectRecorded)); | ||
| if (expectRecorded) | ||
| { | ||
| Assert.That(ac!.NonceChange!.Value.Value, Is.EqualTo(newNonce)); |
There was a problem hiding this comment.
[LOW] Dependent assertions can turn a nonce regression into an exception
When the changed-value case regresses so that ac is null or lacks a nonce change, the first assertion records a failure but the multiple-assert scope continues into this dereference, producing NullReferenceException or InvalidOperationException. This dependent null-check/dereference pattern makes the regression failure less diagnostic.
3 participants
Changes
TracedAccessWorldState.SetNoncerecorded a nonce change into the block access list unconditionally, including when the written nonce equals the account's current nonce. EIP-7928 builds the access list from actual state transitions (if nonce_changed(addr): append), so a no-op write must not appear as anonce_change.This surfaces on any system path that re-asserts a nonce that is already set — e.g. a predeploy install that keeps an existing nonce (
Math.Max(nonce, 1)when the nonce is already1). The redundantSetNonceproduced a spuriouspost == prenonce-change entry, making the block's access list disagree with the spec-derived list.The fix reads the current nonce and records the change only when the value actually differs, matching the existing no-op guard already used for code changes in the same tracer.
Types of changes
Testing
Parameterized regression in
TracedAccessWorldStateTests: an unchangedSetNoncerecords no nonce change, a changed one still does. Verified as a real regression by a positive control (removing the guard fails the unchanged case).Serial suites, all green: State 1184/0, Core (block access list) 63/0, Consensus (block access list) 14/0, Evm 5188/0, Blockchain (block access list) 27/0.
Documentation
No documentation changes required.