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

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

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?

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

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

src/wallet/mod.rs

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

src/wallet/mod.rs

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

src/payment/onchain.rs

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

Hmm, thanks. So now I still wonder if should ever spend more than the explicit override fee rate set by the user. Generally, maybe this auto-retry logic is already to complicated, and it might be easier to just bubble-up the FeeRateTooLow error and have the user use an explicit override fee rate of their choice to adjust the target upwards if they'd ever hit it?

How about we only do the auto-retry when the user didn't provide an explicit fee rate? If they passed a specific rate and BDK rejects it, we bubble up the error and let them adjust. If they passed None, we handle it internally with the 1.5x sanity cap.

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.

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

Might also need a rebase now after #769 landed.

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

tnull · 2026-02-18T10:53:27Z

src/payment/onchain.rs

I think we can drop The original transaction must be signaling RBF replaceability for this to succeed. as this is given for all of our payments, no?

Yes, we can drop that. All our payments signal RBF, so it's not worth documenting.

tnull · 2026-02-18T10:53:49Z

src/payment/onchain.rs

nit: Tick Txid.

tnull · 2026-02-18T10:56:57Z

src/wallet/mod.rs

I this debug_assert shouldn't be necessary, if anything it might be race-y if our KVStore view is somehow temporarily inconsistent.

tnull · 2026-02-18T11:17:39Z

src/wallet/mod.rs

Ah, it seems we shouldn't fallback here to PaymentId(txid.to_byte_array()) but rather log and skip processing if we can't find the respective payment entry?

Then, not sure if we should use find_payment_by_txid at all if we now assume that the payment entry will be in our store?

Agreed, we should not fall back to PaymentId(txid.to_byte_array()) here. However, we should still use find_payment_by_txid because the txid in TxReplaced is the old replaced txid, find_payment_by_txid looks up the pending_payment_store to map it back to the correct payment id (either directly or via conflicting_txids). If no payment id is found, we should log and skip processing.

tnull · 2026-02-18T11:22:15Z

src/wallet/mod.rs

 					self.pending_payment_store.insert_or_update(pending_payment)?;
 				},
-				WalletEvent::TxReplaced { txid, conflicts, tx, .. } => {
+				WalletEvent::TxReplaced { txid, conflicts, .. } => {


I do wonder if this also handles the case of inbound replaced transactions correctly?

Yes, the logic handles inbound transactions correctly as well. I will also expand the integration test to verify the inbound side.

tnull · 2026-02-18T11:24:16Z

src/wallet/mod.rs

I'm confused by this comment: what logic exactly is this referring to - where does the replacement update the store, i.e., why can we be sure it has been updated here? It seems our processing order would ensure we always process this logic first, not TxUnconfirmed, if this is what this comment is referring to?

The comment refers to bump_fee_rbf , It updates the payment store with the new replacement txId before the next wallet sync. So by the time the sync runs and emits TxReplaced, the payment is already updated in the store. I'll reword the comment to make this clearer.

Ah, but this really only holds if the replacement was initiated by us then. How do we handle replacements for inbound payments for example?

For inbound replacements, since we already have the unconfirmed transaction in the store, it relies on using find_payment_by_txid to look up the PaymentID by matching with the txid or conflicts and updates the payment.

tnull · 2026-02-18T11:25:23Z

src/wallet/mod.rs

This probably should use a different error variant.

tnull · 2026-02-18T11:26:34Z

src/wallet/mod.rs

Please try to always log when returning an error, also in this instance.

tnull · 2026-02-18T11:27:26Z

src/wallet/mod.rs

I think we can drop the intermediate variable here, as old_fee_rate.to_sat_per_kwu() is basically same length.

tnull · 2026-02-18T11:33:50Z

src/wallet/mod.rs

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


Hmm, thanks. So now I still wonder if should ever spend more than the explicit override fee rate set by the user. Generally, maybe this auto-retry logic is already to complicated, and it might be easier to just bubble-up the FeeRateTooLow error and have the user use an explicit override fee rate of their choice to adjust the target upwards if they'd ever hit it?

tnull · 2026-02-18T11:36:34Z

src/wallet/mod.rs

+		);
+
+		let pending_payment_store =
+			self.create_pending_payment_from_tx(new_payment.clone(), Vec::new());


Wouldn't we want to track the original Txid as a conflict here?

bump_fee_rbf just creates the initial pending entry, and the empty vec gets treated as None during update, so it won't overwrite existing conflicts. The conflict tracking is handled in the TxReplaced event. I've also updated conflict_txids to include the replaced txid, as this is not returned as part of the conflicts from bdk_wallet, so no intermediate txid gets lost across consecutive bumps.

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

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

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 6 times, most recently from b0b40b1 to dc53ac5 Compare February 17, 2026 16:15

tnull reviewed Feb 18, 2026

View reviewed changes

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch from dc53ac5 to d9d061c Compare February 18, 2026 16:51

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!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

tnull Feb 12, 2026 •

edited

Loading