Skip to content

fix: eth block returns correctly filled logs bloom field#7156

Open
akaladarshi wants to merge 10 commits into
mainfrom
akaladarshi/fix-logs-bloom
Open

fix: eth block returns correctly filled logs bloom field#7156
akaladarshi wants to merge 10 commits into
mainfrom
akaladarshi/fix-logs-bloom

Conversation

@akaladarshi

@akaladarshi akaladarshi commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Summary of changes

Changes introduced in this pull request:

  • While creating the full ethereum block data, the log bloom field is correctly filed as the Ethereum yellow paper suggested.
  • Now we have a new column in DB where we store the computed logs bloom, so we don't have to compute if it is available in the DB.

Reference issue to close (if applicable)

Closes #7151

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Outside contributions

  • I have read and agree to the CONTRIBUTING document.
  • I have read and agree to the AI Policy document. I understand that failure to comply with the guidelines will lead to rejection of the pull request.

Summary by CodeRabbit

  • Bug Fixes
    • Corrected Ethereum block logsBloom values so they are computed from actual block logs instead of using a placeholder value.
    • Improved bloom handling to keep receipts and block data consistent, including better pruning of outdated bloom records.
    • Added support for storing and retrieving bloom data during database upgrades and normal operation.
  • Chores
    • Updated snapshots, filters, and tests to reflect the new block log bloom behavior.

@akaladarshi akaladarshi requested a review from a team as a code owner June 9, 2026 15:54
@akaladarshi akaladarshi requested review from LesnyRumcajs and sudo-shashank and removed request for a team June 9, 2026 15:54
@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 27.12% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: fixing Ethereum block logsBloom population.
Linked Issues check ✅ Passed The PR computes block logsBloom from emitted logs, preserves receipt blooms, and applies it to block responses as requested in #7151.
Out of Scope Changes check ✅ Passed The DB, migration, GC, and test snapshot updates support storing and validating block blooms, so they remain in scope.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch akaladarshi/fix-logs-bloom
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch akaladarshi/fix-logs-bloom

Comment @coderabbitai help to get the list of available commands.

@akaladarshi akaladarshi added the RPC requires calibnet RPC checks to run on CI label Jun 9, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/rpc/methods/eth/filter/mod.rs (1)

392-392: ⚡ Quick win

Add context to the error propagation.

The try_from conversion 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 win

Consider adding a test for compute_block_logs_bloom.

The current test validates accrue_eth_log behavior well (empty bloom, single log, OR composition, idempotence). However, there's no unit test for compute_block_logs_bloom itself, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 7bafcfb and 45541b5.

📒 Files selected for processing (2)
  • src/rpc/methods/eth.rs
  • src/rpc/methods/eth/filter/mod.rs

Comment thread src/rpc/methods/eth/filter/mod.rs
@akaladarshi akaladarshi marked this pull request as draft June 10, 2026 07:20
@akaladarshi akaladarshi force-pushed the akaladarshi/fix-logs-bloom branch 2 times, most recently from b0be642 to 96b6e9c Compare June 19, 2026 16:54
@akaladarshi akaladarshi marked this pull request as ready for review June 19, 2026 16:54
@codecov

codecov Bot commented Jun 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.68966% with 143 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.17%. Comparing base (c13f3c0) to head (8042ce8).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/db/migration/v0_33_8.rs 6.17% 76 Missing ⚠️
src/db/car/many.rs 0.00% 14 Missing ⚠️
src/db/parity_db/gc.rs 0.00% 14 Missing ⚠️
...tool/subcommands/api_cmd/generate_test_snapshot.rs 0.00% 14 Missing ⚠️
src/rpc/methods/eth.rs 84.81% 6 Missing and 6 partials ⚠️
src/db/gc/snapshot.rs 0.00% 6 Missing ⚠️
src/db/parity_db.rs 86.11% 2 Missing and 3 partials ⚠️
src/db/mod.rs 85.71% 0 Missing and 2 partials ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/db/db_impl.rs 34.32% <ø> (ø)
src/db/memory.rs 91.30% <100.00%> (+1.83%) ⬆️
src/db/migration/migration_map.rs 89.74% <ø> (ø)
src/rpc/methods/eth/filter/mod.rs 88.56% <ø> (ø)
src/db/mod.rs 62.00% <85.71%> (+9.22%) ⬆️
src/db/parity_db.rs 68.48% <86.11%> (+1.64%) ⬆️
src/db/gc/snapshot.rs 0.00% <0.00%> (ø)
src/rpc/methods/eth.rs 66.70% <84.81%> (+0.47%) ⬆️
src/db/car/many.rs 51.82% <0.00%> (-2.80%) ⬇️
src/db/parity_db/gc.rs 29.93% <0.00%> (-3.16%) ⬇️
... and 2 more

