Skip to content

Commit 1973f1f

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 1973f1f

File tree

1 file changed

+37
-30
lines changed

1 file changed

+37
-30
lines changed

‎lightning/src/ln/channelmanager.rs

Lines changed: 37 additions & 30 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
}
@@ -3100,11 +3104,16 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31003104
HTLCForwardInfo::AddHTLC { prev_short_channel_id, prev_htlc_id, forward_info: PendingHTLCInfo {
31013105
routing, incoming_shared_secret, payment_hash, amt_to_forward, .. },
31023106
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),
3107+
let (cltv_expiry, onion_payload, payment_data, phantom_shared_secret) = match routing {
3108+
PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry, phantom_shared_secret } => {
3109+
let _legacy_hop_data = msgs::FinalOnionHopData {
3110+
payment_secret: payment_data.payment_secret,
3111+
total_msat: payment_data.total_msat
3112+
};
3113+
(incoming_cltv_expiry, OnionPayload::Invoice { _legacy_hop_data }, Some(payment_data), phantom_shared_secret)
3114+
},
31063115
PendingHTLCRouting::ReceiveKeysend { payment_preimage, incoming_cltv_expiry } =>
3107-
(incoming_cltv_expiry, amt_to_forward, OnionPayload::Spontaneous(payment_preimage), None),
3116+
(incoming_cltv_expiry, OnionPayload::Spontaneous(payment_preimage), None, None),
31083117
_ => {
31093118
panic!("short_channel_id == 0 should imply any pending_forward entries are of type Receive");
31103119
}
@@ -3119,7 +3128,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31193128
},
31203129
value: amt_to_forward,
31213130
timer_ticks: 0,
3122-
total_msat,
3131+
total_msat: if let Some(data) = &payment_data { data.total_msat } else { amt_to_forward },
31233132
cltv_expiry,
31243133
onion_payload,
31253134
};
@@ -3143,7 +3152,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31433152
}
31443153

