Enhance onchain transaction management by Camillarhi · Pull Request #628 · lightningdevkit/ldk-node

Camillarhi · 2025-08-23T16:45:37Z

This PR enhances on-chain transaction management:

Rebroadcast/bumping of on-chain wallet transactions
Handle RBF'd Pending payments

Changes

Added background job for rebroadcasting unconfirmed onchain transactions with max attempt limit
Implemented RBF (Replace-by-Fee) functionality allowing users to bump fees on outbound unconfirmed transactions

Related Issues

Fixes Rebroadcast/bumping of on-chain wallet transactions #367

ldk-reviews-bot · 2025-08-23T16:45:40Z

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

ldk-reviews-bot · 2025-08-25T16:56:52Z

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

ldk-reviews-bot · 2025-08-27T16:57:27Z

🔔 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-08-30T00:00:58Z

🔔 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-09-01T00:01:42Z

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

tnull

Just took an initial look and added a few high-level comments. I have yet to review any of the actual RBF logic changes.

However, as noted elsewhere, please don't make all the changes as single huge commit, but rather break the PR up in logical commits that all have descriptive commit messages outlining what the change is, why it's necessary, etc. For guidance you can have a look at https://cbea.ms/git-commit/

It would also make sense to not include the changes for #452 in this initial PR directly, but do it in a separate PR to keep the diff more manageable and reviewable.

bindings/ldk_node.udl

src/payment/onchain.rs

src/payment/store.rs

moisesPompilio · 2025-09-05T02:36:39Z

Thanks for the PR.
It might be better to split this into at least two commits, one for rebroadcasting and one for RBF, to improve organization and reviewability.

moisesPompilio · 2025-09-05T02:28:37Z

src/wallet/mod.rs

+		self.payment_store.remove(&payment_id)?;
+
+		self.payment_store.insert_or_update(payment_details)?;


I'm not fully sure about this flow: since with RBF we can’t guarantee which transaction will eventually confirm (usually the last one, but not always), removing the previous payment here might cause issues. What happens if the earlier tx ends up confirming instead of the latest — would the wallet balance be deducted while the store still shows the payment as pending? If so, could this lead to a confusing UX for the user?

Excellent catch. You're right to be concerned about the potential for state inconsistency.

However, the update_payment_store method acts as a reconciler that syncs the payment store with the actual state of the BDK wallet. Since the payment store is designed to be a reflection of the wallet's state.

We might potentially still end up with multiple entries for the same payment though, right, as we'd not drop the RBF entry once the original transaction confirms? There would also be no 'history' of all the bumps that happened.

Yes, having multiple entries is a possibility. Regarding dropping the entry once the original confirms, that's what I plan on doing in this issue #452 .

As for the history of bumps, a solution could be introducing another status replaced with a replacement ID that indicates the transaction was bumped and replaced.

I think one way to simplify tracking RBF transactions is to add two fields to each payment: is_rbf (a boolean indicating whether the transaction is part of an RBF sequence) and origin_payment_id (optional, pointing to the original payment in the sequence). The original transaction would have is_rbf = true and origin_payment_id = None. Each RBF attempt would also have is_rbf = true and set origin_payment_id to the original transaction’s ID. If a new RBF is created from an existing RBF, it can reuse the same origin_payment_id, linking it back to the original.

With this setup, handling confirmations becomes straightforward:

If an RBF transaction confirms, find the original transaction via origin_payment_id and clean up all other RBFs linked to it.

When a transaction with is_rbf = true and origin_payment_id = None (i.e., an original transaction) confirms, find all RBF transactions with origin_payment_id equal to its ID and remove them.

This keeps data consistent and ensures that extra lookups are only needed when is_rbf is true, making RBF handling explicit and efficient.

For example, with A as the original transaction and B, C, D as RBFs:

graph TD A[A is_rbf=true origin=None] B[B is_rbf=true origin=A.payment_id] C[C is_rbf=true origin=A.payment_id] D[D is_rbf=true origin=A.payment_id] A --> B A --> C A --> D

Loading

I think there’s a design consideration for transactions that won’t confirm because an RBF replaced them. When an RBF transaction confirms, the original and any other RBFs in the sequence become invalid. In my opinion, a more user-friendly approach is to mark them as failed and reference the txid of the confirming transaction, which makes it clear why these transactions didn’t confirm while keeping the history intact.

Thank you for this feedback. This aligns well with the introduction of a replacement id as mentioned above.

