test: Recover the blockchain test transaction senders - #1634
Conversation
The runner trusted the sender the fixture supplies, so a transaction whose signature the test broke executed as that sender and the block was rejected by whatever rule it happened to break. Recover from the transaction's own encoding instead, and reject it when the signature does not recover. recover_sender() now rejects a v outside its domain, which until now only decode_transaction() checked and the JSON path never runs: without it the v=34 cases of frontier/validation/bad_v_r_s recover a stranger's address and fail on its empty balance.
|
|
||
| // A typed v is {0, 1}. A legacy v is 27 + y_parity, or 35 + 2 * chain_id + y_parity (EIP-155): | ||
| // both bases are odd, so an even v means y_parity 1. | ||
| if (typed ? tx.v > 1 : (tx.v < 27 || (tx.v > 28 && tx.v < 35))) |
There was a problem hiding this comment.
Because the JSON path never decodes, and this is the only function that interprets v.
decode_transaction() bounds v while parsing (transaction.cpp:109-112 for legacy, :128 for typed), so a transaction that came from RLP can never reach here out of domain. A blockchain-test transaction is built field by field from rlp_decoded JSON, which applies no such bound, and this PR is the first caller to recover a sender from one. Without the check the v=34 cases of frontier/validation/bad_v_r_s derive y_parity = 1 from an even v, recover a stranger's address, and get rejected for its empty balance — got INSUFFICIENT_ACCOUNT_FUNDS, expected INVALID_SIGNATURE_VRS, 13 fixtures.
It sits immediately above the y_parity derivation because that is what it guards: the line below assumes v is one of the forms the comment describes.
It does duplicate the decoder's bound for now. The intended end state is the reverse — drop it from decode_transaction() so an out-of-range v is reported as INVALID_SIGNATURE rather than INVALID_ENCODING, which is what would let the over-broad {INVALID_ENCODING, "TransactionException.INVALID_SIGNATURE_VRS"} alternative come out of error_matching.cpp. That changes state-test behaviour, so I kept it out of this PR. Happy to fold it in if you would rather have it in one go.
There was a problem hiding this comment.
Pull request overview
Adds transaction sender recovery to blockchain-test execution so invalid signatures are rejected directly.
Changes:
- Adds opt-in sender recovery to block transitions.
- Enables recovery for expected-invalid blockchain fixtures.
- Rejects out-of-domain signature
vvalues.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
test/utils/block_transition.hpp |
Adds sender-recovery option. |
test/utils/block_transition.cpp |
Recovers senders before execution. |
test/state/transaction.cpp |
Validates signature v. |
test/blockchaintest/blockchaintest_runner.cpp |
Enables recovery for invalid blocks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| blob_gas_limit, {.block_reward = mining_reward(rev)}); | ||
| const auto res = apply_block(pre_state, vm, bi, block_hashes, | ||
| test_block.transactions, rev, blob_gas_limit, | ||
| {.block_reward = mining_reward(rev), .recover_senders = true}); |
| std::optional<state::Transaction> recovered_tx; | ||
| if (opts.recover_senders) | ||
| { | ||
| const auto sender = state::recover_sender(input_tx, tx_bytes); |
| std::optional<state::Transaction> recovered_tx; | ||
| if (opts.recover_senders) | ||
| { | ||
| const auto sender = state::recover_sender(input_tx, tx_bytes); | ||
| if (!sender.has_value()) | ||
| { | ||
| rejected_txs.push_back( | ||
| {computed_tx_hash, i, make_error_code(state::INVALID_SIGNATURE)}); | ||
| continue; | ||
| } | ||
| recovered_tx = input_tx; | ||
| recovered_tx->sender = *sender; | ||
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1634 +/- ##
==========================================
- Coverage 97.71% 97.70% -0.01%
==========================================
Files 171 171
Lines 15593 15602 +9
Branches 3607 3610 +3
==========================================
+ Hits 15236 15244 +8
Misses 269 269
- Partials 88 89 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The runner trusted the sender the fixture supplies, so a transaction whose signature the test broke executed as that sender and the block was rejected by whatever rule it happened to break. Recover from the transaction's own encoding instead, and reject it when the signature does not recover. This removes the exemption added in #1632 and the TODO from #1623; the two fixtures added there now exercise the recovery.
recover_sender()also had to reject avoutside its domain, a rule onlydecode_transaction()enforced and the JSON path never runs. Without it thev=34cases offrontier/validation/bad_v_r_srecover a stranger's address and fail on its empty balance instead of on the signature — 13 blockchain fixtures.Recovery is opt-in via
BlockTransitionOptions, so t8n keeps using the sender it is given.