fix(rpc): harden Emitter::mempool against a malicious RPC

[tvpeter]

Description

This PR fixes Emitter::mempool and mempool_at to protect against DOS and data spoofing vectors originating from a malicious RPC node or a MITM actor. The updates address three vulnerabilities:

• Unbounded Tip-Consistency Loop: Prevents infinite spin loops when a node returns flapping tip headers.
• Unverified Txid Validation: Rejects fetched txs bodies whose computed hash does not match the requested txid.
• Unbounded Memory Usage: Limits tracked mempool transactions in mempool_snapshot to prevent memory exhaustion.

Problem and fix

• Unbounded tip-consistency spin loop: The loop in mempool_at retries get_block_count and get_block_hash until two consecutive calls agree on the tip. If an RPC node alternates responses between calls, this loop spins indefinitely causing high CPU usage and rapid RPC request flooding.
  Fix: bound the loop with MEMPOOL_TIP_CONSISTENCY_MAX_ATTEMPTS
• Unbounded mempool_snapshot memory growth: Every txid returned by getrawmempool that is not already cached is fetched in full via get_raw_transaction and inserted into mempool_snapshot. There is no limit on the number of transactions fetched or tracked per poll, leading to uncontrolled memory growth if the node presents an arbitrary number of transactions.
  Fix: track at most max_mempool_txs transactions (default DEFAULT_MAX_MEMPOOL_TXS = 100_000, configurable via Emitter::with_max_mempool_txs). Once the cap is reached, unknown mempool transactions are skipped. The emitter’s own known transactions bypass the fetch path and are never dropped.
• Missing txid validation on fetch: When inserting fetched transactions into mempool_snapshot via self.mempool_snapshot.insert(txid, tx.clone()), the map key is set to the requested txid rather than tx.compute_txid(). If the RPC server returns a transaction body whose actual txid does not match the requested txid, the crate accepts and caches the mismatched tx without error.
  Fix: recompute the txid of the fetched tx body and reject it on mismatch, before it is cached or emitted

Closes #2282

Notes to the reviewers

• New pub const DEFAULT_MAX_MEMPOOL_TXS: usize = 100_000
• New builder method Emitter::with_max_mempool_txs(usize)
• No changes to Emitter::new or any existing method signatures.
• Failure paths in the fixes reuse the existing bitcoincore_rpc::Error::UnexpectedStructure variant, so no new error type is introduced. mempool_at may now return Err or a truncated snapshot in situations where it previously hung or grew unboundedly

Changelog notice

• FixedEmitter::mempool/mempool_at: bound the tip-consistency retry loop so a flapping or malicious node cannot spin it forever, and reject transaction bodies whose computed txid does not match the requested txid instead of caching a
  mis-keyed entry.

Checklists

All Submissions:

• I followed the contribution guidelines

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.80%. Comparing base (acc06e5) to head (f55891f).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2283      +/-   ##
==========================================
+ Coverage   78.71%   78.80%   +0.09%     
==========================================
  Files          31       31              
  Lines        5966     5979      +13     
  Branches      282      285       +3     
==========================================
+ Hits         4696     4712      +16     
+ Misses       1194     1193       -1     
+ Partials       76       74       -2     

Flag	Coverage Δ
rust	78.80% <100.00%> (+0.09%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[notmandatory]

How did you choose this default? it seems low to me to be a max value. The current mempool has about 80K tx in it. I'd expect even a much larger max would prevent the unbounded malicious issue without risking the number being too low and ignoring tx from a valid non-malicious node.

[tvpeter]

I did not think thoroughly on this default count, as I assumed that users will be more inclined to set a count that serves them and that is why I added the Emitter::with_max_mempool_txs method to allow them to set the count that is suitable for them. But as you noted, I checked and it should ideally be on an upper ceiling of 1m based on Core’s maxmempool default size of 300mb.

[notmandatory]

I'm not 100% convinced this needs to be fixed. What examples can you think of where someone would be connecting to a malicious node? The general recommended practice is to only connect to core nodes you control and I don't know of any public services that provide public RPC access.

[tvpeter]

I'm not 100% convinced this needs to be fixed. What examples can you think of where someone would be connecting to a malicious node? The general recommended practice is to only connect to core nodes you control and I don't know of any public services that provide public RPC access.

For anyone running their own setup, the risk is near zero. However, for a remote setup (node on a VPS), with no TLS, there is a (little) chance of MITM attack. However, that may not be a good argument to warrant these changes. The only sub-issue I think should be considered is the transaction validation on fetch which is already done in the BdkElectrumClient::fetch_tx. I'm happy to scope it to just that issue or drop the entire PR if it does not merit a change.

Thank you.

[notmandatory]

For anyone running their own setup, the risk is near zero. However, for a remote setup (node on a VPS), with no TLS, there is a (little) chance of MITM attack. However, that may not be a good argument to warrant these changes. The only sub-issue I think should be considered is the transaction validation on fetch which is already done in the BdkElectrumClient::fetch_tx. I'm happy to scope it to just that issue or drop the entire PR if it does not merit a change.

Yes I think the code changes here are out of scope but double check if there's somewhere in the docs we need to mention that this feature assumes the user is connected to a bitcoind node they trust. And given that assumption I don't think the tx validation check is needed either. Although it seems like a basic safety thing, I have a concern about how it would impact performance. The RPC interface is ideal for users with large wallets (ie. some sort of shared custodial setup).

But I could be wrong that adding this check has any performance impact, and if it's negligible then it doesn't hurt to add and I'd support doing it. Think about how you might test to see what the impact is on a test wallet validating 1000s of tx during initial sync. Thanks for looking into this.

[notmandatory]

Also want to mention, your code and this discussion are still valuable even if nothing gets merged. It helps other devs (or LLMS 🤖) who might see the same potential issue.

[tvpeter]

Yes I think the code changes here are out of scope but double check if there's somewhere in the docs we need to mention that this feature assumes the user is connected to a bitcoind node they trust.

I think the most suitable place to consider is a fresh section at the module level documentation here

But I could be wrong that adding this check has any performance impact, and if it's negligible then it doesn't hurt to add and I'd support doing it. Think about how you might test to see what the impact is on a test wallet validating 1000s of tx during initial sync.

I ran a benchmark on my fork (#1) and the check's impact seemed negligible. It costs a stable ~0.8 µs/tx for a typical tx (~0.6–2.7 µs across sizes), roughly 2× a bare deserialize, but both are sub-microsecond and it is <1% of the full get_raw_transaction fetch. Extrapolated, even a full ~100k-tx mempool is ~80 ms of hashing on top of a fetch phase measured in tens of seconds. It also runs only on the mempool path and once per newly-fetched tx.

So if you consider the above performance impact level acceptable, then I will scope the PR to a documentation change and the check only otherwise I will happily close it.

Thank you