31453154
macro_rules! check_total_value {
3146-
($payment_data_total_msat: expr, $payment_secret: expr, $payment_preimage: expr) => {{
3155+
($payment_data: expr, $payment_preimage: expr) => {{
31473156
let mut payment_received_generated = false;
31483157
let htlcs = channel_state.claimable_htlcs.entry(payment_hash)
31493158
.or_insert(Vec::new());
@@ -3159,27 +3168,27 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31593168
total_value += htlc.value;
31603169
match &htlc.onion_payload {
31613170
OnionPayload::Invoice { .. } => {
3162-
if htlc.total_msat != $payment_data_total_msat {
3171+
if htlc.total_msat != $payment_data.total_msat {
31633172
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);
3173+
log_bytes!(payment_hash.0), $payment_data.total_msat, htlc.total_msat);
31653174
total_value = msgs::MAX_VALUE_MSAT;
31663175
}
31673176
if total_value >= msgs::MAX_VALUE_MSAT { break; }
31683177
},
31693178
_ => unreachable!(),
31703179
}
31713180
}
3172-
if total_value >= msgs::MAX_VALUE_MSAT || total_value > $payment_data_total_msat {
3181+
if total_value >= msgs::MAX_VALUE_MSAT || total_value > $payment_data.total_msat {
31733182
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);
3183+
log_bytes!(payment_hash.0), total_value, $payment_data.total_msat);
31753184
fail_htlc!(claimable_htlc);
3176-
} else if total_value == $payment_data_total_msat {
3185+
} else if total_value == $payment_data.total_msat {
31773186
htlcs.push(claimable_htlc);
31783187
new_events.push(events::Event::PaymentReceived {
31793188
payment_hash,
31803189
purpose: events::PaymentPurpose::InvoicePayment {
31813190
payment_preimage: $payment_preimage,
3182-
payment_secret: $payment_secret,
3191+
payment_secret: $payment_data.payment_secret,
31833192
},
31843193
amt: total_value,
31853194
});
@@ -3204,16 +3213,16 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
32043213
match payment_secrets.entry(payment_hash) {
32053214
hash_map::Entry::Vacant(_) => {
32063215
match claimable_htlc.onion_payload {
3207-
OnionPayload::Invoice(ref payment_data) => {
3216+
OnionPayload::Invoice { .. } => {
3217+
let payment_data = payment_data.unwrap();
32083218
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) {
32093219
Ok(payment_preimage) => payment_preimage,
32103220
Err(()) => {
32113221
fail_htlc!(claimable_htlc);
32123222
continue
32133223
}
32143224
};
3215-
let payment_secret = payment_data.payment_secret.clone();
3216-
check_total_value!(payment_data.total_msat, payment_secret, payment_preimage);
3225+
check_total_value!(payment_data, payment_preimage);
32173226
},
32183227
OnionPayload::Spontaneous(preimage) => {
32193228
match channel_state.claimable_htlcs.entry(payment_hash) {
@@ -3234,14 +3243,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
32343243
}
32353244
},
32363245
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-
};
3246+
if payment_data.is_none() {
3247+
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));
3248+
fail_htlc!(claimable_htlc);
3249+
continue
3250+
};
3251+
let payment_data = payment_data.unwrap();
32453252
if inbound_payment.get().payment_secret != payment_data.payment_secret {
32463253
log_trace!(self.logger, "Failing new HTLC with payment_hash {} as it didn't match our expected payment secret.", log_bytes!(payment_hash.0));
32473254
fail_htlc!(claimable_htlc);
@@ -3250,7 +3257,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
32503257
log_bytes!(payment_hash.0), payment_data.total_msat, inbound_payment.get().min_value_msat.unwrap());
32513258
fail_htlc!(claimable_htlc);
32523259
} else {
3253-
let payment_received_generated = check_total_value!(payment_data.total_msat, payment_data.payment_secret, inbound_payment.get().payment_preimage);
3260+
let payment_received_generated = check_total_value!(payment_data, inbound_payment.get().payment_preimage);
32543261
if payment_received_generated {
32553262
inbound_payment.remove_entry();
32563263
}
@@ -3469,10 +3476,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
34693476
debug_assert!(false);
34703477
return false;
34713478
}
3472-
if let OnionPayload::Invoice(ref final_hop_data) = htlcs[0].onion_payload {
3479+
if let OnionPayload::Invoice { .. } = htlcs[0].onion_payload {
34733480
// Check if we've received all the parts we need for an MPP (the value of the parts adds to total_msat).
34743481
// 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) {
3482+
if htlcs[0].total_msat == htlcs.iter().fold(0, |total, htlc| total + htlc.value) {
34763483
return true;
34773484
} else if htlcs.into_iter().any(|htlc| {
34783485
htlc.timer_ticks += 1;
@@ -6073,11 +6080,11 @@ impl_writeable_tlv_based!(HTLCPreviousHopData, {
60736080
impl Writeable for ClaimableHTLC {
60746081
fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
60756082
let payment_data = match &self.onion_payload {
6076-
OnionPayload::Invoice(data) => Some(data.clone()),
6083+
OnionPayload::Invoice { _legacy_hop_data } => Some(_legacy_hop_data),
60776084
_ => None,
60786085
};
60796086
let keysend_preimage = match self.onion_payload {
6080-
OnionPayload::Invoice(_) => None,
6087+
OnionPayload::Invoice { .. } => None,
60816088
OnionPayload::Spontaneous(preimage) => Some(preimage.clone()),
60826089
};
60836090
write_tlv_fields!(writer, {
@@ -6125,7 +6132,7 @@ impl Readable for ClaimableHTLC {
61256132
if total_msat.is_none() {
61266133
total_msat = Some(payment_data.as_ref().unwrap().total_msat);
61276134
}
6128-
OnionPayload::Invoice(payment_data.unwrap())
6135+
OnionPayload::Invoice { _legacy_hop_data: payment_data.unwrap() }
61296136
},
61306137
};
61316138
Ok(Self {

0 commit comments

Comments
 (0)