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

Support paying static invoices #3140

Merged

Conversation

valentinewallace
Copy link
Contributor

@valentinewallace valentinewallace commented Jun 20, 2024

Support sending async payments as an always-online sender per lightning/bolts#1149, minus including the invreq in the payment onion.

Async receive is not yet supported, nor is sending async payments as an often-offline sender. Upcoming PRs will add async receive which will allow us to test the flow end-to-end.

Blocked on #3125, #3145, #3085.

@valentinewallace valentinewallace force-pushed the 2024-06-pay-static-invoice branch 2 times, most recently from f316229 to 6eb3711 Compare June 24, 2024 16:15
@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2024

Codecov Report

Attention: Patch coverage is 85.78199% with 30 lines in your changes missing coverage. Please review.

Project coverage is 90.02%. Comparing base (1059f5f) to head (6e27aec).
Report is 32 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/channelmanager.rs 50.00% 16 Missing and 1 partial ⚠️
lightning/src/ln/outbound_payment.rs 94.93% 8 Missing ⚠️
lightning/src/onion_message/messenger.rs 0.00% 2 Missing ⚠️
lightning/src/ln/peer_handler.rs 0.00% 1 Missing ⚠️
lightning/src/offers/signer.rs 93.33% 0 Missing and 1 partial ⚠️
lightning/src/onion_message/functional_tests.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3140      +/-   ##
==========================================
+ Coverage   89.63%   90.02%   +0.38%     
==========================================
  Files         126      126              
  Lines      102383   104805    +2422     
  Branches   102383   104805    +2422     
==========================================
+ Hits        91776    94350    +2574     
+ Misses       7877     7750     -127     
+ Partials     2730     2705      -25     

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

@jkczyz jkczyz self-requested a review June 24, 2024 19:54
@valentinewallace valentinewallace force-pushed the 2024-06-pay-static-invoice branch 2 times, most recently from d75a5d3 to 635a3e6 Compare July 8, 2024 17:43
@valentinewallace
Copy link
Contributor Author

Rebased so I could get #3156 in the follow-up.

@valentinewallace valentinewallace force-pushed the 2024-06-pay-static-invoice branch 2 times, most recently from 4f8fe07 to 51302e8 Compare July 8, 2024 18:17
@TheBlueMatt
Copy link
Collaborator

Needs rebase.

