EIP-8141: canonical paymaster reference assembler and test suite - #12612
EIP-8141: canonical paymaster reference assembler and test suite#12612Marchhill wants to merge 6 commits into
Conversation
Two-pass label-resolving assembler for the canonical paymaster runtime, plus a self-contained keccak-256 so the bytecode and its per-fork code hash are reproducible. Mirrors the current PR #12012 semantics: the validation scheme gate is scheme != ARBITRARY and admin ops authorize on caller == slot0 OR a signer-signed entry at index 1.
Exercises the assembled canonical paymaster runtime end-to-end under the prototype fork: sponsored EOA payment through the real bytecode, wrong/ missing/bespoke signature rejection, deposit rules, the timelocked withdrawal and rotation lifecycles, cancel, non-signer/pending rejection, a near-match code-hash fixture, and the measured validation-path gas.
Add coverage for the gaps flagged by the independent audit: - F1: a 32-byte all-zero msg at index 1 is non-empty but passes the SIGPARAM(msg,1) != 0 value gate, so payment is approved when the signature resolves over the explicit zero digest to slot 0. Pins the current != 0 behavior (a strict len(msg)==0 fix would flip it). - Withdrawal-finalize CALL failure: amount exceeds balance, and a signer contract that reverts on receive. Both revert the finalize and preserve the pending slots so it can be retried. - Reentrancy: a contract signer re-entering finalize (cleared timelock -> revert) and initiate (fresh timelocked pending only). No double spend or drain; the signer is paid exactly once. - Admin via the frame-tx signature route: a signer-signed entry at index 1 authorizes an admin op; non-signer, ARBITRARY, wrong-index and non-empty msg entries do not. - Non-payable finalize: op byte 0x04 with value routes to the deposit guard and reverts before op dispatch. Fix SignEntries to finalize explicit-digest (non-empty msg) entries before computing the sig hash, since their raw signature bytes are part of the sig-hash preimage (only empty-msg entries are elided). Without this the sender's canonical-hash signature went stale whenever an index carried a non-empty msg.
F1 was derived from a stale ethereum/EIPs#12012 draft. The live spec reserves the 32-byte all-zero digest as an invalid msg, and Nethermind already rejects it in static validation (FrameTxValidation.ZeroDigestMsg), so there is no live footgun: the value check and the empty-msg requirement coincide exactly. Rework AllZeroMsgAtIndex1_PassesValueGate_ApprovesPayment into AllZeroMsgAtIndex1_RejectedByStaticValidation_ExecutionGateIsValueOnly, which now pins both layers: (a) the real end-to-end defense — static validation rejects the sponsored tx with ZeroDigestMsg before any frame runs; and (b) defense-in-depth — the paymaster's SIGPARAM(msg,1) != 0 gate is value-only, so a 32-byte zero digest would pass that gate if it ever reached execution, which it never does in a full node. Drops the earlier wording implying a strict len==0 fix would flip it to a vulnerability.
|
Claude finished @Marchhill's task in 8m 10s —— View job Review: EIP-8141 canonical paymaster assembler + test suite
Test-only, additive change (2 new files, no production code touched). The work is careful and the bytecode is correct — I verified it byte-for-byte. Findings are about enforcement and coverage, not correctness of what's here. Verification performedI hand-decoded
I also checked the stack discipline in I could not run FindingsCritical: none. High: none. Medium
Low
Notes, not findings
VerdictNot blocking on correctness — the assembler and the bytecode are right, and I checked them independently rather than taking the PR's word for it. Worth landing after the three Medium items: the drift guard is a few lines, and the two coverage gaps are branches the canonical bytecode explicitly implements but nothing exercises. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if __name__ == '__main__': | ||
| code = assemble() | ||
| print("length:", len(code), "bytes") | ||
| print("bytecode:", "0x" + code.hex()) | ||
| print("codehash:", "0x" + keccak256(code).hex()) |
There was a problem hiding this comment.
Medium — nothing enforces script ↔ constant agreement.
Eip8141CanonicalPaymasterTests documents that PaymasterRuntimeHex/CanonicalCodeHash "must stay in sync with that script", but nothing checks it: the script is not run by CI, and the C# test only asserts keccak(hex) == pinned hash — which holds no matter what the script emits. If someone edits PROGRAM and forgets to re-paste the hex, the drift is silent, and the script then prints a code hash that disagrees with the mempool recognition anchor.
Cheapest fix: make the script self-checking so running it fails loudly.
| if __name__ == '__main__': | |
| code = assemble() | |
| print("length:", len(code), "bytes") | |
| print("bytecode:", "0x" + code.hex()) | |
| print("codehash:", "0x" + keccak256(code).hex()) | |
| EXPECTED_LENGTH = 355 | |
| EXPECTED_CODEHASH = '0xda42f0d11838c4c0c3129b8b8e93e9718127ad6b315e517e1088125707c4d45c' | |
| if __name__ == '__main__': | |
| code = assemble() | |
| codehash = '0x' + keccak256(code).hex() | |
| assert len(code) == EXPECTED_LENGTH, f'length drifted: {len(code)} != {EXPECTED_LENGTH}' | |
| assert codehash == EXPECTED_CODEHASH, f'code hash drifted: {codehash}' | |
| print("length:", len(code), "bytes") | |
| print("bytecode:", "0x" + code.hex()) | |
| print("codehash:", codehash) |
Stronger alternative worth considering: build the runtime in C# (the repo already has Prepare.EvmCode, used in FrameTxProcessorTests) so CI enforces assembly → hash on every run and the Python drops out entirely.
| private static readonly TxFrame PayFrame = | ||
| new(TxFrame.ModeVerify, TxFrame.ApprovePayment, Paymaster, gasLimit: 15_000, UInt256.Zero, default); |
There was a problem hiding this comment.
Medium — missing coverage: underfunded paymaster.
InstructionApprove reverts the pay frame when the payer cannot cover the transaction:
// EvmInstructions.FrameTx.cs:61
if (vm.WorldState.GetBalance(resolvedTarget) < ctx.MaxCost) return EvmExceptionType.Revert;That is the most security-relevant branch for a paymaster specifically — the whole point of the deposit/withdrawal machinery is that the balance backs the sponsorship — and none of the 22 tests exercises it. Every test that reaches APPROVE deploys with 1.Ether.
Suggest adding a case that deploys the paymaster with a balance below MaxCost (sum of frame gas limits × 1 wei here) and asserts the sponsored tx is invalid, plus its mirror: a deposit that brings the balance above MaxCost makes the same tx valid. That also gives test 5's deposit path an end-to-end purpose.
| TxReceipt fromNonSigner = ProcessBlock(BlockTimestamp, AdminTx(NonSignerKey, 0, WithdrawalCall(500)))[0]; | ||
| Assert.That(fromNonSigner.StatusCode, Is.EqualTo(StatusCode.Failure), "a non-signer cannot initiate"); | ||
| Assert.That(Slot(2), Is.EqualTo(UInt256.Zero), "a rejected initiate creates no pending action"); | ||
|
|
||
| ProcessBlock(BlockTimestamp, AdminTx(SignerKey, 0, WithdrawalCall(500))); | ||
| TxReceipt whilePending = ProcessBlock(BlockTimestamp, AdminTx(SignerKey, 1, RotationCall(NewSigner)))[0]; | ||
| Assert.That(whilePending.StatusCode, Is.EqualTo(StatusCode.Failure), "a second action while one is pending is rejected"); | ||
| Assert.That(Slot(3), Is.EqualTo(UInt256.Zero), "the blocked rotation writes nothing"); | ||
| Assert.That(Slot(1), Is.EqualTo((UInt256)500), "the pending withdrawal is untouched"); | ||
| } |
There was a problem hiding this comment.
Medium — missing coverage: the zero-argument initiate guards.
Both initiate paths contain an explicit DUP1 ISZERO JUMPI FAIL guard (bytecode offsets 146–151 for W_INIT, and the mirror in R_INIT; paymaster_asm.py:94 and :106), rejecting amount == 0 and new_signer == 0. Neither is covered, so a mutation that deletes both guards passes all 22 tests — a rotation to address(0) would brick the instance, which is exactly the kind of thing a conformance suite should pin.
Since this test already covers the other two initiate rejections, extending it (or adding two [TestCase]s) is cheap:
ProcessBlock(BlockTimestamp, AdminTx(SignerKey, n, WithdrawalCall(0))) // expect Failure, Slot(2) == 0
ProcessBlock(BlockTimestamp, AdminTx(SignerKey, n, RotationCall(Address.Zero))) // expect Failure, Slot(3) == 0While there: an unrecognised op byte (e.g. 0x05) falling through to FAIL is also untested.
| private static readonly Address NonSigner = TestItem.PrivateKeyC.Address; | ||
| private static readonly PrivateKey NonSignerKey = TestItem.PrivateKeyC; |
There was a problem hiding this comment.
Low — NonSigner and Recipient are the same account.
TestItem.AddressC is PublicKeyC.Address, i.e. TestItem.PrivateKeyC.Address (TestItem.cs:74,81,97). So NonSigner == Recipient (line 66), even though they read as two distinct actors: the action-frame target and the unauthorized caller.
No test currently produces a false pass from this, but it is a latent trap — Fund(NonSigner) in AdminInitiate_RejectsNonSignerAndPendingAction also funds the action-frame recipient, and any future balance assertion on Recipient would be polluted by non-signer activity. Prefer an unrelated key (e.g. TestItem.PrivateKeyG) for NonSigner.
| // Middle byte of the DELAY constant (PUSH3 0x015180) in the withdrawal-initiate path — a mutation | ||
| // here changes the code hash without touching the validation path. | ||
| private const int NearMatchMutationOffset = 158; |
There was a problem hiding this comment.
Low — derive the mutation offset instead of hard-coding it.
I verified 158 is correct today: the set_unlock() PUSH3 0x015180 sits at offsets 156–159, so 158 is the 0x51 middle byte. But this is a magic index into a 355-byte blob — any change to the program above it silently moves it, and the test would then mutate some unrelated byte while still passing (both assertions stay true almost anywhere), quietly losing the "keeps behaving, hash differs" property it is meant to pin.
Locating the 62 01 51 80 sequence at runtime makes it self-maintaining:
// Middle byte of the DELAY constant (PUSH3 0x015180) in the withdrawal-initiate path — a mutation
// here changes the code hash without touching the validation path.
private static readonly int NearMatchMutationOffset =
PaymasterCode.AsSpan().IndexOf<byte>([0x62, 0x01, 0x51, 0x80]) + 2;(PaymasterCode is declared below, so it needs to move above this field.)
| // A codeless-equivalent contract that unconditionally reverts on any call (PUSH0 PUSH0 REVERT), | ||
| // used as a withdrawal target that rejects the incoming value. | ||
| private static readonly byte[] RevertOnCallCode = [0x5f, 0x5f, 0xfd]; |
There was a problem hiding this comment.
Low — the comment contradicts the code. This is not "codeless-equivalent"; it is a contract with code (PUSH0 PUSH0 REVERT) whose whole purpose is to be reached and to revert — the opposite of codeless (a codeless target would accept the value).
| // A codeless-equivalent contract that unconditionally reverts on any call (PUSH0 PUSH0 REVERT), | |
| // used as a withdrawal target that rejects the incoming value. | |
| private static readonly byte[] RevertOnCallCode = [0x5f, 0x5f, 0xfd]; | |
| // A contract that unconditionally reverts on any call (PUSH0 PUSH0 REVERT), used as a | |
| // withdrawal target that rejects the incoming value. | |
| private static readonly byte[] RevertOnCallCode = [0x5f, 0x5f, 0xfd]; |
| """Canonical paymaster runtime assembler - EIP-8141 (Stage 2, finalized). | ||
|
|
||
| Opcode stack orders verified against Nethermind.Evm/Instructions/EvmInstructions.FrameTx.cs | ||
| on branch eip8141-frame-txs-devnet7: | ||
|
|
||
| APPROVE (0xaa): pops (offset, length, scope) top-to-bottom -> push scope, length, offset. | ||
| ApprovePayment scope bit = 0x1 (TxFrame.ApprovePayment), NOT 0x2. | ||
| SIGPARAM (0xb4): pops (signatureIndex, param) top-to-bottom -> push param, then index. | ||
| param 0x00 resolved_signer, 0x01 scheme, 0x02 msg. Scheme ARBITRARY = 0x0. | ||
|
|
||
| Spec (ethereum/EIPs #12012, 2026-07-24 update mirrored): | ||
| - validation scheme gate is `scheme != ARBITRARY` (no named curve). | ||
| - admin authorized() = CALLER == slot0 OR a signer-signed entry at index 1 | ||
| (scheme != ARBITRARY, resolved_signer == slot0, empty msg). | ||
| """ |
There was a problem hiding this comment.
Low — two doc/convention nits.
- Missing SPDX header. Tracked Python in this repo carries one (
scripts/superchain.py,scripts/evm_benchmark_utils.py):# SPDX-FileCopyrightText: 2026 Demerzel Solutions Limited # SPDX-License-Identifier: LGPL-3.0-only
- The docstring pins the verification against "branch
eip8141-frame-txs-devnet7". Per AGENTS.md, comments should read correctly against master, where that branch name is meaningless. Citing the file (EvmInstructions.FrameTx.cs) is enough — drop the branch.
| def encode(item): | ||
| kind = item[0] | ||
| if kind == 'op': | ||
| return bytes([OPS[item[1]]]) | ||
| if kind == 'push': | ||
| return push_bytes(item[1]) | ||
| if kind in ('jumpi', 'jump'): | ||
| return bytes([0x61, 0, 0]) + bytes([OPS['JUMPI'] if kind == 'jumpi' else OPS['JUMP']]) # PUSH2 <hi><lo> + JUMP(I) | ||
| if kind == 'dest': | ||
| return bytes([OPS['JUMPDEST']]) | ||
| raise ValueError(item) | ||
|
|
||
|
|
||
| def assemble(): | ||
| # Pass 1: assign offsets, collect label addresses (all label refs are fixed-width PUSH2). | ||
| labels = {} | ||
| offset = 0 | ||
| for item in PROGRAM: | ||
| if item[0] == 'dest': | ||
| labels[item[1]] = offset | ||
| offset += len(encode(item)) | ||
|
|
||
| # Pass 2: emit, patching PUSH2 operands with resolved addresses. | ||
| out = bytearray() | ||
| for item in PROGRAM: | ||
| if item[0] in ('jumpi', 'jump'): | ||
| addr = labels[item[1]] | ||
| op = OPS['JUMPI'] if item[0] == 'jumpi' else OPS['JUMP'] | ||
| out += bytes([0x61, (addr >> 8) & 0xff, addr & 0xff, op]) | ||
| else: | ||
| out += encode(item) | ||
| return bytes(out) |
There was a problem hiding this comment.
Low — dead jump support. PROGRAM contains no ('jump', …) item, so the 'jump' arms in encode (line 155) and assemble (line 174) plus OPS['JUMP'] are unreachable. AGENTS.md asks for unused code to be removed; dropping them makes both passes read as "every label ref is a fixed-width PUSH2 + JUMPI", which is the actual invariant the two-pass scheme relies on.
| public void NearMatchBytecode_BehavesButCodeHashDiffers() | ||
| { | ||
| Assert.That(Keccak.Compute(PaymasterCode), Is.EqualTo(new Hash256(CanonicalCodeHash)), | ||
| "the assembled bytecode must hash to the pinned canonical code hash"); | ||
|
|
||
| byte[] nearMatch = (byte[])PaymasterCode.Clone(); | ||
| nearMatch[NearMatchMutationOffset] ^= 0x01; | ||
| Assert.That(Keccak.Compute(nearMatch), Is.Not.EqualTo(new Hash256(CanonicalCodeHash)), | ||
| "a one-opcode mutation must fail exact-hash recognition"); |
There was a problem hiding this comment.
Low — split the canonical-hash pin into its own test.
Line 287 is the single most important assertion in the file (the mempool recognition anchor), but it is buried inside a test about near-match bytecode. If the assembler output ever changes, the failure surfaces as NearMatchBytecode_BehavesButCodeHashDiffers — a name that actively misdirects. A standalone AssembledBytecode_MatchesCanonicalCodeHash would fail with the right story, and would keep failing independently of whatever the near-match case does.
| [Test] | ||
| public void MaturedFinalize_AmountExceedsBalance_RevertsAndPreservesPending() | ||
| { | ||
| Fund(ThirdParty); | ||
| DeployPaymaster(PaymasterCode, Signer, 1.Ether); | ||
| UInt256 amount = 2.Ether; | ||
| SetPending(amount, BlockTimestamp); | ||
|
|
||
| AssertFinalizeRevertsAndPreservesPending(BlockTimestamp, amount, BlockTimestamp); | ||
| } | ||
|
|
||
| // 14. Same revert-and-preserve guarantee when the signer is a contract that reverts on receiving | ||
| // the withdrawal value: the CALL returns failure and the finalize rolls back its slot clearing. | ||
| [Test] | ||
| public void MaturedFinalize_SignerRejectsValue_RevertsAndPreservesPending() | ||
| { | ||
| Fund(ThirdParty); | ||
| DeployContract(RevertingReceiver, RevertOnCallCode, UInt256.Zero); | ||
| DeployPaymaster(PaymasterCode, RevertingReceiver, 1.Ether); | ||
| UInt256 amount = 100; | ||
| SetPending(amount, BlockTimestamp); | ||
|
|
||
| AssertFinalizeRevertsAndPreservesPending(BlockTimestamp, amount, BlockTimestamp); | ||
| } |
There was a problem hiding this comment.
Low — these two differ only by setup; parameterize per the DRY test rule.
AGENTS.md: "When tests differ only by inputs and expected outputs, parameterize a single test with [TestCase(...)] or [TestCaseSource(...)]." Tests 13 and 14 already share the assertion helper; only the deploy/amount differ, so a [TestCaseSource] (as done for ReentrancyCases) collapses them.
Also, AssertFinalizeRevertsAndPreservesPending(ulong finalizeTimestamp, UInt256 amount, ulong unlockTime) receives the same value for finalizeTimestamp and unlockTime at both call sites, so that parameterization is currently unused surface — one ulong timestamp would express the intent ("finalize exactly at maturity") more directly.
- assembler self-checks output length and code hash so PROGRAM edits fail loudly instead of drifting from the pinned constants - cover the underfunded-paymaster solvency gate and its deposit-backed mirror, giving the deposit path an end-to-end purpose - fold the two matured-finalize revert cases into a TestCaseSource and drop the always-equal timestamp parameter
|
Addressed the three findings in a188ebd:
Build clean; |
|
Claude finished @Marchhill's task in 4m 13s —— View job Re-review:
|
| # | Prior finding | Sev | Status |
|---|---|---|---|
| 1 | Nothing enforces script ↔ constant agreement | Medium | ✅ Closed. EXPECTED_LENGTH/EXPECTED_CODEHASH asserted in __main__; EXPECTED_CODEHASH is byte-identical to CanonicalCodeHash. One residual window remains — see inline (Low, follow-up). |
| 2 | No coverage of the underfunded paymaster | Medium | APPROVE gate from the charge-time gate — see below. |
| 3 | No coverage of the zero-argument initiate guards | Medium | ❌ Still open, no rationale given. |
| 8 | Tests 13/14 differ only by setup | Low | ✅ Closed. [TestCaseSource(nameof(FinalizeRevertCases))], and AssertFinalizeRevertsAndPreservesPending down to one timestamp. Correctly gated: the RevertingReceiver deploy only happens for the case that needs it. |
| 4, 5, 6, 7, 9, 10 | NonSigner == Recipient; magic offset 158; hash pin buried in the near-match test; "codeless-equivalent" comment; SPDX + branch pin in the docstring; dead 'jump' arms |
Low | Unchanged — all still present. Fine to leave; none is a correctness risk. |
New findings
Medium
-
UnderfundedPaymaster_PayFrameReverts_TxInvalidpasses with the gate it claims to pin removed (inline). There are two independentMaxCostgates:EvmInstructions.FrameTx.cs:61reverts the frame, andTransactionProcessorBase.FrameTx.cs:518silently declines to setPayer.TransactionExecuted is Falseis true under both. Delete:61and the pay frame succeeds,:518bails,Payerstays null, the tx fails with"frame transaction never set a payer"— same observable, test still green. Assertingresult.ErrorDescription == "VERIFY frame reverted"(:210vs:270carry distinct strings) makes it discriminating. Cheap, and it's the difference between the fix landing and appearing to land. Fix this → -
Prior Medium Netcore #3 is still open. The PR comment says "addressed the three findings", but the third item listed was the DRY fold — a Low. The zero-argument initiate guards (
paymaster_asm.py:94,106—DUP1 ISZERO JUMPI FAILin bothW_INITandR_INIT) remain uncovered: deleting both still passes the suite, andRotationCall(Address.Zero)bricks the instance. Two[TestCase]s on the existingAdminInitiate_RejectsNonSignerAndPendingActionclose it. Happy to treat it as deliberately deferred if you'd rather scope it out — just say so and I'll drop it. Fix this →
Low
MaxCostis not the frame-gas sum (inline).maxCost = (intrinsic + Σ frame.gas_limit) × maxFeePerGas(TransactionProcessorBase.FrameTx.cs:68,82), so the threshold is ~350k, not the 315,000 stated at lines 55–56 and 511. No behavioural impact (1,000 ≪, 1,001,000 ≫), but a reader sizing a deposit at 320,000 from that comment would be wrong.- Residual drift window in the Python pin — inline. Follow-up, not this PR.
- Nit:
MaxturedFinalize_FailedWithdrawalCall_...takes abool signerRejectsValuethat drives control flow (? RevertingReceiver : Signerplus a conditional deploy). Passing the signer address (ornull) as the case parameter would read as data rather than a flag. Marginal — the current form is clear enough.
What I verified
- Diff read in full; only the two test-project files change, nothing outside
Nethermind.Evm.Test. No hot-path or consensus impact. EXPECTED_CODEHASH(Python) is character-for-character equal toCanonicalCodeHash(C#), andPaymasterRuntimeHexis untouched by this commit — so the byte-level verification from the previous review still stands.- Both
MaxCostgates read from source, plus the two failure paths (:210,:270) that make finding 1 concrete, and themaxCostformula behind finding 3. - Test 20's arithmetic: the deposit lands on top of
UnderfundedBalance, and 1,001,000 clears the real threshold with margin.
I could not run python3 or dotnet build/dotnet test here (both need approval), so the 24/24 green and "assembler still emits 355 bytes / 0xda42f0d1…c4d45c" claims are from your run, not reproduced by me. The Python asserts are textually what was suggested, and CI runs the C# fixture.
Verdict
Good iteration. Not blocking on correctness — nothing here is wrong. Blocking as not-yet-mergeable only on the two Mediums above, both small: one assertion line in test 19, and two [TestCase]s for the zero-argument guards (or an explicit "deferred" from you).
• branch paymaster-stage2
| TransactionResult result = ExecuteInvalid(tx, BlockTimestamp); | ||
|
|
||
| Assert.That(result.TransactionExecuted, Is.False); |
There was a problem hiding this comment.
Medium — this test passes even with the gate it claims to pin removed.
There are two independent solvency gates against MaxCost:
// EvmInstructions.FrameTx.cs:61 — reverts the frame
if (vm.WorldState.GetBalance(resolvedTarget) < ctx.MaxCost) return EvmExceptionType.Revert;
// TransactionProcessorBase.FrameTx.cs:518 — silently declines to set Payer
if (WorldState.GetBalance(resolvedTarget) < frameContext.MaxCost) return;TransactionExecuted is False cannot tell them apart. Delete the APPROVE gate at :61 and the pay frame succeeds, ApplyPaymentApproval then bails at :518, Payer stays null, and the tx fails with "frame transaction never set a payer" — still TransactionExecuted == False, so this test stays green. The comment promises "APPROVE reverts when the instance holds less than MaxCost", but nothing here pins that.
The two paths carry distinct ErrorDescriptions (:210 vs :270), so one extra assertion makes the test actually discriminating:
| TransactionResult result = ExecuteInvalid(tx, BlockTimestamp); | |
| Assert.That(result.TransactionExecuted, Is.False); | |
| TransactionResult result = ExecuteInvalid(tx, BlockTimestamp); | |
| Assert.That(result.TransactionExecuted, Is.False); | |
| Assert.That(result.ErrorDescription, Is.EqualTo("VERIFY frame reverted"), | |
| "the APPROVE solvency gate reverts the pay frame, rather than the charge-time gate silently leaving the payer unset"); |
| // Below MaxCost for the sponsored tx (frame gas limits sum to 315,000 at 1 wei/gas), so an | ||
| // instance deployed with this balance cannot back the sponsorship until a deposit tops it up. | ||
| private static readonly UInt256 UnderfundedBalance = 1_000; |
There was a problem hiding this comment.
Low — MaxCost is not the frame-gas sum.
TransactionProcessorBase.FrameTx.cs:68,82:
ulong txGasLimit = intrinsicGas + totalFrameGas;
UInt256 maxCost = (UInt256)txGasLimit * tx.DecodedMaxFeePerGas + ...;So MaxCost == (intrinsic + 315,000) × 1, not 315,000 — intrinsic here is the 21,000 base plus per-frame, calldata and signature-verification components. Both this comment and the one at line 511 ("clears the 315,000-wei sum of the frame gas limits") state the frame-gas sum as the threshold. Harmless today (1,000 is far below, 1,001,000 far above), but it misstates the bound a future reader would size a deposit against — 320,000 would look sufficient and would not be. Suggest wording the threshold as intrinsic + Σ frame.gas_limit at 1 wei/gas in both places.
| # Pinned output. Must equal PaymasterRuntimeHex/CanonicalCodeHash in Eip8141CanonicalPaymasterTests.cs; | ||
| # asserted below so editing PROGRAM without re-pasting the hex fails loudly instead of drifting silently. | ||
| EXPECTED_LENGTH = 355 | ||
| EXPECTED_CODEHASH = '0xda42f0d11838c4c0c3129b8b8e93e9718127ad6b315e517e1088125707c4d45c' |
There was a problem hiding this comment.
The drift guard does what was asked, and I confirmed EXPECTED_CODEHASH is byte-identical to CanonicalCodeHash in the C# fixture. Low, residual: it closes the forgot-to-re-paste window but not the updated-both-Python-constants one — edit PROGRAM, update EXPECTED_LENGTH/EXPECTED_CODEHASH, and the C# PaymasterRuntimeHex/CanonicalCodeHash pair remains self-consistent on the old bytecode, so both sides pass while describing different contracts. Nothing to do now (the comment on line 238 names the C# constants, which is the right pointer for a human), but the only structural close is still assembling in C# so one artifact produces both. Worth a follow-up issue rather than more Python.
Add a parameterized case that a zero-amount withdrawal, a rotation to
address(0), and an unrecognised op byte all revert without opening a
pending action. Tighten the underfunded-paymaster test to assert the
APPROVE balance gate reverts the pay frame ("VERIFY frame reverted"),
distinguishing it from the charge-time gate.
|
Addressed the residual re-review items in 7692928:
Build: 0 errors/0 warnings. |
|
Claude finished @Marchhill's task in 3m 6s —— View job Re-review:
|
| # | Prior finding | Sev | Status |
|---|---|---|---|
| Re-review 1 | Test 19 passes with the gate it pins removed | Medium | ✅ Closed. |
| Re-review 2 / first-review 3 | Zero-argument initiate guards uncovered | Medium | ✅ Closed. |
| Re-review 4 | Residual assembler drift window | Low | ✅ Deferred with rationale — accepted, matches my own conclusion (the only structural close is porting the assembler to C#). |
| Re-review 3 | MaxCost is not the frame-gas sum |
Low | ❌ Still open — see below. |
| First review 4, 5, 6, 7, 9, 10 | NonSigner == Recipient; magic offset 158; hash pin buried in the near-match test; "codeless-equivalent" comment; SPDX + branch pin in the docstring; dead 'jump' arms |
Low | Unchanged. None is a correctness risk. |
Verification of the new coverage
PaymasterRuntimeHex is byte-identical to the previous commit, so I re-decoded the relevant regions from the pinned hex directly rather than trusting the Python:
- Dispatch (
5f3560f81c 8060011461005e57 8060021461 00a557 806003146100ec57 60041461012357 5b5f5ffd) — threeDUP1-guarded comparisons for ops 1/2/3, then a consumingEQfor op 4, falling straight through into theFAILJUMPDESTat0x5a. Case 3 (0x05) reachesFAILby fallthrough as claimed.CALLDATASIZE == 1is nonzero, so the 1-byte calldata routes toADMIN, andCALLDATALOAD(0)zero-pads — the op byte reads as0x05. W_INITguard (6001 35 80 15 61005a57) —PUSH1 1 / CALLDATALOAD / DUP1 / ISZERO / JUMPI FAIL. Present and reached.R_INITmirrors it with600355.- Mutation-discrimination, case by case: deleting either guard makes the initiate succeed, writing
slot2 = TIMESTAMP + DELAY. BothStatusCode.FailureandSlot(2) == 0flip. Confirmed the tests are not vacuous.
For test 19: TransactionResult.ErrorDescription is the raw WithDetail string, not a composed one (TransactionProcessor.cs:1829,1835), and "VERIFY frame reverted" occurs exactly once in the tree (TransactionProcessorBase.FrameTx.cs:210), distinct from "frame transaction never set a payer" (:270). With UnderfundedBalance = 1_000 < MaxCost, EvmInstructions.FrameTx.cs:61 reverts the VERIFY frame and :210 produces that exact string. Deleting :61 yields the other string and the test fails. The assertion does what it says.
Findings
Critical: none. High: none. Medium: none.
Low (carried over, unaddressed)
MaxCoststill described as the frame-gas sum — lines 55–56 (frame gas limits sum to 315,000 at 1 wei/gas) and 516 (clears the 315,000-wei sum of the frame gas limits).maxCost = (intrinsic + Σ frame.gas_limit) × maxFeePerGas, so the real threshold is ~350k. No behavioural impact (1,000 ≪, 1,001,000 ≫), but a reader sizing a deposit at 320,000 from these comments would be wrong. Two-word fix; flagging that it was neither changed nor mentioned. Fix this →
Notes, not findings
InitiateGuardRejectionCasesis named for the two guards but also carries the unknown-op fallthrough case, which is not a guard. Cosmetic.- In the rotation case,
Slot(3) == 0is not itself discriminating (a removed guard would storeaddress(0)into slot 3, i.e. still zero).StatusCode.FailureandSlot(2) == 0are what catch it — both present, so the case is sound. Worth knowing if anyone later prunes assertions. - The
"VERIFY frame reverted"literal is now duplicated between production and test, so a reword breaks the test. Extracting aconstwould need a production edit, which is outside this test-only PR's scope — fine as-is.
What I could and could not run
Diff read in full; only the two test-project files change vs the merge base (7dd8c76), nothing outside Nethermind.Evm.Test. No hot-path, consensus, or DI impact. paymaster_asm.py is untouched by this commit, so the byte-level assembler verification from the first review still stands.
python3 and dotnet build/dotnet test both require approval in this environment, so the 27/27 green and 0 errors/0 warnings claims are yours, not reproduced here. The C# fixture is CI-enforced; the Python drift asserts are not, by design.
Verdict
Mergeable. Both Mediums are genuinely closed — I checked the mutations, not just the presence of the tests. The one remaining Low is a comment wording issue; land it whenever convenient.
• branch paymaster-stage2
1 participant
Changes
paymaster_asm.py, a reproducible two-pass assembler for the EIP-8141 canonical paymaster. It emits the 355-byte runtime bytecode and its code hash0xda42f0d11838c4c0c3129b8b8e93e9718127ad6b315e517e1088125707c4d45c.Eip8141CanonicalPaymasterTests, a 22-test conformance suite exercising the canonical paymaster's payment authorization, deposit, withdrawal/rotation timelocks, admin routes (via caller and via signature), reentrancy protection, CALL-failure handling, static-validation rejection of the all-zero message, and non-payable finalize paths.Reference spec: ethereum/EIPs#12012
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
22/22 tests in
Eip8141CanonicalPaymasterTestspass. The assembler deterministically reproduces the 355-byte runtime and code hash.Documentation
Requires documentation update
Requires explanation in Release Notes