feat(chainstore): add fallback blockstore skeleton with bitswap support #6395
Conversation
…al bitswap fallback (ChainSafe#6353) Introduces FallbackBlockstore wrapper implementing get_with_fallback to fetch missing blocks via Bitswap when FOREST_ENABLE_CHAINSTORE_FALLBACK is set. Adds environment variable check and uses asynchronous Bitswap fetch. Closes ChainSafe#6353.
Adds mod blockstore_fallback and re-exports it in store::mod. This exposes the fallback blockstore functionality for optional bitswap fallback. Part of issue ChainSafe#6353.
Add documentation for the FOREST_ENABLE_CHAINSTORE_FALLBACK environment variable and describe how to enable the fallback blockstore via Bitswap. Part of issue ChainSafe#6353.
0xDevNinja
requested review from
LesnyRumcajs and
sudo-shashank
and removed request for
a team
|
|
WalkthroughThe PR introduces an optional chainstore fallback mechanism that enables fetching missing blocks via Bitswap from the peer-to-peer network. Implementation includes trait definitions for block storage and network retrieval, a fallback blockstore wrapper coordinating local and remote access, and documentation describing the feature controlled via an environment variable. Changes
Sequence DiagramsequenceDiagram
participant Client
participant FallbackBlockstore
participant LocalStore as BlockGetter<br/>(Local Store)
participant Bitswap as BitswapFetcher<br/>(Network)
Client->>FallbackBlockstore: get_with_fallback(cid)
FallbackBlockstore->>LocalStore: get(cid)
LocalStore-->>FallbackBlockstore: Some(block) or None
alt Block found locally
FallbackBlockstore-->>Client: Some(block)
else Block not found
FallbackBlockstore->>FallbackBlockstore: Check FOREST_ENABLE_CHAINSTORE_FALLBACK env var
alt Fallback enabled
FallbackBlockstore->>Bitswap: fetch_block(cid)
Bitswap-->>FallbackBlockstore: Ok(()) or Err
alt Fetch succeeded
FallbackBlockstore->>LocalStore: get(cid) [retry]
LocalStore-->>FallbackBlockstore: Some(block)
FallbackBlockstore-->>Client: Some(block)
else Fetch failed
FallbackBlockstore-->>Client: Err
end
else Fallback disabled
FallbackBlockstore-->>Client: None
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @README.md:
- Line 36: The sentence "To enable fallback, set the environment variable:"
contains a double space after "To"; fix it by replacing "To enable fallback,
set the environment variable:" with "To enable fallback, set the environment
variable:" in the README (the line containing the string "To enable fallback,
set the environment variable:").
In @src/chain/store/blockstore_fallback.rs:
- Around line 61-66: The code reads FOREST_ENABLE_CHAINSTORE_FALLBACK via
env::var inside get_with_fallback on the hot path; move that env var read out of
the hot path by caching the boolean at construction or in a static LazyLock so
get_with_fallback checks a stored flag instead of calling env::var each time.
Concretely, add a field like enable_fallback to the BlockstoreFallback (or
initialize a static std::sync::LazyLock<bool> with
env::var("FOREST_ENABLE_CHAINSTORE_FALLBACK") == "1"), set it once at creation,
and replace the inline env::var check in get_with_fallback with a read of that
cached boolean.
🧹 Nitpick comments (1)
README.md (1)
32-43: Section placement breaks "Run with Docker" flow.The "Optional Chainstore Fallback" section is inserted between the "Run with Docker" header (line 30) and the actual Docker instructions (line 45), disrupting the logical structure. Consider either making this a subsection under "Run with Docker" or moving it to a more appropriate location (e.g., after the Docker section or under a dedicated "Configuration" or "Advanced Features" section).
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.mdsrc/chain/store/blockstore_fallback.rssrc/chain/store/mod.rs
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5867
File: src/ipld/util.rs:553-558
Timestamp: 2025-08-08T12:10:45.218Z
Learning: Forest project targets Rust stable >=1.89; features stabilized in 1.88 like let-chains are acceptable in this codebase.
📚 Learning: 2025-08-08T12:10:45.218Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5867
File: src/ipld/util.rs:553-558
Timestamp: 2025-08-08T12:10:45.218Z
Learning: Forest project targets Rust stable >=1.89; features stabilized in 1.88 like let-chains are acceptable in this codebase.
Applied to files:
README.mdsrc/chain/store/blockstore_fallback.rs
📚 Learning: 2025-08-11T12:42:42.848Z
Learnt from: elmattic
Repo: ChainSafe/forest PR: 5836
File: src/tool/subcommands/api_cmd/api_run_tests.rs:186-190
Timestamp: 2025-08-11T12:42:42.848Z
Learning: In the Forest project's test infrastructure (specifically in `src/tool/subcommands/api_cmd/api_run_tests.rs`), the team prefers to use only `ws` (non-secure WebSocket) connections for simplicity in their stateful API test scenarios, as security is not a concern in their testing context.
Applied to files:
README.md
📚 Learning: 2025-10-17T09:36:15.757Z
Learnt from: elmattic
Repo: ChainSafe/forest PR: 6128
File: src/ipld/util.rs:23-30
Timestamp: 2025-10-17T09:36:15.757Z
Learning: Always run `cargo check` or `cargo build` to verify actual compilation errors in the Forest codebase before flagging them as issues. Do not rely solely on documentation or assumptions about trait implementations.
Applied to files:
README.md
📚 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/chain/store/mod.rs
📚 Learning: 2026-01-05T12:54:40.850Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/cron_state.rs:8-8
Timestamp: 2026-01-05T12:54:40.850Z
Learning: In Rust code reviews, do not derive Eq for a struct if any field does not implement Eq (e.g., types from external dependencies). If a type like CronStateLotusJson includes fields wrapping external dependencies that lack Eq, derive PartialEq (or implement PartialEq manually) but avoid deriving Eq. This ensures comparisons compile and reflect actual equivalence semantics. When needed, consider implementing custom PartialEq (and possibly Eq) only after ensuring all fields (or wrappers) implement Eq, or keep PartialEq-only if full equality semantics cannot be expressed.
Applied to files:
src/chain/store/mod.rssrc/chain/store/blockstore_fallback.rs
📚 Learning: 2026-01-05T12:56:13.802Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/evm_state.rs:41-44
Timestamp: 2026-01-05T12:56:13.802Z
Learning: In Rust codebases (e.g., Forest), do not add #[cfg(test)] to functions already annotated with #[test]. The #[test] attribute ensures the function is compiled only for tests, so a separate #[cfg(test)] is redundant and can be removed if present. Apply this check to all Rust files that contain #[test] functions.
Applied to files:
src/chain/store/mod.rssrc/chain/store/blockstore_fallback.rs
🔇 Additional comments (6)
src/chain/store/mod.rs (1)
9-10: LGTM!The module declaration and re-export pattern is consistent with existing modules like
chain_storeanderrors.src/chain/store/blockstore_fallback.rs (5)
1-13: LGTM!Clear module documentation and minimal imports.
15-20: LGTM!The
BlockGettertrait provides a clean abstraction. Verify that existingBlockstoreimplementations can easily implement this trait when integrating the fallback into the main chain store.
22-28: LGTM!The
BitswapFetchertrait clearly documents expectations. Implementors should ensure appropriate timeouts are applied infetch_blockto avoid indefinite hangs during network fetch.
40-49: LGTM!The struct definition and constructor are well-designed. The trait bounds (
BlockGetter + Send + Sync) are appropriately applied at the impl level rather than the trait definition.
54-75: Control flow is correct.The fallback logic properly:
- Attempts local read first
- Checks if fallback is enabled (pending the env var caching fix)
- Propagates bitswap errors
- Re-reads from local store after network fetch
|
|
||
| Forest supports an experimental fallback blockstore to fetch missing blocks from the network via Bitswap when they are not available locally. This behavior is disabled by default. | ||
|
|
||
| To enable fallback, set the environment variable: |
There was a problem hiding this comment.
Typo: double space before "enable".
-To enable fallback, set the environment variable:
+To enable fallback, set the environment variable:📝 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.
| To enable fallback, set the environment variable: | |
| To enable fallback, set the environment variable: |
🤖 Prompt for AI Agents
In @README.md at line 36, The sentence "To enable fallback, set the environment
variable:" contains a double space after "To"; fix it by replacing "To enable
fallback, set the environment variable:" with "To enable fallback, set the
environment variable:" in the README (the line containing the string "To enable
fallback, set the environment variable:").
| let enable = env::var("FOREST_ENABLE_CHAINSTORE_FALLBACK") | ||
| .map(|v| v == "1") | ||
| .unwrap_or(false); | ||
| if !enable { | ||
| return Ok(None); | ||
| } |
There was a problem hiding this comment.
Avoid repeated env::var() calls on hot path.
Reading the environment variable on every get_with_fallback() call incurs syscall overhead and string allocation. During chain sync, this could be called thousands of times. Cache the value at construction time or use std::sync::LazyLock (stable in Rust 1.80+).
🔧 Suggested fix using LazyLock
use std::env;
use std::sync::Arc;
+use std::sync::LazyLock;
use cid::Cid;
use anyhow::Result;
+
+static FALLBACK_ENABLED: LazyLock<bool> = LazyLock::new(|| {
+ env::var("FOREST_ENABLE_CHAINSTORE_FALLBACK")
+ .map(|v| v == "1")
+ .unwrap_or(false)
+});Then in get_with_fallback:
- // Check environment variable to decide if fallback is enabled.
- let enable = env::var("FOREST_ENABLE_CHAINSTORE_FALLBACK")
- .map(|v| v == "1")
- .unwrap_or(false);
- if !enable {
+ if !*FALLBACK_ENABLED {
return Ok(None);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/chain/store/blockstore_fallback.rs around lines 61 - 66, The code reads
FOREST_ENABLE_CHAINSTORE_FALLBACK via env::var inside get_with_fallback on the
hot path; move that env var read out of the hot path by caching the boolean at
construction or in a static LazyLock so get_with_fallback checks a stored flag
instead of calling env::var each time. Concretely, add a field like
enable_fallback to the BlockstoreFallback (or initialize a static
std::sync::LazyLock<bool> with env::var("FOREST_ENABLE_CHAINSTORE_FALLBACK") ==
"1"), set it once at creation, and replace the inline env::var check in
get_with_fallback with a read of that cached boolean.
|
Hey @0xDevNinja could you please fix outstanding AI reviews first and I will review later. |
3 participants
This PR adds initial support for an optional chainstore fallback in Forest. When the new environment variable
FOREST_ENABLE_CHAINSTORE_FALLBACKis set to1, missing blocks will be fetched via Bitswap and then retried in the local store.Key changes:
src/chain/store/blockstore_fallback.rsimplementing aFallbackBlockstorewrapper around any blockstore. It checksFOREST_ENABLE_CHAINSTORE_FALLBACKand uses aBitswapFetcherto retrieve missing blocks when enabled.blockstore_fallbacktosrc/chain/store/mod.rsand re-exported it.BlockGetterandBitswapFetcherto abstract over block fetching and bitswap network.This resolves #6353 by enabling a fallback blockstore similar to Lotus'
LOTUS_ENABLE_CHAINSTORE_FALLBACK. Future enhancements can integrate this wrapper into the main chain store.Closes #6353
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.