Skip to content

fix(mpool): MempoolFilter returns the pending transactions#7250

Merged
akaladarshi merged 6 commits into
mainfrom
akaladarshi/fix-mempool-filter
Jul 2, 2026
Merged

fix(mpool): MempoolFilter returns the pending transactions#7250
akaladarshi merged 6 commits into
mainfrom
akaladarshi/fix-mempool-filter

Conversation

@akaladarshi

@akaladarshi akaladarshi commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Summary of changes

Changes introduced in this pull request:

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

  • New Features

    • Improved pending-transaction filter/subscription to deliver Ethereum tx hashes as transactions are added to the mempool, with per-filter buffering and incremental updates.
    • Refined Ethereum event handling so RPC filters use the correct chain context and react to live message-pool changes.
  • Bug Fixes

    • eth_newPendingTransactionFilter now reports hashes only for transactions that are actually pending in the mempool (not already executed on-chain).
    • eth_getFilterChanges now returns only newly observed pending tx hashes per poll, without repeating previously delivered results.

@akaladarshi akaladarshi requested a review from a team as a code owner June 29, 2026 10:03
@akaladarshi akaladarshi requested review from hanabi1224 and sudo-shashank and removed request for a team June 29, 2026 10:03
@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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: 342b14fd-8afe-4fec-91e6-b5721913e779

📥 Commits

Reviewing files that changed from the base of the PR and between 45f7c4b and 4a9c911.

📒 Files selected for processing (8)
  • CHANGELOG.md
  • src/daemon/mod.rs
  • src/message_pool/msgpool/events.rs
  • src/rpc/methods/eth.rs
  • src/rpc/methods/eth/filter/mempool.rs
  • src/rpc/methods/eth/filter/mod.rs
  • src/tool/subcommands/api_cmd/stateful_tests.rs
  • src/utils/task/mod.rs
🔗 Linked repositories identified

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

  • filecoin-project/lotus (manual)
🚧 Files skipped from review as they are similar to previous changes (6)
  • src/daemon/mod.rs
  • src/message_pool/msgpool/events.rs
  • src/rpc/methods/eth/filter/mod.rs
  • src/rpc/methods/eth.rs
  • src/tool/subcommands/api_cmd/stateful_tests.rs
  • src/rpc/methods/eth/filter/mempool.rs

Walkthrough

Pending-transaction filtering now uses a subscriber-backed mempool stream, buffers hashes per filter, and drains them through the Ethereum RPC path. Handler construction passes chain id and mempool subscriber through daemon and offline-server wiring. Pubsub and stateful tests are updated to match the new hash flow.

Changes

Pending transaction hash flow

Layer / File(s) Summary
Subscriber wrapper and API migration
src/message_pool/msgpool/events.rs, src/message_pool/msgpool/mod.rs, src/message_pool/msgpool/msg_pool.rs, src/message_pool/msgpool/pending_store.rs, src/message_pool/msgpool/selection.rs, src/utils/task/mod.rs
Adds MpoolSubscriber, re-exports it, changes message-pool subscription entry points to return the subscriber handle, and updates the related call sites and derives.
Mempool filter buffering and fan-out
src/rpc/methods/eth/filter/mempool.rs
Replaces the mempool filter with a bounded FIFO of hashes, adds the pending-transaction hash stream helper, redesigns the filter manager around a shared fan-out task, and rewrites the unit tests for the new behavior.
Ethereum event handler wiring
src/rpc/methods/eth/filter/mod.rs, src/daemon/mod.rs, src/tool/offline_server/server.rs, src/tool/subcommands/api_cmd/generate_test_snapshot.rs, src/tool/subcommands/api_cmd/test_snapshot.rs
Extends EthEventHandler construction to accept chain id and MpoolSubscriber, wires those inputs from daemon and offline server code, and updates the related ETH filter tests and snapshot builders.
RPC filter and pubsub hash paths
src/rpc/methods/eth.rs, src/rpc/methods/eth/pubsub.rs
Changes pending filter change handling to drain buffered hashes directly, updates pubsub to use the shared pending-transaction hash stream, and switches the Ethereum hash/result helpers to the new tipset-based paths.
Integration tests and changelog
src/tool/subcommands/api_cmd/stateful_tests.rs, CHANGELOG.md
Adds polling helpers and pending-transaction filter scenarios to the stateful test suite, and records the pending-filter behavior change in the changelog.

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

Possibly related PRs

  • ChainSafe/forest#6941: Touches the same src/rpc/methods/eth/pubsub.rs pending-tx subscription path and maps mempool Add updates into Ethereum tx hashes.
  • ChainSafe/forest#6965: Introduces the PendingStore/MpoolUpdate broadcast plumbing that this PR builds on with MpoolSubscriber.
  • ChainSafe/forest#7077: Modifies the collect_events RPC cache path in the same ETH event handling area.

Suggested reviewers: hanabi1224

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: MempoolFilter now returns pending mempool transactions.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ 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 akaladarshi/fix-mempool-filter
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch akaladarshi/fix-mempool-filter

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

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 4

🤖 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.

