Skip to content

Commit 7fff661

Browse files
wpaulinovalentinewallace
wpaulino
authored and
valentinewallace
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 25060c9 commit 7fff661

File tree

12 files changed

+361
-311
lines changed

12 files changed

+361
-311
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
@@ -1431,16 +1431,6 @@ pub enum Event {
14311431
/// Indicates that the HTLC was accepted, but could not be processed when or after attempting to
14321432
/// forward it.
14331433
///
1434-
/// Some scenarios where this event may be sent include:
1435-
/// * Insufficient capacity in the outbound channel
1436-
/// * While waiting to forward the HTLC, the channel it is meant to be forwarded through closes
1437-
/// * When an unknown SCID is requested for forwarding a payment.
1438-
/// * Expected MPP amount has already been reached
1439-
/// * The HTLC has timed out
1440-
///
1441-
/// This event, however, does not get generated if an HTLC fails to meet the forwarding
1442-
/// requirements (i.e. insufficient fees paid, or a CLTV that is too soon).
1443-
///
14441434
/// # Failure Behavior and Persistence
14451435
/// This event will eventually be replayed after failures-to-handle (i.e., the event handler
14461436
/// returning `Err(ReplayEvent ())`) and will be persisted across restarts.

‎lightning/src/ln/blinded_payment_tests.rs‎

Lines changed: 62 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -336,8 +336,10 @@ fn do_forward_checks_failure(check: ForwardCheckFail, intro_fails: bool) {
336336
// We need the session priv to construct a bogus onion packet later.
337337
*nodes[0].keys_manager.override_random_bytes.lock().unwrap() = Some([3; 32]);
338338
create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0);
339-
let chan_upd_1_2 = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 0).0.contents;
340-
let chan_upd_2_3 = create_announced_chan_between_nodes_with_value(&nodes, 2, 3, 1_000_000, 0).0.contents;
339+
let chan_1_2 = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 0);
340+
let chan_upd_1_2 = chan_1_2.0.contents;
341+
let chan_2_3 = create_announced_chan_between_nodes_with_value(&nodes, 2, 3, 1_000_000, 0);
342+
let chan_upd_2_3 = chan_2_3.0.contents;
341343

