fix: handle MorphTx V0/V1 decoding in DecodeTxsFromBytes#920
fix: handle MorphTx V0/V1 decoding in DecodeTxsFromBytes#920FletcherMan merged 1 commit intomainfrom
Conversation
DecodeTxsFromBytes panicked with "makeslice: len out of range" when decoding MorphTx V1 transactions. The V1 wire format includes a version byte between the type byte and the RLP payload, which the old code mistakenly treated as an RLP prefix, causing a uint8 underflow. Additionally, the old code used rlp.DecodeBytes with *MorphTx which bypasses MorphTx.decode() and cannot handle the V0/V1 field differences. Changes: - Add decodeMorphTx() to handle both V0 and V1 wire formats, using Transaction.UnmarshalBinary() which correctly routes to MorphTx.decode() - Add decodeTypedTx() for standard EIP-2718 typed txs, also using UnmarshalBinary() for consistency - Add bounds check in extractInnerTxFullBytes to prevent panic on invalid RLP prefix bytes - Add test cases for MorphTx V0, V1, and mixed multi-tx decoding Made-with: Cursor
📝 WalkthroughWalkthroughThe pull request refactors the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
node/types/blob.go (1)
228-241:⚠️ Potential issue | 🔴 CriticalReject oversized RLP lengths before allocating.
Line 240 still trusts the declared RLP payload length and allocates before checking how many bytes remain.
DecodeTxsFromBytesis fed directly from blob sidecar data innode/derivation/batch_info.go:95-171, so a malformed prefix like0xfb ffffffffcan request ~4 GiB and take the node down before EOF is returned.🛡️ Proposed hardening
+type remainingReader interface { + io.Reader + Len() int +} + -func decodeTypedTx(typeByte byte, reader io.Reader) (*eth.Transaction, error) { +func decodeTypedTx(typeByte byte, reader remainingReader) (*eth.Transaction, error) { ... } -func decodeMorphTx(reader io.Reader) (*eth.Transaction, error) { +func decodeMorphTx(reader remainingReader) (*eth.Transaction, error) { ... } -func extractInnerTxFullBytes(firstByte byte, reader io.Reader) ([]byte, error) { - sizeByteLen := firstByte - 0xf7 +func extractInnerTxFullBytes(firstByte byte, reader remainingReader) ([]byte, error) { + if firstByte <= 0xf7 { + return nil, fmt.Errorf("expected long-form RLP list prefix, got 0x%x", firstByte) + } + sizeByteLen := int(firstByte - 0xf7) if sizeByteLen > 4 { return nil, fmt.Errorf("invalid RLP size byte length: %d (firstByte=0x%x)", sizeByteLen, firstByte) } sizeByte := make([]byte, sizeByteLen) - if err := binary.Read(reader, binary.BigEndian, sizeByte); err != nil { + if _, err := io.ReadFull(reader, sizeByte); err != nil { return nil, err } size := binary.BigEndian.Uint32(append(make([]byte, 4-len(sizeByte)), sizeByte...)) + if size > uint32(reader.Len()) { + return nil, io.ErrUnexpectedEOF + } txRaw := make([]byte, size) - if err := binary.Read(reader, binary.BigEndian, txRaw); err != nil { + if _, err := io.ReadFull(reader, txRaw); err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/types/blob.go` around lines 228 - 241, In extractInnerTxFullBytes ensure you validate the decoded RLP payload length before allocating txRaw: after computing size (from firstByte and sizeByte), check that size is within safe bounds (e.g. not greater than the remaining bytes available from the provided reader or a project-wide MAX_TX_SIZE) and return an error if it exceeds that limit; do not call make([]byte, size) until that check passes. Reference functions: extractInnerTxFullBytes (for the check/early reject) and DecodeTxsFromBytes/node/derivation/batch_info.go (callers that feed untrusted blob data). Ensure the error is descriptive (e.g. "declared RLP size too large") so malformed prefixes like 0xfb ffffffff cannot trigger huge allocations.
🧹 Nitpick comments (2)
node/types/blob_test.go (2)
156-203: Add a malformed-prefix regression for the panic fix.The new happy-path tests don't exercise the defensive branch added in
extractInnerTxFullBytes. Please add a malformed Morph payload (for example0x7f0180) and assertDecodeTxsFromBytesreturns an error, so the original panic path stays covered by tests.🧪 Minimal regression test
+func TestDecodeTxsFromBytes_InvalidMorphPrefix(t *testing.T) { + _, err := DecodeTxsFromBytes(common.FromHex("0x7f0180")) + require.Error(t, err) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/types/blob_test.go` around lines 156 - 203, Add a regression test that exercises the defensive branch in extractInnerTxFullBytes by passing a malformed Morph prefix (e.g. bytes for "0x7f0180") into DecodeTxsFromBytes and asserting it returns an error; specifically create a new test (similar style to TestDecodeTxsFromBytes_MorphTxV0/V1) that calls DecodeTxsFromBytes with common.FromHex("0x7f0180") (or equivalent raw bytes), requires a non-nil error via require.Error(t, err) and ensures no panic occurs, so the malformed-prefix case is covered.
156-170: Strengthen the Morph assertions beyondType().These tests only prove that decoding returns a Morph tx, not that the V0/V1-specific contents survived decoding. A decoder that still drops the V1-only fields would pass here, so please assert at least one stable value from each Morph fixture that depends on the decoded contents—e.g. an expected hash or another version-sensitive field set. Based on learnings: V0 transactions predate the Morph tx version field, so
None → 0is the correct semantic mapping.Also applies to: 172-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/types/blob_test.go` around lines 156 - 170, The tests only check txs[0].Type()—add assertions that verify a version-sensitive field from each fixture after DecodeTxsFromBytes returns (e.g., type-assert txs[0] to the concrete Morph tx struct and assert its Version or a stable content-derived value like the expected hash or the known payload string present in the V1 fixture ("morph hoodi test tx"); for V0 assert the decoder maps missing version → 0). Update TestDecodeTxsFromBytes_MorphTxV0 and TestDecodeTxsFromBytes_MorphTxV1 to include these extra checks so the decoder must preserve V0/V1-specific fields, while still keeping the existing Type() assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@node/types/blob.go`:
- Around line 228-241: In extractInnerTxFullBytes ensure you validate the
decoded RLP payload length before allocating txRaw: after computing size (from
firstByte and sizeByte), check that size is within safe bounds (e.g. not greater
than the remaining bytes available from the provided reader or a project-wide
MAX_TX_SIZE) and return an error if it exceeds that limit; do not call
make([]byte, size) until that check passes. Reference functions:
extractInnerTxFullBytes (for the check/early reject) and
DecodeTxsFromBytes/node/derivation/batch_info.go (callers that feed untrusted
blob data). Ensure the error is descriptive (e.g. "declared RLP size too large")
so malformed prefixes like 0xfb ffffffff cannot trigger huge allocations.
---
Nitpick comments:
In `@node/types/blob_test.go`:
- Around line 156-203: Add a regression test that exercises the defensive branch
in extractInnerTxFullBytes by passing a malformed Morph prefix (e.g. bytes for
"0x7f0180") into DecodeTxsFromBytes and asserting it returns an error;
specifically create a new test (similar style to
TestDecodeTxsFromBytes_MorphTxV0/V1) that calls DecodeTxsFromBytes with
common.FromHex("0x7f0180") (or equivalent raw bytes), requires a non-nil error
via require.Error(t, err) and ensures no panic occurs, so the malformed-prefix
case is covered.
- Around line 156-170: The tests only check txs[0].Type()—add assertions that
verify a version-sensitive field from each fixture after DecodeTxsFromBytes
returns (e.g., type-assert txs[0] to the concrete Morph tx struct and assert its
Version or a stable content-derived value like the expected hash or the known
payload string present in the V1 fixture ("morph hoodi test tx"); for V0 assert
the decoder maps missing version → 0). Update TestDecodeTxsFromBytes_MorphTxV0
and TestDecodeTxsFromBytes_MorphTxV1 to include these extra checks so the
decoder must preserve V0/V1-specific fields, while still keeping the existing
Type() assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2832882-6b0d-458e-89c0-30c513445186
📒 Files selected for processing (2)
node/types/blob.gonode/types/blob_test.go
fix: handle MorphTx V0/V1 decoding in DecodeTxsFromBytes (#920)
DecodeTxsFromBytes panicked with "makeslice: len out of range" when decoding MorphTx V1 transactions. The V1 wire format includes a version byte between the type byte and the RLP payload, which the old code mistakenly treated as an RLP prefix, causing a uint8 underflow.
Additionally, the old code used rlp.DecodeBytes with *MorphTx which bypasses MorphTx.decode() and cannot handle the V0/V1 field differences.
Changes:
Made-with: Cursor
Summary by CodeRabbit
Bug Fixes
Tests