Bump BDK, address pending payment store follow ups by tnull · Pull Request #937 · lightningdevkit/ldk-node

tnull · 2026-06-12T11:27:45Z

Fixes #770.
Fixes #776.
Fixes #901.

We bump our BDK dependency and address some follow-ups to #658.

ldk-reviews-bot · 2026-06-12T11:27:48Z

👋 Thanks for assigning @jkczyz 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 · 2026-06-15T00:00:56Z

🔔 1st Reminder

Hey @jkczyz! 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 · 2026-06-15T08:41:17Z

Now that bdk_wallet 3.1 shipped over the weekend, I now directly bump to that and include a commit addressing #901. I also added a commit that has us respect anchor reserves when fee bumping.

ldk-reviews-bot · 2026-06-17T00:01:57Z

🔔 2nd Reminder

Hey @jkczyz! 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-06-20T00:00:37Z

🔔 3rd Reminder

Hey @jkczyz! 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-06-22T00:01:46Z

🔔 4th Reminder

Hey @jkczyz! 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.

jkczyz · 2026-06-23T04:42:19Z

-					let pending_payment_details =
-						self.create_pending_payment_from_tx(payment, conflict_txids.clone());
+					let payment_txid = match &payment.kind {
+						PaymentKind::Onchain { txid, .. } => *txid,
+						_ => {
+							log_error!(
+								self.logger,
+								"Payment {:?} is not on-chain during WalletEvent::TxReplaced",
+								payment_id,
+							);
+							continue;
+						},
+					};
+					let pending_payment_details = self.create_pending_payment_from_tx(
+						payment_id,
+						payment_txid,
+						conflict_txids,
+					);


Regarding the approach from #888, it's probably better to take this approach rather than using ChannelReady as you suggest in that PR. IIUC, it will remove the race condition that needs to be protected yet. Gonna give that a try by rebasing on this PR and will report back there.

Alright, sounds good, let me know.

The one concern about this approach is the amount of data in TransactionType::InteractiveFunding. Following #791's approach, we can store minimal information like Vec<Channel>. But we do need to keep other information until confirmation in order to set the correct amount and fee in case an RBF candidate other than the latest confirms.

We can use the pending payment store for this, storing either everything or a minimal fields like this:

pub(crate) struct FundingTxCandidate { pub txid: Txid, pub amount_msat: Option<u64>, pub fee_paid_msat: Option<u64>, }

Since we don't support mixed-mode splicing, we don't need to store the direction. But supporting that or batched splices / v2 opens would require including that here. Or including some other data instead of a direction if we think that it is no longer meaningful in the presence of those features.

Another confusing point about direction, if we splice-out we may say this is outbound, but should it be if it goes to our own address?

The one concern about this approach is the amount of data in TransactionType::InteractiveFunding. Following #791's approach, we can store minimal information like Vec<Channel>. But we do need to keep other information until confirmation in order to set the correct amount and fee in case an RBF candidate other than the latest confirms.

We can use the pending payment store for this, storing either everything or a minimal fields like this:

pub(crate) struct FundingTxCandidate { pub txid: Txid, pub amount_msat: Option<u64>, pub fee_paid_msat: Option<u64>, }

Since we don't support mixed-mode splicing, we don't need to store the direction. But supporting that or batched splices / v2 opens would require including that here. Or including some other data instead of a direction if we think that it is no longer meaningful in the presence of those features.

Okay, but that seems reasonable? It's fine to add additional fields to the pending payment store, as long as we're fine with them getting pruned eventually once the transactions confirm?

Another confusing point about direction, if we splice-out we may say this is outbound, but should it be if it goes to our own address?

That seems also fine? Potentially we could consider a third variant that indicates 'internal balance transfer' or similar, but not sure it's worth?

jkczyz

Looks like the fixups aren't interleaved

jkczyz · 2026-06-24T18:32:14Z

