BOLT 12 `invoice` encoding and building by jkczyz · Pull Request #1926 · lightningdevkit/rust-lightning

jkczyz · 2022-12-20T16:26:19Z

Define an interface for BOLT 12 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 and invoice_request TLV records, including unknown ones, which aren't represented in the contents.

Invoices may be created for either an Offer (from an InvoiceRequest) or a Refund. As such, the provided InvoiceBuilder can be created for either use case. The primary difference is how the signing pubkey is given -- by the writer of the offer or the reader of the refund.

jkczyz · 2022-12-20T16:27:08Z

Still needs tests and some example builder docs, but otherwise should be fully implemented.

jkczyz · 2022-12-20T16:43:05Z

@TheBlueMatt @valentinewallace Let me know if you'd like me to split the PR. Could possibly have the first five commits in as a pre-work PR.

valentinewallace · 2022-12-20T17:32:43Z

@TheBlueMatt @valentinewallace Let me know if you'd like me to split the PR. Could possibly have the first five commits in as a pre-work PR.

Yeah, I'd be in favor of that if you don't mind

jkczyz · 2022-12-20T19:05:09Z

Opened #1927, including an additional commit that I moved up.

TheBlueMatt

Basically LGTM.

TheBlueMatt · 2023-01-06T23:05:57Z

+ &self.contents.fields().paths[..]
+	}
+
+	/// Information for each hop in [`Invoice::paths`] needed for routing payments across it.


This is confusing - a BlindedPath is a path encompassing multiple hops. I assume this really means information for each path?

Oh, right. Was confused for a second, but yeah this information is for all hops on the path, in aggregate, IIUC.

valentinewallace

Still going through a first pass. I assume tests are imminent

valentinewallace · 2023-01-12T01:29:34Z

