Skip to content

fix(state): omit no-op nonce change from block access list #12830

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AnkushinDaniil wants to merge 1 commit into
base: eip8141-frame-txs-devnet7
Choose a base branch
from
+28 −1
Open

fix(state): omit no-op nonce change from block access list #12830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/Nethermind/Nethermind.State.Test/TracedAccessWorldStateTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,29 @@ public void NonceOp_RecordsNonceChange(
}
}

[TestCase(1ul, 1ul, false, TestName = "SetNonce_UnchangedValue_RecordsNoNonceChange")]
[TestCase(1ul, 2ul, true, TestName = "SetNonce_ChangedValue_RecordsNonceChange")]
public void SetNonce_RecordsNonceChange_OnlyWhenValueChanges(
ulong initialNonce, ulong newNonce, bool expectRecorded)
{
(TracedAccessWorldState tws, IDisposable scope) = CreateTracingState(ws =>
ws.CreateAccount(TestItem.AddressA, 0, initialNonce));
using (scope)
{
tws.SetNonce(TestItem.AddressA, newNonce);

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));

@flcl42 flcl42 Aug 15, 2026

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.

[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.

}
}
Comment on lines +124 to +132

@claude claude Bot Aug 15, 2026

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.

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.

}
}

[Test]
public void InsertCode_RecordsCodeChange()
{
Expand Down
6 changes: 5 additions & 1 deletion src/Nethermind/Nethermind.State/TracedAccessWorldState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,12 @@ public override void IncrementNonce(Address address, ulong delta, out ulong oldN

public override void SetNonce(Address address, in ulong nonce)
{
ulong oldNonce = GetNonceInternal(address);
base.SetNonce(address, nonce);
_generatingBlockAccessList.AddNonceChange(address, nonce);
if (nonce != oldNonce)
{
_generatingBlockAccessList.AddNonceChange(address, nonce);
}
Comment on lines +104 to +109

@claude claude Bot Aug 15, 2026

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.

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.

Fix this →

}

public override bool InsertCode(Address address, in ValueHash256 codeHash, ReadOnlyMemory<byte> code, IReleaseSpec spec, bool isGenesis = false)
Expand Down
Loading