rlp: block and txn decoding hardening + decode block raw_transaction return

[goodlyrottenapple]

This PR includes some RLP parser hardening which is not strictly necessary in the usual x86 host case because consensus already does validation but could be a potential corner-case issue in the zkvm witness path, where a malicious block with added garbage could cause divergence in our client compared to a different implementation which would reject the witness outright.

[github-actions]

Reviewed the two changes here: the blob-versioned-hash decoding fix and the new raw_transactions out-parameter on decode_block / decode_transaction_list.

The blob-hash fix is correct — decode_bytes32 rejects any non-32-byte string, so switching the loop condition from size() >= sizeof(bytes32_t) to !empty() turns silently-dropped trailing bytes into a decode error, and valid canonical encodings are unaffected. The legacy-transaction raw slice (before.substr(0, before.size() - ls.size())) is computed correctly, and the lifetime contract is documented in both headers.

Findings (inline):

• [P2] The typed-transaction raw slice is captured as the full string envelope, but decode_transaction_eip2718 is never checked to have consumed all of it — trailing bytes inside the envelope end up in the slice, breaking the documented "exact byte slice" / trie-value contract.
• [P2] No test coverage for either the new overloads or the blob-hash strictness change, despite test_transaction_rlp.cpp / test_block_rlp.cpp existing alongside.
• [P3] Bare namespace { … } instead of MONAD_RLP_ANONYMOUS_NAMESPACE_BEGIN/END.

Verdict: CORRECT

🤖 Generated with Claude Code

[dhil]

Thanks for hoisting this change out of the larger PR. It is nicely self-contained now. The only thing I see we need to figure out is whether the hash payload decoding change is OK.

[Copilot]

🟡 Changes recommended

The typed raw-transaction path appends output before decoding succeeds, and requested regression coverage is missing.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR exposes raw transaction bytes during Ethereum RLP decoding and fixes EIP-4844 blob-hash validation.

Changes:

• Adds raw transaction output overloads for transaction and block decoding.
• Preserves trie-compatible legacy and typed transaction encodings.
• Rejects malformed trailing blob-hash data.

File summaries

File	Summary
category/execution/ethereum/core/rlp/transaction_rlp.hpp	Declares raw transaction output overloads. No findings.
category/execution/ethereum/core/rlp/transaction_rlp.cpp	Implements raw extraction and blob-hash validation. Moderate [P2] (1 vote): append typed raw bytes only after successful decoding. Nits [P2] (2 and 3 votes): add regression coverage for raw output and malformed trailing blob-hash data.
category/execution/ethereum/core/rlp/block_rlp.hpp	Declares block decoding overload with raw transaction output. No findings.
category/execution/ethereum/core/rlp/block_rlp.cpp	Forwards raw transaction collection. Nit [P2] (1 vote): add coverage for raw transaction output paths and slice semantics.

Review details

Suppressed comments (2)

category/execution/ethereum/core/rlp/block_rlp.cpp:226

• [P2] Add coverage for this new output path. Existing block tests call only decode_block(enc); none exercise decode_block(enc, &raw_transactions) or the corresponding transaction-list overload, so the legacy-versus-typed slice semantics and lifetime contract are unverified. Use the existing mixed typed/legacy block fixture and assert the returned views match the transactions-trie encodings.

    BOOST_OUTCOME_TRY(
        block.transactions, decode_transaction_list(payload, raw_transactions));

category/execution/ethereum/core/rlp/transaction_rlp.cpp:419

• [P2] Append typed raw bytes only after successful decoding. In this branch the slice is added before decode_transaction_eip2718() runs, so a malformed typed transaction returns an error but still leaves an entry in the output vector even though no transaction was decoded; the legacy branch does not have this asymmetry. Capture str in a separate view, decode first, and push that captured view only after the decode succeeds.

                    raw_transactions->push_back(str);
                }
                BOOST_OUTCOME_TRY(auto tx, decode_transaction_eip2718(str));

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[goodlyrottenapple]

@claude re-review please

[github-actions]

Re-review of 8052b2f. All findings from the previous round have been addressed:

• Typed-envelope trailing garbage is now rejected (!payload.empty() || !enc.empty() in decode_transaction_eip2718), which also makes the raw slice pushed for typed transactions provably equal to the consumed bytes — the earlier [P2] slice-contract concern is resolved.
• Tests added covering the blob-hash trailing-bytes strictness (both ends of the 1–31 byte range), eip2718 trailing bytes (extra list item, bytes after the envelope, and garbage inside the block body's string wrapper), and the raw_transactions byte-slice contract (aliasing of the input buffer + standalone re-decodability).
• The bare anonymous namespace is gone, replaced by the constrained template with explicit instantiations.

I checked every caller of decode_transaction / decode_transaction_eip2718 (monad_executor.cpp RPC entry points, db/util.cpp decode_transaction_db, monad_block_rlp.cpp, tests): each passes an exact single-transaction buffer or already asserts emptiness afterwards, so the stricter full-consumption requirement does not break any existing call site. The legacy/typed asymmetry (legacy decode still permits trailing input, since legacy transactions are concatenated directly in a block body) is deliberate and documented in the new test.

CI note: all four build jobs failed in the MonadEip7981SpecTestFixtures-download external-fixture download step — an infrastructure flake unrelated to this diff (the changed library TUs compiled cleanly before ninja stopped, and the new test files were never reached). I compiled and ran the changed tests locally: all 12 tests in test_transaction_rlp.cpp and the new Rlp_Block.DecodeBlockRawTransactions pass. CI should be re-run before merge.

Verdict: CORRECT

🤖 Generated with Claude Code