Skip to content

Commit fc77c57

Browse files
TheBlueMattandozw
TheBlueMatt
authored and
andozw
committed
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.
1 parent 62f8df5 commit fc77c57

File tree

1 file changed

+35
-33
lines changed

1 file changed

+35
-33
lines changed

‎lightning/src/ln/channelmanager.rs

+35-33
Original file line numberDiff line numberDiff line change
@@ -159,10 +159,12 @@ pub(crate) struct HTLCPreviousHopData {
159159
}
160160

161161
enum OnionPayload {
162-
/// Contains a total_msat (which may differ from value if this is a Multi-Path Payment) and a
163-
/// payment_secret which prevents path-probing attacks and can associate different HTLCs which
164-
/// are part of the same payment.
165-
Invoice(msgs::FinalOnionHopData),
162+
/// Indicates this incoming onion payload is for the purpose of paying an invoice.
163+
Invoice {
164+
/// This is only here for backwards-compatibility in serialization, in the future it can be
165+
/// removed, breaking clients running 0.0.106 and earlier.
166+
_legacy_hop_data: msgs::FinalOnionHopData,
167+
},
166168
/// Contains the payer-provided preimage.
167169
Spontaneous(PaymentPreimage),
168170
}
@@ -3100,11 +3102,13 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31003102
HTLCForwardInfo::AddHTLC { prev_short_channel_id, prev_htlc_id, forward_info: PendingHTLCInfo {
31013103
routing, incoming_shared_secret, payment_hash, amt_to_forward, .. },
31023104
prev_funding_outpoint } => {
3103-
let (cltv_expiry, total_msat, onion_payload, phantom_shared_secret) = match routing {
3104-
PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry, phantom_shared_secret } =>
3105-
(incoming_cltv_expiry, payment_data.total_msat, OnionPayload::Invoice(payment_data), phantom_shared_secret),
3105+
let (cltv_expiry, onion_payload, payment_data, phantom_shared_secret) = match routing {
3106+
PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry, phantom_shared_secret } => {
3107+
let _legacy_hop_data = payment_data.clone();
3108+
(incoming_cltv_expiry, OnionPayload::Invoice { _legacy_hop_data }, Some(payment_data), phantom_shared_secret)
3109+
},
31063110
PendingHTLCRouting::ReceiveKeysend { payment_preimage, incoming_cltv_expiry } =>
3107-
(incoming_cltv_expiry, amt_to_forward, OnionPayload::Spontaneous(payment_preimage), None),
3111+
(incoming_cltv_expiry, OnionPayload::Spontaneous(payment_preimage), None, None),
31083112
_ => {
31093113
panic!("short_channel_id == 0 should imply any pending_forward entries are of type Receive");
31103114
}
@@ -3119,7 +3123,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31193123
},
31203124
value: amt_to_forward,
31213125
timer_ticks: 0,
3122-
total_msat,
3126+
total_msat: if let Some(data) = &payment_data { data.total_msat } else { amt_to_forward },
31233127
cltv_expiry,
31243128
onion_payload,
31253129
};
@@ -3143,7 +3147,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31433147
}
31443148

