Fix evicted transation #605

moisesPompilio · 2025-08-08T20:05:40Z

This PR fixes an issue where BitcoinD failed to detect when a transaction was removed from the mempool, causing inconsistencies compared to Electrum and Esplora.

The root cause was an endianness mismatch in the evicted_txids check: one txid was in little-endian and the other in big-endian, making proper matching nearly impossible. The fix ensures consistent txid comparison by normalizing endianness.

Additionally, this PR adds an RBF functional test to verify that the node correctly:

Detects a Replace-By-Fee event.
Replaces the original transaction in the mempool.
Uses the updated transaction to open a channel and allocate the correct balance to the user.

ldk-reviews-bot · 2025-08-08T20:05:43Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

tnull

Oh, good catch! I think we should fix this right at deserialization time though.

tnull · 2025-08-11T13:16:51Z

src/chain/bitcoind.rs

@@ -1121,7 +1122,12 @@ impl BitcoindClient {
 		let mempool_entries_cache = mempool_entries_cache.lock().await;
 		let evicted_txids = unconfirmed_txids
 			.into_iter()
-			.filter(|txid| mempool_entries_cache.contains_key(txid))
+			.filter(|txid| {
+				let mut bytes = txid.to_byte_array();


I don't think we should fix this here, but when we first parse in the Txid, i.e., before we even add it to the cache.

While you're doing this, mind adding a few end-to-end tests for all the JsonResponse de/serialization, i.e., asserting that we get the same objects when we encode and decode GetRawMempoolResponse,GetMempoolEntryResponse, etc. These might also good candidates for simple proptests.

Got it, thanks! I’ll make those changes and add the tests as suggested.

tnull

I think this branch also needs a rebase by now.

moisesPompilio · 2025-08-14T20:41:24Z

I updated the implementation to replace bitcoin::consensus::encode::deserialize_hex and bitcoin::consensus::encode::serialize_hex with Txid::from_str and Txid::to_string, aligning the behavior with the official rust-bitcoin Txid documentation, where fmt::Display outputs the reversed-byte hex string expected by RPC. I also modified the logic for removing unconfirmed transactions so that when a transaction is no longer in the mempool, it is marked as evicted and removed from the node's balance count if it has not been included in a block. I think this change may solve issue #534, although I’m not entirely certain.

I also added property-based tests to ensure that JsonResponse serialization and deserialization are fully consistent for GetRawMempoolResponse, GetMempoolEntryResponse, GetRawTransactionResponse, FeeResponse, and MempoolMinFeeResponse. These round-trip checks guarantee that encoded data can be decoded back into identical Rust objects.

Additionally, I expanded the RBF functional tests to cover two scenarios:

RBF transactions in the mempool before confirmation — ensuring the node detects the replacement, updates balances, and removes outdated outputs.
RBF transactions confirmed directly in a block — verifying that the node correctly marks the replacement as confirmed and removes the obsolete unconfirmed transaction.

For this, I introduced distribute_funds_unconfirmed to distribute funds without confirming them, and MultiNodeTestSetup to simplify multi-node orchestration, wallet synchronization, balance validation, RBF preparation, and fee bumping.

tnull

Thanks for catching this!

Changes themselves look mostly good, but I'd prefer to keep the test setup more minimal, as all this boilerplate doesn't provide any real benefit, and the complexity renders CI pretty AFAICT.

tnull · 2025-08-15T10:59:54Z

src/chain/bitcoind.rs

@@ -1109,7 +1109,7 @@ impl BitcoindClient {
 		let mempool_entries_cache = mempool_entries_cache.lock().await;
 		let evicted_txids = unconfirmed_txids
 			.into_iter()
-			.filter(|txid| mempool_entries_cache.contains_key(txid))
+			.filter(|txid| !mempool_entries_cache.contains_key(txid))


Aaah, good catch. Mind renaming unconfirmed_txids to bdk_mempool_txids or bdk_unconfirmed_txids. I think a bit cleaner naming would have likely avoided that bug in the first place.

tnull · 2025-08-15T11:18:03Z

src/chain/bitcoind.rs

@@ -754,7 +754,7 @@ impl BitcoindClient {
 	async fn get_raw_transaction_rpc(
 		rpc_client: Arc<RpcClient>, txid: &Txid,
 	) -> std::io::Result<Option<Transaction>> {
-		let txid_hex = bitcoin::consensus::encode::serialize_hex(txid);
+		let txid_hex = txid.to_string();


Ugh, classic mistake. Thanks for catching this.

tnull · 2025-08-15T11:27:26Z

tests/common/mod.rs

@@ -1074,6 +1110,170 @@ pub(crate) fn do_channel_full_cycle<E: ElectrumApi>(
 	println!("\nB stopped");
 }

+pub(crate) struct MultiNodeTestSetup {


This is a lot of boilerplate, and would use a lot of CI resources for no added benefit. I think it would be preferable if the test_rbf tests would just reuse the existing test utilities. FWIW, to test RBF works as expected you'll only need to setup a single node (maybe one per chain source, as we do for the full_cycle) test, i.e., should just be able to use setup_node.

Yeah, I initially made that struct for a different approach, but it turned out unnecessary. I removed it and now use one node per chain type, keeping the test lighter.

- Replace `bitcoin::consensus::encode::deserialize_hex()` with `hex_str.parse::<Txid>()` when parsing Txids from RPC, and `serialize_hex()` with `txid.to_string()` when sending to RPC, ensuring proper handling of Bitcoin Core's reversed-byte hexadecimal format. - Fix mempool eviction logic: transactions that are no longer in the mempool are now correctly removed from wallet consideration, preventing stale pending transactions from inflating unconfirmed balances. - Refactor `unconfirmed_txids` to `bdk_unconfirmed_txids` to make it easier to identify what are unconfirmed bdk transactions

- Validate roundtrip serialization/deserialization for Txids, transactions, mempool entries, and fee responses. - Ensure Txid parsing/serialization matches Bitcoin Core RPC expectations.

- Test mempool-only RBF handling and balance adjustments. - Test RBF transactions confirmed in block, ensuring stale unconfirmed txs are removed. - Introduce `distribute_funds_unconfirmed` for creating unconfirmed outputs.

moisesPompilio force-pushed the fix-evicted-transation branch from de9c632 to 0a9bee3 Compare August 9, 2025 16:08

tnull reviewed Aug 11, 2025

View reviewed changes

moisesPompilio marked this pull request as ready for review August 11, 2025 14:42

ldk-reviews-bot requested a review from joostjager August 11, 2025 14:42

tnull requested review from tnull and removed request for joostjager August 11, 2025 18:28

tnull reviewed Aug 13, 2025

View reviewed changes

moisesPompilio force-pushed the fix-evicted-transation branch from 0a9bee3 to 5961e27 Compare August 14, 2025 20:14

tnull reviewed Aug 15, 2025

View reviewed changes

moisesPompilio force-pushed the fix-evicted-transation branch 2 times, most recently from 94b240f to f2cf691 Compare August 15, 2025 22:56

moisesPompilio added 3 commits August 15, 2025 19:57

Add property-based tests for RPC JSON responses

98ca264

- Validate roundtrip serialization/deserialization for Txids, transactions, mempool entries, and fee responses. - Ensure Txid parsing/serialization matches Bitcoin Core RPC expectations.

Add RBF integration tests with multi-node setup

f9e0767

- Test mempool-only RBF handling and balance adjustments. - Test RBF transactions confirmed in block, ensuring stale unconfirmed txs are removed. - Introduce `distribute_funds_unconfirmed` for creating unconfirmed outputs.

moisesPompilio force-pushed the fix-evicted-transation branch from f2cf691 to f9e0767 Compare August 15, 2025 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix evicted transation #605

Fix evicted transation #605

Uh oh!

moisesPompilio commented Aug 8, 2025

Uh oh!

ldk-reviews-bot commented Aug 8, 2025 •

edited

Loading

Uh oh!

tnull left a comment

Uh oh!

tnull Aug 11, 2025 •

edited

Loading

Uh oh!

moisesPompilio Aug 11, 2025

Uh oh!

tnull left a comment

Uh oh!

moisesPompilio commented Aug 14, 2025

Uh oh!

tnull left a comment •

edited

Loading

Uh oh!

tnull Aug 15, 2025

Uh oh!

moisesPompilio Aug 15, 2025

Uh oh!

tnull Aug 15, 2025

Uh oh!

tnull Aug 15, 2025

Uh oh!

moisesPompilio Aug 15, 2025

Uh oh!

Uh oh!

Fix evicted transation #605

Are you sure you want to change the base?

Fix evicted transation #605

Uh oh!

Conversation

moisesPompilio commented Aug 8, 2025

Uh oh!

ldk-reviews-bot commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

tnull Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moisesPompilio Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

moisesPompilio commented Aug 14, 2025

Uh oh!

tnull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

moisesPompilio Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

tnull Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

tnull Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

moisesPompilio Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ldk-reviews-bot commented Aug 8, 2025 •

edited

Loading

tnull Aug 11, 2025 •

edited

Loading

tnull left a comment •

edited

Loading