From d6900723df7a9cdd75092e89b60674881700c8b2 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Mon, 22 Apr 2024 16:43:26 -0500 Subject: [PATCH 1/4] Remove unnecessary PaymentPreimage clone --- lightning/src/ln/channelmanager.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 130eab49384..06627ff6c3b 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -5548,7 +5548,7 @@ where } } let purpose = events::PaymentPurpose::from_parts( - payment_preimage.clone(), + payment_preimage, payment_data.payment_secret, payment_context.clone(), ); From 8e562be5d43dce5a07fc09686d4f7f151f5a2706 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Fri, 26 Apr 2024 11:37:24 -0500 Subject: [PATCH 2/4] Remove PaymentContext from OnionPayload PaymentContext is already stored in ClaimablePayment via PaymentPurpose, so there is no need to repeat it in each ClaimableHTLC via OnionPayload. This avoids cloning the PaymentContext each time an HTLC is received, other than for the first HTLC for a payment. --- lightning/src/ln/channelmanager.rs | 44 ++++++++++++------------------ 1 file changed, 17 insertions(+), 27 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 06627ff6c3b..7244d258f65 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -357,11 +357,6 @@ enum OnionPayload { /// This is only here for backwards-compatibility in serialization, in the future it can be /// removed, breaking clients running 0.0.106 and earlier. _legacy_hop_data: Option, - /// The context of the payment included by the recipient in a blinded path, or `None` if a - /// blinded path was not used. - /// - /// Used in part to determine the [`events::PaymentPurpose`]. - payment_context: Option, }, /// Contains the payer-provided preimage. Spontaneous(PaymentPreimage), @@ -5346,7 +5341,7 @@ where } }) => { let blinded_failure = routing.blinded_failure(); - let (cltv_expiry, onion_payload, payment_data, phantom_shared_secret, mut onion_fields) = match routing { + let (cltv_expiry, onion_payload, payment_data, payment_context, phantom_shared_secret, mut onion_fields) = match routing { PendingHTLCRouting::Receive { payment_data, payment_metadata, payment_context, incoming_cltv_expiry, phantom_shared_secret, custom_tlvs, @@ -5355,8 +5350,8 @@ where let _legacy_hop_data = Some(payment_data.clone()); let onion_fields = RecipientOnionFields { payment_secret: Some(payment_data.payment_secret), payment_metadata, custom_tlvs }; - (incoming_cltv_expiry, OnionPayload::Invoice { _legacy_hop_data, payment_context }, - Some(payment_data), phantom_shared_secret, onion_fields) + (incoming_cltv_expiry, OnionPayload::Invoice { _legacy_hop_data }, + Some(payment_data), payment_context, phantom_shared_secret, onion_fields) }, PendingHTLCRouting::ReceiveKeysend { payment_data, payment_preimage, payment_metadata, @@ -5368,7 +5363,7 @@ where custom_tlvs, }; (incoming_cltv_expiry, OnionPayload::Spontaneous(payment_preimage), - payment_data, None, onion_fields) + payment_data, None, None, onion_fields) }, _ => { panic!("short_channel_id == 0 should imply any pending_forward entries are of type Receive"); @@ -5530,7 +5525,7 @@ where match payment_secrets.entry(payment_hash) { hash_map::Entry::Vacant(_) => { match claimable_htlc.onion_payload { - OnionPayload::Invoice { ref payment_context, .. } => { + OnionPayload::Invoice { .. } => { let payment_data = payment_data.unwrap(); let (payment_preimage, min_final_cltv_expiry_delta) = match inbound_payment::verify(payment_hash, &payment_data, self.highest_seen_timestamp.load(Ordering::Acquire) as u64, &self.inbound_payment_key, &self.logger) { Ok(result) => result, @@ -5550,7 +5545,7 @@ where let purpose = events::PaymentPurpose::from_parts( payment_preimage, payment_data.payment_secret, - payment_context.clone(), + payment_context, ); check_total_value!(purpose); }, @@ -5561,13 +5556,10 @@ where } }, hash_map::Entry::Occupied(inbound_payment) => { - let payment_context = match claimable_htlc.onion_payload { - OnionPayload::Spontaneous(_) => { - log_trace!(self.logger, "Failing new keysend HTLC with payment_hash {} because we already have an inbound payment with the same payment hash", &payment_hash); - fail_htlc!(claimable_htlc, payment_hash); - }, - OnionPayload::Invoice { ref payment_context, .. } => payment_context, - }; + if let OnionPayload::Spontaneous(_) = claimable_htlc.onion_payload { + log_trace!(self.logger, "Failing new keysend HTLC with payment_hash {} because we already have an inbound payment with the same payment hash", &payment_hash); + fail_htlc!(claimable_htlc, payment_hash); + } let payment_data = payment_data.unwrap(); if inbound_payment.get().payment_secret != payment_data.payment_secret { log_trace!(self.logger, "Failing new HTLC with payment_hash {} as it didn't match our expected payment secret.", &payment_hash); @@ -5580,7 +5572,7 @@ where let purpose = events::PaymentPurpose::from_parts( inbound_payment.get().payment_preimage, payment_data.payment_secret, - payment_context.clone(), + payment_context, ); let payment_claimable_generated = check_total_value!(purpose); if payment_claimable_generated { @@ -10815,11 +10807,11 @@ impl_writeable_tlv_based!(HTLCPreviousHopData, { impl Writeable for ClaimableHTLC { fn write(&self, writer: &mut W) -> Result<(), io::Error> { - let (payment_data, keysend_preimage, payment_context) = match &self.onion_payload { - OnionPayload::Invoice { _legacy_hop_data, payment_context } => { - (_legacy_hop_data.as_ref(), None, payment_context.as_ref()) + let (payment_data, keysend_preimage) = match &self.onion_payload { + OnionPayload::Invoice { _legacy_hop_data } => { + (_legacy_hop_data.as_ref(), None) }, - OnionPayload::Spontaneous(preimage) => (None, Some(preimage), None), + OnionPayload::Spontaneous(preimage) => (None, Some(preimage)), }; write_tlv_fields!(writer, { (0, self.prev_hop, required), @@ -10831,7 +10823,6 @@ impl Writeable for ClaimableHTLC { (6, self.cltv_expiry, required), (8, keysend_preimage, option), (10, self.counterparty_skimmed_fee_msat, option), - (11, payment_context, option), }); Ok(()) } @@ -10849,7 +10840,6 @@ impl Readable for ClaimableHTLC { (6, cltv_expiry, required), (8, keysend_preimage, option), (10, counterparty_skimmed_fee_msat, option), - (11, payment_context, option), }); let payment_data: Option = payment_data_opt; let value = value_ser.0.unwrap(); @@ -10870,7 +10860,7 @@ impl Readable for ClaimableHTLC { } total_msat = Some(payment_data.as_ref().unwrap().total_msat); } - OnionPayload::Invoice { _legacy_hop_data: payment_data, payment_context } + OnionPayload::Invoice { _legacy_hop_data: payment_data } }, }; Ok(Self { @@ -12106,7 +12096,7 @@ where return Err(DecodeError::InvalidValue); } let purpose = match &htlcs[0].onion_payload { - OnionPayload::Invoice { _legacy_hop_data, payment_context: _ } => { + OnionPayload::Invoice { _legacy_hop_data } => { if let Some(hop_data) = _legacy_hop_data { events::PaymentPurpose::Bolt11InvoicePayment { payment_preimage: match pending_inbound_payments.get(&payment_hash) { From 7b864425ea884f816b3801cd1b4fdb99d95a4e36 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Fri, 26 Apr 2024 12:03:07 -0500 Subject: [PATCH 3/4] Remove InvoiceRequestFields::amount_msats Event::PaymentClaimable and Event::PaymentClaimed already contain the paid amount, so there's no need to included the requested amount in InvoiceRequestFields. --- lightning/src/ln/offers_tests.rs | 3 --- lightning/src/offers/invoice_request.rs | 27 ++++++++----------------- 2 files changed, 8 insertions(+), 22 deletions(-) diff --git a/lightning/src/ln/offers_tests.rs b/lightning/src/ln/offers_tests.rs index 75a2e290f39..5a628537384 100644 --- a/lightning/src/ln/offers_tests.rs +++ b/lightning/src/ln/offers_tests.rs @@ -412,7 +412,6 @@ fn creates_and_pays_for_offer_using_two_hop_blinded_path() { offer_id: offer.id(), invoice_request: InvoiceRequestFields { payer_id: invoice_request.payer_id(), - amount_msats: None, features: InvoiceRequestFeatures::empty(), quantity: None, payer_note_truncated: None, @@ -565,7 +564,6 @@ fn creates_and_pays_for_offer_using_one_hop_blinded_path() { offer_id: offer.id(), invoice_request: InvoiceRequestFields { payer_id: invoice_request.payer_id(), - amount_msats: None, features: InvoiceRequestFeatures::empty(), quantity: None, payer_note_truncated: None, @@ -687,7 +685,6 @@ fn pays_for_offer_without_blinded_paths() { offer_id: offer.id(), invoice_request: InvoiceRequestFields { payer_id: invoice_request.payer_id(), - amount_msats: None, features: InvoiceRequestFeatures::empty(), quantity: None, payer_note_truncated: None, diff --git a/lightning/src/offers/invoice_request.rs b/lightning/src/offers/invoice_request.rs index 9157613fcd9..e1256b71ac1 100644 --- a/lightning/src/offers/invoice_request.rs +++ b/lightning/src/offers/invoice_request.rs @@ -877,13 +877,12 @@ impl VerifiedInvoiceRequest { let InvoiceRequestContents { payer_id, inner: InvoiceRequestContentsWithoutPayerId { - payer: _, offer: _, chain: _, amount_msats, features, quantity, payer_note + payer: _, offer: _, chain: _, amount_msats: _, features, quantity, payer_note }, } = &self.inner.contents; InvoiceRequestFields { payer_id: *payer_id, - amount_msats: *amount_msats, features: features.clone(), quantity: *quantity, payer_note_truncated: payer_note.clone() @@ -1126,12 +1125,6 @@ pub struct InvoiceRequestFields { /// A possibly transient pubkey used to sign the invoice request. pub payer_id: PublicKey, - /// The amount to pay in msats (i.e., the minimum lightning-payable unit for [`chain`]), which - /// must be greater than or equal to [`Offer::amount`], converted if necessary. - /// - /// [`chain`]: InvoiceRequest::chain - pub amount_msats: Option, - /// Features pertaining to requesting an invoice. pub features: InvoiceRequestFeatures, @@ -1150,10 +1143,9 @@ impl Writeable for InvoiceRequestFields { fn write(&self, writer: &mut W) -> Result<(), io::Error> { write_tlv_fields!(writer, { (0, self.payer_id, required), - (2, self.amount_msats.map(|v| HighZeroBytesDroppedBigSize(v)), option), - (4, WithoutLength(&self.features), required), - (6, self.quantity.map(|v| HighZeroBytesDroppedBigSize(v)), option), - (8, self.payer_note_truncated.as_ref().map(|s| WithoutLength(&s.0)), option), + (2, WithoutLength(&self.features), required), + (4, self.quantity.map(|v| HighZeroBytesDroppedBigSize(v)), option), + (6, self.payer_note_truncated.as_ref().map(|s| WithoutLength(&s.0)), option), }); Ok(()) } @@ -1163,15 +1155,14 @@ impl Readable for InvoiceRequestFields { fn read(reader: &mut R) -> Result { _init_and_read_len_prefixed_tlv_fields!(reader, { (0, payer_id, required), - (2, amount_msats, (option, encoding: (u64, HighZeroBytesDroppedBigSize))), - (4, features, (option, encoding: (InvoiceRequestFeatures, WithoutLength))), - (6, quantity, (option, encoding: (u64, HighZeroBytesDroppedBigSize))), - (8, payer_note_truncated, (option, encoding: (String, WithoutLength))), + (2, features, (option, encoding: (InvoiceRequestFeatures, WithoutLength))), + (4, quantity, (option, encoding: (u64, HighZeroBytesDroppedBigSize))), + (6, payer_note_truncated, (option, encoding: (String, WithoutLength))), }); let features = features.unwrap_or(InvoiceRequestFeatures::empty()); Ok(InvoiceRequestFields { - payer_id: payer_id.0.unwrap(), amount_msats, features, quantity, + payer_id: payer_id.0.unwrap(), features, quantity, payer_note_truncated: payer_note_truncated.map(|s| UntrustedString(s)), }) } @@ -2264,7 +2255,6 @@ mod tests { let invoice_request = offer.request_invoice(vec![1; 32], payer_pubkey()).unwrap() .chain(Network::Testnet).unwrap() - .amount_msats(1001).unwrap() .quantity(1).unwrap() .payer_note("0".repeat(PAYER_NOTE_LIMIT * 2)) .build().unwrap() @@ -2277,7 +2267,6 @@ mod tests { fields, InvoiceRequestFields { payer_id: payer_pubkey(), - amount_msats: Some(1001), features: InvoiceRequestFeatures::empty(), quantity: Some(1), payer_note_truncated: Some(UntrustedString("0".repeat(PAYER_NOTE_LIMIT))), From 33b6162fd23a3010bb12b608ce0b93e88510060c Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Fri, 26 Apr 2024 12:13:12 -0500 Subject: [PATCH 4/4] Remove InvoiceRequestFields::features InvoiceRequestFeatures may contain a large, odd bit. Including this in InvoiceRequestFields, which is in each BlindedPath of a Bolt12Invoice, could cause the invoice's onion message to exceed the maximum size. The features are already checked before sending an invoice. --- lightning/src/ln/offers_tests.rs | 4 ---- lightning/src/offers/invoice_request.rs | 21 +++++++-------------- 2 files changed, 7 insertions(+), 18 deletions(-) diff --git a/lightning/src/ln/offers_tests.rs b/lightning/src/ln/offers_tests.rs index 5a628537384..c480cf5b8a8 100644 --- a/lightning/src/ln/offers_tests.rs +++ b/lightning/src/ln/offers_tests.rs @@ -46,7 +46,6 @@ use crate::blinded_path::{BlindedPath, IntroductionNode}; use crate::blinded_path::payment::{Bolt12OfferContext, Bolt12RefundContext, PaymentContext}; use crate::events::{Event, MessageSendEventsProvider, PaymentPurpose}; use crate::ln::channelmanager::{PaymentId, RecentPaymentDetails, Retry, self}; -use crate::ln::features::InvoiceRequestFeatures; use crate::ln::functional_test_utils::*; use crate::ln::msgs::{ChannelMessageHandler, Init, NodeAnnouncement, OnionMessage, OnionMessageHandler, RoutingMessageHandler, SocketAddress, UnsignedGossipMessage, UnsignedNodeAnnouncement}; use crate::offers::invoice::Bolt12Invoice; @@ -412,7 +411,6 @@ fn creates_and_pays_for_offer_using_two_hop_blinded_path() { offer_id: offer.id(), invoice_request: InvoiceRequestFields { payer_id: invoice_request.payer_id(), - features: InvoiceRequestFeatures::empty(), quantity: None, payer_note_truncated: None, }, @@ -564,7 +562,6 @@ fn creates_and_pays_for_offer_using_one_hop_blinded_path() { offer_id: offer.id(), invoice_request: InvoiceRequestFields { payer_id: invoice_request.payer_id(), - features: InvoiceRequestFeatures::empty(), quantity: None, payer_note_truncated: None, }, @@ -685,7 +682,6 @@ fn pays_for_offer_without_blinded_paths() { offer_id: offer.id(), invoice_request: InvoiceRequestFields { payer_id: invoice_request.payer_id(), - features: InvoiceRequestFeatures::empty(), quantity: None, payer_note_truncated: None, }, diff --git a/lightning/src/offers/invoice_request.rs b/lightning/src/offers/invoice_request.rs index e1256b71ac1..357b85c5cf0 100644 --- a/lightning/src/offers/invoice_request.rs +++ b/lightning/src/offers/invoice_request.rs @@ -877,13 +877,12 @@ impl VerifiedInvoiceRequest { let InvoiceRequestContents { payer_id, inner: InvoiceRequestContentsWithoutPayerId { - payer: _, offer: _, chain: _, amount_msats: _, features, quantity, payer_note + payer: _, offer: _, chain: _, amount_msats: _, features: _, quantity, payer_note }, } = &self.inner.contents; InvoiceRequestFields { payer_id: *payer_id, - features: features.clone(), quantity: *quantity, payer_note_truncated: payer_note.clone() .map(|mut s| { s.truncate(PAYER_NOTE_LIMIT); UntrustedString(s) }), @@ -1125,9 +1124,6 @@ pub struct InvoiceRequestFields { /// A possibly transient pubkey used to sign the invoice request. pub payer_id: PublicKey, - /// Features pertaining to requesting an invoice. - pub features: InvoiceRequestFeatures, - /// The quantity of the offer's item conforming to [`Offer::is_valid_quantity`]. pub quantity: Option, @@ -1143,9 +1139,8 @@ impl Writeable for InvoiceRequestFields { fn write(&self, writer: &mut W) -> Result<(), io::Error> { write_tlv_fields!(writer, { (0, self.payer_id, required), - (2, WithoutLength(&self.features), required), - (4, self.quantity.map(|v| HighZeroBytesDroppedBigSize(v)), option), - (6, self.payer_note_truncated.as_ref().map(|s| WithoutLength(&s.0)), option), + (2, self.quantity.map(|v| HighZeroBytesDroppedBigSize(v)), option), + (4, self.payer_note_truncated.as_ref().map(|s| WithoutLength(&s.0)), option), }); Ok(()) } @@ -1155,14 +1150,13 @@ impl Readable for InvoiceRequestFields { fn read(reader: &mut R) -> Result { _init_and_read_len_prefixed_tlv_fields!(reader, { (0, payer_id, required), - (2, features, (option, encoding: (InvoiceRequestFeatures, WithoutLength))), - (4, quantity, (option, encoding: (u64, HighZeroBytesDroppedBigSize))), - (6, payer_note_truncated, (option, encoding: (String, WithoutLength))), + (2, quantity, (option, encoding: (u64, HighZeroBytesDroppedBigSize))), + (4, payer_note_truncated, (option, encoding: (String, WithoutLength))), }); - let features = features.unwrap_or(InvoiceRequestFeatures::empty()); Ok(InvoiceRequestFields { - payer_id: payer_id.0.unwrap(), features, quantity, + payer_id: payer_id.0.unwrap(), + quantity, payer_note_truncated: payer_note_truncated.map(|s| UntrustedString(s)), }) } @@ -2267,7 +2261,6 @@ mod tests { fields, InvoiceRequestFields { payer_id: payer_pubkey(), - features: InvoiceRequestFeatures::empty(), quantity: Some(1), payer_note_truncated: Some(UntrustedString("0".repeat(PAYER_NOTE_LIMIT))), }