342344
let amt_msat = 5000;
343345
let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[3], Some(amt_msat), None);
@@ -391,14 +393,27 @@ fn do_forward_checks_failure(check: ForwardCheckFail, intro_fails: bool) {
391393
check_added_monitors!(nodes[1], 0);
392394
do_commitment_signed_dance(&nodes[1], &nodes[0], &updates_0_1.commitment_signed, true, true);
393395

396+
expect_pending_htlcs_forwardable!(nodes[1]);
397+
check_added_monitors!(nodes[1], 1);
398+
394399
if intro_fails {
395-
fail_blinded_htlc_backwards(payment_hash, 1, &[&nodes[0], &nodes[1]]);
400+
let mut updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
401+
nodes[0].node.handle_update_fail_htlc(nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
402+
do_commitment_signed_dance(&nodes[0], &nodes[1], &updates.commitment_signed, false, false);
403+
let failed_destination = match check {
404+
ForwardCheckFail::InboundOnionCheck => HTLCDestination::InvalidOnion,
405+
ForwardCheckFail::ForwardPayloadEncodedAsReceive => HTLCDestination::FailedPayment { payment_hash },
406+
ForwardCheckFail::OutboundChannelCheck =>
407+
HTLCDestination::NextHopChannel { node_id: Some(nodes[2].node.get_our_node_id()), channel_id: chan_1_2.2 },
408+
};
409+
expect_htlc_handling_failed_destinations!(
410+
nodes[1].node.get_and_clear_pending_events(), &[failed_destination.clone()]
411+
);
412+
expect_payment_failed_conditions(&nodes[0], payment_hash, false,
413+
PaymentFailedConditions::new().expected_htlc_error_data(INVALID_ONION_BLINDING, &[0; 32]));
396414
return
397415
}
398416

399-
expect_pending_htlcs_forwardable!(nodes[1]);
400-
check_added_monitors!(nodes[1], 1);
401-
402417
let mut updates_1_2 = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
403418
let mut update_add = &mut updates_1_2.update_add_htlcs[0];
404419

@@ -408,6 +423,17 @@ fn do_forward_checks_failure(check: ForwardCheckFail, intro_fails: bool) {
408423
check_added_monitors!(nodes[2], 0);
409424
do_commitment_signed_dance(&nodes[2], &nodes[1], &updates_1_2.commitment_signed, true, true);
410425

426+
expect_pending_htlcs_forwardable!(nodes[2]);
427+
let failed_destination = match check {
428+
ForwardCheckFail::InboundOnionCheck|ForwardCheckFail::ForwardPayloadEncodedAsReceive => HTLCDestination::InvalidOnion,
429+
ForwardCheckFail::OutboundChannelCheck =>
430+
HTLCDestination::NextHopChannel { node_id: Some(nodes[3].node.get_our_node_id()), channel_id: chan_2_3.2 },
431+
};
432+
expect_htlc_handling_failed_destinations!(
433+
nodes[2].node.get_and_clear_pending_events(), &[failed_destination.clone()]
434+
);
435+
check_added_monitors!(nodes[2], 1);
436+
411437
let mut updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
412438
let update_malformed = &mut updates.update_fail_malformed_htlcs[0];
413439
assert_eq!(update_malformed.failure_code, INVALID_ONION_BLINDING);
@@ -467,7 +493,10 @@ fn failed_backwards_to_intro_node() {
467493
nodes[2].node.handle_update_add_htlc(nodes[1].node.get_our_node_id(), &payment_event.msgs[0]);
468494
check_added_monitors!(nodes[2], 0);
469495
do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event.commitment_msg, true, true);
470-
nodes[2].node.process_pending_htlc_forwards();
496+
497+
expect_pending_htlcs_forwardable!(nodes[2]);
498+
expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCDestination::InvalidOnion]);
499+
check_added_monitors(&nodes[2], 1);
471500

472501
let mut updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
473502
let mut update_malformed = &mut updates.update_fail_malformed_htlcs[0];
@@ -544,7 +573,7 @@ fn do_forward_fail_in_process_pending_htlc_fwds(check: ProcessPendingHTLCsCheck,
544573
// intro node will error backwards.
545574
$curr_node.node.peer_disconnected($next_node.node.get_our_node_id());
546575
expect_pending_htlcs_forwardable!($curr_node);
547-
expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!($curr_node,
576+
expect_htlc_handling_failed_destinations!($curr_node.node.get_and_clear_pending_events(),
548577
vec![HTLCDestination::NextHopChannel { node_id: Some($next_node.node.get_our_node_id()), channel_id: $failed_chan_id }]);
549578
},
550579
ProcessPendingHTLCsCheck::FwdChannelClosed => {
@@ -560,12 +589,12 @@ fn do_forward_fail_in_process_pending_htlc_fwds(check: ProcessPendingHTLCsCheck,
560589
crate::events::Event::ChannelClosed { .. } => {},
561590
_ => panic!("Unexpected event {:?}", events),
562591
}
592+
check_closed_broadcast(&$curr_node, 1, true);
593+
check_added_monitors!($curr_node, 1);
563594

564595
$curr_node.node.process_pending_htlc_forwards();
565-
expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!($curr_node,
596+
expect_htlc_handling_failed_destinations!($curr_node.node.get_and_clear_pending_events(),
566597
vec![HTLCDestination::UnknownNextHop { requested_forward_scid: $failed_scid }]);
567-
check_closed_broadcast(&$curr_node, 1, true);
568-
check_added_monitors!($curr_node, 1);
569598
$curr_node.node.process_pending_htlc_forwards();
570599
},
571600
}
@@ -646,6 +675,7 @@ fn do_blinded_intercept_payment(intercept_node_fails: bool) {
646675
};
647676
nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
648677
commitment_signed_dance!(nodes[1], nodes[0], &payment_event.commitment_msg, false, true);
678+
expect_pending_htlcs_forwardable!(nodes[1]);
649679

650680
let events = nodes[1].node.get_and_clear_pending_events();
651681
assert_eq!(events.len(), 1);
@@ -923,13 +953,19 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
923953
nodes[2].node.handle_update_add_htlc(nodes[1].node.get_our_node_id(), update_add);
924954
check_added_monitors!(nodes[2], 0);
925955
do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event_1_2.commitment_msg, true, true);
956+
expect_pending_htlcs_forwardable!(nodes[2]);
957+
expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCDestination::InvalidOnion]);
958+
check_added_monitors(&nodes[2], 1);
926959
},
927960
ReceiveCheckFail::ReceiveRequirements => {
928961
let update_add = &mut payment_event_1_2.msgs[0];
929962
update_add.amount_msat -= 1;
930963
nodes[2].node.handle_update_add_htlc(nodes[1].node.get_our_node_id(), update_add);
931964
check_added_monitors!(nodes[2], 0);
932965
do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event_1_2.commitment_msg, true, true);
966+
expect_pending_htlcs_forwardable!(nodes[2]);
967+
expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCDestination::FailedPayment { payment_hash }]);
968+
check_added_monitors(&nodes[2], 1);
933969
},
934970
ReceiveCheckFail::ChannelCheck => {
935971
nodes[2].node.close_channel(&chan_id_1_2, &nodes[1].node.get_our_node_id()).unwrap();
@@ -943,6 +979,9 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
943979

944980
nodes[2].node.handle_shutdown(nodes[1].node.get_our_node_id(), &node_1_shutdown);
945981
commitment_signed_dance!(nodes[2], nodes[1], (), false, true, false, false);
982+
expect_pending_htlcs_forwardable!(nodes[2]);
983+
expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCDestination::FailedPayment { payment_hash }]);
984+
check_added_monitors(&nodes[2], 1);
946985
},
947986
ReceiveCheckFail::ProcessPendingHTLCsCheck => {
948987
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);
@@ -958,6 +997,9 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
958997
nodes[2].node.handle_update_add_htlc(nodes[1].node.get_our_node_id(), &payment_event_1_2.msgs[0]);
959998
check_added_monitors!(nodes[2], 0);
960999
do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event_1_2.commitment_msg, true, true);
1000+
expect_pending_htlcs_forwardable!(nodes[2]);
1001+
expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCDestination::FailedPayment { payment_hash }]);
1002+
check_added_monitors(&nodes[2], 1);
9611003
}
9621004
}
9631005

