fix: make tipset lookup-by-epoch table resilient on chain reorg#7288
Conversation
|
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:
WalkthroughThis PR switches ChainIndex and ChainStore finality checks from tipset-based predicates to epoch-based predicates, persists the heaviest tipset key at lookup checkpoints, and changes daemon warmup to collect events and repopulate lookup entries from the chain. ChangesEpoch finality and chain store updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/chain_store.rs`:
- Around line 183-185: The write order in `ChainStore::set_heaviest_tipset`
should be changed so `set_tipset_key_at_epoch(&head)` happens before
`set_heaviest_tipset_key(head.key())`. Update the sequence around
`head.key().save(self.db())`, `db().set_tipset_key_at_epoch(&head)`, and
`db().set_heaviest_tipset_key(...)` so a failure in the epoch mapping does not
leave the durable head advanced ahead of the in-memory state and notifications.
🪄 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: a6c91500-0699-4278-bf7b-bd9e7c325f56
📒 Files selected for processing (2)
src/chain/store/chain_store.rssrc/chain/store/index.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
hanabi1224
force-pushed
the
hm/correct-ts-lookup-on-reorg
branch
from
4f9e0a3 to
7bcb805
Compare
hanabi1224
force-pushed
the
hm/correct-ts-lookup-on-reorg
branch
from
7bcb805 to
ddb71f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 184-189: The checkpoint lookup in `tipset_by_height` is trusting
`is_epoch_finalized` too early, which can let a stale forked tipset key be
reused or overwrite the cache. Update the logic around `checkpoint_from_epoch`
and the `set_heaviest_tipset`-written table entries so only finalized canonical
chain tipsets are accepted, or verify any loaded checkpoint is an ancestor of
the original `from` before replacing it. Apply the same fix to the corresponding
checkpoint update path in the nearby code that shares this logic.
🪄 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: 908d7131-0e93-47ab-a41b-5ca46f7bb486
📒 Files selected for processing (3)
src/chain/store/chain_store.rssrc/chain/store/index.rssrc/daemon/mod.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
💤 Files with no reviewable changes (1)
- src/daemon/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/chain/store/chain_store.rs
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
@hanabi1224 Can you summarize the root cause? |
hanabi1224
force-pushed
the
hm/correct-ts-lookup-on-reorg
branch
from
940e4af to
fe1916b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/daemon/mod.rs (1)
845-848: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist
cs.ec_calculator_finalized_epoch()out of the filter closure. It’s invariant here, so compute it once before the loop.🤖 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/daemon/mod.rs` around lines 845 - 848, Hoist the invariant finalized-epoch lookup out of the filter closure in the tipset iteration loop. In the `head.chain(cs.db())` loop, compute `cs.ec_calculator_finalized_epoch()` once before the `for ts` statement in `src/daemon/mod.rs`, then reuse that local value inside the `filter` predicate alongside `ChainIndex::is_tipset_lookup_checkpoint(ts.epoch())`.
🤖 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.
Nitpick comments:
In `@src/daemon/mod.rs`:
- Around line 845-848: Hoist the invariant finalized-epoch lookup out of the
filter closure in the tipset iteration loop. In the `head.chain(cs.db())` loop,
compute `cs.ec_calculator_finalized_epoch()` once before the `for ts` statement
in `src/daemon/mod.rs`, then reuse that local value inside the `filter`
predicate alongside `ChainIndex::is_tipset_lookup_checkpoint(ts.epoch())`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bbd82c1d-382c-422d-ae33-b2c00c5f4a8b
📒 Files selected for processing (1)
src/daemon/mod.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
hanabi1224
force-pushed
the
hm/correct-ts-lookup-on-reorg
branch
from
fe1916b to
712eabb
Compare
hanabi1224
force-pushed
the
hm/correct-ts-lookup-on-reorg
branch
from
1de1065 to
dc2528f
Compare
hanabi1224
force-pushed
the
hm/correct-ts-lookup-on-reorg
branch
from
dc2528f to
dff772b
Compare
|
Hm, stateful tests are failing. Is it related to the change? |
@LesnyRumcajs I don't think so, it passed before I made the log level change. Retrying |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
2 participants
Summary of changes
A Forest node got a wrong tipset lookup record at epoch 6155260 (likely from chain reorg) which caused state mismatch in validating tipset@6155285. This PR makes the tipset lookup population logic more robust in cases like chain reorg.
State mistach is gone after reboot with the fix:
Root cause of having the bad record is not fully clear, I don't see bad records on other long running nodes
Sample output:
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit