fix MempoolFilter returning pending transactions#7109
Conversation
Fraccaman
requested review from
akaladarshi and
hanabi1224
and removed request for
a team
|
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 (8)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughRefactors mempool filtering to consume MpoolUpdate broadcasts: exposes the message-pool sender, gives each MempoolFilter its own broadcast receiver, wires the sender into EthEventHandler/MempoolFilterManager, updates RPCs to return pending mempool hashes, and adjusts tests to poll mempool presence. ChangesMempool-driven pending transaction filter
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPC as Eth RPC
participant FilterManager
participant MempoolFilter
participant MessagePool
Client->>RPC: eth_getFilterChanges request
RPC->>FilterManager: lookup & drain(filter_id, chain_id)
FilterManager->>MempoolFilter: drain(chain_id)
MempoolFilter->>MessagePool: consume MpoolUpdate via Receiver.try_recv()
MempoolFilter->>RPC: return EthFilterResult::Hashes
RPC->>Client: respond with hashes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/rpc/methods/eth.rs (1)
1106-1120: ⚡ Quick winConsolidate the signed-message hash derivation into one helper.
eth_tx_hash_from_signed_messagebelow still reimplements the same delegated/secp/BLS branches, so this extraction can drift from the event/log path. Please make one helper delegate to the other and keep the hash rules in one place.♻️ Minimal consolidation
fn eth_tx_hash_from_signed_message( message: &SignedMessage, eth_chain_id: EthChainIdType, ) -> anyhow::Result<EthHash> { - if message.is_delegated() { - let (_, tx) = eth_tx_from_signed_eth_message(message, eth_chain_id)?; - Ok(tx.eth_hash()?.into()) - } else if message.is_secp256k1() { - Ok(message.cid().into()) - } else { - Ok(message.message().cid().into()) - } + eth_hash_from_signed_message(message, eth_chain_id) }🤖 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/rpc/methods/eth.rs` around lines 1106 - 1120, The logic that derives an Ethereum-style hash from a SignedMessage has been factored into eth_hash_from_signed_message but eth_tx_hash_from_signed_message still duplicates the delegated/secp256k1/BLS branching; consolidate by making eth_tx_hash_from_signed_message call eth_hash_from_signed_message (or vice‑versa) so the hash rules live in one place. Update eth_tx_hash_from_signed_message to remove its own branching and delegate to eth_hash_from_signed_message (passing the same SignedMessage and chain_id), ensuring all callers use the single helper to avoid drift between paths.
🤖 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`:
- Line 67: The current drain uses
pending.into_iter().take(self.max_results).collect() which returns an empty vec
when self.max_results == 0; change the logic so a zero max_results means “no
cap”: check self.max_results and if it is 0 collect the full pending.into_iter()
otherwise collect after take(self.max_results). Use the existing pending
iterator and self.max_results symbol to implement the conditional branch so the
returned collection contains all items when max_results == 0.
In `@src/tool/subcommands/api_cmd/stateful_tests.rs`:
- Around line 300-311: The mempool polling in wait_in_mempool has a very short
timeout (retries = 100 with 10ms sleeps); increase the retry budget or
per-iteration sleep to reduce flakiness in CI by changing the retries or
Duration::from_millis call in the wait_in_mempool function (e.g., bump retries
to a larger value like 1000 or increase the sleep to 50–100ms) so the loop has a
longer total wait before the ensure! triggers; update the retries initialization
and/or the tokio::time::sleep(Duration::from_millis(...)).
---
Nitpick comments:
In `@src/rpc/methods/eth.rs`:
- Around line 1106-1120: The logic that derives an Ethereum-style hash from a
SignedMessage has been factored into eth_hash_from_signed_message but
eth_tx_hash_from_signed_message still duplicates the delegated/secp256k1/BLS
branching; consolidate by making eth_tx_hash_from_signed_message call
eth_hash_from_signed_message (or vice‑versa) so the hash rules live in one
place. Update eth_tx_hash_from_signed_message to remove its own branching and
delegate to eth_hash_from_signed_message (passing the same SignedMessage and
chain_id), ensuring all callers use the single helper to avoid drift between
paths.
🪄 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: 15f882bf-4135-452b-b120-7f6d544dc8b8
📒 Files selected for processing (8)
src/daemon/mod.rssrc/message_pool/msgpool/msg_pool.rssrc/message_pool/msgpool/pending_store.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/filter/mempool.rssrc/rpc/methods/eth/filter/mod.rssrc/tool/offline_server/server.rssrc/tool/subcommands/api_cmd/stateful_tests.rs
| } | ||
| } | ||
| } | ||
| pending.into_iter().take(self.max_results).collect() |
There was a problem hiding this comment.
Handle max_results == 0 as “no cap” in drain output.
Line 67 currently truncates with take(0), which returns an empty vector when max_results is zero; this conflicts with the filter subsystem’s “0 disables cap” convention.
Proposed fix
- pending.into_iter().take(self.max_results).collect()
+ if self.max_results == 0 {
+ return pending.into_iter().collect();
+ }
+ pending.into_iter().take(self.max_results).collect()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pending.into_iter().take(self.max_results).collect() | |
| if self.max_results == 0 { | |
| return pending.into_iter().collect(); | |
| } | |
| pending.into_iter().take(self.max_results).collect() |
🤖 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/rpc/methods/eth/filter/mempool.rs` at line 67, The current drain uses
pending.into_iter().take(self.max_results).collect() which returns an empty vec
when self.max_results == 0; change the logic so a zero max_results means “no
cap”: check self.max_results and if it is 0 collect the full pending.into_iter()
otherwise collect after take(self.max_results). Use the existing pending
iterator and self.max_results symbol to implement the conditional branch so the
returned collection contains all items when max_results == 0.
0f8af7d to
4f45897
Compare
Fraccaman
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 19 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
@Fraccaman CI linter seems to be failing please fix it, you can use Also all the commits needs to signed, so please do that as well. |
Fraccaman
force-pushed
the
fix-mempool-filters
branch
2 times, most recently
from
53c3804 to
e39aa6f
Compare
|
@akaladarshi I signed the commits and ran |
Hey @Fraccaman, Sorry for delayed response. Also you need to first run the |
Fraccaman
force-pushed
the
fix-mempool-filters
branch
from
e39aa6f to
5464537
Compare
|
I rebased but |
akaladarshi
left a comment
akaladarshi
left a comment
There was a problem hiding this comment.
We also need to add tests for the EthGetFilterChanges where we call it multiple times and see if each call returns the new pending transaction since the last poll.
|
|
||
| /// Clone of the [`MpoolUpdate`] broadcast sender. New independent | ||
| /// receivers can be derived by calling `subscribe()` on the clone. | ||
| pub(in crate::message_pool) fn event_sender(&self) -> broadcast::Sender<MpoolUpdate> { |
There was a problem hiding this comment.
We made the events private for a specific reason, events should only be sent by the message pool module and every other module will read the data through subscribe method receiver.
This breaks the encapsulation.
|
|
||
| /// Clone of the [`MpoolUpdate`] sender. Each call to `subscribe()` on | ||
| /// the returned sender yields an independent receiver. | ||
| pub fn mpool_event_sender(&self) -> broadcast::Sender<MpoolUpdate> { |
There was a problem hiding this comment.
Same here this breaks the encapsulation, event sender is send only should not shared.
| } | ||
|
|
||
| /// Derive the Ethereum-style hash for a `SignedMessage`. | ||
| pub fn eth_hash_from_signed_message( |
There was a problem hiding this comment.
We already have similar fn eth_tx_hash_from_signed_message, please reuse it.
| Ok(MpoolUpdate::Remove(m)) => { | ||
| if let Some(h) = hash_or_log(&m, chain_id) { | ||
| pending.shift_remove(&h); |
There was a problem hiding this comment.
What is the actual purpose doing this ?
|
hey @akaladarshi im on vacation, ill fix this as soon as im back |
2 participants
Summary of changes
MempoolFilter was returning hashes of already-mined transactions instead of pending ones. This PR fixes it by subscribing each mempool filter to the mempool's Add/Remove broadcast channel, so it reports only transactions still in the pool.
Changes introduced in this pull request:
MempoolFilterManager::installnow create a Receiver oninstalldraintoMempoolFilterto add/remove pending tx hashesdraininEthGetFilterChanges::handlemethodeth_new_pending_transaction_filtertest + minor unitReference issue to close (if applicable)
Closes #7091
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Bug Fixes
Tests