Refactor tipset_by_height and load_child_tipset to distinguish error from miss#7005
Merged
sudo-shashank merged 6 commits intomainfrom May 7, 2026
Merged
Refactor tipset_by_height and load_child_tipset to distinguish error from miss#7005sudo-shashank merged 6 commits intomainfrom
sudo-shashank merged 6 commits intomainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors tipset lookup APIs: ChangesTipset lookup & call-site propagation
Sequence Diagram(s)(Skipped — changes are API return-shape refactors and enriched error contexts; no new multi-component sequential control flow requiring visualization.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
sudo-shashank
changed the title
![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: 1
🧹 Nitpick comments (4)
src/state_manager/mod.rs (1)
472-478: ⚡ Quick winAvoid the second
load_child_tipseton theNonepath.When
load_child_tipset(ts)returnsNone,load_tipset_statefalls through toload_executed_tipset(ts), which immediately callsload_child_tipset(ts)again inside the cache fill. That adds an extra chain-store lookup on head-tipset cache misses in a hot path. Thread the already-resolved child tipset into the executed-tipset load instead of re-querying it.Also applies to: 497-503
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/state_manager/mod.rs` around lines 472 - 478, The match in load_tipset_state currently calls self.chain_store().load_child_tipset(ts) and, on None, falls through to load_executed_tipset(ts) which re-queries load_child_tipset inside its cache fill; to avoid the duplicate lookup, change load_executed_tipset to accept an Option<ChildTipset> (or add a helper like load_executed_tipset_with_child) and pass the already-resolved child tipset when available so the cache-fill path uses the provided child instead of calling load_child_tipset again; apply the same pattern to the other occurrence referenced (around lines 497-503) so both sites thread the resolved child tipset into the executed-tipset loader.src/tool/subcommands/archive_cmd.rs (1)
844-854: ⚡ Quick winMirror the two-stage error context used in
do_export.Here the new lookup only labels the miss case. If
tipset_by_heightreturns a real storage/index error, the CLI loses the requested epoch from the error path.Proposed pattern
let tipset = chain_index - .tipset_by_height(epoch, heaviest_tipset.clone(), ResolveNullTipset::TakeOlder)? + .tipset_by_height(epoch, heaviest_tipset.clone(), ResolveNullTipset::TakeOlder) + .with_context(|| format!("failed to resolve tipset at epoch {epoch}"))? .context("tipset not found at requested epoch")?; let child_tipset = chain_index .tipset_by_height( epoch + 1, heaviest_tipset.clone(), ResolveNullTipset::TakeNewer, - )? + ) + .with_context(|| format!("failed to resolve child tipset after epoch {epoch}"))? .context("child tipset not found at epoch+1")?;As per coding guidelines "Use
anyhow::Result<T>for most operations and add context with.context()when errors occur".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tool/subcommands/archive_cmd.rs` around lines 844 - 854, The two tipset lookups using chain_index.tipset_by_height (for variables tipset and child_tipset with ResolveNullTipset::TakeOlder/TakeNewer and heaviest_tipset) only add a final `.context("... not found")` which hides the requested epoch on storage/index errors; change both calls to use the two-stage pattern used in do_export: first attempt the call, map or propagate the underlying error, and then apply `.context(format!("tipset not found at requested epoch {}", epoch))` (and similarly for epoch+1) so that real storage/index errors retain their original messages while the missing-tipset case still includes the epoch context.src/rpc/methods/chain.rs (1)
397-400: ⚡ Quick winPreserve call-site context for the
Err(_)path too.These updated lookups mostly convert
Noneinto a better error, but they still let storage/index failures escape without height-specific context. The finality helper fallback lookups also propagate raw errors today.Proposed pattern
let tss = ctx .chain_index() - .tipset_by_height(height, ts, ResolveNullTipset::TakeOlder)? + .tipset_by_height(height, ts, ResolveNullTipset::TakeOlder) + .with_context(|| format!("failed to resolve tipset at height {height}"))? .with_context(|| format!("tipset not found at height {height}"))?;Use the same two-stage pattern in the export call sites and in the finality fallback lookups so both
Err(_)andOk(None)are diagnosable.As per coding guidelines "Use
anyhow::Result<T>for most operations and add context with.context()when errors occur".Also applies to: 596-599, 898-900, 929-930, 1071-1074, 1099-1102, 1237-1254
src/rpc/methods/eth.rs (1)
957-960: ⚡ Quick winAdd context before unwrapping the lookup
Result.These call sites currently only annotate the
Ok(None)case. Iftipset_by_heightfails due to blockstore/index IO, the?returns early and the queried height never appears in the error.Proposed pattern
chain .chain_index() - .tipset_by_height(height, head, resolve)? + .tipset_by_height(height, head, resolve) + .with_context(|| format!("failed to resolve tipset at height {height}"))? .with_context(|| format!("tipset not found at height {height}"))Apply the same two-stage pattern to the canonical-height lookup as well.
As per coding guidelines "Use
anyhow::Result<T>for most operations and add context with.context()when errors occur".Also applies to: 973-976
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/methods/eth.rs` around lines 957 - 960, The call to chain.chain_index().tipset_by_height(...) currently applies ? before adding context, so IO/index errors lose the height context; change to the two-stage pattern: first call chain.chain_index().tipset_by_height(height, head, resolve).with_context(|| format!("failed to lookup tipset at height {height}"))? to attach context to any Err, then separately handle the Ok(None) case (e.g., .ok_or_else(|| anyhow::anyhow!("tipset not found at height {height}"))?) so that both IO errors and "not found" return messages include the height; apply the same change for the other occurrence around tipset_by_height at the later call site.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/chain/store/index.rs`:
- Around line 121-126: The chain walk currently uses
from.chain(&self.db).tuple_windows() which stops silently on missing ancestor
tipsets and causes missing-parent errors to be treated as Ok(None); replace that
iterator usage with an explicit ancestor-load loop that calls the tipset
parent-loading API (e.g. invoke the Tipset/Store method that loads parents from
self.db for each step) and propagate any storage/load errors as Err(...)
immediately, only returning Ok(None) when you have explicitly verified the chain
contains no tipset at the requested epoch; update the same logic used in the
comparable block further down (the code region replacing tuple_windows) so both
places explicitly load parents and fail on missing data instead of downgrading
to Ok(None).
---
Nitpick comments:
In `@src/rpc/methods/eth.rs`:
- Around line 957-960: The call to chain.chain_index().tipset_by_height(...)
currently applies ? before adding context, so IO/index errors lose the height
context; change to the two-stage pattern: first call
chain.chain_index().tipset_by_height(height, head, resolve).with_context(||
format!("failed to lookup tipset at height {height}"))? to attach context to any
Err, then separately handle the Ok(None) case (e.g., .ok_or_else(||
anyhow::anyhow!("tipset not found at height {height}"))?) so that both IO errors
and "not found" return messages include the height; apply the same change for
the other occurrence around tipset_by_height at the later call site.
In `@src/state_manager/mod.rs`:
- Around line 472-478: The match in load_tipset_state currently calls
self.chain_store().load_child_tipset(ts) and, on None, falls through to
load_executed_tipset(ts) which re-queries load_child_tipset inside its cache
fill; to avoid the duplicate lookup, change load_executed_tipset to accept an
Option<ChildTipset> (or add a helper like load_executed_tipset_with_child) and
pass the already-resolved child tipset when available so the cache-fill path
uses the provided child instead of calling load_child_tipset again; apply the
same pattern to the other occurrence referenced (around lines 497-503) so both
sites thread the resolved child tipset into the executed-tipset loader.
In `@src/tool/subcommands/archive_cmd.rs`:
- Around line 844-854: The two tipset lookups using chain_index.tipset_by_height
(for variables tipset and child_tipset with
ResolveNullTipset::TakeOlder/TakeNewer and heaviest_tipset) only add a final
`.context("... not found")` which hides the requested epoch on storage/index
errors; change both calls to use the two-stage pattern used in do_export: first
attempt the call, map or propagate the underlying error, and then apply
`.context(format!("tipset not found at requested epoch {}", epoch))` (and
similarly for epoch+1) so that real storage/index errors retain their original
messages while the missing-tipset case still includes the epoch context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1d34d777-c49e-410e-86e7-998b738ebfad
📒 Files selected for processing (19)
src/chain/store/chain_store.rssrc/chain/store/index.rssrc/dev/subcommands/export_state_tree_cmd.rssrc/dev/subcommands/state_cmd.rssrc/interpreter/fvm3.rssrc/interpreter/fvm4.rssrc/message_pool/msgpool/provider.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/chain_rand.rssrc/state_manager/mod.rssrc/tool/subcommands/archive_cmd.rssrc/tool/subcommands/benchmark_cmd.rssrc/tool/subcommands/index_cmd.rssrc/tool/subcommands/snapshot_cmd.rs
sudo-shashank
requested review from
LesnyRumcajs and
hanabi1224
and removed request for
a team
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/chain/store/index.rs (1)
183-187:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate checkpoint load errors instead of treating them like cache misses.
This fast path currently collapses
Err(_)andOk(None)into the same branch. If a cached checkpoint key hits a real blockstore/IO failure,tipset_by_heightsilently retries from the originalfromtipset instead of fast-failing.Suggested fix
let mut checkpoint_from_epoch = to; while checkpoint_from_epoch < from_epoch { - if let Some(checkpoint_from_key) = - self.ts_height_cache.get_cloned(&checkpoint_from_epoch) - && let Ok(Some(checkpoint_from)) = self.load_tipset(&checkpoint_from_key) - { - from = checkpoint_from; - break; + if let Some(checkpoint_from_key) = + self.ts_height_cache.get_cloned(&checkpoint_from_epoch) + { + match self.load_tipset(&checkpoint_from_key) { + Ok(Some(checkpoint_from)) => { + from = checkpoint_from; + break; + } + Ok(None) => {} + Err(err) => return Err(err), + } } checkpoint_from_epoch = next_checkpoint(checkpoint_from_epoch); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chain/store/index.rs` around lines 183 - 187, The fast-path inside tipset_by_height currently treats load_tipset errors the same as cache misses; change the logic around checkpoint_from_epoch so that after retrieving checkpoint_from_key via self.ts_height_cache.get_cloned(&checkpoint_from_epoch), you explicitly match the result of self.load_tipset(&checkpoint_from_key): if load_tipset returns Err(e) propagate/return that Err immediately (fast-fail), if it returns Ok(Some(checkpoint_from)) continue the fast-path as intended, and if it returns Ok(None) treat it as a cache miss and fall back to the existing slow path. Update the block containing checkpoint_from_epoch, ts_height_cache.get_cloned, and load_tipset to implement this explicit matching/propagation.src/rpc/methods/chain.rs (1)
1235-1249:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't collapse EC finality lookup errors into
None.Both branches here still swallow
tipset_by_heightfailures (let Ok(Some(...))/.ok().flatten()). That makes blockstore corruption look like “no finalized tipset” and defeats the new miss-vs-error split forChainGetTipSetFinalityStatus.Suggested fix
- let finalized = if depth >= 0 - && let Ok(Some(ts)) = ctx.chain_index().tipset_by_height( - (head.epoch() - depth).max(0), - head.shallow_clone(), - ResolveNullTipset::TakeOlder, - ) - { - Some(ts) - } else { - let ec_finality_epoch = - (head.epoch() - ctx.chain_config().policy.chain_finality).max(0); - ctx.chain_index() - .tipset_by_height(ec_finality_epoch, head, ResolveNullTipset::TakeOlder) - .ok() - .flatten() - }; + let finalized = if depth >= 0 { + let threshold_epoch = (head.epoch() - depth).max(0); + match ctx + .chain_index() + .tipset_by_height( + threshold_epoch, + head.shallow_clone(), + ResolveNullTipset::TakeOlder, + ) + .with_context(|| { + format!("failed to resolve EC finality tipset at height {threshold_epoch}") + })? + { + Some(ts) => Some(ts), + None => { + let ec_finality_epoch = + (head.epoch() - ctx.chain_config().policy.chain_finality).max(0); + ctx.chain_index() + .tipset_by_height(ec_finality_epoch, head, ResolveNullTipset::TakeOlder) + .with_context(|| { + format!( + "failed to resolve fallback EC finality tipset at height {ec_finality_epoch}" + ) + })? + } + } + } else { + let ec_finality_epoch = + (head.epoch() - ctx.chain_config().policy.chain_finality).max(0); + ctx.chain_index() + .tipset_by_height(ec_finality_epoch, head, ResolveNullTipset::TakeOlder) + .with_context(|| { + format!( + "failed to resolve fallback EC finality tipset at height {ec_finality_epoch}" + ) + })? + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/methods/chain.rs` around lines 1235 - 1249, The code currently silences errors from ctx.chain_index().tipset_by_height in both branches (the pattern using let Ok(Some(...)) and .ok().flatten()), turning real errors into a None "no finalized tipset" result; update the logic in the block that computes finalized so that you explicitly handle Result from ctx.chain_index().tipset_by_height: match the Result to propagate Err (return or map the error into the RPC error used by ChainGetTipSetFinalityStatus), treat Ok(Some(ts)) as Some(ts), and treat Ok(None) as the intended None/miss case; locate the computation assigned to finalized and replace the uses of let Ok(Some(...)) and .ok().flatten() with explicit match/if let handling that distinguishes Err vs Ok(None) so blockstore/lookup errors are not collapsed into a missing-finalized-tipset outcome.
🧹 Nitpick comments (1)
src/rpc/methods/chain.rs (1)
397-400: ⚡ Quick winAdd context on the
Resultlayer before unwrapping theOption.These call sites only annotate the
Nonecase right now. Iftipset_by_heightreturns a real storage error, the RPC loses the height-specific context that this PR is trying to preserve.Example pattern
let start_ts = ctx .chain_index() - .tipset_by_height(epoch, head, ResolveNullTipset::TakeOlder)? + .tipset_by_height(epoch, head, ResolveNullTipset::TakeOlder) + .with_context(|| format!("couldn't get a tipset at height {epoch}"))? .with_context(|| format!("tipset not found at height {epoch}"))?;As per coding guidelines, "
**/*.rs: Useanyhow::Result<T>for most operations and add context with.context()when errors occur`."Also applies to: 596-600, 896-899, 927-930, 1071-1074, 1099-1102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/methods/chain.rs` around lines 397 - 400, The call to chain_index().tipset_by_height(..., ResolveNullTipset::TakeOlder) currently attaches the height context only when the Option is None; instead add context on the Result layer before unwrapping so storage errors also get the height info: call .context(|| format!("tipset not found at height {epoch}")) (or .with_context on the Result) immediately after tipset_by_height(...) and before any ? that converts the Result/Option, e.g. adjust the chain_index().tipset_by_height(...) usage in the current snippet and the other similar tipset_by_height call sites in this file so the error from storage is annotated with the epoch message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/chain/store/index.rs`:
- Around line 183-187: The fast-path inside tipset_by_height currently treats
load_tipset errors the same as cache misses; change the logic around
checkpoint_from_epoch so that after retrieving checkpoint_from_key via
self.ts_height_cache.get_cloned(&checkpoint_from_epoch), you explicitly match
the result of self.load_tipset(&checkpoint_from_key): if load_tipset returns
Err(e) propagate/return that Err immediately (fast-fail), if it returns
Ok(Some(checkpoint_from)) continue the fast-path as intended, and if it returns
Ok(None) treat it as a cache miss and fall back to the existing slow path.
Update the block containing checkpoint_from_epoch, ts_height_cache.get_cloned,
and load_tipset to implement this explicit matching/propagation.
In `@src/rpc/methods/chain.rs`:
- Around line 1235-1249: The code currently silences errors from
ctx.chain_index().tipset_by_height in both branches (the pattern using let
Ok(Some(...)) and .ok().flatten()), turning real errors into a None "no
finalized tipset" result; update the logic in the block that computes finalized
so that you explicitly handle Result from ctx.chain_index().tipset_by_height:
match the Result to propagate Err (return or map the error into the RPC error
used by ChainGetTipSetFinalityStatus), treat Ok(Some(ts)) as Some(ts), and treat
Ok(None) as the intended None/miss case; locate the computation assigned to
finalized and replace the uses of let Ok(Some(...)) and .ok().flatten() with
explicit match/if let handling that distinguishes Err vs Ok(None) so
blockstore/lookup errors are not collapsed into a missing-finalized-tipset
outcome.
---
Nitpick comments:
In `@src/rpc/methods/chain.rs`:
- Around line 397-400: The call to chain_index().tipset_by_height(...,
ResolveNullTipset::TakeOlder) currently attaches the height context only when
the Option is None; instead add context on the Result layer before unwrapping so
storage errors also get the height info: call .context(|| format!("tipset not
found at height {epoch}")) (or .with_context on the Result) immediately after
tipset_by_height(...) and before any ? that converts the Result/Option, e.g.
adjust the chain_index().tipset_by_height(...) usage in the current snippet and
the other similar tipset_by_height call sites in this file so the error from
storage is annotated with the epoch message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 683bb560-d9e6-4ae9-8e20-70282996de34
📒 Files selected for processing (2)
src/chain/store/index.rssrc/rpc/methods/chain.rs
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/methods/chain.rs (1)
1202-1216:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFinality status still suppresses lookup failures.
These branches discard tipset load errors with
if let Ok(parent)and.ok().flatten(). On a corrupted or partially-pruned chain,ChainGetTipSetFinalityStatus,safe, andfinalizedcan now return a fallback tipset instead of surfacing the storage failure, which is the same ambiguity this refactor is trying to remove elsewhere.🛠️ Suggested direction
while chain.len() < chain_len { chain.push(ts.len() as i64); - if let Ok(parent) = ctx.chain_index().load_required_tipset(ts.parents()) { - // insert 0 for null rounds - if let Ok(n_null_tipsets_to_pad) = usize::try_from(ts.epoch() - parent.epoch() - 1) - && n_null_tipsets_to_pad > 0 - { - let target_len = - (chain.len().saturating_add(n_null_tipsets_to_pad)).min(chain_len); - chain.resize(target_len, 0); - } - ts = parent; - } else { + if ts.epoch() == 0 { break; } + let parent = ctx + .chain_index() + .load_required_tipset(ts.parents()) + .context("failed to load ancestor while computing EC finality")?; + if let Ok(n_null_tipsets_to_pad) = usize::try_from(ts.epoch() - parent.epoch() - 1) + && n_null_tipsets_to_pad > 0 + { + let target_len = + (chain.len().saturating_add(n_null_tipsets_to_pad)).min(chain_len); + chain.resize(target_len, 0); + } + ts = parent; } … - let finalized = if depth >= 0 - && let Ok(Some(ts)) = ctx.chain_index().tipset_by_height( - (head.epoch() - depth).max(0), - head.shallow_clone(), - ResolveNullTipset::TakeOlder, - ) { - Some(ts) + let finalized = if depth >= 0 { + ctx.chain_index() + .tipset_by_height( + (head.epoch() - depth).max(0), + head.shallow_clone(), + ResolveNullTipset::TakeOlder, + ) + .with_context(|| format!("failed to resolve EC finalized tipset at depth {depth}"))? } else { let ec_finality_epoch = (head.epoch() - ctx.chain_config().policy.chain_finality).max(0); ctx.chain_index() .tipset_by_height(ec_finality_epoch, head, ResolveNullTipset::TakeOlder) - .ok() - .flatten() + .with_context(|| format!("failed to resolve EC finalized tipset at epoch {ec_finality_epoch}"))? };As per coding guidelines:
**/*.rs: Useanyhow::Result<T>for most operations and add context with.context()when errors occur.Also applies to: 1235-1248
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/methods/chain.rs` around lines 1202 - 1216, The loop currently swallows storage errors by using "if let Ok(parent)" and .ok().flatten(), causing ChainGetTipSetFinalityStatus (and helper code paths like safe and finalized) to return fallback tipsets instead of propagating load failures; change these to propagate errors with anyhow::Result and add context: replace the "if let Ok(parent) = ctx.chain_index().load_required_tipset(ts.parents()) { ... } else { break; }" pattern with a fallible load that uses ? (or map_err(...).context(...)?), e.g. call ctx.chain_index().load_required_tipset(ts.parents()).context("loading parent tipset for ChainGetTipSetFinalityStatus")? and handle the result accordingly so storage failures bubble up; apply the same treatment to the similar block around lines 1235-1248 (and any .ok().flatten() uses in this function) so errors are surfaced instead of suppressed.
♻️ Duplicate comments (1)
src/chain/store/index.rs (1)
121-125:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
tipset_by_heightstill has no clean-miss path.Line 212 still walks with
from.chain(&self.db).tuple_windows(), and the only unresolved exit at Lines 230-232 isErr(...), soResult<Option<Tipset>, _>never actually producesOk(None). That means callers in this PR still can't observe the new miss-vs-error split from#6755. Either keep the old non-optional signature or switch this to an explicit parent-load loop so missing ancestors stayErrand a proven miss can returnOk(None).Also applies to: 162-233
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/rpc/methods/chain.rs`:
- Around line 1202-1216: The loop currently swallows storage errors by using "if
let Ok(parent)" and .ok().flatten(), causing ChainGetTipSetFinalityStatus (and
helper code paths like safe and finalized) to return fallback tipsets instead of
propagating load failures; change these to propagate errors with anyhow::Result
and add context: replace the "if let Ok(parent) =
ctx.chain_index().load_required_tipset(ts.parents()) { ... } else { break; }"
pattern with a fallible load that uses ? (or map_err(...).context(...)?), e.g.
call ctx.chain_index().load_required_tipset(ts.parents()).context("loading
parent tipset for ChainGetTipSetFinalityStatus")? and handle the result
accordingly so storage failures bubble up; apply the same treatment to the
similar block around lines 1235-1248 (and any .ok().flatten() uses in this
function) so errors are surfaced instead of suppressed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 14a65015-54f6-456a-a482-9641c7fe7033
📒 Files selected for processing (2)
src/chain/store/index.rssrc/rpc/methods/chain.rs
sudo-shashank
force-pushed
the
shashank/refactor-tipset-lookup
branch
from
f111bb7 to
39c0377
Compare
sudo-shashank
force-pushed
the
shashank/refactor-tipset-lookup
branch
from
39c0377 to
878bb44
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/methods/chain.rs (1)
1235-1250:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStorage/IO errors silently swallowed in first
tipset_by_heightcall.The
let Ok(Some(ts)) = ...pattern treatsErr(...)the same asOk(None), causing genuine storage/IO errors to fall through to the else branch instead of propagating. This contradicts the PR objective of ensuring fast-fail behavior for real errors.Consider separating the error case from the cache-miss case:
Proposed fix to propagate errors while preserving fallback for cache miss
- let finalized = if depth >= 0 - && let Ok(Some(ts)) = ctx.chain_index().tipset_by_height( + let finalized = if depth >= 0 { + match ctx.chain_index().tipset_by_height( (head.epoch() - depth).max(0), head.shallow_clone(), ResolveNullTipset::TakeOlder, - ) { - Some(ts) + )? { + Some(ts) => Some(ts), + None => { + let ec_finality_epoch = + (head.epoch() - ctx.chain_config().policy.chain_finality).max(0); + ctx.chain_index().tipset_by_height( + ec_finality_epoch, + head, + ResolveNullTipset::TakeOlder, + )? + } + } } else { let ec_finality_epoch = (head.epoch() - ctx.chain_config().policy.chain_finality).max(0); ctx.chain_index().tipset_by_height( ec_finality_epoch, head, ResolveNullTipset::TakeOlder, )? };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/methods/chain.rs` around lines 1235 - 1250, The current let Ok(Some(ts)) = ctx.chain_index().tipset_by_height(...) pattern silently treats Err as Ok(None) and lets storage/IO errors fall through; change this to explicitly match the Result from ctx.chain_index().tipset_by_height (or use let tmp = ...? if you want to propagate immediately) so that Err is propagated (using ? or returning the error) while still distinguishing Ok(Some(ts)) (use that for finalized) and Ok(None) (perform the fallback that computes ec_finality_epoch and calls ctx.chain_index().tipset_by_height(...) with ResolveNullTipset::TakeOlder); specifically update the logic around finalized, head, depth, ctx.chain_index().tipset_by_height and ResolveNullTipset::TakeOlder to handle Err vs Ok(None) separately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/rpc/methods/chain.rs`:
- Around line 1235-1250: The current let Ok(Some(ts)) =
ctx.chain_index().tipset_by_height(...) pattern silently treats Err as Ok(None)
and lets storage/IO errors fall through; change this to explicitly match the
Result from ctx.chain_index().tipset_by_height (or use let tmp = ...? if you
want to propagate immediately) so that Err is propagated (using ? or returning
the error) while still distinguishing Ok(Some(ts)) (use that for finalized) and
Ok(None) (perform the fallback that computes ec_finality_epoch and calls
ctx.chain_index().tipset_by_height(...) with ResolveNullTipset::TakeOlder);
specifically update the logic around finalized, head, depth,
ctx.chain_index().tipset_by_height and ResolveNullTipset::TakeOlder to handle
Err vs Ok(None) separately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 40b2a2e1-f45f-4091-9918-b92aeb1a0ca5
📒 Files selected for processing (2)
src/chain/store/index.rssrc/rpc/methods/chain.rs
hanabi1224
reviewed
May 6, 2026
hanabi1224
reviewed
May 6, 2026
sudo-shashank
force-pushed
the
shashank/refactor-tipset-lookup
branch
2 times, most recently
from
3f8d1dd to
1cf95d2
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/chain/store/index.rs (1)
123-125:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't downgrade missing ancestors to a clean miss.
from.chain(&self.db).tuple_windows()can still stop the walk when a parent tipset can't be loaded, and the newOk(None)/NotFoundpath plus test now treat that as an ordinary miss. That reintroduces the ambiguity this PR is trying to remove: broken ancestor links or blockstore gaps should fast-fail asErr(...), not be surfaced as "tipset not found".
tipset_by_heightneeds an explicit parent-loading loop that propagates ancestor load failures immediately, and this test should assert that error instead ofNone.Also applies to: 212-230, 233-241, 370-386
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chain/store/index.rs` around lines 123 - 125, The current tipset_by_height implementation uses from.chain(&self.db).tuple_windows() which can stop when a parent tipset fails to load and then returns Ok(None)/NotFound; instead change tipset_by_height (in src/chain/store/index.rs) to perform an explicit parent-loading loop that attempts to load each parent tipset via the BlockStore/db and immediately propagate load failures as Err(...) (rather than treating them as a clean miss), ensuring any missing ancestor or blockstore gap returns an error; update analogous logic in the other affected blocks (around the ranges you noted, e.g., the other tipset-by-height-like handlers at ~212-230, 233-241, 370-386) to use the same parent-loading-and-propagate pattern and update the related test to expect an error when a parent cannot be loaded instead of Ok(None).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/chain/store/index.rs`:
- Around line 123-125: The current tipset_by_height implementation uses
from.chain(&self.db).tuple_windows() which can stop when a parent tipset fails
to load and then returns Ok(None)/NotFound; instead change tipset_by_height (in
src/chain/store/index.rs) to perform an explicit parent-loading loop that
attempts to load each parent tipset via the BlockStore/db and immediately
propagate load failures as Err(...) (rather than treating them as a clean miss),
ensuring any missing ancestor or blockstore gap returns an error; update
analogous logic in the other affected blocks (around the ranges you noted, e.g.,
the other tipset-by-height-like handlers at ~212-230, 233-241, 370-386) to use
the same parent-loading-and-propagate pattern and update the related test to
expect an error when a parent cannot be loaded instead of Ok(None).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 20b8280d-6706-4045-9833-bf22d55764ab
📒 Files selected for processing (8)
src/chain/store/index.rssrc/dev/subcommands/export_state_tree_cmd.rssrc/dev/subcommands/state_cmd.rssrc/interpreter/fvm3.rssrc/interpreter/fvm4.rssrc/rpc/methods/f3.rssrc/rpc/methods/state.rssrc/tool/subcommands/index_cmd.rs
sudo-shashank
force-pushed
the
shashank/refactor-tipset-lookup
branch
from
1cf95d2 to
9e202ff
Compare
Contributor
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/rpc/methods/chain.rs (1)
1241-1256:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't swallow
tipset_by_heighterrors in EC finality resolution.The
if let Ok(Some(ts)) = ...check still collapses genuine lookup failures into the fallback path. That defeats this PR'sOk(None)vsErr(_)contract and can return a stale EC-finalized tipset when the preferred lookup actually hit a storage/IO error.Proposed fix
- let finalized = if depth >= 0 - && let Ok(Some(ts)) = ctx.chain_index().tipset_by_height( - (head.epoch() - depth).max(0), - head.shallow_clone(), - ResolveNullTipset::TakeOlder, - ) { - Some(ts) + let finalized = if depth >= 0 { + Some(ctx.chain_index().load_required_tipset_by_height( + (head.epoch() - depth).max(0), + head.shallow_clone(), + ResolveNullTipset::TakeOlder, + )?) } else { let ec_finality_epoch = (head.epoch() - ctx.chain_config().policy.chain_finality).max(0); ctx.chain_index().tipset_by_height( ec_finality_epoch, head, ResolveNullTipset::TakeOlder, )? };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/methods/chain.rs` around lines 1241 - 1256, The current `if let Ok(Some(ts)) = ctx.chain_index().tipset_by_height(...)` hides genuine errors by treating Err(_) the same as Ok(None), causing storage/IO failures to fall through to the EC-finality fallback; change that logic to match on the first `tipset_by_height` call (the one using `(head.epoch() - depth).max(0)` and `ResolveNullTipset::TakeOlder`) and: return Err on Err(e) (propagate the error), use Some(ts) on Ok(Some(ts)), and only use the EC-finality fallback when you get Ok(None); refer to `ctx.chain_index().tipset_by_height`, `ResolveNullTipset::TakeOlder`, `ec_finality_epoch`, and the `finalized` assignment when making this change.
🧹 Nitpick comments (3)
src/dev/subcommands/state_cmd.rs (1)
95-100: ⚡ Quick winAdd context to
load_child_tipsetfailures too.The new
with_context(...)only decorates theNonecase after?unwraps theResult. A real storage/IO failure fromload_child_tipsetstill bubbles up without the epoch-specific context that this command needs.Proposed fix
- let ts_next = chain_store.load_child_tipset(&ts)?.with_context(|| { + let ts_next = chain_store + .load_child_tipset(&ts) + .with_context(|| format!("failed to load child tipset for epoch {}", ts.epoch()))? + .with_context(|| { format!( "no child tipset for epoch {} (may be chain head)", ts.epoch() ) - })?; + })?;As per coding guidelines
**/*.rs: Useanyhow::Result<T>for most operations and add context with.context()when errors occur.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dev/subcommands/state_cmd.rs` around lines 95 - 100, The call to chain_store.load_child_tipset currently only wraps the None case with with_context after the Result is unwrapped, so actual IO/storage errors lack epoch-specific context; change the expression to add context on the Result itself (use .context(...) from anyhow) before the ? so any Err from chain_store.load_child_tipset includes the formatted message referencing ts.epoch(), i.e., call chain_store.load_child_tipset(&ts).context(format!(...))? and assign to ts_next as before to ensure both Err and None cases carry the epoch context.src/state_manager/mod.rs (2)
259-263: ⚡ Quick winAdd rewind-specific context to the required height lookup.
This now surfaces genuine lookup failures, but the bare
?drops the actor-bundle rewind context that explains which target height failed.validate_rangebelow already uses the stronger pattern.Suggested change
- let target_head = self.chain_index().load_required_tipset_by_height( - (expected_height_info.epoch - 1).max(0), - head, - ResolveNullTipset::TakeOlder, - )?; + let rewind_epoch = (expected_height_info.epoch - 1).max(0); + let target_head = self + .chain_index() + .load_required_tipset_by_height( + rewind_epoch, + head, + ResolveNullTipset::TakeOlder, + ) + .with_context(|| { + format!( + "failed to resolve rewind target at height {rewind_epoch} from head height {current_epoch}" + ) + })?;As per coding guidelines, "Use anyhow::Result for most operations and add context with
.context()when errors occur".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/state_manager/mod.rs` around lines 259 - 263, The call to chain_index().load_required_tipset_by_height that assigns target_head can fail but currently uses `?`, losing rewind-specific context; update the call in the target_head assignment (the load_required_tipset_by_height invocation where expected_height_info is used) to append .context(...) before the ? to include a clear rewind-specific message (mentioning the target epoch/height from expected_height_info and why the rewind failed), mirroring the stronger pattern used by validate_range so errors surface with the desired actor-bundle rewind context.
458-459: ⚡ Quick winWrap
load_child_tipsetfailures with tipset context before propagating them.These calls now correctly preserve
Err(_), but the raw error will not say which tipset was being resolved or whether it came fromload_tipset_statevsload_executed_tipset. Since this PR is making those failures user-visible, addingwith_context(...)here will make fast-fail diagnostics much clearer.Suggested change
- match self.chain_store().load_child_tipset(ts)? { + match self + .chain_store() + .load_child_tipset(ts) + .with_context(|| { + format!( + "failed to load child tipset while loading tipset state for {}@{}", + ts.key(), + ts.epoch() + ) + })? { Some(receipt_ts) => Ok(TipsetState { state_root: *receipt_ts.parent_state(), receipt_root: *receipt_ts.parent_message_receipts(), }), None => Ok(self.load_executed_tipset(ts).await?.into()), @@ - let receipt_ts = self.chain_store().load_child_tipset(ts)?; + let receipt_ts = self + .chain_store() + .load_child_tipset(ts) + .with_context(|| { + format!( + "failed to load child tipset while loading executed tipset for {}@{}", + ts.key(), + ts.epoch() + ) + })?;As per coding guidelines, "Use anyhow::Result for most operations and add context with
.context()when errors occur".Also applies to: 485-486
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/state_manager/mod.rs` around lines 458 - 459, Wrap failures from chain_store().load_child_tipset(ts) (and the similar call around load_executed_tipset/load_tipset_state) with contextual information before returning so the propagated anyhow::Error includes which tipset and which resolution path failed; specifically, call .context(...) (or with_context(...)) on the Result returned by load_child_tipset(ts) and the related calls and include the tipset identifier (ts) and the operation name (e.g., "resolving child tipset for", "loading executed tipset", or "loading tipset state") in the message to make diagnostics explicit (refer to load_child_tipset, TipsetState, load_executed_tipset, load_tipset_state).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tool/subcommands/archive_cmd.rs`:
- Around line 587-590: Reject diff values that are greater than or equal to the
target tipset epoch before attempting to resolve the diff tipset: add a check
just before the call to index.load_required_tipset_by_height(...) that compares
diff with ts.epoch() (or the target epoch variable) and return an error (using
.context("diff epoch must be smaller than target epoch")? or equivalent) if diff
>= ts.epoch(); this prevents relying on ResolveNullTipset::TakeOlder to silently
return the anchor tipset and ensures the validation message is surfaced.
---
Outside diff comments:
In `@src/rpc/methods/chain.rs`:
- Around line 1241-1256: The current `if let Ok(Some(ts)) =
ctx.chain_index().tipset_by_height(...)` hides genuine errors by treating Err(_)
the same as Ok(None), causing storage/IO failures to fall through to the
EC-finality fallback; change that logic to match on the first `tipset_by_height`
call (the one using `(head.epoch() - depth).max(0)` and
`ResolveNullTipset::TakeOlder`) and: return Err on Err(e) (propagate the error),
use Some(ts) on Ok(Some(ts)), and only use the EC-finality fallback when you get
Ok(None); refer to `ctx.chain_index().tipset_by_height`,
`ResolveNullTipset::TakeOlder`, `ec_finality_epoch`, and the `finalized`
assignment when making this change.
---
Nitpick comments:
In `@src/dev/subcommands/state_cmd.rs`:
- Around line 95-100: The call to chain_store.load_child_tipset currently only
wraps the None case with with_context after the Result is unwrapped, so actual
IO/storage errors lack epoch-specific context; change the expression to add
context on the Result itself (use .context(...) from anyhow) before the ? so any
Err from chain_store.load_child_tipset includes the formatted message
referencing ts.epoch(), i.e., call
chain_store.load_child_tipset(&ts).context(format!(...))? and assign to ts_next
as before to ensure both Err and None cases carry the epoch context.
In `@src/state_manager/mod.rs`:
- Around line 259-263: The call to chain_index().load_required_tipset_by_height
that assigns target_head can fail but currently uses `?`, losing rewind-specific
context; update the call in the target_head assignment (the
load_required_tipset_by_height invocation where expected_height_info is used) to
append .context(...) before the ? to include a clear rewind-specific message
(mentioning the target epoch/height from expected_height_info and why the rewind
failed), mirroring the stronger pattern used by validate_range so errors surface
with the desired actor-bundle rewind context.
- Around line 458-459: Wrap failures from chain_store().load_child_tipset(ts)
(and the similar call around load_executed_tipset/load_tipset_state) with
contextual information before returning so the propagated anyhow::Error includes
which tipset and which resolution path failed; specifically, call .context(...)
(or with_context(...)) on the Result returned by load_child_tipset(ts) and the
related calls and include the tipset identifier (ts) and the operation name
(e.g., "resolving child tipset for", "loading executed tipset", or "loading
tipset state") in the message to make diagnostics explicit (refer to
load_child_tipset, TipsetState, load_executed_tipset, load_tipset_state).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 397d1550-fa58-492c-bcef-355e148f5533
📒 Files selected for processing (18)
src/chain/store/index.rssrc/dev/subcommands/export_state_tree_cmd.rssrc/dev/subcommands/state_cmd.rssrc/interpreter/fvm3.rssrc/interpreter/fvm4.rssrc/message_pool/msgpool/provider.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/chain_rand.rssrc/state_manager/mod.rssrc/tool/subcommands/archive_cmd.rssrc/tool/subcommands/benchmark_cmd.rssrc/tool/subcommands/index_cmd.rssrc/tool/subcommands/snapshot_cmd.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/rpc/methods/f3.rs
- src/rpc/methods/eth.rs
- src/rpc/methods/eth/tipset_resolver.rs
- src/chain/store/index.rs
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/chain/store/index.rs (1)
184-186:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't ignore checkpoint tipset load failures.
A hit in
ts_height_cacheshould not silently fall back to a slower scan whenload_tipsetreturnsErrorOk(None). That turns blockstore corruption/IO failures into a fake cache miss and can later surface as a misleadingNone/NotFoundfrom this lookup path. Use a required load here and propagate the failure instead of skipping it.Suggested fix
- if let Some(checkpoint_from_key) = - self.ts_height_cache.get_cloned(&checkpoint_from_epoch) - && let Ok(Some(checkpoint_from)) = self.load_tipset(&checkpoint_from_key) - { - from = checkpoint_from; - break; - } + if let Some(checkpoint_from_key) = + self.ts_height_cache.get_cloned(&checkpoint_from_epoch) + { + from = self.load_required_tipset(&checkpoint_from_key)?; + break; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chain/store/index.rs` around lines 184 - 186, The current conditional treats an Err/Ok(None) from load_tipset as a cache miss; instead make loading the checkpoint required and propagate failures: after finding checkpoint_from_key in ts_height_cache, call self.load_tipset(&checkpoint_from_key) and if it returns Err or Ok(None) return/propagate that error (don’t fall through to the slow scan), otherwise use the loaded checkpoint_from; update the logic around checkpoint_from_epoch, checkpoint_from_key and load_tipset to ensure load failures are surfaced rather than ignored.src/rpc/methods/chain.rs (1)
1242-1255:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not suppress lookup errors in EC finalized tipset resolution.
At Line 1242,
let Ok(Some(ts)) = ...turnsErr(...)into a silent fallback path. That reintroduces miss/error conflation and can hide real blockstore/IO failures instead of fast-failing.Suggested fix
- let finalized = if depth >= 0 - && let Ok(Some(ts)) = ctx.chain_index().tipset_by_height( - (head.epoch() - depth).max(0), - head.shallow_clone(), - ResolveNullTipset::TakeOlder, - ) { - Some(ts) - } else { - let ec_finality_epoch = - (head.epoch() - ctx.chain_config().policy.chain_finality).max(0); - ctx.chain_index().tipset_by_height( - ec_finality_epoch, - head, - ResolveNullTipset::TakeOlder, - )? - }; + let finalized = if depth >= 0 { + match ctx.chain_index().tipset_by_height( + (head.epoch() - depth).max(0), + head.shallow_clone(), + ResolveNullTipset::TakeOlder, + )? { + Some(ts) => Some(ts), + None => { + let ec_finality_epoch = + (head.epoch() - ctx.chain_config().policy.chain_finality).max(0); + ctx.chain_index().tipset_by_height( + ec_finality_epoch, + head, + ResolveNullTipset::TakeOlder, + )? + } + } + } else { + let ec_finality_epoch = + (head.epoch() - ctx.chain_config().policy.chain_finality).max(0); + ctx.chain_index().tipset_by_height( + ec_finality_epoch, + head, + ResolveNullTipset::TakeOlder, + )? + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/methods/chain.rs` around lines 1242 - 1255, The current code swallows errors by using `let Ok(Some(ts)) = ctx.chain_index().tipset_by_height(...)`, converting any `Err` into the fallback path; instead call `ctx.chain_index().tipset_by_height(...)` and propagate errors with `?`, then branch on the Option result: if `Some(ts)` use it, otherwise compute `ec_finality_epoch` and call `ctx.chain_index().tipset_by_height(ec_finality_epoch, head, ResolveNullTipset::TakeOlder)?` to also propagate errors; reference `tipset_by_height`, `ctx.chain_index()`, `ResolveNullTipset::TakeOlder`, `ec_finality_epoch`, `head`, and `depth`.
♻️ Duplicate comments (1)
src/chain/store/index.rs (1)
212-230:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
tuple_windows()still downgrades missing parents intoOk(None).
from.chain(&self.db)stops when an ancestor can't be loaded, so this loop can end on broken ancestry and returnOk(None)instead of the underlying storage/load error. That reintroduces the ambiguity this PR is meant to remove, and it also makesload_required_tipset_by_heightreportNotFoundfor corruption/pruned-parent failures. This needs an explicit parent-loading walk that propagates load errors immediately;Noneshould stay reserved for a proven logical miss.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chain/store/index.rs` around lines 212 - 230, The loop using from.chain(&self.db).tuple_windows() can silently stop on a missing/broken parent and return Ok(None), masking storage/load errors; change the logic in load_required_tipset_by_height to perform an explicit parent-loading walk (iteratively load parent tipset via db loads) instead of relying on tuple_windows so you can immediately propagate any Err from the DB/load routines; ensure the code paths that inspect epochs (child.epoch(), parent.epoch()) still update ts_height_cache when is_checkpoint/is_finalized, handle ResolveNullTipset (TakeOlder/TakeNewer) the same way, and only return Ok(None) for a proven logical miss, never when a storage load returned an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/chain/store/index.rs`:
- Around line 184-186: The current conditional treats an Err/Ok(None) from
load_tipset as a cache miss; instead make loading the checkpoint required and
propagate failures: after finding checkpoint_from_key in ts_height_cache, call
self.load_tipset(&checkpoint_from_key) and if it returns Err or Ok(None)
return/propagate that error (don’t fall through to the slow scan), otherwise use
the loaded checkpoint_from; update the logic around checkpoint_from_epoch,
checkpoint_from_key and load_tipset to ensure load failures are surfaced rather
than ignored.
In `@src/rpc/methods/chain.rs`:
- Around line 1242-1255: The current code swallows errors by using `let
Ok(Some(ts)) = ctx.chain_index().tipset_by_height(...)`, converting any `Err`
into the fallback path; instead call `ctx.chain_index().tipset_by_height(...)`
and propagate errors with `?`, then branch on the Option result: if `Some(ts)`
use it, otherwise compute `ec_finality_epoch` and call
`ctx.chain_index().tipset_by_height(ec_finality_epoch, head,
ResolveNullTipset::TakeOlder)?` to also propagate errors; reference
`tipset_by_height`, `ctx.chain_index()`, `ResolveNullTipset::TakeOlder`,
`ec_finality_epoch`, `head`, and `depth`.
---
Duplicate comments:
In `@src/chain/store/index.rs`:
- Around line 212-230: The loop using from.chain(&self.db).tuple_windows() can
silently stop on a missing/broken parent and return Ok(None), masking
storage/load errors; change the logic in load_required_tipset_by_height to
perform an explicit parent-loading walk (iteratively load parent tipset via db
loads) instead of relying on tuple_windows so you can immediately propagate any
Err from the DB/load routines; ensure the code paths that inspect epochs
(child.epoch(), parent.epoch()) still update ts_height_cache when
is_checkpoint/is_finalized, handle ResolveNullTipset (TakeOlder/TakeNewer) the
same way, and only return Ok(None) for a proven logical miss, never when a
storage load returned an error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c8754371-c75c-4648-ab80-5346fd6bc616
📒 Files selected for processing (19)
src/chain/store/chain_store.rssrc/chain/store/index.rssrc/dev/subcommands/export_state_tree_cmd.rssrc/dev/subcommands/state_cmd.rssrc/interpreter/fvm3.rssrc/interpreter/fvm4.rssrc/message_pool/msgpool/provider.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/chain_rand.rssrc/state_manager/mod.rssrc/tool/subcommands/archive_cmd.rssrc/tool/subcommands/benchmark_cmd.rssrc/tool/subcommands/index_cmd.rssrc/tool/subcommands/snapshot_cmd.rs
✅ Files skipped from review due to trivial changes (1)
- src/dev/subcommands/export_state_tree_cmd.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/interpreter/fvm3.rs
- src/rpc/methods/eth/filter/mod.rs
- src/tool/subcommands/archive_cmd.rs
- src/state_manager/mod.rs
hanabi1224
approved these changes
May 7, 2026
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
2 participants
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
Changes introduced in this pull request:
tipset_by_heightandload_child_tipsetnow returnResult<Option<Tipset>, Error>(Ok(None) = clean cache miss, Err = actual storage/IO failure)Reference issue to close (if applicable)
Closes #6755
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Bug Fixes
Improvements
API Changes