... and 4 files with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c13f3c0...8042ce8. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sudo-shashank sudo-shashank left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like lotus we should store the computed logs bloom so we don't re-compute it again and again.

@sudo-shashank

Copy link
Copy Markdown
Contributor

There are few more changes required and needs to be ported from the list of changes filecoin-project/lotus#13618 for both correctness and performance.

- Add a tipset_bloom table to the chain index DB for block-level ETH log blooms.
- Add a migration for existing chain index DBs.
- Compute and store tipset-level blooms while indexing actor events.
- Store empty blooms for indexed tipsets with no events.
- Rebuild/store blooms when previously reverted events are unreverted.
- Delete bloom rows on revert.
- Delete old bloom rows during chain index GC.
- Expose GetTipsetBloom on the chain indexer.
- Use indexed blooms when constructing ETH block responses for:
- eth_getBlockByHash
- eth_getBlockByNumber
- new-head subscription block payloads
- Preserve compatibility by falling back to the previous full bloom when no indexed bloom is available.
- Extend TestTxReceiptBloom to cover receipt bloom, empty block bloom, block-by-hash bloom, and block-by-number bloom.

@akaladarshi

Copy link
Copy Markdown
Collaborator Author

There are few more changes required and needs to be ported from the list of changes filecoin-project/lotus#13618 for both correctness and performance.

- Add a tipset_bloom table to the chain index DB for block-level ETH log blooms.
- Add a migration for existing chain index DBs.
- Compute and store tipset-level blooms while indexing actor events.
- Store empty blooms for indexed tipsets with no events.
- Rebuild/store blooms when previously reverted events are unreverted.
- Delete bloom rows on revert.
- Delete old bloom rows during chain index GC.
- Expose GetTipsetBloom on the chain indexer.
- Use indexed blooms when constructing ETH block responses for:
- eth_getBlockByHash
- eth_getBlockByNumber
- new-head subscription block payloads
- Preserve compatibility by falling back to the previous full bloom when no indexed bloom is available.
- Extend TestTxReceiptBloom to cover receipt bloom, empty block bloom, block-by-hash bloom, and block-by-number bloom.

@sudo-shashank When I started working on this, there was a possibility of us shifting to the SQL DB since, that is not the case anymore I will store the data in the current index DB

@akaladarshi akaladarshi marked this pull request as draft June 24, 2026 05:34
@akaladarshi akaladarshi force-pushed the akaladarshi/fix-logs-bloom branch from 96b6e9c to 887dba6 Compare July 1, 2026 12:21
@akaladarshi akaladarshi marked this pull request as ready for review July 1, 2026 12:21

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (4)
CHANGELOG.md (1)

50-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reference the issue instead of the PR.

This entry should link to issue #7151 rather than PR #7156 so the changelog follows the repo’s traceability convention. Based on learnings, when both exist, use [#ISSUE_NO](link-to-issue): description.

🤖 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 `@CHANGELOG.md` at line 50, The changelog entry currently links to the pull
request instead of the tracking issue, so update the bullet in CHANGELOG.md to
reference issue `#7151` using the repo’s standard [`#ISSUE_NO`](link-to-issue):
description format. Keep the existing description text, but change the linked
identifier from the PR reference to the issue reference so the entry matches the
traceability convention.

Source: Learnings

src/db/gc/snapshot.rs (1)

351-358: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Silent no-op if heaviest_car_tipset() fails.

The delete_blooms_before_height error path logs a warning, but if db.heaviest_car_tipset() itself returns Err, the whole pruning step silently no-ops with no log line, unlike the similar Ok(ts) pattern used later in this function (Lines 365-378) where failures elsewhere are still surfaced. Since this can leave stale bloom rows accumulating unnoticed, consider logging on the Err branch too.

Proposed fix
-            if let Ok(head) = db.heaviest_car_tipset() {
-                let cutoff = head.epoch() - self.recent_state_roots;
-                if let Err(e) = db.delete_blooms_before_height(cutoff) {
-                    tracing::warn!("failed to prune stale block blooms: {e:#}");
-                }
-            }
+            match db.heaviest_car_tipset() {
+                Ok(head) => {
+                    let cutoff = head.epoch() - self.recent_state_roots;
+                    if let Err(e) = db.delete_blooms_before_height(cutoff) {
+                        tracing::warn!("failed to prune stale block blooms: {e:#}");
+                    }
+                }
+                Err(e) => tracing::warn!("failed to get heaviest car tipset for bloom pruning: {e:#}"),
+            }
🤖 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/db/gc/snapshot.rs` around lines 351 - 358, The pruning block in snapshot
cleanup silently skips work when db.heaviest_car_tipset() returns Err, so add
explicit logging in that error branch instead of only handling the Ok(head)
case. Update the bloom-pruning logic in the snapshot cleanup path to mirror the
later Ok(ts) style used in the same function by surfacing the failure with
tracing::warn! (or equivalent) and keeping the existing
delete_blooms_before_height error handling unchanged.
src/db/tests/subtests/mod.rs (1)

71-100: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Consider adding an exact-boundary case to the prune test.

The test exercises heights 100 (deleted, < 150) and 200 (kept, > 150), but never a bloom stored exactly at the cutoff (height == 150), which per decode_block_bloom's h < height check should be kept. A small addition would pin down the inclusive boundary explicitly.

🤖 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/db/tests/subtests/mod.rs` around lines 71 - 100, The block bloom prune
test in block_bloom_prune only covers values below and above the cutoff, so add
an exact-cutoff case to lock in the boundary behavior. In the same test, write a
bloom through EthBlockBloomStore at height 150, then call
delete_blooms_before_height(150) and assert that read_bloom still returns it,
alongside the existing assertions for the lower and higher heights.
src/db/memory.rs (1)

150-172: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Resilient decode-failure handling — good pattern, but see parity_db.rs for a divergent (fail-fast) counterpart.

retain here silently drops any entry that fails to decode, which is a reasonable, resilient default for pruning. Note that ParityDb::delete_blooms_before_height (src/db/parity_db.rs) instead propagates a decode error via ?, aborting the entire prune on a single corrupted row — see the comment there for the concrete risk. Worth aligning both implementations to the same failure semantics.

🤖 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/db/memory.rs` around lines 150 - 172, The bloom-pruning logic in
MemoryDB::delete_blooms_before_height currently treats decode failures as silent
drops, which diverges from the fail-fast behavior in
ParityDb::delete_blooms_before_height. Align the failure semantics between the
two implementations by choosing one consistent approach and applying it in the
delete_blooms_before_height path, using the same decode_block_bloom handling
pattern in both MemoryDB and ParityDb so a corrupted entry is either always
tolerated or always surfaced the same way.
🤖 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/db/migration/v0_33_7.rs`:
- Around line 46-54: Rollback the database rename if `parity_db::Db::add_column`
fails in `v0_33_7` so `old_db` is not left as an empty placeholder while
`temp_db` still contains the real data. Update the migration flow around the
`rename`, `create_dir_all`, and `add_column` steps to restore the original
directory or clean up both `old_db` and `temp_db` before returning the error, so
later startup logic does not delete the only valid copy.

In `@src/db/mod.rs`:
- Around line 156-170: decode_block_bloom currently accepts any payload after
the 8-byte height prefix, so malformed cached blooms can slip through as
logsBloom. Tighten the validation in decode_block_bloom (and any callers that
read its output) to require exactly the expected 256-byte bloom length, and
treat anything shorter or longer as invalid so the cache path falls back or is
ignored.

In `@src/db/parity_db.rs`:
- Around line 269-282: In `ParityDB::delete_blooms_before_height`, a single bad
bloom entry currently aborts pruning because `decode_block_bloom(&entry)?`
propagates immediately. Update this loop to mirror
`MemoryDB::delete_blooms_before_height` by handling decode failures per entry:
skip malformed rows, optionally log them, and continue collecting stale keys for
the rest. Keep the pruning path resilient so one corrupted `EthBlockBloom`
record does not stop `self.db.commit(...)` from deleting the valid stale
entries.

---

Nitpick comments:
In `@CHANGELOG.md`:
- Line 50: The changelog entry currently links to the pull request instead of
the tracking issue, so update the bullet in CHANGELOG.md to reference issue
`#7151` using the repo’s standard [`#ISSUE_NO`](link-to-issue): description format.
Keep the existing description text, but change the linked identifier from the PR
reference to the issue reference so the entry matches the traceability
convention.

In `@src/db/gc/snapshot.rs`:
- Around line 351-358: The pruning block in snapshot cleanup silently skips work
when db.heaviest_car_tipset() returns Err, so add explicit logging in that error
branch instead of only handling the Ok(head) case. Update the bloom-pruning
logic in the snapshot cleanup path to mirror the later Ok(ts) style used in the
same function by surfacing the failure with tracing::warn! (or equivalent) and
keeping the existing delete_blooms_before_height error handling unchanged.

In `@src/db/memory.rs`:
- Around line 150-172: The bloom-pruning logic in
MemoryDB::delete_blooms_before_height currently treats decode failures as silent
drops, which diverges from the fail-fast behavior in
ParityDb::delete_blooms_before_height. Align the failure semantics between the
two implementations by choosing one consistent approach and applying it in the
delete_blooms_before_height path, using the same decode_block_bloom handling
pattern in both MemoryDB and ParityDb so a corrupted entry is either always
tolerated or always surfaced the same way.

In `@src/db/tests/subtests/mod.rs`:
- Around line 71-100: The block bloom prune test in block_bloom_prune only
covers values below and above the cutoff, so add an exact-cutoff case to lock in
the boundary behavior. In the same test, write a bloom through
EthBlockBloomStore at height 150, then call delete_blooms_before_height(150) and
assert that read_bloom still returns it, alongside the existing assertions for
the lower and higher heights.
🪄 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: a154e991-22c1-4133-82f4-19962bd5eaa2

📥 Commits

Reviewing files that changed from the base of the PR and between 96b6e9c and 887dba6.

📒 Files selected for processing (19)
  • CHANGELOG.md
  • scripts/tests/api_compare/filter-list
  • scripts/tests/api_compare/filter-list-gateway
  • src/db/car/many.rs
  • src/db/db_impl.rs
  • src/db/gc/snapshot.rs
  • src/db/memory.rs
  • src/db/migration/migration_map.rs
  • src/db/migration/mod.rs
  • src/db/migration/v0_33_7.rs
  • src/db/mod.rs
  • src/db/parity_db.rs
  • src/db/parity_db/gc.rs
  • src/db/tests/mem_test.rs
  • src/db/tests/parity_test.rs
  • src/db/tests/subtests/mod.rs
  • src/rpc/methods/eth.rs
  • src/tool/subcommands/api_cmd/generate_test_snapshot.rs
  • src/tool/subcommands/api_cmd/test_snapshots.txt
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • filecoin-project/lotus (manual)
💤 Files with no reviewable changes (1)
  • scripts/tests/api_compare/filter-list
✅ Files skipped from review due to trivial changes (2)
  • src/db/migration/mod.rs
  • src/db/db_impl.rs
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/tool/subcommands/api_cmd/test_snapshots.txt
  • scripts/tests/api_compare/filter-list-gateway
  • src/rpc/methods/eth.rs

Comment thread src/db/migration/v0_33_7.rs Outdated
Comment on lines +46 to +54
std::fs::rename(&old_db, &temp_db).context("failed to rename database directory")?;

// Create a placeholder so the delete step succeeds
std::fs::create_dir_all(&old_db).context("failed to create placeholder directory")?;

info!("Adding EthBlockBloom column to database");
let mut opts = paritydb_0_33_6::to_options(temp_db.clone());
parity_db::Db::add_column(&mut opts, paritydb_0_33_6::eth_block_bloom_column_options())
.context("failed to add EthBlockBloom column")?;

@coderabbitai coderabbitai Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect migration wrapper behavior around migrate_core failures.
fd -p 'migration_map.rs' src/db/migration --exec sed -n '1,140p' {}
fd -p 'v0_33_7.rs' src/db/migration --exec sed -n '37,58p' {}

Repository: ChainSafe/forest

