Skip to content

Commit d8d9dc7

Browse files
wpaulinowpaulino
committed
Enable decoding new incoming HTLC onions when fully committed
This commit ensures all new incoming HTLCs going forward will have their onion decoded when they become fully committed to decide how we should proceed with each one. As a result, we'll obtain `HTLCHandlingFailed` events for _any_ failed HTLC that comes across a channel. Previously, we would evaluate the incoming HTLC within `can_accept_incoming_htlc` upon receiving it, but not yet committed, so we'd always have to account for it ourselves manually when checking certain HTLC limits. With this change, we no longer need to do so as it will already be accounted for within the pending HTLC stats computation. We will now start writing channels with the new serialization version (4), and we will still be able to downgrade back to the commit that introduced it since reading version 4 is supported. Note that existing pending inbound HTLCs may already have their resolution if they were received in a previous version of LDK. We must support those until we no longer allow downgrading beyond this commit.
1 parent df9f3c0 commit d8d9dc7

File tree

12 files changed

+356
-310
lines changed

12 files changed

+356
-310
lines changed

‎fuzz/src/full_stack.rs‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1338,8 +1338,8 @@ mod tests {
13381338
// end of update_add_htlc from 0 to 1 via client and mac
13391339
ext_from_hex("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff ab00000000000000000000000000000000000000000000000000000000000000 03000000000000000000000000000000", &mut test);
13401340

1341-
// Two feerate requests to check dust exposure
1342-
ext_from_hex("00fd00fd", &mut test);
1341+
// One feerate request to check dust exposure
1342+
ext_from_hex("00fd", &mut test);
13431343

13441344
// inbound read from peer id 0 of len 18
13451345
ext_from_hex("030012", &mut test);
@@ -1362,8 +1362,8 @@ mod tests {
13621362

13631363
// process the now-pending HTLC forward
13641364
ext_from_hex("07", &mut test);
1365-
// Three feerate requests to check dust exposure
1366-
ext_from_hex("00fd00fd00fd", &mut test);
1365+
// Four feerate requests to check dust exposure while forwarding the HTLC
1366+
ext_from_hex("00fd00fd00fd00fd", &mut test);
13671367
// client now sends id 1 update_add_htlc and commitment_signed (CHECK 7: UpdateHTLCs event for node 03020000 with 1 HTLCs for channel 3f000000)
13681368

13691369
// we respond with commitment_signed then revoke_and_ack (a weird, but valid, order)
@@ -1439,8 +1439,8 @@ mod tests {
14391439
// end of update_add_htlc from 0 to 1 via client and mac
14401440
ext_from_hex("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff ab00000000000000000000000000000000000000000000000000000000000000 03000000000000000000000000000000", &mut test);
14411441

1442-
// Two feerate requests to check dust exposure
1443-
ext_from_hex("00fd00fd", &mut test);
1442+
// One feerate request to check dust exposure
1443+
ext_from_hex("00fd", &mut test);
14441444

14451445
// now respond to the update_fulfill_htlc+commitment_signed messages the client sent to peer 0
14461446
// inbound read from peer id 0 of len 18
@@ -1474,8 +1474,8 @@ mod tests {
14741474
// process the now-pending HTLC forward
14751475
ext_from_hex("07", &mut test);
14761476

1477-
// Three feerate requests to check dust exposure
1478-
ext_from_hex("00fd00fd00fd", &mut test);
1477+
// Four feerate requests to check dust exposure while forwarding the HTLC
1478+
ext_from_hex("00fd00fd00fd00fd", &mut test);
14791479

14801480
// client now sends id 1 update_add_htlc and commitment_signed (CHECK 7 duplicate)
14811481
// we respond with revoke_and_ack, then commitment_signed, then update_fail_htlc
@@ -1574,8 +1574,8 @@ mod tests {
15741574
// end of update_add_htlc from 0 to 1 via client and mac
15751575
ext_from_hex("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff 5300000000000000000000000000000000000000000000000000000000000000 03000000000000000000000000000000", &mut test);
15761576

1577-
// Two feerate requests to check dust exposure
1578-
ext_from_hex("00fd00fd", &mut test);
1577+
// One feerate request to check dust exposure
1578+
ext_from_hex("00fd", &mut test);
15791579

15801580
// inbound read from peer id 0 of len 18
15811581
ext_from_hex("030012", &mut test);
@@ -1598,8 +1598,8 @@ mod tests {
15981598

15991599
// process the now-pending HTLC forward
16001600
ext_from_hex("07", &mut test);
1601-
// Three feerate requests to check dust exposure
1602-
ext_from_hex("00fd00fd00fd", &mut test);
1601+
// Four feerate requests to check dust exposure while forwarding the HTLC
1602+
ext_from_hex("00fd00fd00fd00fd", &mut test);
16031603
// client now sends id 1 update_add_htlc and commitment_signed (CHECK 7 duplicate)
16041604

16051605
// connect a block with one transaction of len 125

‎lightning/src/events/mod.rs‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1426,16 +1426,6 @@ pub enum Event {
14261426
/// Indicates that the HTLC was accepted, but could not be processed when or after attempting to
14271427
/// forward it.
14281428
///
1429-
/// Some scenarios where this event may be sent include:
1430-
/// * Insufficient capacity in the outbound channel
1431-
/// * While waiting to forward the HTLC, the channel it is meant to be forwarded through closes
1432-
/// * When an unknown SCID is requested for forwarding a payment.
1433-
/// * Expected MPP amount has already been reached
1434-
/// * The HTLC has timed out
1435-
///
1436-
/// This event, however, does not get generated if an HTLC fails to meet the forwarding
1437-
/// requirements (i.e. insufficient fees paid, or a CLTV that is too soon).
1438-
///
14391429
/// # Failure Behavior and Persistence
14401430
/// This event will eventually be replayed after failures-to-handle (i.e., the event handler
14411431
/// returning `Err(ReplayEvent ())`) and will be persisted across restarts.

‎lightning/src/ln/blinded_payment_tests.rs‎

Lines changed: 57 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -309,8 +309,10 @@ fn do_forward_checks_failure(check: ForwardCheckFail, intro_fails: bool) {
309309
// We need the session priv to construct a bogus onion packet later.
310310
*nodes[0].keys_manager.override_random_bytes.lock().unwrap() = Some([3; 32]);
311311
create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0);
312-
let chan_upd_1_2 = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 0).0.contents;
313-
let chan_upd_2_3 = create_announced_chan_between_nodes_with_value(&nodes, 2, 3, 1_000_000, 0).0.contents;
312+
let chan_1_2 = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 0);
313+
let chan_upd_1_2 = chan_1_2.0.contents;
314+
let chan_2_3 = create_announced_chan_between_nodes_with_value(&nodes, 2, 3, 1_000_000, 0);
315+
let chan_upd_2_3 = chan_2_3.0.contents;
314316

315317
let amt_msat = 5000;
316318
let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[3], Some(amt_msat), None);
@@ -364,18 +366,27 @@ fn do_forward_checks_failure(check: ForwardCheckFail, intro_fails: bool) {
364366
check_added_monitors!(nodes[1], 0);
365367
do_commitment_signed_dance(&nodes[1], &nodes[0], &updates_0_1.commitment_signed, true, true);
366368

369+
expect_pending_htlcs_forwardable!(nodes[1]);
370+
check_added_monitors!(nodes[1], 1);
371+
367372
if intro_fails {
368373
let mut updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
369374
nodes[0].node.handle_update_fail_htlc(nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
370375
do_commitment_signed_dance(&nodes[0], &nodes[1], &updates.commitment_signed, false, false);
376+
let failed_destination = match check {
377+
ForwardCheckFail::InboundOnionCheck => HTLCDestination::InvalidOnion,
378+
ForwardCheckFail::ForwardPayloadEncodedAsReceive => HTLCDestination::FailedPayment { payment_hash },
379+
ForwardCheckFail::OutboundChannelCheck =>
380+
HTLCDestination::NextHopChannel { node_id: Some(nodes[2].node.get_our_node_id()), channel_id: chan_1_2.2 },
381+
};
382+
expect_htlc_handling_failed_destinations!(
383+
nodes[1].node.get_and_clear_pending_events(), &[failed_destination.clone()]
384+
);
371385
expect_payment_failed_conditions(&nodes[0], payment_hash, false,
372386
PaymentFailedConditions::new().expected_htlc_error_data(INVALID_ONION_BLINDING, &[0; 32]));
373387
return
374388
}
375389

376-
expect_pending_htlcs_forwardable!(nodes[1]);
377-
check_added_monitors!(nodes[1], 1);
378-
379390
let mut updates_1_2 = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
380391
let mut update_add = &mut updates_1_2.update_add_htlcs[0];
381392

@@ -385,6 +396,17 @@ fn do_forward_checks_failure(check: ForwardCheckFail, intro_fails: bool) {
385396
check_added_monitors!(nodes[2], 0);
386397
do_commitment_signed_dance(&nodes[2], &nodes[1], &updates_1_2.commitment_signed, true, true);
387398

399+
expect_pending_htlcs_forwardable!(nodes[2]);
400+
let failed_destination = match check {
401+
ForwardCheckFail::InboundOnionCheck|ForwardCheckFail::ForwardPayloadEncodedAsReceive => HTLCDestination::InvalidOnion,
402+
ForwardCheckFail::OutboundChannelCheck =>
403+
HTLCDestination::NextHopChannel { node_id: Some(nodes[3].node.get_our_node_id()), channel_id: chan_2_3.2 },
404+
};
405+
expect_htlc_handling_failed_destinations!(
406+
nodes[2].node.get_and_clear_pending_events(), &[failed_destination.clone()]
407+
);
408+
check_added_monitors!(nodes[2], 1);
409+
388410
let mut updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
389411
let update_malformed = &mut updates.update_fail_malformed_htlcs[0];
390412
assert_eq!(update_malformed.failure_code, INVALID_ONION_BLINDING);
@@ -444,7 +466,10 @@ fn failed_backwards_to_intro_node() {
444466
nodes[2].node.handle_update_add_htlc(nodes[1].node.get_our_node_id(), &payment_event.msgs[0]);
445467
check_added_monitors!(nodes[2], 0);
446468
do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event.commitment_msg, true, true);
447-
nodes[2].node.process_pending_htlc_forwards();
469+
470+
expect_pending_htlcs_forwardable!(nodes[2]);
471+
expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCDestination::InvalidOnion]);
472+
check_added_monitors(&nodes[2], 1);
448473

449474
let mut updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
450475
let mut update_malformed = &mut updates.update_fail_malformed_htlcs[0];
@@ -521,7 +546,7 @@ fn do_forward_fail_in_process_pending_htlc_fwds(check: ProcessPendingHTLCsCheck,
521546
// intro node will error backwards.
522547
$curr_node.node.peer_disconnected($next_node.node.get_our_node_id());
523548
expect_pending_htlcs_forwardable!($curr_node);
524-
expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!($curr_node,
549+
expect_htlc_handling_failed_destinations!($curr_node.node.get_and_clear_pending_events(),
525550
vec![HTLCDestination::NextHopChannel { node_id: Some($next_node.node.get_our_node_id()), channel_id: $failed_chan_id }]);
526551
},
527552
ProcessPendingHTLCsCheck::FwdChannelClosed => {
@@ -537,12 +562,12 @@ fn do_forward_fail_in_process_pending_htlc_fwds(check: ProcessPendingHTLCsCheck,
537562
crate::events::Event::ChannelClosed { .. } => {},
538563
_ => panic!("Unexpected event {:?}", events),
539564
}
565+
check_closed_broadcast(&$curr_node, 1, true);
566+
check_added_monitors!($curr_node, 1);
540567

541568
$curr_node.node.process_pending_htlc_forwards();
542-
expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!($curr_node,
569+
expect_htlc_handling_failed_destinations!($curr_node.node.get_and_clear_pending_events(),
543570
vec![HTLCDestination::UnknownNextHop { requested_forward_scid: $failed_scid }]);
544-
check_closed_broadcast(&$curr_node, 1, true);
545-
check_added_monitors!($curr_node, 1);
546571
$curr_node.node.process_pending_htlc_forwards();
547572
},
548573
}
@@ -628,6 +653,7 @@ fn do_blinded_intercept_payment(intercept_node_fails: bool) {
628653
};
629654
nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
630655
commitment_signed_dance!(nodes[1], nodes[0], &payment_event.commitment_msg, false, true);
656+
expect_pending_htlcs_forwardable!(nodes[1]);
631657

632658
let events = nodes[1].node.get_and_clear_pending_events();
633659
assert_eq!(events.len(), 1);
@@ -933,13 +959,19 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
933959
nodes[2].node.handle_update_add_htlc(nodes[1].node.get_our_node_id(), update_add);
934960
check_added_monitors!(nodes[2], 0);
935961
do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event_1_2.commitment_msg, true, true);
962+
expect_pending_htlcs_forwardable!(nodes[2]);
963+
expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCDestination::InvalidOnion]);
964+
check_added_monitors(&nodes[2], 1);
936965
},
937966
ReceiveCheckFail::ReceiveRequirements => {
938967
let update_add = &mut payment_event_1_2.msgs[0];
939968
update_add.amount_msat -= 1;
940969
nodes[2].node.handle_update_add_htlc(nodes[1].node.get_our_node_id(), update_add);
941970
check_added_monitors!(nodes[2], 0);
942971
do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event_1_2.commitment_msg, true, true);
972+
expect_pending_htlcs_forwardable!(nodes[2]);
973+
expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCDestination::FailedPayment { payment_hash }]);
974+
check_added_monitors(&nodes[2], 1);
943975
},
944976
ReceiveCheckFail::ChannelCheck => {
945977
nodes[2].node.close_channel(&chan_id_1_2, &nodes[1].node.get_our_node_id()).unwrap();
@@ -953,6 +985,9 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
953985

954986
nodes[2].node.handle_shutdown(nodes[1].node.get_our_node_id(), &node_1_shutdown);
955987
commitment_signed_dance!(nodes[2], nodes[1], (), false, true, false, false);
988+
expect_pending_htlcs_forwardable!(nodes[2]);
989+
expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCDestination::FailedPayment { payment_hash }]);
990+
check_added_monitors(&nodes[2], 1);
956991
},
957992
ReceiveCheckFail::ProcessPendingHTLCsCheck => {
958993
assert_eq!(payment_event_1_2.msgs[0].cltv_expiry, nodes[0].best_block_info().1 + 1 + excess_final_cltv_delta_opt.unwrap() as u32 + TEST_FINAL_CLTV);
@@ -968,6 +1003,9 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
9681003
nodes[2].node.handle_update_add_htlc(nodes[1].node.get_our_node_id(), &payment_event_1_2.msgs[0]);
9691004
check_added_monitors!(nodes[2], 0);
9701005
do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event_1_2.commitment_msg, true, true);
1006+
expect_pending_htlcs_forwardable!(nodes[2]);
1007+
expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCDestination::FailedPayment { payment_hash }]);
1008+
check_added_monitors(&nodes[2], 1);
9711009
}
9721010
}
9731011

