Skip to content

fix(evm): price the EIP-8250 nonce calldata as transaction data - #12745

Open
AnkushinDaniil wants to merge 4 commits into
eip8141-frame-txs-devnet7from
daniil/eip8250-nonce-calldata-gas
Open

fix(evm): price the EIP-8250 nonce calldata as transaction data#12745
AnkushinDaniil wants to merge 4 commits into
eip8141-frame-txs-devnet7from
daniil/eip8250-nonce-calldata-gas

Conversation

@AnkushinDaniil

Copy link
Copy Markdown
Contributor

EIP-8250 replaces the EIP-8141 nonce field with nonce_keys and nonce_seq, and prices their encoding as transaction data:

nonce_calldata = rlp(nonce_keys) || rlp(nonce_seq)

with nonce_calldata_cost added to standard_gas_limit and nonce_calldata_tokens added to calldata_tokens. The frame transaction intrinsic gas charged the frame data, the signature fields and nothing for the key set, so a transaction carrying keyed nonces was under-charged by exactly the bytes those two fields add.

This is a consensus fault, not an under-priced payload: a block built by a client that charges it carries a gasUsed this one recomputes lower, and the block is rejected. It showed up on a two-client devnet as a fork on the first keyed-nonce transaction, 304 gas apart on a payload whose key set encodes as d1 90 <16-byte key> 80.

The key set replaces the plain nonce rather than joining it, so a payload that still carries the plain nonce adds no data bytes and keeps the EIP-8141 figure — the existing frame transaction gas tests are unchanged.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)

Testing

Two cases in FrameTxProcessorTests pin the delta against the bytes the payload adds, one key encoding as a single byte and one carrying a zero byte so both token rates are covered. Nethermind.Evm.Test FrameTxProcessorTests: 52 passed.

@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @AnkushinDaniil's task in 4m 54s —— View job


Review: fix(evm): price the EIP-8250 nonce calldata as transaction data

  • Gather context (diff, base branch, EIP-8250 envelope)
  • Review CountNonceCalldataTokens correctness
  • Check floor-gas / EIP-7623 interaction
  • Check DoS surface (unbounded stackalloc)
  • Review tests
  • Post findings

The diagnosis is right and the fix is in the right place. CalculateFrameTxIntrinsicGas charged frame data and signature fields and nothing for the key set, so rlp(nonce_keys) || rlp(nonce_seq) rode along free — a gasUsed divergence, not just an under-priced payload. Adding the tokens into the same tokens accumulator that feeds tokens * GasCostOf.TxDataZero matches how EIP-8141 prices everything else, and the field mapping checks out: FrameTxDecoder.cs:46-47 reads nonce_keys then nonce_seq into Transaction.Nonce, which is what this method prices.

I re-derived both test expectations and they're correct: c1 07 | 01 → 3 non-zero × 4 = 12 tokens × 4 = 48; c3 82 01 00 | 01 → (4 non-zero × 4) + 1 zero = 17 tokens × 4 = 68.

No Critical or High findings. Three Medium, all about how the change is scoped and pinned rather than the arithmetic.

Findings

# Severity Finding
1 Medium Charge is not gated on spec.IsEip8250Enabled
2 Medium stackalloc bound is an implicit cross-assembly invariant
3 Medium Priced encoding duplicates the payload encoding with nothing tying them
4 Low CalculateFrameTxIntrinsicGas XML summary not updated (see below)
5 Low Test coverage gaps: no multi-key set, no max-size set

1 — fork scoping. Eip8141TransitionTimestamp and Eip8250TransitionTimestamp are independent (ChainSpecParamsJson.cs:200-201), so an 8141-on / 8250-off window is configurable. CountNonceCalldataTokens takes spec but only passes it through to CountCalldataTokens, so in that window a payload carrying nonce_keys gets charged the 8250 figure while ExecuteFrameTx:33 still runs it down the plain-nonce path. FrameTxDecoder.cs:44-46 defers the fork check to validation, but on this branch that validation doesn't exist — AreNonceKeysWellFormed and IsNonceSetValid have no non-test call sites, so nothing rejects nonce_keys under a pre-8250 spec. This method is currently the only code reacting to the field, and it reacts unconditionally. One-line guard.

2 — stackalloc. Bounded to ~540 bytes today, but only via FrameTxDecoder.DecodeNonceKeys enforcing MaxNonceKeys in a different assembly; Transaction.NonceKeys is a public settable property and nothing re-checks the length before line 450. I don't think it's reachable today, so this is defense-in-depth — but a stackalloc overrun is an uncatchable StackOverflowException on a P2P-fed path, which is worth a local bound.

