Enhance onchain transaction management #628

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.

Introduces a `RebroadcastPolicy` to manage the automatic rebroadcasting of unconfirmed transactions with exponential backoff. This prevents excessive network spam while systematically retrying stuck transactions. The feature is enabled by default but can be disabled via the builder: `builder.set_auto_rebroadcast_unconfirmed(false)`. Configuration options: - min_rebroadcast_interval: Base delay between attempts (seconds) - max_broadcast_attempts: Total attempts before abandonment - backoff_factor: Multiplier for exponential delay increase Sensible defaults are provided (300s, 24 attempts, 1.5x backoff).

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 - Implements fee rate estimation with safety limits - Maintains payment history consistency across wallet updates - Includes integration tests for various RBF scenarios

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?

Camillarhi · 2025-09-26T09:53:41Z

@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?

Thanks for the update, no problem at all on the review delay. I’ll go through the links you mentioned and follow up with thoughts

Camillarhi · 2025-09-27T01:32:16Z

@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?

Thanks for the update, no problem at all on the review delay. I’ll go through the links you mentioned and follow up with thoughts

Thanks for the update and for sharing those links. I've reviewed the BDK events implementation, and I agree that using the new events system would be a much cleaner approach for tracking on-chain payment states and handling RBF cases by updating the payment stores with the events emitted.

It makes perfect sense to let #448 land first to establish the new events structure, and then rebase the changes from this PR on top of that foundation.

On a related note, I saw that you opened #488. Would you like me to continue working on that, or would you prefer to handle it yourself? I'm happy to help.

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 from 8b7cf78 to 06e7ff3 Compare September 11, 2025 22:13

Camillarhi added 2 commits September 12, 2025 00:05

Camillarhi force-pushed the feat/tx-rebroadcast-fee-bumping branch 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

Open

tnull reviewed Sep 26, 2025

View reviewed changes

		self.payment_store.remove(&payment_id)?;

		self.payment_store.insert_or_update(payment_details)?;

Enhance onchain transaction management #628

Are you sure you want to change the base?

Enhance onchain transaction management #628

Uh oh!

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!

Camillarhi commented Sep 26, 2025

Uh oh!

Camillarhi commented Sep 27, 2025

Camillarhi commented Aug 23, 2025 •

edited

Loading

ldk-reviews-bot commented Aug 23, 2025 •

edited

Loading

tnull left a comment •

edited

Loading