Skip to content

Commit 723c1a6

Browse files
Merge pull request #2062 from alecchendev/2023-02-allow-overshoot-mpp
Allow overshooting final htlc amount and expiry
2 parents 9fd6127 + 1d31b0e commit 723c1a6

File tree

5 files changed

+274
-27
lines changed

5 files changed

+274
-27
lines changed

lightning/src/events/mod.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ pub enum HTLCDestination {
230230
///
231231
/// Some of the reasons may include:
232232
/// * HTLC Timeouts
233-
/// * Expected MPP amount to claim does not equal HTLC total
233+
/// * Expected MPP amount has already been reached
234234
/// * Claimable amount does not match expected amount
235235
FailedPayment {
236236
/// The payment hash of the payment we attempted to process.
@@ -712,7 +712,7 @@ pub enum Event {
712712
/// * Insufficient capacity in the outbound channel
713713
/// * While waiting to forward the HTLC, the channel it is meant to be forwarded through closes
714714
/// * When an unknown SCID is requested for forwarding a payment.
715-
/// * Claiming an amount for an MPP payment that exceeds the HTLC total
715+
/// * Expected MPP amount has already been reached
716716
/// * The HTLC has timed out
717717
///
718718
/// This event, however, does not get generated if an HTLC fails to meet the forwarding

lightning/src/ln/channelmanager.rs

+62-21
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,10 @@ pub(super) struct PendingHTLCInfo {
120120
pub(super) routing: PendingHTLCRouting,
121121
pub(super) incoming_shared_secret: [u8; 32],
122122
payment_hash: PaymentHash,
123+
/// Amount received
123124
pub(super) incoming_amt_msat: Option<u64>, // Added in 0.0.113
125+
/// Sender intended amount to forward or receive (actual amount received
126+
/// may overshoot this in either case)
124127
pub(super) outgoing_amt_msat: u64,
125128
pub(super) outgoing_cltv_value: u32,
126129
}
@@ -192,9 +195,15 @@ struct ClaimableHTLC {
192195
cltv_expiry: u32,
193196
/// The amount (in msats) of this MPP part
194197
value: u64,
198+
/// The amount (in msats) that the sender intended to be sent in this MPP
199+
/// part (used for validating total MPP amount)
200+
sender_intended_value: u64,
195201
onion_payload: OnionPayload,
196202
timer_ticks: u8,
197-
/// The sum total of all MPP parts
203+
/// The total value received for a payment (sum of all MPP parts if the payment is a MPP).
204+
/// Gets set to the amount reported when pushing [`Event::PaymentClaimable`].
205+
total_value_received: Option<u64>,
206+
/// The sender intended sum total of all MPP parts specified in the onion
198207
total_msat: u64,
199208
}
200209

@@ -2092,9 +2101,9 @@ where
20922101
payment_hash: PaymentHash, amt_msat: u64, cltv_expiry: u32, phantom_shared_secret: Option<[u8; 32]>) -> Result<PendingHTLCInfo, ReceiveError>
20932102
{
20942103
// final_incorrect_cltv_expiry
2095-
if hop_data.outgoing_cltv_value != cltv_expiry {
2104+
if hop_data.outgoing_cltv_value > cltv_expiry {
20962105
return Err(ReceiveError {
2097-
msg: "Upstream node set CLTV to the wrong value",
2106+
msg: "Upstream node set CLTV to less than the CLTV set by the sender",
20982107
err_code: 18,
20992108
err_data: cltv_expiry.to_be_bytes().to_vec()
21002109
})
@@ -2178,7 +2187,7 @@ where
21782187
payment_hash,
21792188
incoming_shared_secret: shared_secret,
21802189
incoming_amt_msat: Some(amt_msat),
2181-
outgoing_amt_msat: amt_msat,
2190+
outgoing_amt_msat: hop_data.amt_to_forward,
21822191
outgoing_cltv_value: hop_data.outgoing_cltv_value,
21832192
})
21842193
}
@@ -2660,7 +2669,7 @@ where
26602669
}
26612670

26622671
#[cfg(test)]
2663-
fn test_send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, keysend_preimage: Option<PaymentPreimage>, payment_id: PaymentId, recv_value_msat: Option<u64>, onion_session_privs: Vec<[u8; 32]>) -> Result<(), PaymentSendFailure> {
2672+
pub(super) fn test_send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, keysend_preimage: Option<PaymentPreimage>, payment_id: PaymentId, recv_value_msat: Option<u64>, onion_session_privs: Vec<[u8; 32]>) -> Result<(), PaymentSendFailure> {
26642673
let best_block_height = self.best_block.read().unwrap().height();
26652674
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
26662675
self.pending_outbound_payments.test_send_payment_internal(route, payment_hash, payment_secret, keysend_preimage, payment_id, recv_value_msat, onion_session_privs, &self.node_signer, best_block_height,
@@ -3258,7 +3267,7 @@ where
32583267
HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo {
32593268
prev_short_channel_id, prev_htlc_id, prev_funding_outpoint, prev_user_channel_id,
32603269
forward_info: PendingHTLCInfo {
3261-
routing, incoming_shared_secret, payment_hash, outgoing_amt_msat, ..
3270+
routing, incoming_shared_secret, payment_hash, incoming_amt_msat, outgoing_amt_msat, ..
32623271
}
32633272
}) => {
32643273
let (cltv_expiry, onion_payload, payment_data, phantom_shared_secret) = match routing {
@@ -3272,16 +3281,21 @@ where
32723281
panic!("short_channel_id == 0 should imply any pending_forward entries are of type Receive");
32733282
}
32743283
};
3275-
let claimable_htlc = ClaimableHTLC {
3284+
let mut claimable_htlc = ClaimableHTLC {
32763285
prev_hop: HTLCPreviousHopData {
32773286
short_channel_id: prev_short_channel_id,
32783287
outpoint: prev_funding_outpoint,
32793288
htlc_id: prev_htlc_id,
32803289
incoming_packet_shared_secret: incoming_shared_secret,
32813290
phantom_shared_secret,
32823291
},
3283-
value: outgoing_amt_msat,
3292+
// We differentiate the received value from the sender intended value
3293+
// if possible so that we don't prematurely mark MPP payments complete
3294+
// if routing nodes overpay
3295+
value: incoming_amt_msat.unwrap_or(outgoing_amt_msat),
3296+
sender_intended_value: outgoing_amt_msat,
32843297
timer_ticks: 0,
3298+
total_value_received: None,
32853299
total_msat: if let Some(data) = &payment_data { data.total_msat } else { outgoing_amt_msat },
32863300
cltv_expiry,
32873301
onion_payload,
@@ -3326,7 +3340,7 @@ where
33263340
fail_htlc!(claimable_htlc, payment_hash);
33273341
continue
33283342
}
3329-
let (_, htlcs) = claimable_payments.claimable_htlcs.entry(payment_hash)
3343+
let (_, ref mut htlcs) = claimable_payments.claimable_htlcs.entry(payment_hash)
33303344
.or_insert_with(|| (purpose(), Vec::new()));
33313345
if htlcs.len() == 1 {
33323346
if let OnionPayload::Spontaneous(_) = htlcs[0].onion_payload {
@@ -3335,9 +3349,9 @@ where
33353349
continue
33363350
}
33373351
}
3338-
let mut total_value = claimable_htlc.value;
3352+
let mut total_value = claimable_htlc.sender_intended_value;
33393353
for htlc in htlcs.iter() {
3340-
total_value += htlc.value;
3354+
total_value += htlc.sender_intended_value;
33413355
match &htlc.onion_payload {
33423356
OnionPayload::Invoice { .. } => {
33433357
if htlc.total_msat != $payment_data.total_msat {
@@ -3350,18 +3364,24 @@ where
33503364
_ => unreachable!(),
33513365
}
33523366
}
3353-
if total_value >= msgs::MAX_VALUE_MSAT || total_value > $payment_data.total_msat {
3354-
log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the total value {} ran over expected value {} (or HTLCs were inconsistent)",
3355-
log_bytes!(payment_hash.0), total_value, $payment_data.total_msat);
3367+
// The condition determining whether an MPP is complete must
3368+
// match exactly the condition used in `timer_tick_occurred`
3369+
if total_value >= msgs::MAX_VALUE_MSAT {
33563370
fail_htlc!(claimable_htlc, payment_hash);
3357-
} else if total_value == $payment_data.total_msat {
3371+
} else if total_value - claimable_htlc.sender_intended_value >= $payment_data.total_msat {
3372+
log_trace!(self.logger, "Failing HTLC with payment_hash {} as payment is already claimable",
3373+
log_bytes!(payment_hash.0));
3374+
fail_htlc!(claimable_htlc, payment_hash);
3375+
} else if total_value >= $payment_data.total_msat {
33583376
let prev_channel_id = prev_funding_outpoint.to_channel_id();
33593377
htlcs.push(claimable_htlc);
3378+
let amount_msat = htlcs.iter().map(|htlc| htlc.value).sum();
3379+
htlcs.iter_mut().for_each(|htlc| htlc.total_value_received = Some(amount_msat));
33603380
new_events.push(events::Event::PaymentClaimable {
33613381
receiver_node_id: Some(receiver_node_id),
33623382
payment_hash,
33633383
purpose: purpose(),
3364-
amount_msat: total_value,
3384+
amount_msat,
33653385
via_channel_id: Some(prev_channel_id),
33663386
via_user_channel_id: Some(prev_user_channel_id),
33673387
});
@@ -3415,13 +3435,15 @@ where
34153435
}
34163436
match claimable_payments.claimable_htlcs.entry(payment_hash) {
34173437
hash_map::Entry::Vacant(e) => {
3438+
let amount_msat = claimable_htlc.value;
3439+
claimable_htlc.total_value_received = Some(amount_msat);
34183440
let purpose = events::PaymentPurpose::SpontaneousPayment(preimage);
34193441
e.insert((purpose.clone(), vec![claimable_htlc]));
34203442
let prev_channel_id = prev_funding_outpoint.to_channel_id();
34213443
new_events.push(events::Event::PaymentClaimable {
34223444
receiver_node_id: Some(receiver_node_id),
34233445
payment_hash,
3424-
amount_msat: outgoing_amt_msat,
3446+
amount_msat,
34253447
purpose,
34263448
via_channel_id: Some(prev_channel_id),
34273449
via_user_channel_id: Some(prev_user_channel_id),
@@ -3681,7 +3703,9 @@ where
36813703
if let OnionPayload::Invoice { .. } = htlcs[0].onion_payload {
36823704
// Check if we've received all the parts we need for an MPP (the value of the parts adds to total_msat).
36833705
// In this case we're not going to handle any timeouts of the parts here.
3684-
if htlcs[0].total_msat == htlcs.iter().fold(0, |total, htlc| total + htlc.value) {
3706+
// This condition determining whether the MPP is complete here must match
3707+
// exactly the condition used in `process_pending_htlc_forwards`.
3708+
if htlcs[0].total_msat <= htlcs.iter().fold(0, |total, htlc| total + htlc.sender_intended_value) {
36853709
return true;
36863710
} else if htlcs.into_iter().any(|htlc| {
36873711
htlc.timer_ticks += 1;
@@ -3960,6 +3984,7 @@ where
39603984
// provide the preimage, so worrying too much about the optimal handling isn't worth
39613985
// it.
39623986
let mut claimable_amt_msat = 0;
3987+
let mut prev_total_msat = None;
39633988
let mut expected_amt_msat = None;
39643989
let mut valid_mpp = true;
39653990
let mut errs = Vec::new();
@@ -3987,14 +4012,22 @@ where
39874012
break;
39884013
}
39894014

3990-
if expected_amt_msat.is_some() && expected_amt_msat != Some(htlc.total_msat) {
3991-
log_error!(self.logger, "Somehow ended up with an MPP payment with different total amounts - this should not be reachable!");
4015+
if prev_total_msat.is_some() && prev_total_msat != Some(htlc.total_msat) {
4016+
log_error!(self.logger, "Somehow ended up with an MPP payment with different expected total amounts - this should not be reachable!");
4017+
debug_assert!(false);
4018+
valid_mpp = false;
4019+
break;
4020+
}
4021+
prev_total_msat = Some(htlc.total_msat);
4022+
4023+
if expected_amt_msat.is_some() && expected_amt_msat != htlc.total_value_received {
4024+
log_error!(self.logger, "Somehow ended up with an MPP payment with different received total amounts - this should not be reachable!");
39924025
debug_assert!(false);
39934026
valid_mpp = false;
39944027
break;
39954028
}
4029+
expected_amt_msat = htlc.total_value_received;
39964030

3997-
expected_amt_msat = Some(htlc.total_msat);
39984031
if let OnionPayload::Spontaneous(_) = &htlc.onion_payload {
39994032
// We don't currently support MPP for spontaneous payments, so just check
40004033
// that there's one payment here and move on.
@@ -6794,7 +6827,9 @@ impl Writeable for ClaimableHTLC {
67946827
(0, self.prev_hop, required),
67956828
(1, self.total_msat, required),
67966829
(2, self.value, required),
6830+
(3, self.sender_intended_value, required),
67976831
(4, payment_data, option),
6832+
(5, self.total_value_received, option),
67986833
(6, self.cltv_expiry, required),
67996834
(8, keysend_preimage, option),
68006835
});
@@ -6806,15 +6841,19 @@ impl Readable for ClaimableHTLC {
68066841
fn read<R: Read>(reader: &mut R) -> Result<Self, DecodeError> {
68076842
let mut prev_hop = crate::util::ser::RequiredWrapper(None);
68086843
let mut value = 0;
6844+
let mut sender_intended_value = None;
68096845
let mut payment_data: Option<msgs::FinalOnionHopData> = None;
68106846
let mut cltv_expiry = 0;
6847+
let mut total_value_received = None;
68116848
let mut total_msat = None;
68126849
let mut keysend_preimage: Option<PaymentPreimage> = None;
68136850
read_tlv_fields!(reader, {
68146851
(0, prev_hop, required),
68156852
(1, total_msat, option),
68166853
(2, value, required),
6854+
(3, sender_intended_value, option),
68176855
(4, payment_data, option),
6856+
(5, total_value_received, option),
68186857
(6, cltv_expiry, required),
68196858
(8, keysend_preimage, option)
68206859
});
@@ -6842,6 +6881,8 @@ impl Readable for ClaimableHTLC {
68426881
prev_hop: prev_hop.0.unwrap(),
68436882
timer_ticks: 0,
68446883
value,
6884+
sender_intended_value: sender_intended_value.unwrap_or(value),
6885+
total_value_received,
68456886
total_msat: total_msat.unwrap(),
68466887
onion_payload,
68476888
cltv_expiry,

0 commit comments

Comments
 (0)