+ invoice_request: &'a InvoiceRequest, paths: Vec<BlindedPath>, payinfo: Vec<BlindedPayInfo>,
+ created_at: Duration, payment_hash: PaymentHash
+	) -> Result<Self, SemanticError> {
+ let amount_msats = match invoice_request.amount_msats() {


Weird that invoice_request.amount_msats() doesn't return the offer's amount in msats if it's available, since that is its amount

This is largely to avoid using unreachable!(). Technically, the two errors below are unreachable, but if we move the code into InvoiceRequest::amount_msats(), then they could return None instead, I suppose? But if we add currency conversion, we'd likely want to do it at the builder-level, I think.
Related discussion from the last PR: #1927 (comment)

I still think invoice_request.amount_msats() should return an Option to avoid an unreachable, just think it should return self.contents.amount_msats.or(offer.contents.amount_msats()) (i.e. None if the offer has a Currency amount), because the offer's amount is its amount in that case IIUC. Anyway, can be punted

There was some confusion over if we should support sending to a non-bitcoin-denominated offer, which was my fault, we can/should revisit this when we add that support.

valentinewallace · 2023-01-12T01:43:54Z

+	///
+	/// [`Invoice`]: crate::offers::invoice::Invoice
+	pub fn respond_with(
+ &self, paths: Vec<BlindedPath>, payinfo: Vec<BlindedPayInfo>, created_at: Duration,


Should we have std-only versions that automatically supplies created_at?

End up making the parameter no-std only and otherwise automated as suggested.

codecov-commenter · 2023-01-13T15:24:04Z

Codecov Report

Base: 90.73% // Head: 91.39% // Increases project coverage by +0.66% 🎉

Coverage data is based on head (15f1295) compared to base (ad40573).
Patch coverage: 93.80% of modified lines in pull request are covered.

Additional details and impacted files

@@ Coverage Diff @@## main #1926 +/- ##
==========================================
+ Coverage 90.73% 91.39% +0.66% 
==========================================
Files 97 98 +1 Lines 50604 57080 +6476 Branches 50604 57080 +6476 ==========================================
+ Hits 45914 52167 +6253 - Misses 4690 4913 +223

Impacted Files	Coverage Δ
lightning/src/offers/parse.rs	`94.00% <ø> (ø)`
lightning/src/routing/gossip.rs	`92.15% <ø> (ø)`
lightning/src/util/ser_macros.rs	`86.73% <50.00%> (-0.30%)`	⬇️
lightning/src/offers/invoice_request.rs	`96.12% <62.50%> (+1.12%)`	⬆️
lightning/src/offers/refund.rs	`95.89% <68.75%> (+2.15%)`	⬆️
lightning/src/util/ser.rs	`91.41% <82.35%> (-0.30%)`	⬇️
lightning/src/offers/offer.rs	`93.33% <91.66%> (+0.06%)`	⬆️
lightning/src/offers/invoice.rs	`94.79% <94.79%> (ø)`
lightning/src/offers/merkle.rs	`100.00% <100.00%> (ø)`
lightning/src/util/enforcing_trait_impls.rs	`81.15% <0.00%> (-1.77%)`	⬇️
... and 21 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt · 2023-01-15T04:41:26Z

This all basically LGTM, feel free to squash and fixup the CI failures and I'm ready to land.

jkczyz · 2023-01-16T06:03:34Z

Added tests, but still need to add an example.

Also, see 8c76a69 for small clean-up such that Iterable is not special-cased. Open to a better name for that as well.

jkczyz · 2023-01-18T16:07:03Z

Added examples in the module docs and missing cross-linking. PTAL.

jkczyz · 2023-01-18T16:19:18Z

 $field = Some($trait::read(&mut $reader $(, $read_arg)*)?);
 	}};
+	($reader: expr, $field: ident, (option, encoding: ($fieldty: ty, $encoding: ident, $encoder:ty))) => {{
+ decode_tlv!($reader, $field, (option, encoding: ($fieldty, $encoding)));


Looks like I'll need to rebase to account for #1823. Let me know when it's good to squash and rebase.

Go for it, IMO.

Explicitly using TlvRecord::record_bytes makes reading the code more obvious than hiding which bytes are used in AsRef<[u8]>::as_ref.

Provide a helper for skipping signature TLV records from a TLV stream. This prevents needing to duplicate the check for signature TLV records when writing a TLV stream without signatures in an upcoming commit.

When using bytes from an InvoiceRequest to constructing bytes for an Invoice, any signature TLV records in the bytes must be excluded. Define a wrapper for encoding such pre-serialized bytes in this manner. This will allow the forthcoming InvoiceBuilder to construct bytes for an Invoice properly.

jkczyz · 2023-01-18T21:47:12Z

Rebased and squashed though added a missed parsing check on the signing pubkey after reviewing the spec. It's already handled correctly in the builder.

diff --git a/lightning/src/offers/invoice.rs b/lightning/src/offers/invoice.rs
index 9b5fe3fd..2e6fcf05 100644
--- a/lightning/src/offers/invoice.rs+++ b/lightning/src/offers/invoice.rs@@ -705,7 +705,11 @@ impl TryFrom<PartialInvoiceTlvStream> for InvoiceContents {
};
match offer_tlv_stream.node_id {
- Some(_) => {+ Some(expected_signing_pubkey) => {+ if fields.signing_pubkey != expected_signing_pubkey {+ return Err(SemanticError::InvalidSigningPubkey);+ }+
let invoice_request = InvoiceRequestContents::try_from(
(payer_tlv_stream, offer_tlv_stream, invoice_request_tlv_stream)
)?;
@@ -1455,6 +1459,17 @@ mod tests {
assert_eq!(e, ParseError::InvalidSemantics(SemanticError::MissingSigningPubkey));
},
}
++ let invalid_pubkey = payer_pubkey();+ let mut tlv_stream = invoice.as_tlv_stream();+ tlv_stream.3.node_id = Some(&invalid_pubkey);++ match Invoice::try_from(tlv_stream.to_bytes()) {+ Ok(_) => panic!("expected error"),+ Err(e) => {+ assert_eq!(e, ParseError::InvalidSemantics(SemanticError::InvalidSigningPubkey));+ },+ }
}
#[test]
diff --git a/lightning/src/offers/parse.rs b/lightning/src/offers/parse.rs
index 904ce403..a7d13e57 100644
--- a/lightning/src/offers/parse.rs+++ b/lightning/src/offers/parse.rs@@ -147,6 +147,8 @@ pub enum SemanticError {
MissingDescription,
/// A signing pubkey was not provided.
MissingSigningPubkey,
+	/// A signing pubkey was provided but a different one was expected.+	InvalidSigningPubkey,
/// A signing pubkey was provided but was not expected.
UnexpectedSigningPubkey,
/// A quantity was expected but was missing.

TheBlueMatt · 2023-01-18T23:10:37Z

+ network,
+ };
+
+ if !address.is_standard() {


Hmm, this is probably too restrictive - we want to send to it even if its a v3 witness program or whatever, its not our job to enforce forwards compat. rust-bitcoin/rust-bitcoin#1558

Yeah, that does make sense. I may have been reading too much into this part of the spec:
MUST ignore any fallback_address for which address does not meet known requirements for the given version
Since there wouldn't be any known requirements for, say, version 3, we shouldn't ignore them, I guess?

I thought there were requirements around v0 scripts being only exactly the "correct" length but otherwise none. I could be wrong, but I'd love for rust-bitcoin upstream to fix this.

Yeah, length of V0 and V1 scripts are covered by is_standard but other versions are considered non-standard. Best to have it fixed upstream, I agree, but we'll have to explicitly allow for other versions now, it seems?

Yea, I think we should just backport the v0 length check, note that there is no length restriction on v1 - see https://github.com/bitcoin/bitcoin/blob/e9262ea32a6e1d364fb7974844fadc36f931f8c6/src/script/standard.cpp#L183-L201

Looks like V1 must be WITNESS_V1_TAPROOT_SIZE (32), which is what Address::is_standard checks.

Oh, nevermind. I see the fall-through case is fine with it.

Ok, now we only skip V0 addresses that aren't the right size.

TheBlueMatt · 2023-01-18T23:53:08Z

+ &self, payment_paths: Vec<(BlindedPath, BlindedPayInfo)>, payment_hash: PaymentHash,
+ signing_pubkey: PublicKey,
+ #[cfg(any(test, not(feature = "std")))]
+ created_at: Duration


Any idea of docs.rs will document this in a sane way?

Hmm... it should be using std according to our Cargo.toml file:
rust-lightning/lightning/Cargo.toml
Lines 14 to 16 in ad40573
[package.metadata.docs.rs]
features = ["std"]
rustdoc-args = ["--cfg", "docsrs"]
So I think that means it should leave the parameter out.

Hmm, but isn't that confusing? The docs will only be correct for std users and no-std users will be left wondering why their code doesn't compile?

Yeah, would be best to note in the docs then. Not sure if there's a better way. Even if we made separate methods, the docs will only list the std one.

IIRC docsrs is smart and magic and will list all features with this is only available with feature X tags, but maybe we'd have to enable both no-std and std for that?

Yeah, we'd have to enable both. Not sure how much time I want to spend getting that to work at the moment. Updated the docs for now.

TheBlueMatt · 2023-01-19T00:06:40Z

Oops, no-std builds are failing on the new serialization iterable things.

jkczyz · 2023-01-19T00:08:54Z

New push fixes a problem with no-std:

diff --git a/lightning/src/offers/invoice.rs b/lightning/src/offers/invoice.rs
index 2e6fcf05..fbf193a5 100644
--- a/lightning/src/offers/invoice.rs+++ b/lightning/src/offers/invoice.rs@@ -550,12 +550,12 @@ tlv_stream!(InvoiceTlvStream, InvoiceTlvStreamRef, 160..240, {
(176, node_id: PublicKey),
});
-type BlindedPathIter<'a> = std::iter::Map<+type BlindedPathIter<'a> = core::iter::Map<
core::slice::Iter<'a, (BlindedPath, BlindedPayInfo)>,
for<'r> fn(&'r (BlindedPath, BlindedPayInfo)) -> &'r BlindedPath,
>;
-type BlindedPayInfoIter<'a> = std::iter::Map<+type BlindedPayInfoIter<'a> = core::iter::Map<
core::slice::Iter<'a, (BlindedPath, BlindedPayInfo)>,
for<'r> fn(&'r (BlindedPath, BlindedPayInfo)) -> &'r BlindedPayInfo,
>;

TheBlueMatt · 2023-01-19T01:29:52Z

Looks like no-std is still sad.

valentinewallace

Basically LGTM

valentinewallace · 2023-01-19T01:15:24Z

+ self
+	}
+
+	/// Adds a P2WSH address to [`Invoice::fallbacks`].


Is there any reason users would want one type of fallback address over another? May be nice to document

Ah, sorry, I don't know if the current additions add much (am I missing something?). I was thinking more of the tradeoffs/use cases between p2wsh vs p2wpkh vs taproot, or a link to a reference for something like that. May be out of our purview.

Yeah, seems more like a wallet concern.

valentinewallace · 2023-01-19T20:50:53Z

+ return None;
+ }
+
+ let version = address.version;


This may have been discussed already, but the spec also specifies MUST ignore any fallback_address for which version is greater than 16.

Ah, right. We were failing to parse instead. I've updated FallbackAddress to use u8 and now ignore any address with version greater than 16. Should be fine given FallbackAddress is not public.

Define an interface for BOLT 12 `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` and `invoice_request` TLV records, including unknown ones, which aren't represented in the contents. Invoices may be created for an Offer (from an InvoiceRequest) or for a Refund. The primary difference is how the signing pubkey is given -- by the writer of the offer or the reader of the refund.

Add a builder for creating invoices for an offer from a given request and required fields. Other settings are optional and duplicative settings will override previous settings. Building produces a semantically valid `invoice` message for the offer, which then can be signed with the key associated with the offer's signing pubkey.

Add a builder for creating invoices for a refund and required fields. Other settings are optional and duplicative settings will override previous settings. Building produces a semantically valid `invoice` message for the refund, which then can be signed with the key associated with the provided signing pubkey.

For std builds, Invoice::created_at can be automatically set upon construction using SystemTime::now() offset by SystemTime::UNIX_EPOCH. Change InvoiceRequest::respond_with and Refund::respond_with to only take a created_at parameter in no-std builds.

Tests for checking invoice message semantics when building an invoice as defined by BOLT 12.

Tests for checking invoice semantics when parsing invoice bytes as defined by BOLT 12.

valentinewallace

Went through the spec again and garnered some more feedback, but I think it can all be saved for follow-up

valentinewallace · 2023-01-21T00:35:38Z

+ invoice_request: &'a InvoiceRequest, paths: Vec<BlindedPath>, payinfo: Vec<BlindedPayInfo>,
+ created_at: Duration, payment_hash: PaymentHash
+	) -> Result<Self, SemanticError> {
+ let amount_msats = match invoice_request.amount_msats() {


I still think invoice_request.amount_msats() should return an Option to avoid an unreachable, just think it should return self.contents.amount_msats.or(offer.contents.amount_msats()) (i.e. None if the offer has a Currency amount), because the offer's amount is its amount in that case IIUC. Anyway, can be punted

valentinewallace · 2023-01-21T00:41:10Z

+ self
+	}
+
+	/// Adds a P2WSH address to [`Invoice::fallbacks`].


Ah, sorry, I don't know if the current additions add much (am I missing something?). I was thinking more of the tradeoffs/use cases between p2wsh vs p2wpkh vs taproot, or a link to a reference for something like that. May be out of our purview.

valentinewallace · 2023-01-21T00:42:43Z

+	/// The caller is expected to remember the preimage of `payment_hash` in order to claim a payment
+	/// for the invoice.
+	///
+	/// The `payment_paths` parameter is useful for maintaining the payment recipient's privacy. It


I think this could use more details about why it's required even when you're using a public node id. Maybe could live on Invoice::payment_paths with a docs link here

valentinewallace · 2023-01-21T00:52:36Z

+	/// must be given, which is used to set [`Invoice::created_at`] since [`std::time::SystemTime`]
+	/// is not available.
+	///
+	/// The caller is expected to remember the preimage of `payment_hash` in order to claim a payment


lightning-invoice has links to ChannelManager::create_inbound_payment*, may want some here too

We'll wait until the code is used in onion message handling before documenting. For Offers that code will construct the payment hash, though responding to a Refund may need some utility.

valentinewallace · 2023-01-21T00:59:03Z

+
+impl<'a> UnsignedInvoice<'a> {
+	/// Signs the invoice using the given function.
+	pub fn sign<F, E>(self, sign: F) -> Result<Invoice, SignError<E>>


Will we need a new method on NodeSigner for this, or does NodeSigner::sign_invoice work? May want to add some documentation for how users will likely want to practically sign this using LDK

Yeah, will need a dedicated method in a later PR. Was wondering what the interface should be given a the merkle root is what needs to be signed and would need to be calculated from the bytes as formed below (i.e., invoice_request_bytes which excludes the payer's signature and includes unknown fields).
cc: @TheBlueMatt

Yea, not sure what the right API is exactly but I think our target should basically be an unsigned list of fields and a util method to construct the merkle root.

valentinewallace · 2023-01-21T01:09:41Z

+	/// for the invoice.
+	///
+	/// The `payment_paths` parameter is useful for maintaining the payment recipient's privacy. It
+	/// must contain one or more elements.


Should also note that these should be specified in order of most preferred to least preferred

jkczyz

Went through the spec again and garnered some more feedback, but I think it can all be saved for follow-up

Let's leave remaining comments to a follow-up (maybe in #1972) to avoid needing to rebase #1977.

jkczyz · 2023-01-21T03:56:04Z

+ self
+	}
+
+	/// Adds a P2WSH address to [`Invoice::fallbacks`].


Yeah, seems more like a wallet concern.

jkczyz · 2023-01-21T04:06:04Z

+	/// must be given, which is used to set [`Invoice::created_at`] since [`std::time::SystemTime`]
+	/// is not available.
+	///
+	/// The caller is expected to remember the preimage of `payment_hash` in order to claim a payment


We'll wait until the code is used in onion message handling before documenting. For Offers that code will construct the payment hash, though responding to a Refund may need some utility.

jkczyz · 2023-01-23T20:38:06Z

+
+impl<'a> UnsignedInvoice<'a> {
+	/// Signs the invoice using the given function.
+	pub fn sign<F, E>(self, sign: F) -> Result<Invoice, SignError<E>>


Yeah, will need a dedicated method in a later PR. Was wondering what the interface should be given a the merkle root is what needs to be signed and would need to be calculated from the bytes as formed below (i.e., invoice_request_bytes which excludes the payer's signature and includes unknown fields).
cc: @TheBlueMatt

0.0.114 - Mar 3, 2023 - "Faster Async BOLT12 Retries" API Updates =========== * `InvoicePayer` has been removed and its features moved directly into `ChannelManager`. As such it now requires a simplified `Router` and supports `send_payment_with_retry` (and friends). `ChannelManager::retry_payment` was removed in favor of the automated retries. Invoice payment utilities in `lightning-invoice` now call the new code (lightningdevkit#1812, lightningdevkit#1916, lightningdevkit#1929, lightningdevkit#2007, etc). * `Sign`/`BaseSign` has been renamed `ChannelSigner`, with `EcdsaChannelSigner` split out in anticipation of future schnorr/taproot support (lightningdevkit#1967). * The catch-all `KeysInterface` was split into `EntropySource`, `NodeSigner`, and `SignerProvider`. `KeysManager` implements all three (lightningdevkit#1910, lightningdevkit#1930). * `KeysInterface::get_node_secret` is now `KeysManager::get_node_secret_key` and is no longer required for external signers (lightningdevkit#1951, lightningdevkit#2070). * A `lightning-transaction-sync` crate has been added which implements keeping LDK in sync with the chain via an esplora server (lightningdevkit#1870). Note that it can only be used on nodes that *never* ran a previous version of LDK. * `Score` is updated in `BackgroundProcessor` instead of via `Router` (lightningdevkit#1996). * `ChainAccess::get_utxo` (now `UtxoAccess`) can now be resolved async (lightningdevkit#1980). * BOLT12 `Offer`, `InvoiceRequest`, `Invoice` and `Refund` structs as well as associated builders have been added. Such invoices cannot yet be paid due to missing support for blinded path payments (lightningdevkit#1927, lightningdevkit#1908, lightningdevkit#1926). * A `lightning-custom-message` crate has been added to make combining multiple custom messages into one enum/handler easier (lightningdevkit#1832). * `Event::PaymentPathFailure` is now generated for failure to send an HTLC over the first hop on our local channel (lightningdevkit#2014, lightningdevkit#2043). * `lightning-net-tokio` no longer requires an `Arc` on `PeerManager` (lightningdevkit#1968). * `ChannelManager::list_recent_payments` was added (lightningdevkit#1873). * `lightning-background-processor` `std` is now optional in async mode (lightningdevkit#1962). * `create_phantom_invoice` can now be used in `no-std` (lightningdevkit#1985). * The required final CLTV delta on inbound payments is now configurable (lightningdevkit#1878) * bitcoind RPC error code and message are now surfaced in `block-sync` (lightningdevkit#2057). * Get `historical_estimated_channel_liquidity_probabilities` was added (lightningdevkit#1961). * `ChannelManager::fail_htlc_backwards_with_reason` was added (lightningdevkit#1948). * Macros which implement serialization using TLVs or straight writing of struct fields are now public (lightningdevkit#1823, lightningdevkit#1976, lightningdevkit#1977). Backwards Compatibility ======================= * Any inbound payments with a custom final CLTV delta will be rejected by LDK if you downgrade prior to receipt (lightningdevkit#1878). * `Event::PaymentPathFailed::network_update` will always be `None` if an 0.0.114-generated event is read by a prior version of LDK (lightningdevkit#2043). * `Event::PaymentPathFailed::all_paths_removed` will always be false if an 0.0.114-generated event is read by a prior version of LDK. Users who rely on it to determine payment retries should migrate to `Event::PaymentFailed`, in a separate release prior to upgrading to LDK 0.0.114 if downgrading is supported (lightningdevkit#2043). Performance Improvements ======================== * Channel data is now stored per-peer and channel updates across multiple peers can be operated on simultaneously (lightningdevkit#1507). * Routefinding is roughly 1.5x faster (lightningdevkit#1799). * Deserializing a `NetworkGraph` is roughly 6x faster (lightningdevkit#2016). * Memory usage for a `NetworkGraph` has been reduced substantially (lightningdevkit#2040). * `KeysInterface::get_secure_random_bytes` is roughly 200x faster (lightningdevkit#1974). Bug Fixes ========= * Fixed a bug where a delay in processing a `PaymentSent` event longer than the time taken to persist a `ChannelMonitor` update, when occurring immediately prior to a crash, may result in the `PaymentSent` event being lost (lightningdevkit#2048). * Fixed spurious rejections of rapid gossip sync data when the graph has been updated by other means between gossip syncs (lightningdevkit#2046). * Fixed a panic in `KeysManager` when the high bit of `starting_time_nanos` is set (lightningdevkit#1935). * Resolved an issue where the `ChannelManager::get_persistable_update_future` future would fail to wake until a second notification occurs (lightningdevkit#2064). * Resolved a memory leak when using `ChannelManager::send_probe` (lightningdevkit#2037). * Fixed a deadlock on some platforms at least when using async `ChannelMonitor` updating (lightningdevkit#2006). * Removed debug-only assertions which were reachable in threaded code (lightningdevkit#1964). * In some cases when payment sending fails on our local channel retries no longer take the same path and thus never succeed (lightningdevkit#2014). * Retries for spontaneous payments have been fixed (lightningdevkit#2002). * Return an `Err` if `lightning-persister` fails to read the directory listing rather than panicing (lightningdevkit#1943). * `peer_disconnected` will now never be called without `peer_connected` (lightningdevkit#2035) Security ======== 0.0.114 fixes several denial-of-service vulnerabilities which are reachable from untrusted input from channel counterparties or in deployments accepting inbound connections or channels. It also fixes a denial-of-service vulnerability in rare cases in the route finding logic. * The number of pending un-funded channels as well as peers without funded channels is now limited to avoid denial of service (lightningdevkit#1988). * A second `channel_ready` message received immediately after the first could lead to a spurious panic (lightningdevkit#2071). This issue was introduced with 0conf support in LDK 0.0.107. * A division-by-zero issue was fixed in the `ProbabilisticScorer` if the amount being sent (including previous-hop fees) is equal to a channel's capacity while walking the graph (lightningdevkit#2072). The division-by-zero was introduced with historical data tracking in LDK 0.0.112. In total, this release features 130 files changed, 21457 insertions, 10113 deletions in 343 commits from 18 authors, in alphabetical order: * Alec Chen * Allan Douglas R. de Oliveira * Andrei * Arik Sosman * Daniel Granhão * Duncan Dean * Elias Rohrer * Jeffrey Czyz * John Cantrell * Kurtsley * Matt Corallo * Max Fang * Omer Yacine * Valentine Wallace * Viktor Tigerström * Wilmer Paulino * benthecarman * jurvis

jkczyz · 2022-12-20T16:26:19Z

Define an interface for BOLT 12 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 and invoice_request TLV records, including unknown ones, which aren't represented in the contents.

Invoices may be created for either an Offer (from an InvoiceRequest) or a Refund. As such, the provided InvoiceBuilder can be created for either use case. The primary difference is how the signing pubkey is given -- by the writer of the offer or the reader of the refund.

jkczyz · 2022-12-20T16:27:08Z

Still needs tests and some example builder docs, but otherwise should be fully implemented.

jkczyz · 2022-12-20T16:43:05Z

@TheBlueMatt @valentinewallace Let me know if you'd like me to split the PR. Could possibly have the first five commits in as a pre-work PR.

valentinewallace · 2022-12-20T17:32:43Z

@TheBlueMatt @valentinewallace Let me know if you'd like me to split the PR. Could possibly have the first five commits in as a pre-work PR.

Yeah, I'd be in favor of that if you don't mind

jkczyz · 2022-12-20T19:05:09Z

Opened #1927, including an additional commit that I moved up.

TheBlueMatt

Basically LGTM.

TheBlueMatt · 2023-01-06T23:05:57Z

+ &self.contents.fields().paths[..]
+	}
+
+	/// Information for each hop in [`Invoice::paths`] needed for routing payments across it.


This is confusing - a BlindedPath is a path encompassing multiple hops. I assume this really means information for each path?

Oh, right. Was confused for a second, but yeah this information is for all hops on the path, in aggregate, IIUC.

valentinewallace

Still going through a first pass. I assume tests are imminent

valentinewallace · 2023-01-12T01:29:34Z

+ invoice_request: &'a InvoiceRequest, paths: Vec<BlindedPath>, payinfo: Vec<BlindedPayInfo>,
+ created_at: Duration, payment_hash: PaymentHash
+	) -> Result<Self, SemanticError> {
+ let amount_msats = match invoice_request.amount_msats() {


Weird that invoice_request.amount_msats() doesn't return the offer's amount in msats if it's available, since that is its amount

This is largely to avoid using unreachable!(). Technically, the two errors below are unreachable, but if we move the code into InvoiceRequest::amount_msats(), then they could return None instead, I suppose? But if we add currency conversion, we'd likely want to do it at the builder-level, I think.
Related discussion from the last PR: #1927 (comment)

I still think invoice_request.amount_msats() should return an Option to avoid an unreachable, just think it should return self.contents.amount_msats.or(offer.contents.amount_msats()) (i.e. None if the offer has a Currency amount), because the offer's amount is its amount in that case IIUC. Anyway, can be punted

There was some confusion over if we should support sending to a non-bitcoin-denominated offer, which was my fault, we can/should revisit this when we add that support.

valentinewallace · 2023-01-12T01:43:54Z

+	///
+	/// [`Invoice`]: crate::offers::invoice::Invoice
+	pub fn respond_with(
+ &self, paths: Vec<BlindedPath>, payinfo: Vec<BlindedPayInfo>, created_at: Duration,


Should we have std-only versions that automatically supplies created_at?

End up making the parameter no-std only and otherwise automated as suggested.

codecov-commenter · 2023-01-13T15:24:04Z

Codecov Report

Base: 90.73% // Head: 91.39% // Increases project coverage by +0.66% 🎉

Coverage data is based on head (15f1295) compared to base (ad40573).
Patch coverage: 93.80% of modified lines in pull request are covered.

Additional details and impacted files

@@ Coverage Diff @@## main #1926 +/- ##
==========================================
+ Coverage 90.73% 91.39% +0.66% 
==========================================
Files 97 98 +1 Lines 50604 57080 +6476 Branches 50604 57080 +6476 ==========================================
+ Hits 45914 52167 +6253 - Misses 4690 4913 +223

Impacted Files	Coverage Δ
lightning/src/offers/parse.rs	`94.00% <ø> (ø)`
lightning/src/routing/gossip.rs	`92.15% <ø> (ø)`
lightning/src/util/ser_macros.rs	`86.73% <50.00%> (-0.30%)`	⬇️
lightning/src/offers/invoice_request.rs	`96.12% <62.50%> (+1.12%)`	⬆️
lightning/src/offers/refund.rs	`95.89% <68.75%> (+2.15%)`	⬆️
lightning/src/util/ser.rs	`91.41% <82.35%> (-0.30%)`	⬇️
lightning/src/offers/offer.rs	`93.33% <91.66%> (+0.06%)`	⬆️
lightning/src/offers/invoice.rs	`94.79% <94.79%> (ø)`
lightning/src/offers/merkle.rs	`100.00% <100.00%> (ø)`
lightning/src/util/enforcing_trait_impls.rs	`81.15% <0.00%> (-1.77%)`	⬇️
... and 21 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt · 2023-01-15T04:41:26Z

This all basically LGTM, feel free to squash and fixup the CI failures and I'm ready to land.

jkczyz · 2023-01-16T06:03:34Z

Added tests, but still need to add an example.

Also, see 8c76a69 for small clean-up such that Iterable is not special-cased. Open to a better name for that as well.

jkczyz · 2023-01-18T16:07:03Z

Added examples in the module docs and missing cross-linking. PTAL.

jkczyz · 2023-01-18T16:19:18Z

 $field = Some($trait::read(&mut $reader $(, $read_arg)*)?);
 	}};
+	($reader: expr, $field: ident, (option, encoding: ($fieldty: ty, $encoding: ident, $encoder:ty))) => {{
+ decode_tlv!($reader, $field, (option, encoding: ($fieldty, $encoding)));


Looks like I'll need to rebase to account for #1823. Let me know when it's good to squash and rebase.

Go for it, IMO.

Explicitly using TlvRecord::record_bytes makes reading the code more obvious than hiding which bytes are used in AsRef<[u8]>::as_ref.

Provide a helper for skipping signature TLV records from a TLV stream. This prevents needing to duplicate the check for signature TLV records when writing a TLV stream without signatures in an upcoming commit.

When using bytes from an InvoiceRequest to constructing bytes for an Invoice, any signature TLV records in the bytes must be excluded. Define a wrapper for encoding such pre-serialized bytes in this manner. This will allow the forthcoming InvoiceBuilder to construct bytes for an Invoice properly.

jkczyz · 2023-01-18T21:47:12Z

Rebased and squashed though added a missed parsing check on the signing pubkey after reviewing the spec. It's already handled correctly in the builder.

diff --git a/lightning/src/offers/invoice.rs b/lightning/src/offers/invoice.rs
index 9b5fe3fd..2e6fcf05 100644
--- a/lightning/src/offers/invoice.rs+++ b/lightning/src/offers/invoice.rs@@ -705,7 +705,11 @@ impl TryFrom<PartialInvoiceTlvStream> for InvoiceContents {
};
match offer_tlv_stream.node_id {
- Some(_) => {+ Some(expected_signing_pubkey) => {+ if fields.signing_pubkey != expected_signing_pubkey {+ return Err(SemanticError::InvalidSigningPubkey);+ }+
let invoice_request = InvoiceRequestContents::try_from(
(payer_tlv_stream, offer_tlv_stream, invoice_request_tlv_stream)
)?;
@@ -1455,6 +1459,17 @@ mod tests {
assert_eq!(e, ParseError::InvalidSemantics(SemanticError::MissingSigningPubkey));
},
}
++ let invalid_pubkey = payer_pubkey();+ let mut tlv_stream = invoice.as_tlv_stream();+ tlv_stream.3.node_id = Some(&invalid_pubkey);++ match Invoice::try_from(tlv_stream.to_bytes()) {+ Ok(_) => panic!("expected error"),+ Err(e) => {+ assert_eq!(e, ParseError::InvalidSemantics(SemanticError::InvalidSigningPubkey));+ },+ }
}
#[test]
diff --git a/lightning/src/offers/parse.rs b/lightning/src/offers/parse.rs
index 904ce403..a7d13e57 100644
--- a/lightning/src/offers/parse.rs+++ b/lightning/src/offers/parse.rs@@ -147,6 +147,8 @@ pub enum SemanticError {
MissingDescription,
/// A signing pubkey was not provided.
MissingSigningPubkey,
+	/// A signing pubkey was provided but a different one was expected.+	InvalidSigningPubkey,
/// A signing pubkey was provided but was not expected.
UnexpectedSigningPubkey,
/// A quantity was expected but was missing.

TheBlueMatt · 2023-01-18T23:10:37Z

+ network,
+ };
+
+ if !address.is_standard() {


Hmm, this is probably too restrictive - we want to send to it even if its a v3 witness program or whatever, its not our job to enforce forwards compat. rust-bitcoin/rust-bitcoin#1558

Yeah, that does make sense. I may have been reading too much into this part of the spec:
MUST ignore any fallback_address for which address does not meet known requirements for the given version
Since there wouldn't be any known requirements for, say, version 3, we shouldn't ignore them, I guess?

I thought there were requirements around v0 scripts being only exactly the "correct" length but otherwise none. I could be wrong, but I'd love for rust-bitcoin upstream to fix this.

Yeah, length of V0 and V1 scripts are covered by is_standard but other versions are considered non-standard. Best to have it fixed upstream, I agree, but we'll have to explicitly allow for other versions now, it seems?

Yea, I think we should just backport the v0 length check, note that there is no length restriction on v1 - see https://github.com/bitcoin/bitcoin/blob/e9262ea32a6e1d364fb7974844fadc36f931f8c6/src/script/standard.cpp#L183-L201

Looks like V1 must be WITNESS_V1_TAPROOT_SIZE (32), which is what Address::is_standard checks.

Oh, nevermind. I see the fall-through case is fine with it.

Ok, now we only skip V0 addresses that aren't the right size.

TheBlueMatt · 2023-01-18T23:53:08Z

+ &self, payment_paths: Vec<(BlindedPath, BlindedPayInfo)>, payment_hash: PaymentHash,
+ signing_pubkey: PublicKey,
+ #[cfg(any(test, not(feature = "std")))]
+ created_at: Duration


Any idea of docs.rs will document this in a sane way?

Hmm... it should be using std according to our Cargo.toml file:
rust-lightning/lightning/Cargo.toml
Lines 14 to 16 in ad40573
[package.metadata.docs.rs]
features = ["std"]
rustdoc-args = ["--cfg", "docsrs"]
So I think that means it should leave the parameter out.

Hmm, but isn't that confusing? The docs will only be correct for std users and no-std users will be left wondering why their code doesn't compile?

Yeah, would be best to note in the docs then. Not sure if there's a better way. Even if we made separate methods, the docs will only list the std one.

IIRC docsrs is smart and magic and will list all features with this is only available with feature X tags, but maybe we'd have to enable both no-std and std for that?

Yeah, we'd have to enable both. Not sure how much time I want to spend getting that to work at the moment. Updated the docs for now.

TheBlueMatt · 2023-01-19T00:06:40Z

Oops, no-std builds are failing on the new serialization iterable things.

jkczyz · 2023-01-19T00:08:54Z

New push fixes a problem with no-std:

diff --git a/lightning/src/offers/invoice.rs b/lightning/src/offers/invoice.rs
index 2e6fcf05..fbf193a5 100644
--- a/lightning/src/offers/invoice.rs+++ b/lightning/src/offers/invoice.rs@@ -550,12 +550,12 @@ tlv_stream!(InvoiceTlvStream, InvoiceTlvStreamRef, 160..240, {
(176, node_id: PublicKey),
});
-type BlindedPathIter<'a> = std::iter::Map<+type BlindedPathIter<'a> = core::iter::Map<
core::slice::Iter<'a, (BlindedPath, BlindedPayInfo)>,
for<'r> fn(&'r (BlindedPath, BlindedPayInfo)) -> &'r BlindedPath,
>;
-type BlindedPayInfoIter<'a> = std::iter::Map<+type BlindedPayInfoIter<'a> = core::iter::Map<
core::slice::Iter<'a, (BlindedPath, BlindedPayInfo)>,
for<'r> fn(&'r (BlindedPath, BlindedPayInfo)) -> &'r BlindedPayInfo,
>;

TheBlueMatt · 2023-01-19T01:29:52Z

Looks like no-std is still sad.

valentinewallace

Basically LGTM

valentinewallace · 2023-01-19T01:15:24Z

+ self
+	}
+
+	/// Adds a P2WSH address to [`Invoice::fallbacks`].


Is there any reason users would want one type of fallback address over another? May be nice to document

Ah, sorry, I don't know if the current additions add much (am I missing something?). I was thinking more of the tradeoffs/use cases between p2wsh vs p2wpkh vs taproot, or a link to a reference for something like that. May be out of our purview.

Yeah, seems more like a wallet concern.

valentinewallace · 2023-01-19T20:50:53Z

+ return None;
+ }
+
+ let version = address.version;