31453149
macro_rules! check_total_value {
3146-
($payment_data_total_msat: expr, $payment_secret: expr, $payment_preimage: expr) => {{
3150+
($payment_data: expr, $payment_preimage: expr) => {{
31473151
let mut payment_received_generated = false;
31483152
let htlcs = channel_state.claimable_htlcs.entry(payment_hash)
31493153
.or_insert(Vec::new());
@@ -3159,27 +3163,27 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31593163
total_value += htlc.value;
31603164
match &htlc.onion_payload {
31613165
OnionPayload::Invoice { .. } => {
3162-
if htlc.total_msat != $payment_data_total_msat {
3166+
if htlc.total_msat != $payment_data.total_msat {
31633167
log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the HTLCs had inconsistent total values (eg {} and {})",
3164-
log_bytes!(payment_hash.0), $payment_data_total_msat, htlc.total_msat);
3168+
log_bytes!(payment_hash.0), $payment_data.total_msat, htlc.total_msat);
31653169
total_value = msgs::MAX_VALUE_MSAT;
31663170
}
31673171
if total_value >= msgs::MAX_VALUE_MSAT { break; }
31683172
},
31693173
_ => unreachable!(),
31703174
}
31713175
}
3172-
if total_value >= msgs::MAX_VALUE_MSAT || total_value > $payment_data_total_msat {
3176+
if total_value >= msgs::MAX_VALUE_MSAT || total_value > $payment_data.total_msat {
31733177
log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the total value {} ran over expected value {} (or HTLCs were inconsistent)",
3174-
log_bytes!(payment_hash.0), total_value, $payment_data_total_msat);
3178+
log_bytes!(payment_hash.0), total_value, $payment_data.total_msat);
31753179
fail_htlc!(claimable_htlc);
3176-
} else if total_value == $payment_data_total_msat {
3180+
} else if total_value == $payment_data.total_msat {
31773181
htlcs.push(claimable_htlc);
31783182
new_events.push(events::Event::PaymentReceived {
31793183
payment_hash,
31803184
purpose: events::PaymentPurpose::InvoicePayment {
31813185
payment_preimage: $payment_preimage,
3182-
payment_secret: $payment_secret,
3186+
payment_secret: $payment_data.payment_secret,
31833187
},
31843188
amt: total_value,
31853189
});
@@ -3204,16 +3208,16 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
32043208
match payment_secrets.entry(payment_hash) {
32053209
hash_map::Entry::Vacant(_) => {
32063210
match claimable_htlc.onion_payload {
3207-
OnionPayload::Invoice(ref payment_data) => {
3211+
OnionPayload::Invoice { .. } => {
3212+
let payment_data = payment_data.unwrap();
32083213
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) {
32093214
Ok(payment_preimage) => payment_preimage,
32103215
Err(()) => {
32113216
fail_htlc!(claimable_htlc);
32123217
continue
32133218
}
32143219
};
3215-
let payment_secret = payment_data.payment_secret.clone();
3216-
check_total_value!(payment_data.total_msat, payment_secret, payment_preimage);
3220+
check_total_value!(payment_data, payment_preimage);
32173221
},
32183222
OnionPayload::Spontaneous(preimage) => {
32193223
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
32343238
}
32353239
},
32363240
hash_map::Entry::Occupied(inbound_payment) => {
3237-
let payment_data =
3238-
if let OnionPayload::Invoice(ref data) = claimable_htlc.onion_payload {
3239-
data.clone()
3240-
} else {
3241-
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));
3242-
fail_htlc!(claimable_htlc);
3243-
continue
3244-
};
3241+
if payment_data.is_none() {
3242+
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));
3243+
fail_htlc!(claimable_htlc);
3244+
continue
3245+
};
3246+
let payment_data = payment_data.unwrap();
32453247
if inbound_payment.get().payment_secret != payment_data.payment_secret {
32463248
log_trace!(self.logger, "Failing new HTLC with payment_hash {} as it didn't match our expected payment secret.", log_bytes!(payment_hash.0));
32473249
fail_htlc!(claimable_htlc);
@@ -3250,7 +3252,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
32503252
log_bytes!(payment_hash.0), payment_data.total_msat, inbound_payment.get().min_value_msat.unwrap());
32513253
fail_htlc!(claimable_htlc);
32523254
} else {
3253-
let payment_received_generated = check_total_value!(payment_data.total_msat, payment_data.payment_secret, inbound_payment.get().payment_preimage);
3255+
let payment_received_generated = check_total_value!(payment_data, inbound_payment.get().payment_preimage);
32543256
if payment_received_generated {
32553257
inbound_payment.remove_entry();
32563258
}
@@ -3469,10 +3471,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
34693471
debug_assert!(false);
34703472
return false;
34713473
}
3472-
if let OnionPayload::Invoice(ref final_hop_data) = htlcs[0].onion_payload {
3474+
if let OnionPayload::Invoice { .. } = htlcs[0].onion_payload {
34733475
// Check if we've received all the parts we need for an MPP (the value of the parts adds to total_msat).
34743476
// In this case we're not going to handle any timeouts of the parts here.
3475-
if final_hop_data.total_msat == htlcs.iter().fold(0, |total, htlc| total + htlc.value) {
3477+
if htlcs[0].total_msat == htlcs.iter().fold(0, |total, htlc| total + htlc.value) {
34763478
return true;
34773479
} else if htlcs.into_iter().any(|htlc| {
34783480
htlc.timer_ticks += 1;
@@ -6073,11 +6075,11 @@ impl_writeable_tlv_based!(HTLCPreviousHopData, {
60736075
impl Writeable for ClaimableHTLC {
60746076
fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
60756077
let payment_data = match &self.onion_payload {
6076-
OnionPayload::Invoice(data) => Some(data.clone()),
6078+
OnionPayload::Invoice { _legacy_hop_data } => Some(_legacy_hop_data),
60776079
_ => None,
60786080
};
60796081
let keysend_preimage = match self.onion_payload {
6080-
OnionPayload::Invoice(_) => None,
6082+
OnionPayload::Invoice { .. } => None,
60816083
OnionPayload::Spontaneous(preimage) => Some(preimage.clone()),
60826084
};
60836085
write_tlv_fields!(writer, {
@@ -6125,7 +6127,7 @@ impl Readable for ClaimableHTLC {
61256127
if total_msat.is_none() {
61266128
total_msat = Some(payment_data.as_ref().unwrap().total_msat);
61276129
}
6128-
OnionPayload::Invoice(payment_data.unwrap())
6130+
OnionPayload::Invoice { _legacy_hop_data: payment_data.unwrap() }
61296131
},
61306132
};
61316133
Ok(Self {

0 commit comments

Comments
 (0)