3 — duplication. Lines 444-448 re-implement FrameTxDecoder.NonceKeysContentLength, and 451-458 re-implement the nonce_keys/nonce half of EncodePayload. This is the same failure class the PR fixes, one level up: the bug was "price ≠ wire bytes", and the fix adds a fresh way for price to drift from wire bytes. The two hand-computed test literals were derived from this method, not from the encoder, so they don't catch a divergence. Either share one encoder or add a test that pins the priced length to the encoded-payload length delta.

4 — doc. The summary on CalculateFrameTxIntrinsicGas (lines 387-392, outside the diff) still lists only "frame data and signature fields". Worth a word. Its EIP-7623 note also now under-describes things: EIP-8250 adds nonce_calldata_tokens to calldata_tokens, which is exactly the floor input that note says is unimplemented. Skipping the floor for the nonce tokens is consistent with the existing choice — just make the comment say so.

Not issues

  • Empty NonceKeys array ([], non-null): encodes as c0 + nonce, priced coherently, no crash.
  • Perf: ~540 bytes of stack and one RLP pass per frame tx, on a path that already does signature verification. Negligible; the duplication in Netcore #3 is the stronger reason to restructure, not speed.
  • nonce_seq width: Transaction.Nonce is ulong, and Rlp.LengthOf(ulong) / writer.Encode(ulong) match what FrameTxDecoder writes.

Caveat

I could not build or run the test suite — dotnet build required an approval this job doesn't have, so all findings are from static reading. The author reports 52 passing in FrameTxProcessorTests; the two new expectations independently reconcile with the byte layout, which is the part I could verify by hand.


· branch daniil/eip8250-nonce-calldata-gas

