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.

…ed clarity

…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

…oval Pending payments that become invalid or are removed from the mempool are now marked as failed. RBF tests were updated to check that only confirmed transactions succeed and all others are failed.only the actually confirmed transactions have the correct

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?

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?

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

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 added 4 commits October 7, 2025 16:09

nit: Refactor node setup in tests to use new_node function for improv…

4eb8745

…ed clarity

Add property-based reorg test for RBF transactions

fe805cf

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 569cebe to c635cb4 Compare October 7, 2025 19:38

tnull reviewed Oct 9, 2025

View reviewed changes

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!

Uh oh!

ldk-reviews-bot commented Sep 23, 2025 •

edited

Loading