@valentinewallace valentinewallace force-pushed the 2024-06-pay-static-invoice branch 3 times, most recently from 2d0e7a7 to f0d9fde Compare July 10, 2024 19:17
lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/outbound_payment.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
@valentinewallace valentinewallace force-pushed the 2024-06-pay-static-invoice branch 2 times, most recently from 2ab6fe8 to bd29d90 Compare July 10, 2024 20:46
lightning/src/ln/outbound_payment.rs Outdated Show resolved Hide resolved
@@ -1463,7 +1463,7 @@ where
let context = match context {
None => OffersContext::Unknown {},
Some(MessageContext::Offers(context)) => context,
Some(MessageContext::Custom(_)) => {
Some(MessageContext::Custom(_)) | Some(MessageContext::AsyncPayments(_)) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are offers messages allowed to have an async context? Some docs on AsyncPaymentsContext would be nice that say where they're allowed and what they're used for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added docs.

Why are offers messages allowed to have an async context?

They shouldn't be, hence the debug assert below. Lmk if I'm missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

To rid ourselves of the assert, we need a larger refactor such that ParsedOnionMessageContents (or something like it) contains the appropriate context in each variant. Out of scope of this PR, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably use a _ catchall instead, though. Same elsewhere.

@@ -1473,14 +1473,22 @@ where
},
#[cfg(async_payments)]
ParsedOnionMessageContents::AsyncPayments(AsyncPaymentsMessage::HeldHtlcAvailable(msg)) => {
debug_assert!(context.is_none());
Copy link
Collaborator

Choose a reason for hiding this comment

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

While we do encrypt the context, I don't believe we MAC it, potentially allowing someone creating a blinded path to us to brute-force their way into this branch (they only have to get a few bytes to match). Same below for the debug assertion.

The other branches have debug asserts because the correctness is checked in peel_onion_message (and their assert messages should really be updated to mention that!), but you don't do that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed peeled_onion_message and removed this assertion. One thing I'm not sure about is whether to fail held_htlc_available messages that have Some context, since we expect None. I went with ignoring the context for now since we would've failed to decode it if it contained unknown even TLVs.

lightning/src/ln/outbound_payment.rs Show resolved Hide resolved
lightning/src/blinded_path/message.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
invoice, payment_id, &*self.entropy_source
)?;

let mut pending_async_payments_messages = self.pending_async_payments_messages.lock().unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we consider an Event at this point to tell the user the payment is pending+async and not to expect resolution immediately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, tracking this todo on #2298.

lightning/src/ln/outbound_payment.rs Outdated Show resolved Hide resolved
lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
@valentinewallace valentinewallace mentioned this pull request Jul 12, 2024
21 tasks
@valentinewallace valentinewallace force-pushed the 2024-06-pay-static-invoice branch 2 times, most recently from df323f3 to 1d6ba59 Compare July 12, 2024 21:34
@valentinewallace valentinewallace force-pushed the 2024-06-pay-static-invoice branch 2 times, most recently from fe21bc4 to 60c390f Compare July 26, 2024 18:49
@valentinewallace valentinewallace force-pushed the 2024-06-pay-static-invoice branch 2 times, most recently from 1ac6a17 to 5b8c21e Compare July 31, 2024 00:29
lightning/src/offers/signer.rs Outdated Show resolved Hide resolved
@@ -249,6 +249,7 @@ impl<'a> TlvStream<'a> {
}

/// A slice into a [`TlvStream`] for a record.
#[derive(Eq, PartialEq)]
Copy link
Contributor

Choose a reason for hiding this comment

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

We only need to compare record_bytes, so we can have from_same_offer map to that before comparing in order to avoid redundant checks.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We can drop the derive now , too.

) {
Ok(paths) => paths,
Err(()) => {
self.abandon_payment_with_reason(payment_id, PaymentFailureReason::RouteNotFound);
Copy link
Contributor

Choose a reason for hiding this comment

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

Were we going to create a new PaymentFailureReason variant for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, as mentioned here #3140 (comment) this is done in the follow-up.

@@ -11094,10 +11232,20 @@ where
None
}

fn release_held_htlc(&self, _message: ReleaseHeldHtlc) {}
fn release_held_htlc(&self, _message: ReleaseHeldHtlc, _context: AsyncPaymentsContext) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Might better to start these methods with handle_ given "release" is overloaded in release_pending_messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense to me, will do this in the follow-up though since this is pre-existing and PR big.

// contents from its TLV stream.
debug_assert!(false, "LDK requires an msat amount in either the invreq or the invreq's underlying offer");
abandon_with_entry!(entry, PaymentFailureReason::UnexpectedError);
return Err(Bolt12PaymentError::UnknownRequiredFeatures)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use use a different error here or comment why UnknownRequiredFeatures is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that historically we've been okay using fairly unsuitable errors for clearly unreachable code (i.e. with a debug_assert!(false) present). 🤔 Not sure there's a more suitable error for this case.

@valentinewallace
Copy link
Contributor Author

Squashed in the new changes with the following diff:

diff --git a/lightning/src/offers/signer.rs b/lightning/src/offers/signer.rs
index d6372bca8..d8caa2175 100644
--- a/lightning/src/offers/signer.rs
+++ b/lightning/src/offers/signer.rs
@@ -44,7 +44,7 @@ const OFFER_PAYMENT_ID_HMAC_INPUT: &[u8; 16] = &[5; 16];
 const ASYNC_PAYMENT_ID_HMAC_INPUT: &[u8; 16] = &[6; 16];

 // HMAC input for a `PaymentHash`. The HMAC is used in `OffersContext::InboundPayment`.
-const PAYMENT_HASH_HMAC_INPUT: &[u8; 16] = &[6; 16];
+const PAYMENT_HASH_HMAC_INPUT: &[u8; 16] = &[7; 16];

 /// Message metadata which possibly is derived from [`MetadataMaterial`] such that it can be
 /// verified.
diff --git a/lightning/src/offers/static_invoice.rs b/lightning/src/offers/static_invoice.rs
index 74955abe0..d083cae88 100644
--- a/lightning/src/offers/static_invoice.rs
+++ b/lightning/src/offers/static_invoice.rs
@@ -315,8 +315,10 @@ impl StaticInvoice {
        }

        pub(crate) fn from_same_offer(&self, invreq: &InvoiceRequest) -> bool {
-               let invoice_offer_tlv_stream = TlvStream::new(&self.bytes).range(OFFER_TYPES);
-               let invreq_offer_tlv_stream = TlvStream::new(invreq.bytes()).range(OFFER_TYPES);
+               let invoice_offer_tlv_stream = TlvStream::new(&self.bytes).range(OFFER_TYPES)
+                       .map(|tlv_record| tlv_record.record_bytes);
+               let invreq_offer_tlv_stream = TlvStream::new(invreq.bytes()).range(OFFER_TYPES)
+                       .map(|tlv_record| tlv_record.record_bytes);
                invoice_offer_tlv_stream.eq(invreq_offer_tlv_stream)
        }
 }