Inline comments:
In `@src/rpc/methods/eth/filter/mempool.rs`:
- Around line 256-258: The async test in the mempool filter flow should not rely
on a single tokio::task::yield_now() to let the fan-out task deliver the
broadcast event. Update the test around the filter subscription/drain logic in
the mempool.rs path to poll with a small timeout until both filters have
received the hash, using the existing filter/broadcast setup instead of assuming
one scheduler yield is sufficient.
- Around line 119-128: The fan-out task startup in ensure_fanout_task currently
calls tokio::spawn unconditionally, which can panic if there is no active Tokio
runtime. Update the install path that reaches ensure_fanout_task to check for a
runtime first and return an installation error instead of spawning blindly, and
make sure the caller of ensure_fanout_task propagates that Result so the
existing error handling is preserved.

In `@src/rpc/methods/eth/filter/mod.rs`:
- Around line 142-146: `EthEventHandler::new()` is wiring pending-filter RPCs to
`MpoolSubscriber::dummy()`, so `eth_newPendingTransactionFilter` never receives
mempool adds in states built from it. Update the RPC state constructors that use
pending filters, especially the snapshot/test paths in `generate_test_snapshot`
and `test_snapshot`, to pass a real `MessagePool` subscriber
(`mpool.subscriber()`) into `EthEventHandler::from_config(...)` instead of
relying on the dummy subscriber. Make sure the `EthEventHandler` and related
`RPCState` setup still use the same `EventsConfig` and chain ID, but are
connected to the live mempool bus.

In `@src/tool/subcommands/api_cmd/stateful_tests.rs`:
- Around line 1041-1044: The pending-tx assertions are still happening after
chain inclusion because wait_pending_message() continues into StateWaitMsg, so
the tests do not actually verify pre-mining mempool behavior. Update the
stateful tests to inspect the filter immediately after the first MpoolPending
observation, or split wait_pending_message() into a helper that returns once the
CID appears so the test can stop before StateWaitMsg. Apply the same fix in both
affected test sections that use wait_pending_message() and
poll_pending_filter_until().
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: b1fab06a-d564-4511-ba0e-b6e9944153cd

📥 Commits

Reviewing files that changed from the base of the PR and between 28d3c1c and 11b230e.

📒 Files selected for processing (14)
  • CHANGELOG.md
  • src/daemon/mod.rs
  • src/message_pool/msgpool/events.rs
  • src/message_pool/msgpool/mod.rs
  • src/message_pool/msgpool/msg_pool.rs
  • src/message_pool/msgpool/pending_store.rs
  • src/message_pool/msgpool/selection.rs
  • src/rpc/methods/eth.rs
  • src/rpc/methods/eth/filter/mempool.rs
  • src/rpc/methods/eth/filter/mod.rs
  • src/rpc/methods/eth/pubsub.rs
  • src/tool/offline_server/server.rs
  • src/tool/subcommands/api_cmd/stateful_tests.rs
  • src/utils/task/mod.rs
🔗 Linked repositories identified

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

  • filecoin-project/lotus (manual)

Comment thread src/rpc/methods/eth/filter/mempool.rs Outdated
Comment thread src/rpc/methods/eth/filter/mempool.rs
Comment thread src/rpc/methods/eth/filter/mod.rs
Comment thread src/tool/subcommands/api_cmd/stateful_tests.rs
@akaladarshi akaladarshi added the RPC requires calibnet RPC checks to run on CI label Jun 29, 2026
@codecov

codecov Bot commented Jun 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.86538% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.23%. Comparing base (ef3a30b) to head (4a9c911).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/rpc/methods/eth/filter/mempool.rs 96.66% 3 Missing and 2 partials ⚠️
...tool/subcommands/api_cmd/generate_test_snapshot.rs 0.00% 5 Missing ⚠️
src/daemon/mod.rs 0.00% 4 Missing ⚠️
src/rpc/methods/eth/pubsub.rs 0.00% 3 Missing ⚠️
src/rpc/methods/eth.rs 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/message_pool/msgpool/events.rs 100.00% <100.00%> (ø)
src/message_pool/msgpool/mod.rs 97.91% <ø> (ø)
src/message_pool/msgpool/msg_pool.rs 88.28% <100.00%> (+0.46%) ⬆️
src/message_pool/msgpool/pending_store.rs 82.85% <100.00%> (ø)
src/message_pool/msgpool/selection.rs 87.45% <100.00%> (ø)
src/rpc/methods/eth/filter/mod.rs 88.56% <100.00%> (+0.15%) ⬆️
src/tool/offline_server/server.rs 28.89% <100.00%> (+1.32%) ⬆️
src/tool/subcommands/api_cmd/test_snapshot.rs 82.63% <100.00%> (+0.42%) ⬆️
src/rpc/methods/eth.rs 66.22% <0.00%> (+1.26%) ⬆️
src/rpc/methods/eth/pubsub.rs 58.80% <0.00%> (+2.12%) ⬆️
... and 3 more

... and 5 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 ef3a30b...4a9c911. 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 previously approved these changes Jul 2, 2026

@sudo-shashank sudo-shashank 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.

LGTM

Comment thread src/rpc/methods/eth/filter/mempool.rs Outdated
Comment thread src/rpc/methods/eth/filter/mempool.rs Outdated
Comment thread src/message_pool/msgpool/events.rs Outdated
@akaladarshi akaladarshi added this pull request to the merge queue Jul 2, 2026
Merged via the queue into main with commit 66ac79e Jul 2, 2026
64 of 65 checks passed
@akaladarshi akaladarshi deleted the akaladarshi/fix-mempool-filter branch July 2, 2026 11:21
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.

4 participants