fix(l1): validate block header logs_bloom against executed receipts#6766
Conversation
|
🤖 Kimi Code ReviewThe PR correctly addresses a consensus vulnerability where the Security/Consensus: Correct
Code Quality: Minor suggestions
Testing: Adequate
Verification completeness: Good
Conclusion: The PR is correct and should be merged after considering the performance optimization regarding pre-computed receipt blooms. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code Review
Aside from those points, the added blockchain-side validation is correct and the new regression test covers the primary import path well. I couldn’t run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Lines of code reportTotal lines added: Detailed view |
Greptile SummaryThis PR closes a gap in L1 block validation: the header's
Confidence Score: 4/5Safe to merge; the new check is strictly additive and covers all three L1 import paths. The implementation correctly reproduces the same OR-of-per-receipt-blooms logic used by geth/reth. The bloom computation is sound and the call-sites are complete. The two observations are: bloom is hashed twice per import (once inside crates/common/validation.rs (redundant bloom computation) and test/tests/blockchain/logs_bloom_tests.rs (underspecified error assertion)
|
| Filename | Overview |
|---|---|
| crates/common/validation.rs | Adds validate_logs_bloom which recomputes the aggregate bloom per receipt and compares it to the header; logic is correct but bloom is already computed inside validate_receipts_root → compute_receipts_root → encode_inner_with_bloom, resulting in double hashing. |
| crates/blockchain/blockchain.rs | Correctly inserts validate_logs_bloom at all three post-execution sites (execute_block, execute_block_pipeline, execute_block_from_state), immediately after validate_receipts_root and before validate_requests_hash. |
| crates/common/errors.rs | Adds LogsBloomMismatch variant to InvalidBlockError with a clear error message; no issues. |
| crates/common/common.rs | Re-exports validate_logs_bloom alongside other validation helpers; straightforward and consistent. |
| test/tests/blockchain/logs_bloom_tests.rs | Regression test that builds a manually-crafted block with a tampered logs_bloom and asserts rejection; correct setup but only checks is_err() rather than the specific LogsBloomMismatch variant. |
| test/tests/blockchain/mod.rs | Adds the new test module declaration; no issues. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[add_block / execute_block] --> B[validate_block_pre_execution]
B --> C[Execute transactions]
C --> D[validate_gas_used]
D --> E[validate_receipts_root\ncomputes bloom per receipt internally]
E --> F[validate_logs_bloom NEW\naggregates bloom OR per receipt\ncompares to header.logs_bloom]
F -->|match| G[validate_requests_hash]
F -->|mismatch| H[Err LogsBloomMismatch]
G --> I[validate_block_access_list_hash]
I --> J[Commit to storage]
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
crates/common/validation.rs:100-115
**Redundant bloom computation on every block import**
`validate_receipts_root` (called immediately before this function) already invokes `compute_receipts_root`, which in turn calls `encode_inner_with_bloom` → `bloom_from_logs` for every receipt. `validate_logs_bloom` then calls `bloom_from_logs` a second time over the same logs. For a block carrying many log-heavy transactions this doubles the keccak hashing load on the import hot-path. A straightforward mitigation is to accept an already-computed `Bloom` rather than raw receipts, letting the call-site pass the value it got from `compute_receipts_root`. Alternatively, the aggregate bloom could be accumulated inside `compute_receipts_root` and returned alongside the root hash.
### Issue 2 of 2
test/tests/blockchain/logs_bloom_tests.rs:103-108
The assertion only checks that `add_block` returns any error, not that it specifically returns `LogsBloomMismatch`. If a future refactor accidentally causes the block to be rejected earlier (e.g. a pre-execution header check), this test would continue to pass while the bloom validation itself silently stopped working. Asserting the specific error variant pins the test to the intended invariant.
```suggestion
let result = blockchain.add_block(block);
assert!(
matches!(
result,
Err(ethrex_blockchain::error::ChainError::InvalidBlock(
ethrex_common::errors::InvalidBlockError::LogsBloomMismatch
))
),
"expected LogsBloomMismatch, got: {result:?}"
);
```
Reviews (1): Last reviewed commit: "fix(l1): validate block header logs_bloo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pub fn validate_logs_bloom( | ||
| block_header: &BlockHeader, | ||
| receipts: &[Receipt], | ||
| crypto: &dyn Crypto, | ||
| ) -> Result<(), InvalidBlockError> { | ||
| let mut computed_bloom = Bloom::zero(); | ||
| for receipt in receipts { | ||
| computed_bloom |= bloom_from_logs(&receipt.logs, crypto); | ||
| } | ||
|
|
||
| if computed_bloom == block_header.logs_bloom { | ||
| Ok(()) | ||
| } else { | ||
| Err(InvalidBlockError::LogsBloomMismatch) | ||
| } | ||
| } |
There was a problem hiding this comment.
validate_receipts_root (called immediately before this function) already invokes compute_receipts_root, which in turn calls encode_inner_with_bloom → bloom_from_logs for every receipt. validate_logs_bloom then calls bloom_from_logs a second time over the same logs. For a block carrying many log-heavy transactions this doubles the keccak hashing load on the import hot-path. A straightforward mitigation is to accept an already-computed Bloom rather than raw receipts, letting the call-site pass the value it got from compute_receipts_root. Alternatively, the aggregate bloom could be accumulated inside compute_receipts_root and returned alongside the root hash.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/common/validation.rs
Line: 100-115
Comment:
**Redundant bloom computation on every block import**
`validate_receipts_root` (called immediately before this function) already invokes `compute_receipts_root`, which in turn calls `encode_inner_with_bloom` → `bloom_from_logs` for every receipt. `validate_logs_bloom` then calls `bloom_from_logs` a second time over the same logs. For a block carrying many log-heavy transactions this doubles the keccak hashing load on the import hot-path. A straightforward mitigation is to accept an already-computed `Bloom` rather than raw receipts, letting the call-site pass the value it got from `compute_receipts_root`. Alternatively, the aggregate bloom could be accumulated inside `compute_receipts_root` and returned alongside the root hash.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed — folded into a single-pass compute_receipts_root_and_logs_bloom that computes each receipt's bloom once and reuses it for both the receipts trie and the aggregate header bloom (via Receipt::encode_inner_with_precomputed_bloom). The duplicate bloom_from_logs per receipt is gone.
| let result = blockchain.add_block(block); | ||
| assert!( | ||
| result.is_err(), | ||
| "a block whose header logs_bloom does not match the executed receipts must be \ | ||
| rejected, but add_block returned: {result:?}" | ||
| ); |
There was a problem hiding this comment.
add_block returns any error, not that it specifically returns LogsBloomMismatch. If a future refactor accidentally causes the block to be rejected earlier (e.g. a pre-execution header check), this test would continue to pass while the bloom validation itself silently stopped working. Asserting the specific error variant pins the test to the intended invariant.
| let result = blockchain.add_block(block); | |
| assert!( | |
| result.is_err(), | |
| "a block whose header logs_bloom does not match the executed receipts must be \ | |
| rejected, but add_block returned: {result:?}" | |
| ); | |
| let result = blockchain.add_block(block); | |
| assert!( | |
| matches!( | |
| result, | |
| Err(ethrex_blockchain::error::ChainError::InvalidBlock( | |
| ethrex_common::errors::InvalidBlockError::LogsBloomMismatch | |
| )) | |
| ), | |
| "expected LogsBloomMismatch, got: {result:?}" | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/tests/blockchain/logs_bloom_tests.rs
Line: 103-108
Comment:
The assertion only checks that `add_block` returns any error, not that it specifically returns `LogsBloomMismatch`. If a future refactor accidentally causes the block to be rejected earlier (e.g. a pre-execution header check), this test would continue to pass while the bloom validation itself silently stopped working. Asserting the specific error variant pins the test to the intended invariant.
```suggestion
let result = blockchain.add_block(block);
assert!(
matches!(
result,
Err(ethrex_blockchain::error::ChainError::InvalidBlock(
ethrex_common::errors::InvalidBlockError::LogsBloomMismatch
))
),
"expected LogsBloomMismatch, got: {result:?}"
);
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed — the test now asserts the specific ChainError::InvalidBlock(InvalidBlockError::LogsBloomMismatch) rather than is_err(), so an unrelated earlier rejection can't mask a regression in the bloom check. Added test/tests/common/logs_bloom_validation_tests.rs covering the validator directly (matching → ok, flipped bloom bit → LogsBloomMismatch, wrong root → ReceiptsRootMismatch).
🤖 Claude Code ReviewNow I have a complete picture. Here's my review: Review: fix(l1): validate block header logs_bloom against executed receiptsSummaryThis is a correct, well-scoped consensus-correctness fix. The core logic is sound and the architectural reasoning (receipts root commits to per-receipt encoding but not to the aggregate header bloom) is accurate. Implementation —
|
| ) -> Result<(), InvalidBlockError> { | ||
| let mut computed_bloom = Bloom::zero(); | ||
| for receipt in receipts { | ||
| computed_bloom |= bloom_from_logs(&receipt.logs, crypto); |
There was a problem hiding this comment.
Could we reuse the bloom_from_logs calculation done for validate_receipts_root?
There was a problem hiding this comment.
Done — good call. The two checks now run in one pass: compute_receipts_root_and_logs_bloom hashes each receipt's bloom once and feeds it to both the receipts trie (via the new Receipt::encode_inner_with_precomputed_bloom) and the OR-ed header bloom, and validate_receipts_root_and_logs_bloom replaces the two separate calls at all four sites. No more double bloom_from_logs.
A nice side effect: since the check now adds zero extra keccak, it was cheap enough to also wire into the zkVM guest path (crates/guest-program/src/common/execution.rs), which previously validated the receipts root but not the bloom — closing that consensus gap in proofs too.
|
Heads-up that the implementation changed meaningfully since the last review (re-review appreciated, @azteca1998):
Rebased onto current |
…bloom # Conflicts: # test/tests/common/mod.rs
…bloom (lambdaclass#6821) **Motivation** The "Benchmark Block execution" job (`pr_perf_blocks_exec.yaml`) started failing with `LogsBloomMismatch` after lambdaclass#6766, which added header `logs_bloom` validation to every import path. The job imports `fixtures/blockchain/l2-1k-erc20.rlp`, a fixture produced by an old ethrex that never populated the header bloom — so every header stored a zero `logs_bloom`, while its ERC20 blocks emit `Transfer` logs. The first such block now fails validation. **Description** Regenerates `l2-1k-erc20.rlp` so its headers carry the correct bloom. The bloom can't be patched in place (the header hash commits to it), so the chain is re-derived: each block is re-executed against a fresh `perf-ci.json` state, the aggregate bloom is recomputed from the executed receipts, written into the header, and every `parent_hash` is re-linked to the corrected predecessor. 611 of 1110 headers gained a non-zero bloom; the corrected chain imports cleanly through the normal validating path. The file size is unchanged (bloom and `parent_hash` are fixed-length), so the diff is a clean LFS pointer swap. This was produced by a one-off script (not kept in-tree). The other ethrex-generated fixtures (`2000-blocks.rlp`, `l2-loadtest.rlp`) were checked the same way and need **no change** — their blocks are plain ETH transfers that emit no logs, so a zero bloom is already correct. Verified with a binary that includes lambdaclass#6766: original fixture → `Invalid Block: Logs bloom does not match`; regenerated fixture → `Import completed blocks=1110`. No client code touched. **Checklist** - [ ] Updated `STORE_SCHEMA_VERSION` (crates/storage/lib.rs) if the PR includes breaking changes to the `Store` requiring a re-sync. — N/A (no `Store` changes)
…lambdaclass#6817) **Motivation** A transaction's chain id is bound into its signature — EIP-155 folds it into `v` for legacy txs, and typed transactions (EIP-1559/2930/4844/7702) carry an explicit `chain_id` field that is part of the signing payload. This binds a signature to a single chain so it cannot be replayed elsewhere. During **block import**, ethrex recovers a typed tx's sender from the tx's *own* chain-id signing domain and never re-checks that chain id against the node's chain. The only chain-id check lives in the mempool (`MempoolError::InvalidChainId`), but blocks arriving via `engine_newPayload` or p2p block gossip/sync never pass through the mempool. As a result a block containing a transaction signed for a *foreign* chain id executes and is **accepted**. geth's signer rejects the same transaction at block validation (`ErrInvalidChainId`), so ethrex would import a block the rest of the network rejects — a consensus divergence reachable on every imported block. **Description** - Adds `InvalidBlockError::InvalidTransactionChainId { have, want }`. - Adds a per-transaction chain-id check to the shared `validate_block_pre_execution` (`crates/common/validation.rs`): every transaction whose `chain_id()` is `Some` and does not equal `chain_config.chain_id` is rejected pre-execution. Because that validator is called by the full-node import path, the L2 block producer, **and** the zkVM guest program, the check is enforced everywhere a block is validated (same approach as lambdaclass#6766 for `logs_bloom`). - Legacy pre-EIP-155 transactions carry no chain id (`chain_id() == None`) and are intentionally left untouched (replay-allowed by design), mirroring the existing mempool check. - `PrivilegedL2Transaction` is exempt: its sender comes from an unsigned `from` field, so chain id is not a signature-binding scalar for it, and on L2 it may legitimately name a different *source* chain for cross-chain deposits (`l1_watcher.rs` builds mint txs with `chain_id = source_chain_id`). On L1 privileged txs are already rejected as an unsupported type (lambdaclass#6752), so the exemption opens no L1 gap. **Test** `test/tests/blockchain/wrong_chain_id_tests.rs` builds a fully-valid block (correct post-state, roots, gas) whose only defect is an EIP-1559 transaction signed for a foreign chain id, and asserts `add_block` rejects it. Confirmed **red before the fix** — `add_block` returned `Ok(())`, i.e. ethrex *accepted* the block — and green after. **Checklist** - [x] Regression test added (red before, green after) - [x] `cargo fmt` + `cargo clippy` clean - [x] Enforced on full-node import, L2 block producer, and zkVM guest paths
5 participants
Motivation
During block import, ethrex validates
gas_used,receipts_root,requests_hash, and the BAL hash, but never checks the block header'slogs_bloomagainst the bloom of the executed receipts. The header bloom is only computed when building a payload, never re-validated on import. The receipts root commits to the per-receipt blooms, not to the header's aggregatelogs_bloomfield, so a block with correct roots but an arbitrarylogs_bloompasses every existing check. geth/reth (and the execution spec) recompute and compare the bloom, so ethrex would import/attest a block the rest of the network rejects — a consensus divergence at the Engine API — and serve a wrong bloom to RPC consumers.Description
The receipts root and the aggregate
logs_bloomare now validated together in a single pass over the receipts, so each receipt's bloom is hashed only once (it feeds both the receipts trie and the OR-ed header bloom):compute_receipts_root_and_logs_bloom(receipts, crypto) -> (H256, Bloom)(crates/common/types/block.rs) andReceipt::encode_inner_with_precomputed_bloomso the bloom computed for the trie is reused for the aggregate instead of being recomputed.validate_receipts_root_and_logs_bloom(header, receipts, crypto)(crates/common/validation.rs), returningInvalidBlockError::ReceiptsRootMismatchor the newInvalidBlockError::LogsBloomMismatch. This replaces the separatevalidate_receipts_root/validate_logs_bloomcalls.crates/blockchain(execute_block,execute_block_pipeline,execute_block_from_state) and the stateless/zkVM guest path (crates/guest-program/src/common/execution.rs), so proof-based execution enforces the same bloom check as full-node import. Because the bloom is already hashed for the receipts root, adding the check costs no extra keccak — which is what made it cheap enough to include in the cycle-counted guest.Tests:
test/tests/blockchain/logs_bloom_tests.rs— end-to-end viaadd_block; now asserts the specificLogsBloomMismatchrejection (not just any error), so the check can't silently rot behind an unrelated failure.test/tests/common/logs_bloom_validation_tests.rs— exercises the validator with receipts that carry logs (non-trivial bloom): matching header passes; a flipped bloom bit →LogsBloomMismatch; a wrong receipts root →ReceiptsRootMismatch; and the single-pass root equals the standalonecompute_receipts_root(guards the dedup refactor).