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)structFundingTxCandidate{pubtxid:Txid,pubamount_msat:Option<u64>,pubfee_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)structFundingTxCandidate{pubtxid:Txid,pubamount_msat:Option<u64>,pubfee_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

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.

Camillarhi · 2026-06-25T10:02:09Z

Thanks for this @tnull. Just a few commit nits, there's an f before two commit messages f Build RBF test with UniFFI and f Keep pending tx conflicts canonical

tnull · 2026-06-25T11:09:30Z

Thanks for this @tnull. Just a few commit nits, there's an f before two commit messages f Build RBF test with UniFFI and f Keep pending tx conflicts canonical

Yes, this is intentional as they are fixups to-be-squashed after review.

jkczyz

LGTM. Please squash.

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

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

tnull · 2026-06-26T05:21:25Z

LGTM. Please squash.

Squashed without further changes.

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 da75376Compare June 15, 2026 08:40

tnull mentioned this pull request Jun 19, 2026
Add splice RBF support #888
Merged

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 2 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 486c2b3Compare June 25, 2026 07:32

tnull force-pushed the 2026-06-fix-770-payment-store-followups branch from 486c2b3 to ad0bbc2Compare 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

jkczyz reviewed Jun 26, 2026
View reviewed changes

tnull added 4 commits June 26, 2026 07:21

Group pending payment storage constants
7d9afb3
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
65ee795
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

tnull force-pushed the 2026-06-fix-770-payment-store-followups branch from ad0bbc2 to c82c2e5Compare June 26, 2026 05:21

jkczyz approved these changes Jun 26, 2026
View reviewed changes

tnull merged commit ad4a3a8 into lightningdevkit:mainJun 26, 2026
23 checks passed

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!

jkczyzJun 23, 2026

Choose a reason for hiding this comment

Uh oh!

tnullJun 23, 2026

Choose a reason for hiding this comment

Uh oh!

jkczyzJun 23, 2026

Choose a reason for hiding this comment

Uh oh!

jkczyzJun 23, 2026

Choose a reason for hiding this comment

Uh oh!

tnullJun 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!

jkczyzJun 24, 2026

Choose a reason for hiding this comment

Uh oh!

tnullJun 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!

Camillarhi commented Jun 25, 2026

Uh oh!

tnull commented Jun 25, 2026

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

tnull commented Jun 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tnull commented Jun 12, 2026•
edited
Loading

ldk-reviews-bot commented Jun 12, 2026•
edited
Loading

tnullJun 25, 2026•
edited
Loading