fix(stateless): bind supplied public keys to transaction signatures - #13785
LukaszRozmej wants to merge 2 commits into
Conversation
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 finished @LukaszRozmej's task in 1m 9s —— View job Review of PR #13785: bind supplied public keys to transaction signatures
Verdict: no Critical, High or Medium findings. One Low comment on the test is posted inline. Correctness and security
Performance
Low
Comment qualityThe added comments are good. The XML summary and remarks on I did not build the code or run tests in this job; the review is from reading the diff and the surrounding code. |
Guest cost on ZisK — block 25532382 (1232 transactions)Both arms built in the same worktree with the same toolchain (
That is ~14,000 steps and ~1.9M cost units per transaction, and it is not only the Two directions if this proves too expensive:
|
rubo
left a comment
There was a problem hiding this comment.
Perhaps the initial approach of recovering addresses without relying on input pub keys would be the same or even slightly cheaper.
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Nethermind.Consensus.Test389/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
Requires explanation in Release Notes
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.SecP256k1Verifyexists 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.