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

BOLT 12 static invoice encoding and building #3082

Merged

Conversation

valentinewallace
Copy link
Contributor

@valentinewallace valentinewallace commented May 23, 2024

Defines an interface for static BOLT 12 invoices, based on lightning/bolts#1149.

Similar to single-use BOLT 12 invoices, except without a payment hash. Currently may only be created from Offers.

I have a local branch successfully paying one of these invoices, though with a lot of checks that will require pre-factors commented out, e.g. PaymentPurpose checks.

  • Module-level docs
  • Make structs private

Copy link
Contributor Author

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

Conceptual feedback appreciated. Left some comments calling out specific things that might warrant discussion :)

if invoice.payment_paths.is_empty() {
return Err(Bolt12SemanticError::MissingPaths);
}
if invoice.offer.chains().len() > 1 {
Copy link
Contributor Author

@valentinewallace valentinewallace May 23, 2024

Choose a reason for hiding this comment

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

I'm currently disallowing offers with multiple chains because they complicate the AP spec + code, given that we’ll need to either add a separate chain field in the invoice TLV range or include a single invreq TLV in static invoices.

I'm not convinced multi-chain offers will ever make sense for often-offline receivers given they will always be using blinded paths, which don't seem likely to work across chains IIUC. There are currently no LN nodes impls that support running multiple chains simultaneously anyway. I'm sort of in favor of removing multi-chain support from the offers spec.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be checked on the passed Offer prior to creating the InvoiceContents

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I went ahead and fixed that in the latest push.

tagged_hash: TaggedHash,
}