@@ -1168,6 +1206,12 @@ fn min_htlc() {
11681206
nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &payment_event_0_1.msgs[0]);
11691207
check_added_monitors!(nodes[1], 0);
11701208
do_commitment_signed_dance(&nodes[1], &nodes[0], &payment_event_0_1.commitment_msg, true, true);
1209+
expect_pending_htlcs_forwardable!(nodes[1]);
1210+
expect_htlc_handling_failed_destinations!(
1211+
nodes[1].node.get_and_clear_pending_events(),
1212+
&[HTLCDestination::NextHopChannel { node_id: Some(nodes[2].node.get_our_node_id()), channel_id: chan_1_2.2 }]
1213+
);
1214+
check_added_monitors(&nodes[1], 1);
11711215
let mut updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
11721216
nodes[0].node.handle_update_fail_htlc(nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
11731217
do_commitment_signed_dance(&nodes[0], &nodes[1], &updates.commitment_signed, false, false);
@@ -1497,9 +1541,11 @@ fn fails_receive_tlvs_authentication() {
14971541
let mut payment_event = SendEvent::from_event(ev);
14981542

14991543
nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
1500-
check_added_monitors!(nodes[1], 0);
15011544
do_commitment_signed_dance(&nodes[1], &nodes[0], &payment_event.commitment_msg, true, true);
1545+
expect_pending_htlcs_forwardable!(nodes[1]);
15021546
nodes[1].node.process_pending_htlc_forwards();
1547+
check_added_monitors!(nodes[1], 1);
1548+
expect_htlc_handling_failed_destinations!(nodes[1].node.get_and_clear_pending_events(), &[HTLCDestination::InvalidOnion]);
15031549

15041550
let mut update_fail = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
15051551
assert!(update_fail.update_fail_htlcs.len() == 1);

0 commit comments

Comments
 (0)