Track inbound payments by `PaymentId` by tnull · Pull Request #948 · lightningdevkit/ldk-node

tnull · 2026-06-24T15:15:21Z

Closes #298.

This finally switches our inbound payments over to be fully tracked by payment id, and decouples payment IDs from payment hashes.

ldk-reviews-bot · 2026-06-24T15:15:24Z

👋 Thanks for assigning @tankyleo 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.

tankyleo · 2026-07-07T02:06:34Z

 	payment_store: Arc<PaymentStore>,
+	pending_payment_store: Arc<PendingPaymentStore>,


Here and elsewhere, can we avoid adding a separate store to Bolt11Payment and EventHandler, and move the prune_expired_pending_payments to an internal method that gets called internally inside the public methods of the store?

You mean conceptually merging pending payment store and payment store? Not sure we'd want to go there as it would limit the degrees of freedom we have in each.

tankyleo · 2026-07-07T02:29:57Z

 		}

 		self.channel_manager.fail_htlc_backwards(&payment_hash);
+		self.runtime.block_on(self.pending_payment_store.remove(&payment_id))?;


Similar vein as above comment, can we do this inside payment_store.update a few lines above?

See #948 (comment), but not sure if I understand your idea correctly.

tankyleo · 2026-07-07T02:51:29Z

