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

 			.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

 		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

 		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

 	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.

tnull

This needs a rebase by now.

- 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

tnull · 2025-08-18T13:06:36Z

src/chain/bitcoind.rs

+	}
+
+	proptest! {
+		#![proptest_config(proptest::test_runner::Config::with_cases(250))]


While good for test coverage initially, 250 seems like a high number if we run it every time we push to CI, no? Maybe 10 or 20 seems more reasonable?

Good point. Reducing from 250 to 20 only saves a few milliseconds, but I agree 250 is excessive for CI. 20 runs seems more reasonable for these scenarios.

tnull · 2025-08-18T13:09:44Z

tests/integration_tests_rust.rs

+				match bitcoind.send_raw_transaction(&tx) {
+					Ok(res) => {
+						// Mine a block immediately so the transaction is confirmed
+						// before any node identifies it as a transaction that was in the mempool.


Hmm, depending on the chain source that might be race-y, no? In particular, for bitcoind RPC we might happen to poll the mempool just before the block gets mined?

Yes, race conditions are possible, but they don’t affect the test outcome. When is_insert_block is true, there are three possible scenarios:

When the node checks the mempool, it sees the RBF transaction. The old transaction is removed to allow the RBF. I haven’t triggered this in my tests, but it wouldn’t break anything.

When the node checks the mempool, it finds it empty, either because the transaction is already in a block or the block is being processed. Old unconfirmed transactions are removed and the correct transaction is picked from the block. This scenario is rare but safe.

When the node checks confirmed transactions in the block, it sees the RBF transaction already included. The old unconfirmed transaction is removed. This is the most common scenario in the test and effectively tests the case where the node first encounters the RBF transaction via the block rather than the mempool.

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

tnull

Mostly looks good, some minor comments.

tnull · 2025-08-19T12:15:55Z

tests/integration_tests_rust.rs

+				}
+			}
+			fee_output_index = option_fee_output_index.expect(
+				"No output available for fee pumping. Need at least one output not being modified.",


nit: bump?

tnull · 2025-08-19T12:16:06Z

tests/integration_tests_rust.rs

+					},
+					Err(_) => {
+						if _attempt == attempts - 1 {
+							panic!("Failed to pump fee after {} attempts", attempts);


nit: bump?

tnull · 2025-08-19T12:19:00Z

tests/integration_tests_rust.rs

+		nodes.iter().map(|node| node.onchain_payment().new_address().unwrap()).collect::<Vec<_>>();
+	let amount_sat = 2_100_000;
+	let mut txid;
+	macro_rules! distribute_funds_all_nodes {


Can we make all these helpers methods rather than macros? Macros have serveral drawbacks, in particular they're much harder to debug in case of a failure as the compiler won't output the correct line numbers then.

Yes, though I think distribute_funds_all_nodes and validate_balances can stay as macros. The first just calls a function, and in the second the main failure point is assert_eq!, so keeping them as macro_rules! helps keep the helper functions cleaner.

Hmm I would generally prefer to inline them or use methods, but won't hold this PR up over it.

- 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.

tnull

Thanks again! Landing this!

tnull · 2025-08-20T09:20:14Z

tests/integration_tests_rust.rs

+		nodes.iter().map(|node| node.onchain_payment().new_address().unwrap()).collect::<Vec<_>>();
+	let amount_sat = 2_100_000;
+	let mut txid;
+	macro_rules! distribute_funds_all_nodes {


Hmm I would generally prefer to inline them or use methods, but won't hold this PR up over it.

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 3 times, most recently from f2cf691 to f9e0767 Compare August 15, 2025 23:00

tnull self-requested a review August 18, 2025 10:59

tnull reviewed Aug 18, 2025

View reviewed changes

moisesPompilio force-pushed the fix-evicted-transation branch from f9e0767 to d28e051 Compare August 18, 2025 12:37

tnull reviewed Aug 18, 2025

View reviewed changes

Add property-based tests for RPC JSON responses

5fe06c8

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

moisesPompilio force-pushed the fix-evicted-transation branch from d28e051 to 456b3ab Compare August 18, 2025 16:07

tnull self-requested a review August 19, 2025 12:06

tnull reviewed Aug 19, 2025

View reviewed changes

Add RBF integration tests with multi-node setup

f3aab90

- 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 456b3ab to f3aab90 Compare August 19, 2025 13:46

tnull approved these changes Aug 20, 2025

View reviewed changes

tnull merged commit 947acf3 into lightningdevkit:main Aug 20, 2025
15 checks passed

Fix evicted transation #605

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

tnull Aug 18, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

ldk-reviews-bot commented Aug 8, 2025 •

edited

Loading

tnull Aug 11, 2025 •

edited

Loading

tnull left a comment •

edited

Loading

tnull Aug 18, 2025 •

edited

Loading