fix(evm:PLA-716): validate block bloom emptiness by default#48
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dab939ea4b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !dirs.ValidateLogsBloomEmptiness && !dirs.ValidateLogsBloomMatch { | ||
| return nil | ||
| } | ||
| if dirs.GroundTruthLogs == nil || block == nil || block.LogsBloom == "" { |
There was a problem hiding this comment.
validateBlockLogsBloom returns early when block.LogsBloom == "", which bypasses both emptiness and full-match validation even when GroundTruthLogs is provided. In the scenario this change targets (using ground-truth logs to detect inconsistent block data), an upstream that omits logsBloom will now be accepted instead of rejected, so malformed/incomplete block payloads can slip through integrity checks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR strengthens EVM block integrity validation by defaulting validateLogsBloomEmptiness to on and adding block-level bloom validation against caller-provided GroundTruthLogs, with accompanying fixtures/tests and a recomputation benchmark.
Changes:
- Enable
validateLogsBloomEmptinessby default via directive defaults and document the new default. - Add block-level
logsBloomvalidation againstGroundTruthLogsfor emptiness consistency and optional full bloom matching. - Add static fixtures + regression tests + benchmarks for bloom recomputation cost at different log volumes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/pages/config/failsafe/integrity.mdx | Documents validateLogsBloomEmptiness as default-on and updates the sample config commentary accordingly. |
| common/defaults.go | Sets DirectiveDefaultsConfig.ValidateLogsBloomEmptiness to default true. |
| common/defaults_test.go | Adds regression test asserting directive defaults include core integrity defaults (incl. bloom emptiness). |
| architecture/evm/eth_getBlockByNumber.go | Adds ground-truth-based block logsBloom validation (emptiness + optional full match). |
| architecture/evm/eth_getBlockByNumber_validation_test.go | Adds fixtures-driven tests for emptiness/match behavior plus a benchmark for bloom recomputation. |
| architecture/evm/testdata/block_validation_block.json | Adds a minimal block fixture containing logsBloom used for validation tests. |
| architecture/evm/testdata/block_validation_logs.json | Adds a minimal ground-truth logs fixture used for bloom validation tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| gtLogCount := 0 | ||
| for _, log := range dirs.GroundTruthLogs { | ||
| if log != nil { | ||
| gtLogCount++ | ||
| } | ||
| } |
There was a problem hiding this comment.
validateBlockLogsBloom iterates over dirs.GroundTruthLogs twice when ValidateLogsBloomMatch is enabled (first to count non-nil logs, then again to recompute the bloom). For large ground-truth log sets this adds measurable overhead; consider combining the count with the bloom recomputation loop (or only doing the pre-count when ValidateLogsBloomEmptiness is enabled and match is disabled).
| } | ||
|
|
||
| if !bytes.Equal(expected, providedBloom) { | ||
| return common.NewErrEndpointContentValidation(fmt.Errorf("block logsBloom does not match ground-truth logs"), u) |
There was a problem hiding this comment.
The error returned on bloom mismatch ("block logsBloom does not match ground-truth logs") lacks identifying context (e.g., block number/hash and how many ground-truth logs were used), which makes upstream debugging harder. Consider including block.Number / block.Hash (when present) and gtLogCount in the validation error message.
| return common.NewErrEndpointContentValidation(fmt.Errorf("block logsBloom does not match ground-truth logs"), u) | |
| return common.NewErrEndpointContentValidation( | |
| fmt.Errorf("block logsBloom does not match ground-truth logs (non-nil ground-truth log records: %d)", gtLogCount), | |
| u, | |
| ) |
|
@claude review |
|
Claude finished @0x666c6f's task in 2m 5s —— View job Claude's Review
Overall this is clean, well-structured work. The logic is clear, the fixtures are minimal and useful, and the benchmark gives real data on recomputation cost. A few issues are worth addressing before merging.
|
| Severity | Issue |
|---|---|
| 🔴 | Empty logsBloom guard bypasses emptiness validation |
| 🟡 | Double-iteration of ground-truth logs when both flags are on |
| 🟡 | Bloom mismatch error lacks block number/hash and log count |
| 🟡 | Missing test cases for zero-bloom and empty-string logsBloom paths |
| 🟢 | Default-on change needs migration note consideration |
|
|
@CodeW review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if !dirs.ValidateLogsBloomEmptiness && !dirs.ValidateLogsBloomMatch { | ||
| return nil | ||
| } | ||
| if dirs.GroundTruthLogs == nil || block == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
validateBlockLogsBloom relies on the distinction between GroundTruthLogs == nil (no ground truth provided → skip) and len(GroundTruthLogs) == 0 (explicitly provided empty logs → should validate emptiness). However, common.RequestDirectives.Clone() currently only copies GroundTruthLogs when len(d.GroundTruthLogs) > 0, so an explicit empty slice will be dropped during directive cloning (e.g., code paths that do nq.Directives().Clone()), causing this validation to be skipped unexpectedly. Preserve the “explicit empty” semantics by updating RequestDirectives.Clone() to copy GroundTruthLogs when the slice is non-nil (even if empty).
There was a problem hiding this comment.
Fixed in 1d67aa3. Block-level bloom validation now requires GroundTruthLogsComplete, and RequestDirectives.Clone() preserves explicit empty GroundTruthLogs slices plus the completeness flag. See common/request.go and architecture/evm/eth_getBlockByNumber.go.
| fmt.Errorf("block logsBloom is non-zero but got 0 ground-truth log records"), | ||
| u, | ||
| ) | ||
| } | ||
| if bloomIsZero && hasLogs { | ||
| return common.NewErrEndpointContentValidation( | ||
| fmt.Errorf("block logsBloom is zero but got %d ground-truth log records", gtLogCount), |
There was a problem hiding this comment.
The new bloom/log emptiness validation errors don’t include block identifiers (number/hash), unlike the missing/mismatch errors below. Including block.Number/block.Hash (and maybe gtLogCount) in these two messages would make upstream debugging much easier, especially now that this check is default-on.
| fmt.Errorf("block logsBloom is non-zero but got 0 ground-truth log records"), | |
| u, | |
| ) | |
| } | |
| if bloomIsZero && hasLogs { | |
| return common.NewErrEndpointContentValidation( | |
| fmt.Errorf("block logsBloom is zero but got %d ground-truth log records", gtLogCount), | |
| fmt.Errorf( | |
| "block logsBloom is non-zero but got %d ground-truth log records (block number=%s hash=%s)", | |
| gtLogCount, | |
| block.Number, | |
| block.Hash, | |
| ), | |
| u, | |
| ) | |
| } | |
| if bloomIsZero && hasLogs { | |
| return common.NewErrEndpointContentValidation( | |
| fmt.Errorf( | |
| "block logsBloom is zero but got %d ground-truth log records (block number=%s hash=%s)", | |
| gtLogCount, | |
| block.Number, | |
| block.Hash, | |
| ), |
There was a problem hiding this comment.
Fixed in 1d67aa3. The emptiness validation errors now include block number/hash context; see architecture/evm/eth_getBlockByNumber.go.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2678ff3e4
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !bloomIsZero && !hasLogs { | ||
| return common.NewErrEndpointContentValidation( | ||
| fmt.Errorf("block logsBloom is non-zero but got 0 ground-truth log records"), |
There was a problem hiding this comment.
logsBloom represents all logs in the block, but this branch assumes GroundTruthLogs is a complete block log set and rejects any non-zero bloom when that slice is empty. If a library caller populates GroundTruthLogs from a filtered eth_getLogs query (address/topic filters), empty results are valid even when the block has unrelated logs, so this check produces false content-validation failures and unnecessary retries; with the new default-on emptiness directive this becomes a production-facing regression.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
logsBloomemptiness validation foreth_getBlockByNumberwhen ground-truth logs are availablevalidateLogsBloomEmptinessdefault-on so inconsistent block/log emptiness is rejected automaticallyChanges
logsBloomagainstGroundTruthLogsfor emptiness and optional full bloom matchingvalidateLogsBloomEmptinessLinear