Fix resolve_to_key in message pool to be reorg stable #6910
Open
sudo-shashank wants to merge 11 commits intomainfrom
Open
Fix resolve_to_key in message pool to be reorg stable #6910sudo-shashank wants to merge 11 commits intomainfrom
sudo-shashank wants to merge 11 commits intomainfrom
Conversation
Contributor
|
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
Comment |
sudo-shashank
requested review from
LesnyRumcajs and
hanabi1224
and removed request for
a team
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
hanabi1224
reviewed
Apr 20, 2026
|
|
||
| let api = TestApi::default(); | ||
| let bls_sig_cache = SizeTrackingLruCache::new_mocked(); | ||
| let key_cache = SizeTrackingLruCache::new_mocked(); |
Contributor
There was a problem hiding this comment.
I added a similar cache to state manager in #6928 we should consolidate them. @coderabbitai create a tracking issue.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
Let me first look at PR #6928 to understand the cache that was added there.
🏁 Script executed:
#!/bin/bash
gh pr view 6928 --repo ChainSafe/forest --json title,body,url,state | jq .
Length of output: 75
I have enough context to create the tracking issue. Let me do that now.
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo ChainSafe/forest \
--title "Consolidate finality-lookback resolve caches in mpool and state manager" \
--body "## Overview
PR `#6910` introduces a finality-lookback cache in \`resolve_to_key\` inside \`src/message_pool/msgpool/provider.rs\` (message pool) to make ID→key address resolution reorg-stable.
PR `#6928` adds a similar cache to the state manager.
These two caches serve related purposes and should be consolidated into a shared abstraction to avoid duplication and keep the logic in sync.
## Tasks
- [ ] Review the cache implementations in `#6910` (\`src/message_pool/msgpool/provider.rs\`) and `#6928` (state manager)
- [ ] Identify the common interface / shared logic
- [ ] Extract a shared utility or trait that both the message pool and state manager can use
- [ ] Update both call sites to use the consolidated implementation
- [ ] Add/update tests as appropriate
## References
- PR `#6910`: https://github.com/ChainSafe/forest/pull/6910
- PR `#6928`: https://github.com/ChainSafe/forest/pull/6928
- Requested by `@hanabi1224` in https://github.com/ChainSafe/forest/pull/6910#discussion_r3109534232" \
--assignee hanabi1224
Length of output: 75
@hanabi1224, I'm unable to create the GitHub issue directly from this environment due to a missing authentication token. Here are the full details so you (or anyone on the team) can open it manually:
Title: Consolidate finality-lookback resolve caches in mpool and state manager
Body:
## Overview
PR `#6910` introduces a finality-lookback cache in `resolve_to_key` inside
`src/message_pool/msgpool/provider.rs` (message pool) to make ID→key address
resolution reorg-stable.
PR `#6928` adds a similar cache to the state manager.
These two caches serve related purposes and should be consolidated into a shared
abstraction to avoid duplication and keep the logic in sync.
## Tasks
- [ ] Review the cache implementations in `#6910` (`src/message_pool/msgpool/provider.rs`)
and `#6928` (state manager)
- [ ] Identify the common interface / shared logic
- [ ] Extract a shared utility or trait that both the message pool and state manager can use
- [ ] Update both call sites to use the consolidated implementation
- [ ] Add/update tests as appropriate
## References
- PR `#6910`: https://github.com/ChainSafe/forest/pull/6910
- PR `#6928`: https://github.com/ChainSafe/forest/pull/6928
- Requested by `@hanabi1224` in https://github.com/ChainSafe/forest/pull/6910#discussion_r3109534232
Assignee: hanabi1224
Apologies for not being able to create it directly — please let me know if you'd like me to retry or adjust any details.
🧠 Learnings used
Learnt from: sudo-shashank
Repo: ChainSafe/forest PR: 6788
File: src/message_pool/msgpool/msg_pool.rs:286-300
Timestamp: 2026-03-31T15:27:58.502Z
Learning: In Forest's `src/message_pool/msgpool/msg_pool.rs`, the `key_cache: SizeTrackingLruCache<Address, Address>` (used in `resolve_to_key`) is intentionally keyed only by `Address` with no tipset component. This directly matches Lotus's `keyCache *lru.Cache[address.Address, address.Address]` in `chain/messagepool/messagepool.go`. ID→key address mappings are stable across reorgs because they are derived from actor creation (content-addressed), so a tipset-aware cache key or cache flush on head changes is not needed. Do not flag this as a reorg-safety bug.
Learnt from: sudo-shashank
Repo: ChainSafe/forest PR: 6788
File: src/rpc/methods/mpool.rs:301-306
Timestamp: 2026-03-25T09:06:55.615Z
Learning: In Forest's `MpoolGetNonce` handler (src/rpc/methods/mpool.rs), returning bare `mpool.get_sequence()` without going through `NonceTracker` is intentional and matches Lotus behavior. Lotus documents `MpoolGetNonce` as explicitly non-atomic and recommends using `MpoolPushMessage` instead. Do not flag the discrepancy between `MpoolGetNonce` (mpool.get_sequence) and `MpoolPushMessage` (NonceTracker.sign_and_push) as a bug.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6761
File: src/state_manager/mod.rs:496-511
Timestamp: 2026-03-18T14:19:21.819Z
Learning: In `src/state_manager/mod.rs` (Forest, ChainSafe/forest), the process-global `static CACHE: LazyLock<SizeTrackingLruCache<TipsetKey, ExecutedTipset>>` inside `load_executed_tipset` is intentionally shared across all `StateManager` instances. This is safe because `TipsetKey` values are CID-based (content-addressed, cryptographic hashes of block headers), so the same key always maps to the same deterministic `ExecutedTipset` result regardless of which `StateManager` or `ChainStore` instance processes it. Do not flag this as a bug in code review.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5978
File: .github/workflows/unit-tests.yml:72-76
Timestamp: 2025-08-25T09:57:27.412Z
Learning: hanabi1224's strategy for RPC snapshot cache versioning: branch isolation is not needed currently, but if snapshot generation logic changes in the future, a version string can be added to the cache key to handle the evolution.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5944
File: src/chain/store/index.rs:0-0
Timestamp: 2025-08-18T03:09:47.932Z
Learning: In Forest's tipset_by_height caching implementation, hanabi1224 prefers performance-conscious solutions that leverage finality guarantees rather than expensive chain walking for fork detection. The approach of constraining cache lookups to finalized epochs (using CHECKPOINT_INTERVAL >= CHAIN_FINALITY) provides fork safety without the performance cost of ancestry verification.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5978
File: .github/workflows/unit-tests.yml:72-76
Timestamp: 2025-08-25T09:53:37.443Z
Learning: hanabi1224 intentionally designs RPC snapshot test cache to be OS/arch-agnostic, sharing cached snapshot files across different platforms using enableCrossOsArchive: true and hash-based cache keys to improve cache efficiency and reduce redundant downloads.
Learnt from: sudo-shashank
Repo: ChainSafe/forest PR: 6788
File: src/message_pool/msgpool/mod.rs:0-0
Timestamp: 2026-03-31T15:28:12.127Z
Learning: In Forest's `head_change` function (`src/message_pool/msgpool/mod.rs`), reverted messages are stored in `rmsgs` keyed by the resolved key address (via `resolve_to_key`), and when re-adding them to the pending pool after a reorg, `msg.from()` is re-resolved against the new head tipset. This mirrors Lotus's `HeadChange` implementation in `chain/messagepool/messagepool.go`, which also uses the raw `m.Message.From` field (unresolved) throughout the revert and re-add path. Do not flag the re-resolution of `msg.from()` at re-add time as incorrect; this is the accepted design matching Lotus behavior.
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 6000
File: src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs:4-9
Timestamp: 2025-09-11T16:03:14.328Z
Learning: In the Forest codebase's state_decode_params_tests module, common imports like `to_vec`, `TokenAmount`, `Cid`, etc. are centralized in the mod.rs file and made available to submodules through `use super::*;`, eliminating the need for duplicate imports in individual test files.
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 6000
File: src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs:4-9
Timestamp: 2025-09-11T16:03:14.328Z
Learning: In the Forest codebase's state_decode_params_tests module, common imports like `to_vec`, `TokenAmount`, `Cid`, etc. are centralized in the mod.rs file and made available to submodules through `use super::*;`, eliminating the need for duplicate imports in individual test files.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6712
File: src/chain/store/chain_store.rs:172-174
Timestamp: 2026-03-10T13:22:53.227Z
Learning: In `src/chain/store/chain_store.rs` (Forest, ChainSafe/forest PR `#6712`), `get_required_tipset_key` was changed to use `TipsetKey::load(self.blockstore(), &hash.to_cid())` instead of reading from `eth_mappings`. This is backward compatible because the old `put_tipset_key` already persisted tipset key bytes to the blockstore via `put_cbor_default`, with the EthHash derived from that CID (`hash = tsk_cid.into()`). Since `EthHash::from(cid)` and `hash.to_cid()` are mutual inverses, existing databases can serve old tipset-key lookups without any backfill.
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: sudo-shashank
Repo: ChainSafe/forest PR: 6710
File: src/wallet/subcommands/wallet_cmd.rs:641-649
Timestamp: 2026-03-11T10:10:29.857Z
Learning: In `src/wallet/subcommands/wallet_cmd.rs` (ChainSafe/forest, PR `#6710`), `resolve_target_address` intentionally returns `is_0x_recipient = false` for Filecoin-form f410f addresses (even though they resolve to Delegated/ETH-mapped addresses) and `true` only for `0x`-prefixed hex input. This is a deliberate choice to mirror Lotus CLI behavior, where method selection (InvokeContract vs METHOD_SEND) is driven by whether the user typed a `0x` address, not the resolved address protocol. The test `test_resolve_method_num_send_to_delegated` (f1 → f410f → METHOD_SEND) reflects this intentional design. Do not flag the `false` return for f410f-form Filecoin addresses as a bug.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6780
File: src/chain/store/index.rs:188-200
Timestamp: 2026-03-25T09:23:15.859Z
Learning: In Forest's `src/chain/store/index.rs`, the `ts_height_cache` write guard uses a global `is_finalized_fn` (comparing `ts.epoch() <= heaviest_tipset.epoch() - CHAIN_FINALITY`, optionally stricter with F3 finalized epoch) rather than a per-call `from_epoch - CHAIN_FINALITY` bound. This is correct and fork-safe: EC finality guarantees the chain cannot reorg past `heaviest - CHAIN_FINALITY`, so any tipset at or below that threshold is necessarily on the canonical chain regardless of which `from` tipset started the walk. Do not flag this as a fork-safety bug.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6780
File: src/chain/store/chain_store.rs:181-183
Timestamp: 2026-03-25T09:33:03.318Z
Learning: In Forest (ChainSafe/forest, src/chain/store/chain_store.rs), `set_f3_finalized_tipset` unconditionally replaces the cached F3 finalized tipset without a monotonicity guard. This is intentional: even if a delayed startup fetch races with a later `F3.Finalize` call and temporarily overwrites a newer epoch with an older one, eventual correctness is not broken. Do not suggest adding an epoch-monotonicity guard to `set_f3_finalized_tipset`.
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 6685
File: src/rpc/methods/eth.rs:3540-3549
Timestamp: 2026-03-10T13:30:38.793Z
Learning: In Forest's `debug_trace_transaction` (src/rpc/methods/eth.rs), the prestate tracer resolves top-level sender/recipient into `touched` via `EthAddress::from_filecoin_address`. This is intentional and sufficient: `debug_traceTransaction` is an Ethereum-namespace API whose callers only ever supply ETH-format transaction hashes, which always correspond to delegated f410f senders. Native Filecoin f1/f3 messages produce CID-derived hashes that are never surfaced by Ethereum tooling, so f1/f3 senders cannot reach this code path in practice. Do not flag the direct `from_filecoin_address` conversion as insufficient for the prestate tracer in this function.
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.
Learnt from: LesnyRumcajs
Repo: ChainSafe/forest PR: 6903
File: src/shim/executor.rs:243-251
Timestamp: 2026-04-14T17:24:13.631Z
Learning: In the ChainSafe/forest (LesnyRumcajs) codebase, do not require rustdoc comments on public methods when the method’s purpose is clearly self-explanatory from its name (e.g., `entries(&self)`, `into_entries(self)`). When reviewing Rust (`.rs`) files, avoid flagging missing documentation for these obviously named public APIs, reserving doc requirements for less clear or non-obvious public methods.
hanabi1224
reviewed
Apr 20, 2026
| let lookback_ts = if ts.epoch() > self.chain_config().policy.chain_finality { | ||
| self.chain_index() | ||
| .tipset_by_height( | ||
| ts.epoch() - self.chain_config().policy.chain_finality, |
Contributor
There was a problem hiding this comment.
What if ts is at a finalized epoch already?
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:
Reference issue to close (if applicable)
Closes #6891
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Bug Fixes
Tests