Skip to content

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

Open
tvpeter wants to merge 1 commit into
bitcoindevkit:masterfrom
tvpeter:fix/rpc-verify-mempool-txid
Open

tvpeter wants to merge 1 commit into
bitcoindevkit:masterfrom
tvpeter:fix/rpc-verify-mempool-txid

Conversation

@tvpeter

@tvpeter tvpeter commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

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:

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

@tvpeter
tvpeter requested a review from evanlinjin as a code owner September 16, 2026 07:45
@tvpeter 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
@codecov

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

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

tvpeter commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author

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

Copy link
Copy Markdown
Member

@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.
@tvpeter
tvpeter force-pushed the fix/rpc-verify-mempool-txid branch from 2141610 to 3fba250 Compare September 18, 2026 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants