fix(rpc): avoid unecessarily cloning Tipset #6227

hanabi1224 · 2025-11-04T12:49:47Z

Summary of changes

Changes introduced in this pull request:

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

Release Notes

Refactor
- Optimized memory handling in chain RPC methods (ChainHead, ChainGetTipSet, ChainGetTipSetByHeight, ChainGetTipSetAfterHeight, ChainGetFinalizedTipset, ChainGetTipSetV2) to reduce unnecessary data copying.
- Extended internal trait implementations to support reference-counted types.

coderabbitai · 2025-11-04T12:50:10Z

Walkthrough

This PR refactors Tipset handling across the codebase to use Arc<Tipset> instead of direct cloning. Multiple RPC chain methods, CLI commands, and the f3 subsystem are updated to return arc-wrapped tipsets. A new HasLotusJson trait implementation for Arc<T> is added to support serialization.

Changes

Cohort / File(s)	Summary
RPC chain methods `src/rpc/methods/chain.rs`	Six RPC method handlers (ChainGetFinalizedTipset, ChainGetTipSetByHeight, ChainGetTipSetAfterHeight, ChainHead, ChainGetTipSet, ChainGetTipSetV2) switch return type from `Tipset` to `Arc<Tipset>`. Internal helper `get_f3_finality_tipset` also returns `Arc<Tipset>`. Public struct `ApiHeadChange.tipset` field changes to `Arc<Tipset>`. Cloned returns replaced with direct Arc propagation.
CLI subcommands `src/cli/subcommands/chain_cmd.rs`, `src/cli/subcommands/f3_cmd.rs`	`tipset_by_epoch_or_offset` in chain_cmd returns `Arc<Tipset>`. Inner `get_heads` function in f3_cmd returns `(Arc<Tipset>, FinalityCertificate)`. Imports updated to include `Arc`. Call sites adjusted to work with dereferenced arc values.
Lotus JSON serialization `src/lotus_json/arc.rs` (new), `src/lotus_json/mod.rs`	New module `arc.rs` implements `HasLotusJson` trait for `Arc<T>` with `into_lotus_json`, `from_lotus_json`, and test-only `snapshots()` methods. Module declaration added to `mod.rs`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Areas requiring attention:
- Verification that all RPC return type changes properly handle the Arc wrapper in client deserialization paths
- Confirmation that Deref coercion appropriately handles tipset field accesses throughout the chain RPC implementations
- Review of ApiHeadChange serialization with the new Arc<Tipset> field to ensure notification channel consumers receive properly formatted messages
- Validation of the new HasLotusJson implementation for Arc<T> to ensure trait bounds and snapshot wrapping are correct

Possibly related PRs

feat: forest-cli snapshot export-diff #6074: Parallel implementation of the same Tipset → Arc conversion across chain RPCs, archive export, and CLI call sites
feat(api): implement the ChainGetFinalizedTipset API #6133: Related updates to chain RPC handlers including get_f3_finality_tipset and ChainGetFinalizedTipset to use Arc-wrapped tipsets
spec: Add ChainGetFinalizedTipset API #6114: Introduces ChainGetFinalizedTipset RPC modifications that align with this PR's arc-wrapping pattern

Suggested labels

RPC

Suggested reviewers

akaladarshi
sudo-shashank
LesnyRumcajs

Pre-merge checks and finishing touches

✅ 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 clearly and specifically describes the main change: avoiding unnecessary cloning of Tipset objects in RPC methods, which aligns with the core modifications across multiple files (chain.rs, chain_cmd.rs, f3_cmd.rs) that replace Tipset with Arc to eliminate redundant clones.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch hm/rpc-avoid-cloning-tipset

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 39b45d9 and 2f26c77.

📒 Files selected for processing (5)

src/cli/subcommands/chain_cmd.rs (3 hunks)
src/cli/subcommands/f3_cmd.rs (2 hunks)
src/lotus_json/arc.rs (1 hunks)
src/lotus_json/mod.rs (1 hunks)
src/rpc/methods/chain.rs (14 hunks)

🧰 Additional context used

🧠 Learnings (6)

📓 Common learnings

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5944
File: src/chain/store/index.rs:0-0
Timestamp: 2025-08-18T03:09:47.932Z
Learning: In Forest's tipset_by_height caching implementation, hanabi1224 prefers performance-conscious solutions that leverage finality guarantees rather than expensive chain walking for fork detection. The approach of constraining cache lookups to finalized epochs (using CHECKPOINT_INTERVAL >= CHAIN_FINALITY) provides fork safety without the performance cost of ancestry verification.

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 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.

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.

