Include a `PaymentContext` in `PaymentPurpose` #2970

jkczyz · 2024-03-26T18:18:40Z

This is an alternative approach to #2929. Instead of adding an InvoiceGenerated event, expand PaymentPurpose with more variants that include additional context, which is exposed in PaymentClaimable and PaymentClaimed events. PaymentContext is included to payment::ReceiveTlvs such that the sender includes it in the onion when making a payment.

jkczyz · 2024-03-26T18:21:01Z

@TheBlueMatt @tnull This doesn't include adding the data the payment path yet. Just wanted to draft this to make sure the approach of piping the data from payment::ReceiveTlvs to Event is desirable. In particular, if including PaymentContext in PaymentPurpose is reasonable or if it should be a separate field.

jkczyz · 2024-03-26T18:21:53Z

FY, the top three commits are relevant.

codecov-commenter · 2024-03-26T18:28:22Z

Codecov Report

Attention: Patch coverage is 78.96907% with 102 lines in your changes are missing coverage. Please review.

Project coverage is 89.26%. Comparing base (59778da) to head (c4524a0).
Report is 4 commits behind head on main.

❗ Current head c4524a0 differs from pull request most recent head 478911d. Consider uploading reports for the commit 478911d to get more accurate results

Files	Patch %	Lines
lightning/src/events/mod.rs	62.50%	24 Missing ⚠️
lightning/src/blinded_path/payment.rs	52.77%	17 Missing ⚠️
lightning/src/ln/channelmanager.rs	82.14%	15 Missing ⚠️
lightning/src/ln/functional_test_utils.rs	55.00%	9 Missing ⚠️
lightning/src/offers/invoice_error.rs	0.00%	9 Missing ⚠️
lightning/src/offers/invoice_request.rs	90.62%	8 Missing and 1 partial ⚠️
lightning/src/ln/chanmon_update_fail_tests.rs	50.00%	5 Missing ⚠️
lightning/src/ln/functional_tests.rs	50.00%	5 Missing ⚠️
lightning/src/ln/offers_tests.rs	95.52%	3 Missing ⚠️
lightning/src/offers/invoice.rs	76.92%	3 Missing ⚠️
... and 1 more

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2970      +/-   ##
==========================================
- Coverage   89.29%   89.26%   -0.04%     
==========================================
  Files         117      117              
  Lines       96251    98421    +2170     
  Branches    96251    98421    +2170     
==========================================
+ Hits        85950    87858    +1908     
- Misses       8076     8339     +263     
+ Partials     2225     2224       -1

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

TheBlueMatt · 2024-03-26T18:36:57Z

lightning/src/events/mod.rs