Useful for ensuring that an inbound static invoice matches one of our outbound
invreqs, otherwise it is an unexpected invoice and should be ignored and not
paid.
Adds a pending outbound payment variant for async payments, which indicates
that we have received a static invoice to pay and have generated a keysend preimage
for the eventual payment. When the recipient comes back online, we'll
transition from this new state to Retryable and actually forward the HTLCs.
Allows us to repurpose this util to create blinded paths in an async payments
message context while maintaining support for the offers context.
Supported when the sender is an always-online node. Here we send the initial
held_htlc_available onion message upon receipt of a static invoice, next we'll
need to actually send HTLCs upon getting a response to said OM.
Because we may receive a static invoice to pay days before the recipient
actually comes back online to receive the payment, it's good to do as many
checks as we can up-front. Here we ensure that the blinded paths provided
in the invoice won't cause us to exceed the maximum onion packet size.
This new util will be able to send to both static and non-static BOLT 12
invoices.
Async receive is not yet supported.

Here we process inbound release_htlc onion messages, check that they actually
correspond to one of our outbound payments, and actually forward the HTLCs.
Valid release_htlc receipt indicates that the recipient has now come online to
receive.
Async payments may have very high expires because we may be waiting for days
for the recipient to come online, so it's important that users be able to
abandon these payments early if needed.
If someone sends us an unexpected or duplicate release_held_htlc onion message,
we should simply ignore it and not persist the entire ChannelManager in
response.
We want to specify that these methods are only to be used in an outbound offers
payment context, because we'll be adding similar methods for the outbound async
payments context in upcoming commits.
See AsyncPaymentsContext::hmac, but this prevents the recipient from
deanonymizing us. Without this, if they are able to guess the correct payment
id, then they could create a blinded path to us and confirm our identity.

We also move the PAYMENT_HASH_HMAC_INPUT const to use &[7; 16], which is safe
because this const was added since the last release. This ordering reads more
smoothly.
Currently used when initiating an async payment via held_htlc_available OM. This
OM needs a reply path back to us, so use this error for our invoice_error OM if
we fail to create said reply path.
While these variants may sound similar, they are very different. One is so
temporary it's never even persisted to disk, the other is a state we will stay
in for hours or days. See added docs for more info.
This field isn't necessary because we already authenticate the messages via the
blinded reply paths payment_id, nonce and HMAC.
@valentinewallace
Copy link
Contributor Author

Oops, and a rustfmt fix:

diff --git a/lightning/src/offers/static_invoice.rs b/lightning/src/offers/static_invoice.rs
index d083cae88..4910c57c5 100644
--- a/lightning/src/offers/static_invoice.rs
+++ b/lightning/src/offers/static_invoice.rs
@@ -315,9 +315,11 @@ impl StaticInvoice {
        }

        pub(crate) fn from_same_offer(&self, invreq: &InvoiceRequest) -> bool {
-               let invoice_offer_tlv_stream = TlvStream::new(&self.bytes).range(OFFER_TYPES)
+               let invoice_offer_tlv_stream = TlvStream::new(&self.bytes)
+                       .range(OFFER_TYPES)
                        .map(|tlv_record| tlv_record.record_bytes);
-               let invreq_offer_tlv_stream = TlvStream::new(invreq.bytes()).range(OFFER_TYPES)
+               let invreq_offer_tlv_stream = TlvStream::new(invreq.bytes())
+                       .range(OFFER_TYPES)
                        .map(|tlv_record| tlv_record.record_bytes);
                invoice_offer_tlv_stream.eq(invreq_offer_tlv_stream)
        }

