fix: avoid some unnecessary clones in chain follower

[hanabi1224]

Summary of changes

Changes introduced in this pull request:

Reference issue to close (if applicable)

Closes

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.

Summary by CodeRabbit

• Refactor
  • Optimized memory usage in chain synchronization operations by improving how internal data references are passed throughout the system, reducing unnecessary copying of data structures.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This PR refactors function signatures in chain_follower.rs to pass network context and chain store as borrowed references rather than owned or cloned values. Key public functions (get_full_tipset, get_full_tipset_batch, update_peer_info, handle_peer_disconnected_event) are updated to accept references, with all call sites and internal paths adjusted accordingly.

Changes

Cohort / File(s)	Summary
Reference borrowing refactor src/chain_sync/chain_follower.rs	Updated public function signatures to accept borrowed &SyncNetworkContext<DB> instead of owned SyncNetworkContext<DB>, and &ChainStore<DB> instead of Arc<ChainStore<DB>>. Affected: get_full_tipset, get_full_tipset_batch, update_peer_info, handle_peer_disconnected_event. Call sites adjusted to pass borrowed references; internal task execution paths updated to propagate borrowed context.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• Call site verification: Ensure all invocations of updated functions pass borrowed references correctly and lifetime constraints are satisfied
• Task spawning logic: Verify borrowed references are properly handled when cloning is necessary for spawned tasks

Suggested reviewers

• sudo-shashank
• LesnyRumcajs
• akaladarshi

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% 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 accurately summarizes the main change: refactoring function signatures to use borrowed references instead of owned/cloned values, eliminating unnecessary clones in the chain follower module.

✨ 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/reduce-clones

---

📜 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 326099f and 9e206c4.

📒 Files selected for processing (1)

• src/chain_sync/chain_follower.rs (9 hunks)

🧰 Additional context used

🧠 Learnings (2)

📓 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: 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.

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-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/chain_sync/chain_follower.rs

🧬 Code graph analysis (1)

src/chain_sync/chain_follower.rs (3)

src/rpc/mod.rs (1)

• chain_store (474-476)

src/state_manager/mod.rs (1)

• chain_store (348-350)

src/daemon/context.rs (1)

• chain_store (69-71)

⏰ 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: Build forest binaries on Linux AMD64
• GitHub Check: tests-release
• GitHub Check: tests
• GitHub Check: Build Ubuntu
• GitHub Check: cargo-publish-dry-run
• GitHub Check: Build MacOS
• GitHub Check: All lint checks

🔇 Additional comments (8)

src/chain_sync/chain_follower.rs (8)

163-166: LGTM - Reduced unnecessary network clone

The change to pass &network instead of cloning the network context is correct and improves performance.

---

171-176: LGTM - Efficient borrowing pattern

The changes to pass borrowed references (&network and state_manager.chain_store()) instead of clones are correct. Deref coercion handles the conversion from &Arc<ChainStore<DB>> to &ChainStore<DB> seamlessly.

---

183-183: LGTM - Consistent borrowing pattern

Correctly passes borrowed references, consistent with the other call sites.

---

356-378: LGTM - Well-balanced refactoring

The signature change to accept &SyncNetworkContext<DB> is correct. Note that line 367 still clones the network when spawning a task, which is necessary since the spawned task needs to own the network context. The refactoring successfully eliminates unnecessary clones while preserving correctness.

---

418-424: LGTM - Signature change is correct

The change to accept a borrowed reference is appropriate since this function doesn't spawn tasks and operates synchronously on the network context.

---

446-467: LGTM - Internal function refactored correctly

The signature changes to accept borrowed references are correct and improve performance. All internal call sites have been appropriately updated.

---

864-872: LGTM - Correct usage of updated function

The call correctly passes borrowed references, with &network borrowing the owned network context and cs relying on Deref coercion. This is consistent with the refactoring approach.

---

426-444: Verify external call sites for this public function

The signature changes are correct and improve efficiency. However, since this is a public function, ensure that all external call sites (outside this file) have been updated to pass borrowed references instead of owned values.

#!/bin/bash
# Description: Find all call sites of get_full_tipset to verify they've been updated

# Search for calls to get_full_tipset
rg -n -C3 'get_full_tipset\s*\(' --type=rust -g '!src/chain_sync/chain_follower.rs'

---

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

[LesnyRumcajs]

I like this fight against the clones!