Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Comment |
src/daemon/mod.rs
Outdated
| let HeadChange::Apply(ts) = receiver.recv().await?; | ||
| // Continuously listen for head changes | ||
| loop { | ||
| let HeadChange::Apply(ts) = receiver.recv().await?; |
There was a problem hiding this comment.
@coderabbitai HeadChange::Revert is missing in Forest. Create an issue to track implementing it. Lotus interface: https://github.com/filecoin-project/lotus/blob/04dd7f2e3611a38f1b9ac374196c691fe3447bae/chain/events/events.go#L26
type TipSetObserver interface {
Apply(ctx context.Context, from, to *types.TipSet) error
Revert(ctx context.Context, from, to *types.TipSet) error
}There was a problem hiding this comment.
🧩 Analysis chain
@hanabi1224, I've created a GitHub issue to track the implementation of HeadChange::Revert in Forest. The issue includes a reference to the Lotus TipSetObserver interface, the current Forest behavior (silently ignoring revert events in the indexer loop), and links back to this PR and comment.
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo ChainSafe/forest \
--title "Implement HeadChange::Revert in Forest" \
--body "## Summary
Forest currently only handles \`HeadChange::Apply\` events in the chain indexer and related services. The \`HeadChange::Revert\` variant is missing, which means chain reorganizations (reorgs) are not properly handled.
## Lotus Reference
The Lotus implementation defines a \`TipSetObserver\` interface with both \`Apply\` and \`Revert\` methods:
\`\`\`go
type TipSetObserver interface {
Apply(ctx context.Context, from, to *types.TipSet) error
Revert(ctx context.Context, from, to *types.TipSet) error
}
\`\`\`
Reference: https://github.com/filecoin-project/lotus/blob/04dd7f2e3611a38f1b9ac374196c691fe3447bae/chain/events/events.go#L26
## Current Forest Behavior
In \`src/daemon/mod.rs\`, the indexer loop only matches \`HeadChange::Apply\`:
\`\`\`rust
loop {
let HeadChange::Apply(ts) = receiver.recv().await?;
// ...
}
\`\`\`
The \`HeadChange::Revert\` case is silently ignored, meaning index state is not rolled back on chain reorgs.
## Expected Behavior
Forest should implement a \`HeadChange::Revert\` handler analogous to the Lotus \`TipSetObserver::Revert\` method, to correctly undo index state when a tipset is reverted during a chain reorganization.
## References
- Raised in PR: https://github.com/ChainSafe/forest/pull/6720
- Comment: https://github.com/ChainSafe/forest/pull/6720#discussion_r2923416164
- Requested by: `@hanabi1224`
" \
--assignee hanabi1224
Repository: ChainSafe/forest
Length of output: 47
@hanabi1224, the issue has been created and assigned to you: #6729
It tracks implementing HeadChange::Revert in Forest to properly handle chain reorganizations, with a reference to the Lotus TipSetObserver interface and a link back to this PR/comment.
🧠 Learnings used
Learnt from: LesnyRumcajs
Repo: ChainSafe/forest PR: 6702
File: src/chain/store/base_fee.rs:58-72
Timestamp: 2026-03-09T09:59:01.183Z
Learning: In `src/chain/store/base_fee.rs` (Forest, ChainSafe/forest), the FIP-0115 activation condition in `compute_base_fee` uses `ts.epoch() >= next_upgrade_height` (where `ts` is the parent tipset), not `ts.epoch() + 1 >= next_upgrade_height`. This is intentional and matches the Lotus reference implementation in `chain/store/basefee.go`.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/cron_state.rs:8-8
Timestamp: 2026-01-05T12:54:40.850Z
Learning: In Rust code reviews, do not derive Eq for a struct if any field does not implement Eq (e.g., types from external dependencies). If a type like CronStateLotusJson includes fields wrapping external dependencies that lack Eq, derive PartialEq (or implement PartialEq manually) but avoid deriving Eq. This ensures comparisons compile and reflect actual equivalence semantics. When needed, consider implementing custom PartialEq (and possibly Eq) only after ensuring all fields (or wrappers) implement Eq, or keep PartialEq-only if full equality semantics cannot be expressed.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/evm_state.rs:41-44
Timestamp: 2026-01-05T12:56:13.802Z
Learning: In Rust codebases (e.g., Forest), do not add #[cfg(test)] to functions already annotated with #[test]. The #[test] attribute ensures the function is compiled only for tests, so a separate #[cfg(test)] is redundant and can be removed if present. Apply this check to all Rust files that contain #[test] functions.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6666
File: src/tool/subcommands/archive_cmd.rs:628-631
Timestamp: 2026-03-02T09:43:34.946Z
Learning: In Rust sources related to Forest snapshot handling, implement the rule: enable message_receipts and events (message_receipts: true, events: true) only for GC snapshots as defined in src/db/gc/snapshot.rs, since these are internal snapshots created during garbage collection. For user-facing export commands such as src/tool/subcommands/archive_cmd.rs, disable receipts and events by default (message_receipts: false, events: false) to keep user-facing snapshots smaller, unless explicitly requested. This guidance targets Rust files; apply consistently across similar snapshot-related modules, using the narrowest applicable scope when extending beyond the two specified files.
80855f0 to
9865f72
Compare
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions