Fix Invalid Pending Payments and Strengthen RBF Tests #647

moisesPompilio · 2025-09-23T20:15:55Z

Pending payments that are invalidated or removed from the mempool are now marked as failed. Currently, “failed” simply means the transaction is no longer in BDK’s canonical transaction list while still pending in our DB. It could later return to a pending or confirmed state if it reappears. More precise context for these “failed” states will be possible once bdk_wallet#257 is merged, as that adds richer uncanonica transaction status details.

Additional improvements:

Property-based tests simulating multiple RBF attempts, random confirmations, and chain reorgs to ensure correct handling when any RBF transaction confirms.
generate_block_and_insert_transactions to mine blocks with chosen transactions, enabling tests where either the original or a replacement transaction is confirmed.
Refactored node setup in tests with a new_node helper for clarity.

Note: This PR partially addresses issue #452. I believe it does not conflict with PR #628, as this change is more general. It handles any transaction that is no longer canonical in BDK, not just RBF replacements. This fixes the problem where such transactions would remain pending indefinitely due to lack of further updates, by marking them as failed. PR #628, on the other hand, specifically handles RBFs initiated by the node.

ldk-reviews-bot · 2025-09-23T20:15:57Z

I've assigned @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.

ldk-reviews-bot · 2025-09-25T20:27:24Z

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Camillarhi · 2025-09-26T21:16:51Z

src/wallet/mod.rs

 					} else {
 						PaymentStatus::Pending
 					};
+					if payment_status == PaymentStatus::Pending {


Thanks for the PR. Since PaymentStatus::Pending is only assigned in the else branch above, we can move the unconfirmed_txid.push(id) call directly into that block. This avoids the extra comparison afterward and makes the logic a bit more concise and self-contained.

Great! Thanks

ldk-reviews-bot · 2025-09-29T00:00:12Z

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-10-01T00:00:51Z

🔔 3rd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-10-04T00:00:40Z

🔔 4th Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-10-06T00:01:17Z

🔔 5th Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-10-08T00:01:58Z

🔔 6th Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tnull

Excuse the delay here!

tnull · 2025-10-09T07:50:33Z

tests/common/mod.rs

 	(node_a, node_b)
 }

+pub(crate) fn new_node(chain_source: &TestChainSource, anchor_channels: bool) -> TestNode {


The change from the config_node macro to this method makes sense to me, but maybe we want to align names here?

Should this rather be renamed to setup_node, while renaming the current setup_node to setup_node_from_config?

Yes, I agree with you. I’ve made the change.

tnull · 2025-10-09T07:53:16Z

tests/common/mod.rs

-use bitcoin::hashes::hex::FromHex;
 use bitcoin::hashes::sha256::Hash as Sha256;
-use bitcoin::hashes::Hash;
+use bitcoin::hashes::{hex::FromHex, Hash};


nit: Please group imports at the module level.

tnull · 2025-10-09T08:21:17Z

tests/common/mod.rs

+	let address = bitcoind.new_address().expect("failed to get new address");
+
+	let request_block_template =
+		corepc_node::TemplateRequest { rules: vec![electrsd::corepc_node::TemplateRules::Segwit] };


nit: Please avoid the full type qualifiers, rather import the types via use above.

tnull · 2025-10-09T08:27:54Z

tests/common/mod.rs

 	node
 }