@@ -249,6 +249,7 @@ impl<'a> TlvStream<'a> {
}

/// A slice into a [`TlvStream`] for a record.
#[derive(Eq, PartialEq)]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We can drop the derive now , too.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

A few things that will need following up on before we ship this, but good enough for landing behind a cfg flag.

///
/// [`ReleaseHeldHtlc`]: crate::onion_message::async_payments::ReleaseHeldHtlc
/// [`HeldHtlcAvailable`]: crate::onion_message::async_payments::HeldHtlcAvailable
nonce: Nonce,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't have commits that rewrite things done earlier in the same PR.

InvoiceReceived {
payment_hash: PaymentHash,
retry_strategy: Retry,
// Note this field is currently just replicated from AwaitingInvoice but not actually
// used anywhere.
max_total_routing_fee_msat: Option<u64>,
},
// This state applies when we are paying an often-offline recipient and another node on the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these comments not being added in the same commit that adds StaticInvoiceReceived? Also, presumably they should be doc comments (for those who look at internal docs, or use an IDE...though it doesn't matter to me).

} => {
let invreq = &retryable_invoice_request
.as_ref()
.ok_or(Bolt12PaymentError::UnexpectedInvoice)?
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is always gonna fail, right? RetryableInvoiceRequest is take()n pretty quickly after building the AwaitingInvoice to retry sending the invreq, so by the time we get an invoice its usually gone.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh, sorry... I should have caught this. Do we want to hold on to the invoice request indefinitely? Maybe that would useful in the future for proof-of-payer? Alternatively, we could store the OfferId instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably have to, right? We want to include the invreq in the payment HTLC.

@@ -11106,7 +11106,7 @@ where
fn release_held_htlc(&self, _message: ReleaseHeldHtlc, _context: AsyncPaymentsContext) {}

fn release_pending_messages(&self) -> Vec<(AsyncPaymentsMessage, MessageSendInstructions)> {
Vec::new()
core::mem::take(&mut self.pending_async_payments_messages.lock().unwrap())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why isn't this in the commit that adds pending_async_payments_messages?

@@ -10943,6 +10943,35 @@ where
let secp_ctx = &self.secp_ctx;
let expanded_key = &self.inbound_payment_key;

macro_rules! handle_pay_invoice_res {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this a separate commit? Why not just squash the macro into the previous commit instead of having non-DRY code at all?

@@ -967,6 +968,10 @@ impl OutboundPayments {
if !invoice.from_same_offer(invreq) {
return Err(Bolt12PaymentError::UnexpectedInvoice)
}
if invoice.invoice_features().requires_unknown_bits_from(&features) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this in a separate commit? Its exclusively updating code that was added in Support initiating an async payment to a static invoice., same for Set max path len on receipt of static invoice.

@@ -1725,6 +1907,22 @@ impl OutboundPayments {
true
}
},
PendingOutboundPayment::StaticInvoiceReceived { route_params, payment_hash, .. } => {
let is_stale =
route_params.payment_params.expiry_time.unwrap_or(u64::MAX) <
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is entirely set based on the static invoice, which means we may hold a pending outbound HTLC for a year or two if that's what the static invoice's expiry time is. We need a way to actually time these out. Maybe as a part of #3262 (but we shouldn't let it be a footgun where we hold a pending payment forever if a user forgets to override the value/maybe we should have a default of 1 week).

PaymentParameters::from_bolt12_invoice(&invoice), invoice.amount_msats()
);
if let Some(max_fee_msat) = max_total_routing_fee_msat {
route_params.max_total_routing_fee_msat = Some(max_fee_msat);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need a way to set a max total routing fee that is non-default on static invoice payments, though this isn't new, we need it generally for bolt 12 (#3262).

pub struct ReleaseHeldHtlc {
/// Used to release the HTLC held upstream if it matches the corresponding
/// [`HeldHtlcAvailable::payment_release_secret`].
pub payment_release_secret: [u8; 32],
Copy link
Collaborator

Choose a reason for hiding this comment

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

This commit could have gone first to avoid rewriting a bunch of code that was added in this PR, though I get that it was added late in the PR's life.

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