fix(rpc): verify fetched mempool txids before caching and document trust assumption by tvpeter · Pull Request #2316 · bitcoindevkit/bdk

tvpeter · 2026-09-16T07:45:32Z

Description

This PR adds a check to validate fetched raw mempool transaction hashes against the requested txid before caching in Emitter::mempool_at, preventing cache poisoning and state corruption from unverified RPC responses. It also adds a module level documentation that bdk_bitcoind_rpc assumes connection to a trusted bitcoind node.

This addresses only the missing txid-validation part of #2282, following discussions on #2283.

Notes to the reviewers

The check tx.compute_txid() != txid returns the existing Error::UnexpectedStructure on mismatch, so it is non-breaking.

Changelog notice

Emitter::mempool/mempool_at: verify that a fetched transaction's computed txid matches the requested txid before caching, rejecting a mismatch with Error::UnexpectedStructure.

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 · 2026-09-16T07:51:45Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.85%. Comparing base (e417c43) to head (3fba250).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2316   +/-   ##
=======================================
  Coverage   78.84%   78.85%           
=======================================
  Files          31       31           
  Lines        6060     6062    +2     
  Branches      288      289    +1     
=======================================
+ Hits         4778     4780    +2     
  Misses       1203     1203           
  Partials       79       79

Flag	Coverage Δ
rust	`78.85% <100.00%> (+<0.01%)`	⬆️

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.

tvpeter · 2026-09-16T08:31:10Z

The MSRV job is failing because of a transitive dependency yoke-derive (via esplora-client -> reqwest -> url -> idna-> icu -> yoke -> yoke-derive) that was updated yesterday (2026-09-15) to 0.8.3 and requires Rustc 1.87 to build. So we can either pin it to 0.8.2 in pin-msrv or bump the MSRV to 1.87.

Happy to open a tiny PR for the pin if that's preferred.

evanlinjin · 2026-09-18T05:18:56Z

@tvpeter I also experienced the CI failure and opened this PR: #2317

- Add `tx.compute_txid() == txid` check to validate that fetched raw mempool transaction hashes against their requested txid before caching in Emitter::mempool_at. Returns `Error::UnexpectedStructure` for a mismatch. - Add a regression test for the above check. - Document that the crate assumes a trusted `bitcoind` connection.

Dmenec

Approach ACK

I was thinking if the validation should retry (maybe 3 times at most) when failing so you don't throw the whole poll, but I think in this specific scenario it would be more annoying than useful.

Not sure if compute_txid cover the witness, so maybe a node could still serve a different witness for the same txid? If not, maybe worth a word in the doc so it doesn't read as the whole tx being verified.

Also, checking the code I noticed that next_block fetches blocks by hash but never checks the returned block actually hashes to what was requested:

bdk/crates/bitcoind_rpc/src/lib.rs

Lines 282 to 291 in 3fba250

    
               pub fn next_block(&mut self) -> Result<Option<BlockEvent<Block>>, bitcoincore_rpc::Error> { 
        
                   if let Some((checkpoint, block)) = poll(self, move |hash, client| client.get_block(hash))? { 
        
                       // Stop tracking unconfirmed transactions that have been confirmed in this block. 
        
                       for tx in &block.txdata { 
        
                           self.mempool_snapshot.remove(&tx.compute_txid()); 
        
                       } 
        
                       return Ok(Some(BlockEvent { block, checkpoint })); 
        
                   } 
        
                   Ok(None) 
        
               }

I think in the same way we should check the block hash, merkle root and witness commitment. Checking only the hash isn't enough, since it only covers the header, so a node could serve the genuine header with forged txs. I saw your benchmarks in #2283 so, as much, it should be a few ms per block. Given the scope was narrowed there, maybe better as a follow-up?

Also, just a few nits I've seen through

Dmenec · 2026-09-21T11:13:18Z

        Ok(())
    }
+
+    //A fetched mempool tx whose body does not match the requested txid is rejected.


Suggested change

//A fetched mempool tx whose body does not match the requested txid is rejected.

// A fetched mempool tx whose body does not match the requested txid is rejected.

Dmenec · 2026-09-22T07:15:22Z

    ///
    /// This is the no-std version of [`mempool`](Self::mempool).
+    ///
+    /// # Errors


nit: I would add smthg like this in mempool() so callers also have this information.

/// See [`mempool_at`](Self::mempool_at#errors) for errors.

Dmenec · 2026-09-22T07:26:56Z

+        impl RpcApi for LyingNode {
+            fn call<T: for<'a> serde::de::Deserialize<'a>>(
+                &self,
+                _cmd: &str,
+                _args: &[serde_json::Value],
+            ) -> Result<T, Error> {
+                unreachable!()
+            }
+            fn get_block_count(&self) -> Result<u64, Error> {
+                Ok(100)
+            }
+            fn get_block_hash(&self, _height: u64) -> Result<BlockHash, Error> {
+                Ok(self.tip_hash)
+            }
+            fn get_raw_mempool(&self) -> Result<Vec<Txid>, Error> {
+                Ok(vec![self.announced])
+            }
+            fn get_raw_transaction(
+                &self,
+                _txid: &Txid,
+                _block_hash: Option<&BlockHash>,
+            ) -> Result<Transaction, Error> {
+                Ok(self.served.clone())
+            }
+        }


nit: since all RpcApi methods default to call, the mock could implement only call and match on the command name. Reads like a table and gets rid of the unreachable!()

fn call<T: for<'a> serde::de::Deserialize<'a>>( &self, cmd: &str, _args: &[serde_json::Value], ) -> Result<T, Error> { let value = match cmd { "getblockcount" => serde_json::json!(100), "getblockhash" => serde_json::to_value(self.tip_hash)?, "getrawmempool" => serde_json::to_value([self.announced])?, "getrawtransaction" => serialize_hex(&self.served).into(), _ => unimplemented!("unexpected RPC call {cmd}"), }; Ok(serde_json::from_value(value)?) }

should need use bitcoin::consensus::encode::serialize_hex; in the test module

Dmenec · 2026-09-22T07:40:57Z

+serde = "1"
+serde_json = "1"


nit: these deps aren't needed, bitcoincore_rpc already re-exports both. In the test module this should be enough

use bitcoincore_rpc::jsonrpc::{serde, serde_json};

tvpeter requested a review from evanlinjin as a code owner September 16, 2026 07:45

github-project-automation Bot added this to BDK Chain Sep 16, 2026

tvpeter changed the title ~~fix(rpc): verify fetched mempool tx txids and document trust assumption~~ fix(rpc): verify fetched mempool txids before caching and document trust assumption Sep 16, 2026

tvpeter mentioned this pull request Sep 16, 2026

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

Closed

6 tasks

tvpeter requested a review from notmandatory September 16, 2026 07:51

tvpeter requested a review from oleonardolima September 16, 2026 07:51

tvpeter force-pushed the fix/rpc-verify-mempool-txid branch from 2141610 to 3fba250 Compare September 18, 2026 12:10

Dmenec reviewed Sep 22, 2026

View reviewed changes

Navigation Menu

fix(rpc): verify fetched mempool txids before caching and document trust assumption - #2316

fix(rpc): verify fetched mempool txids before caching and document trust assumption#2316
tvpeter wants to merge 1 commit into
bitcoindevkit:masterfrom
tvpeter:fix/rpc-verify-mempool-txid

tvpeter commented Sep 16, 2026

Uh oh!

codecov Bot commented Sep 16, 2026 •

edited

Loading

Uh oh!

tvpeter commented Sep 16, 2026

Uh oh!

evanlinjin commented Sep 18, 2026

Uh oh!

Dmenec left a comment

Uh oh!

Dmenec Sep 21, 2026

Uh oh!

Dmenec Sep 22, 2026

Uh oh!

Dmenec Sep 22, 2026

Uh oh!

Dmenec Sep 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	pub fn next_block(&mut self) -> Result<Option<BlockEvent<Block>>, bitcoincore_rpc::Error> {
	if let Some((checkpoint, block)) = poll(self, move \|hash, client\| client.get_block(hash))? {
	// Stop tracking unconfirmed transactions that have been confirmed in this block.
	for tx in &block.txdata {
	self.mempool_snapshot.remove(&tx.compute_txid());
	}
	return Ok(Some(BlockEvent { block, checkpoint }));
	}
	Ok(None)
	}

	//A fetched mempool tx whose body does not match the requested txid is rejected.
	// A fetched mempool tx whose body does not match the requested txid is rejected.

		serde = "1"
		serde_json = "1"

Conversation

tvpeter commented Sep 16, 2026

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

Bugfixes:

Uh oh!

codecov Bot commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tvpeter commented Sep 16, 2026

Uh oh!

evanlinjin commented Sep 18, 2026

Uh oh!

Dmenec left a comment

Choose a reason for hiding this comment

Uh oh!

Dmenec Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

Dmenec Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

Dmenec Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

Dmenec Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Sep 16, 2026 •

edited

Loading