refactor: optimize cache key type for resolve_to_key#6961
Merged
hanabi1224 merged 3 commits intomainfrom Apr 23, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
WalkthroughMessage-pool address-resolution now uses finality-stable deterministic resolution and an ID-keyed cache: cache keys changed from Changes
Sequence Diagram(s)sequenceDiagram
participant MP as MessagePool
participant Cache as KeyCache (AddressId -> Address)
participant Prov as Provider
participant SM as StateManager / ChainStore
MP->>Cache: lookup(addr.id() -> AddressId)
alt cache hit
Cache-->>MP: cached Address
else cache miss
MP->>Prov: resolve_to_deterministic_address_at_finality(addr, tipset)
Prov->>SM: compute lookback state & resolve deterministically
SM-->>Prov: deterministic Address
Prov-->>MP: deterministic Address
MP->>Cache: insert(AddressId, Address)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
hanabi1224
requested review from
LesnyRumcajs and
sudo-shashank
and removed request for
a team
hanabi1224
commented
Apr 23, 2026
hanabi1224
commented
Apr 23, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/message_pool/msgpool/provider.rs (1)
122-132:⚠️ Potential issue | 🟠 MajorReturn an error until a finality lookback tipset exists.
This branch currently resolves against
tswhen the chain is younger thanchain_finality, which contradicts the contract on Lines 51-52 and lets the mpool cache mappings that are not finality-stable yet. Please fail fast here instead of treating the current head as final.Suggested fix
- let lookback_ts = if ts.epoch() > self.chain_config().policy.chain_finality { + 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, ts.clone(), ResolveNullTipset::TakeOlder, ) .map_err(|e| Error::Other(e.to_string()))? } else { - ts.clone() + return Err(Error::Other( + "cannot resolve ID address to a finality-stable deterministic address yet" + .into(), + )); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/message_pool/msgpool/provider.rs` around lines 122 - 132, The branch that sets lookback_ts currently uses ts.clone() when ts.epoch() <= self.chain_config().policy.chain_finality; instead, fail fast by returning an error so we don't cache non-finality-stable mappings: in the function that computes lookback_ts (referencing lookback_ts, self.chain_config().policy.chain_finality, and self.chain_index().tipset_by_height with ResolveNullTipset), replace the else arm with an early Err(...) return (e.g., using Error::Other with a clear message) indicating that a finality lookback tipset does not yet exist rather than using ts.clone().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/message_pool/msgpool/provider.rs`:
- Around line 122-132: The branch that sets lookback_ts currently uses
ts.clone() when ts.epoch() <= self.chain_config().policy.chain_finality;
instead, fail fast by returning an error so we don't cache non-finality-stable
mappings: in the function that computes lookback_ts (referencing lookback_ts,
self.chain_config().policy.chain_finality, and
self.chain_index().tipset_by_height with ResolveNullTipset), replace the else
arm with an early Err(...) return (e.g., using Error::Other with a clear
message) indicating that a finality lookback tipset does not yet exist rather
than using ts.clone().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6d390790-a546-431e-a2da-70b4fa4adcfa
📒 Files selected for processing (6)
src/message_pool/msgpool/mod.rssrc/message_pool/msgpool/msg_pool.rssrc/message_pool/msgpool/provider.rssrc/message_pool/msgpool/selection.rssrc/message_pool/msgpool/test_provider.rssrc/state_manager/mod.rs
hanabi1224
force-pushed
the
hm/cache-key-for-resolve_to_key
branch
from
c498ce7 to
0660f70
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/message_pool/msgpool/provider.rs (1)
122-140: Add operation-specific context to error mapping.At Line 129, Line 137, and Line 140, converting errors with
e.to_string()drops step-level context, which makes runtime diagnosis harder.Suggested patch
- let lookback_ts = if ts.epoch() > self.chain_config().policy.chain_finality { + let finality = self.chain_config().policy.chain_finality; + let lookback_ts = if ts.epoch() > finality { self.chain_index() .tipset_by_height( - ts.epoch() - self.chain_config().policy.chain_finality, + ts.epoch() - finality, ts.clone(), ResolveNullTipset::TakeOlder, ) - .map_err(|e| Error::Other(e.to_string()))? + .map_err(|e| { + Error::Other(format!( + "failed loading lookback tipset at epoch {}: {e}", + ts.epoch() - finality + )) + })? } else { // Matches the logic at <https://github.com/filecoin-project/lotus/blob/v1.35.1/chain/stmgr/stmgr.go#L361> ts.clone() }; let state = StateTree::new_from_root(self.blockstore().clone(), lookback_ts.parent_state()) - .map_err(|e| Error::Other(e.to_string()))?; + .map_err(|e| Error::Other(format!("failed building state tree: {e}")))?; state .resolve_to_deterministic_addr(self.blockstore(), *addr) - .map_err(|e| Error::Other(e.to_string())) + .map_err(|e| { + Error::Other(format!( + "failed resolving deterministic address at finality for {addr}: {e}" + )) + })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 the current code and only fix it if needed. In `@src/message_pool/msgpool/provider.rs` around lines 122 - 140, The error mappings around the lookback/tipset and state resolution drop useful context; update the calls that currently use map_err(|e| Error::Other(e.to_string())) for tipset_by_height, StateTree::new_from_root, and state.resolve_to_deterministic_addr to add operation-specific context (e.g., using anyhow::Context and .context("...") or otherwise wrapping the error with a message) so failures include which step failed (computing lookback_ts via tipset_by_height, constructing StateTree from parent_state, or resolving deterministic address) and relevant values (epoch/chain_finality, parent_state, addr); adjust the function signature to return anyhow::Result if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/message_pool/msgpool/provider.rs`:
- Around line 122-140: The error mappings around the lookback/tipset and state
resolution drop useful context; update the calls that currently use map_err(|e|
Error::Other(e.to_string())) for tipset_by_height, StateTree::new_from_root, and
state.resolve_to_deterministic_addr to add operation-specific context (e.g.,
using anyhow::Context and .context("...") or otherwise wrapping the error with a
message) so failures include which step failed (computing lookback_ts via
tipset_by_height, constructing StateTree from parent_state, or resolving
deterministic address) and relevant values (epoch/chain_finality, parent_state,
addr); adjust the function signature to return anyhow::Result if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bfa04351-eada-43f5-82a3-cbbe759a7519
📒 Files selected for processing (6)
src/message_pool/msgpool/mod.rssrc/message_pool/msgpool/msg_pool.rssrc/message_pool/msgpool/provider.rssrc/message_pool/msgpool/selection.rssrc/message_pool/msgpool/test_provider.rssrc/state_manager/mod.rs
✅ Files skipped from review due to trivial changes (1)
- src/state_manager/mod.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/message_pool/msgpool/test_provider.rs
- src/message_pool/msgpool/mod.rs
- src/message_pool/msgpool/msg_pool.rs
- src/message_pool/msgpool/selection.rs
LesnyRumcajs
previously approved these changes
Apr 23, 2026
Co-authored-by: Copilot <copilot@github.com>
LesnyRumcajs
approved these changes
Apr 23, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
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
Follow up on https://github.com/ChainSafe/forest/pull/6910/changes#r3109534232
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Refactor
Documentation