Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a PaymentId for inbound payments #3303

Merged
merged 3 commits into from
Sep 24, 2024

Conversation

TheBlueMatt
Copy link
Collaborator

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

Copy link

codecov bot commented Sep 8, 2024

Codecov Report

Attention: Patch coverage is 86.30137% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.62%. Comparing base (cdd1298) to head (aaf529d).
Report is 14 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 93.18% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3303      +/-   ##
==========================================
- Coverage   89.64%   89.62%   -0.03%     
==========================================
  Files         126      126              
  Lines      102676   102736      +60     
  Branches   102676   102736      +60     
==========================================
+ Hits        92043    92072      +29     
- Misses       7909     7931      +22     
- Partials     2724     2733       +9     
Flag Coverage Δ
89.62% <86.30%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@tnull tnull self-requested a review September 9, 2024 07:52
@@ -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(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@TheBlueMatt
Copy link
Collaborator Author

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
Copy link
Contributor

G8XSU commented Sep 11, 2024

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
Copy link
Contributor

tnull commented Sep 12, 2024

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

@TheBlueMatt
Copy link
Collaborator Author

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
Copy link
Contributor

G8XSU commented Sep 13, 2024

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? 😅

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I think.

One question regarding the use of channel_id for ID derivation.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is my understanding, yes.

@TheBlueMatt
Copy link
Collaborator Author

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
Copy link
Contributor

tnull commented Sep 16, 2024

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
Copy link
Collaborator Author

Ah, rebase error.

@tnull
Copy link
Contributor

tnull commented Sep 17, 2024

Feel free to squash from my side.

@TheBlueMatt
Copy link
Collaborator Author

Squashed with no changes.

@G8XSU
Copy link
Contributor

G8XSU commented Sep 17, 2024

Ci and lint check is failing.

@TheBlueMatt
Copy link
Collaborator Author

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

@tnull
Copy link
Contributor

tnull commented Sep 19, 2024

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));
     |                      ~~~

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.
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
Copy link
Collaborator Author

Ugh, I thought CI ran on a rebased version but I guess not, rebased without changes.

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK if CI passes.

Copy link
Contributor

@G8XSU G8XSU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint check failure in CI.

Copy link
Contributor

@arik-so arik-so left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, what's the holdup on merging? Lint check failure seems unrelated @G8XSU

@tnull tnull merged commit a0d0f02 into lightningdevkit:main Sep 24, 2024
16 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants