Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable decoding HTLC onions when fully committed #2933

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Enable decoding HTLC onions when fully committed #2933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
19ef855
Add expected_htlc_destination argument to run_onion_failure_test
wpaulino Mar 7, 2024
183fd66
Enable decoding new incoming HTLC onions when fully committed
wpaulino Mar 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions fuzz/src/full_stack.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1344,8 +1344,8 @@ mod tests {
// end of update_add_htlc from 0 to 1 via client and mac
ext_from_hex("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff ab00000000000000000000000000000000000000000000000000000000000000 03000000000000000000000000000000", &mut test);

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

// inbound read from peer id 0 of len 18
ext_from_hex("030012", &mut test);
Expand All @@ -1368,8 +1368,8 @@ mod tests {

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

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

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

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

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

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

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

// inbound read from peer id 0 of len 18
ext_from_hex("030012", &mut test);
Expand All @@ -1604,8 +1604,8 @@ mod tests {

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

// connect a block with one transaction of len 125
Expand Down
2 changes: 2 additions & 0 deletions lightning/src/events/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1361,6 +1361,8 @@ pub enum Event {
/// * When an unknown SCID is requested for forwarding a payment.
/// * Expected MPP amount has already been reached
/// * The HTLC has timed out
/// * The HTLC failed to meet the forwarding requirements (i.e. insufficient fees paid, or a
/// CLTV that is too soon)
///
/// This event, however, does not get generated if an HTLC fails to meet the forwarding
/// requirements (i.e. insufficient fees paid, or a CLTV that is too soon).
Expand Down
64 changes: 54 additions & 10 deletions lightning/src/ln/blinded_payment_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,8 +290,10 @@ fn do_forward_checks_failure(check: ForwardCheckFail, intro_fails: bool) {
// We need the session priv to construct a bogus onion packet later.
*nodes[0].keys_manager.override_random_bytes.lock().unwrap() = Some([3; 32]);
create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0);
let chan_upd_1_2 = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 0).0.contents;
let chan_upd_2_3 = create_announced_chan_between_nodes_with_value(&nodes, 2, 3, 1_000_000, 0).0.contents;
let chan_1_2 = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 0);
let chan_upd_1_2 = chan_1_2.0.contents;
let chan_2_3 = create_announced_chan_between_nodes_with_value(&nodes, 2, 3, 1_000_000, 0);
let chan_upd_2_3 = chan_2_3.0.contents;

let amt_msat = 5000;
let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[3], Some(amt_msat), None);
Expand Down Expand Up @@ -345,18 +347,27 @@ fn do_forward_checks_failure(check: ForwardCheckFail, intro_fails: bool) {
check_added_monitors!(nodes[1], 0);
do_commitment_signed_dance(&nodes[1], &nodes[0], &updates_0_1.commitment_signed, true, true);

expect_pending_htlcs_forwardable!(nodes[1]);
check_added_monitors!(nodes[1], 1);

if intro_fails {
let mut updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
nodes[0].node.handle_update_fail_htlc(nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
do_commitment_signed_dance(&nodes[0], &nodes[1], &updates.commitment_signed, false, false);
let failed_destination = match check {
ForwardCheckFail::InboundOnionCheck => HTLCDestination::InvalidOnion,
ForwardCheckFail::ForwardPayloadEncodedAsReceive => HTLCDestination::FailedPayment { payment_hash },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it should be ::InvalidOnion...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think so too, but the error we get from processing doesn't come out as malformed from construct_pending_htlc_status.

We get a PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC)) with the reason:

Got blinded non final data with an HMAC of 0

I believe this has always been the case and is just being surfaced by this PR now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior of the intro node returning update_fail rather than malformed there aligns with the route blinding spec; intro nodes always error in the same way regardless of the "real" error. So it seems the issue is that real error isn't being bubbled up to be accurately included in the event, only the blinded one. That said, this case seems quite rare so doesn't seem worth addressing.

ForwardCheckFail::OutboundChannelCheck =>
HTLCDestination::NextHopChannel { node_id: Some(nodes[2].node.get_our_node_id()), channel_id: chan_1_2.2 },
};
expect_htlc_handling_failed_destinations!(
nodes[1].node.get_and_clear_pending_events(), &[failed_destination.clone()]
);
expect_payment_failed_conditions(&nodes[0], payment_hash, false,
PaymentFailedConditions::new().expected_htlc_error_data(INVALID_ONION_BLINDING, &[0; 32]));
return
}

expect_pending_htlcs_forwardable!(nodes[1]);
check_added_monitors!(nodes[1], 1);

let mut updates_1_2 = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
let mut update_add = &mut updates_1_2.update_add_htlcs[0];

Expand All @@ -366,6 +377,17 @@ fn do_forward_checks_failure(check: ForwardCheckFail, intro_fails: bool) {
check_added_monitors!(nodes[2], 0);
do_commitment_signed_dance(&nodes[2], &nodes[1], &updates_1_2.commitment_signed, true, true);

expect_pending_htlcs_forwardable!(nodes[2]);
let failed_destination = match check {
ForwardCheckFail::InboundOnionCheck|ForwardCheckFail::ForwardPayloadEncodedAsReceive => HTLCDestination::InvalidOnion,
ForwardCheckFail::OutboundChannelCheck =>
HTLCDestination::NextHopChannel { node_id: Some(nodes[3].node.get_our_node_id()), channel_id: chan_2_3.2 },
};
expect_htlc_handling_failed_destinations!(
nodes[2].node.get_and_clear_pending_events(), &[failed_destination.clone()]
);
check_added_monitors!(nodes[2], 1);

let mut updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
let update_malformed = &mut updates.update_fail_malformed_htlcs[0];
assert_eq!(update_malformed.failure_code, INVALID_ONION_BLINDING);
Expand Down Expand Up @@ -425,7 +447,10 @@ fn failed_backwards_to_intro_node() {
nodes[2].node.handle_update_add_htlc(nodes[1].node.get_our_node_id(), &payment_event.msgs[0]);
check_added_monitors!(nodes[2], 0);
do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event.commitment_msg, true, true);
nodes[2].node.process_pending_htlc_forwards();

expect_pending_htlcs_forwardable!(nodes[2]);
expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCDestination::InvalidOnion]);
check_added_monitors(&nodes[2], 1);

