Skip to content

fix(perf): cache resolve_to_deterministic_address result#6928

Merged
hanabi1224 merged 1 commit intomainfrom
hm/cache-resolve_to_deterministic_address
Apr 16, 2026
Merged

fix(perf): cache resolve_to_deterministic_address result#6928
hanabi1224 merged 1 commit intomainfrom
hm/cache-resolve_to_deterministic_address

Conversation

@hanabi1224
Copy link
Copy Markdown
Contributor

@hanabi1224 hanabi1224 commented Apr 16, 2026

Summary of changes

Changes introduced in this pull request:

Reference issue to close (if applicable)

Closes #6879

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

  • Bug Fixes
    • Improved reliability of address resolution with refined caching mechanisms for deterministic address lookups
    • Enhanced event filtering behavior with conditional event skipping logic

@hanabi1224 hanabi1224 added the RPC requires calibnet RPC checks to run on CI label Apr 16, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 16, 2026

Walkthrough

The changes implement caching for deterministic address resolution to improve eth_getLogs performance. A new LRU cache is added to StateManager for ID-protocol address resolution, and event collection now reuses cached resolution results per event emitter rather than resolving addresses repeatedly with error context.

Changes

Cohort / File(s) Summary
StateManager Address Resolution Cache
src/state_manager/mod.rs
Added id_to_deterministic_address_cache LRU cache field (capacity 1024) to StateManager. Extended resolve_to_deterministic_address to cache ID-protocol address resolutions: checks cache first, resolves on miss via parent-state or tipset state, then stores result before returning.
EthEventHandler Event Collection Optimization
src/rpc/methods/eth/filter/mod.rs
Modified collect_events to cache address resolution results per event emitter in a local resolved_id_addrs HashMap, reusing cached addresses across events. Removed per-event error context wrapping from resolution failures. Events now skip conditionally only for SkipEvent::OnUnresolvedAddress; unresolved id_addr values are otherwise retained. Removed explicit Vec type annotations.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

  • LesnyRumcajs
  • sudo-shashank
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: caching the result of resolve_to_deterministic_address, which directly addresses the performance issue identified in the linked issue.
Linked Issues check ✅ Passed The PR implements caching for resolve_to_deterministic_address resolution results to improve eth_getLogs performance, directly addressing the performance regression reported in #6879.
Out of Scope Changes check ✅ Passed All changes are scoped to performance optimization: caching resolution results in StateManager and reusing cached values in EthEventHandler::collect_events, with no unrelated modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 hm/cache-resolve_to_deterministic_address
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch hm/cache-resolve_to_deterministic_address

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

@hanabi1224 hanabi1224 force-pushed the hm/cache-resolve_to_deterministic_address branch from 450edd6 to 3016c68 Compare April 16, 2026 14:14
@hanabi1224 hanabi1224 marked this pull request as ready for review April 16, 2026 14:20
@hanabi1224 hanabi1224 requested a review from a team as a code owner April 16, 2026 14:20
@hanabi1224 hanabi1224 requested review from LesnyRumcajs and sudo-shashank and removed request for a team April 16, 2026 14:20
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/rpc/methods/eth/filter/mod.rs (1)

324-331: Consider adding trace-level logging for failed address resolutions.

The .ok() pattern silently discards resolution errors. While this is acceptable for performance, adding a trace log would help with debugging without impacting hot paths:

Suggested improvement
 let r = ctx
     .state_manager
     .resolve_to_deterministic_address(id_addr, tipset)
     .await
-    .ok();
+    .inspect_err(|e| tracing::trace!("failed to resolve emitter {emitter}: {e:#}"))
+    .ok();
 resolved_id_addrs.insert(emitter, r);

This would help diagnose issues when events are unexpectedly skipped without affecting normal performance.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/rpc/methods/eth/filter/mod.rs` around lines 324 - 331, The code silently
drops errors by calling .ok() on the result of
ctx.state_manager.resolve_to_deterministic_address(id_addr, tipset).await;
change this to match the Result so failures emit a trace-level log (e.g., using
trace! or ctx.logger.trace) including the emitter, id_addr and the error, and
then insert None into resolved_id_addrs as before; keep the hot path behavior
(still treating failures as None) but add the trace log in the Err branch to aid
debugging in resolve_to_deterministic_address handling.
🤖 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/rpc/methods/eth/filter/mod.rs`:
- Around line 324-331: The code silently drops errors by calling .ok() on the
result of ctx.state_manager.resolve_to_deterministic_address(id_addr,
tipset).await; change this to match the Result so failures emit a trace-level
log (e.g., using trace! or ctx.logger.trace) including the emitter, id_addr and
the error, and then insert None into resolved_id_addrs as before; keep the hot
path behavior (still treating failures as None) but add the trace log in the Err
branch to aid debugging in resolve_to_deterministic_address handling.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: dd42f9e6-234e-4318-b0c0-78f27a9662ae

📥 Commits

Reviewing files that changed from the base of the PR and between 2c22018 and 3016c68.

📒 Files selected for processing (2)
  • src/rpc/methods/eth/filter/mod.rs
  • src/state_manager/mod.rs

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 16, 2026

Codecov Report

❌ Patch coverage is 78.78788% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.87%. Comparing base (2c22018) to head (3016c68).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/state_manager/mod.rs 73.33% 0 Missing and 4 partials ⚠️
src/rpc/methods/eth/filter/mod.rs 83.33% 3 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/rpc/methods/eth/filter/mod.rs 88.96% <83.33%> (+0.41%) ⬆️
src/state_manager/mod.rs 66.57% <73.33%> (+0.06%) ⬆️

... and 11 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c22018...3016c68. 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.

@hanabi1224 hanabi1224 added this pull request to the merge queue Apr 16, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 16, 2026
@hanabi1224 hanabi1224 added this pull request to the merge queue Apr 16, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 16, 2026
@hanabi1224 hanabi1224 added this pull request to the merge queue Apr 16, 2026
Merged via the queue into main with commit 59e370a Apr 16, 2026
85 of 88 checks passed
@hanabi1224 hanabi1224 deleted the hm/cache-resolve_to_deterministic_address branch April 16, 2026 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RPC requires calibnet RPC checks to run on CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

eth_getLogs performance issue

2 participants