Repository navigation
fix(spec-specs, tests): reject empty and 0xff-prefixed transaction bytes - #3861
Conversation
Typed transactions sit in the block body as byte strings, and `decode_transaction` read `tx[0]` without checking the length. An empty byte string crashed `apply_body` with an `IndexError` instead of invalidating the block (Berlin onwards). Since Amsterdam, a first byte of 0xff also tripped `assert tx[0] <= 0xFE`; EIP-2718 reserves 0xff, so treat it as an unknown type like any other. Raise `InvalidBlock` for an empty transaction and `TransactionTypeError` for 0xff, and add decoding tests for every fork with typed transactions.
marioevz
left a comment
marioevz
left a comment
There was a problem hiding this comment.
Thanks for fixing. We need to implement a reference spec test for this so implementations can use it to test. Please see comment below.
There was a problem hiding this comment.
We also need a transaction_test in tests/frontier/validation/test_transaction_rlp.py.
There was a problem hiding this comment.
Added test_invalid_envelope to test_transaction_rlp.py: an empty transaction and one with the reserved 0xff type byte, as transaction_tests from Frontier on. Without the spec fix, decoding these crashes on 11 of the 32 cases.
Cover the inputs fixed in the spec with `transaction_test`s, so clients can check them too: an empty transaction, and one whose first byte is the 0xff type reserved by EIP-2718. Both are invalid from Frontier on.
EddyW19
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## forks/bogota #3861 +/- ##
================================================
+ Coverage 94.99% 95.09% +0.09%
================================================
Files 673 673
Lines 40888 40904 +16
Branches 3769 3778 +9
================================================
+ Hits 38843 38896 +53
+ Misses 1428 1397 -31
+ Partials 617 611 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
marioevz
left a comment
marioevz
left a comment
There was a problem hiding this comment.
LGTM, great catch. Thanks for implementing!
Description
decode_transactionreadstx[0]without checking the length of the byte string. Block bodies carry typed transactions as byte strings, so a block with an empty entry (0x80) crashesapply_bodywith anIndexErrorinstead of being rejected as invalid. This goes back to Berlin.Since #2365, Amsterdam and Bogota also hit
assert tx[0] <= 0xFEwhen the first byte is0xff, so that crashes with anAssertionError. EIP-2718 reserves0xff, so it's now handled like any other unknown type and raisesTransactionTypeError.An empty transaction now raises
InvalidBlock. Added decoding tests for both cases on every fork with typed transactions; they fail on currentforks/bogota(16IndexError, 4AssertionError) and pass with this change.Related Issues or PRs
Same kind of problem as #3828: malformed input crashed block processing instead of being rejected.
Checklist
just static<type>(<area>): <description>, where<type>and<area>come from an appropriateC-<type>, respectivelyA-<area>, label. The title should match the target squash commit message.Cute Animal Picture