let mut updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
let mut update_malformed = &mut updates.update_fail_malformed_htlcs[0];
Expand Down Expand Up @@ -502,7 +527,7 @@ fn do_forward_fail_in_process_pending_htlc_fwds(check: ProcessPendingHTLCsCheck,
// intro node will error backwards.
$curr_node.node.peer_disconnected($next_node.node.get_our_node_id());
expect_pending_htlcs_forwardable!($curr_node);
expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!($curr_node,
expect_htlc_handling_failed_destinations!($curr_node.node.get_and_clear_pending_events(),
vec![HTLCDestination::NextHopChannel { node_id: Some($next_node.node.get_our_node_id()), channel_id: $failed_chan_id }]);
},
ProcessPendingHTLCsCheck::FwdChannelClosed => {
Expand All @@ -518,12 +543,12 @@ fn do_forward_fail_in_process_pending_htlc_fwds(check: ProcessPendingHTLCsCheck,
crate::events::Event::ChannelClosed { .. } => {},
_ => panic!("Unexpected event {:?}", events),
}
check_closed_broadcast(&$curr_node, 1, true);
check_added_monitors!($curr_node, 1);

$curr_node.node.process_pending_htlc_forwards();
expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!($curr_node,
expect_htlc_handling_failed_destinations!($curr_node.node.get_and_clear_pending_events(),
vec![HTLCDestination::UnknownNextHop { requested_forward_scid: $failed_scid }]);
check_closed_broadcast(&$curr_node, 1, true);
check_added_monitors!($curr_node, 1);
$curr_node.node.process_pending_htlc_forwards();
},
}
Expand Down Expand Up @@ -609,6 +634,7 @@ fn do_blinded_intercept_payment(intercept_node_fails: bool) {
};
nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
commitment_signed_dance!(nodes[1], nodes[0], &payment_event.commitment_msg, false, true);
expect_pending_htlcs_forwardable!(nodes[1]);

let events = nodes[1].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
Expand Down Expand Up @@ -914,13 +940,19 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
nodes[2].node.handle_update_add_htlc(nodes[1].node.get_our_node_id(), update_add);
check_added_monitors!(nodes[2], 0);
do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event_1_2.commitment_msg, true, true);
expect_pending_htlcs_forwardable!(nodes[2]);
expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCDestination::InvalidOnion]);
check_added_monitors(&nodes[2], 1);
},
ReceiveCheckFail::ReceiveRequirements => {
let update_add = &mut payment_event_1_2.msgs[0];
update_add.amount_msat -= 1;
nodes[2].node.handle_update_add_htlc(nodes[1].node.get_our_node_id(), update_add);
check_added_monitors!(nodes[2], 0);
do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event_1_2.commitment_msg, true, true);
expect_pending_htlcs_forwardable!(nodes[2]);
expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCDestination::FailedPayment { payment_hash }]);
check_added_monitors(&nodes[2], 1);
},
ReceiveCheckFail::ChannelCheck => {
nodes[2].node.close_channel(&chan_id_1_2, &nodes[1].node.get_our_node_id()).unwrap();
Expand All @@ -934,6 +966,9 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {

nodes[2].node.handle_shutdown(nodes[1].node.get_our_node_id(), &node_1_shutdown);
commitment_signed_dance!(nodes[2], nodes[1], (), false, true, false, false);
expect_pending_htlcs_forwardable!(nodes[2]);
expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCDestination::FailedPayment { payment_hash }]);
check_added_monitors(&nodes[2], 1);
},
ReceiveCheckFail::ProcessPendingHTLCsCheck => {
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);
Expand All @@ -949,6 +984,9 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
nodes[2].node.handle_update_add_htlc(nodes[1].node.get_our_node_id(), &payment_event_1_2.msgs[0]);
check_added_monitors!(nodes[2], 0);
do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event_1_2.commitment_msg, true, true);
expect_pending_htlcs_forwardable!(nodes[2]);
expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCDestination::FailedPayment { payment_hash }]);
check_added_monitors(&nodes[2], 1);
}
}

Expand Down Expand Up @@ -1149,6 +1187,12 @@ fn min_htlc() {
nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &payment_event_0_1.msgs[0]);
check_added_monitors!(nodes[1], 0);
do_commitment_signed_dance(&nodes[1], &nodes[0], &payment_event_0_1.commitment_msg, true, true);
expect_pending_htlcs_forwardable!(nodes[1]);
expect_htlc_handling_failed_destinations!(
nodes[1].node.get_and_clear_pending_events(),
&[HTLCDestination::NextHopChannel { node_id: Some(nodes[2].node.get_our_node_id()), channel_id: chan_1_2.2 }]
);
check_added_monitors(&nodes[1], 1);
let mut updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
nodes[0].node.handle_update_fail_htlc(nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
do_commitment_signed_dance(&nodes[0], &nodes[1], &updates.commitment_signed, false, false);
Expand Down
Loading
Loading