Comment on lines +436 to +442
private static ulong CountNonceCalldataTokens(Transaction tx, IReleaseSpec spec)
{
UInt256[]? nonceKeys = tx.NonceKeys;
if (nonceKeys is null)
{
return 0;
}

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.

Medium — the new charge is not fork-scoped.

IsEip8141Enabled and IsEip8250Enabled are independent transitions (ChainSpecParamsJson.Eip8141TransitionTimestamp / Eip8250TransitionTimestamp, ReleaseSpec.cs:94-95), so an 8141-on / 8250-off window is representable. spec is already a parameter here but only reaches CountCalldataTokens, so in that window a payload carrying nonce_keys is charged the EIP-8250 figure while ExecuteFrameTx still runs it down the plain-nonce path (accountNonce != tx.Nonce, line 33).

FrameTxDecoder.cs:44-46 says "the fork that admits each is enforced in validation", but I can't find that validation on this branch — AreNonceKeysWellFormed / IsNonceSetValid have no non-test call sites, so nothing today rejects nonce_keys under a pre-8250 spec. Until that validation lands, this method is the only thing reacting to the field, and it reacts unconditionally.

Cheapest fix while the envelope work is in flight:

Suggested change
private static ulong CountNonceCalldataTokens(Transaction tx, IReleaseSpec spec)
{
UInt256[]? nonceKeys = tx.NonceKeys;
if (nonceKeys is null)
{
return 0;
}
private static ulong CountNonceCalldataTokens(Transaction tx, IReleaseSpec spec)
{
UInt256[]? nonceKeys = tx.NonceKeys;
if (nonceKeys is null || !spec.IsEip8250Enabled)
{
return 0;
}

Fix this →

keysContentLength += Rlp.LengthOf(nonceKey);
}

Span<byte> nonceCalldata = stackalloc byte[Rlp.LengthOfSequence(keysContentLength) + Rlp.LengthOf(tx.Nonce)];

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.

Medium — stackalloc sized from an array whose only bound lives in another assembly.

keysContentLength is nonceKeys.Length × up to 33. Today that is bounded to 16 × 33 + 3 + 9 ≈ 540 bytes, but only because FrameTxDecoder.DecodeNonceKeys (FrameTxDecoder.cs:149-166) enforces Eip8250Constants.MaxNonceKeys, and because no other path currently populates Transaction.NonceKeys (it's a public settable property on Nethermind.Core.Transaction). Nothing between the decoder and this line re-checks it — CalculateFrameTxIntrinsicGas is called at line 63, before any nonce-key well-formedness check (there is none on this branch), and AreNonceKeysWellFormed is never invoked in production code.

I don't believe it's reachable today, so this is defense-in-depth rather than a live bug — but a stackalloc overrun is an uncatchable StackOverflowException that takes the node down, on a path fed by P2P data. Per .agents/rules/robustness.md ("validate data from untrusted sources before use"), this deserves a local bound rather than an implicit cross-assembly invariant.

Two options:

  • Guard: if (nonceKeys.Length > Eip8250Constants.MaxNonceKeys) return 0; (or reject the tx), which also documents the bound at the allocation site.
  • Better: drop the buffer entirely. CountCalldataTokens only needs (length, zero count), so you can accumulate both per key from Rlp.LengthOf/the key's big-endian bytes plus the ≤4-byte sequence header, with no allocation and no size assumption.

Comment on lines +444 to +459
int keysContentLength = 0;
foreach (UInt256 nonceKey in nonceKeys)
{
keysContentLength += Rlp.LengthOf(nonceKey);
}

Span<byte> nonceCalldata = stackalloc byte[Rlp.LengthOfSequence(keysContentLength) + Rlp.LengthOf(tx.Nonce)];
RlpWriter writer = new(nonceCalldata);
writer.StartSequence(keysContentLength);
foreach (UInt256 nonceKey in nonceKeys)
{
writer.Encode(nonceKey);
}

writer.Encode(tx.Nonce);
return CountCalldataTokens(nonceCalldata, spec);

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.

Medium — the priced encoding is a second, independent copy of the payload encoding.

Lines 444-448 duplicate FrameTxDecoder.NonceKeysContentLength (FrameTxDecoder.cs:168-177) verbatim, and lines 451-458 duplicate the nonce_keys + nonce half of FrameTxDecoder.EncodePayload (FrameTxDecoder.cs:99-107). Nothing links them.

That's the same failure class this PR is fixing, one level up: the bug was "the price doesn't match the bytes on the wire", and the fix reintroduces a way for the price to drift from the bytes on the wire. If the envelope encoding changes — key width, a length prefix, nonce_seq widening past ulong — one side moves and the other silently doesn't, and you get another gasUsed fork.

Two ways to tie them together, in preference order per the repo's DRY rule:

  1. Reuse FrameTxDecoder's helpers (make NonceKeysContentLength internal + InternalsVisibleTo, or expose a single NonceCalldataTokens-shaped helper next to the encoder that both call). One encoder, one price.
  2. If you'd rather keep the encodings separate, add a test that pins them to each other rather than to hand-computed constants — e.g. encode the tx with and without NonceKeys via FrameTxDecoder and assert the byte-length delta equals Rlp.LengthOfSequence(keysContentLength). That converts a silent divergence into a red test.

Right now the only thing checking they agree is the two hand-computed literals in FrameTxProcessorTests, which were derived from this method rather than from the encoder.

Fix this →

Comment on lines +735 to +749
[TestCase(7ul, 48ul, TestName = "Execute_KeyedNoncePayload_ChargesASingleByteKey")]
[TestCase(0x0100ul, 68ul, TestName = "Execute_KeyedNoncePayload_ChargesAKeyCarryingAZeroByte")]
public void Execute_KeyedNoncePayload_ChargesItsCalldataCost(ulong nonceKey, ulong expectedExtraGas)
{
DeploySmartSender(ApproveCode(TxFrame.ApproveExecutionAndPayment));

CallOutputTracer plain = new();
Assert.That(Process(FrameTx(nonce: 0, SelfVerifyFrame()), tracer: plain).TransactionExecuted, Is.True);

Transaction keyed = FrameTx(nonce: 1, SelfVerifyFrame());
keyed.NonceKeys = [(UInt256)nonceKey];
CallOutputTracer keyedTracer = new();
Assert.That(Process(keyed, tracer: keyedTracer).TransactionExecuted, Is.True);

Assert.That(keyedTracer.GasSpent - plain.GasSpent, Is.EqualTo(expectedExtraGas));

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 — coverage gaps. The two cases cover both token rates on a single-key set, which is the narrowest shape the feature supports. Two more [TestCase]s would meaningfully widen it, and both target real arithmetic in the new code:

  • A multi-key set. Every case here produces a one-byte RLP sequence header (c1/c3), so Rlp.LengthOfSequence(keysContentLength) and the keysContentLength accumulator loop are only exercised in their trivial branch. A set whose content exceeds 55 bytes (two 32-byte keys does it: f8 42 …) puts the long-form header on the wire and is where an off-by-one in the length accumulation would show up.
  • MaxNonceKeys × 32-byte keys. This is the largest nonceCalldata the method can be handed and the value the stackalloc bound rests on (see my comment on line 450). Pinning it makes the buffer-size assumption a tested invariant rather than an implicit one.

Also, plain is built at nonce: 0 and keyed at nonce: 1. The delta is correct — plain prices no nonce bytes at all, and rlp(1) is one byte — but it reads as if the two nonces cancel. Since rlp(0) is 0x80 (one zero byte, worth 1 token) and rlp(1) is 0x01 (one non-zero byte, worth 4), a reader checking the arithmetic has to notice the plain side contributes nothing before the numbers work out. One clause in the <remarks> would save that.

The parameterisation itself and the byte-layout <remarks> are the right shape — pinning the expected gas to the literal encoding is exactly what makes this a useful consensus regression test.

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

EVM Opcode Benchmark Diff

Aggregated runs: base=3, pr=3
Noisy rerun opcodes: EXTCODESIZE

Improvements (1)

Opcode Base Median (ns) PR Median (ns) Delta Abs Δ (ns) Base CV PR CV Threshold Uncertainty Effective
EXTCODESIZE 861.600 104.100 -87.92% 757.500 1.5% 5.0% ±5.0% ±1.5% ±10.0%

{
// A key set carried before the EIP-8250 transition is not consumed as one either: ExecuteFrameTx
// takes the plain-nonce path, so the payload is priced at the EIP-8141 figure.
if (tx.NonceKeys is not { } nonceKeys || !spec.IsEip8250Enabled)

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.

A frame transaction carrying nonce_keys is accepted and executed on a chain where EIP-8141 is active but EIP-8250 is not: the decoder defers the fork check to validation, but the TxType.FrameTx validator chain gates only on IsEip8141Enabled and nothing else inspects NonceKeys. An 8141-only client cannot decode that payload, so it rejects the block — the same split this PR closes, one fork earlier. Gating belongs in the validator, where the spec is available.

public ValidationResult IsWellFormed(Transaction transaction, IReleaseSpec releaseSpec) =>
    transaction.NonceKeys is { } keys
        ? releaseSpec.IsEip8250Enabled
            ? KeyedNonceManager.AreNonceKeysWellFormed(keys)
                ? ValidationResult.Success
                : TxErrorMessages.MalformedNonceKeys
            : TxErrorMessages.InvalidTxType(releaseSpec.Name)
        : ValidationResult.Success;

return 0;
}

Span<byte> buffer = stackalloc byte[MaxNonceCalldataLength];

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.

The token count prices buffer[..Length(tx)] — the predicted length — rather than what the writer actually wrote. RlpWriter.Position reports that directly, and using it also drops one of two walks over the key array. Worth doing because if Encode ever writes short, the unwritten tail gets priced; combined with [SkipLocalsInit] that tail would be uninitialised stack, making the charge machine-dependent.

Span<byte> buffer = stackalloc byte[MaxNonceCalldataLength];
RlpWriter writer = new(buffer);
FrameTxNonceCalldata.Encode(tx, ref writer);
return CountCalldataTokens(buffer[..writer.Position], spec);

/// The key set replaces the EIP-8141 <c>nonce</c> field rather than joining it, so a payload that still
/// carries the plain nonce adds no data bytes and is priced at the EIP-8141 figure.
/// </remarks>
private static ulong CountNonceCalldataTokens(Transaction tx, IReleaseSpec spec)

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.

This method's 540-byte stackalloc is zero-initialised on every keyed-nonce transaction to hold a payload that is usually three to five bytes. [SkipLocalsInit] removes the memset and matches the per-method idiom already used in EvmStack.cs and VirtualMachine.cs.

if (transaction.NonceKeys is { } nonceKeys)
{
writer.StartSequence(KeysContentLength(nonceKeys));
foreach (UInt256 nonceKey in nonceKeys)

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.

These two loops (:179, :196) copy 32 bytes per key only for the callee to take a reference to the copy. They were moved rather than written here, but the move put them on the intrinsic-gas path as well as the encoder path, so a 16-key transaction pays the copies on every execution. foreach (ref readonly UInt256 key in nonceKeys.AsSpan()) avoids it — UInt256 is a readonly struct, so there is no defensive copy.

foreach (ref readonly UInt256 nonceKey in nonceKeys.AsSpan())
{
    contentLength += Rlp.LengthOf(in nonceKey);
}

@AnkushinDaniil
AnkushinDaniil force-pushed the daniil/eip8250-keyed-nonce-envelope branch from 511feae to da2275b Compare August 12, 2026 14:58
Base automatically changed from daniil/eip8250-keyed-nonce-envelope to eip8141-frame-txs-devnet7 August 12, 2026 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants