Skip to content

fix(mpool): resolve_to_key error handling#6890

Merged
sudo-shashank merged 3 commits intomainfrom
shashank/fix-resolve-to-key
Apr 11, 2026
Merged

fix(mpool): resolve_to_key error handling#6890
sudo-shashank merged 3 commits intomainfrom
shashank/fix-resolve-to-key

Conversation

@sudo-shashank
Copy link
Copy Markdown
Contributor

@sudo-shashank sudo-shashank commented Apr 10, 2026

Summary of changes

Changes introduced in this pull request:

  • log and continue if resolve_to_key fails.

Reference issue to close (if applicable)

Closes

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

    • Address-resolution failures in message pool operations now emit debug logs with error details and skip affected items instead of aborting processing.
  • Refactor

    • Improved logging and error-handling around message address resolution for better observability and more graceful failure behavior.

@sudo-shashank sudo-shashank added the RPC requires calibnet RPC checks to run on CI label Apr 10, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 8692687b-dca2-4026-8f3a-2c0b7009f4d6

📥 Commits

Reviewing files that changed from the base of the PR and between ad1d4fa and b727d5f.

📒 Files selected for processing (1)
  • src/message_pool/msgpool/provider.rs
✅ Files skipped from review due to trivial changes (1)
  • src/message_pool/msgpool/provider.rs

Walkthrough

Convert address-resolution failures in the message pool to debug-level logs and continue processing instead of returning errors; no public APIs or signatures changed.

Changes

Cohort / File(s) Summary
Message pool error handling
src/message_pool/msgpool/mod.rs, src/message_pool/msgpool/msg_pool.rs
Suppress resolve_to_key errors: emit tracing::debug! with context (actor/from/address and error) and continue. republish_pending_messages skips that actor; remove_from_selected_msgs proceeds without removal; pending_for logs and returns None on failure.
Comment update
src/message_pool/msgpool/provider.rs
Replace a detailed NOTE comment above resolve_to_key with a TODO(forest) reference; no functional change.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Suggested reviewers

  • hanabi1224
  • LesnyRumcajs
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(mpool): resolve_to_key error handling' directly and specifically describes the main change: converting resolve_to_key errors from failures to logging and continuation, which is the core modification across all three files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ 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 shashank/fix-resolve-to-key
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch shashank/fix-resolve-to-key

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

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/message_pool/msgpool/mod.rs (1)

347-351: Nice resilience improvement in head-change removal path.

This prevents head_change from failing hard on a single address-resolution error and keeps processing moving.

Optional: add a metric counter for skipped removals due to resolution failure, so this is visible even when debug logs are off.
Based on learnings: In Forest's head_change function (src/message_pool/msgpool/mod.rs), re-resolving msg.from() at re-add time is intentional and matches Lotus behavior.

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

In `@src/message_pool/msgpool/mod.rs` around lines 347 - 351, The head-change
removal now swallows address-resolution errors by using resolve_to_key(self.api,
self.key_cache, from, self.ts).inspect_err(...) so removals continue; update the
block around resolve_to_key/remove (symbols: resolve_to_key, remove, self.api,
self.key_cache, from, self.ts, pending, sequence) to also increment a metric
counter when resolution fails (e.g., a skipped_removals or
head_change_resolve_fail counter) and keep the existing tracing::debug log—i.e.,
on Err path call the metric increment before returning/continuing, and on Ok
continue to call remove(&resolved, self.pending, sequence, true)?.
🤖 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/mod.rs`:
- Around line 347-351: The head-change removal now swallows address-resolution
errors by using resolve_to_key(self.api, self.key_cache, from,
self.ts).inspect_err(...) so removals continue; update the block around
resolve_to_key/remove (symbols: resolve_to_key, remove, self.api,
self.key_cache, from, self.ts, pending, sequence) to also increment a metric
counter when resolution fails (e.g., a skipped_removals or
head_change_resolve_fail counter) and keep the existing tracing::debug log—i.e.,
on Err path call the metric increment before returning/continuing, and on Ok
continue to call remove(&resolved, self.pending, sequence, true)?.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 890f9f73-6dbb-4a53-a8bd-0d80aa5d55fb

📥 Commits

Reviewing files that changed from the base of the PR and between 6fab16d and e4d1a0a.

📒 Files selected for processing (2)
  • src/message_pool/msgpool/mod.rs
  • src/message_pool/msgpool/msg_pool.rs

@sudo-shashank sudo-shashank marked this pull request as ready for review April 11, 2026 00:07
@sudo-shashank sudo-shashank requested a review from a team as a code owner April 11, 2026 00:07
@sudo-shashank sudo-shashank requested review from LesnyRumcajs and hanabi1224 and removed request for a team April 11, 2026 00:07
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 11, 2026

Codecov Report

❌ Patch coverage is 30.00000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.11%. Comparing base (6fab16d) to head (b727d5f).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/message_pool/msgpool/mod.rs 16.66% 3 Missing and 2 partials ⚠️
src/message_pool/msgpool/msg_pool.rs 50.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/message_pool/msgpool/provider.rs 54.16% <ø> (-2.09%) ⬇️
src/message_pool/msgpool/msg_pool.rs 88.65% <50.00%> (-0.18%) ⬇️
src/message_pool/msgpool/mod.rs 91.21% <16.66%> (-0.62%) ⬇️

... and 4 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 6fab16d...b727d5f. 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.

@sudo-shashank sudo-shashank force-pushed the shashank/fix-resolve-to-key branch from ad1d4fa to b727d5f Compare April 11, 2026 00:33
@sudo-shashank sudo-shashank enabled auto-merge April 11, 2026 00:46
@sudo-shashank sudo-shashank added this pull request to the merge queue Apr 11, 2026
Merged via the queue into main with commit b0c5e23 Apr 11, 2026
46 checks passed
@sudo-shashank sudo-shashank deleted the shashank/fix-resolve-to-key branch April 11, 2026 01:18
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.

2 participants