Add a `PaymentId` for inbound payments #3303

TheBlueMatt · 2024-09-08T21:07:59Z

We expect our users to have fully idempotent `Event` handling as we
may replay events on restart for one of a number of reasons. This
isn't a big deal as long as all our events have some kind of
identifier users can use to check if the `Event` has already been
handled.

For outbound payments, this is the `PaymentId` they provide in the
send methods, however for inbound payments we don't have a great
option.

`PaymentHash` largely suffices - users can simply always claim in
response to a `PaymentClaimable` of sufficient value and treat a
`PaymentClaimed` event as duplicate any time they see a second one
for the same `PaymentHash`. This mostly works, but may result in
accepting duplicative payments if someone (incorrectly) pays twice
for the same `PaymentHash`.

Users could also fail for duplicative `PaymentClaimable` events of
the same `PaymentHash`, but doing so may result in spuriously
failing a payment if the `PaymentClaimable` event is a replay and
they never saw a corresponding `PaymentClaimed` event.

While none of this will result in spuriously thinking they've been
paid when they have not, it does result in some pretty awkward
semantics which we'd rather avoid our users having to deal with.

Instead, here, we add a new `PaymentId` which is simply an HMAC of
the HTLCs (as Channel ID, HTLC ID pairs) which were included in the
payment.

Based on #3302

codecov · 2024-09-08T21:14:49Z

Codecov Report

Attention: Patch coverage is 89.88764% with 9 lines in your changes missing coverage. Please review.

Project coverage is 90.72%. Comparing base (a75fdab) to head (bc26c21).
Report is 42 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/events/mod.rs	36.36%	7 Missing ⚠️
lightning/src/ln/channelmanager.rs	96.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3303      +/-   ##
==========================================
+ Coverage   89.66%   90.72%   +1.05%     
==========================================
  Files         126      126              
  Lines      102411   109498    +7087     
  Branches   102411   109498    +7087     
==========================================
+ Hits        91826    99337    +7511     
+ Misses       7857     7515     -342     
+ Partials     2728     2646      -82

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

G8XSU · 2024-09-10T19:01:29Z

lightning/src/ln/channelmanager.rs

@@ -3086,6 +3133,7 @@ where
 			fake_scid_rand_bytes: entropy_source.get_secure_random_bytes(),

 			probing_cookie_secret: entropy_source.get_secure_random_bytes(),
+			inbound_payment_id_secret: entropy_source.get_secure_random_bytes(),


Instead of these secrets being random, should these be derived from keysmanager?
That makes them deterministic i.e. no need to persist, no more irrecoverable data, and just determinism.

(It is more or less equivalent since we persist these currently and they will remain the same for the life of channelmanager.)

I did think about this for a minute, but not clear that adding yet another method on a public interface is an improvement here?

I thought we already have a method to getNodeSecret and we could just implement hkdf to derive keys in simple manner if possible outside of public interface.

True, we could do it by HKDF'ing with some NUMS point, but that seems substantially over-engineered (we have to pick a NUMS point, for starters), and introduces a performance penalty (plus is weird in an async signer context, though we don't strictly speaking allow for an async node_secret currently, and its not clear we ever will). I'm not entirely sure its worth it, even if both of the above issues aren't really critical or insurmountable, just more work..mostly I'm not clear on how much gain there is here.

Ok, fair enough.
I was not thinking about just this but other secrets in CM can be derived as well, this makes us more independent of channel-manager loss.

lightning/src/events/mod.rs

TheBlueMatt · 2024-09-11T20:11:25Z

One detail of the design here that's worth calling out is that these IDs only exist after we receive the full payment. There won't be a way to tie a given Invoice handed out to these IDs (though we could create different IDs for BOLT 12 Invoices and use those in place of these IDs, if we want, as a later change).

G8XSU · 2024-09-11T20:20:12Z

One detail of the design here that's worth calling out is that these IDs only exist after we receive the full payment.

And iiuc we cannot use them for partial MPP payments.
What if used just the first received htlc for a payment to compute the hash?

tnull · 2024-09-12T16:49:01Z

This needs a rebase now that #3302 has been merged.

In the next commit we'll change the order of HTLCs in `PaymentClaim[able,ed]` events. This shouldn't break anything, but our current functional tests check that the HTLCs are provided in the order they expect (the order they were received). Instead, here we only validate that each claimed HTLC matches one expected path.

