Skip to content

fix(stateless): bind supplied public keys to transaction signatures - #13785

Open
LukaszRozmej wants to merge 2 commits into
masterfrom
fix/stateless-verify-transaction-public-keys
Open

LukaszRozmej wants to merge 2 commits into
masterfrom
fix/stateless-verify-transaction-public-keys

Conversation

@LukaszRozmej

@LukaszRozmej LukaszRozmej commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Changes

  • Verify each stateless input's transaction public key against the signature it belongs to, and derive the sender from the recovered key, instead of deriving it from the supplied key directly.
  • Reject the block when a key does not match, including a wrong SEC1 prefix.
  • Cover the check with a parameterized test over an unmodified key, a different signer's key, a flipped byte, and a wrong prefix.

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

Nethermind.Consensus.Test 389/389. Mutation-checked: with the executor change reverted, three of the new test's four cases fail, so the test does catch the behaviour it describes.

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

Remarks

The keys are a hint that lets a guest skip recovery, and the spec (recover_sender_from_public_key) requires verifying them: an implementation may skip full recovery but must still bind the key to the signature and to the recovery id, because a signature's other recovery candidate verifies on its own and derives a different sender.

This PR takes the reference approach - recover and compare - so the hint currently buys nothing beyond a 64-byte comparison. A verify primitive that also binds the y-parity would make it a saving again (verify costs less than recovery by a point decompression and an inversion); Accelerators.SecP256k1Verify exists but is parity-blind, so using it today would reintroduce the sibling-candidate gap. Worth raising in the zkVM standards work, together with whether the schema should carry EIP-7702 authorization keys too - those are recovered in-guest today and have no hint.

Guest cost on ZisK, block 25532382 (1232 transactions), same worktree and toolchain for both arms, both producing the pinned expected output: +17,249,781 steps (+5.64%) and +2,360,595,500 cost (+6.48%), i.e. ~14,000 steps per transaction. Per-bucket numbers are in the comment below; the cost is the secp256k1 accelerator calls plus the signing-hash RLP encode the guest did not do before.

The stateless input carries one uncompressed public key per transaction.
The guest derived the sender from that key directly, so the key was never
tied to the signature it claims to belong to.

Recover the key from the signature and compare all 65 bytes before taking
the sender from it, as the stateless spec's recover_sender_from_public_key
requires. Comparing against the recovered key also pins the recovery id:
a signature's other recovery candidate verifies on its own and would name
a different sender.
@claude

claude Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @LukaszRozmej's task in 1m 9s —— View job


Review of PR #13785: bind supplied public keys to transaction signatures

  • Read diff
  • Check the executor context: key length and prefix, how ecdsa recovery works (std and zkEVM), chain-id handling, and downstream validation
  • Post review

Verdict: no Critical, High or Medium findings. One Low comment on the test is posted inline.

Correctness and security

  • The fix works. The old code took the sender straight from publicKeys[i][1..] without checking it. That let a prover name any sender for any transaction, and since the block hash only commits to the signed transactions, the executor could be made to accept the wrong state transition. Now the executor recovers the key from the signature, compares it to the supplied one, and takes the sender from the recovered key. This closes the gap.
  • The recovery id is pinned. Recovery uses the recovery id that comes from v. Comparing the full 64-byte key then rules out the signature's other recovery candidate, as the XML remark says.
  • The 0x04 prefix check is needed. SszPublicKey has a fixed length of 65 bytes, so declared[0] and declared[1..] are always in bounds. Without the prefix check, a key with a wrong prefix but correct X and Y would have been accepted.
  • Chain-id handling matches the rest of the code. !spec.ValidateChainId is the same flag that TransactionProcessor, BlockValidator.ValidateTransactions and RecoverSignatures use. The key check therefore agrees with the sender normal processing would recover.
  • Malleability is still covered. Low-s (EIP-2) and other signature-shape checks still run later in TxValidator.IsWellFormed, inside ValidateSuggestedBlock. Recovery here does not bypass them.
  • Unsigned transactions fail cleanly. The Signature is null early return avoids the InvalidDataException that RecoverPublicKey(tx) would throw. That exception would have been caught anyway, but on zkVM there is no exception unwinding (see the FailureOutput remarks), so returning false is the better path.
  • The zkEVM build is fine. EthereumEcdsa.zkevm.cs RecoverPublicKey uses Accelerators.SecP256k1Recover with the explicit recovery id, so it is parity-bound as expected.

Performance

  • This adds one ecrecover per transaction in the guest, which the PR already calls out. Once SenderAddress is set, TransactionProcessor and BlockValidator skip their own recovery, so no work is repeated after this step.
  • One PublicKey is allocated per transaction. That is negligible next to the recovery itself.

Low

  • Test duplication. The new test repeats the setup, the re-encode splice and the "valid" case of Encoded_execution_checks_reconstructed_header_hash. Adding the three key mutations to that test would remove the duplication and also exercise the Amsterdam payload. Inline comment · Fix this →

Comment quality

The added comments are good. The XML summary and remarks on TryAssignSenders explain the recovery-id binding (the why), and the doc on UncompressedPublicKeyPrefix names the SEC1 form. Nothing restates the code.

I did not build the code or run tests in this job; the review is from reading the diff and the surrounding code.

@LukaszRozmej

Copy link
Copy Markdown
Member Author

Guest cost on ZisK — block 25532382 (1232 transactions)

Both arms built in the same worktree with the same toolchain (bflat-riscv64-11 @ e48bd7b, -Ot --no-stacktrace-data --ldflags=--strip-all) and run on ziskemu 1.3.0-alpha. Both produce the pinned expected output d38ffa06…a833 + 0101…0100, so the block still validates with the check in place.

base (ce501a9734) with the check Δ
STEPS 305,711,105 322,960,886 +17,249,781 (+5.64%)
TOTAL cost 36,454,175,082 38,814,770,582 +2,360,595,500 (+6.48%)
MAIN 20,788,355,140 21,961,340,248 +1,172,985,108 (+5.64%)
OPCODES 4,450,590,329 4,728,001,715 +277,411,386 (+6.23%)
PRECOMPILES 7,509,717,320 8,125,058,328 +615,341,008 (+8.19%)
MEMORY 3,418,202,469 3,713,060,467 +294,857,998 (+8.63%)
FROPS 3,294,913,160 3,527,712,089 +232,798,929 (+7.07%)
BASE 287,309,824 287,309,824 0

That is ~14,000 steps and ~1.9M cost units per transaction, and it is not only the ecrecover: PRECOMPILES carries the secp256k1 accelerator calls and the extra keccak, while MAIN and MEMORY carry the signing-hash RLP re-encode per transaction, which the guest did not do at all before.

Two directions if this proves too expensive:

  • A verify primitive that also binds the recovery id (y-parity of R) would drop the point decompression and the inversion, but keep the signing-hash encode. Accelerators.SecP256k1Verify exists today and is parity-blind, so it cannot be used as-is without reopening the sibling-candidate gap.
  • The signing-hash encode is unavoidable per the spec, but it is the same RLP the transaction hash already walks; worth checking whether the two can share a pass.

@rubo rubo left a comment

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.

Perhaps the initial approach of recovering addresses without relying on input pub keys would be the same or even slightly cheaper.

This branch has not been deployed

No deployments
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.

2 participants