fix: check each RPC transaction hash against its own encoding in verify_block_integrity

[flyq]

Summary

PR 3/6 of the #170 split, stacked on #195. One rewrite of a security-critical check, separated from the cleanup so it gets argued on its own. It turned out to be a fix rather than a perf change: the check it replaces was a tautology.

verify_block_integrity checks the claimed transaction hash against the envelope's own encoding

Before: tx.inner.clone().into_inner().trie_hash() == *hash() per transaction, then a second clone inside ordered_trie_root_with_encoder for the transactions root.

That hash check never checked anything. Encodable2718::trie_hash() defaults to keccak256(encoded_2718()), but every variant in play overrides it: Signed<T>::trie_hash returns *self.hash() (alloy-consensus 1.1.2 src/signed.rs:515), Sealed<T>::trie_hash returns self.hash() (alloy-eips 1.1.2 src/eip2718.rs:327), and OpTxEnvelope's TransactionEnvelope derive forwards per variant. The deserializer seeds that cached hash from the RPC hash field through new_unchecked (src/signed.rs:583), so the old predicate compared the provider's claim with itself. A provider returning the right transaction bytes under a wrong hash passed verification, and the transactions-root check cannot catch that because the root is computed from the bytes.

After: each envelope is encoded once via encoded_2718(); keccak256 of those bytes is checked against the claimed hash, and the same bytes feed the ordered trie (crates/stateless-common/src/rpc_client.rs:1587, :1609). The two clones per transaction are gone and the keccak is new work, the first real check of the field. The other half of the trade is memory: encoded_txs keeps every transaction's encoding resident until the trie root finishes, where the old encoder closure reused one scratch buffer per item — one wire-size copy per transaction, on top of the parsed Block<Transaction> that is resident anyway.

Pinned: verify_block_integrity_rejects_a_forged_transaction_hash (rpc_client.rs:3001) forges the hash of the first and last transaction of a mainnet fixture block through a JSON round trip and asserts the block is rejected, with the transactions root left intact so only the hash check can reject. It asserts index 0 is the deposit (Sealed override) and the last index a signed envelope (Signed override), so a fixture change cannot silently drop either path.

Testing

cargo fmt --all --check, cargo clippy --workspace --all-targets --all-features (0 warnings), cargo sort --check, full workspace suite 481 passed / 0 failed, cargo test -p stateless-core --no-default-features --lib --no-run clean.

The positive path is covered by the validator's mock-RPC integration test, which fetches every fixture block through the verifying path.

[mega-maxwell]

Claude review status

Living comment — rewritten in place. The review workflow keeps this single comment up to date instead of posting a new one each round, so it always describes the latest reviewed commit and the earlier text is intentionally gone. No reply is needed here; reply to a finding in its own review thread, and answer an open question in a reply on this PR. The next review round reconciles your answer.

✅ Review clean

Last reviewed: 5c39a74a..5c39a74a · updated 2026-09-15T07:40:32+00:00

New this round: 0 finding(s), 0 question(s) · Resolved this round: 1 · Open questions: 0

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.5%. Comparing base (7ca2253) to head (5c39a74).
⚠️ Report is 14 commits behind head on main.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e62e3e14ae

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[mega-maxwell]

❓ Review complete — 1 open question(s)

Reviewed 015aa8af..c0ac4b31.

Incremental review of the verify_block_integrity single-encode rewrite (with a new forged-hash regression test) and the gas_used sourcing change in replay_block.

Open questions — answer them in a reply on this PR. Each one is marked answered here once a later review round confirms the answer, so this list stays current:

✅ **Answered** — Head 5c39a74 reverts the executor.rs hunk in full: gas_used is again `receipts.last().map(...).cumulative_gas_used()` at executor.rs:496, so the receipt-chain equivalence is inherent and no debug_assert pin is needed — the `drop the hunk` resolution vincent-k2026 recommended on 2026-09-12.

• The PR description's "Pinned two ways" section still promises a debug_assert_eq! at the gas_used derivation site and a dedicated replayed_gas_used_matches_the_mainnet_header fixture test, but both were removed in the current head — the surviving pin is verify_replay_outputs (output.gas_used == header.gas_used) exercised by validate_block_mainnet_fixtures. Was dropping the receipt-chain-equivalence pin intentional, or should either the description or the pin be restored?
• Why it matters: The removed debug_assert_eq! was the only check that would catch a mega-evm change where execution_result.gas_used stops equaling receipts.last().cumulative_gas_used() while still matching the header — a signal of upstream derivation drift, even when it does not surface as a consensus failure. If the description reflects the intended safety net, either it or the code should be aligned before landing.
• How to verify: Look at crates/stateless-core/src/executor.rs:497 — only let gas_used = execution_result.gas_used; remains — and search the repo for replayed_gas_used_matches_the_mainnet_header (no hits outside .pr-review/). Confirm whether the author still considers verify_replay_outputs the single pin, and update the PR description accordingly, or re-add the internal debug_assert_eq! pin.

[vincent-k2026]

