Skip to content

Commit c611aff

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 c611aff

File tree

1 file changed

+34
-30
lines changed

1 file changed

+34
-30
lines changed

‎lightning/src/ln/channelmanager.rs

Lines changed: 34 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.106 and earlier.
168+
_legacy_hop_data: msgs::FinalOnionHopData,
169+
},
166170
/// Contains the payer-provided preimage.
167171
Spontaneous(PaymentPreimage),
168172
}
@@ -3100,11 +3104,13 @@ 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 = payment_data.clone();
3110+
(incoming_cltv_expiry, OnionPayload::Invoice { _legacy_hop_data }, Some(payment_data), phantom_shared_secret)
3111+
},
31063112
PendingHTLCRouting::ReceiveKeysend { payment_preimage, incoming_cltv_expiry } =>
3107-
(incoming_cltv_expiry, amt_to_forward, OnionPayload::Spontaneous(payment_preimage), None),
3113+
(incoming_cltv_expiry, OnionPayload::Spontaneous(payment_preimage), None, None),
31083114
_ => {
31093115
panic!("short_channel_id == 0 should imply any pending_forward entries are of type Receive");
31103116
}
@@ -3119,7 +3125,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31193125
},
31203126
value: amt_to_forward,
31213127
timer_ticks: 0,
3122-
total_msat,
3128+
total_msat: if let Some(data) = &payment_data { data.total_msat } else { amt_to_forward },
31233129
cltv_expiry,
31243130
onion_payload,
31253131
};
@@ -3143,7 +3149,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31433149
}
31443150

31453151
macro_rules! check_total_value {
3146-
($payment_data_total_msat: expr, $payment_secret: expr, $payment_preimage: expr) => {{
3152+
($payment_data: expr, $payment_preimage: expr) => {{
31473153
let mut payment_received_generated = false;
31483154
let htlcs = channel_state.claimable_htlcs.entry(payment_hash)
31493155
.or_insert(Vec::new());
@@ -3159,27 +3165,27 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31593165
total_value += htlc.value;
31603166
match &htlc.onion_payload {
31613167
OnionPayload::Invoice { .. } => {
3162-
if htlc.total_msat != $payment_data_total_msat {
3168+
if htlc.total_msat != $payment_data.total_msat {
31633169
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);
3170+
log_bytes!(payment_hash.0), $payment_data.total_msat, htlc.total_msat);
31653171
total_value = msgs::MAX_VALUE_MSAT;
31663172
}
31673173
if total_value >= msgs::MAX_VALUE_MSAT { break; }
31683174
},
31693175
_ => unreachable!(),
31703176
}
31713177
}
3172-
if total_value >= msgs::MAX_VALUE_MSAT || total_value > $payment_data_total_msat {
3178+
if total_value >= msgs::MAX_VALUE_MSAT || total_value > $payment_data.total_msat {
31733179
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);
3180+
log_bytes!(payment_hash.0), total_value, $payment_data.total_msat);
31753181
fail_htlc!(claimable_htlc);
3176-
} else if total_value == $payment_data_total_msat {
3182+
} else if total_value == $payment_data.total_msat {
31773183
htlcs.push(claimable_htlc);
31783184
new_events.push(events::Event::PaymentReceived {
31793185
payment_hash,
31803186
purpose: events::PaymentPurpose::InvoicePayment {
31813187
payment_preimage: $payment_preimage,
3182-
payment_secret: $payment_secret,
3188+
payment_secret: $payment_data.payment_secret,
31833189
},
31843190
amt: total_value,
31853191
});
@@ -3204,16 +3210,16 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
32043210
match payment_secrets.entry(payment_hash) {
32053211
hash_map::Entry::Vacant(_) => {
32063212
match claimable_htlc.onion_payload {
3207-
OnionPayload::Invoice(ref payment_data) => {
3213+
OnionPayload::Invoice { .. } => {
3214+
let payment_data = payment_data.unwrap();
32083215
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) {
32093216
Ok(payment_preimage) => payment_preimage,
32103217
Err(()) => {
32113218
fail_htlc!(claimable_htlc);
32123219
continue
32133220
}
32143221
};
3215-
let payment_secret = payment_data.payment_secret.clone();
3216-
check_total_value!(payment_data.total_msat, payment_secret, payment_preimage);
3222+
check_total_value!(payment_data, payment_preimage);
32173223
},
32183224
OnionPayload::Spontaneous(preimage) => {
32193225
match channel_state.claimable_htlcs.entry(payment_hash) {
@@ -3234,14 +3240,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
32343240
}
32353241
},
32363242
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-
};
3243+
if payment_data.is_none() {
3244+
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));
3245+
fail_htlc!(claimable_htlc);
3246+
continue
3247+
};
3248+
let payment_data = payment_data.unwrap();
32453249
if inbound_payment.get().payment_secret != payment_data.payment_secret {
32463250
log_trace!(self.logger, "Failing new HTLC with payment_hash {} as it didn't match our expected payment secret.", log_bytes!(payment_hash.0));
32473251
fail_htlc!(claimable_htlc);
@@ -3250,7 +3254,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
32503254
log_bytes!(payment_hash.0), payment_data.total_msat, inbound_payment.get().min_value_msat.unwrap());
32513255
fail_htlc!(claimable_htlc);
32523256
} else {
3253-
let payment_received_generated = check_total_value!(payment_data.total_msat, payment_data.payment_secret, inbound_payment.get().payment_preimage);
3257+
let payment_received_generated = check_total_value!(payment_data, inbound_payment.get().payment_preimage);
32543258
if payment_received_generated {
32553259
inbound_payment.remove_entry();
32563260
}
@@ -3469,10 +3473,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
34693473
debug_assert!(false);
34703474
return false;
34713475
}
3472-
if let OnionPayload::Invoice(ref final_hop_data) = htlcs[0].onion_payload {
3476+
if let OnionPayload::Invoice { .. } = htlcs[0].onion_payload {
34733477
// Check if we've received all the parts we need for an MPP (the value of the parts adds to total_msat).
34743478
// 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) {
3479+
if htlcs[0].total_msat == htlcs.iter().fold(0, |total, htlc| total + htlc.value) {
34763480
return true;
34773481
} else if htlcs.into_iter().any(|htlc| {
34783482
htlc.timer_ticks += 1;
@@ -6073,11 +6077,11 @@ impl_writeable_tlv_based!(HTLCPreviousHopData, {
60736077
impl Writeable for ClaimableHTLC {
60746078
fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
60756079
let payment_data = match &self.onion_payload {
6076-
OnionPayload::Invoice(data) => Some(data.clone()),
6080+
OnionPayload::Invoice { _legacy_hop_data } => Some(_legacy_hop_data),
60776081
_ => None,
60786082
};
60796083
let keysend_preimage = match self.onion_payload {
6080-
OnionPayload::Invoice(_) => None,
6084+
OnionPayload::Invoice { .. } => None,
60816085
OnionPayload::Spontaneous(preimage) => Some(preimage.clone()),
60826086
};
60836087
write_tlv_fields!(writer, {
@@ -6125,7 +6129,7 @@ impl Readable for ClaimableHTLC {
61256129
if total_msat.is_none() {
61266130
total_msat = Some(payment_data.as_ref().unwrap().total_msat);
61276131
}
6128-
OnionPayload::Invoice(payment_data.unwrap())
6132+
OnionPayload::Invoice { _legacy_hop_data: payment_data.unwrap() }
61296133
},
61306134
};
61316135
Ok(Self {

0 commit comments

Comments
 (0)