fix(primitives): override rlp_decode/encode_signed for TxMorph#36
fix(primitives): override rlp_decode/encode_signed for TxMorph#36chengwenxi merged 4 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds version-prefix handling for TxMorph RLP: V1 is encoded/decoded with a leading 0x01 byte while V0 remains raw RLP. Introduces signed encode/length APIs and a decode-with-signature path; updates header/length logic and expands tests for V0/V1 behaviors and edge cases. Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
chengwenxi
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/primitives/src/transaction/morph_transaction.rs`:
- Around line 717-733: The version-detection treats 0x00 as MORPH_TX_VERSION_0
but doesn't advance the buffer, so Header::decode(buf) sees the 0x00 and fails;
update the detection logic around first_byte, MORPH_TX_VERSION_1 and
MORPH_TX_VERSION_0 to either reject 0x00 or consume it — implement the suggested
fix by, when first_byte == 0, advancing buf (same as the MORPH_TX_VERSION_1
branch) before setting version to MORPH_TX_VERSION_0 so Header::decode receives
the correct RLP list start; ensure the code references first_byte, buf,
MORPH_TX_VERSION_0, MORPH_TX_VERSION_1 and Header::decode.
|
Code review Found 1 issue:
morph-reth/crates/primitives/src/transaction/morph_transaction.rs Lines 717 to 733 in f608cd9 |
0x00 is not a valid RLP list header (valid range is 0xC0-0xFF), so the first_byte == 0 condition in decode_fields and rlp_decode_with_signature was dead code — Header::decode would return list=false and immediately fail with UnexpectedString. Remove it so invalid inputs fall through to the unsupported version error instead.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/primitives/src/transaction/morph_transaction.rs (1)
1905-2072: Add a targeted regression test for invalid0x00prefix inputs.Given the version-detection fix, it’s worth locking this down with a negative test that asserts
0x00is rejected (both signed and unsigned decode paths).Test sketch
+ #[test] + fn test_morph_decode_rejects_zero_prefix() { + let mut buf = [0x00u8].as_slice(); + let err = TxMorph::decode(&mut buf).unwrap_err(); + assert!(matches!(err, alloy_rlp::Error::Custom(_))); + } + + #[test] + fn test_morph_signed_decode_rejects_zero_prefix() { + use alloy_consensus::transaction::RlpEcdsaDecodableTx; + let mut buf = [0x00u8].as_slice(); + let err = TxMorph::rlp_decode_with_signature(&mut buf).unwrap_err(); + assert!(matches!(err, alloy_rlp::Error::Custom(_))); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/primitives/src/transaction/morph_transaction.rs` around lines 1905 - 2072, Add a negative regression test that verifies inputs starting with 0x00 are rejected for both signed and unsigned decode paths: create a buffer beginning with 0x00 followed by arbitrary bytes and assert TxMorph::rlp_decode_with_signature returns an Err (or None) for signed decode and TxMorph::rlp_decode (or the unsigned decode helper used) also returns an Err for unsigned decode; additionally test the EIP-2718 path by constructing an eip2718 buffer with MORPH_TX_TYPE_ID then 0x00 and assert Signed::<TxMorph>::decode_2718 returns an error. Use the existing test helpers and the same signature/tx structures from test_morph_signed_v1_decode_2718_roundtrip and test_morph_signed_v0_decode_2718_roundtrip to keep consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/primitives/src/transaction/morph_transaction.rs`:
- Around line 351-353: Decodable::decode still accepts a leading 0x00 byte
causing a different failure path than the version-aware decoders in
morph_transaction.rs; update Decodable::decode to explicitly detect and reject a
leading 0x00 (same check/early-error path used by the version-aware decoder
logic in morph_transaction) and return the same error variant used elsewhere so
all decode entry points behave consistently when encountering 0x00. Ensure you
modify the first-byte inspection in Decodable::decode to mirror the first_byte
>= 0xC0 / version handling and add the 0x00 rejection branch returning the
common decode error.
---
Nitpick comments:
In `@crates/primitives/src/transaction/morph_transaction.rs`:
- Around line 1905-2072: Add a negative regression test that verifies inputs
starting with 0x00 are rejected for both signed and unsigned decode paths:
create a buffer beginning with 0x00 followed by arbitrary bytes and assert
TxMorph::rlp_decode_with_signature returns an Err (or None) for signed decode
and TxMorph::rlp_decode (or the unsigned decode helper used) also returns an Err
for unsigned decode; additionally test the EIP-2718 path by constructing an
eip2718 buffer with MORPH_TX_TYPE_ID then 0x00 and assert
Signed::<TxMorph>::decode_2718 returns an error. Use the existing test helpers
and the same signature/tx structures from
test_morph_signed_v1_decode_2718_roundtrip and
test_morph_signed_v0_decode_2718_roundtrip to keep consistency.
…ode_fields Align the first-byte check in Decodable::decode with decode_fields so that a leading 0x00 byte is rejected with "unsupported morph tx version" instead of being silently passed through as a V0 transaction.
Code reviewFound 1 issue:
The PR correctly removes the dead morph-reth/crates/primitives/src/transaction/morph_transaction.rs Lines 816 to 825 in d7bec78 Note: 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/primitives/src/transaction/morph_transaction.rs (2)
351-353: Comment wording still implies unsupported versions are accepted.Line 352 says “V1+ format” with
(0x01, 0x02, ...), but the implementation only acceptsMORPH_TX_VERSION_1and rejects others. Please tighten the comment to avoid signaling v2+ support.Suggested doc-only patch
- // - V1+ format: first byte is version (0x01, 0x02, ...) followed by RLP + // - V1 format: first byte is version (0x01) followed by RLP🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/primitives/src/transaction/morph_transaction.rs` around lines 351 - 353, The comment above the version check currently implies "V1+ (0x01, 0x02, ...)" is accepted but the code only permits MORPH_TX_VERSION_1; update the comment near the if first_byte >= 0xC0 check in morph_transaction.rs to state that only V0 (legacy AltFeeTx) and V1 (MORPH_TX_VERSION_1) are supported and that higher version numbers are not accepted, so readers won't expect v2+ compatibility.
1905-2072: Add a direct regression test for leading0x00across decode entry points.These roundtrip tests are good, but the exact bug fixed in this PR (
0x00classification) is not asserted directly. A focused test would lock this behavior.Suggested regression test
+ #[test] + fn test_morph_decode_rejects_leading_zero_consistently() { + use alloy_consensus::transaction::RlpEcdsaDecodableTx; + + let raw = [0x00u8]; + + let err_decode = TxMorph::decode(&mut raw.as_slice()).unwrap_err(); + assert!(matches!( + err_decode, + alloy_rlp::Error::Custom("unsupported morph tx version") + )); + + let err_fields = TxMorph::decode_fields(&mut raw.as_slice()).unwrap_err(); + assert!(matches!( + err_fields, + alloy_rlp::Error::Custom("unsupported morph tx version") + )); + + let err_signed = + TxMorph::rlp_decode_with_signature(&mut raw.as_slice()).unwrap_err(); + assert!(matches!( + err_signed, + alloy_rlp::Error::Custom("unsupported morph tx version") + )); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/primitives/src/transaction/morph_transaction.rs` around lines 1905 - 2072, Add a focused regression test (e.g., test_morph_leading_zero_regression) that constructs a V0 TxMorph and signature, then ensures decoding tolerates a leading 0x00 byte across both entry points: call tx.rlp_encode_signed + prepend a 0x00 to the signed buffer and assert TxMorph::rlp_decode_with_signature decodes successfully with version==MORPH_TX_VERSION_0; also build a Signed::new_unhashed(tx, sig) and produce eip2718_buf via Signed::encode_2718, then insert a leading 0x00 before the RLP payload and assert Signed::<TxMorph>::decode_2718 decodes successfully and tx().is_v0(); reference the symbols TxMorph::rlp_encode_signed, TxMorph::rlp_decode_with_signature, Signed::new_unhashed, Signed::encode_2718, and Signed::<TxMorph>::decode_2718 when adding the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/primitives/src/transaction/morph_transaction.rs`:
- Around line 351-353: The comment above the version check currently implies
"V1+ (0x01, 0x02, ...)" is accepted but the code only permits
MORPH_TX_VERSION_1; update the comment near the if first_byte >= 0xC0 check in
morph_transaction.rs to state that only V0 (legacy AltFeeTx) and V1
(MORPH_TX_VERSION_1) are supported and that higher version numbers are not
accepted, so readers won't expect v2+ compatibility.
- Around line 1905-2072: Add a focused regression test (e.g.,
test_morph_leading_zero_regression) that constructs a V0 TxMorph and signature,
then ensures decoding tolerates a leading 0x00 byte across both entry points:
call tx.rlp_encode_signed + prepend a 0x00 to the signed buffer and assert
TxMorph::rlp_decode_with_signature decodes successfully with
version==MORPH_TX_VERSION_0; also build a Signed::new_unhashed(tx, sig) and
produce eip2718_buf via Signed::encode_2718, then insert a leading 0x00 before
the RLP payload and assert Signed::<TxMorph>::decode_2718 decodes successfully
and tx().is_v0(); reference the symbols TxMorph::rlp_encode_signed,
TxMorph::rlp_decode_with_signature, Signed::new_unhashed, Signed::encode_2718,
and Signed::<TxMorph>::decode_2718 when adding the test.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a80f24a8-d13b-4fa8-a54b-4284d96a804b
📒 Files selected for processing (1)
crates/primitives/src/transaction/morph_transaction.rs
fixed 13f39e0 |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests