fix: prefer `Tipset::persist` over `Block::persist` #6083

hanabi1224 · 2025-09-15T14:38:51Z

Summary of changes

Changes introduced in this pull request:

prefer Tipset::persist over Block::persist to ensure message root of every block is persisted

Reference issue to close (if applicable)

Closes

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.

Summary by CodeRabbit

Refactor
- Consolidated data persistence at the tipset level, reducing multiple per-block writes to a single call.
- Simplified chain syncing and finalization flows by using bulk persistence.
- Removed a redundant public persistence method to streamline the API surface.
Chores
- Reduced internal dependencies by removing an unused trait import.
Impact
- Improves efficiency and reliability of data persistence during sync and finalization.
- Lowers complexity, making related operations faster and less error-prone.

coderabbitai · 2025-09-15T14:38:59Z

Walkthrough

Removed Block::persist and its Blockstore dependency. Refactored FullTipset::persist to persist header and messages directly. Updated chain follower and RPC finalize handlers to call FullTipset::persist instead of per-block persistence. Validation logic remains unchanged.

Changes

Cohort / File(s)	Summary
Block API removal `src/blocks/block.rs`	Removed `Block::persist(&impl Blockstore)` and the `Blockstore` import; other APIs unchanged.
FullTipset persistence refactor `src/blocks/tipset.rs`	`FullTipset::persist` now persists components directly: header, BLS messages, then Secp messages via `persist_objects`; retains `TipsetValidator::validate_msg_root`.
Chain follower update `src/chain_sync/chain_follower.rs`	In `get_full_tipset_batch`, replaced per-block persistence loop with `tipset.persist(chain_store.blockstore())`.
RPC finalize update `src/rpc/methods/f3.rs`	In `Finalize::handle`, replaced per-block persistence loop with `fts.persist(ctx.store())`.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller as ChainFollower/RPC
  participant FTS as FullTipset
  participant Validator as TipsetValidator
  participant Store as Blockstore

  Caller->>FTS: persist(db)
  note right of FTS: New flow: persist components directly
  FTS->>Validator: validate_msg_root(db, block)
  Validator-->>FTS: ok or error
  alt validation ok
    FTS->>Store: persist_objects([header])
    Store-->>FTS: ok
    FTS->>Store: persist_objects([BLS messages])
    Store-->>FTS: ok
    FTS->>Store: persist_objects([Secp messages])
    Store-->>FTS: ok
    FTS-->>Caller: ok
  else validation error
    FTS-->>Caller: error
  end

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

LesnyRumcajs
elmattic
akaladarshi

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title succinctly and accurately describes the primary change: preferring Tipset::persist instead of Block::persist. It is specific, concise, and directly reflects the changes in the diff and PR objectives (removing Block::persist and updating callers). The "fix:" prefix follows conventional commit style and is appropriate here.

✨ Finishing touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch hm/ts-persist

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/chain_sync/chain_follower.rs (1)
470-472: Bulk persist per-tipset — LGTM.

Tiny nit: avoid repeated blockstore() calls.
-    for tipset in tipsets.iter() {
-        tipset.persist(chain_store.blockstore())?;
-    }
+    let db = chain_store.blockstore();
+    for tipset in &tipsets {
+        tipset.persist(db)?;
+    }
src/blocks/tipset.rs (1)
555-557: Avoid double-reference to db; persist messages before header to reduce transient dangling refs.

Passing &db makes it &&DB; compile‑time OK but noisy. Also, persisting messages first minimizes a window where headers point to not‑yet‑stored data. If validate_msg_root already persists TxMeta, this ordering is even safer.
-            crate::chain::persist_objects(&db, std::iter::once(block.header()))?;
-            crate::chain::persist_objects(&db, block.bls_msgs().iter())?;
-            crate::chain::persist_objects(&db, block.secp_msgs().iter())?;
+            // Persist message objects first, then header.
+            crate::chain::persist_objects(db, block.bls_msgs().iter())?;
+            crate::chain::persist_objects(db, block.secp_msgs().iter())?;
+            crate::chain::persist_objects(db, std::iter::once(block.header()))?;
Can you confirm TipsetValidator::validate_msg_root persists the TxMeta (and AMT nodes) to the blockstore? If not, we should add an explicit persist for TxMeta here or in validate_msg_root.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5b61c18 and 9bc46a7.

📒 Files selected for processing (4)

src/blocks/block.rs (0 hunks)
src/blocks/tipset.rs (1 hunks)
src/chain_sync/chain_follower.rs (1 hunks)
src/rpc/methods/f3.rs (1 hunks)

💤 Files with no reviewable changes (1)

src/blocks/block.rs

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: hanabi1224
PR: ChainSafe/forest#5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.

🧬 Code graph analysis (3)

src/rpc/methods/f3.rs (3)

src/tool/subcommands/api_cmd/test_snapshot.rs (1)

ctx (127-178)

src/tool/subcommands/api_cmd/generate_test_snapshot.rs (1)

ctx (106-159)

src/rpc/methods/sync.rs (1)

ctx (177-252)

src/chain_sync/chain_follower.rs (2)

src/state_manager/mod.rs (1)

chain_store (296-298)

src/rpc/mod.rs (1)

chain_store (466-468)

src/blocks/tipset.rs (1)

src/chain/store/chain_store.rs (2)

persist_objects (516-528)

db (502-502)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

GitHub Check: tests-release
GitHub Check: tests
GitHub Check: Build forest binaries on Linux AMD64
GitHub Check: All lint checks
GitHub Check: cargo-publish-dry-run
GitHub Check: Build MacOS
GitHub Check: Build Ubuntu

🔇 Additional comments (1)

src/rpc/methods/f3.rs (1)

597-597: Switch to FullTipset::persist — confirm no remaining Block::persist usages

Good call. Sandbox ripgrep skipped files ("No files were searched"); run locally: rg -n --hidden -uu 'Block::persist|\bblock\s*.persist\s*(' and confirm there are no hits.

LesnyRumcajs

Does this fix anything in particular?

hanabi1224 · 2025-09-15T22:23:18Z

Does this fix anything in particular?

No

fix: prefer Tipset::persist over Block::persist

9bc46a7

hanabi1224 marked this pull request as ready for review September 15, 2025 14:39

hanabi1224 requested a review from a team as a code owner September 15, 2025 14:39

hanabi1224 requested review from LesnyRumcajs and akaladarshi and removed request for a team September 15, 2025 14:39

coderabbitai bot reviewed Sep 15, 2025

View reviewed changes

LesnyRumcajs approved these changes Sep 15, 2025

View reviewed changes

akaladarshi approved these changes Sep 15, 2025

View reviewed changes

hanabi1224 added this pull request to the merge queue Sep 15, 2025

Merged via the queue into main with commit 36e372e Sep 15, 2025
44 checks passed

hanabi1224 deleted the hm/ts-persist branch September 15, 2025 22:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: prefer `Tipset::persist` over `Block::persist` #6083

fix: prefer `Tipset::persist` over `Block::persist` #6083

Uh oh!

hanabi1224 commented Sep 15, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Sep 15, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

LesnyRumcajs left a comment

Uh oh!

hanabi1224 commented Sep 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix: prefer Tipset::persist over Block::persist #6083

fix: prefer Tipset::persist over Block::persist #6083

Uh oh!

Conversation

hanabi1224 commented Sep 15, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reference issue to close (if applicable)

Other information and links

Change checklist

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

LesnyRumcajs left a comment

Choose a reason for hiding this comment

Uh oh!

hanabi1224 commented Sep 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix: prefer `Tipset::persist` over `Block::persist` #6083

fix: prefer `Tipset::persist` over `Block::persist` #6083

hanabi1224 commented Sep 15, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Sep 15, 2025 •

edited

Loading