📚 Learning: 2025-09-11T16:03:14.328Z

Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 6000
File: src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs:4-9
Timestamp: 2025-09-11T16:03:14.328Z
Learning: In the Forest codebase's state_decode_params_tests module, common imports like `to_vec`, `TokenAmount`, `Cid`, etc. are centralized in the mod.rs file and made available to submodules through `use super::*;`, eliminating the need for duplicate imports in individual test files.

Applied to files:

src/lotus_json/mod.rs

📚 Learning: 2025-08-25T13:35:24.230Z

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5969
File: src/tool/subcommands/snapshot_cmd.rs:412-412
Timestamp: 2025-08-25T13:35:24.230Z
Learning: In src/tool/subcommands/snapshot_cmd.rs, the +1 in `last_epoch = ts.epoch() - epochs as i64 + 1` fixes an off-by-1 bug where specifying --check-stateroots=N would validate N+1 epochs instead of N epochs, causing out-of-bounds errors when the snapshot contains only N recent state roots.

Applied to files:

src/cli/subcommands/chain_cmd.rs

📚 Learning: 2025-08-08T12:11:55.266Z

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5867
File: src/ipld/util.rs:461-487
Timestamp: 2025-08-08T12:11:55.266Z
Learning: Forest (src/ipld/util.rs, Rust): In UnorderedChainStream::poll_next, dropping `extract_sender` (when no more tipsets and the extract queue is empty) is the intended shutdown signal for workers. Any subsequent attempt to enqueue work after this drop is a logic error and should be treated as an error; do not change `send()` to ignore a missing sender.

Applied to files:

src/cli/subcommands/chain_cmd.rs
src/rpc/methods/chain.rs

📚 Learning: 2025-10-17T14:24:47.046Z

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6167
File: src/tool/subcommands/state_compute_cmd.rs:89-91
Timestamp: 2025-10-17T14:24:47.046Z
Learning: In `src/tool/subcommands/state_compute_cmd.rs`, when using `ReadOpsTrackingStore` to generate minimal snapshots, `HEAD_KEY` should be written to `db.tracker` (not `db` itself) before calling `export_forest_car()`, because the export reads from the tracker MemoryDB which accumulates only the accessed data during computation.

Applied to files:

src/cli/subcommands/f3_cmd.rs

📚 Learning: 2025-08-18T03:09:47.932Z

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5944
File: src/chain/store/index.rs:0-0
Timestamp: 2025-08-18T03:09:47.932Z
Learning: In Forest's tipset_by_height caching implementation, hanabi1224 prefers performance-conscious solutions that leverage finality guarantees rather than expensive chain walking for fork detection. The approach of constraining cache lookups to finalized epochs (using CHECKPOINT_INTERVAL >= CHAIN_FINALITY) provides fork safety without the performance cost of ancestry verification.

Applied to files:

src/rpc/methods/chain.rs

🧬 Code graph analysis (3)

src/lotus_json/arc.rs (2)

src/lotus_json/mod.rs (17)

snapshots (147-147)

snapshots (567-569)

snapshots (581-583)

snapshots (598-600)

snapshots (622-624)

serde_json (302-302)

serde_json (326-326)

into_lotus_json (148-148)

into_lotus_json (570-572)

into_lotus_json (584-586)

into_lotus_json (601-607)

into_lotus_json (625-632)

from_lotus_json (149-149)

from_lotus_json (573-575)

from_lotus_json (587-592)

from_lotus_json (608-614)

from_lotus_json (633-640)

src/rpc/methods/chain.rs (4)

snapshots (1262-1292)

snapshots (1325-1327)

into_lotus_json (1294-1301)

from_lotus_json (1303-1308)

src/cli/subcommands/chain_cmd.rs (1)

src/cli/subcommands/mod.rs (1)

print_pretty_lotus_json (113-116)

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

src/rpc/methods/state.rs (16)

handle (109-114)

handle (132-138)

handle (151-157)

handle (172-178)

handle (196-205)

handle (223-231)

handle (248-255)

handle (272-282)

handle (298-305)

handle (335-465)

handle (484-492)

handle (508-538)

handle (555-561)

handle (577-597)

handle (615-624)

handle (641-663)

