Avoid storing a full FinalOnionHopData in OnionPayload::Invoice
authorMatt Corallo <git@bluematt.me>
Thu, 28 Apr 2022 17:10:04 +0000 (10:10 -0700)
committerSean Andersen <seana@squareup.com>
Mon, 2 May 2022 18:41:02 +0000 (11:41 -0700)
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

index 4e6ed6b62614eff264f99e73296e6316c5daf0c7..a28d6347332694af4dab7fc0737dd13a2a38c834 100644 (file)
@@ -159,10 +159,12 @@ pub(crate) struct HTLCPreviousHopData {
 }
 
 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),
+       /// Indicates this incoming onion payload is for the purpose of paying an invoice.
+       Invoice {
+               /// 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: msgs::FinalOnionHopData,
+       },
        /// Contains the payer-provided preimage.
        Spontaneous(PaymentPreimage),
 }
@@ -3100,11 +3102,13 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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, phantom_shared_secret) = match routing {
-                                                                       PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry, phantom_shared_secret } =>
-                                                                               (incoming_cltv_expiry, payment_data.total_msat, OnionPayload::Invoice(payment_data), phantom_shared_secret),
+                                                               let (cltv_expiry, onion_payload, payment_data, phantom_shared_secret) = match routing {
+                                                                       PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry, phantom_shared_secret } => {
+                                                                               let _legacy_hop_data = payment_data.clone();
+                                                                               (incoming_cltv_expiry, OnionPayload::Invoice { _legacy_hop_data }, Some(payment_data), phantom_shared_secret)
+                                                                       },
                                                                        PendingHTLCRouting::ReceiveKeysend { payment_preimage, incoming_cltv_expiry } =>
-                                                                               (incoming_cltv_expiry, amt_to_forward, OnionPayload::Spontaneous(payment_preimage), None),
+                                                                               (incoming_cltv_expiry, OnionPayload::Spontaneous(payment_preimage), None, None),
                                                                        _ => {
                                                                                panic!("short_channel_id == 0 should imply any pending_forward entries are of type Receive");
                                                                        }
@@ -3119,7 +3123,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                                        },
                                                                        value: amt_to_forward,
                                                                        timer_ticks: 0,
-                                                                       total_msat,
+                                                                       total_msat: if let Some(data) = &payment_data { data.total_msat } else { amt_to_forward },
                                                                        cltv_expiry,
                                                                        onion_payload,
                                                                };
@@ -3143,7 +3147,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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());
@@ -3159,9 +3163,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                                                        total_value += htlc.value;
                                                                                        match &htlc.onion_payload {
                                                                                                OnionPayload::Invoice { .. } => {
-                                                                                                       if htlc.total_msat != $payment_data_total_msat {
+                                                                                                       if htlc.total_msat != $payment_data.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), $payment_data_total_msat, htlc.total_msat);
+                                                                                                                       log_bytes!(payment_hash.0), $payment_data.total_msat, htlc.total_msat);
                                                                                                                total_value = msgs::MAX_VALUE_MSAT;
                                                                                                        }
                                                                                                        if total_value >= msgs::MAX_VALUE_MSAT { break; }
@@ -3169,17 +3173,17 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                                                                _ => unreachable!(),
                                                                                        }
                                                                                }
-                                                                               if total_value >= msgs::MAX_VALUE_MSAT || total_value > $payment_data_total_msat {
+                                                                               if total_value >= msgs::MAX_VALUE_MSAT || total_value > $payment_data.total_msat {
                                                                                        log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the total value {} ran over expected value {} (or HTLCs were inconsistent)",
-                                                                                               log_bytes!(payment_hash.0), total_value, $payment_data_total_msat);
+                                                                                               log_bytes!(payment_hash.0), total_value, $payment_data.total_msat);
                                                                                        fail_htlc!(claimable_htlc);
-                                                                               } else if total_value == $payment_data_total_msat {
+                                                                               } else if total_value == $payment_data.total_msat {
                                                                                        htlcs.push(claimable_htlc);
                                                                                        new_events.push(events::Event::PaymentReceived {
                                                                                                payment_hash,
                                                                                                purpose: events::PaymentPurpose::InvoicePayment {
                                                                                                        payment_preimage: $payment_preimage,
-                                                                                                       payment_secret: $payment_secret,
+                                                                                                       payment_secret: $payment_data.payment_secret,
                                                                                                },
                                                                                                amt: total_value,
                                                                                        });
@@ -3204,7 +3208,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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(()) => {
@@ -3212,8 +3217,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                                                                                continue
                                                                                                        }
                                                                                                };
-                                                                                               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) {
@@ -3234,14 +3238,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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);
@@ -3250,7 +3252,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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();
                                                                                        }
@@ -3469,10 +3471,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                debug_assert!(false);
                                                return false;
                                        }
-                                       if let OnionPayload::Invoice(ref final_hop_data) = htlcs[0].onion_payload {
+                                       if let OnionPayload::Invoice { .. } = htlcs[0].onion_payload {
                                                // Check if we've received all the parts we need for an MPP (the value of the parts adds to total_msat).
                                                // In this case we're not going to handle any timeouts of the parts here.
-                                               if final_hop_data.total_msat == htlcs.iter().fold(0, |total, htlc| total + htlc.value) {
+                                               if htlcs[0].total_msat == htlcs.iter().fold(0, |total, htlc| total + htlc.value) {
                                                        return true;
                                                } else if htlcs.into_iter().any(|htlc| {
                                                        htlc.timer_ticks += 1;
@@ -6073,11 +6075,11 @@ impl_writeable_tlv_based!(HTLCPreviousHopData, {
 impl Writeable for ClaimableHTLC {
        fn write<W: Writer>(&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, {
@@ -6125,7 +6127,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 {