In the next commit we'll start generating `PaymentId`s for inbound payments randomly by HMAC'ing the HTLC set of the payment. Here we start by defining the HMAC secret for these HMACs. This requires one small test adaptation and a full_stack_target fuzz change because it changes the RNG consumption.

TheBlueMatt · 2024-09-12T18:46:14Z

And iiuc we cannot use them for partial MPP payments.

Indeed. Currently we (very deliberately) don't expose incomplete MPP payments to users, and not sure that we will, so I'm not toooo concerned about it.

What if used just the first received htlc for a payment to compute the hash?

One thing I want here is the ability to reconstruct the PaymentId even if we lose the ChannelManager and rebuild the payment data from the ChannelMonitor list. I'm not sure how to do that from an HTLC subset, or at least it makes it harder.

This needs a rebase now that #3302 has been merged.

Done.

G8XSU · 2024-09-13T21:06:57Z

One thing I want here is the ability to reconstruct the PaymentId even if we lose the ChannelManager and rebuild the payment data from the ChannelMonitor list.

How will this work when a random secret is involved in generating the paymentId? 😅

tnull

LGTM, I think.

One question regarding the use of channel_id for ID derivation.

tnull · 2024-09-16T10:03:46Z

lightning/src/ln/channelmanager.rs

+		let mut prev_pair = None;
+		let mut hasher = HmacEngine::new(key);
+		for (channel_id, htlc_id) in htlcs {
+			hasher.input(&channel_id.0);


Just to double-check my understanding of the current splicing spec: channel_id and short_channel_id will now be stable even after splices, and even after a 'channel upgrade', right? IIRC, this might have been an issue with earlier splicing/DF drafts, no?

That is my understanding, yes.

TheBlueMatt · 2024-09-16T14:43:19Z

How will this work when a random secret is involved in generating the paymentId? 😅

Sorry, wasn't being clear. I mean having a very stale ChannelManager.

tnull · 2024-09-16T15:00:37Z

CI is failing:

error[E0407]: method `for_inbound_from_htlcs` is not a member of trait `Verification`
   --> lightning/src/ln/channelmanager.rs:492:2
    |
492 | /     fn for_inbound_from_htlcs<I: Iterator<Item=(ChannelId, u64)>>(key: &[u8; 32], htlcs: I) -> PaymentId {
493 | |         let mut prev_pair = None;
494 | |         let mut hasher = HmacEngine::new(key);
495 | |         for (channel_id, htlc_id) in htlcs {
...   |
503 | |         PaymentId(Hmac::<Sha256>::from_engine(hasher).to_byte_array())
504 | |     }
    | |_____^ not a member of trait `Verification`
....
error[E0599]: no function or associated item named `for_inbound_from_htlcs` found for struct `PaymentId` in the current scope
   --> lightning/src/ln/channelmanager.rs:800:14
    |
468 | pub struct PaymentId(pub [u8; Self::LENGTH]);
    | -------------------- function or associated item `for_inbound_from_htlcs` not found for this struct
...
800 |         PaymentId::for_inbound_from_htlcs(
    |                    ^^^^^^^^^^^^^^^^^^^^^^ function or associated item not found in `PaymentId`

TheBlueMatt · 2024-09-16T16:47:10Z

Ah, rebase error.

G8XSU · 2024-09-16T22:10:03Z

lightning/src/ln/channelmanager.rs

@@ -2239,6 +2285,9 @@ where
 	/// keeping additional state.
 	probing_cookie_secret: [u8; 32],

+	/// When generating [`PaymentId`]s for inbound payments, we HMAC the HTLCs with this secret.
+	inbound_payment_id_secret: [u8; 32],


should we call this just payment_id_secret?
Just in case we want to generate payment_id similarly for forwarded payments?

We can rename it if we change what its used for (and if its for forwarded HTLCs I don't think we should call it "payment" at all - we don't forward payments we forward HTLCs :) )

tnull · 2024-09-17T08:55:15Z

Feel free to squash from my side.

We expect our users to have fully idempotent `Event` handling as we may replay events on restart for one of a number of reasons. This isn't a big deal as long as all our events have some kind of identifier users can use to check if the `Event` has already been handled. For outbound payments, this is the `PaymentId` they provide in the send methods, however for inbound payments we don't have a great option. `PaymentHash` largely suffices - users can simply always claim in response to a `PaymentClaimable` of sufficient value and treat a `PaymentClaimed` event as duplicate any time they see a second one for the same `PaymentHash`. This mostly works, but may result in accepting duplicative payments if someone (incorrectly) pays twice for the same `PaymentHash`. Users could also fail for duplicative `PaymentClaimable` events of the same `PaymentHash`, but doing so may result in spuriously failing a payment if the `PaymentClaimable` event is a replay and they never saw a corresponding `PaymentClaimed` event. While none of this will result in spuriously thinking they've been paid when they have not, it does result in some pretty awkward semantics which we'd rather avoid our users having to deal with. Instead, here, we add a new `PaymentId` which is simply an HMAC of the HTLCs (as Channel ID, HTLC ID pairs) which were included in the payment.

TheBlueMatt · 2024-09-17T19:32:48Z

Squashed with no changes.

G8XSU · 2024-09-17T20:21:10Z

Ci and lint check is failing.

TheBlueMatt · 2024-09-18T18:06:03Z

CI failures were just that upstream git was failing when I pushed, kicked CI. The lint failures are fixed in a different PR.

tnull · 2024-09-19T08:21:32Z

CI failures were just that upstream git was failing when I pushed, kicked CI. The lint failures are fixed in a different PR.

Seems kicking didn't work, CI is still failing:

error[E0599]: no method named `signing_pubkey` found for struct `Offer` in the current scope
    --> lightning/src/ln/offers_tests.rs:2290:19
     |
2290 |     assert_eq!(offer.signing_pubkey(), Some(bob_id));
     |                      ^^^^^^^^^^^^^^
     |
    ::: lightning/src/offers/offer.rs:548:1
     |
548  | pub struct Offer {
     | ---------------- method `signing_pubkey` not found for this struct
     |
help: there is a method `issuer_signing_pubkey` with a similar name
     |
2290 |     assert_eq!(offer.issuer_signing_pubkey(), Some(bob_id));
     |                      ~~~

tnull self-requested a review September 9, 2024 07:52

G8XSU reviewed Sep 10, 2024

View reviewed changes

TheBlueMatt added 2 commits September 12, 2024 18:36

TheBlueMatt force-pushed the 2024-09-inbound-payment-id branch from 7b33c21 to d0e12c1 Compare September 12, 2024 18:45

tnull reviewed Sep 16, 2024

View reviewed changes

G8XSU reviewed Sep 16, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-09-inbound-payment-id branch from bc26c21 to 1fcbb9f Compare September 17, 2024 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a `PaymentId` for inbound payments #3303

Add a `PaymentId` for inbound payments #3303

TheBlueMatt commented Sep 8, 2024

codecov bot commented Sep 8, 2024 •

edited

Loading

G8XSU Sep 10, 2024

TheBlueMatt Sep 10, 2024

G8XSU Sep 11, 2024

TheBlueMatt Sep 12, 2024

G8XSU Sep 13, 2024

TheBlueMatt commented Sep 11, 2024

G8XSU commented Sep 11, 2024

tnull commented Sep 12, 2024

TheBlueMatt commented Sep 12, 2024

G8XSU commented Sep 13, 2024

tnull left a comment

tnull Sep 16, 2024

TheBlueMatt Sep 16, 2024

TheBlueMatt commented Sep 16, 2024

tnull commented Sep 16, 2024

TheBlueMatt commented Sep 16, 2024

G8XSU Sep 16, 2024

TheBlueMatt Sep 17, 2024

tnull commented Sep 17, 2024

TheBlueMatt commented Sep 17, 2024

G8XSU commented Sep 17, 2024

TheBlueMatt commented Sep 18, 2024

tnull commented Sep 19, 2024

Add a PaymentId for inbound payments #3303

Are you sure you want to change the base?

Add a PaymentId for inbound payments #3303

Conversation

TheBlueMatt commented Sep 8, 2024

codecov bot commented Sep 8, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Sep 11, 2024

G8XSU commented Sep 11, 2024

tnull commented Sep 12, 2024

TheBlueMatt commented Sep 12, 2024

G8XSU commented Sep 13, 2024

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Sep 16, 2024

tnull commented Sep 16, 2024

TheBlueMatt commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Sep 17, 2024

TheBlueMatt commented Sep 17, 2024

G8XSU commented Sep 17, 2024

TheBlueMatt commented Sep 18, 2024

tnull commented Sep 19, 2024

Add a `PaymentId` for inbound payments #3303

Add a `PaymentId` for inbound payments #3303

codecov bot commented Sep 8, 2024 •

edited

Loading