Length of output: 6587


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the migration flow, rollback/error handling, and any tests covering failure paths.
rg -n "fn migrate\(|migrate_core\(|remove_dir_all\(|rename\(|rollback|temporary_db_path|migrate failed|migration" src/db/migration -g '!**/target/**'

echo '--- v0_33_7 ---'
sed -n '1,220p' src/db/migration/v0_33_7.rs

echo '--- migration map ---'
sed -n '1,220p' src/db/migration/migration_map.rs

echo '--- search tests ---'
rg -n "v0_33_7|Migration0_33_6_0_33_7|temporary_db|rollback|failed to add EthBlockBloom" src -g '*test*' -g '*tests*'

Repository: ChainSafe/forest

Length of output: 31336


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect how startup/version detection interacts with leftover old_db/temp_db directories.
sed -n '1,260p' src/db/migration/db_migration.rs

echo '---'
rg -n "current db version|version.*dir|old_db_path|new_db_path|temporary_db_path|is_migration_required|Mode::Current|current mode|version database|db version" src -g '!**/target/**'

Repository: ChainSafe/forest

Length of output: 9388


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect version detection and how leftover placeholder/temp directories are interpreted.
sed -n '1,220p' src/db/db_mode.rs

Repository: ChainSafe/forest

Length of output: 5437


Rollback the rename on add_column failure. If this errors, old_db is left as an empty semver-named placeholder while temp_db still holds the only copy of the database; the next startup will treat the placeholder as the source DB and pre_checks will delete temp_db, losing the data. Restore old_db or clean up both paths before returning the error.

