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.

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

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

Move the pending payment index record into the payment store module. This keeps the primary payment record and its pending index within the same persistence boundary. Co-Authored-By: HAL 9000

Persist only the payment id, current txid, and conflict txids in the pending payment index. This avoids duplicating payment state before the format ships and keeps RBF lookup aliases intact across replacement events. Co-Authored-By: HAL 9000

Update splice integration expectations for BDK 3.1's fee rounding fix. This keeps the remaining LDK/BDK fee-accounting drift explicit. It prevents future dependency changes from restoring the larger surplus. Refs lightningdevkit#901 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-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?

Consume cancellation transactions. This avoids cloning output scripts during wallet cancellation. Co-Authored-By: HAL 9000

Rely on the surrounding txid-change branch. This keeps conflict tracking behavior unchanged. Co-Authored-By: HAL 9000

jkczyz

Looks like the fixups aren't interleaved

jkczyz · 2026-06-24T18:12:45Z

-	let expected_splice_in_onchain_cost_sat = 254;
+	let expected_splice_in_onchain_cost_sat = 253;

-	// LDK's fee calculation differs from BDK wallet's, which over pays on fees. Rather than giving
-	// the extra fees to the miner, LDK sends it to the channel balance since there may not be a
-	// change output.
-	//
-	// TODO: Some of the discrepancy is addressed upstream, so this number should be adjusted when
-	// updating the BDK wallet dependency. See: https://github.com/bitcoindevkit/bdk_wallet/pull/479
-	let expected_splice_in_lightning_balance_sat = 4_000_003;
+	// BDK 3.1.0 avoids the previous per-UTXO fee rounding during coin selection. Keep the
+	// remaining 2-sat LDK/BDK fee-accounting drift explicit so a dependency change cannot silently
+	// reintroduce the larger surplus. Rather than giving the extra sats to the miner, LDK sends
+	// them to the channel balance since there may not be a change output.
+	let expected_splice_in_lightning_balance_sat = 4_000_002;


Should this go in the first commit? Otherwise, the test fails on all the commits until this one.

jkczyz · 2026-06-24T18:28:36Z

-		// Update the underlying payment details if present
-		if let Some(payment_update) = update.payment_update {
-			updated |= self.details.update(payment_update);
+		if self.txid != update.txid {
+			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);
+			}
+			updated = true;
 		}

-		if let Some(new_conflicting_txids) = update.conflicting_txids {
-			if self.conflicting_txids != new_conflicting_txids {
-				self.conflicting_txids = new_conflicting_txids;
+		for txid in update.conflicting_txids {
+			if txid != self.txid && !self.conflicting_txids.contains(&txid) {
+				self.conflicting_txids.push(txid);
 				updated = true;
 			}
 		}


Claude found this and claims that a unit test would catch it:

In PendingPaymentDetails::update, the txid-rotation branch only de-dupes old_txid before pushing it — it doesn't exclude the new self.txid from conflicting_txids. The merge loop right below it does guard with txid != self.txid, so the two paths are inconsistent:

if self.txid != update.txid { let old_txid = self.txid; self.txid = update.txid; if !self.conflicting_txids.contains(&old_txid) { // doesn't exclude the new self.txid self.conflicting_txids.push(old_txid); } updated = true; } for txid in update.conflicting_txids { if txid != self.txid && !self.conflicting_txids.contains(&txid) { // this one does ... } }

This lets the canonical txid end up listed among its own conflicts. Scenario: after an RBF T1 -> T2, the record is { payment_id: P(T1), txid: T2, conflicting_txids: [T1] }. If T2 is later evicted and the original T1 confirms (shallower than ANTI_REORG_DELAY), TxConfirmed { txid: T1 } resolves back to P(T1) via the direct branch in find_payment_by_txid, and the pending arm calls create_pending_payment_from_tx(P(T1), T1, vec![]). update() then rotates self.txid back to T1 and pushes T2, leaving { txid: T1, conflicting_txids: [T1, T2] } — T1 now conflicts with itself.

The impact is small (lookups still resolve correctly), but the persisted record becomes self-inconsistent, which also means the "conflicting_txids never contains the current txid" invariant isn't actually maintained on this path. Could fix it by normalizing after the rotation, e.g.:

self.txid = update.txid; self.conflicting_txids.retain(|c| *c != self.txid);

Reachability depends on BDK re-emitting a TxConfirmed for a previously-replaced tx after the replacement is dropped — does that match what you'd expect here?

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.

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 added 9 commits June 15, 2026 10:14

Bump BDK wallet dependencies

305a54d

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

bba9652

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

Group pending payment storage constants

bc2b070

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

af7dc4c

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

Co-locate pending payment indexes

9acb2ad

Move the pending payment index record into the payment store module. This keeps the primary payment record and its pending index within the same persistence boundary. Co-Authored-By: HAL 9000

Store compact pending payment index records

5668ca9

Persist only the payment id, current txid, and conflict txids in the pending payment index. This avoids duplicating payment state before the format ships and keeps RBF lookup aliases intact across replacement events. Co-Authored-By: HAL 9000

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 added 2 commits June 24, 2026 11:10

f - Avoid cloning cancellation scripts

d9b76ea

Consume cancellation transactions. This avoids cloning output scripts during wallet cancellation. Co-Authored-By: HAL 9000

f - Drop redundant txid comparison

a6a0dee

Rely on the surrounding txid-change branch. This keeps conflict tracking behavior unchanged. Co-Authored-By: HAL 9000

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

jkczyz reviewed Jun 24, 2026

View reviewed changes

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!

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!

jkczyz left a comment

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!

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