fix: optimize tipset_by_height performance with known_blocks.yaml#6753
Draft
hanabi1224 wants to merge 3 commits intomainfrom
Draft
fix: optimize tipset_by_height performance with known_blocks.yaml#6753hanabi1224 wants to merge 3 commits intomainfrom
tipset_by_height performance with known_blocks.yaml#6753hanabi1224 wants to merge 3 commits intomainfrom
Conversation
Contributor
WalkthroughAdds a KnownBlocks checkpoint loader and cache, exposes ChainStore::tipset_by_height, migrates many callers from ChainIndex::tipset_by_height to ChainStore::tipset_by_height (often passing None), and updates genesis lookup to use KNOWN_BLOCKS. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ChainStore
participant KnownCache as "Known Tipsets (cache)"
participant ChainIndex
participant BlockStore
Client->>ChainStore: tipset_by_height(epoch, from?, resolve)
alt checkpoints available
ChainStore->>KnownCache: known_tipsets(network)
KnownCache-->>ChainStore: Vec<Tipset>
ChainStore->>ChainStore: select seed tipset by epoch proximity
else no seed
ChainStore->>ChainStore: fallback to provided/from or heaviest
end
ChainStore->>ChainIndex: tipset_by_height(epoch, seed_from, resolve)
ChainIndex->>BlockStore: load headers / tipset data
BlockStore-->>ChainIndex: headers
ChainIndex-->>ChainStore: resolved Tipset
ChainStore-->>Client: Tipset
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Comment |
hanabi1224
force-pushed
the
hm/optimize-tipset-by-height
branch
from
9987d48 to
d1c5d9b
Compare
hanabi1224
force-pushed
the
hm/optimize-tipset-by-height
branch
from
d1c5d9b to
fe4f628
Compare
hanabi1224
requested review from
LesnyRumcajs and
akaladarshi
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/blocks/mod.rs (1)
9-9:pub(crate)looks like the safer default here.
checkpointsappears to be an internal implementation detail for this PR, so exporting it publicly widens the API surface with build-generated data and cache internals for no clear benefit. If external crates do not need it, preferpub(crate) mod checkpoints;.As per coding guidelines, "
**/mod.rs: Each module should have amod.rsfile with public API exports and private submodules for implementation details`."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/blocks/mod.rs` at line 9, Change the module visibility for the implementation-detail module `checkpoints` from public to crate-private: replace the `pub mod checkpoints;` declaration in `mod.rs` with `pub(crate) mod checkpoints;` so the cache/build internals aren’t exported publicly; if any external crate currently depends on `checkpoints`, either add a public API wrapper or move the dependent code inside the crate to avoid breaking changes.src/blocks/tipset.rs (1)
427-427: Match known checkpoint CIDs against the full tipset, not just min-ticket block.This keeps the fast-path robust for multi-block tipsets where the known CID may not be the min-ticket block.
♻️ Proposed change
- if let Some(known_block_cid) = known_blocks.get(&tipset.epoch()) - && known_block_cid == tipset.min_ticket_block().cid() + if let Some(known_block_cid) = known_blocks.get(&tipset.epoch()) + && tipset.key().contains(known_block_cid.clone()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/blocks/tipset.rs` at line 427, The comparison currently checks known_block_cid == tipset.min_ticket_block().cid(), which misses cases where the known CID is some other block in a multi-block tipset; update the check to test membership against the full tipset (e.g., use tipset.cids().contains(&known_block_cid) or tipset.blocks().iter().any(|b| b.cid() == known_block_cid)) so the fast-path matches when any block in the tipset equals known_block_cid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/blocks/checkpoints.rs`:
- Around line 43-56: The current cache (CACHE_CALIBNET / CACHE_MAINNET) only
memoizes the success path from known_blocks_to_known_tipsets and re-runs
initialization on every ChainStore::tipset_by_height call when a blockstore is
missing checkpoint data; change the design to memoize the Result per blockstore
instance instead of global OnceLock: move the cache into ChainStore (e.g., add a
field like known_tipsets_cache: HashMap<NetworkChain,
OnceCell<Result<Vec<Tipset>, anyhow::Error>>> or similar) and store the Ok or
Err from known_blocks_to_known_tipsets so failures are cached too; update
callers that use CACHE_CALIBNET/CACHE_MAINNET to read from ChainStore’s
per-instance cache and return the cached Err immediately to avoid repeated work
in known_tipsets()/ChainStore::tipset_by_height.
In `@src/chain/store/chain_store.rs`:
- Around line 261-269: The match unconditionally prefers `from` when present
even if `from.epoch() < to`, which can forward an unusable start and make
`ChainIndex::tipset_by_height()` fail; before the match, normalize `from` by
turning any `Some(a)` with `a.epoch() < to` into `None` (e.g., `let from = match
from { Some(a) if a.epoch() >= to => Some(a), _ => None };`) so the subsequent
match on `(from, best_known_from)` will fall through to `best_known_from` or
`self.heaviest_tipset()` when `from` is below `to`, ensuring
`tipset_by_height()` gets a valid start point.
- Around line 253-260: The match that chooses the starting tipset (using the
local best_known_from variable computed from known_tipsets(self.blockstore(),
&self.chain_config().network)) can return a caller-provided from that is below
the target epoch to, causing a cryptic failure later in
ChainIndex::tipset_by_height; after you pick the effective from (the logic that
currently picks between the input from, best_known_from, or
self.heaviest_tipset()), add a validation that verifies from.epoch() >= to and
if not return an explicit Err(Error::Other(...)) noting the start tipset epoch
and the target epoch; update the code paths involving best_known_from, the match
that picks from, and any use of heaviest_tipset() so they perform this check
immediately after selection.
---
Nitpick comments:
In `@src/blocks/mod.rs`:
- Line 9: Change the module visibility for the implementation-detail module
`checkpoints` from public to crate-private: replace the `pub mod checkpoints;`
declaration in `mod.rs` with `pub(crate) mod checkpoints;` so the cache/build
internals aren’t exported publicly; if any external crate currently depends on
`checkpoints`, either add a public API wrapper or move the dependent code inside
the crate to avoid breaking changes.
In `@src/blocks/tipset.rs`:
- Line 427: The comparison currently checks known_block_cid ==
tipset.min_ticket_block().cid(), which misses cases where the known CID is some
other block in a multi-block tipset; update the check to test membership against
the full tipset (e.g., use tipset.cids().contains(&known_block_cid) or
tipset.blocks().iter().any(|b| b.cid() == known_block_cid)) so the fast-path
matches when any block in the tipset equals known_block_cid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 90e7aa9a-05b8-4797-8016-d57cd0f31eb6
📒 Files selected for processing (13)
src/blocks/checkpoints.rssrc/blocks/mod.rssrc/blocks/tipset.rssrc/chain/store/chain_store.rssrc/dev/subcommands/state_cmd.rssrc/rpc/methods/chain.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/filter/mod.rssrc/rpc/methods/eth/tipset_resolver.rssrc/rpc/methods/f3.rssrc/rpc/methods/state.rssrc/state_manager/mod.rssrc/tool/subcommands/index_cmd.rs
Comment on lines
+43
to
+56
| static CACHE_CALIBNET: OnceLock<Vec<Tipset>> = OnceLock::new(); | ||
| static CACHE_MAINNET: OnceLock<Vec<Tipset>> = OnceLock::new(); | ||
| let (cache, known_blocks) = match network { | ||
| NetworkChain::Calibnet => (&CACHE_CALIBNET, &KNOWN_BLOCKS.calibnet), | ||
| NetworkChain::Mainnet => (&CACHE_MAINNET, &KNOWN_BLOCKS.mainnet), | ||
| _ => anyhow::bail!("unsupported network {network}"), | ||
| }; | ||
| if let Some(v) = cache.get() { | ||
| Ok(v) | ||
| } else { | ||
| let tipsets = known_blocks_to_known_tipsets(bs, known_blocks)?; | ||
| _ = cache.set(tipsets); | ||
| cache.get().context("infallible") | ||
| } |
Contributor
There was a problem hiding this comment.
Memoize checkpoint lookup failures per blockstore.
This only caches the success path. If one checkpoint header or parent tipset is missing, known_tipsets() keeps retrying the same initialization work on every ChainStore::tipset_by_height() call before falling back. On stores without the full checkpoint set, that turns the optimization into permanent overhead. Consider moving this cache onto ChainStore so success/failure is memoized per blockstore instance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/blocks/checkpoints.rs` around lines 43 - 56, The current cache
(CACHE_CALIBNET / CACHE_MAINNET) only memoizes the success path from
known_blocks_to_known_tipsets and re-runs initialization on every
ChainStore::tipset_by_height call when a blockstore is missing checkpoint data;
change the design to memoize the Result per blockstore instance instead of
global OnceLock: move the cache into ChainStore (e.g., add a field like
known_tipsets_cache: HashMap<NetworkChain, OnceCell<Result<Vec<Tipset>,
anyhow::Error>>> or similar) and store the Ok or Err from
known_blocks_to_known_tipsets so failures are cached too; update callers that
use CACHE_CALIBNET/CACHE_MAINNET to read from ChainStore’s per-instance cache
and return the cached Err immediately to avoid repeated work in
known_tipsets()/ChainStore::tipset_by_height.
Comment on lines
+253
to
+260
| let best_known_from = if let Ok(known_tipsets) = | ||
| known_tipsets(self.blockstore(), &self.chain_config().network) | ||
| && let Some(ts) = known_tipsets.iter().find(|ts| ts.epoch() > to).cloned() | ||
| { | ||
| Some(ts) | ||
| } else { | ||
| None | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'known_blocks|known_blocks\.yaml|checkpoint|CHAIN_FINALITY|finality' build.rs srcRepository: ChainSafe/forest
Length of output: 50372
🏁 Script executed:
#!/bin/bash
# Look at the update_chain_checkpoints function and surrounding finality checks
sed -n '110,160p' src/dev/subcommands/update_checkpoints_cmd.rs
# Check CHECKPOINT_INTERVAL and CHAIN_FINALITY relationship
rg -n 'CHAIN_FINALITY|chain_finality' src/dev/subcommands/update_checkpoints_cmd.rs
# Check constants.rs or similar for CHAIN_FINALITY value
rg -n 'const.*CHAIN_FINALITY|const.*CHECKPOINT_INTERVAL' --type rsRepository: ChainSafe/forest
Length of output: 1999
🏁 Script executed:
#!/bin/bash
# Find the fetch_checkpoint_at_height function and check for finality guards
rg -A 20 'async fn fetch_checkpoint_at_height' src/dev/subcommands/update_checkpoints_cmd.rs
# Search for CHAIN_FINALITY definitions in networks or config
rg 'chain_finality\s*[:=]' src/networks/ src/shim/ | head -20
# Look at the end of update_chain_checkpoints to see if there's finality validation
sed -n '110,200p' src/dev/subcommands/update_checkpoints_cmd.rs | tail -50Repository: ChainSafe/forest
Length of output: 3335
🏁 Script executed:
#!/bin/bash
# Check CHAIN_FINALITY values for mainnet/calibnet
rg -B 2 -A 2 'chain_finality.*:.*900|chain_finality.*=.*900' src/networks/
# Look at full tipset_by_height implementation to check the `from` filtering issue
sed -n '246,275p' src/chain/store/chain_store.rsRepository: ChainSafe/forest
Length of output: 1168
🏁 Script executed:
#!/bin/bash
# Find CHAIN_FINALITY values for mainnet and calibnet
rg 'chain_finality.*900' src/networks/ -A 1 -B 1
# Also check the policy structure
rg -A 5 'struct.*Policy' src/shim/ | head -30
# Check if there's any finality-distance validation when adding checkpoints
rg -B 5 -A 5 'latest_checkpoint_epoch.*=' src/dev/subcommands/update_checkpoints_cmd.rsRepository: ChainSafe/forest
Length of output: 990
🏁 Script executed:
#!/bin/bash
# Find the actual CHAIN_FINALITY constants for networks
rg 'chain_finality\s*[:=]\s*900' src/networks/
# Check what CHECKPOINT_INTERVAL is
rg 'CHECKPOINT_INTERVAL' src/dev/subcommands/update_checkpoints_cmd.rs
# Check mainnet policy to see if CHECKPOINT_INTERVAL >= CHAIN_FINALITY
python3 << 'EOF'
checkpoint_interval = 86400 # 30 days from code
chain_finality = 900 # EC consensus finality for Filecoin
print(f"CHECKPOINT_INTERVAL: {checkpoint_interval} epochs")
print(f"CHAIN_FINALITY: {chain_finality} epochs")
print(f"CHECKPOINT_INTERVAL >= CHAIN_FINALITY: {checkpoint_interval >= chain_finality}")
print(f"\nDistance from head when rounding down:")
print(f" Max distance: {checkpoint_interval - 1} epochs")
print(f" Safe if >= CHAIN_FINALITY: {checkpoint_interval - 1 >= chain_finality}")
EOF
# Check if there's any safety margin or validation applied AFTER fetching
sed -n '110,180p' src/dev/subcommands/update_checkpoints_cmd.rs | grep -A 10 'Fetching checkpoints'Repository: ChainSafe/forest
Length of output: 1055
🏁 Script executed:
#!/bin/bash
# Check if there's validation for from.epoch() >= to in chain_index.tipset_by_height
rg -A 30 'fn tipset_by_height' src/chain/index.rs | head -40
# Also check if the caller-provided `from` parameter is documented
rg -B 5 'pub fn tipset_by_height' src/chain/store/chain_store.rsRepository: ChainSafe/forest
Length of output: 313
🏁 Script executed:
#!/bin/bash
# Find where ChainIndex is defined
fd ChainIndex src/ --type f | head -5
# Find the implementation of chain_index().tipset_by_height()
rg -l 'impl.*ChainIndex' src/ | head -5
# Search for the actual tipset_by_height in ChainIndex
rg -A 30 'impl.*ChainIndex' src/chain/ | grep -A 30 'tipset_by_height'Repository: ChainSafe/forest
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Find the tipset_by_height method in ChainIndex
rg -A 40 'fn tipset_by_height' src/chain/store/index.rs
# Also verify the behavior when from.epoch() < to
sed -n '1,100p' src/chain/store/index.rs | tail -50Repository: ChainSafe/forest
Length of output: 3717
Add validation to reject caller-provided from that is already below the target height to.
The checkpoint finality constraint is sound—CHECKPOINT_INTERVAL (86400 epochs) >> CHAIN_FINALITY (900 epochs) guarantees that known tipsets are immutable. However, the match logic at lines 253–260 does not validate that the selected from satisfies from.epoch() >= to. If a caller passes a from with from.epoch() < to and no closer checkpoint is found, this invalid from will be delegated to ChainIndex::tipset_by_height(), which will immediately fail with a cryptic error ("looking for tipset with height greater than start point").
Add a validation after the match (or inline in the match arms) to ensure from.epoch() >= to:
let from = match (from, best_known_from) {
(Some(a), Some(b)) if b.epoch() < a.epoch() => b,
(Some(a), _) => a,
(None, Some(b)) => b,
(None, None) => self.heaviest_tipset(),
};
if from.epoch() < to {
return Err(Error::Other(format!("start tipset epoch {} is before target epoch {}", from.epoch(), to)));
}This prevents downstream errors and provides a clear, actionable error message to callers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/chain/store/chain_store.rs` around lines 253 - 260, The match that
chooses the starting tipset (using the local best_known_from variable computed
from known_tipsets(self.blockstore(), &self.chain_config().network)) can return
a caller-provided from that is below the target epoch to, causing a cryptic
failure later in ChainIndex::tipset_by_height; after you pick the effective from
(the logic that currently picks between the input from, best_known_from, or
self.heaviest_tipset()), add a validation that verifies from.epoch() >= to and
if not return an explicit Err(Error::Other(...)) noting the start tipset epoch
and the target epoch; update the code paths involving best_known_from, the match
that picks from, and any use of heaviest_tipset() so they perform this check
immediately after selection.
Comment on lines
+261
to
+269
| let from = match (from, best_known_from) { | ||
| // prefer `b` when `b` is closer to `to` | ||
| (Some(a), Some(b)) if b.epoch() < a.epoch() => b, | ||
| // prefer `a` when presents | ||
| (Some(a), _) => a, | ||
| (None, Some(b)) => b, | ||
| // fallback to chain head | ||
| (None, None) => self.heaviest_tipset(), | ||
| }; |
Contributor
There was a problem hiding this comment.
Ignore unusable from tipsets before delegating to ChainIndex.
If a caller passes Some(from) with from.epoch() < to, the (Some(a), _) => a arm forwards an invalid start point and ChainIndex::tipset_by_height() fails immediately, even though best_known_from or the head could satisfy the lookup. Filter from with epoch() >= to before this match.
♻️ Suggested fix
- let from = match (from, best_known_from) {
+ let from = match (from.filter(|ts| ts.epoch() >= to), best_known_from) {
// prefer `b` when `b` is closer to `to`
(Some(a), Some(b)) if b.epoch() < a.epoch() => b,
// prefer `a` when presents
(Some(a), _) => a,
(None, Some(b)) => b,
// fallback to chain head
(None, None) => self.heaviest_tipset(),
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/chain/store/chain_store.rs` around lines 261 - 269, The match
unconditionally prefers `from` when present even if `from.epoch() < to`, which
can forward an unusable start and make `ChainIndex::tipset_by_height()` fail;
before the match, normalize `from` by turning any `Some(a)` with `a.epoch() <
to` into `None` (e.g., `let from = match from { Some(a) if a.epoch() >= to =>
Some(a), _ => None };`) so the subsequent match on `(from, best_known_from)`
will fall through to `best_known_from` or `self.heaviest_tipset()` when `from`
is below `to`, ensuring `tipset_by_height()` gets a valid start point.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/chain/store/chain_store.rs (1)
307-317: Add a doc comment forload_child_tipset.This public helper now relies on
TakeNewerplus a parent-key check to skip null epochs and reject unrelated branches; that contract is subtle enough to document explicitly.As per coding guidelines: "Document public functions and structs with doc comments".📝 Suggested doc comment
+ /// Returns the next non-null child tipset of `ts` on the current heaviest chain. + /// Returns [`Error::NotFound`] when `ts` is the current head or is not on that chain. pub fn load_child_tipset(&self, ts: &Tipset) -> Result<Tipset, Error> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/store/chain_store.rs` around lines 307 - 317, Add a doc comment to the public function load_child_tipset describing its contract: that it attempts to return the immediate child tipset at epoch+1, uses ResolveNullTipset::TakeNewer to skip null epochs, validates the child by checking that maybe_child.parents() equals ts.key() and returns Error::NotFound if the parent-key check fails (thus rejecting unrelated branches), and mention the returned Result<Tipset, Error> semantics. Include brief notes on why ResolveNullTipset::TakeNewer is used and the parental-key check so callers understand null-epoch behavior and branch rejection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/chain/store/chain_store.rs`:
- Around line 253-269: best_known_from may pick a checkpoint that is not
finalized relative to the current active head and thus choose a tipset from the
wrong fork; update the lookup in the block where best_known_from is computed
(using known_tipsets(self.blockstore(), &self.chain_config().network) and the
subsequent match that picks `from`) to filter candidate tipsets by requiring
their epoch to be <= effective_head_epoch - chain_finality (compute
effective_head_epoch from the current head/heaviest tipset via
self.heaviest_tipset().epoch() or equivalent) before allowing them to override
`from`, so only checkpoints finalized against the active head can replace the
caller/default start tipset.
---
Nitpick comments:
In `@src/chain/store/chain_store.rs`:
- Around line 307-317: Add a doc comment to the public function
load_child_tipset describing its contract: that it attempts to return the
immediate child tipset at epoch+1, uses ResolveNullTipset::TakeNewer to skip
null epochs, validates the child by checking that maybe_child.parents() equals
ts.key() and returns Error::NotFound if the parent-key check fails (thus
rejecting unrelated branches), and mention the returned Result<Tipset, Error>
semantics. Include brief notes on why ResolveNullTipset::TakeNewer is used and
the parental-key check so callers understand null-epoch behavior and branch
rejection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e41bbbde-bdf9-4592-89c6-30822b17cf69
📒 Files selected for processing (6)
src/chain/store/chain_store.rssrc/dev/subcommands/state_cmd.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/filter/mod.rssrc/rpc/methods/state.rssrc/state_manager/mod.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/rpc/methods/eth.rs
- src/rpc/methods/state.rs
- src/dev/subcommands/state_cmd.rs
- src/rpc/methods/eth/filter/mod.rs
Comment on lines
+253
to
+269
| let best_known_from = if let Ok(known_tipsets) = | ||
| known_tipsets(self.blockstore(), &self.chain_config().network) | ||
| && let Some(ts) = known_tipsets.iter().find(|ts| ts.epoch() > to).cloned() | ||
| { | ||
| Some(ts) | ||
| } else { | ||
| None | ||
| }; | ||
| let from = match (from, best_known_from) { | ||
| // prefer `b` when `b` is closer to `to` | ||
| (Some(a), Some(b)) if b.epoch() < a.epoch() => b, | ||
| // prefer `a` when presents | ||
| (Some(a), _) => a, | ||
| (None, Some(b)) => b, | ||
| // fallback to chain head | ||
| (None, None) => self.heaviest_tipset(), | ||
| }; |
Contributor
There was a problem hiding this comment.
Only seed from known checkpoints once they're finalized against the active head.
best_known_from can replace the caller/default start tipset purely because it's closer to to. When the active head is still within chain_finality of that checkpoint, the checkpoint is not guaranteed to be on the same branch yet, so this lookup can answer from the wrong fork. Filter checkpoint candidates against the effective head epoch before letting them override from.
♻️ Possible fix
+ let head_epoch = from
+ .as_ref()
+ .map(|ts| ts.epoch())
+ .unwrap_or_else(|| self.heaviest_tipset().epoch());
let best_known_from = if let Ok(known_tipsets) =
known_tipsets(self.blockstore(), &self.chain_config().network)
- && let Some(ts) = known_tipsets.iter().find(|ts| ts.epoch() > to).cloned()
+ && let Some(ts) = known_tipsets
+ .iter()
+ .find(|ts| {
+ ts.epoch() > to
+ && ts.epoch() + self.chain_config().policy.chain_finality <= head_epoch
+ })
+ .cloned()
{
Some(ts)
} else {
None
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/chain/store/chain_store.rs` around lines 253 - 269, best_known_from may
pick a checkpoint that is not finalized relative to the current active head and
thus choose a tipset from the wrong fork; update the lookup in the block where
best_known_from is computed (using known_tipsets(self.blockstore(),
&self.chain_config().network) and the subsequent match that picks `from`) to
filter candidate tipsets by requiring their epoch to be <= effective_head_epoch
- chain_finality (compute effective_head_epoch from the current head/heaviest
tipset via self.heaviest_tipset().epoch() or equivalent) before allowing them to
override `from`, so only checkpoints finalized against the active head can
replace the caller/default start tipset.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/chain/store/chain_store.rs (1)
270-278:⚠️ Potential issue | 🟡 MinorFilter out invalid caller-provided
frombefore choosing the start tipset.At Line 270-Line 275,
Some(a)can still win even whena.epoch() < to, which later fails inChainIndex::tipset_by_height()with a less actionable error. Normalizefromfirst (from.filter(|ts| ts.epoch() >= to)) or return an explicit error before delegation.♻️ Suggested fix
- let from = match (from, best_known_from) { + let from = match (from.filter(|ts| ts.epoch() >= to), best_known_from) { // prefer `b` when `b` is closer to `to` (Some(a), Some(b)) if b.epoch() < a.epoch() => b, // prefer `a` when presents (Some(a), _) => a, (None, Some(b)) => b, // fallback to chain head (None, None) => head, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/store/chain_store.rs` around lines 270 - 278, Normalize and filter the caller-provided `from` before the match that selects the start tipset: ensure any supplied `from` is rejected or removed if its epoch is less than `to` (e.g., apply from.filter(|ts| ts.epoch() >= to) or return an explicit error) so that the match between `from`, `best_known_from`, and `head` cannot pick an invalid tipset and downstream calls like ChainIndex::tipset_by_height / tipset_by_height won't fail with a less actionable error; update the logic that references `from`, `best_known_from`, and `head` to operate on the normalized/validated `from`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/chain/store/chain_store.rs`:
- Around line 270-278: Normalize and filter the caller-provided `from` before
the match that selects the start tipset: ensure any supplied `from` is rejected
or removed if its epoch is less than `to` (e.g., apply from.filter(|ts|
ts.epoch() >= to) or return an explicit error) so that the match between `from`,
`best_known_from`, and `head` cannot pick an invalid tipset and downstream calls
like ChainIndex::tipset_by_height / tipset_by_height won't fail with a less
actionable error; update the logic that references `from`, `best_known_from`,
and `head` to operate on the normalized/validated `from`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c3b9c4e6-2c34-47fa-a91a-4e4745112318
📒 Files selected for processing (1)
src/chain/store/chain_store.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
This PR optimizes
tipset_by_heightperformance withknown_blocks.yamlThe below RPC request is now blazingly fast before the runtime cache is populated
{ "jsonrpc": "2.0", "method": "Filecoin.ChainGetTipSetByHeight", "params": [ 86300, null ], "id": 123 }Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Refactor