Regarding retaining the history of bumped transactions and marking them as failed when any confirms, this could lead to increasing clutter of failed transactions if RBF is used frequently.

My proposed approach during node sync and store updates is when a confirmed transaction is encountered, if it's an RBF transaction, we can confirm that transaction and clear/remove the history of bumps from the store. This prevents unnecessary accumulation of failed transaction records while maintaining proper transaction state tracking.

Camillarhi · 2025-09-05T13:19:14Z

Thanks for the PR. It might be better to split this into at least two commits, one for rebroadcasting and one for RBF, to improve organization and reviewability.

Thanks for the review! I've split the PR into two separate commits as suggested

ldk-reviews-bot · 2025-09-06T00:00:58Z

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

ldk-reviews-bot · 2025-09-08T00:01:42Z

🔔 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-09-10T00:02:19Z

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

tnull

Did another pass and added some comments. Still have to take an even closer look at the RBF logic and think through edge cases.

src/config.rs

src/builder.rs

src/config.rs

src/lib.rs

src/payment/onchain.rs

src/payment/store.rs

src/wallet/mod.rs

tnull · 2025-09-10T12:38:22Z

src/wallet/mod.rs

+		self.payment_store.remove(&payment_id)?;
+
+		self.payment_store.insert_or_update(payment_details)?;


We might potentially still end up with multiple entries for the same payment though, right, as we'd not drop the RBF entry once the original transaction confirms? There would also be no 'history' of all the bumps that happened.

ldk-reviews-bot · 2025-09-20T00:00:40Z

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

ldk-reviews-bot · 2025-09-22T00:01:50Z

🔔 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-09-24T00:02:44Z

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

tnull

@Camillarhi Excuse the delay here - last few weeks have been super busy pushing towards an LDK 0.2 beta release. Will see to get back to review on this in a proper ASAP, it might take another week or so though.

In the meantime I want to note that recently there has been some movement towards #446 as BDK now added events (see bitcoindevkit/bdk_wallet#6 / bitcoindevkit/bdk_wallet#310) which will ship as part of the next BDK 2.2 release. So I do wonder if we should generally first move to update our payment store based on the events emitted from syncing (also allowing for #448 to land), and then basing the changes here on top. This might be in particular interesting as bitcoindevkit/bdk_wallet#310 included a TxReplaced event which might allow us to detect and track (all?) RBF transactions belonging to a 'payment' more easily.

Any thoughts on this?

ldk-reviews-bot · 2026-02-02T00:00:43Z

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

tnull

Thanks for the rebase, here are some updated comments.

src/wallet/mod.rs

src/config.rs

src/payment/pending_payment_store.rs

src/wallet/mod.rs

src/payment/pending_payment_store.rs

src/wallet/mod.rs

tests/integration_tests_rust.rs

tnull

Let me know when this is ready for the next round of review. Unfortunately seems to need yet-another rebase already. Sorry!

Camillarhi · 2026-02-03T13:36:51Z

Let me know when this is ready for the next round of review. Unfortunately seems to need yet-another rebase already. Sorry!

Sure, I will.

ldk-reviews-bot · 2026-02-07T00:00:06Z

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

ldk-reviews-bot · 2026-02-09T00:00:42Z

🔔 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 · 2026-02-11T00:01:07Z

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

tnull

Already looks pretty good, some comments, and it will need a rebase now that the broadcaster interface changed.

tnull · 2026-02-12T11:31:28Z

src/wallet/mod.rs

 	Address, Amount, FeeRate, OutPoint, ScriptBuf, Transaction, TxOut, Txid, WPubkeyHash, Weight,
 	WitnessProgram, WitnessVersion,
 };
+


nit: Changes like this are unrelated and in this case will be reverted by the weekly cargo +nightly fmt job anyways.

tnull · 2026-02-12T11:32:42Z

src/wallet/mod.rs

+					// Get all on-chain payments that are Pending
 					let pending_payments: Vec<PendingPaymentDetails> =
 						self.pending_payment_store.list_filter(|p| {
 							p.details.status == PaymentStatus::Pending


Hmm, wouldn't we expect all in the Pending store to be pending? In fact, should we add a debug assert in 1-2 places that ensures any entries are marked pending, and any other entries are removed from the pending-payment store?

Yes, this is true. All records in PendingStore should be marked as pending. I will also add the debug assert

tnull · 2026-02-12T11:47:16Z

src/wallet/mod.rs

+
+						if !txs_to_broadcast.is_empty() {
+							let tx_refs: Vec<&Transaction> = txs_to_broadcast.iter().collect();
+							self.broadcaster.broadcast_transactions(&tx_refs);


This will need a rebase now as the BroadcasterInterface changed on main (note that the 'real' introduction of TransactionType etc will be in #79).

Sure, I will rebase. Also, did you mean #791 ?

Also, did you mean #791 ?

Ah, yes, indeed!

tnull · 2026-02-12T11:49:47Z

src/wallet/mod.rs

-						ConfirmationStatus::Unconfirmed,
-					);
+					// We fetch payment details here since the replacement has updated the stored state
+					let payment =


Hmm, but that would mean we expect this get to always return Some at this point, right? Can we add a debug_assert checking that it does?

Hmm seems we hitting some of the new debug_asserts in test_rbf_via_direct_block_insertion:

Payment b25fa25838fc1654906457e86846d380996624ff8750c4910049be4350710f1c expected in store during WalletEvent::TxReplaced but not found

Might be good to understand what's happening here. Possibly some of the fixes in #769 could also be related, but not sure.

Thanks, I will check out what's happening

tnull · 2026-02-12T11:51:13Z

src/wallet/mod.rs

+		};
+
+		let old_tx =
+			locked_wallet.tx_details(txid).ok_or(Error::InvalidPaymentId)?.tx.deref().clone();


Hmm, same here, we expect this to never fail - can we add a debug_assert?

tnull · 2026-02-12T11:53:08Z

src/wallet/mod.rs

+		let min_required_fee_rate_sat_per_kwu =
+			old_fee_rate_sat_per_kwu + INCREMENTAL_RELAY_FEE_SAT_PER_1000_WEIGHT as u64;
+
+		let confirmation_target = ConfirmationTarget::OnchainPayment;


See above, we might want to allow for an override target fee rate like we do for send_to_address etc?

Yes, that is fine, I will go ahead and add that

tnull · 2026-02-12T11:53:29Z

src/payment/onchain.rs

+	/// higher fee, resulting in faster confirmation potential.
+	///
+	/// Returns the Txid of the new replacement transaction if successful.
+	pub fn bump_fee_rbf(&self, payment_id: PaymentId) -> Result<Txid, Error> {


Do we need an optional fee-rate override parameter here, similar to what we have in send_to_address?

Yes, I will go ahead and include it

tnull · 2026-02-12T11:56:30Z

src/wallet/mod.rs

+						Error::InvalidFeeRate
+					})?;
+
+					builder.fee_rate(required_fee_rate);


Hmm, should we apply an upper-bound to the require_fee_rate, i.e., ensure it's still in some margin of what we expected? Maybe we even want to apply some upper-bound (relative to the original rate?) to our estimate to ensure we don't spend all our funds on fees if the chain source's fee estimation endpoint is lying to us?

Yeah, I thought about this too, that's why I computed a min_required_fee_rate_sat_per_kwu above:

let min_required_fee_rate_sat_per_kwu = old_fee_rate_sat_per_kwu + INCREMENTAL_RELAY_FEE_SAT_PER_1000_WEIGHT as u64;

Then take the max of that and the estimated_fee_rate. Based on the tests so far, the final_fee_rate is what ends up being used in most cases, the difference between it and required_fee_rate has been around 2-3 sat/kwu. I think it'd be pretty rare for the chain source estimate to actually win out, but an upper-bound sanity check on it is reasonable to add

Based on the tests so far, the final_fee_rate is what ends up being used in most cases, the difference between it and required_fee_rate has been around 2-3 sat/kwu. I think it'd be pretty rare for the chain source estimate to actually win out, but an upper-bound sanity check on it is reasonable to add

Did you do tests on mainnet as well? Note that testing such things on regtest or even testnet/signet will lead to bogus values.

Yes, this is correct. I will introduce an upper-bound sanity check.

@tnull, I introduced a sanity check at 1.5x our target fee rate to avoid overpaying

ldk-reviews-bot · 2026-02-14T15:46:56Z

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

tnull

Seems CI is unhappy/hanging due to some of the debug_asserts getting hit now.

tnull · 2026-02-16T17:39:25Z

Might also need a rebase now after #769 landed.

Add automatic rebroadcasting of unconfirmed transactions triggered by the `ChainTipChanged` event from BDK. This ensures pending transactions remain in mempools.

Add `Replace-by-Fee` functionality to allow users to increase fees on pending outbound transactions, improving confirmation likelihood during network congestion. - Uses BDK's `build_fee_bump` for transaction replacement - Validates transaction eligibility: must be outbound and unconfirmed - Maintains payment history consistency across wallet updates - Includes integration tests for various RBF scenarios

Camillarhi · 2026-02-17T17:00:38Z

Might also need a rebase now after #769 landed.

@tnull , this is ready for another round of review.

ldk-reviews-bot requested a review from tnull August 23, 2025 16:56

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch from 39922cc to c228760 Compare August 23, 2025 16:58

Camillarhi mentioned this pull request Aug 26, 2025

Figure out rebroadcasting / RBF strategy #22

Open

tnull reviewed Sep 1, 2025

View reviewed changes

bindings/ldk_node.udl Outdated Show resolved Hide resolved

bindings/ldk_node.udl Outdated Show resolved Hide resolved

src/payment/onchain.rs Outdated Show resolved Hide resolved

src/payment/onchain.rs Outdated Show resolved Hide resolved

src/payment/store.rs Outdated Show resolved Hide resolved

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch 3 times, most recently from a52b1e5 to 3523954 Compare September 3, 2025 22:44

Camillarhi requested a review from tnull September 3, 2025 22:50

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch from 3523954 to 609d0a0 Compare September 3, 2025 22:53

moisesPompilio reviewed Sep 5, 2025

View reviewed changes

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch from 609d0a0 to 8b7cf78 Compare September 5, 2025 13:17

tnull reviewed Sep 10, 2025

View reviewed changes

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch 2 times, most recently from 06e7ff3 to 6e6e2aa Compare September 11, 2025 23:09

tnull self-requested a review September 17, 2025 11:11

moisesPompilio mentioned this pull request Sep 23, 2025

Fix Invalid Pending Payments and Strengthen RBF Tests #647

Closed

tnull reviewed Sep 26, 2025

View reviewed changes

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch from 8ec7391 to aa1558a Compare January 30, 2026 09:12

Camillarhi requested a review from tnull January 30, 2026 09:13

tnull reviewed Feb 2, 2026

View reviewed changes

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch from aa1558a to 8631a4f Compare February 3, 2026 01:22

tnull reviewed Feb 3, 2026

View reviewed changes

Camillarhi mentioned this pull request Feb 3, 2026

direct_conflicts should guarantee ordering with most recent conflict last bitcoindevkit/bdk#2112

Open

11 tasks

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch 2 times, most recently from 8dd9382 to 764538a Compare February 4, 2026 10:38

Camillarhi requested a review from tnull February 4, 2026 10:39

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch from 764538a to 00de553 Compare February 9, 2026 10:35

tnull reviewed Feb 12, 2026

View reviewed changes

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch from 00de553 to b5b191b Compare February 12, 2026 15:44

Camillarhi requested a review from tnull February 12, 2026 15:46

tnull reviewed Feb 16, 2026

View reviewed changes

Rebroadcast unconfirmed transactions on each new block

38f9a87

Add automatic rebroadcasting of unconfirmed transactions triggered by the `ChainTipChanged` event from BDK. This ensures pending transactions remain in mempools.

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch 5 times, most recently from d22d4bc to b0b40b1 Compare February 17, 2026 16:14

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch from b0b40b1 to dc53ac5 Compare February 17, 2026 16:15

		self.payment_store.remove(&payment_id)?;

		self.payment_store.insert_or_update(payment_details)?;

Conversation

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

Uh oh!

Changes

Related Issues

Uh oh!

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

Uh oh!

Uh oh!

ldk-reviews-bot commented Aug 25, 2025

Uh oh!

ldk-reviews-bot commented Aug 27, 2025

Uh oh!

ldk-reviews-bot commented Aug 30, 2025

Uh oh!

ldk-reviews-bot commented Sep 1, 2025

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moisesPompilio commented Sep 5, 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!

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!

Camillarhi commented Sep 5, 2025

Uh oh!

ldk-reviews-bot commented Sep 6, 2025

Uh oh!

ldk-reviews-bot commented Sep 8, 2025

Uh oh!

ldk-reviews-bot commented Sep 10, 2025

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Sep 20, 2025

Uh oh!

ldk-reviews-bot commented Sep 22, 2025

Uh oh!

ldk-reviews-bot commented Sep 24, 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!

ldk-reviews-bot commented Feb 2, 2026

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Camillarhi commented Aug 23, 2025 •

edited

Loading

ldk-reviews-bot commented Aug 23, 2025 •

edited

Loading

tnull left a comment •

edited

Loading