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

🎉 This PR is now ready for review!
Please choose at least one reviewer by assigning them on the right bar.
If no reviewers are assigned within 10 minutes, I'll automatically assign one.
Once the first reviewer has submitted a review, a second will be assigned if required.

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.

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

Rebased without further changes for now.

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

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

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

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:10:49Z

+			.collect::<Vec<_>>();
+
+		for payment_id in expired_payment_ids {
+			self.runtime.block_on(self.pending_payment_store.remove(&payment_id))?;


Can we do a batch_remove here so we avoid repeatedly locking and unlocking multiple mutexes ?

tankyleo · 2026-07-07T17:16:07Z

+			.lock()
+			.expect("lock")
+			.get(payment_hash)
+			.cloned()


nit: this clone is not necessary we can rework this method to remove it

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.

tankyleo · 2026-07-07T17:48:00Z

+			.collect::<Vec<_>>();
+
+		for payment_id in expired_payment_ids {
+			if let Err(e) = self.pending_payment_store.remove(&payment_id).await {


As above, batch_remove here would be useful

tankyleo · 2026-07-07T18:12:17Z

+				let (mut payment_id, payment_info) =
+					self.resolve_inbound_payment_id(event_payment_id, &payment_hash);
+				let pending_payment = if payment_info.is_none() {
+					self.find_pending_inbound_payment(&payment_hash).await?


Should we add a debug assert somewhere in the call chain here that there is a single payment id for that hash ?

tankyleo · 2026-07-07T18:16:51Z

-		);
-		self.runtime.block_on(self.payment_store.insert(payment))?;
+		if let Some(payment_hash) = manual_claim_payment_hash {
+			self.register_manual_claim_invoice(


found with codex: is it worth making the "check duplicates" step and the "register invoice" step atomic? Ie try to insert the invoice, and the pending_payment_store returns DuplicatePayment if there is a collision in the hash.

tankyleo · 2026-07-07T19:05:12Z

+		Ok(self.pending_payment_store.get_pending_manual_bolt11_by_payment_hash(payment_hash))
+	}
+
+	fn resolve_inbound_payment_id(


From codex:

Manual-claim payment handling can lose the mapping from LDK payment ids to the user-facing random id across replay, causing duplicate/misidentified payment records.

Review comment:

[P2] Resolve manual claims by stored hash after replay _ /home/ubuntu/ldk-node/src/event.rs:654-658
For manual-claim payments the durable payment record is stored under the random id emitted in PaymentClaimable, while a replayed LDK PaymentClaimed event carries the LDK-derived id (or the hash-as-id fallback). If the pending row has already been removed, e.g. after a crash or event_queue.add_ event failure later in this handler, this resolver misses the existing payment and the replay inserts/emits a duplicate payment under the LDK id. Please also resolve existing Bolt11 records by their stored payment hash, or keep the pending mapping until the user event is durably queued.

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 added 4 commits July 7, 2026 17:15

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

		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!

tnull commented Jul 7, 2026

Uh oh!

tnull commented Jul 7, 2026

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!

tankyleo Jul 7, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ldk-reviews-bot commented Jun 24, 2026 •

edited

Loading

tankyleo Jul 7, 2026 •

edited

Loading