+pub(crate) fn generate_block_and_insert_transactions<E: ElectrumApi>(


I wonder if it would be simpler to

Put the RBF into the mempool

Sync the wallet so it registers

Mine a block

Call Bitcoin Core's invalidateblock to discard the tip and the RBF

Put the original TX into the mempool

Sync the wallet again

If I'm not mistaken that would save us all of this boilerplate?

That approach doesn’t seem to work. After the block is mined and then invalidated, the replaced transaction isn’t discarded. I tested this flow, and when the original transaction is reintroduced, it fails because the existing transaction has a higher fee.

That approach doesn’t seem to work. After the block is mined and then invalidated, the replaced transaction isn’t discarded. I tested this flow, and when the original transaction is reintroduced, it fails because the existing transaction has a higher fee.

Interesting, I was under the impression that invalidateblock doesn't put the transactions back into the mempool but would simply discard them. It's likely not working because you use the bitcoind wallet to generate and RBF the transaction, having it track as part of the wallet (rather than 'just' in the mempool).

In any case, the block mining boilerplate is probably fine here then, even though I had hoped it could be simplified.

during a reorg, Bitcoin Core doesn’t drop the transactions but tries to reinsert them into the mempool. I think invalidateblock behaves similarly. So even if a different wallet were used instead of Bitcoin Core’s, the same behavior would likely occur.

tnull · 2025-10-09T08:34:00Z

src/wallet/mod.rs

+			let id = failed_payment.id;
+			let payment_update = PaymentDetailsUpdate {
+				id,
+				status: Some(PaymentStatus::Failed),


Hmm, not convinced we should do this, as on a semantic level there is no such thing as a 'failed' onchain payment. Valid transactions are forever pending as they could always be rebroadcasted even after being evicted from the local mempool. That is why we currently only track them as Pending or Confirmed (once they are irrevocably confirmed).

I understand your point. When a transaction leaves the mempool due to a low fee, it can still be valid and eventually confirmed. The issue arises with RBF, since only one of the conflicting transactions can be confirmed, making the others effectively double-spent and thus “failed”. The challenge is that BDK doesn’t currently provide a way to distinguish between these cases.

My main motivation is that keeping transactions evicted from the mempool marked as Pending can lead to UX confusion, especially with RBF. For example, if a user broadcasts a transaction and then later replaces it via RBF, they would see two onchain payments, one that gets confirmed and another that never will. Leaving the latter as Pending would incorrectly suggest it’s still valid. Maybe having an additional status like Dropped or Evicted could better represent transactions that are no longer canonical in BDK?

The challenge is that BDK doesn’t currently provide a way to distinguish between these cases.

I think the new WalletEvent::TxReplaced should allow us to distinguish the cases and track all 'failed'/RBF'd transactions as part of the same payment entry? That should be tackled as part of #658, IMO.

My main motivation is that keeping transactions evicted from the mempool marked as Pending can lead to UX confusion, especially with RBF. For example, if a user broadcasts a transaction and then later replaces it via RBF, they would see two onchain payments, one that gets confirmed and another that never will. Leaving the latter as Pending would incorrectly suggest it’s still valid. Maybe having an additional status like Dropped or Evicted could better represent transactions that are no longer canonical in BDK?

Right, that's basically #452 which should be addressed by #658. Apart from that, leaving non-RBF'd (but evicted) transactions as Pending is closer to reality and hence IMO preferable as long as we're not 100% they could eventually return and get mined.

I agree that this can be handled in PR #658, so I’ve removed the part that marked non-canonical transactions as failed and kept only the additional tests. I also tested the new event handling and confirmed it correctly differentiates between replaced and dropped transactions, except in one specific case when the RBF transaction changes the outputs and no longer points to the wallet address. In that case, the transaction is marked as dropped instead of replaced.

…nfig Introduce setup_node for clearer node creation. Rename old setup_node to setup_node_from_config for custom configs.

…testing - Introduce `generate_block_and_insert_transactions` to manually mine a block with arbitrary transactions. - Update `bump_fee_and_broadcast` to optionally insert transactions directly into a block (bypassing the mempool) when `is_insert_block` is true. - Add integration test to insert the original (pre-RBF) transaction into a block instead of the RBF, to cover scenarios where the original transaction is confirmed and the RBF is not.

This test simulates multiple RBF transactions, randomly confirms one, then simulates a reorg and confirms another at random. This ensures the wallet correctly handles cases where any RBF transaction may be confirmed after

ldk-reviews-bot requested a review from tnull September 23, 2025 20:26

Camillarhi reviewed Sep 26, 2025

View reviewed changes

moisesPompilio force-pushed the fix-rbf-acumulate-transaction-electrum-esplora branch from 56a50c1 to 569cebe Compare September 26, 2025 21:44

moisesPompilio force-pushed the fix-rbf-acumulate-transaction-electrum-esplora branch from 569cebe to c635cb4 Compare October 7, 2025 19:38

tnull reviewed Oct 9, 2025

View reviewed changes

nit: introduce setup_node and rename setup_node to setup_node_from_co…

cc9f907

…nfig Introduce setup_node for clearer node creation. Rename old setup_node to setup_node_from_config for custom configs.

moisesPompilio force-pushed the fix-rbf-acumulate-transaction-electrum-esplora branch from c635cb4 to ab6990e Compare October 9, 2025 20:41

moisesPompilio added 2 commits October 10, 2025 15:48

Add property-based reorg test for RBF transactions

dd9d92d

This test simulates multiple RBF transactions, randomly confirms one, then simulates a reorg and confirms another at random. This ensures the wallet correctly handles cases where any RBF transaction may be confirmed after

moisesPompilio force-pushed the fix-rbf-acumulate-transaction-electrum-esplora branch from ab6990e to dd9d92d Compare October 10, 2025 18:48

Fix Invalid Pending Payments and Strengthen RBF Tests #647

Are you sure you want to change the base?

Fix Invalid Pending Payments and Strengthen RBF Tests #647

Conversation

moisesPompilio commented Sep 23, 2025

Uh oh!

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

Uh oh!

Uh oh!

ldk-reviews-bot commented Sep 25, 2025

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!

ldk-reviews-bot commented Sep 29, 2025

Uh oh!

ldk-reviews-bot commented Oct 1, 2025

Uh oh!

ldk-reviews-bot commented Oct 4, 2025

Uh oh!

ldk-reviews-bot commented Oct 6, 2025

Uh oh!

ldk-reviews-bot commented Oct 8, 2025

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!

Choose a reason for hiding this comment

Uh oh!

tnull Oct 10, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ldk-reviews-bot commented Sep 23, 2025 •

edited

Loading

tnull Oct 10, 2025 •

edited

Loading