+			let old_txid = self.txid;
+			self.txid = update.txid;
+			if old_txid != self.txid && !self.conflicting_txids.contains(&old_txid) {
+				self.conflicting_txids.push(old_txid);


IIUC, this means an update can add but never remove.

Outdated by now, might revisit in the future. Now opened #950 to track that.

Update the direct BDK wallet stack to the latest crate releases. This lets follow-up wallet event code use the upstream BDK API. It also preserves temporary transaction cleanup after BDK removed its cancel_tx helper. Co-Authored-By: HAL 9000

Use BDK's wallet event helper for mempool updates. This removes the local event diffing copy now that BDK exposes the needed event API. Co-Authored-By: HAL 9000

Move affected on-chain payments back to pending when BDK reports that their transaction is unconfirmed again. This keeps payment history aligned with wallet events after a reorg. It does not update payment records directly from disconnected-block notifications. Co-Authored-By: HAL 9000

tnull · 2026-06-25T07:36:19Z

Now decided to drop two commits (consolidating in one file, compacting pending payment store) to defer the decision how to simplify/refactor pending payment store post #948, as there we'll likely start to track user-registered payments (manual claim flow) in the pending payment store, i.e., it could be simpler to just keep the PaymentDetails around for that, though I'm not quite sure yet.

Normalize pending on-chain conflict lists after payment updates so a txid that becomes canonical again is not persisted as its own conflict. This preserves the existing empty-conflict update behavior used by RBF event handling while keeping pending indexes consistent. Co-Authored-By: HAL 9000

Keep pending payment namespace constants next to the primary payment store constants. This keeps related persistence keys discoverable together. Co-Authored-By: HAL 9000

Stop exporting the pending payment index record from the public payment module. The pending index is an internal persistence detail and should not become public API before this ships. Co-Authored-By: HAL 9000

RBF can spend fee increases from the original transaction's change output. Check the replacement fee increase against the current anchor-channel reserve before signing. This prevents high manual fee rates from consuming funds reserved for anchor spends. This finding was discovered by Project Loupe. Co-Authored-By: HAL 9000

Convert the explicit fee rate through Into so the anchor-reserve RBF test builds with both the native and UniFFI fee-rate APIs. Co-Authored-By: HAL 9000

tnull requested a review from jkczyz June 12, 2026 11:27

tnull mentioned this pull request Jun 12, 2026

Update bdk_wallet to v3.0.0 and refactor test helpers #907

Closed

tnull force-pushed the 2026-06-fix-770-payment-store-followups branch from 70654b2 to da75376 Compare June 15, 2026 08:40

tnull mentioned this pull request Jun 19, 2026

Add splice RBF support #888

Open

jkczyz reviewed Jun 23, 2026

View reviewed changes

tnull added this to the 0.8 milestone Jun 24, 2026

tnull requested a review from jkczyz June 24, 2026 09:13

jkczyz reviewed Jun 24, 2026

View reviewed changes

tnull added 3 commits June 25, 2026 09:27

Bump BDK wallet dependencies

678732d

Update the direct BDK wallet stack to the latest crate releases. This lets follow-up wallet event code use the upstream BDK API. It also preserves temporary transaction cleanup after BDK removed its cancel_tx helper. Co-Authored-By: HAL 9000

Use BDK mempool wallet events

b384487

Use BDK's wallet event helper for mempool updates. This removes the local event diffing copy now that BDK exposes the needed event API. Co-Authored-By: HAL 9000

tnull force-pushed the 2026-06-fix-770-payment-store-followups branch from a6a0dee to 486c2b3 Compare June 25, 2026 07:32

tnull added 5 commits June 25, 2026 10:17

Group pending payment storage constants

7989d47

Keep pending payment namespace constants next to the primary payment store constants. This keeps related persistence keys discoverable together. Co-Authored-By: HAL 9000

Keep pending payment details internal

8ebc2fe

Stop exporting the pending payment index record from the public payment module. The pending index is an internal persistence detail and should not become public API before this ships. Co-Authored-By: HAL 9000

f Build RBF test with UniFFI

ad0bbc2

Convert the explicit fee rate through Into so the anchor-reserve RBF test builds with both the native and UniFFI fee-rate APIs. Co-Authored-By: HAL 9000

tnull force-pushed the 2026-06-fix-770-payment-store-followups branch from 486c2b3 to ad0bbc2 Compare June 25, 2026 08:31

tnull mentioned this pull request Jun 25, 2026

Consider simplifying/refactoring PendingPaymentStore #950

Open

tnull requested a review from jkczyz June 25, 2026 08:41

Uh oh!

Conversation

tnull commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 15, 2026

Uh oh!

tnull commented Jun 15, 2026

Uh oh!

ldk-reviews-bot commented Jun 17, 2026

Uh oh!

ldk-reviews-bot commented Jun 20, 2026

Uh oh!

ldk-reviews-bot commented Jun 22, 2026

Uh oh!

Uh oh!

Uh oh!

jkczyz Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

tnull Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

jkczyz Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

jkczyz Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

tnull Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jkczyz Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

tnull Jun 25, 2026 • 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 commented Jun 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tnull commented Jun 12, 2026 •

edited

Loading

ldk-reviews-bot commented Jun 12, 2026 •

edited

Loading

tnull Jun 25, 2026 •

edited

Loading