Skip to content

Enable decoding HTLC onions when fully committed #2933

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

Merged
merged 2 commits into from
Jan 24, 2025
Merged

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
df9f3c0
Add expected_htlc_destination argument to run_onion_failure_test
wpaulino Mar 7, 2024
d8d9dc7
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 @@ -1338,8 +1338,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 @@ -1362,8 +1362,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 @@ -1439,8 +1439,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 @@ -1474,8 +1474,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 @@ -1574,8 +1574,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 @@ -1598,8 +1598,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
10 changes: 0 additions & 10 deletions lightning/src/events/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1426,16 +1426,6 @@ pub enum Event {
/// Indicates that the HTLC was accepted, but could not be processed when or after attempting to
/// forward it.
///
/// Some scenarios where this event may be sent include:
/// * Insufficient capacity in the outbound channel
/// * While waiting to forward the HTLC, the channel it is meant to be forwarded through closes
/// * When an unknown SCID is requested for forwarding a payment.
/// * Expected MPP amount has already been reached
/// * The HTLC has timed out
///
/// 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).
///
/// # Failure Behavior and Persistence
/// This event will eventually be replayed after failures-to-handle (i.e., the event handler
/// returning `Err(ReplayEvent ())`) and will be persisted across restarts.
Expand Down
68 changes: 57 additions & 11 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 @@ -309,8 +309,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 @@ -364,18 +366,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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you confirm this has always been the case but is just being exposed now as a result of the changes? If so, it's probably best to address on its own as a follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looked into this again. So the issue is that as coded in fe65648 we'll never map the result of construct_pending_htlc_status to HTLCDestination::InvalidOnion, to do so would require checking the onion error code within and tailoring the HTLCDestination more based on that. Don't think it's worth bothering to address though as this case is quite rare (and also broken for unblinded forwards, just not tested).

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 @@ -385,6 +396,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 @@ -444,7 +466,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 @@ -521,7 +546,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 @@ -537,12 +562,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 @@ -628,6 +653,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 @@ -933,13 +959,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 @@ -953,6 +985,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 @@ -968,6 +1003,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 @@ -1168,6 +1206,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 Expand Up @@ -1497,9 +1541,11 @@ fn fails_receive_tlvs_authentication() {
let mut payment_event = SendEvent::from_event(ev);

nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
check_added_monitors!(nodes[1], 0);
do_commitment_signed_dance(&nodes[1], &nodes[0], &payment_event.commitment_msg, true, true);
expect_pending_htlcs_forwardable!(nodes[1]);
nodes[1].node.process_pending_htlc_forwards();
check_added_monitors!(nodes[1], 1);
expect_htlc_handling_failed_destinations!(nodes[1].node.get_and_clear_pending_events(), &[HTLCDestination::InvalidOnion]);

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