@@ -1158,6 +1200,12 @@ fn min_htlc() {
11581200
nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &payment_event_0_1.msgs[0]);
11591201
check_added_monitors!(nodes[1], 0);
11601202
do_commitment_signed_dance(&nodes[1], &nodes[0], &payment_event_0_1.commitment_msg, true, true);
1203+
expect_pending_htlcs_forwardable!(nodes[1]);
1204+
expect_htlc_handling_failed_destinations!(
1205+
nodes[1].node.get_and_clear_pending_events(),
1206+
&[HTLCDestination::NextHopChannel { node_id: Some(nodes[2].node.get_our_node_id()), channel_id: chan_1_2.2 }]
1207+
);
1208+
check_added_monitors(&nodes[1], 1);
11611209
let mut updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
11621210
nodes[0].node.handle_update_fail_htlc(nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
11631211
do_commitment_signed_dance(&nodes[0], &nodes[1], &updates.commitment_signed, false, false);
@@ -1344,9 +1392,11 @@ fn fails_receive_tlvs_authentication() {
13441392
let mut payment_event = SendEvent::from_event(ev);
13451393

13461394
nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
1347-
check_added_monitors!(nodes[1], 0);
13481395
do_commitment_signed_dance(&nodes[1], &nodes[0], &payment_event.commitment_msg, true, true);
1396+
expect_pending_htlcs_forwardable!(nodes[1]);
13491397
nodes[1].node.process_pending_htlc_forwards();
1398+
check_added_monitors!(nodes[1], 1);
1399+
expect_htlc_handling_failed_destinations!(nodes[1].node.get_and_clear_pending_events(), &[HTLCDestination::InvalidOnion]);
13501400

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

0 commit comments

Comments
 (0)