Skip to content

Commit d0cf6b8

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 35004fd commit d0cf6b8

File tree

1 file changed

+35
-28
lines changed

1 file changed

+35
-28
lines changed

‎lightning/src/ln/channelmanager.rs

Lines changed: 35 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,11 @@ enum OnionPayload {
162162
/// Contains a total_msat (which may differ from value if this is a Multi-Path Payment) and a
163163
/// payment_secret which prevents path-probing attacks and can associate different HTLCs which
164164
/// are part of the same payment.
165-
Invoice(msgs::FinalOnionHopData),
165+
Invoice {
166+
/// This is only here for backwards-compatibility in serialization, in the future it can be
167+
/// removed, breaking clients running 0.0.104 and earlier.
168+
_legacy_hop_data: msgs::FinalOnionHopData,
169+
},
166170
/// Contains the payer-provided preimage.
167171
Spontaneous(PaymentPreimage),
168172
}
@@ -3095,11 +3099,16 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
30953099
HTLCForwardInfo::AddHTLC { prev_short_channel_id, prev_htlc_id, forward_info: PendingHTLCInfo {
30963100
routing, incoming_shared_secret, payment_hash, amt_to_forward, .. },
30973101
prev_funding_outpoint } => {
3098-
let (cltv_expiry, total_msat, onion_payload, phantom_shared_secret) = match routing {
3099-
PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry, phantom_shared_secret } =>
3100-
(incoming_cltv_expiry, payment_data.total_msat, OnionPayload::Invoice(payment_data), phantom_shared_secret),
3102+
let (cltv_expiry, onion_payload, payment_data, phantom_shared_secret) = match routing {
3103+
PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry, phantom_shared_secret } => {
3104+
let _legacy_hop_data = msgs::FinalOnionHopData {
3105+
payment_secret: payment_data.payment_secret,
3106+
total_msat: payment_data.total_msat
3107+
};
3108+
(incoming_cltv_expiry, OnionPayload::Invoice { _legacy_hop_data }, Some(payment_data), phantom_shared_secret)
3109+
},
31013110
PendingHTLCRouting::ReceiveKeysend { payment_preimage, incoming_cltv_expiry } =>
3102-
(incoming_cltv_expiry, amt_to_forward, OnionPayload::Spontaneous(payment_preimage), None),
3111+
(incoming_cltv_expiry, OnionPayload::Spontaneous(payment_preimage), None, None),
31033112
_ => {
31043113
panic!("short_channel_id == 0 should imply any pending_forward entries are of type Receive");
31053114
}
@@ -3114,7 +3123,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31143123
},
31153124
value: amt_to_forward,
31163125
timer_ticks: 0,
3117-
total_msat,
3126+
total_msat: if let Some(data) = &payment_data { data.total_msat } else { amt_to_forward },
31183127
cltv_expiry,
31193128
onion_payload,
31203129
};
@@ -3138,7 +3147,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31383147
}
31393148