Checked both claims against the upstream sources (alloy-consensus / alloy-eips 1.1.2, alloy-evm's trait defaults, mega-evm v1.7.0 30ce038). Part 1 is a real fix and the test is the right shape. Part 2 is equivalent as argued, but I'd drop it. No blockers.

Part 1: the tautology is real

Verified each link:

• default Encodable2718::trie_hash() is keccak256(encoded_2718()) — alloy-eips eip2718.rs:272-274
• Signed<T> overrides it to *self.hash() — alloy-consensus signed.rs:515-517
• Sealed<T> (the deposit path) overrides it to self.hash() — alloy-eips eip2718.rs:327-329
• the cached hash is seeded straight from the RPC hash field — signed.rs:578-588, Signed::deserialize -> new_unchecked(tx, sig, value.hash.into_owned())

So the old check compared the provider's claim with itself, and the transactions-root check can't cover for it since the root comes from the bytes. verify_block_integrity is the trust boundary against the RPC provider (rpc_client.rs:544), so a check that reads like it validates the hash and never did is worth calling a fix — agreed on the retitle.

verify_block_integrity_rejects_a_forged_transaction_hash is the right shape: positive control first, then both override paths (Sealed at index 0, Signed at the end), with the transactions root left intact so only the hash check can reject. No need for the "I reverted it locally" evidence — the four source facts above already imply it.

Part 2: equivalent, but the change buys nothing

One link the description skips: the call site is executor.apply_post_execution_changes() (executor.rs:488), not finish(). They're connected by alloy-evm's default impl (block/mod.rs:209-216, self.finish().map(|(_, result)| result)), which MegaBlockExecutor doesn't override, and the factory here really is MegaBlockExecutorFactory (executor.rs:293). Worth a sentence, because if mega-evm ever overrides apply_post_execution_changes the argument breaks silently.

With that, the equivalence holds — finish() is character-for-character the old expression, computed after commit_system_call_outcomes.

My problem is the ROI:

• No benefit. receipts.last() is O(1). No perf win, no fewer lines. The only difference is where the value comes from.
• The dependency points the wrong way. header.gas_used is "the last receipt's cumulative gas" by consensus definition. The old line was that definition; the new one trusts an executor's self-reported field, backed by a comment and a version pin. Concretely: the receipt chain is anchored to the header through the receipts-root check in verify_replay_outputs; BlockExecutionResult::gas_used is anchored by nothing.
• A drift shows up as a false reject, not a miss. If upstream ever redefines the field, every legitimate block fails GasUsedMismatch and it reads like a chain-wide consensus divergence. Whether CI catches it first depends on a fixture happening to cover the differing case.

On dropping the debug_assert_eq! — the reasoning ("it could only fire on a correct upstream redefinition, which the header check adjudicates") covers half of it. If the redefinition is wrong, the header check turns everything red and the assert is what tells you it's a field-semantics drift rather than a real divergence. One adjudicates, the other attributes; they're not the same altitude.

Suggestion: either drop this hunk (the old line is the spec written out) or keep it and restore the assert. I'd lean to dropping it — part 1 is a clean fix and this is the only hunk in the PR that needs an argument at all.

Small stuff

1. tx.inner.inner.inner() — three inners and no way to tell what each layer is (op Transaction -> alloy Transaction -> Recovered::inner()). A named binding would help.
2. The description says the two clones are gone but doesn't mention that encoded_txs: Vec<Vec<u8>> now holds the whole block's encodings resident — not free on a high-TPS block. Fair trade, just say both halves.
3. The test's coverage claim ("first is the deposit (Sealed), last a signed envelope") is a comment, not an assertion. If a fixture change makes index 0 a normal tx, the test stays green and the Sealed path silently loses coverage. Cheap to assert the variant.
4. Commit headline is still perf: encode each transaction once... (e62e3e14). If this isn't squashed under the PR title, a security fix lands in the log as a perf change and gets hard to find later.
5. The bot's open question is stale — the current description already explains why the debug_assert_eq! and the third fixture test were dropped. A one-line reply closes it.

[flyq]

Dropped the gas_used hunk: crates/stateless-core/src/executor.rs is back to the base version in 5c39a74, so the header check reads the last receipt's cumulative gas as before and the PR is the hash check alone. Title and description updated to match.

Small items, same commit:

1. The envelope comes out of a named Recovered<_> binding (crates/stateless-common/src/rpc_client.rs:1585), and the signer binding is recovered_signer so the two cannot be read as one another.
2. Description states both halves of the trade: the two clones are gone, and encoded_txs holds one encoded copy per transaction until the trie root finishes.
3. The test asserts the variant at each index (rpc_client.rs:3018): index 0 is_deposit(), the last index not.
4. The repo allows squash merges only and takes the squash title from the PR title (ruleset allowed_merge_methods: ["squash"], repo squash_merge_commit_title: PR_TITLE), so fix: is what lands in the log; the branch commit's perf: headline never reaches main.
5. Answered in a separate comment so the bot's reconcile round picks it up.

[flyq]

Answering the open question: the gas_used hunk is gone in 5c39a74 (crates/stateless-core/src/executor.rs is back to the base version, reading the last receipt's cumulative gas), so there is no derivation-site pin to restore and the description no longer mentions one. verify_replay_outputs (executor.rs:622) is unchanged.

[mega-maxwell]

⚠️ Review needs attention — 1 finding(s)

0 blocking · 0 should-fix · 1 suggestion(s) · 0 open question(s)

Reviewed head 5c39a74a.

Findings without inline anchors:

• crates/stateless-core/src/executor.rs:496 — [Minor] PR description and title advertise a reverted gas_used change Anyone reading the PR (and the squash-merge commit that inherits body+title) is told a second consensus-critical derivation moved; reviewers scanning the description look for a safety story for a change that no longer ships, and the audit trail misstates what actually landed. Suggested fix: Drop §2 and the , read gas_used from the executor clause from the title (shorten to fix: check each RPC transaction hash against its own encoding in verify_block_integrity). If the executor hunk is instead intended to ship here, restore it and re-anchor the receipt-chain equivalence pin.