Conversation
Recover each transaction's sender in apply_block(), the way the state test runner already does, and ignore the JSON "sender" field. A transaction whose signature recovers no sender is now rejected as TransactionException.INVALID_SIGNATURE_VRS instead of executing as the sender the input names. evmone was the only t8n taking that field: the execution specs, geth, erigon, besu, nimbus and ethereumjs all recover. The blockchain test runner opts out, for the reason its own TODO gives. recover_sender() rejects a v no signature has as well (a typed v above 1, a legacy one other than 27, 28 or EIP-155's 35 + 2 * chain_id + y_parity). The RLP decoder bounds v already, but a transaction read from JSON carries whatever v the input names. A legacy transaction's chain id is read from its v rather than taken from --state.chainid. Overwriting it made the EIP-155 chain id check vacuous and built the signing preimage of a transaction meant for another chain wrong. The t8n fixtures carried signatures belonging to no sender they name, so they are re-signed with the key of the account funding them.
There was a problem hiding this comment.
🟡 Changes recommended
Sender recovery currently applies post-fork signature rules to historical revisions where they are not valid.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates evmone t8n to recover transaction senders from signatures instead of trusting JSON input.
Changes:
- Recovers senders and rejects invalid signatures during block application.
- Derives legacy chain IDs from EIP-155
v. - Adds regression tests and re-signs affected fixtures.
File summaries
| File | Description |
|---|---|
test/utils/t8n.cpp |
Derives legacy chain IDs from v. |
test/utils/blockchaintest_runner.cpp |
Disables recovery for blockchain fixtures. |
test/utils/block_transition.hpp |
Adds the sender-recovery option. |
test/utils/block_transition.cpp |
Recovers senders before execution. |
test/unittests/tooling_t8n_test.cpp |
Adds signature and chain-ID tests. |
test/state/transaction.hpp |
Documents stricter recovery behavior. |
test/state/transaction.cpp |
Validates v during recovery. |
test/integration/evmone-cli/t8n/prague_pre_state_only/txs.json |
Re-signs the fixture transaction. |
test/integration/evmone-cli/t8n/prague_empty_requests/txs.json |
Re-signs the fixture transaction. |
test/integration/evmone-cli/t8n/osaka_blob_schedule/txs.json |
Re-signs the fixture transaction. |
test/integration/evmone-cli/t8n/CMakeLists.txt |
Updates expected hashes and RLP. |
test/integration/evmone-cli/t8n/cancun_create_tx/txs.json |
Re-signs the fixture transaction. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| if (opts.recover_senders) | ||
| { | ||
| const auto sender = state::recover_sender(tx, txbytes); |
Contributor
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1711 +/- ##
==========================================
- Coverage 98.02% 98.02% -0.01%
==========================================
Files 179 179
Lines 16357 16430 +73
Branches 3763 3773 +10
==========================================
+ Hits 16034 16105 +71
- Misses 243 244 +1
- Partials 80 81 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
evmone t8ntook each transaction's sender from the JSON"sender"field and never validated the signature:from_json_tx_commonreads it, andvalidate_transactionnever touchesv,rors. A transaction whose signature recovers no public key executed as the sender the input named, so no invalid-signature test could be filled with--evm-bin evmone.recover_sender()existed but was wired only to the state test runner ("Recover the signer, as a node does, instead of taking it from JSON") and the blockchain test RLP path.senderis not part of the t8n input schema: geth'stxWithKeyadds onlysecretKeyandprotectedtotypes.Transaction, whose JSON has no sender key. Checked on onetxs.json, the execution specs, geth, erigon, besu, nimbus and ethereumjs all recover; evmone was the only one taking the field. It inherited the read from the GeneralStateTest loader, wheretransaction.senderis legitimate.Recovery moves into
apply_block(), which already encodes each transaction for its hash. The blockchain test runner opts out, for the reason its own TODO gives.Two things had to follow:
recover_sender()now rejects avno signature has (a typedvabove 1, a legacy one other than 27, 28 or EIP-155's35 + 2 * chain_id + y_parity). The RLP decoder boundsvalready, so this was unreachable before; a transaction read from JSON carries whatevervthe input names, andv = 34recovered a garbage address instead of failing.vinstead of--state.chainid. Overwriting it forcedtx.chain_id == block.chain_id, making the EIP-155 check invalidate_transactionvacuous, and built the wrong signing preimage for a transaction meant for another chain.The t8n fixtures carried signatures belonging to no sender they name: the pasted
(0x468a915f…, 0x5cedae08…)pair recovers to three different addresses over the three payloads it appears on, none of them the0xa94f5374…they claim. They are re-signed with that account's key, so the allocs are untouched.max_vnow uses a validv = 0xffffffffffffffff(chain id0x7fffffffffffffee, y parity 0) rather than one that was silently accepted.Verified:
ctest1307/1307 (thebenchcases need theevm-benchmarkssubmodule). Against EEST'sfillat Prague, this branch vs master over 47,425 tests: 0 regressions, 69 fixed, exactlytest_bad_v_r_s(60) and the newtest_unrecoverable_signature(9) from ethereum/execution-specs#3573. Consumer side unchanged:evmone testpasses all 4,898 filled fixture files. EEST needs a matchingEvmoneExceptionMapperentry, which is ethereum/execution-specs#3573's to carry.https://claude.ai/code/session_019wquSXUuvWyx3N6zwNXV2v