-
Notifications
You must be signed in to change notification settings - Fork 189
feat: add --n-epochs to forest-cli state compute
#5946
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f0fdbf6
0500263
8fb563f
518014f
1e0a4da
b49d29b
54c67d6
3c5cc35
cbf06fc
c289198
bf5c127
b99bc19
fee02eb
33e47c0
51235d0
8ac5a0f
c23e81f
9c31a63
b372888
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |||||||||||||||||
| // SPDX-License-Identifier: Apache-2.0, MIT | ||||||||||||||||||
|
|
||||||||||||||||||
| mod types; | ||||||||||||||||||
| use futures::stream::FuturesOrdered; | ||||||||||||||||||
| pub use types::*; | ||||||||||||||||||
|
|
||||||||||||||||||
| use crate::blocks::{Tipset, TipsetKey}; | ||||||||||||||||||
|
|
@@ -68,6 +69,7 @@ use nunny::vec as nonempty; | |||||||||||||||||
| use parking_lot::Mutex; | ||||||||||||||||||
| use schemars::JsonSchema; | ||||||||||||||||||
| use serde::{Deserialize, Serialize}; | ||||||||||||||||||
| use std::num::NonZeroUsize; | ||||||||||||||||||
| use std::ops::Mul; | ||||||||||||||||||
| use std::path::PathBuf; | ||||||||||||||||||
| use std::{sync::Arc, time::Duration}; | ||||||||||||||||||
|
|
@@ -1419,41 +1421,68 @@ impl RpcMethod<2> for StateFetchRoot { | |||||||||||||||||
|
|
||||||||||||||||||
| pub enum ForestStateCompute {} | ||||||||||||||||||
|
|
||||||||||||||||||
| impl RpcMethod<1> for ForestStateCompute { | ||||||||||||||||||
| impl RpcMethod<2> for ForestStateCompute { | ||||||||||||||||||
| const NAME: &'static str = "Forest.StateCompute"; | ||||||||||||||||||
| const PARAM_NAMES: [&'static str; 1] = ["epoch"]; | ||||||||||||||||||
| const N_REQUIRED_PARAMS: usize = 1; | ||||||||||||||||||
| const PARAM_NAMES: [&'static str; 2] = ["epoch", "n_epochs"]; | ||||||||||||||||||
| const API_PATHS: BitFlags<ApiPaths> = ApiPaths::all(); | ||||||||||||||||||
| const PERMISSION: Permission = Permission::Read; | ||||||||||||||||||
|
|
||||||||||||||||||
| type Params = (ChainEpoch,); | ||||||||||||||||||
| type Ok = Cid; | ||||||||||||||||||
| type Params = (ChainEpoch, Option<NonZeroUsize>); | ||||||||||||||||||
| type Ok = Vec<ForestComputeStateOutput>; | ||||||||||||||||||
|
|
||||||||||||||||||
| async fn handle( | ||||||||||||||||||
| ctx: Ctx<impl Blockstore + Send + Sync + 'static>, | ||||||||||||||||||
| (epoch,): Self::Params, | ||||||||||||||||||
| (from_epoch, n_epochs): Self::Params, | ||||||||||||||||||
| ) -> Result<Self::Ok, ServerError> { | ||||||||||||||||||
| let ts = ctx.chain_index().tipset_by_height( | ||||||||||||||||||
| epoch, | ||||||||||||||||||
| let n_epochs = n_epochs.map(|n| n.get()).unwrap_or(1) as ChainEpoch; | ||||||||||||||||||
| let to_epoch = from_epoch + n_epochs - 1; | ||||||||||||||||||
| let to_ts = ctx.chain_index().tipset_by_height( | ||||||||||||||||||
| to_epoch, | ||||||||||||||||||
| ctx.chain_store().heaviest_tipset(), | ||||||||||||||||||
| ResolveNullTipset::TakeOlder, | ||||||||||||||||||
| )?; | ||||||||||||||||||
| // Attempt to load full tipset from the store | ||||||||||||||||||
| if crate::chain_sync::load_full_tipset(ctx.chain_store(), ts.key()).is_err() { | ||||||||||||||||||
| // Load full tipset from the network | ||||||||||||||||||
| let fts = ctx | ||||||||||||||||||
| .sync_network_context | ||||||||||||||||||
| .chain_exchange_messages(None, &ts) | ||||||||||||||||||
| .await | ||||||||||||||||||
| .map_err(|e| anyhow::anyhow!(e))?; | ||||||||||||||||||
| fts.persist(ctx.store())?; | ||||||||||||||||||
| } | ||||||||||||||||||
| let from_ts = ctx.chain_index().tipset_by_height( | ||||||||||||||||||
| from_epoch, | ||||||||||||||||||
| to_ts.clone(), | ||||||||||||||||||
| ResolveNullTipset::TakeOlder, | ||||||||||||||||||
| )?; | ||||||||||||||||||
|
|
||||||||||||||||||
| let StateOutput { state_root, .. } = ctx | ||||||||||||||||||
| .state_manager | ||||||||||||||||||
| .compute_tipset_state(ts, crate::state_manager::NO_CALLBACK, VMTrace::NotTraced) | ||||||||||||||||||
| .await?; | ||||||||||||||||||
| let mut futures = FuturesOrdered::new(); | ||||||||||||||||||
| 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 { | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FuturesOrdered has no push_front; use push (or reverse inputs before pushing)
Apply this minimal fix: - futures.push_front(async move {
+ futures.push(async move {Note: If you relied on
📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query: 💡 Result: The push_front method for the Documentation summary for
Additional context:
Comparison of Future Addition Methods:
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 Citations:
@hanabi1224 You are absolutely correct, and I apologize for my mistake. The original code using 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
🧠 Learnings used |
||||||||||||||||||
| 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 | ||||||||||||||||||
|
Comment on lines
+1459
to
+1463
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainVerify 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 The • File: - 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 pub use chain_follower::load_full_tipset;🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hanabi1224 Thank you for the clarification! You're absolutely right - I can see that 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
🧠 Learnings used |
||||||||||||||||||
| .map_err(|e| anyhow::anyhow!(e))?; | ||||||||||||||||||
| fts.persist(chain_store.blockstore())?; | ||||||||||||||||||
| } | ||||||||||||||||||
| anyhow::Ok(ts) | ||||||||||||||||||
| }); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| Ok(state_root) | ||||||||||||||||||
| let mut results = Vec::with_capacity(n_epochs as _); | ||||||||||||||||||
| while let Some(Ok(ts)) = futures.next().await { | ||||||||||||||||||
| let epoch = ts.epoch(); | ||||||||||||||||||
| let tipset_key = ts.key().clone(); | ||||||||||||||||||
| let StateOutput { state_root, .. } = ctx | ||||||||||||||||||
| .state_manager | ||||||||||||||||||
| .compute_tipset_state(ts, crate::state_manager::NO_CALLBACK, VMTrace::NotTraced) | ||||||||||||||||||
| .await?; | ||||||||||||||||||
| results.push(ForestComputeStateOutput { | ||||||||||||||||||
| state_root, | ||||||||||||||||||
| epoch, | ||||||||||||||||||
| tipset_key, | ||||||||||||||||||
| }); | ||||||||||||||||||
| } | ||||||||||||||||||
| Ok(results) | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also a breaking change to the
Forest.StateComputeAPI, no?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't guarantee it, but it's still worth mentioning in case someone relied on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LesnyRumcajs Changelog updated.