macro_rules! invoice_accessors { ($self: ident, $contents: expr) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

~5 of these methods could theoretically be DRY'd further with their single-use invoice equivalents, but what makes it tricky is that the equivalent methods' docs reference Refunds. It seemed not worth spending too much time on given the methods themselves are like 3 lines.

/// Unless [`StaticInvoiceBuilder::relative_expiry`] is set, the invoice will expire 24 hours
/// after `created_at`.
pub fn for_offer_using_keys(
offer: &'a Offer, payment_paths: Vec<(BlindedPayInfo, BlindedPath)>, created_at: Duration,
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 considered whether it would make sense to allow creating these invoices from Refunds. Input welcome here, but my thinking is that if an async payer provides someone a refund then their always-online peer could hold onto a payee's single-use invoice via OM mailbox until the payer come online. This flow may need some fleshing out down the line, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right... Refunds are communicated out-of-band, so it seems a StaticInvoice wouldn't be relevant as it requires user action to read it and thus create the invoice. I'm a little confused about the scenario then because the payee (i.e., the user) should be online at this point though the payer (i.e., creator of the Refund) could possibly have gone offline before the payee sends them the invoice.

lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
@jkczyz jkczyz self-requested a review May 23, 2024 17:14
@valentinewallace
Copy link
Contributor Author

Pushed a fix for the test.

@codecov-commenter
Copy link

codecov-commenter commented May 23, 2024

Codecov Report

Attention: Patch coverage is 95.46859% with 44 lines in your changes missing coverage. Please review.

Project coverage is 91.17%. Comparing base (3cd1cb5) to head (bafe4ed).
Report is 75 commits behind head on main.

Files Patch % Lines
lightning/src/offers/static_invoice.rs 95.93% 22 Missing and 10 partials ⚠️
lightning/src/offers/invoice.rs 89.47% 10 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3082      +/-   ##
==========================================
+ Coverage   89.83%   91.17%   +1.33%     
==========================================
  Files         119      121       +2     
  Lines       97516   107815   +10299     
  Branches    97516   107815   +10299     
==========================================
+ Hits        87605    98296   +10691     
+ Misses       7332     7068     -264     
+ Partials     2579     2451     -128     

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

@TheBlueMatt
Copy link
Collaborator

Seems fine to me? I don't really have a strong opinion about this vs trying to reuse Invoice, happy to let @jkczyz chime in since its all his code here anyway.

@jkczyz
Copy link
Contributor

jkczyz commented May 29, 2024

Seems fine to me? I don't really have a strong opinion about this vs trying to reuse Invoice, happy to let @jkczyz chime in since its all his code here anyway.

Haven't taken a thorough look yet, but yeah my feeling was to make a separate type for static invoices. Maybe there could be a separate InvoiceContents variant instead, but it wouldn't be able to reuse InvoiceFields since that requires a PaymentHash. And then Invoice::payment_hash would need to return an Option, which I don't think we want.

@jkczyz
Copy link
Contributor

jkczyz commented May 29, 2024

Haven't taken a thorough look yet, but yeah my feeling was to make a separate type for static invoices. Maybe there could be a separate InvoiceContents variant instead, but it wouldn't be able to reuse InvoiceFields since that requires a PaymentHash. And then Invoice::payment_hash would need to return an Option, which I don't think we want.

A counterpoint might be that with #3078 we might want to have a single invoice type for the event. In that case, maybe we can live with an Option and where send_payment_for_bolt12_invoice can pay either. @valentinewallace What will the static invoice handling code look like? Will the onion message use a separate TLV for such invoice? Or will it reuse 66? Just thinking about the implications when it comes to parsing.

@TheBlueMatt
Copy link
Collaborator

Isn't part of the point of #3078 that people might want to look at the payment hash before they pay, in which case a single type won't help?

@valentinewallace
Copy link
Contributor Author

For reference, here are the pros and cons of using a separate type from where I stand:

Pros of separate type:

  • Avoids Optional return value in payment_hash and amount_msat methods
  • Avoids having non-applicable payer_* methods on static invoices, i.e. payer_id, invreq features
  • Because static invoices are required to come from offers (not refunds):
    • their docs can be more custom to them and not reference refunds
    • methods like offer_features can return non-Options

Cons of separate type:

  • a lot of not-easily-DRY’d extra code
  • may be annoying if we want to include static invoices in e.g. InvoiceReceived events
  • PaymentPurpose looks like it might need some reworking for static invoices (though this may also apply to re-purposing Bolt12Invoice)

I may be missing some since I didn’t go too far down the same-type road but this is what’s come up so far.

@valentinewallace
Copy link
Contributor Author

valentinewallace commented May 30, 2024

What will the static invoice handling code look like? Will the onion message use a separate TLV for such invoice? Or will it reuse 66? Just thinking about the implications when it comes to parsing.

Feel free to check out handle_message in the top commit of my WIP branch for handling: https://github.com/valentinewallace/rust-lightning/tree/2024-04-bolt12-keysend-invoice-2 (it doesn't include the async flow at all, just pays the static invoice immediately on receipt).

I used a separate TLV for static invoices, not 66.

@valentinewallace
Copy link
Contributor Author

Isn't part of the point of #3078 that people might want to look at the payment hash before they pay, in which case a single type won't help?

Is it possible that users may want to set their own keysend preimage? Not sure if InvoiceReceived may still be useful for them.

@jkczyz
Copy link
Contributor

jkczyz commented May 30, 2024

Isn't part of the point of #3078 that people might want to look at the payment hash before they pay, in which case a single type won't help?

Respond there regarding the sending case. It's more for receiving when the payment hash is important.

/// Unless [`StaticInvoiceBuilder::relative_expiry`] is set, the invoice will expire 24 hours
/// after `created_at`.
pub fn for_offer_using_keys(
offer: &'a Offer, payment_paths: Vec<(BlindedPayInfo, BlindedPath)>, created_at: Duration,
Copy link
Contributor

Choose a reason for hiding this comment

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

Right... Refunds are communicated out-of-band, so it seems a StaticInvoice wouldn't be relevant as it requires user action to read it and thus create the invoice. I'm a little confused about the scenario then because the payee (i.e., the user) should be online at this point though the payer (i.e., creator of the Refund) could possibly have gone offline before the payee sends them the invoice.

lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
if invoice.payment_paths.is_empty() {
return Err(Bolt12SemanticError::MissingPaths);
}
if invoice.offer.chains().len() > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be checked on the passed Offer prior to creating the InvoiceContents

lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
@valentinewallace
Copy link
Contributor Author

Added tests and fixed up some things in the latest push so gonna go ahead and take this out of draft. Still a few outstanding TODOs in the PR description. Thanks @jkczyz for the review 🫡

@valentinewallace
Copy link
Contributor Author

Addressed feedback, this should be good for review.

Going to skip adding code example module-level docs until the methods to create these new structs exist on ChannelManager, since that's currently the intended usage.

Looks like this needs rebase as well so lmk if I can go ahead with that.

@jkczyz
Copy link
Contributor

jkczyz commented Jun 4, 2024

Looks like this needs rebase as well so lmk if I can go ahead with that.

Yes, please go ahead and rebase.

lightning/src/offers/invoice_macros.rs Outdated Show resolved Hide resolved
lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
Will be useful when we later add support for static BOLT 12 invoices.
Will be useful when we support static BOLT 12 invoices.
lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/static_invoice.rs Show resolved Hide resolved
lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/invoice.rs Show resolved Hide resolved
@valentinewallace
Copy link
Contributor Author

Squashed with no changes.

Comment on lines +224 to +235
/// Paths to the node that may supply the invoice on the recipient's behalf, originating from
/// publicly reachable nodes. Taken from [`Offer::paths`].
///
/// [`Offer::paths`]: crate::offers::offer::Offer::paths
pub fn offer_message_paths(&$self) -> &[BlindedPath] {
$contents.offer_message_paths()
}

/// Paths to the recipient for indicating that a held HTLC is available to claim when they next
/// come online.
pub fn message_paths(&$self) -> &[BlindedPath] {
$contents.message_paths()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit inconsistent that Bolt12Invoice::message_paths and StaticInvoice::message_paths don't correspond to the same thing. We may want to re-think the naming here. Further, message_paths may not be descriptive enough especially when we also add paths for payment notification to the offer. Though I'm having trouble finding a way to differentiate naming the latter from the one added here.

Possible naming:

  • request_paths for offer_paths TLV
  • payment_paths for invoice_paths and invoice_blindedpay
  • notification_paths for paths add to offer for upcoming payment notifications
  • ??? for paths added to invoice for async payments

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't feel super strongly since users are unlikely to be accessing these fields directly, but I'm happy to rename these methods. Since we're pulling in other considerations like payment notifications and renaming offer fields as well, I'm working on a follow-up based on this PR that should be open sometime today, if that works for you. Naming suggestions SGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #3118.

.unwrap()
.build_and_sign(&secp_ctx)
.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, strange that it's inconsistent with when the first call can fit on one line.

$contents.offer_message_paths()
}

/// Paths to the recipient for indicating that a held HTLC is available to claim when they next
Copy link
Contributor

Choose a reason for hiding this comment

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

For refreshing my understanding, who is holding the HTLC here? Is this for sending held_htlc_available? Do you have a reference for this flow? I didn't see it in the spec PR other than:

  • if invoice_payment_hash is unset:
    • MUST reject the invoice if invoice_message_paths is not present or is empty.
    • MUST pay asynchronously using the held_htlc_available onion message
      flow, where the onion message is sent over invoice_message_paths.

Copy link
Contributor Author

@valentinewallace valentinewallace Jun 12, 2024

Choose a reason for hiding this comment

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

who is holding the HTLC here?

If the sender is often-offline, then their LSP is holding the HTLC. Otherwise, the sender holds the HTLC themselves until the recipient comes online.

Is this for sending held_htlc_available?

Yep!

For reference, lightning/bolts#1149 is based on lightning/bolts#989, which is the flow referred to in the quote. Let me know if something's unclear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thanks! I was only looking at the last commits and not also the first ones. 🤦‍♂️

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.

Basically LGTM

lightning/src/offers/invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/invoice.rs Outdated Show resolved Hide resolved
Will be used in static invoices. Also test that we'll fail to decode if these
paths are included in single-use BOLT 12 invoices.
@valentinewallace
Copy link
Contributor Author

Went ahead and squashed in updates:

diff --git a/lightning/src/offers/invoice.rs b/lightning/src/offers/invoice.rs
index c570aed66..6695121b3 100644
--- a/lightning/src/offers/invoice.rs
+++ b/lightning/src/offers/invoice.rs
@@ -135,7 +135,7 @@ use std::time::SystemTime;

 pub(crate) const DEFAULT_RELATIVE_EXPIRY: Duration = Duration::from_secs(7200);

-/// Tag for the hash function used when signing a BOLT 12 invoice's merkle root.
+/// Tag for the hash function used when signing a [`Bolt12Invoice`]'s merkle root.
 pub const SIGNATURE_TAG: &'static str = concat!("lightning", "invoice", "signature");

 /// Builds a [`Bolt12Invoice`] from either:
@@ -1164,7 +1164,7 @@ tlv_stream!(InvoiceTlvStream, InvoiceTlvStreamRef, 160..240, {
        (174, features: (Bolt12InvoiceFeatures, WithoutLength)),
        (176, node_id: PublicKey),
        // Only present in `StaticInvoice`s.
-       (178, message_paths: (Vec<BlindedPath>, WithoutLength)),
+       (238, message_paths: (Vec<BlindedPath>, WithoutLength)),
 });

 pub(super) type BlindedPathIter<'a> = core::iter::Map<
diff --git a/lightning/src/offers/static_invoice.rs b/lightning/src/offers/static_invoice.rs
index dd132bde7..77e2b1211 100644
--- a/lightning/src/offers/static_invoice.rs
+++ b/lightning/src/offers/static_invoice.rs
@@ -17,7 +17,6 @@ use crate::ln::msgs::DecodeError;
 use crate::offers::invoice::{
        check_invoice_signing_pubkey, construct_payment_paths, filter_fallbacks, BlindedPathIter,
        BlindedPayInfo, BlindedPayInfoIter, FallbackAddress, InvoiceTlvStream, InvoiceTlvStreamRef,
-       SIGNATURE_TAG,
 };
 use crate::offers::invoice_macros::{invoice_accessors_common, invoice_builder_methods_common};
 use crate::offers::merkle::{
@@ -43,8 +42,11 @@ use crate::offers::invoice::is_expired;
 #[allow(unused_imports)]
 use crate::prelude::*;

-/// Static invoices default to expiring after 24 hours.
-const DEFAULT_RELATIVE_EXPIRY: Duration = Duration::from_secs(3600 * 24);
+/// Static invoices default to expiring after 2 weeks.
+const DEFAULT_RELATIVE_EXPIRY: Duration = Duration::from_secs(3600 * 24 * 14);
+
+/// Tag for the hash function used when signing a [`StaticInvoice`]'s merkle root.
+pub const SIGNATURE_TAG: &'static str = concat!("lightning", "static_invoice", "signature");

 /// A `StaticInvoice` is a reusable payment request corresponding to an [`Offer`].
 ///
@@ -566,13 +568,13 @@ mod tests {
        use crate::ln::features::{Bolt12InvoiceFeatures, OfferFeatures};
        use crate::ln::inbound_payment::ExpandedKey;
        use crate::ln::msgs::DecodeError;
-       use crate::offers::invoice::{InvoiceTlvStreamRef, SIGNATURE_TAG};
+       use crate::offers::invoice::InvoiceTlvStreamRef;
        use crate::offers::merkle;
        use crate::offers::merkle::{SignatureTlvStreamRef, TaggedHash};
        use crate::offers::offer::{Offer, OfferBuilder, OfferTlvStreamRef, Quantity};
        use crate::offers::parse::{Bolt12ParseError, Bolt12SemanticError};
        use crate::offers::static_invoice::{
-               StaticInvoice, StaticInvoiceBuilder, DEFAULT_RELATIVE_EXPIRY,
+               StaticInvoice, StaticInvoiceBuilder, DEFAULT_RELATIVE_EXPIRY, SIGNATURE_TAG,
        };
        use crate::offers::test_utils::*;
        use crate::sign::KeyMaterial;

jkczyz
jkczyz previously approved these changes Jun 12, 2024
lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
$contents.offer_message_paths()
}

/// Paths to the recipient for indicating that a held HTLC is available to claim when they next
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thanks! I was only looking at the last commits and not also the first ones. 🤦‍♂️

Define an interface for BOLT 12 static invoice messages. The underlying
format consists of the original bytes and the parsed contents.

The bytes are later needed for serialization. This is because it must
mirror all the offer TLV records, including unknown ones, which aren't
represented in the contents.

Invoices may be created from an offer.
@valentinewallace
Copy link
Contributor Author

Squashed in the latest change with the following diff:

diff --git a/lightning/src/offers/static_invoice.rs b/lightning/src/offers/static_invoice.rs
index 77e2b1211..c6b6fa766 100644
--- a/lightning/src/offers/static_invoice.rs
+++ b/lightning/src/offers/static_invoice.rs
@@ -428,8 +428,7 @@ impl InvoiceContents {
        }

        fn fallbacks(&self) -> Vec<Address> {
-               let chain =
-                       self.offer.chains().first().cloned().unwrap_or_else(|| self.offer.implied_chain());
+               let chain = self.chain();
                self.fallbacks
                        .as_ref()
                        .map(|fallbacks| filter_fallbacks(chain, fallbacks))

jkczyz
jkczyz previously approved these changes Jun 12, 2024
TheBlueMatt
TheBlueMatt previously approved these changes Jun 12, 2024
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.

Please add a fuzzer for the new type in a followup.

lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/static_invoice.rs Outdated Show resolved Hide resolved
Add a builder for creating static invoices for an offer. Building produces a
semantically valid static invoice for the offer, which can then be signed with
the key associated with the offer's signing pubkey.
@valentinewallace
Copy link
Contributor Author

Updated the outdated docs:

diff --git a/lightning/src/offers/static_invoice.rs b/lightning/src/offers/static_invoice.rs
index c6b6fa766..d0846b29a 100644
--- a/lightning/src/offers/static_invoice.rs
+++ b/lightning/src/offers/static_invoice.rs
@@ -82,12 +82,9 @@ struct InvoiceContents {

 /// Builds a [`StaticInvoice`] from an [`Offer`].
 ///
-/// See [module-level documentation] for usage.
-///
 /// [`Offer`]: crate::offers::offer::Offer
-/// [`InvoiceRequest`]: crate::offers::invoice_request::InvoiceRequest
-/// [`Bolt12Invoice`]: crate::offers::invoice::Bolt12Invoice
 /// This is not exported to bindings users as builder patterns don't map outside of move semantics.
+// TODO: add module-level docs and link here
 pub struct StaticInvoiceBuilder<'a> {
        offer_bytes: &'a Vec<u8>,
        invoice: InvoiceContents,

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.

Please add fuzzing in a followup :)

@valentinewallace
Copy link
Contributor Author

Please add fuzzing in a followup :)

Will do, I added this to the tracking issue and will hopefully get to it soon.

@TheBlueMatt TheBlueMatt merged commit f267ffe into lightningdevkit:main Jun 12, 2024
13 of 16 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