Skip to content

fix: finality-gated address cache - #7355

Merged
LesnyRumcajs merged 1 commit into
mainfrom
hb/finality-gated-address-cache
Jul 14, 2026
Merged

fix: finality-gated address cache#7355
LesnyRumcajs merged 1 commit into
mainfrom
hb/finality-gated-address-cache

Conversation

@LesnyRumcajs

@LesnyRumcajsLesnyRumcajs commented Jul 14, 2026

Copy link
Copy Markdown
Member

Summary of changes

Changes introduced in this pull request:

Reference issue to close (if applicable)

Closes#7341

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Outside contributions

  • I have read and agree to the CONTRIBUTING document.
  • I have read and agree to the AI Policy document. I understand that failure to comply with the guidelines will lead to rejection of the pull request.

Summary by CodeRabbit

  • New Features

    • Improved deterministic address resolution based on chain finality.
    • Added safer handling for addresses that may be affected by reorganizations.
  • Bug Fixes

    • Improved address-resolution caching to avoid retaining results that may become outdated.
    • Added support for resolving addresses using appropriate historical chain state.
  • Tests

    • Added coverage for finality boundaries, reorganization scenarios, cache behavior, and non-ID addresses.
  • Chores

    • Updated the project dictionary with lookback and memoize.

@LesnyRumcajs
LesnyRumcajs requested a review from a team as a code ownerJuly 14, 2026 13:17
@LesnyRumcajs
LesnyRumcajs requested review from hanabi1224 and sudo-shashank and removed request for a teamJuly 14, 2026 13:17
@coderabbitai

coderabbitaiBot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Finality-aware deterministic address resolution is centralized in ChainStore. The message pool delegates to it, while StateManager caches only reorg-stable ID-address results and adds tests for finality boundaries. The project dictionary gains lookback and memoize.

Changes

Finality-aware address resolution

Layer / File(s)Summary
ChainStore finality resolution
src/chain/store/chain_store.rs
Adds AtFinalityResolution and resolves addresses using current or finality-lookback state.
Message pool delegation
src/message_pool/msgpool/provider.rs
Delegates deterministic address resolution to ChainStore and maps results or errors.
Finality-aware caching and tests
src/state_manager/address_resolution.rs, src/state_manager/mod.rs
Caches only reorg-stable ID-address resolutions, adds uncached resolution support, and tests finality-depth behavior.
Dictionary update
Layer / File(s)Summary
Dictionary entries
.config/forest.dic
Updates the count to 279 and adds lookback and memoize.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
participant MessagePoolProvider
participant ChainStore
participant StateManager
participant IdToAddressCache
MessagePoolProvider->>ChainStore: resolve deterministic address at finality
ChainStore-->>MessagePoolProvider: AtFinalityResolution
StateManager->>IdToAddressCache: check ID-address cache
StateManager->>ChainStore: resolve uncached ID address at finality
ChainStore-->>StateManager: ReorgStable or Unstable
StateManager->>IdToAddressCache: cache only ReorgStable result
Loading

Possibly related PRs

Suggested reviewers:hanabi1224, sudo-shashank

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Docstring Coverage✅ PassedDocstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
Description Check✅ PassedCheck skipped - CodeRabbit’s high-level summary is enabled.
Title check✅ PassedThe title clearly matches the main change: caching addresses only when finality conditions are met.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch hb/finality-gated-address-cache
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch hb/finality-gated-address-cache

Comment @coderabbitai help to get the list of available commands.

Comment threadsrc/chain/store/chain_store.rs

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

108-116: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Preserve finality before caching ID resolutions.resolve_to_deterministic_address_at_finality collapses the ReorgStable/Unstable distinction, and resolve_to_key caches every resolved ID address unconditionally. That lets non-finalized mappings stick in the long-lived mpool cache; keep the stability signal here or skip caching unstable results.

🤖 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/message_pool/msgpool/provider.rs` around lines 108 - 116, Update
resolve_to_deterministic_address_at_finality and the resolve_to_key caching flow
to preserve the AtFinalityResolution stability state instead of unconditionally
converting it to Address. Cache ID resolutions only when the result is
ReorgStable, while returning Unstable resolutions without inserting them into
the long-lived mpool cache.
🤖 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/message_pool/msgpool/provider.rs`:
- Around line 108-116: Update resolve_to_deterministic_address_at_finality and
the resolve_to_key caching flow to preserve the AtFinalityResolution stability
state instead of unconditionally converting it to Address. Cache ID resolutions
only when the result is ReorgStable, while returning Unstable resolutions
without inserting them into the long-lived mpool cache.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 62f928fd-a86b-47d0-9efe-eea301e8092a

📥 Commits

Reviewing files that changed from the base of the PR and between 717906b and fca68a0.

📒 Files selected for processing (5)
  • .config/forest.dic
  • src/chain/store/chain_store.rs
  • src/message_pool/msgpool/provider.rs
  • src/state_manager/address_resolution.rs
  • src/state_manager/mod.rs
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • filecoin-project/lotus(manual)

@codecov

codecovBot commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.04520% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.74%. Comparing base (5289a39) to head (fca68a0).
⚠️ Report is 7 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing linesPatch %Lines
src/state_manager/address_resolution.rs97.24%2 Missing and 2 partials ⚠️
src/chain/store/chain_store.rs88.46%2 Missing and 1 partial ⚠️
Additional details and impacted files
Files with missing linesCoverage Δ
src/message_pool/msgpool/provider.rs69.84% <100.00%> (+1.89%)⬆️
src/state_manager/mod.rs60.34% <100.00%> (+0.51%)⬆️
src/chain/store/chain_store.rs70.96% <88.46%> (+0.34%)⬆️
src/state_manager/address_resolution.rs91.93% <97.24%> (+17.86%)⬆️

... and 24 files with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5289a39...fca68a0. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@LesnyRumcajsLesnyRumcajs added the RPC requires calibnet RPC checks to run on CI label Jul 14, 2026
@LesnyRumcajs
LesnyRumcajs added this pull request to the merge queueJul 14, 2026
Merged via the queue into main with commit e39d57fJul 14, 2026
60 checks passed
@LesnyRumcajs
LesnyRumcajs deleted the hb/finality-gated-address-cache branch July 14, 2026 14:46
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RPCrequires calibnet RPC checks to run on CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cache StateAccountKey RPC method

2 participants

@LesnyRumcajs@hanabi1224