fix: eth block returns correctly filled logs bloom field#7156
Draft
akaladarshi wants to merge 2 commits into
Draft
fix: eth block returns correctly filled logs bloom field#7156akaladarshi wants to merge 2 commits into
akaladarshi wants to merge 2 commits into
Conversation
akaladarshi
requested review from
LesnyRumcajs and
sudo-shashank
and removed request for
a team
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughExtracts reusable helpers to accrue Ethereum log address/topics into a Bloom and compute a block's logs_bloom from executed messages; integrates the helpers into block and receipt construction, adds a unit test, and extracts per-message event collection into a dedicated helper. ChangesBlock Bloom Computation and Event Collection Refactoring
Sequence DiagramsequenceDiagram
participant Block as Block::from_filecoin_tipset_with_full_tx
participant ComputeBloom as compute_block_logs_bloom
participant EventCollect as collect_events_from_messages
participant AccrueLog as accrue_eth_log
participant BloomResult as Block.logs_bloom
Block->>ComputeBloom: state_manager, tipset, executed_messages
ComputeBloom->>EventCollect: collect events from executed_messages
EventCollect->>ComputeBloom: EthLog list
loop per EthLog
ComputeBloom->>AccrueLog: provide log.address + log.topics
AccrueLog->>ComputeBloom: accumulate Bloom bits
end
ComputeBloom->>BloomResult: return accumulated logs_bloom
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsLinked repositories: Couldn't analyze Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/rpc/methods/eth/filter/mod.rs (1)
392-392: ⚡ Quick winAdd context to the error propagation.
The
try_fromconversion lacks.context(), which violates the coding guideline to "add context with.context()when errors occur." While unlikely to fail in practice, adding context improves debuggability.🔧 Suggested fix
- let event_idx_base = u64::try_from(event_count)?; + let event_idx_base = u64::try_from(event_count) + .context("event count exceeds u64::MAX")?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/methods/eth/filter/mod.rs` at line 392, The conversion call let event_idx_base = u64::try_from(event_count)? should propagate a contextual error; replace the bare ? with a Context-wrapped error (e.g., call .context(...) before ?). Update the expression referencing u64::try_from(event_count) so failures include a clear message like "failed to convert event_count to u64" (affecting the event_idx_base assignment) to satisfy the `.context()` guideline.Source: Coding guidelines
src/rpc/methods/eth.rs (1)
4562-4600: ⚡ Quick winConsider adding a test for
compute_block_logs_bloom.The current test validates
accrue_eth_logbehavior well (empty bloom, single log, OR composition, idempotence). However, there's no unit test forcompute_block_logs_bloomitself, which handles event collection and address resolution. An integration test or unit test covering:
- Blocks with no events → empty bloom
- Blocks with valid EVM events → correct bloom
- Blocks with mixed valid/invalid events → partial bloom
would increase confidence in the full bloom computation path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rpc/methods/eth/filter/mod.rs`:
- Around line 358-365: Add a doc comment above the public function
collect_events_from_messages that succinctly describes its purpose (collect
events from executed messages in a tipset), its parameters (state_manager:
&StateManager, tipset: &Tipset, executed_messages: &[ExecutedMessage], spec:
Option<&impl Matcher>, skip_event: SkipEvent, collected_events: &mut
Vec<CollectedEvent>), its behavior (filters/matches events using spec, skips
events per skip_event, appends found CollectedEvent items into
collected_events), and its return value (anyhow::Result<()> indicating success
or error); make sure the doc mentions that collected_events is mutated in-place
and any notable side effects or error conditions the caller should expect.
---
Nitpick comments:
In `@src/rpc/methods/eth/filter/mod.rs`:
- Line 392: The conversion call let event_idx_base = u64::try_from(event_count)?
should propagate a contextual error; replace the bare ? with a Context-wrapped
error (e.g., call .context(...) before ?). Update the expression referencing
u64::try_from(event_count) so failures include a clear message like "failed to
convert event_count to u64" (affecting the event_idx_base assignment) to satisfy
the `.context()` guideline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 80cc42cd-3208-4465-a098-75793490f3be
📒 Files selected for processing (2)
src/rpc/methods/eth.rssrc/rpc/methods/eth/filter/mod.rs
Comment on lines
+358
to
+365
| pub async fn collect_events_from_messages( | ||
| state_manager: &StateManager, | ||
| tipset: &Tipset, | ||
| executed_messages: &[ExecutedMessage], | ||
| spec: Option<&impl Matcher>, | ||
| skip_event: SkipEvent, | ||
| collected_events: &mut Vec<CollectedEvent>, | ||
| ) -> anyhow::Result<()> { |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add doc comment for the new public function.
The new collect_events_from_messages function is public but lacks documentation. As per coding guidelines, all public functions should have doc comments explaining their purpose, parameters, and behavior.
📝 Suggested documentation
+ /// Collects events from executed messages for a given tipset.
+ ///
+ /// This is a lower-level helper that operates on already-loaded executed messages,
+ /// enabling reuse from both RPC event collection and block bloom computation.
+ ///
+ /// # Parameters
+ /// - `state_manager`: State manager for address resolution
+ /// - `tipset`: The tipset containing the executed messages
+ /// - `executed_messages`: Pre-loaded executed messages from the tipset
+ /// - `spec`: Optional filter spec to match specific events
+ /// - `skip_event`: Strategy for handling unresolved addresses
+ /// - `collected_events`: Output buffer for matching events
pub async fn collect_events_from_messages(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub async fn collect_events_from_messages( | |
| state_manager: &StateManager, | |
| tipset: &Tipset, | |
| executed_messages: &[ExecutedMessage], | |
| spec: Option<&impl Matcher>, | |
| skip_event: SkipEvent, | |
| collected_events: &mut Vec<CollectedEvent>, | |
| ) -> anyhow::Result<()> { | |
| /// Collects events from executed messages for a given tipset. | |
| /// | |
| /// This is a lower-level helper that operates on already-loaded executed messages, | |
| /// enabling reuse from both RPC event collection and block bloom computation. | |
| /// | |
| /// # Parameters | |
| /// - `state_manager`: State manager for address resolution | |
| /// - `tipset`: The tipset containing the executed messages | |
| /// - `executed_messages`: Pre-loaded executed messages from the tipset | |
| /// - `spec`: Optional filter spec to match specific events | |
| /// - `skip_event`: Strategy for handling unresolved addresses | |
| /// - `collected_events`: Output buffer for matching events | |
| pub async fn collect_events_from_messages( | |
| state_manager: &StateManager, | |
| tipset: &Tipset, | |
| executed_messages: &[ExecutedMessage], | |
| spec: Option<&impl Matcher>, | |
| skip_event: SkipEvent, | |
| collected_events: &mut Vec<CollectedEvent>, | |
| ) -> anyhow::Result<()> { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/rpc/methods/eth/filter/mod.rs` around lines 358 - 365, Add a doc comment
above the public function collect_events_from_messages that succinctly describes
its purpose (collect events from executed messages in a tipset), its parameters
(state_manager: &StateManager, tipset: &Tipset, executed_messages:
&[ExecutedMessage], spec: Option<&impl Matcher>, skip_event: SkipEvent,
collected_events: &mut Vec<CollectedEvent>), its behavior (filters/matches
events using spec, skips events per skip_event, appends found CollectedEvent
items into collected_events), and its return value (anyhow::Result<()>
indicating success or error); make sure the doc mentions that collected_events
is mutated in-place and any notable side effects or error conditions the caller
should expect.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #7151
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Refactor
Tests
Chores