From b0c7616735b3ed2132639c6ee439c2d08fa17e72 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 22 Dec 2021 03:05:39 +0000 Subject: [PATCH] Avoid storing a full FinalOnionHopData in OnionPayload::Invoice We only use it to check the amount when processing MPP parts, but store the full object (including new payment metadata) in it. Because we now store the amount in the parent structure, there is no need for it at all in the `OnionPayload`. Sadly, for serialization compatibility, we need it to continue to exist, at least temporarily, but we can avoid populating the new fields in that case. --- lightning/src/ln/channelmanager.rs | 56 +++++++++++++++++------------- 1 file changed, 31 insertions(+), 25 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 23426e219..fbb5c76ff 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -449,7 +449,11 @@ enum OnionPayload { /// Contains a total_msat (which may differ from value if this is a Multi-Path Payment) and a /// payment_secret which prevents path-probing attacks and can associate different HTLCs which /// are part of the same payment. - Invoice(msgs::FinalOnionHopData), + Invoice { + /// This is only here for backwards-compatibility in serialization, in the future it can be + /// removed, breaking clients running 0.0.104 and earlier. + _legacy_hop_data: msgs::FinalOnionHopData, + }, /// Contains the payer-provided preimage. Spontaneous(PaymentPreimage), } @@ -3173,11 +3177,16 @@ impl ChannelMana HTLCForwardInfo::AddHTLC { prev_short_channel_id, prev_htlc_id, forward_info: PendingHTLCInfo { routing, incoming_shared_secret, payment_hash, amt_to_forward, .. }, prev_funding_outpoint } => { - let (cltv_expiry, total_msat, onion_payload) = match routing { - PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry } => - (incoming_cltv_expiry, payment_data.total_msat, OnionPayload::Invoice(payment_data)), + let (cltv_expiry, onion_payload, payment_data) = match routing { + PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry } => { + let _legacy_hop_data = msgs::FinalOnionHopData { + payment_secret: payment_data.payment_secret, + total_msat: payment_data.total_msat + }; + (incoming_cltv_expiry, OnionPayload::Invoice { _legacy_hop_data }, Some(payment_data)) + }, PendingHTLCRouting::ReceiveKeysend { payment_preimage, incoming_cltv_expiry } => - (incoming_cltv_expiry, amt_to_forward, OnionPayload::Spontaneous(payment_preimage)), + (incoming_cltv_expiry, OnionPayload::Spontaneous(payment_preimage), None), _ => { panic!("short_channel_id == 0 should imply any pending_forward entries are of type Receive"); } @@ -3190,7 +3199,7 @@ impl ChannelMana incoming_packet_shared_secret: incoming_shared_secret, }, value: amt_to_forward, - total_msat, + total_msat: if let Some(data) = &payment_data { data.total_msat } else { amt_to_forward }, cltv_expiry, onion_payload, }; @@ -3213,7 +3222,7 @@ impl ChannelMana } macro_rules! check_total_value { - ($payment_data_total_msat: expr, $payment_secret: expr, $payment_preimage: expr) => {{ + ($payment_data: expr, $payment_preimage: expr) => {{ let mut payment_received_generated = false; let htlcs = channel_state.claimable_htlcs.entry(payment_hash) .or_insert(Vec::new()); @@ -3228,7 +3237,7 @@ impl ChannelMana for htlc in htlcs.iter() { total_value += htlc.value; match &htlc.onion_payload { - OnionPayload::Invoice(_) => { + OnionPayload::Invoice { .. } => { if htlc.total_msat != claimable_htlc.total_msat { log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the HTLCs had inconsistent total values (eg {} and {})", log_bytes!(payment_hash.0), claimable_htlc.total_msat, htlc.total_msat); @@ -3250,7 +3259,7 @@ impl ChannelMana payment_hash, purpose: events::PaymentPurpose::InvoicePayment { payment_preimage: $payment_preimage, - payment_secret: $payment_secret, + payment_secret: $payment_data.payment_secret, }, amt: total_value, }); @@ -3275,7 +3284,8 @@ impl ChannelMana match payment_secrets.entry(payment_hash) { hash_map::Entry::Vacant(_) => { match claimable_htlc.onion_payload { - OnionPayload::Invoice(ref payment_data) => { + OnionPayload::Invoice { .. } => { + let payment_data = payment_data.unwrap(); let payment_preimage = match inbound_payment::verify(payment_hash, &payment_data, self.highest_seen_timestamp.load(Ordering::Acquire) as u64, &self.inbound_payment_key, &self.logger) { Ok(payment_preimage) => payment_preimage, Err(()) => { @@ -3283,9 +3293,7 @@ impl ChannelMana continue } }; - let payment_data_total_msat = payment_data.total_msat; - let payment_secret = payment_data.payment_secret.clone(); - check_total_value!(payment_data_total_msat, payment_secret, payment_preimage); + check_total_value!(payment_data, payment_preimage); }, OnionPayload::Spontaneous(preimage) => { match channel_state.claimable_htlcs.entry(payment_hash) { @@ -3306,14 +3314,12 @@ impl ChannelMana } }, hash_map::Entry::Occupied(inbound_payment) => { - let payment_data = - if let OnionPayload::Invoice(ref data) = claimable_htlc.onion_payload { - data.clone() - } else { - log_trace!(self.logger, "Failing new keysend HTLC with payment_hash {} because we already have an inbound payment with the same payment hash", log_bytes!(payment_hash.0)); - fail_htlc!(claimable_htlc); - continue - }; + if payment_data.is_none() { + log_trace!(self.logger, "Failing new keysend HTLC with payment_hash {} because we already have an inbound payment with the same payment hash", log_bytes!(payment_hash.0)); + fail_htlc!(claimable_htlc); + continue + }; + 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.", log_bytes!(payment_hash.0)); fail_htlc!(claimable_htlc); @@ -3322,7 +3328,7 @@ impl ChannelMana log_bytes!(payment_hash.0), payment_data.total_msat, inbound_payment.get().min_value_msat.unwrap()); fail_htlc!(claimable_htlc); } else { - let payment_received_generated = check_total_value!(payment_data.total_msat, payment_data.payment_secret, inbound_payment.get().payment_preimage); + let payment_received_generated = check_total_value!(payment_data, inbound_payment.get().payment_preimage); if payment_received_generated { inbound_payment.remove_entry(); } @@ -5922,11 +5928,11 @@ impl_writeable_tlv_based!(HTLCPreviousHopData, { impl Writeable for ClaimableHTLC { fn write(&self, writer: &mut W) -> Result<(), io::Error> { let payment_data = match &self.onion_payload { - OnionPayload::Invoice(data) => Some(data.clone()), + OnionPayload::Invoice { _legacy_hop_data } => Some(_legacy_hop_data), _ => None, }; let keysend_preimage = match self.onion_payload { - OnionPayload::Invoice(_) => None, + OnionPayload::Invoice { .. } => None, OnionPayload::Spontaneous(preimage) => Some(preimage.clone()), }; write_tlv_fields!(writer, { @@ -5974,7 +5980,7 @@ impl Readable for ClaimableHTLC { if total_msat.is_none() { total_msat = Some(payment_data.as_ref().unwrap().total_msat); } - OnionPayload::Invoice(payment_data.unwrap()) + OnionPayload::Invoice { _legacy_hop_data: payment_data.unwrap() } }, }; Ok(Self { -- 2.39.5