31403149
macro_rules! check_total_value {
3141-
($payment_data_total_msat: expr, $payment_secret: expr, $payment_preimage: expr) => {{
3150+
($payment_data: expr, $payment_preimage: expr) => {{
31423151
let mut payment_received_generated = false;
31433152
let htlcs = channel_state.claimable_htlcs.entry(payment_hash)
31443153
.or_insert(Vec::new());
@@ -3154,9 +3163,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31543163
total_value += htlc.value;
31553164
match &htlc.onion_payload {
31563165
OnionPayload::Invoice { .. } => {
3157-
if htlc.total_msat != $payment_data_total_msat {
3166+
if htlc.total_msat != $payment_data.total_msat {
31583167
log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the HTLCs had inconsistent total values (eg {} and {})",
3159-
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);
31603169
total_value = msgs::MAX_VALUE_MSAT;
31613170
}
31623171
if total_value >= msgs::MAX_VALUE_MSAT { break; }
@@ -3166,15 +3175,15 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31663175
}
31673176
if total_value >= msgs::MAX_VALUE_MSAT || total_value > claimable_htlc.total_msat {
31683177
log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the total value {} ran over expected value {} (or HTLCs were inconsistent)",
3169-
log_bytes!(payment_hash.0), total_value, $payment_data_total_msat);
3178+
log_bytes!(payment_hash.0), total_value, $payment_data.total_msat);
31703179
fail_htlc!(claimable_htlc);
3171-
} else if total_value == $payment_data_total_msat {
3180+
} else if total_value == $payment_data.total_msat {
31723181
htlcs.push(claimable_htlc);
31733182
new_events.push(events::Event::PaymentReceived {
31743183
payment_hash,
31753184
purpose: events::PaymentPurpose::InvoicePayment {
31763185
payment_preimage: $payment_preimage,
3177-
payment_secret: $payment_secret,
3186+
payment_secret: $payment_data.payment_secret,
31783187
},
31793188
amt: total_value,
31803189
});
@@ -3199,16 +3208,16 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31993208
match payment_secrets.entry(payment_hash) {
32003209
hash_map::Entry::Vacant(_) => {
32013210
match claimable_htlc.onion_payload {
3202-
OnionPayload::Invoice(ref payment_data) => {
3211+
OnionPayload::Invoice { .. } => {
3212+
let payment_data = payment_data.unwrap();
32033213
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) {
32043214
Ok(payment_preimage) => payment_preimage,
32053215
Err(()) => {
32063216
fail_htlc!(claimable_htlc);
32073217
continue
32083218
}
32093219
};
3210-
let payment_secret = payment_data.payment_secret.clone();
3211-
check_total_value!(payment_data.total_msat, payment_secret, payment_preimage);
3220+
check_total_value!(payment_data, payment_preimage);
32123221
},
32133222
OnionPayload::Spontaneous(preimage) => {
32143223
match channel_state.claimable_htlcs.entry(payment_hash) {
@@ -3229,14 +3238,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
32293238
}
32303239
},
32313240
hash_map::Entry::Occupied(inbound_payment) => {
3232-
let payment_data =
3233-
if let OnionPayload::Invoice(ref data) = claimable_htlc.onion_payload {
3234-
data.clone()
3235-
} else {
3236-
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));
3237-
fail_htlc!(claimable_htlc);
3238-
continue
3239-
};
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();
32403247
if inbound_payment.get().payment_secret != payment_data.payment_secret {
32413248
log_trace!(self.logger, "Failing new HTLC with payment_hash {} as it didn't match our expected payment secret.", log_bytes!(payment_hash.0));
32423249
fail_htlc!(claimable_htlc);
@@ -3245,7 +3252,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
32453252
log_bytes!(payment_hash.0), payment_data.total_msat, inbound_payment.get().min_value_msat.unwrap());
32463253
fail_htlc!(claimable_htlc);
32473254
} else {
3248-
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);
32493256
if payment_received_generated {
32503257
inbound_payment.remove_entry();
32513258
}
@@ -3464,7 +3471,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
34643471
debug_assert!(false);
34653472
return false;
34663473
}
3467-
if let OnionPayload::Invoice(ref final_hop_data) = htlcs[0].onion_payload {
3474+
if let OnionPayload::Invoice { _legacy_hop_data: ref final_hop_data } = htlcs[0].onion_payload {
34683475
// Check if we've received all the parts we need for an MPP (the value of the parts adds to total_msat).
34693476
// In this case we're not going to handle any timeouts of the parts here.
34703477
if final_hop_data.total_msat == htlcs.iter().fold(0, |total, htlc| total + htlc.value) {
@@ -6066,11 +6073,11 @@ impl_writeable_tlv_based!(HTLCPreviousHopData, {
60666073
impl Writeable for ClaimableHTLC {
60676074
fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
60686075
let payment_data = match &self.onion_payload {
6069-
OnionPayload::Invoice(data) => Some(data.clone()),
6076+
OnionPayload::Invoice { _legacy_hop_data } => Some(_legacy_hop_data),
60706077
_ => None,
60716078
};
60726079
let keysend_preimage = match self.onion_payload {
6073-
OnionPayload::Invoice(_) => None,
6080+
OnionPayload::Invoice { .. } => None,
60746081
OnionPayload::Spontaneous(preimage) => Some(preimage.clone()),
60756082
};
60766083
write_tlv_fields!(writer, {
@@ -6118,7 +6125,7 @@ impl Readable for ClaimableHTLC {
61186125
if total_msat.is_none() {
61196126
total_msat = Some(payment_data.as_ref().unwrap().total_msat);
61206127
}
6121-
OnionPayload::Invoice(payment_data.unwrap())
6128+
OnionPayload::Invoice { _legacy_hop_data: payment_data.unwrap() }
61226129
},
61236130
};
61246131
Ok(Self {

0 commit comments

Comments
 (0)