+						payment_secret,
+						..
+					} => {
+						if payment_info.is_none() {


nit: we can DRY this branch for each PaymentPurpose variant

tankyleo · 2026-07-07T02:59:32Z

-				|| payment.status == PaymentStatus::Succeeded
-			{
-				log_error!(self.logger, "Payment error: an invoice must not be paid twice.");
-				return Err(Error::DuplicatePayment);


Hmm this is for outbound bolt11s, are we good to delete this ?

So, LDK itself will error if we recently paid the same invoice. The issue is that going forward we can't necessarily easily keep a secondary index on the payment store, which would be necessary to look up entries by payment hash. We might need to re-add that at some point but I tried to avoid it here.

Note that a) the more important case is the receive case b) checking for duplication based on the payment store state was already a somewhat leaky concept as users ofc. might simply remove entries, at which point we don't know anything about them anymore.

That said, all ears if you have an alternative suggestion.

Is it really true that LDK errors if the payment id is different?

I was reading https://github.com/lightningdevkit/rust-lightning/blob/506cb91f2e0fb87906188b79777bcf42595d3623/lightning/src/ln/channelmanager.rs#L5653-L5665

Is it really true that LDK errors if the payment id is different?

I was reading https://github.com/lightningdevkit/rust-lightning/blob/506cb91f2e0fb87906188b79777bcf42595d3623/lightning/src/ln/channelmanager.rs#L5653-L5665

Hmm, good catch. Then I was either misremembering or it changed at some point, but I thought DuplicatePayment was going off the payment hash, not the id. Still need to double-check how to avoid this case then, although I'm still a bit dubious how to avoid it.

tnull · 2026-07-07T14:58:58Z

Rebased without further changes for now.

tnull · 2026-07-07T15:27:49Z

Force-pushed some fixes:

> git diff-tree -U2  d1b5ffb3 b1f65649
diff --git a/bindings/python/src/ldk_node/test_ldk_node.py b/bindings/python/src/ldk_node/test_ldk_node.py
index 304caf9c..063c95f7 100644
--- a/bindings/python/src/ldk_node/test_ldk_node.py
+++ b/bindings/python/src/ldk_node/test_ldk_node.py
@@ -236,5 +236,5 @@ class TestLdkNode(unittest.TestCase):

         sender_payment = node_1.payment(keysend_payment_id)
-        receiver_payment = node_2.payment(keysend_payment_id)
+        receiver_payment = node_2.payment(received_event.payment_id)

         self.assertIsNotNone(sender_payment)
diff --git a/tests/integration_tests_migration.rs b/tests/integration_tests_migration.rs
index c4e63451..59621476 100644
--- a/tests/integration_tests_migration.rs
+++ b/tests/integration_tests_migration.rs
@@ -204,5 +204,5 @@ async fn migrate_node_across_all_backends() {
 	let invoice = node_b.bolt11_payment().receive(10_000, &description.into(), 3600).unwrap();
 	let ln_send_id = node.bolt11_payment().send(&invoice, None).unwrap();
-	expect_payment_successful_event!(node, Some(ln_send_id), None);
+	expect_payment_successful_event!(node, ln_send_id, None);
 	expect_payment_received_event!(node_b, 10_000);

@@ -212,5 +212,5 @@ async fn migrate_node_across_all_backends() {
 	let invoice = node.bolt11_payment().receive(5_000, &description.into(), 3600).unwrap();
 	let ln_receive_id = node_b.bolt11_payment().send(&invoice, None).unwrap();
-	expect_payment_successful_event!(node_b, Some(ln_receive_id), None);
+	expect_payment_successful_event!(node_b, ln_receive_id, None);
 	expect_payment_received_event!(node, 5_000);

tankyleo · 2026-07-07T17:20:15Z

+	fn replace_in_index(
+		&self, before: Option<&PendingPaymentDetails>, after: Option<&PendingPaymentDetails>,
+	) {
+		let mut index = self.manual_bolt11_payment_hash_index.lock().expect("lock");


Could we return early here before locking if the payment is not a manual bolt11 ? It seems wasteful to lock the index even though we have enough information here to know whether we'll actually modify the index.

First had added a fixup that I now dropped again. The modest gain didn't seem worth the added complexity/churn. Let me know if you feel strongly though.

The switch to tracking payments by ID happened with LDK Node v0.3.0, which is >1.5 years old by now. We can be pretty certain that nobody is upgrading from an older version to the upcoming v0.8. Here we hence make the `payment_id` fields in `Event` required which is a nice API simplification that will also be utilized in the next commit. Co-Authored-By: HAL 9000

Pending manual invoice records need persisted expiry times so stale reservations can be pruned before checking for duplicate payment hashes. Pending BOLT11 reservations also need indexed lookup by payment hash so duplicate checks can avoid scanning the full pending store. Co-Authored-By: HAL 9000

Avoid copying the full id list during manual BOLT11 lookups. Holding the index lock while reading the in-memory store does not change persistence ordering. Co-Authored-By: HAL 9000

Document the pending-store invariant that a manual BOLT11 hash maps to at most one pending payment id. Co-Authored-By: HAL 9000

Represent pending-payment pruning conditions as either wall-clock time or block height so later manual BOLT11 handling can keep the claimable phase tied to the HTLC claim deadline. Co-Authored-By: HAL 9000

Expose a pending-store batch removal path so callers can prune expired pending payments without repeatedly taking the pending-store mutation lock. Co-Authored-By: HAL 9000

Manual BOLT11 invoices need a pending entry before HTLC arrival. That lets duplicate registrations be rejected without scanning finalized payments. Keep the legacy hash-derived payment ID in this step; the next commit switches BOLT11 payments to randomized IDs. Co-Authored-By: HAL 9000

Reject duplicate manual BOLT11 hashes from the pending store before creating the invoice. This keeps duplicate detection and registration serialized, while preserving the persist-before-index ordering for crash recovery. Co-Authored-By: HAL 9000

Use the batch removal path for both manual BOLT11 pruning callsites introduced by the manual-invoice tracking change. Co-Authored-By: HAL 9000

Keep manual BOLT11 invoice reservations time-based until the claimable event arrives, then prune by the HTLC claim deadline height so cleanup cannot run before the claim window closes. Co-Authored-By: HAL 9000

A failed batch prune can already have durably removed earlier entries. Keep the pending-payment secondary index in step with those partial removals so a retry can finish pruning and later reuse expired manual hashes. Co-Authored-By: HAL 9000

Create inbound BOLT11 records from claimable and claimed events. Stop pre-creating automatic BOLT11 records when invoices are generated. Generate outbound BOLT11 IDs from KeysManager entropy instead of deriving them from the payment hash. Co-Authored-By: HAL 9000

Keep manual pending payments available until PaymentReceived is durably queued, so replay after queue persistence failure can still resolve the user-facing payment ID. Co-Authored-By: HAL 9000

joostjager · 2026-07-15T14:11:36Z

Looking at this through the lens of self-imposed versus intrinsic complexity, I think this PR again makes the cost of our current architecture clear. LDK and ldk-node persist related state independently, without a consistent commit boundary, so we have to recover consistency through careful ordering, replay, cleanup, and expiry logic.

tnull force-pushed the 2026-06-payment-id-prefactors branch from 51dba72 to ff3345d Compare June 24, 2026 15:21

tnull marked this pull request as draft June 24, 2026 15:22

tnull force-pushed the 2026-06-payment-id-prefactors branch from ff3345d to 544e100 Compare June 24, 2026 15:23

tnull commented Jun 24, 2026

View reviewed changes

Comment thread src/payment/bolt11.rs

This was referenced Jun 25, 2026

Bump BDK, address pending payment store follow ups #937

Merged

Add splice RBF support #888

Merged

Consider simplifying/refactoring PendingPaymentStore #950

Open

tnull force-pushed the 2026-06-payment-id-prefactors branch from 544e100 to 4c3f2e4 Compare July 2, 2026 13:45

tnull marked this pull request as ready for review July 2, 2026 13:46

ldk-reviews-bot requested a review from tankyleo July 2, 2026 13:47

tnull removed the request for review from tankyleo July 2, 2026 13:50

tnull marked this pull request as draft July 2, 2026 13:50

tnull force-pushed the 2026-06-payment-id-prefactors branch from 4c3f2e4 to 4a24ee7 Compare July 2, 2026 14:08

tnull requested a review from tankyleo July 2, 2026 14:09

tnull marked this pull request as ready for review July 2, 2026 14:09

joostjager mentioned this pull request Jul 6, 2026

Payment Persistence Gap Matrix #966

Open

tankyleo reviewed Jul 7, 2026

View reviewed changes

tnull force-pushed the 2026-06-payment-id-prefactors branch from 4a24ee7 to d1b5ffb Compare July 7, 2026 14:58

tnull force-pushed the 2026-06-payment-id-prefactors branch from d1b5ffb to b1f6564 Compare July 7, 2026 15:27

tankyleo reviewed Jul 7, 2026

View reviewed changes

tnull added 3 commits July 15, 2026 11:38

f Avoid cloning pending payment ids

64802f3

Avoid copying the full id list during manual BOLT11 lookups. Holding the index lock while reading the in-memory store does not change persistence ordering. Co-Authored-By: HAL 9000

tnull force-pushed the 2026-06-payment-id-prefactors branch from b1f6564 to c4cc340 Compare July 15, 2026 12:35

tnull added 3 commits July 15, 2026 14:40

f Assert unique manual payment hashes

59e9707

Document the pending-store invariant that a manual BOLT11 hash maps to at most one pending payment id. Co-Authored-By: HAL 9000

f Use enum pending expiries

d4e67d1

Represent pending-payment pruning conditions as either wall-clock time or block height so later manual BOLT11 handling can keep the claimable phase tied to the HTLC claim deadline. Co-Authored-By: HAL 9000

Add pending payment batch removal

4ef9c03

Expose a pending-store batch removal path so callers can prune expired pending payments without repeatedly taking the pending-store mutation lock. Co-Authored-By: HAL 9000

tnull added 4 commits July 15, 2026 14:40

f Reserve manual invoice hashes first

da39b52

Reject duplicate manual BOLT11 hashes from the pending store before creating the invoice. This keeps duplicate detection and registration serialized, while preserving the persist-before-index ordering for crash recovery. Co-Authored-By: HAL 9000

f Batch-prune expired pending payments

ccd8971

Use the batch removal path for both manual BOLT11 pruning callsites introduced by the manual-invoice tracking change. Co-Authored-By: HAL 9000

f Use claim-height pending expiry

1d29fcc

Keep manual BOLT11 invoice reservations time-based until the claimable event arrives, then prune by the HTLC claim deadline height so cleanup cannot run before the claim window closes. Co-Authored-By: HAL 9000

tnull force-pushed the 2026-06-payment-id-prefactors branch from c4cc340 to 046355b Compare July 15, 2026 12:58

tnull added 3 commits July 15, 2026 15:11

f Queue claim events before cleanup

452c643

Keep manual pending payments available until PaymentReceived is durably queued, so replay after queue persistence failure can still resolve the user-facing payment ID. Co-Authored-By: HAL 9000

tnull force-pushed the 2026-06-payment-id-prefactors branch from 046355b to 452c643 Compare July 15, 2026 13:19

tnull requested a review from tankyleo July 15, 2026 13:19

		payment_store: Arc<PaymentStore>,
		pending_payment_store: Arc<PendingPaymentStore>,

Uh oh!

Conversation

tnull commented Jun 24, 2026

Uh oh!

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

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!

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!

tnull Jul 15, 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 Jul 7, 2026

Uh oh!

tnull commented Jul 7, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Jul 15, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joostjager commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ldk-reviews-bot commented Jun 24, 2026 •

edited

Loading

tnull Jul 15, 2026 •

edited

Loading

tnull Jul 15, 2026 •

edited

Loading