Refactor tipset_by_height and load_child_tipset to distinguish error from miss#7005
Open
sudo-shashank wants to merge 3 commits intomainfrom
Open
Refactor tipset_by_height and load_child_tipset to distinguish error from miss#7005sudo-shashank wants to merge 3 commits intomainfrom
sudo-shashank wants to merge 3 commits intomainfrom
Conversation
Contributor
WalkthroughRefactors tipset lookup APIs so 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 docstrings
🧪 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
| ResolveNullTipset::TakeNewer => return Ok(Some(child)), | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
It should return None at line 330
hanabi1224
reviewed
May 6, 2026
| ) | ||
| .context("Failed to get tipset cid")?; | ||
| )? | ||
| .with_context(|| format!("tipset not found at epoch {epoch}"))?; |
Contributor
There was a problem hiding this comment.
We could use load_required_xx to avoid repetitive with_context(|| format!("tipset not found at epoch {epoch}"))?;
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