@@ -67,6 +68,8 @@ pub enum PaymentPurpose {
 		/// [`ChannelManager::create_inbound_payment`]: crate::ln::channelmanager::ChannelManager::create_inbound_payment
 		/// [`ChannelManager::create_inbound_payment_for_hash`]: crate::ln::channelmanager::ChannelManager::create_inbound_payment_for_hash
 		payment_secret: PaymentSecret,
+		///
+		payment_context: Option<PaymentContext>,


Should we have a new enum variant for BOLT12 that is distinct from InvoicePayment? If BOLT12 were the same flow I'd think not but now we have the ability to have an invoice paid more than once, which feels like it would be a separate flow on the event-consuming end?

Initially, my intention was to have a top-level field in PaymentClaimable. But while piping that data through I noticed that the approach here seemed to make sense internally at least. We have ClaimingPayment and ClaimablePayment, both of which have a required PaymentPurpose field. So while I could make another variant to OnionPayload for PaymentContext, we'd still need a PaymentPurpose to create those structs (ClaimingPayment and ClaimablePayment). So this seemed like the least disruptive approach, but definitely looking for a Concept ACK on this or an alternative approach.

Oh, I didn't mean a different variant in PaymentContext for BOLT11, I meant a new variant in PaymentPurpose for BOLT12.

Ah, let me give that a try.

Oh, PaymentPurpose doesn't use impl_writeable_tlv_based_enum_upgradable. Would that be a problem?

That way we don't have two levels of payment types (i.e. PaymentPurpose and PaymentContext). And we don't break downgrades that way. Thoughts?

I'm probably waaay to optimistic, but having an entirely separate variant for BOLT12 might also allow us to drop the BOLT11 variant more easily some day?

I can add a Bolt12InvoicePayment variant though it's gonna have the same fields as InvoicePayment plus an optional (still) PaymentContext because we want to support downgrades there.

Not sure I understand this, can't we have it be required and read an upgradable_required and then support downgrades by discarding the event?

I'm fine with that, but maybe it makes more sense to rename InvoicePayment to ExpectedPayment (opposite to the SpontaneousPayment variant) and add an Option as is currently done here?

Seems fine with me.

That way we don't have two levels of payment types (i.e. PaymentPurpose and PaymentContext). And we don't break downgrades that way. Thoughts?

I wonder if we shouldn't make PaymentContext non-Option and have an empty BOLT11 variant, then?

Not sure I understand this, can't we have it be required and read an upgradable_required and then support downgrades by discarding the event?

Yeah, I think you're right.

I'm fine with that, but maybe it makes more sense to rename InvoicePayment to ExpectedPayment (opposite to the SpontaneousPayment variant) and add an Option as is currently done here?

Seems fine with me.

Ok, I'll try this out.

I wonder if we shouldn't make PaymentContext non-Option and have an empty BOLT11 variant, then?

Same here.

FYI, still need to finish this.

The latest push makes PaymentContext non-Option in all structs, which means downgrades will fail for unknown contexts. It also uses a Bolt11Invoice context for any existing serializations without one. @TheBlueMatt let me know if this is what you had in mind.

lightning/src/blinded_path/payment.rs

TheBlueMatt · 2024-03-26T18:39:52Z

lightning/src/blinded_path/payment.rs

+/// [`BlindedPath`]: crate::blinded_path::BlindedPath
+/// [`Event::PaymentClaimable`]: crate::events::Event::PaymentClaimable
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub enum PaymentContext {


Does this need to be an enum? If its only going to be a BOLT12 thing it seems like overkill, though if we ever end up with a BOLT13 we'd need it? Or is it also intended to have a refund variant and that's why?

Yeah, possibly for refunds and future / alternative payment protocols / extensions. Hard to say for sure, but could need a Bolt12Subscription, for instance. Maybe static invoices (for offline receive) unless we are able to obtain a sender-included InvoiceRequest for use here.

Hard to say for sure, but could need a Bolt12Subscription, for instance

Wouldn't that just be repeated payments for the same OfferId?

Maybe static invoices (for offline receive) unless we are able to obtain a sender-included InvoiceRequest for use here.

I believe the current thinking is the InvoiceRequest will be copied into the HTLC onion.

Hard to say for sure, but could need a Bolt12Subscription, for instance

Wouldn't that just be repeated payments for the same OfferId?

It's certainly possible. But who knows what the spec will ultimately look like. In general, it seems better to allow expanding the number of variants than adding increasingly more Option fields for future uses. The latter may require the user to figure out how to interpret it.

Yea, hard to balance future uses at the serialization layer vs complexifying the API today for it. I wonder if we can't make the API simpler (ie expose just a struct) but at the serialization layer do something smarter?

lightning/src/offers/offer.rs

jkczyz

Pushed a few commits that add the PaymentContext to the BlindedPaths.

benthecarman · 2024-03-27T03:14:02Z

lightning/src/blinded_path/payment.rs

+	Bolt12Offer {
+		/// The identifier of the [`Offer`].
+		///
+		/// [`Offer`]: crate::offers::offer::Offer
+		offer_id: OfferId,
+	},
+	/// The payment was made for invoice sent for a BOLT 12 [`Refund`].
+	///
+	/// [`Refund`]: crate::offers::refund::Refund
+	Bolt12Refund {},


Where does the offer id come from? Also is there no id for a refund?

Our main concern for Mutiny is we can't label the receives from bolt 12 if we can't tie it to our original bolt12

At the moment, the OfferId is a nonce taken from the offer metadata. You can use OfferBuilder::offer_id to retrieve it. Maybe we could have a VerifiedOffer so you don't need to grab it from the builder before creating the offer.

Note that OfferId is the id for inbound payments. We're going to provide most if not all of the data from the InvoiceRequest in the PaymentContext::Bolt12Offer variant, too. And you'll have the PaymentHash from the Bolt12Invoice directly in Event::PaymentClaimable. So you should be able to use that to correlated all inbound payments to an offer.

For outbound payments, we already use the PaymentId passed to pay_for_offer which is given back in Event::PaymentSent.

For a Refund creator (i.e., an outbound payment), the id is the PaymentId passed to create_refund_builder, which is encrypted in the payer_metadata and also given back in Event::PaymentSent. For the inbound payment, the id is the PaymentHash in the Bolt12Invoice sent to the Refund creator. That invoice is now returned by request_refund_payment in this PR.

Note that OfferId is the id for inbound payments. We're going to provide most if not all of the data from the InvoiceRequest in the PaymentContext::Bolt12Offer variant, too. And you'll have the PaymentHash from the Bolt12Invoice directly in Event::PaymentClaimable. So you should be able to use that to correlated all inbound payments to an offer.

I'm still not entirely convinced just using payment_hash will be an adequate identifier for inbound payments going forward, given that we're about to potentially have (more) spurious payment events after #2957. It would be really nice if we could also expose PaymentId for inbound payments that could function as a "real" idempotency token with documented guarantees for the user, even if we'd derive it from the PaymentHash internally. This would make the API more uniform and probably less confusing for the user.

Also, with BOLT12 multiple invoices may be generated for each offer and each of these could potentially get paid multiple times. Even though the latter is a bug on the payer's end it would be nice if we could expose a way to discern duplicate payments from spurious/duplicate events.

I'm still not entirely convinced just using payment_hash will be an adequate identifier for inbound payments going forward, given that we're about to potentially have (more) spurious payment events after #2957. It would be really nice if we could also expose PaymentId for inbound payments that could function as a "real" idempotency token with documented guarantees for the user, even if we'd derive it from the PaymentHash internally. This would make the API more uniform and probably less confusing for the user.

Do you have a concrete proposal in mind?

Also, with BOLT12 multiple invoices may be generated for each offer and each of these could potentially get paid multiple times. Even though the latter is a bug on the payer's end it would be nice if we could expose a way to discern duplicate payments from spurious/duplicate events.

Just to be clear, don't you already need to handle duplicate payments for the same invoice in BOLT 11? And, at least according to our docs, you shouldn't claim duplicate payments: https://docs.rs/lightning/latest/lightning/events/enum.Event.html#note-1

Do you have a concrete proposal in mind?

I think the ~only thing we can do is channel_id + htlc_id (picking one by some tiebreaker for MPP payments). We should probably do this, but its largely unrelated to this PR, I think.

jkczyz · 2024-03-27T20:57:37Z

lightning/src/offers/offer.rs

@@ -236,10 +254,15 @@ macro_rules! offer_derived_metadata_builder_methods { ($secp_context: ty) => {
 				features: OfferFeatures::empty(), absolute_expiry: None, issuer: None, paths: None,
 				supported_quantity: Quantity::One, signing_pubkey: node_id,
 			},
-			metadata_strategy: core::marker::PhantomData,
+			metadata_strategy: DerivedMetadata(OfferId(nonce)),


There is a drawback to using the Nonce for the OfferId. For the bindings version of the builders, you'd be able to create two different offers but that have the same id. For instance, since build doesn't consume the builder, you could build an offer and reuse the builder to create a different offer. Invoice requests would still verify correctly, but the offers would have the same id because the nonce is generated at builder-construction time.

An alternative could be to use the tagged hash of the merkle root computed at build time for the OfferId. This would allow us to have an OfferId for any offer, not just the offers we create from ChannelManager.

We could do the same for Refund if having an id is desirable for inbound payments. Though, the PaymentHash should suffice.

An alternative could be to use the tagged hash of the merkle root computed at build time for the OfferId.

Does it have to be at build-time? Can't we just calculate a hash of all the fields in the offer when we see it again in the invoice_request? I would feel a lot happier doing it this way, I think.

Well, it wouldn't necessarily need to be done by OfferBuilder. But a user like LDK Node is probably going to store the Offer so would want to use the OfferId as the key after creating it. So it could also just be a method on Offer that computes it on the fly.

But, yes, we'd then re-create the OfferId when receiving the InvoiceRequest.

Right, re-calculating it every time the user calls id() is probably not ideal...

Updated OfferId to use the tagged hash of the merkle root.

jkczyz · 2024-03-27T21:07:59Z

lightning/src/events/mod.rs

@@ -67,6 +68,8 @@ pub enum PaymentPurpose {
 		/// [`ChannelManager::create_inbound_payment`]: crate::ln::channelmanager::ChannelManager::create_inbound_payment
 		/// [`ChannelManager::create_inbound_payment_for_hash`]: crate::ln::channelmanager::ChannelManager::create_inbound_payment_for_hash
 		payment_secret: PaymentSecret,
+		///
+		payment_context: Option<PaymentContext>,


Not sure I understand this, can't we have it be required and read an upgradable_required and then support downgrades by discarding the event?

Yeah, I think you're right.

I'm fine with that, but maybe it makes more sense to rename InvoicePayment to ExpectedPayment (opposite to the SpontaneousPayment variant) and add an Option as is currently done here?

Seems fine with me.

Ok, I'll try this out.

I wonder if we shouldn't make PaymentContext non-Option and have an empty BOLT11 variant, then?

Same here.

TheBlueMatt · 2024-03-29T15:30:10Z

Looks like this needs a small rebase.

jkczyz · 2024-03-29T17:34:42Z

Looks like this needs a small rebase.

Rebased

jkczyz · 2024-03-29T22:19:00Z

This is good for review. Need to add/update unit tests for InvoiceRequestFields construction, but shouldn't be much work.

TheBlueMatt

Mostly LGTM, mentioned offline but I think we need to move the context into the event because we may want a context in SpontaneousPayment events with async BOLT12 payments.

lightning/src/events/mod.rs

lightning/src/ln/channelmanager.rs

lightning/src/offers/invoice_request.rs

lightning/src/offers/merkle.rs

lightning/src/offers/offer.rs

tnull

Did a pass, looks good so far, only one minor question regarding logging.

While reviewing I noticed that we have several instances of SecretKey::from_slice(hmac.as_byte_array()).unwrap() in signer.rs. The libsecp256k1 docs say that there is a (although negligible) chance that this would panic. Seems in other places we make sure to handle the error. Do we also want to do this in these instances, or are we fine with taking the chances?

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2024-04-11T13:21:55Z

While reviewing I noticed that we have several instances of SecretKey::from_slice(hmac.as_byte_array()).unwrap() in signer.rs. The libsecp256k1 docs say that there is a (although negligible) chance that this would panic.

The probability of this is somewhere between 1/2^127 and 1/2^128. If we ever hit that its more likely someone found an issue in SHA-256 and is exploiting it, so panic is probably fine (but we should consider expect in general).

jkczyz · 2024-04-15T02:07:01Z

Mostly LGTM, mentioned offline but I think we need to move the context into the event because we may want a context in SpontaneousPayment events with async BOLT12 payments.

Discussed offline. Ended up expanding PaymentPurpose with new variants. PaymentContext is used when creating a BlindedPath, which is piped through ChannelManager until an event is created with a PaymentPurpose. The PaymentContext variants wrap a specific context which is re-used in the PaymentPurpose variants to avoid unnecessary duplication.

jkczyz · 2024-04-15T02:40:38Z

@coderabbitai coming in hot out of nowhere

coderabbitai · 2024-04-15T02:40:52Z

Tip

For best results, initiate chat on the files or code changes.

@jkczyz, thanks for the tag! If there's anything specific you need help with, such as reviewing code, generating unit tests, or analyzing coverage issues, please let me know how I can assist!

tnull

Really like that we have the relevant fields available in Payment{Claimable,Claimed} API now!

Btw, this seems to need a rebase.

lightning/src/blinded_path/payment.rs

lightning/src/events/mod.rs

tnull · 2024-04-15T10:28:09Z

lightning/src/events/mod.rs

+	///
+	/// [`Offer`]: crate::offers::offer::Offer
+	Bolt12OfferPayment {
+		/// The preimage to the payment hash. If provided, this can be handed directly to


nit: Just realized that generally the PaymentPurpose docs only really make sense in the context of PaymentClaimable, even though we now include it in PaymentClaimed for some time. E.g., handing the preimage directly to claim_funds (again) would lead to trying to double-claim in the context of PaymentClaimed.

This is really pre-existing so no need to fix it here, but eventually might be nice to have the docs make sense in all used contexts?

lightning/src/ln/channelmanager.rs

lightning/src/blinded_path/payment.rs

TheBlueMatt · 2024-04-15T14:32:06Z

Needs rebase as well, it seems.

jkczyz · 2024-04-15T21:10:24Z

Rebasing in the meanwhile.

TheBlueMatt

Can probably drop the TODO from the "Define an OfferId when built with DerivedMetadata" commit message when you squash :)

TheBlueMatt · 2024-04-16T15:03:44Z

lightning/src/blinded_path/payment.rs

@@ -137,7 +201,8 @@ impl Writeable for ReceiveTlvs {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
 		encode_tlv_stream!(w, {
 			(12, self.payment_constraints, required),
-			(65536, self.payment_secret, required)
+			(65536, self.payment_secret, required),
+			(65538, self.payment_context, required)


Shouldn't this be an odd value so that users can get invoices issued by new LDK, downgrade, and still accept the payment? Not sure it matters much cause we have important offers bugfixes elsewhere but still.

Sure thing. Updated.

lightning/src/events/mod.rs

lightning/src/ln/channelmanager.rs

lightning/src/offers/offer.rs

TheBlueMatt · 2024-04-16T15:48:53Z

lightning/src/offers/invoice_request.rs

+	pub payer_id: PublicKey,
+
+	/// A chain from [`Offer::chains`] that the offer is valid for.
+	pub chain: Option<ChainHash>,


Do we need this? Every byte here is very expensive so IMO we should avoid if possible.

We can probably drop it. In practice, it will be None since it may not be set when the offer is for mainnet, but we can't count on the counterparty to do so. And this should always be ChannelManager's chain, so should be ok to drop.

Ah, right, forgot its usually None, but, yea, we already check it, no need to store it again.

lightning/src/offers/invoice_request.rs

lightning/src/blinded_path/payment.rs

When sending an invoice for a refund, information from the invoice may be useful for caller. For instance, the payment_hash can be used to track whether the refund was paid. Return the invoice to facilitate this use case.

Providing LDK-specific data in a BlindedPath allows for the data to be received with the corresponding payment. This is useful as it can then be surfaced in PaymentClaimable events where the user may correlated with an Offer, for instance. Define a PaymentContext enum for including various context (e.g., offer, refund, static invoice).

PendingInboundPayment::BlindedReceive includes a PaymentContext, which the recipient includes in each blinded path. Included this when processing HTLCs in ChannelManager, first into PendingHTLCRouting and then to OnionPayload. Later, this will be included in the PaymentPurpose when surfaced to user in PaymentClaimable and PaymentClaimed events.

Use a merkle root hash of the offer TLV records to define an offer id. Will be included in a BOLT 12 invoice's blinded payment paths in order for the recipient to identify which offer the payment is for.

Extract the OfferId from the offer metadata sent back in the InvoiceRequest and include it in VerifiedInvoiceRequest. This can be used to correspond the eventual payment for the invoice response by including it in the invoice's blinded payment paths.

When constructing blinded payment paths for a Bolt12Invoice response, include the OfferId so that the eventual payment can be correlated with the Offer.

When requesting a payment for a refund, include a context in the Bolt12Invoice's blinded payment paths indicated it is for such. When the eventual payment is received, the user can use the payment hash to correlate it with the corresponding Refund.

PaymentPurpose will be expanded to include BOLT 12 payment context. Rename the InvoicePayment variant accordinly before new variants are added.

In order to provide more context in PaymentClaimable and PaymentClaimed events, introduce new variants of PaymentPurpose for use with BOLT 12 payments. Separate variants are used for offers and refunds. An unknown variant is used for backwards compatibility and ease of testing, but is otherwise not publicly constructable.

When constructing a PaymentPurpose in ChannelManager, use the PaymentContext from OnionPayload to determine which variant to construct, including those for BOLT 12 payments.

When receiving a payment, it's useful to know information about the InvoiceRequest. Include this data in PaymentContext::Bolt12Offer so users can display information about an inbound payment (e.g., the payer note).

Clarify that payment_preimage should only be used to claim a payment when handling Event::PaymentClaimable, not Event::PaymentClaimed.

TheBlueMatt · 2024-04-18T14:55:09Z

lightning/src/ln/channelmanager.rs

+												let purpose = events::PaymentPurpose::from_parts(
+													payment_preimage.clone(),
+													payment_data.payment_secret,
+													payment_context.clone(),


I'd like to clean up all the clones around here in a followup. They used to be cheap but now they have some malloc cost.

Hmm... I'm not sure if we can. Maybe we could replace OnionPayload in ClaimableHTLC with a PaymentPurpose?

I mean maybe we shouldn't have added payment_context to OnionPayload? It looks trivial to remove.

Ah, so then we would check PaymentPurpose in PaymentClaimable against the one received in PendingHTLCRouting::Receive but not store it in each ClaimableHTLC (via OnionPayload). Yeah, that should work.

TheBlueMatt · 2024-04-18T15:13:39Z

lightning/src/offers/invoice_request.rs

+	/// must be greater than or equal to [`Offer::amount`], converted if necessary.
+	///
+	/// [`chain`]: InvoiceRequest::chain
+	pub amount_msats: Option<u64>,


Actually, what's the point of including this? Do users ever actually care about the amount set in the invoice_request by the time they've received the actual payment (and know exactly how much it was for)?

Probably not? But I guess the sender could have overpaid, so it might be needed for accounting purposes.

But this doesn't fully capture the overpayment - PaymentClaimable::amount_msat exists for that purpose, and the sender could overpay in their invoice_request and then overpay more in the actual HTLCs, so it'd be wrong to look at this.

TheBlueMatt · 2024-04-18T15:21:11Z

Gonna go ahead and land this but I'd like to discuss my above two comments before we release.

Follow-ups to #2970

…followups Follow-ups to lightningdevkit#2970

v0.0.123 - May 08, 2024 - "BOLT12 Dust Sweeping" API Updates =========== * To reduce risk of force-closures and improve HTLC reliability the default dust exposure limit has been increased to `MaxDustHTLCExposure::FeeRateMultiplier(10_000)`. Users with existing channels might want to consider using `ChannelManager::update_channel_config` to apply the new default (lightningdevkit#3045). * `ChainMonitor::archive_fully_resolved_channel_monitors` is now provided to remove from memory `ChannelMonitor`s that have been fully resolved on-chain and are now not needed. It uses the new `Persist::archive_persisted_channel` to inform the storage layer that such a monitor should be archived (lightningdevkit#2964). * An `OutputSweeper` is now provided which will automatically sweep `SpendableOutputDescriptor`s, retrying until the sweep confirms (lightningdevkit#2825). * After initiating an outbound channel, a peer disconnection no longer results in immediate channel closure. Rather, if the peer is reconnected before the channel times out LDK will automatically retry opening it (lightningdevkit#2725). * `PaymentPurpose` now has separate variants for BOLT12 payments, which include fields from the `invoice_request` as well as the `OfferId` (lightningdevkit#2970). * `ChannelDetails` now includes a list of in-flight HTLCs (lightningdevkit#2442). * `Event::PaymentForwarded` now includes `skimmed_fee_msat` (lightningdevkit#2858). * The `hashbrown` dependency has been upgraded and the use of `ahash` as the no-std hash table hash function has been removed. As a consequence, LDK's `Hash{Map,Set}`s no longer feature several constructors when LDK is built with no-std; see the `util::hash_tables` module instead. On platforms that `getrandom` supports, setting the `possiblyrandom/getrandom` feature flag will ensure hash tables are resistant to HashDoS attacks, though the `possiblyrandom` crate should detect most common platforms (lightningdevkit#2810, lightningdevkit#2891). * `ChannelMonitor`-originated requests to the `ChannelSigner` can now fail and be retried using `ChannelMonitor::signer_unblocked` (lightningdevkit#2816). * `SpendableOutputDescriptor::to_psbt_input` now includes the `witness_script` where available as well as new proprietary data which can be used to re-derive some spending keys from the base key (lightningdevkit#2761, lightningdevkit#3004). * `OutPoint::to_channel_id` has been removed in favor of `ChannelId::v1_from_funding_outpoint` in preparation for v2 channels with a different `ChannelId` derivation scheme (lightningdevkit#2797). * `PeerManager::get_peer_node_ids` has been replaced with `list_peers` and `peer_by_node_id`, which provide more details (lightningdevkit#2905). * `Bolt11Invoice::get_payee_pub_key` is now provided (lightningdevkit#2909). * `Default[Message]Router` now take an `entropy_source` argument (lightningdevkit#2847). * `ClosureReason::HTLCsTimedOut` has been separated out from `ClosureReason::HolderForceClosed` as it is the most common case (lightningdevkit#2887). * `ClosureReason::CooperativeClosure` is now split into `{Counterparty,Locally}Initiated` variants (lightningdevkit#2863). * `Event::ChannelPending::channel_type` is now provided (lightningdevkit#2872). * `PaymentForwarded::{prev,next}_user_channel_id` are now provided (lightningdevkit#2924). * Channel init messages have been refactored towards V2 channels (lightningdevkit#2871). * `BumpTransactionEvent` now contains the channel and counterparty (lightningdevkit#2873). * `util::scid_utils` is now public, with some trivial utilities to examine short channel ids (lightningdevkit#2694). * `DirectedChannelInfo::{source,target}` are now public (lightningdevkit#2870). * Bounds in `lightning-background-processor` were simplified by using `AChannelManager` (lightningdevkit#2963). * The `Persist` impl for `KVStore` no longer requires `Sized`, allowing for the use of `dyn KVStore` as `Persist` (lightningdevkit#2883, lightningdevkit#2976). * `From<PaymentPreimage>` is now implemented for `PaymentHash` (lightningdevkit#2918). * `NodeId::from_slice` is now provided (lightningdevkit#2942). * `ChannelManager` deserialization may now fail with `DangerousValue` when LDK's persistence API was violated (lightningdevkit#2974). Bug Fixes ========= * Excess fees on counterparty commitment transactions are now included in the dust exposure calculation. This lines behavior up with some cases where transaction fees can be burnt, making them effectively dust exposure (lightningdevkit#3045). * `Future`s used as an `std::...::Future` could grow in size unbounded if it was never woken. For those not using async persistence and using the async `lightning-background-processor`, this could cause a memory leak in the `ChainMonitor` (lightningdevkit#2894). * Inbound channel requests that fail in `ChannelManager::accept_inbound_channel` would previously have stalled from the peer's perspective as no `error` message was sent (lightningdevkit#2953). * Blinded path construction has been tuned to select paths more likely to succeed, improving BOLT12 payment reliability (lightningdevkit#2911, lightningdevkit#2912). * After a reorg, `lightning-transaction-sync` could have failed to follow a transaction that LDK needed information about (lightningdevkit#2946). * `RecipientOnionFields`' `custom_tlvs` are now propagated to recipients when paying with blinded paths (lightningdevkit#2975). * `Event::ChannelClosed` is now properly generated and peers are properly notified for all channels that as a part of a batch channel open fail to be funded (lightningdevkit#3029). * In cases where user event processing is substantially delayed such that we complete multiple round-trips with our peers before a `PaymentSent` event is handled and then restart without persisting the `ChannelManager` after having persisted a `ChannelMonitor[Update]`, on startup we may have `Err`d trying to deserialize the `ChannelManager` (lightningdevkit#3021). * If a peer has relatively high latency, `PeerManager` may have failed to establish a connection (lightningdevkit#2993). * `ChannelUpdate` messages broadcasted for our own channel closures are now slightly more robust (lightningdevkit#2731). * Deserializing malformed BOLT11 invoices may have resulted in an integer overflow panic in debug builds (lightningdevkit#3032). * In exceedingly rare cases (no cases of this are known), LDK may have created an invalid serialization for a `ChannelManager` (lightningdevkit#2998). * Message processing latency handling BOLT12 payments has been reduced (lightningdevkit#2881). * Latency in processing `Event::SpendableOutputs` may be reduced (lightningdevkit#3033). Node Compatibility ================== * LDK's blinded paths were inconsistent with other implementations in several ways, which have been addressed (lightningdevkit#2856, lightningdevkit#2936, lightningdevkit#2945). * LDK's messaging blinded paths now support the latest features which some nodes may begin relying on soon (lightningdevkit#2961). * LDK's BOLT12 structs have been updated to support some last-minute changes to the spec (lightningdevkit#3017, lightningdevkit#3018). * CLN v24.02 requires the `gossip_queries` feature for all peers, however LDK by default does not set it for those not using a `P2PGossipSync` (e.g. those using RGS). This change was reverted in CLN v24.02.2 however for now LDK always sets the `gossip_queries` feature. This change is expected to be reverted in a future LDK release (lightningdevkit#2959). Security ======== 0.0.123 fixes a denial-of-service vulnerability which we believe to be reachable from untrusted input when parsing invalid BOLT11 invoices containing non-ASCII characters. * BOLT11 invoices with non-ASCII characters in the human-readable-part may cause an out-of-bounds read attempt leading to a panic (lightningdevkit#3054). Note that all BOLT11 invoices containing non-ASCII characters are invalid. In total, this release features 150 files changed, 19307 insertions, 6306 deletions in 360 commits since 0.0.121 from 17 authors, in alphabetical order: * Arik Sosman * Duncan Dean * Elias Rohrer * Evan Feenstra * Jeffrey Czyz * Keyue Bao * Matt Corallo * Orbital * Sergi Delgado Segura * Valentine Wallace * Willem Van Lint * Wilmer Paulino * benthecarman * jbesraa * olegkubrakov * optout * shaavan

TheBlueMatt reviewed Mar 26, 2024

View reviewed changes

jkczyz commented Mar 27, 2024

View reviewed changes

jkczyz force-pushed the 2024-03-offer-id branch from d36af73 to e94b2e0 Compare March 27, 2024 03:06

benthecarman reviewed Mar 27, 2024

View reviewed changes

jkczyz force-pushed the 2024-03-offer-id branch from e94b2e0 to c9161b9 Compare March 27, 2024 16:51

jkczyz commented Mar 27, 2024

View reviewed changes

jkczyz force-pushed the 2024-03-offer-id branch from c9161b9 to 44b1d86 Compare March 29, 2024 05:23

jkczyz force-pushed the 2024-03-offer-id branch from 44b1d86 to bb54746 Compare March 29, 2024 17:34

jkczyz force-pushed the 2024-03-offer-id branch from bb54746 to ead4645 Compare March 29, 2024 21:32

jkczyz marked this pull request as ready for review March 29, 2024 22:17

TheBlueMatt added this to the 0.0.123 milestone Apr 10, 2024

TheBlueMatt reviewed Apr 10, 2024

View reviewed changes

tnull reviewed Apr 11, 2024

View reviewed changes

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

coderabbitai bot previously approved these changes Apr 15, 2024

View reviewed changes

jkczyz dismissed coderabbitai[bot]’s stale review via 9158e30 April 15, 2024 02:00

jkczyz force-pushed the 2024-03-offer-id branch from ead4645 to 9158e30 Compare April 15, 2024 02:00

tnull reviewed Apr 15, 2024

View reviewed changes

jkczyz force-pushed the 2024-03-offer-id branch from 9158e30 to 327901b Compare April 15, 2024 21:09

jkczyz force-pushed the 2024-03-offer-id branch 2 times, most recently from 2691b1d to dc9fa10 Compare April 16, 2024 00:31

TheBlueMatt reviewed Apr 16, 2024

View reviewed changes

jkczyz force-pushed the 2024-03-offer-id branch 2 times, most recently from 20a8a38 to c4524a0 Compare April 17, 2024 22:07

jkczyz added 14 commits April 18, 2024 09:12

Return the invoice when requesting a refund

12c3a24

When sending an invoice for a refund, information from the invoice may be useful for caller. For instance, the payment_hash can be used to track whether the refund was paid. Return the invoice to facilitate this use case.

Define an OfferId for BOLT 12 Offers

b9970ff

Use a merkle root hash of the offer TLV records to define an offer id. Will be included in a BOLT 12 invoice's blinded payment paths in order for the recipient to identify which offer the payment is for.

Include Offer context in blinded payment paths

48d9245

When constructing blinded payment paths for a Bolt12Invoice response, include the OfferId so that the eventual payment can be correlated with the Offer.

Rename InvoicePayment to Bolt11InvoicePayment

e74f243

PaymentPurpose will be expanded to include BOLT 12 payment context. Rename the InvoicePayment variant accordinly before new variants are added.

Add PaymentPurpose::is_keysend utility method

ef3ec8f

Use BOLT 12 PaymentPurpose variants

0c52ea2

When constructing a PaymentPurpose in ChannelManager, use the PaymentContext from OnionPayload to determine which variant to construct, including those for BOLT 12 payments.

Test for PaymentContext in offers_tests.rs

b06b2c7

Include InvoiceRequest fields in PaymentContext

8367e02

When receiving a payment, it's useful to know information about the InvoiceRequest. Include this data in PaymentContext::Bolt12Offer so users can display information about an inbound payment (e.g., the payer note).

Update PaymentPurpose's payment_preimage docs

478911d

Clarify that payment_preimage should only be used to claim a payment when handling Event::PaymentClaimable, not Event::PaymentClaimed.

jkczyz force-pushed the 2024-03-offer-id branch from c4524a0 to 478911d Compare April 18, 2024 14:26

tnull approved these changes Apr 18, 2024

View reviewed changes

TheBlueMatt approved these changes Apr 18, 2024

View reviewed changes

TheBlueMatt merged commit 56a87cc into lightningdevkit:main Apr 18, 2024
16 checks passed

TheBlueMatt mentioned this pull request Apr 18, 2024

Discuss/Do #2970 followups #3002

Closed

jkczyz mentioned this pull request Apr 26, 2024

Provide Bolt12Invoice used for inbound payment #2929

Closed

TheBlueMatt added a commit that referenced this pull request Apr 29, 2024

Merge pull request #3028 from jkczyz/2024-04-offer-id-followups

d00e550

Follow-ups to #2970

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request May 6, 2024

Merge pull request lightningdevkit#3028 from jkczyz/2024-04-offer-id-…

be21ab0

…followups Follow-ups to lightningdevkit#2970

tnull mentioned this pull request May 22, 2024

Track inbound payments by also unique payment IDs lightningdevkit/ldk-node#298

Open

3 tasks

Include a PaymentContext in PaymentPurpose #2970

Include a PaymentContext in PaymentPurpose #2970

Conversation

jkczyz commented Mar 26, 2024 • edited Loading

jkczyz commented Mar 26, 2024

jkczyz commented Mar 26, 2024

codecov-commenter commented Mar 26, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Mar 29, 2024

jkczyz commented Mar 29, 2024

jkczyz commented Mar 29, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

tnull left a comment • edited Loading

Choose a reason for hiding this comment

TheBlueMatt commented Apr 11, 2024 • edited Loading

jkczyz commented Apr 15, 2024

jkczyz commented Apr 15, 2024

coderabbitai bot commented Apr 15, 2024

tnull left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 15, 2024

jkczyz commented Apr 15, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 18, 2024

Include a `PaymentContext` in `PaymentPurpose` #2970

Include a `PaymentContext` in `PaymentPurpose` #2970

jkczyz commented Mar 26, 2024 •

edited

Loading

codecov-commenter commented Mar 26, 2024 •

edited

Loading

tnull Mar 27, 2024 •

edited

Loading

tnull Mar 27, 2024 •

edited

Loading

jkczyz Mar 27, 2024 •

edited

Loading

tnull left a comment •

edited

Loading

TheBlueMatt commented Apr 11, 2024 •

edited

Loading

tnull left a comment •

edited

Loading