🤖 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/db/migration/v0_33_7.rs` around lines 46 - 54, Rollback the database
rename if `parity_db::Db::add_column` fails in `v0_33_7` so `old_db` is not left
as an empty placeholder while `temp_db` still contains the real data. Update the
migration flow around the `rename`, `create_dir_all`, and `add_column` steps to
restore the original directory or clean up both `old_db` and `temp_db` before
returning the error, so later startup logic does not delete the only valid copy.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems viable, no @akaladarshi ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LesnyRumcajs Sounds good — I'll hold off pending @akaladarshi's input on the rollback approach.


🧠 Learnings used
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/cron_state.rs:8-8
Timestamp: 2026-01-05T12:54:40.850Z
Learning: In Rust code reviews, do not derive Eq for a struct if any field does not implement Eq (e.g., types from external dependencies). If a type like CronStateLotusJson includes fields wrapping external dependencies that lack Eq, derive PartialEq (or implement PartialEq manually) but avoid deriving Eq. This ensures comparisons compile and reflect actual equivalence semantics. When needed, consider implementing custom PartialEq (and possibly Eq) only after ensuring all fields (or wrappers) implement Eq, or keep PartialEq-only if full equality semantics cannot be expressed.

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/evm_state.rs:41-44
Timestamp: 2026-01-05T12:56:13.802Z
Learning: In Rust codebases (e.g., Forest), do not add #[cfg(test)] to functions already annotated with #[test]. The #[test] attribute ensures the function is compiled only for tests, so a separate #[cfg(test)] is redundant and can be removed if present. Apply this check to all Rust files that contain #[test] functions.

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6666
File: src/tool/subcommands/archive_cmd.rs:628-631
Timestamp: 2026-03-02T09:43:34.946Z
Learning: In Rust sources related to Forest snapshot handling, implement the rule: enable message_receipts and events (message_receipts: true, events: true) only for GC snapshots as defined in src/db/gc/snapshot.rs, since these are internal snapshots created during garbage collection. For user-facing export commands such as src/tool/subcommands/archive_cmd.rs, disable receipts and events by default (message_receipts: false, events: false) to keep user-facing snapshots smaller, unless explicitly requested. This guidance targets Rust files; apply consistently across similar snapshot-related modules, using the narrowest applicable scope when extending beyond the two specified files.

Learnt from: LesnyRumcajs
Repo: ChainSafe/forest PR: 6903
File: src/shim/executor.rs:243-251
Timestamp: 2026-04-14T17:24:13.631Z
Learning: In the ChainSafe/forest (LesnyRumcajs) codebase, do not require rustdoc comments on public methods when the method’s purpose is clearly self-explanatory from its name (e.g., `entries(&self)`, `into_entries(self)`). When reviewing Rust (`.rs`) files, avoid flagging missing documentation for these obviously named public APIs, reserving doc requirements for less clear or non-obvious public methods.

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 7077
File: src/utils/cache/size_tracking.rs:174-182
Timestamp: 2026-05-19T10:55:04.684Z
Learning: In ChainSafe/forest code reviews, avoid raising review comments for issues that are likely compile-time failures (e.g., missing imports, unresolved types, or other compiler errors that CI will catch). Instead, focus feedback on logic, correctness, and design/maintainability concerns, since the CI pipeline reliably verifies compilation.

Learnt from: LesnyRumcajs
Repo: ChainSafe/forest PR: 7113
File: src/rpc/mod.rs:954-975
Timestamp: 2026-05-27T13:30:50.172Z
Learning: In ChainSafe/forest, CI runs tests with `cargo nextest`, which executes each test in its own fresh process. As a result, process-global state is not shared across tests (e.g., metric registries like `crate::metrics::default_registry()`), so assertions on absolute metric counts in tests (e.g., `count == 1`) are expected to be safe. Reviewers should not require before/after delta comparisons or other cross-test pollution guards for metrics count assertions unless a test setup explicitly breaks the test-per-process isolation.

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 7129
File: src/blocks/tipset.rs:192-193
Timestamp: 2026-06-02T11:42:22.733Z
Learning: When implementing or reviewing `get_size2::GetSize` for Rust types in this repository, do not attempt to call `get_heap_size_with_tracker()` directly on fields typed as `Arc<NonEmpty<T>>` (i.e., `nunny::Vec<T>` / `NonEmpty<T>`). Because `nunny::Vec<T>` (NonEmpty<T>) does **not** implement `GetSize`, neither will `Arc<NonEmpty<T>>`. Instead, compute heap size for `Arc<NonEmpty<T>>` fields using the existing `nunny_vec_heap_size_helper` helper (in `src/utils/get_size/mod.rs`), which deref-coerces through the `Arc` to the inner `NonEmpty<T>` and provides the correct heap-size calculation.

Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 7182
File: src/message_pool/msgpool/utils.rs:75-76
Timestamp: 2026-06-16T17:07:30.443Z
Learning: In the ChainSafe/forest codebase, SignedMessage (signed Filecoin messages) will never have an ID address in the sender (`from`) because protocol-level signature verification rejects ID senders. Therefore, when reviewing Rust code that processes SignedMessage and uses a map keyed by the sender `Address` (e.g., `HashMap<Address, ...>`), it is safe to do a direct lookup like `map.get_mut(from)` using the sender address from the SignedMessage. Do not flag this as an address-type mismatch or require iterating/resolving keys when the lookup is based on the SignedMessage’s own `from` value.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai check again

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, something went wrong! Please try again later. 🐰 💔

Comment thread src/db/mod.rs Outdated
Comment thread src/db/parity_db.rs Outdated
Comment thread src/db/car/many.rs Outdated
EthBlockBloomStore::write_bloom(self.writer(), key, height, bloom)
}

fn delete_blooms_before_height(&self, height: i64) -> anyhow::Result<()> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn delete_blooms_before_height(&self, height: i64) -> anyhow::Result<()> {
fn delete_blooms_before_height(&self, height: ChainEpoch) -> anyhow::Result<()> {

Here and elsewhere. It's not exactly strict typing, but type aliases in Rust exist for areason.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment thread src/db/gc/snapshot.rs
.unwrap_or_default()
);

// Prune blooms whose events are no longer retained by the lite snapshot.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll let @hanabi1224 chip in here.

Comment thread src/db/migration/migration_map.rs Outdated
"0.22.0" -> "0.22.1" @ Migration0_22_0_0_22_1,
"0.25.3" -> "0.26.0" @ Migration0_25_3_0_26_0,
"0.30.5" -> "0.31.0" @ Migration0_30_5_0_31_0,
"0.33.6" -> "0.33.7" @ Migration0_33_6_0_33_7,

@LesnyRumcajs LesnyRumcajs Jul 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These need to be updated, no?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah 0.33.7 is already released, will update to 0.33.8

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@akaladarshi akaladarshi force-pushed the akaladarshi/fix-logs-bloom branch from 488c3c4 to 0fd4c9d Compare July 2, 2026 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RPC requires calibnet RPC checks to run on CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

eth: eth_getBlockBy* / newHeads return an all-ones logsBloom

3 participants