fix(evm): price the EIP-8250 nonce calldata as transaction data - #12745
fix(evm): price the EIP-8250 nonce calldata as transaction data#12745AnkushinDaniil wants to merge 4 commits into
Conversation
|
Claude finished @AnkushinDaniil's task in 4m 54s —— View job Review:
|
| # | 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
NonceKeysarray ([], non-null): encodes asc0+ 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_seqwidth:Transaction.Nonceisulong, andRlp.LengthOf(ulong)/writer.Encode(ulong)match whatFrameTxDecoderwrites.
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
| private static ulong CountNonceCalldataTokens(Transaction tx, IReleaseSpec spec) | ||
| { | ||
| UInt256[]? nonceKeys = tx.NonceKeys; | ||
| if (nonceKeys is null) | ||
| { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
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:
| 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; | |
| } |
| keysContentLength += Rlp.LengthOf(nonceKey); | ||
| } | ||
|
|
||
| Span<byte> nonceCalldata = stackalloc byte[Rlp.LengthOfSequence(keysContentLength) + Rlp.LengthOf(tx.Nonce)]; |
There was a problem hiding this comment.
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.
CountCalldataTokensonly needs (length, zero count), so you can accumulate both per key fromRlp.LengthOf/the key's big-endian bytes plus the ≤4-byte sequence header, with no allocation and no size assumption.
| 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); |
There was a problem hiding this comment.
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:
- Reuse
FrameTxDecoder's helpers (makeNonceKeysContentLengthinternal +InternalsVisibleTo, or expose a singleNonceCalldataTokens-shaped helper next to the encoder that both call). One encoder, one price. - 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
NonceKeysviaFrameTxDecoderand assert the byte-length delta equalsRlp.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.
| [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)); |
There was a problem hiding this comment.
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), soRlp.LengthOfSequence(keysContentLength)and thekeysContentLengthaccumulator 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 largestnonceCalldatathe method can be handed and the value thestackallocbound 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.
EVM Opcode Benchmark DiffAggregated runs: base=3, pr=3 Improvements (1)
|
a836f85 to
4397362
Compare
| { | ||
| // 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) |
There was a problem hiding this comment.
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]; |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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);
}511feae to
da2275b
Compare
EIP-8250 replaces the EIP-8141
noncefield withnonce_keysandnonce_seq, and prices their encoding as transaction data:with
nonce_calldata_costadded tostandard_gas_limitandnonce_calldata_tokensadded tocalldata_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
gasUsedthis 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 asd1 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
Testing
Two cases in
FrameTxProcessorTestspin 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.TestFrameTxProcessorTests: 52 passed.