⏰ 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: cargo-publish-dry-run
GitHub Check: Build MacOS
GitHub Check: Build Ubuntu
GitHub Check: All lint checks

🔇 Additional comments (9)

src/lotus_json/arc.rs (1)

18-20: Clone on serialization is expected but worth noting.

The into_lotus_json implementation clones the inner value, which means serializing an Arc<Tipset> will still incur a clone. This is unavoidable given the trait's signature (consumes self and returns T::LotusJson), but it means the Arc optimization primarily benefits internal usage rather than serialization paths.

This is acceptable since the goal is to avoid clones during chain operations, not during RPC response serialization.

src/lotus_json/mod.rs (1)

223-223: LGTM!

The internal module declaration is correctly placed and aligns with the broader Arc-based tipset handling changes across the codebase.

src/cli/subcommands/f3_cmd.rs (1)

9-9: LGTM!

The Arc-based return type for get_heads correctly eliminates unnecessary cloning. The function body accesses Tipset methods through Deref, which is clean and efficient.

Also applies to: 152-158

src/cli/subcommands/chain_cmd.rs (1)

10-10: LGTM!

The Arc-based return type eliminates cloning while maintaining clean usage through Deref. The explicit import of print_pretty_lotus_json (line 10) is appropriate.

Also applies to: 19-19, 143-154

src/rpc/methods/chain.rs (5)

143-170: LGTM - Clean Arc-based refactor.

The changes to ChainGetFinalizedTipset and get_f3_finality_tipset correctly return Arc<Tipset> directly instead of cloning. This eliminates unnecessary allocations for finalized tipset queries.

862-884: LGTM - Consistent Arc returns across RPC methods.

All the tipset-returning RPC methods (ChainGetTipSetByHeight, ChainGetTipSetAfterHeight, ChainHead, ChainGetTipSet, ChainGetTipSetV2) now return Arc<Tipset> directly. This is a clean, consistent pattern that avoids cloning tipsets for RPC responses.

Also applies to: 886-913, 931-946, 968-988

1241-1250: LGTM - Arc usage in ApiHeadChange.

Changing ApiHeadChange.tipset to Arc<Tipset> eliminates cloning when broadcasting head changes. The serialization via #[serde(with = "crate::lotus_json")] works correctly with the new HasLotusJson implementation for Arc<T>.

1294-1301: Excellent use of Arc::unwrap_or_clone.

Using Arc::unwrap_or_clone in the PathChange::into_lotus_json conversion is optimal - it will unwrap the Arc without cloning if there's a unique reference, otherwise it clones. This minimizes allocations during path change serialization.

1090-1121: LGTM - Arc-based head change notifications.

The chain_notify implementation now broadcasts Arc<Tipset> in ApiHeadChange messages, both for the initial "current" head (line 1101) and subsequent "apply" changes (line 1115). This avoids cloning tipsets for each subscriber.

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

LesnyRumcajs

rock-solid

fix(rpc): avoid unecessarily cloning Tipset

2f26c77

hanabi1224 marked this pull request as ready for review November 4, 2025 12:51

hanabi1224 requested a review from a team as a code owner November 4, 2025 12:51

hanabi1224 requested review from LesnyRumcajs and elmattic and removed request for a team November 4, 2025 12:51

LesnyRumcajs approved these changes Nov 4, 2025

View reviewed changes

LesnyRumcajs added this pull request to the merge queue Nov 4, 2025

Merged via the queue into main with commit 6ce89b0 Nov 4, 2025
46 checks passed

LesnyRumcajs deleted the hm/rpc-avoid-cloning-tipset branch November 4, 2025 14:02

coderabbitai bot mentioned this pull request Nov 19, 2025

fix: avoid unecessary clone(s) in some Eth RPC methods #6253

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(rpc): avoid unecessarily cloning Tipset #6227

fix(rpc): avoid unecessarily cloning Tipset #6227

Uh oh!

hanabi1224 commented Nov 4, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Nov 4, 2025 •

edited

Loading

Uh oh!

LesnyRumcajs left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix(rpc): avoid unecessarily cloning Tipset #6227

fix(rpc): avoid unecessarily cloning Tipset #6227

Uh oh!

Conversation

hanabi1224 commented Nov 4, 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

Release Notes

Uh oh!

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

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

LesnyRumcajs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hanabi1224 commented Nov 4, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 4, 2025 •

edited

Loading