state: Recognize chain-id protected legacy transaction - #1616
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes legacy (EIP-155) chain-id protection handling—especially the ambiguous chainId == 0 case—by keeping the legacy signature v verbatim through encode/decode and using v >= 35 as the definitive “chain-id protected” signal. It updates state-test JSON import/export to preserve v, makes RLP decoding the inverse of encoding for legacy v, and expands unit coverage for chain-id validation and legacy v bounds.
Changes:
- Preserve legacy wire
vinstate::Transactiondecode/encode and derivechain_idfrom it. - Introduce
Transaction::chain_id_protected()and use it in chain-id validation (including chain 0 protected legacy tx). - Export/load
vin state-test JSON and update unit tests to cover chain 0 protection and max legacyvbehavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/utils/statetest_loader.cpp | Loads optional v for state-test multi-tx templates so legacy chain protection can be determined. |
| test/utils/statetest_export.cpp | Exports v to state-test JSON to preserve legacy chain binding semantics for re-execution. |
| test/utils/rlp_encode.cpp | Clarifies legacy v semantics in encoder comment (verbatim v carries chain binding since EIP-155). |
| test/unittests/state_transition_tx_test.cpp | Adds chain-id validation coverage for protected legacy tx with chainId == 0. |
| test/unittests/state_rlp_decode_test.cpp | Updates legacy round-trip expectations to keep v verbatim; adds max-legacy-v case. |
| test/state/transaction.hpp | Adds chain_id_protected() and documents v as wire value for legacy transactions. |
| test/state/transaction.cpp | Decodes legacy v as uint64_t verbatim and derives chain_id alongside it. |
| test/state/state.cpp | Switches chain-id validation to chain_id_protected() gating (pre-EIP-155 legacy allowed cross-chain). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
441
to
444
| // chain_id must match the chain, unless the transaction is not signed for any chain in | ||
| // particular: a legacy transaction predating EIP-155 may be included in any of them. | ||
| if (tx.chain_id_protected() && tx.chain_id != block.chain_id) | ||
| return make_error_code(INVALID_CHAIN_ID); |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1616 +/- ##
==========================================
- Coverage 97.46% 97.46% -0.01%
==========================================
Files 170 170
Lines 15388 15383 -5
Branches 3596 3593 -3
==========================================
- Hits 14998 14993 -5
Misses 282 282
Partials 108 108
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
chfast
commented
Jul 27, 2026
…d one The chain id check exempted every legacy transaction with chain_id 0, because the decoder folded the wire v into (chain_id, y_parity), and that makes v = 35/36 -- protected for chain 0 (EIP-155) -- indistinguishable from the unprotected v = 27/28. A transaction signed for chain 0 was therefore accepted on any chain. Keep v verbatim instead, the way rlp_encode() writes it and the JSON transaction loader reads it, and derive chain_id next to it: v >= 35 is exactly the EIP-155 form, so Transaction::chain_id_protected() decides the check, and decoding a transaction becomes the inverse of encoding it. Requiring the whole v to fit uint64_t bounds a legacy chain id to 2**63 - 18, the limit the loader already had. A state test identifies its sender directly and usually carries no signature, so the exported transaction now also carries v; without it a re-executed test would take every legacy transaction for an unprotected one.
chfast
force-pushed
the
state/tx-chain-id-protected
branch
from
July 27, 2026 11:25
0f5b23c to
13a7971
Compare
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.
The chain id check exempted every legacy transaction with chain_id 0, because the decoder folded the wire v into (chain_id, y_parity), and that makes v = 35/36 -- protected for chain 0 (EIP-155) -- indistinguishable from the unprotected v = 27/28. A transaction signed for chain 0 was therefore accepted on any chain.
Keep v verbatim instead, the way rlp_encode() writes it and the JSON transaction loader reads it, and derive chain_id next to it: v >= 35 is exactly the EIP-155 form, so Transaction::chain_id_protected() decides the check, and decoding a transaction becomes the inverse of encoding it. Requiring the whole v to fit uint64_t bounds a legacy chain id to 2**63 - 18, the limit the loader already had.
A state test identifies its sender directly and usually carries no signature, so the exported transaction now also carries v; without it a re-executed test would take every legacy transaction for an unprotected one.