feat: populate tipset_by_height cache on startup #6100
Conversation
WalkthroughChainStore and ChainIndex fields were made private with new accessor methods (including blockstore() returning &Arc). StateManager no longer holds ChainConfig and its constructors now take only Arc. Call sites across sync, RPC, interpreter, tools, daemon, and tests updated; daemon startup now pre-warms tipset cache. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Daemon as Daemon::start_services
participant CS as ChainStore
participant SM as StateManager
participant Warm as warmup_in_background
participant Metrics as Metrics/StateTree
Daemon->>CS: ChainStore::new(..., chain_config) %% chain_config moved into ChainStore
Daemon->>SM: StateManager::new(Arc<ChainStore>)
par Background warmup
Daemon->>Warm: spawn blocking task
Warm->>CS: chain_index().tipset_by_height(1, heaviest, TakeOlder) %% prefill cache
Warm-->>Daemon: log success/failure + duration
and Metrics init
Daemon->>Metrics: init metrics
Metrics->>CS: blockstore() -> used for StateTree / collectors
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-08-18T03:09:47.932ZApplied to files:
📚 Learning: 2025-09-17T11:32:44.185ZApplied to files:
⏰ 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)
🔇 Additional comments (4)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/state_manager/mod.rs (1)
212-226: Bug: populate_cache stores the wrong state_root for the parent tipset.For a (child, parent) window, the state/receipt for the parent are child.parent_state() and child.min_ticket_block().message_receipts. Using child.min_ticket_block().state_root caches the child’s post-state under the parent key.
Apply this fix:
- self.cache.insert( - parent.key().clone(), - StateOutputValue { - state_root: child.min_ticket_block().state_root, - receipt_root: child.min_ticket_block().message_receipts, - }, - ) + self.cache.insert( + parent.key().clone(), + StateOutputValue { + // Parent’s state/receipts are recorded by the child tipset. + state_root: *child.parent_state(), + receipt_root: child.min_ticket_block().message_receipts, + }, + )
🧹 Nitpick comments (2)
src/chain_sync/tipset_syncer.rs (1)
203-203: Avoid cloning Arc in hot paths — accept a reference insteadget_lookback_tipset_for_round is called from: src/fil_cns/validation.rs, src/chain_sync/tipset_syncer.rs, src/state_manager/mod.rs, src/interpreter/fvm2.rs, src/interpreter/fvm3.rs, src/interpreter/fvm4.rs, src/rpc/methods/miner.rs. Arc::clone() only increments the refcount, but if this is a hot path refactor the function to take &ChainIndex (or &Arc<ChainIndex<...>>) and update callers to eliminate repeated Arc::clone() overhead.
src/daemon/mod.rs (1)
226-229: Metrics closure uses new accessors correctly; consider caching.This constructs a
StateTreeon every scrape. If this becomes hot, cache the actor major version keyed by head tipset key and update it on head changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
src/chain/store/chain_store.rs(4 hunks)src/chain/store/index.rs(3 hunks)src/chain_sync/chain_follower.rs(5 hunks)src/chain_sync/tipset_syncer.rs(2 hunks)src/chain_sync/validation.rs(1 hunks)src/daemon/context.rs(2 hunks)src/daemon/mod.rs(6 hunks)src/fil_cns/validation.rs(3 hunks)src/interpreter/fvm2.rs(1 hunks)src/interpreter/fvm3.rs(1 hunks)src/interpreter/fvm4.rs(1 hunks)src/interpreter/vm.rs(3 hunks)src/libp2p/chain_exchange/provider.rs(2 hunks)src/message_pool/msgpool/provider.rs(1 hunks)src/rpc/methods/eth.rs(4 hunks)src/rpc/methods/eth/filter/mod.rs(1 hunks)src/rpc/methods/sync.rs(2 hunks)src/rpc/mod.rs(1 hunks)src/state_manager/mod.rs(27 hunks)src/state_manager/tests.rs(6 hunks)src/state_manager/utils.rs(2 hunks)src/tool/offline_server/server.rs(1 hunks)src/tool/subcommands/api_cmd/api_compare_tests.rs(1 hunks)src/tool/subcommands/api_cmd/generate_test_snapshot.rs(1 hunks)src/tool/subcommands/api_cmd/test_snapshot.rs(1 hunks)src/tool/subcommands/index_cmd.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: hanabi1224
PR: ChainSafe/forest#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
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-08T12:11:55.266Z
Learnt from: hanabi1224
PR: ChainSafe/forest#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/message_pool/msgpool/provider.rs
🧬 Code graph analysis (18)
src/interpreter/vm.rs (3)
src/chain/store/chain_store.rs (1)
chain_index(247-249)src/rpc/mod.rs (1)
chain_index(472-474)src/state_manager/mod.rs (1)
chain_index(290-292)
src/rpc/methods/eth/filter/mod.rs (3)
src/rpc/methods/sync.rs (1)
ctx(177-252)src/tool/subcommands/api_cmd/generate_test_snapshot.rs (1)
ctx(106-159)src/tool/subcommands/api_cmd/test_snapshot.rs (1)
ctx(127-178)
src/state_manager/tests.rs (1)
src/state_manager/mod.rs (2)
new(180-182)chain_store(285-287)
src/interpreter/fvm2.rs (5)
src/shim/state_tree.rs (3)
new_from_root(168-188)new(156-166)new(422-436)src/chain/store/chain_store.rs (2)
new(116-142)new(572-576)src/interpreter/fvm3.rs (1)
new(48-65)src/interpreter/fvm4.rs (1)
new(48-65)src/interpreter/vm.rs (1)
new(175-271)
src/interpreter/fvm3.rs (2)
src/chain/store/chain_store.rs (2)
new(116-142)new(572-576)src/interpreter/vm.rs (1)
new(175-271)
src/tool/subcommands/api_cmd/generate_test_snapshot.rs (2)
src/chain/store/chain_store.rs (3)
chain_config(252-254)new(116-142)new(572-576)src/state_manager/mod.rs (3)
chain_config(295-297)new(180-182)chain_store(285-287)
src/interpreter/fvm4.rs (2)
src/chain/store/chain_store.rs (2)
new(116-142)new(572-576)src/chain/store/index.rs (1)
new(42-46)
src/tool/offline_server/server.rs (4)
src/chain/store/chain_store.rs (3)
chain_config(252-254)new(116-142)new(572-576)src/daemon/context.rs (2)
chain_config(65-67)chain_store(69-71)src/rpc/mod.rs (2)
chain_config(476-478)chain_store(468-470)src/state_manager/mod.rs (3)
chain_config(295-297)new(180-182)chain_store(285-287)
src/fil_cns/validation.rs (3)
src/daemon/context.rs (1)
chain_store(69-71)src/rpc/mod.rs (1)
chain_store(468-470)src/state_manager/mod.rs (1)
chain_store(285-287)
src/daemon/context.rs (2)
src/rpc/mod.rs (1)
chain_store(468-470)src/state_manager/mod.rs (2)
chain_store(285-287)new(180-182)
src/tool/subcommands/api_cmd/test_snapshot.rs (2)
src/chain/store/chain_store.rs (3)
chain_config(252-254)new(116-142)new(572-576)src/state_manager/mod.rs (3)
chain_config(295-297)new(180-182)chain_store(285-287)
src/tool/subcommands/index_cmd.rs (2)
src/chain/store/chain_store.rs (3)
chain_config(252-254)new(116-142)new(572-576)src/state_manager/mod.rs (3)
chain_config(295-297)new(180-182)chain_store(285-287)
src/rpc/methods/sync.rs (3)
src/rpc/mod.rs (2)
store(480-482)chain_config(476-478)src/chain/store/chain_store.rs (4)
block_messages(469-482)chain_config(252-254)new(116-142)new(572-576)src/state_manager/mod.rs (2)
chain_config(295-297)new(180-182)
src/tool/subcommands/api_cmd/api_compare_tests.rs (2)
src/chain/store/chain_store.rs (1)
chain_config(252-254)src/state_manager/mod.rs (3)
chain_config(295-297)new(180-182)chain_store(285-287)
src/chain_sync/chain_follower.rs (3)
src/daemon/context.rs (1)
chain_store(69-71)src/rpc/mod.rs (1)
chain_store(468-470)src/state_manager/mod.rs (1)
chain_store(285-287)
src/chain/store/chain_store.rs (4)
src/state_manager/mod.rs (3)
blockstore(276-278)chain_index(290-292)chain_config(295-297)src/rpc/mod.rs (2)
chain_index(472-474)chain_config(476-478)src/daemon/context.rs (1)
chain_config(65-67)src/tool/subcommands/archive_cmd.rs (1)
genesis_timestamp(1145-1149)
src/state_manager/mod.rs (5)
src/chain/store/chain_store.rs (6)
new(116-142)new(572-576)blockstore(242-244)chain_index(247-249)chain_config(252-254)block_messages(469-482)src/interpreter/vm.rs (1)
new(175-271)src/networks/mod.rs (4)
epoch(468-481)chain_config(737-742)network_version(427-432)network_version(745-755)src/state_manager/chain_rand.rs (5)
clone(27-34)state(191-191)state(194-194)state(209-209)state(211-211)src/state_migration/mod.rs (1)
run_state_migrations(93-162)
src/daemon/mod.rs (4)
src/chain/store/index.rs (1)
chain(191-198)src/shim/state_tree.rs (1)
new_from_root(168-188)src/daemon/context.rs (1)
chain_store(69-71)src/state_manager/mod.rs (1)
chain_store(285-287)
⏰ 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
- GitHub Check: tests-release
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: All lint checks
🔇 Additional comments (48)
src/chain/store/index.rs (3)
29-29: Proper encapsulation of DB fieldThe change from
pub db: DBto privatedb: DBcorrectly implements encapsulation, preventing direct field access and ensuring all DB access goes through the controlled accessor method.
48-50: Consistent accessor patternThe new
db()accessor method provides controlled access to the underlying database, maintaining consistency with the broader refactor pattern seen across the codebase.
132-133: Cache size increase with mainnet capacity rationaleThe cache size increase from 4096 to 20480 entries is well-justified by the comment explaining mainnet capacity requirements (20480 * 900 = 18432000 epochs). This aligns with the PR's objective to populate the
tipset_by_heightcache on startup for better performance.src/rpc/mod.rs (1)
473-473: Correct delegation to new accessor patternThe change from direct field access (
self.chain_store().chain_index) to method call (self.chain_store().chain_index()) properly utilizes the new accessor pattern while maintaining the same return type and functionality.src/chain_sync/validation.rs (1)
76-76: Proper use of blockstore accessorThe change from
&chainstore.dbtochainstore.blockstore()correctly adapts to the new accessor-based API. Theblockstore()method now returns&Arc<DB>as indicated in the summary, which provides the same functionality but through a controlled interface.src/rpc/methods/eth/filter/mod.rs (1)
431-431: Consistent chain index accessor usageThe change from
ctx.chain_store().chain_index.tipset_by_height(...)toctx.chain_index().tipset_by_height(...)follows the established pattern in the broader refactor, using the accessor method instead of direct field access.src/message_pool/msgpool/provider.rs (1)
118-118: Proper accessor method usageThe change from direct field access to
chain_index()method call maintains the same functionality while adhering to the new encapsulation pattern established throughout the codebase.src/state_manager/utils.rs (1)
48-48: Consistent config access through new accessor patternBoth changes from
self.chain_config.policytoself.chain_config().policyproperly utilize the new accessor method. The functionality remains identical while following the established pattern of accessing configuration through methods rather than direct field access.Also applies to: 60-60
src/tool/offline_server/server.rs (2)
97-97: Move semantics for chain_config in ChainStore::newThe change from
chain_config.clone()tochain_configcorrectly transfers ownership of theArc<ChainConfig>to theChainStore::newmethod, which now consumes the value directly rather than requiring a clone.
100-100: Updated StateManager constructor signatureThe change from
StateManager::new(chain_store.clone(), chain_config)toStateManager::new(chain_store.clone())correctly reflects the updated constructor signature whereStateManagerno longer takes a separatechain_configparameter but retrieves it from theChainStore.src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
2166-2166: Updated initialization pattern for ChainStore and StateManagerBoth changes correctly follow the new initialization pattern established across the codebase:
- Line 2166:
chain_configis moved intoChainStore::newby value instead of being cloned- Line 2169:
StateManager::newnow takes only thechain_storeparameter, obtaining the config internallyThis maintains the same functionality while adhering to the refactored API design.
Also applies to: 2169-2169
src/interpreter/fvm4.rs (3)
90-90: LGTM!The change to use
self.chain_index.db()accessor is correct and consistent with the refactoring to privatize ChainIndex fields.
96-96: LGTM!The change to use
self.chain_index.db()for TrackingBlockstore is consistent with the accessor pattern.
103-103: LGTM!The change to use
Arc::clone(self.chain_index.db())follows the same accessor pattern consistently.src/interpreter/vm.rs (3)
207-207: LGTM!The change from direct field access to
chain_index.db()accessor method is correct and aligns with the refactoring to make ChainIndex fields private.
235-235: LGTM!Consistent use of the
chain_index.db()accessor for V3 FVM initialization.
258-258: LGTM!Consistent use of the
chain_index.db()accessor for V2 FVM initialization.src/tool/subcommands/api_cmd/generate_test_snapshot.rs (1)
119-131: LGTM!The refactored initialization correctly moves the
chain_configintoChainStore::newand updatesStateManager::newto take only the chain store, aligning with the broader API changes to encapsulate configuration within ChainStore.src/interpreter/fvm3.rs (3)
91-91: LGTM!The change to use
Arc::clone(self.chain_index.db())is correct and consistent with the accessor pattern.
97-97: LGTM!The change to use
self.chain_index.db()for TrackingBlockstore is consistent with the accessor pattern.
103-103: LGTM!The change to use
Arc::clone(self.chain_index.db())follows the same accessor pattern consistently.src/chain_sync/tipset_syncer.rs (2)
192-193: LGTM!The change from direct field access to the
chain_index()accessor method is correct and consistent with the ChainStore refactoring.
203-204: LGTM!The changes to use accessor methods
chain_index()andchain_config()properly align with the privatized fields in ChainStore.src/interpreter/fvm2.rs (3)
86-87: LGTM: Accessor method transition for database access.The change from direct field access (
&self.chain_index.db) to method call (self.chain_index.db()) aligns with the broader refactor to privatize ChainIndex fields and expose them through accessor methods.
93-93: LGTM: Consistent accessor method usage.Properly updated to use the new
db()accessor method instead of direct field access.
99-99: LGTM: Final accessor method update completes the transition.All database access points in this method have been consistently updated to use the new accessor pattern.
src/daemon/context.rs (2)
69-71: LGTM: Clean delegation to StateManager's chain store accessor.This new accessor method provides a clean way to access the chain store through the AppContext, maintaining proper encapsulation by delegating to the StateManager's
chain_store()method.
254-254: LGTM: StateManager constructor signature simplified.The StateManager constructor now takes only the ChainStore parameter, with ChainConfig being accessible through
chain_store.chain_config(). This simplifies the constructor while maintaining functionality.src/libp2p/chain_exchange/provider.rs (1)
34-34: LGTM: Consistent accessor method usage throughout.Both instances of
chain_store.chain_indexhave been properly updated tochain_store.chain_index()to use the new accessor method pattern. This maintains consistency with the broader refactoring effort.Also applies to: 48-48
src/tool/subcommands/api_cmd/test_snapshot.rs (1)
145-145: LGTM: Constructor signature updates for consistency.Both changes align with the broader refactor:
- ChainStore::new now takes
chain_configdirectly as Arc- StateManager::new simplified to take only the chain_store parameter
These changes maintain the same functionality while using the new constructor signatures.
Also applies to: 150-150
src/chain_sync/chain_follower.rs (3)
482-484: LGTM: Accessor method transition in tipset loading.The change from
chain_store.chain_indextochain_store.chain_index()properly uses the new accessor method while maintaining identical functionality for tipset retrieval.
624-624: LGTM: Configuration access through chain store.Both instances now use
self.cs.chain_config()to access the configuration through the chain store, maintaining consistency with the broader refactor pattern.Also applies to: 636-636
961-961: LGTM: Test updates for blockstore accessor.Both test methods now use
cs.blockstore().clone()instead of direct field access, ensuring tests remain compatible with the new accessor pattern.Also applies to: 1025-1025
src/rpc/methods/sync.rs (2)
124-124: LGTM: Streamlined blockstore access.The change from
&ctx.chain_store().dbtoctx.store()simplifies the blockstore access pattern. Thectx.store()method delegates tochain_store().blockstore(), providing cleaner, more direct access to the underlying blockstore.
196-196: LGTM: Constructor signature updates in test setup.Both changes align with the refactored APIs:
- ChainStore::new now takes
chain_configdirectly (line 196)- StateManager::new simplified to single parameter (line 202)
The test setup maintains identical functionality with the new constructor signatures.
Also applies to: 202-202
src/state_manager/tests.rs (1)
72-72: LGTM: Consistent StateManager constructor updates across all tests.All six test instances have been properly updated to use the simplified StateManager::new constructor that takes only the chain_store parameter. The ChainConfig is now accessed through the chain store's accessor methods, maintaining the same functionality while following the new API design.
This demonstrates thorough test coverage updates that align with the broader refactoring effort.
Also applies to: 134-134, 167-167, 226-226, 269-269, 312-312
src/fil_cns/validation.rs (1)
60-62: Accessor refactor looks correct.Switched to ChainStore::chain_index() and StateManager/ChainStore accessors without altering behavior. No issues spotted.
Also applies to: 75-79, 85-89
src/tool/subcommands/index_cmd.rs (1)
74-76: Constructor/API updates and accessor usage LGTM.
- Moving chain_config into ChainStore::new and using StateManager::new(chain_store) aligns with the new API.
- chain_index().tipset_by_height() call is correct for resolving "from".
Also applies to: 78-79, 88-93
src/rpc/methods/eth.rs (1)
880-881: Mechanical switch to accessors is sound.All tipset resolution and chain_config reads now go through ChainStore accessors; semantics unchanged.
Also applies to: 898-907, 923-926, 940-942, 1637-1639
src/chain/store/chain_store.rs (2)
241-254: New ChainStore accessors are consistent and minimal.Returning &Arc for blockstore and exposing chain_index()/chain_config() is coherent with the rest of the refactor.
342-343: Using ChainIndex::db() for genesis timestamp is appropriate.This avoids reaching into private fields and preserves behavior.
src/state_manager/mod.rs (3)
180-182: StateManager constructors updated correctly.Pulling beacon/config via ChainStore accessors matches the new ownership model.
Also applies to: 189-197
275-283: Accessor and Arc ergonomics LGTM.blockstore() returning &Arc, blockstore_owned(), and chain_index()/chain_config() accessors are used correctly.
Also applies to: 289-297, 299-306
1675-1679: Good: next-tipset lookup uses correct mapping.try_lookup_state_from_next_tipset correctly derives (state_root, receipt_root) from the next tipset’s parent_state and message_receipts.
src/daemon/mod.rs (4)
11-11: Import looks correct.Needed for the new warmup logic using
ResolveNullTipset.
42-42: Import looks correct.
Instantis used in the warmup timer.
520-520: Good: switched tochain_store.blockstore()for the collector.Aligns with the privatized fields and avoids direct DB coupling.
613-613: Orphaned background task: warmup is not tracked by JoinSet.
warmup_in_backgroundspawns a blocking task that isn't added toservices. Ifstart_servicesis restarted (e.g., GC-triggered reboot loop), prior warmups keep running, leading to duplicated work and leaked tasks holdingChainStorealive.Apply this diff to attach the task to
services:- warmup_in_background(&ctx); + warmup_in_background(&ctx, &mut services);Likely an incorrect or invalid review comment.
| fn warmup_in_background(ctx: &AppContext) { | ||
| // Populate `tipset_by_height` cache | ||
| let cs = ctx.chain_store().clone(); | ||
| tokio::task::spawn_blocking(move || { | ||
| let start = Instant::now(); | ||
| match cs.chain_index().tipset_by_height( | ||
| // 0 would short-curcuit the cache | ||
| 1, | ||
| cs.heaviest_tipset(), | ||
| ResolveNullTipset::TakeOlder, | ||
| ) { | ||
| Ok(_) => { | ||
| tracing::warn!( | ||
| "Successfully populated tipset_by_height cache, took {}", | ||
| humantime::format_duration(start.elapsed()) | ||
| ); | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!("Failed to populate tipset_by_height cache: {e}"); | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Manage warmup task lifecycle; use info-level on success; fix typo.
- Track the spawned task via
JoinSetto avoid leaks across restarts. - Log success at
info!(notwarn!). - Fix “short-curcuit” → “short-circuit”.
- Optional: bound the warmup range to a finality-sized window to reduce IO and align with your prior preference for finality-bounded caching. This keeps fork safety while avoiding a full walk from head to height 1.
Apply this diff:
-fn warmup_in_background(ctx: &AppContext) {
- // Populate `tipset_by_height` cache
- let cs = ctx.chain_store().clone();
- tokio::task::spawn_blocking(move || {
- let start = Instant::now();
- match cs.chain_index().tipset_by_height(
- // 0 would short-curcuit the cache
- 1,
- cs.heaviest_tipset(),
- ResolveNullTipset::TakeOlder,
- ) {
- Ok(_) => {
- tracing::warn!(
- "Successfully populated tipset_by_height cache, took {}",
- humantime::format_duration(start.elapsed())
- );
- }
- Err(e) => {
- tracing::warn!("Failed to populate tipset_by_height cache: {e}");
- }
- }
- });
-}
+fn warmup_in_background(ctx: &AppContext, services: &mut JoinSet<anyhow::Result<()>>) {
+ // Populate `tipset_by_height` cache.
+ let cs = ctx.chain_store().clone();
+ services.spawn(async move {
+ // Run off the async executor; don't block the runtime.
+ let joinres = tokio::task::spawn_blocking(move || {
+ let start = Instant::now();
+ // Height 0 short-circuits and doesn't populate the cache.
+ match cs.chain_index().tipset_by_height(
+ 1,
+ cs.heaviest_tipset(),
+ ResolveNullTipset::TakeOlder,
+ ) {
+ Ok(_) => {
+ tracing::info!(
+ "Populated tipset_by_height cache in {}",
+ humantime::format_duration(start.elapsed())
+ );
+ }
+ Err(e) => {
+ tracing::warn!("Failed to populate tipset_by_height cache: {e}");
+ }
+ }
+ }).await;
+ if let Err(e) = joinres {
+ tracing::warn!("Warmup task panicked: {e}");
+ }
+ Ok(())
+ });
+}Optional (no diff): bound the warmup to a finality window (e.g., call tipset_by_height(head_epoch - FINALITY, head, …)), or gate via a config flag for resource‑constrained environments.
📝 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.
| fn warmup_in_background(ctx: &AppContext) { | |
| // Populate `tipset_by_height` cache | |
| let cs = ctx.chain_store().clone(); | |
| tokio::task::spawn_blocking(move || { | |
| let start = Instant::now(); | |
| match cs.chain_index().tipset_by_height( | |
| // 0 would short-curcuit the cache | |
| 1, | |
| cs.heaviest_tipset(), | |
| ResolveNullTipset::TakeOlder, | |
| ) { | |
| Ok(_) => { | |
| tracing::warn!( | |
| "Successfully populated tipset_by_height cache, took {}", | |
| humantime::format_duration(start.elapsed()) | |
| ); | |
| } | |
| Err(e) => { | |
| tracing::warn!("Failed to populate tipset_by_height cache: {e}"); | |
| } | |
| } | |
| }); | |
| } | |
| fn warmup_in_background(ctx: &AppContext, services: &mut JoinSet<anyhow::Result<()>>) { | |
| // Populate `tipset_by_height` cache. | |
| let cs = ctx.chain_store().clone(); | |
| services.spawn(async move { | |
| // Run off the async executor; don't block the runtime. | |
| let joinres = tokio::task::spawn_blocking(move || { | |
| let start = Instant::now(); | |
| // Height 0 short-circuits and doesn't populate the cache. | |
| match cs.chain_index().tipset_by_height( | |
| 1, | |
| cs.heaviest_tipset(), | |
| ResolveNullTipset::TakeOlder, | |
| ) { | |
| Ok(_) => { | |
| tracing::info!( | |
| "Populated tipset_by_height cache in {}", | |
| humantime::format_duration(start.elapsed()) | |
| ); | |
| } | |
| Err(e) => { | |
| tracing::warn!("Failed to populate tipset_by_height cache: {e}"); | |
| } | |
| } | |
| }).await; | |
| if let Err(e) = joinres { | |
| tracing::warn!("Warmup task panicked: {e}"); | |
| } | |
| Ok(()) | |
| }); | |
| } |
🤖 Prompt for AI Agents
In src/daemon/mod.rs around lines 631-653, the warmup_in_background routine
currently spawns a blocking task without tracking it, logs a successful warmup
at warn! and contains a typo; change it to spawn and record the task in a
JoinSet (or otherwise store the JoinHandle in the daemon/AppContext so it can be
awaited/aborted on restart) instead of fire-and-forget, change the successful
branch to tracing::info! (not tracing::warn!), fix the comment typo
“short-curcuit” → “short-circuit”, and—optionally—limit the tipset_by_height
walk to a finality-sized window (e.g., head_epoch - FINALITY) or gate the warmup
behind a config flag to reduce IO.
There was a problem hiding this comment.
@hanabi1224, thanks for fixing the log level! 🐰
I see you've addressed the tracing::warn! → tracing::info! change for the success case. The other items from my review (task lifecycle management via JoinSet and the "short-circuit" typo) can be addressed separately if needed.
The warmup functionality looks good overall - it's a nice performance optimization for startup! ✨
(´｡• ᵕ •｡`) ♡
🧠 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.
Learnt from: hanabi1224
PR: ChainSafe/forest#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.
There was a problem hiding this comment.
@hanabi1224, perfect! Thanks for fixing the typo as well. 🎯
With both the log level and typo addressed, the warmup function looks much cleaner now. The remaining task lifecycle management (tracking via JoinSet) is more of an architectural consideration and can be addressed separately if you decide it's worthwhile for your use case.
The cache warmup is a great addition to improve startup performance! 🚀
🧠 Learnings used
Learnt from: hanabi1224
PR: ChainSafe/forest#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
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 (2)
src/daemon/mod.rs (2)
612-612: Track the warmup task viaJoinSetto avoid fire‑and‑forget.
Prevents leaks across service restarts and surfaces panics. Apply with the function change below.- warmup_in_background(&ctx); + warmup_in_background(&ctx, &mut services);
631-653: Manage warmup task lifecycle; handle panics; fix typo.
- Use
JoinSetand join the blocking task to catch panics.- Typo: “short-curcuit” → “short-circuit”.
-fn warmup_in_background(ctx: &AppContext) { - // Populate `tipset_by_height` cache - let cs = ctx.chain_store().clone(); - tokio::task::spawn_blocking(move || { - let start = Instant::now(); - match cs.chain_index().tipset_by_height( - // 0 would short-curcuit the cache - 1, - cs.heaviest_tipset(), - ResolveNullTipset::TakeOlder, - ) { - Ok(_) => { - tracing::info!( - "Successfully populated tipset_by_height cache, took {}", - humantime::format_duration(start.elapsed()) - ); - } - Err(e) => { - tracing::warn!("Failed to populate tipset_by_height cache: {e}"); - } - } - }); -} +fn warmup_in_background(ctx: &AppContext, services: &mut JoinSet<anyhow::Result<()>>) { + // Populate `tipset_by_height` cache. + let cs = ctx.chain_store().clone(); + services.spawn(async move { + // Run off the async executor; don't block the runtime. + let joinres = tokio::task::spawn_blocking(move || { + let start = Instant::now(); + match cs.chain_index().tipset_by_height( + // 0 would short-circuit the cache + 1, + cs.heaviest_tipset(), + ResolveNullTipset::TakeOlder, + ) { + Ok(_) => { + tracing::info!( + "Populated tipset_by_height cache in {}", + humantime::format_duration(start.elapsed()) + ); + } + Err(e) => { + tracing::warn!("Failed to populate tipset_by_height cache: {e}"); + } + } + }).await; + if let Err(e) = joinres { + tracing::warn!("Warmup task panicked: {e}"); + } + Ok(()) + }); +}
🧹 Nitpick comments (2)
src/daemon/mod.rs (2)
226-229: Switch to blockstore handle is correct; consider avoiding per-scrapeStateTreerebuild.
The API change is wired correctly. Minor perf: constructingStateTreeon every metrics scrape can be heavy; cache the actor version (e.g.,OnceCellupdated on head-change) and read it here.
631-653: Bound warmup to the chain-finality window or make it configurable.
Starting at height 1 is IO-heavy — limit warmup to [head_epoch − chain_finality, head_epoch] or add a config toggle. Change src/daemon/mod.rs::warmup_in_background to compute start = max(1, cs.heaviest_tipset().epoch() - chain_config.policy.chain_finality) and pass that to tipset_by_height instead of 1. ChainConfig::policy.chain_finality is available (see src/networks/mod.rs and usages in src/f3/mod.rs and src/chain/store/chain_store.rs).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/daemon/mod.rs(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: hanabi1224
PR: ChainSafe/forest#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
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.
⏰ 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
- GitHub Check: tests-release
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: All lint checks
🔇 Additional comments (3)
src/daemon/mod.rs (3)
11-11: LGTM: correct import for null-tipset resolution.
Needed forResolveNullTipset::TakeOlderin warmup.
42-42: LGTM:Instantimport is appropriate for timing the warmup.
520-520: LGTM: pass blockstore toEthMappingCollector::new.
Matches the refactor away from direct DB handle access.
019b673 to
42dfb6f
Compare
LesnyRumcajs
left a comment
There was a problem hiding this comment.
Does this increase startup time?
@LesnyRumcajs No, the warmup task runs in background |
Summary of changes
Changes introduced in this pull request:
tipset_by_heightcache on startupChainStoreandChainIndexto make all fields privateReference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit
Performance
Reliability
Tools
Refactor