From 9ade8eb23b1cc9ac34cef5cf7031f9fc5bb21e9e Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Wed, 13 Sep 2023 23:52:11 -0400 Subject: [PATCH 1/6] Rename onion util internal var This variable is ultimately for setting PaymentPathFailed::payment_failed_permanently, so use this name rather than flipping a bool back and forth. --- lightning/src/ln/onion_utils.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 666221b2dd4..c591010ae4d 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -507,7 +507,7 @@ pub(super) fn process_onion_failure( is_permanent: true, }); let short_channel_id = Some(route_hop.short_channel_id); - res = Some((network_update, short_channel_id, !is_from_final_node)); + res = Some((network_update, short_channel_id, is_from_final_node)); return } }; @@ -659,7 +659,7 @@ pub(super) fn process_onion_failure( short_channel_id = Some(route_hop.short_channel_id); } - res = Some((network_update, short_channel_id, !(error_code & PERM == PERM && is_from_final_node))); + res = Some((network_update, short_channel_id, error_code & PERM == PERM && is_from_final_node)); let (description, title) = errors::get_onion_error_description(error_code); if debug_field_size > 0 && err_packet.failuremsg.len() >= 4 + debug_field_size { @@ -668,9 +668,9 @@ pub(super) fn process_onion_failure( log_info!(logger, "Onion Error[from {}: {}({:#x})] {}", route_hop.pubkey, title, error_code, description); } }).expect("Route that we sent via spontaneously grew invalid keys in the middle of it?"); - if let Some((network_update, short_channel_id, payment_retryable)) = res { + if let Some((network_update, short_channel_id, payment_failed_permanently)) = res { DecodedOnionFailure { - network_update, short_channel_id, payment_retryable, + network_update, short_channel_id, payment_retryable: !payment_failed_permanently, #[cfg(test)] onion_error_code: error_code_ret, #[cfg(test)] From 61ab1f85130de9b8a4346c821b34a145692e85e6 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Wed, 20 Sep 2023 14:49:58 -0400 Subject: [PATCH 2/6] Struct-ify onion util internal result type Improves readability. --- lightning/src/ln/onion_utils.rs | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index c591010ae4d..db61f0c90d0 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -444,7 +444,14 @@ pub(super) fn process_onion_failure( } = htlc_source { (path, session_priv, first_hop_htlc_msat) } else { unreachable!() }; - let mut res = None; + + // Learnings from the HTLC failure to inform future payment retries and scoring. + struct FailureLearnings { + network_update: Option, + short_channel_id: Option, + payment_failed_permanently: bool, + } + let mut res: Option = None; let mut htlc_msat = *first_hop_htlc_msat; let mut error_code_ret = None; let mut error_packet_ret = None; @@ -507,7 +514,9 @@ pub(super) fn process_onion_failure( is_permanent: true, }); let short_channel_id = Some(route_hop.short_channel_id); - res = Some((network_update, short_channel_id, is_from_final_node)); + res = Some(FailureLearnings { + network_update, short_channel_id, payment_failed_permanently: is_from_final_node + }); return } }; @@ -659,7 +668,10 @@ pub(super) fn process_onion_failure( short_channel_id = Some(route_hop.short_channel_id); } - res = Some((network_update, short_channel_id, error_code & PERM == PERM && is_from_final_node)); + res = Some(FailureLearnings { + network_update, short_channel_id, + payment_failed_permanently: error_code & PERM == PERM && is_from_final_node + }); let (description, title) = errors::get_onion_error_description(error_code); if debug_field_size > 0 && err_packet.failuremsg.len() >= 4 + debug_field_size { @@ -668,7 +680,9 @@ pub(super) fn process_onion_failure( log_info!(logger, "Onion Error[from {}: {}({:#x})] {}", route_hop.pubkey, title, error_code, description); } }).expect("Route that we sent via spontaneously grew invalid keys in the middle of it?"); - if let Some((network_update, short_channel_id, payment_failed_permanently)) = res { + if let Some(FailureLearnings { + network_update, short_channel_id, payment_failed_permanently + }) = res { DecodedOnionFailure { network_update, short_channel_id, payment_retryable: !payment_failed_permanently, #[cfg(test)] From 9c41f129c0da084e33f6c367b9ab58df3b0fd586 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 14 Sep 2023 11:33:01 -0400 Subject: [PATCH 3/6] DecodedOnionFailure::payment_retryable -> ::payment_failed_permanently Our ultimate goal with this field is to set PaymentPathFailed::payment_failed_permanently, so use this name rather than flipping a bool back and forth across methods. --- lightning/src/ln/onion_utils.rs | 8 ++++---- lightning/src/ln/outbound_payment.rs | 13 +++++++------ 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index db61f0c90d0..d6bf117e814 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -426,7 +426,7 @@ pub(super) fn build_first_hop_failure_packet(shared_secret: &[u8], failure_type: pub(crate) struct DecodedOnionFailure { pub(crate) network_update: Option, pub(crate) short_channel_id: Option, - pub(crate) payment_retryable: bool, + pub(crate) payment_failed_permanently: bool, #[cfg(test)] pub(crate) onion_error_code: Option, #[cfg(test)] @@ -684,7 +684,7 @@ pub(super) fn process_onion_failure( network_update, short_channel_id, payment_failed_permanently }) = res { DecodedOnionFailure { - network_update, short_channel_id, payment_retryable: !payment_failed_permanently, + network_update, short_channel_id, payment_failed_permanently, #[cfg(test)] onion_error_code: error_code_ret, #[cfg(test)] @@ -694,7 +694,7 @@ pub(super) fn process_onion_failure( // only not set either packet unparseable or hmac does not match with any // payment not retryable only when garbage is from the final node DecodedOnionFailure { - network_update: None, short_channel_id: None, payment_retryable: !is_from_final_node, + network_update: None, short_channel_id: None, payment_failed_permanently: is_from_final_node, #[cfg(test)] onion_error_code: None, #[cfg(test)] @@ -838,7 +838,7 @@ impl HTLCFailReason { if let &HTLCSource::OutboundRoute { ref path, .. } = htlc_source { DecodedOnionFailure { network_update: None, - payment_retryable: true, + payment_failed_permanently: false, short_channel_id: Some(path.hops[0].short_channel_id), #[cfg(test)] onion_error_code: Some(*failure_code), diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 5ea772e5d4f..a4d5c3a389e 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -1484,10 +1484,11 @@ impl OutboundPayments { ) -> bool where L::Target: Logger { #[cfg(test)] let DecodedOnionFailure { - network_update, short_channel_id, payment_retryable, onion_error_code, onion_error_data + network_update, short_channel_id, payment_failed_permanently, onion_error_code, + onion_error_data } = onion_error.decode_onion_failure(secp_ctx, logger, &source); #[cfg(not(test))] - let DecodedOnionFailure { network_update, short_channel_id, payment_retryable } = + let DecodedOnionFailure { network_update, short_channel_id, payment_failed_permanently } = onion_error.decode_onion_failure(secp_ctx, logger, &source); let payment_is_probe = payment_is_probe(payment_hash, &payment_id, probing_cookie_secret); @@ -1528,8 +1529,8 @@ impl OutboundPayments { payment.get_mut().insert_previously_failed_scid(scid); } - if payment_is_probe || !is_retryable_now || !payment_retryable { - let reason = if !payment_retryable { + if payment_is_probe || !is_retryable_now || payment_failed_permanently { + let reason = if payment_failed_permanently { PaymentFailureReason::RecipientRejected } else { PaymentFailureReason::RetriesExhausted @@ -1559,7 +1560,7 @@ impl OutboundPayments { let path_failure = { if payment_is_probe { - if !payment_retryable { + if payment_failed_permanently { events::Event::ProbeSuccessful { payment_id: *payment_id, payment_hash: payment_hash.clone(), @@ -1583,7 +1584,7 @@ impl OutboundPayments { events::Event::PaymentPathFailed { payment_id: Some(*payment_id), payment_hash: payment_hash.clone(), - payment_failed_permanently: !payment_retryable, + payment_failed_permanently, failure: events::PathFailure::OnPath { network_update }, path: path.clone(), short_channel_id, From 5f5119fa3dd151276052827ea0b02fa20cf926e7 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 14 Sep 2023 11:41:35 -0400 Subject: [PATCH 4/6] Correct DecodedOnionFailure when processing we-are-intro-node path We don't support sending to paths where we are the intro node yet, but may as well set the failure correctly now. --- lightning/src/ln/onion_utils.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index d6bf117e814..4b5ccfe92dd 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -474,7 +474,9 @@ pub(super) fn process_onion_failure( // Got an error from within a blinded route. error_code_ret = Some(BADONION | PERM | 24); // invalid_onion_blinding error_packet_ret = Some(vec![0; 32]); - is_from_final_node = false; + res = Some(FailureLearnings { + network_update: None, short_channel_id: None, payment_failed_permanently: false + }); return }, }; From 9df61dc0b8d74f9323d0a25bf7b7ca2f9ab48f1c Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 14 Sep 2023 11:46:02 -0400 Subject: [PATCH 5/6] Fix PaymentPathFailed::payment_failed_permanently on blinded path fail Previously this value would be incorrectly set to true because we wouldn't account for blinded hops when determining if we were processing the last hop's failure packet. --- lightning/src/ln/onion_utils.rs | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 4b5ccfe92dd..f3f8608604d 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -481,6 +481,26 @@ pub(super) fn process_onion_failure( }, }; + // The failing hop includes either the inbound channel to the recipient or the outbound channel + // from the current hop (i.e., the next hop's inbound channel). + let num_blinded_hops = path.blinded_tail.as_ref().map_or(0, |bt| bt.hops.len()); + // For 1-hop blinded paths, the final `path.hops` entry is the recipient. + is_from_final_node = route_hop_idx + 1 == path.hops.len() && num_blinded_hops <= 1; + let failing_route_hop = if is_from_final_node { route_hop } else { + match path.hops.get(route_hop_idx + 1) { + Some(hop) => hop, + None => { + // The failing hop is within a multi-hop blinded path. + error_code_ret = Some(BADONION | PERM | 24); // invalid_onion_blinding + error_packet_ret = Some(vec![0; 32]); + res = Some(FailureLearnings { + network_update: None, short_channel_id: None, payment_failed_permanently: false + }); + return + } + } + }; + let amt_to_forward = htlc_msat - route_hop.fee_msat; htlc_msat = amt_to_forward; @@ -492,11 +512,6 @@ pub(super) fn process_onion_failure( chacha.process(&packet_decrypted, &mut decryption_tmp[..]); packet_decrypted = decryption_tmp; - // The failing hop includes either the inbound channel to the recipient or the outbound channel - // from the current hop (i.e., the next hop's inbound channel). - is_from_final_node = route_hop_idx + 1 == path.hops.len(); - let failing_route_hop = if is_from_final_node { route_hop } else { &path.hops[route_hop_idx + 1] }; - let err_packet = match msgs::DecodedOnionErrorPacket::read(&mut Cursor::new(&packet_decrypted)) { Ok(p) => p, Err(_) => return From 6299f7d14f4a6b47e133c6abaa3a05ed50177b0f Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 15 Sep 2023 16:55:12 -0400 Subject: [PATCH 6/6] Blame outbound channel on UPDATE onion failure with 0-len update We've run into this several times in the wild, likely due to https://github.com/ElementsProject/lightning/issues/6200 wherein a node on the path will error with 0x1000 but not provide a channel update (a spec violation). Previously, we would blame the inbound edge even though the buggy peer wanted us to blame the outbound edge. Since this issue seems to be recurring and our blaming the inbound edge is causing us to punish innocent channels, trust the peer that the outbound edge is the one to blame. --- lightning/src/ln/onion_route_tests.rs | 30 +++++++++++++++++++++++++-- lightning/src/ln/onion_utils.rs | 3 ++- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index 3731c31c5b4..c124360d1ee 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -645,8 +645,34 @@ fn test_onion_failure() { }, || nodes[2].node.fail_htlc_backwards(&payment_hash), false, None, Some(NetworkUpdate::NodeFailure { node_id: route.paths[0].hops[1].pubkey, is_permanent: true }), Some(channels[1].0.contents.short_channel_id)); - run_onion_failure_test_with_fail_intercept("0-length channel update in UPDATE onion failure", 200, &nodes, - &route, &payment_hash, &payment_secret, |_msg| {}, |msg| { + run_onion_failure_test_with_fail_intercept("0-length channel update in intermediate node UPDATE onion failure", + 100, &nodes, &route, &payment_hash, &payment_secret, |msg| { + msg.amount_msat -= 1; + }, |msg| { + let session_priv = SecretKey::from_slice(&[3; 32]).unwrap(); + let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap(); + let mut decoded_err_packet = msgs::DecodedOnionErrorPacket { + failuremsg: vec![ + 0x10, 0x7, // UPDATE|7 + 0x0, 0x0 // 0-len channel update + ], + pad: vec![0; 255 - 4 /* 4-byte error message */], + hmac: [0; 32], + }; + let um = onion_utils::gen_um_from_shared_secret(&onion_keys[0].shared_secret.as_ref()); + let mut hmac = HmacEngine::::new(&um); + hmac.input(&decoded_err_packet.encode()[32..]); + decoded_err_packet.hmac = Hmac::from_engine(hmac).into_inner(); + msg.reason = onion_utils::encrypt_failure_packet( + &onion_keys[0].shared_secret.as_ref(), &decoded_err_packet.encode()[..]) + }, || {}, true, Some(0x1000|7), + Some(NetworkUpdate::ChannelFailure { + short_channel_id: channels[1].0.contents.short_channel_id, + is_permanent: false, + }), + Some(channels[1].0.contents.short_channel_id)); + run_onion_failure_test_with_fail_intercept("0-length channel update in final node UPDATE onion failure", + 200, &nodes, &route, &payment_hash, &payment_secret, |_msg| {}, |msg| { let session_priv = SecretKey::from_slice(&[3; 32]).unwrap(); let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap(); let mut decoded_err_packet = msgs::DecodedOnionErrorPacket { diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index f3f8608604d..390826e4f3d 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -641,8 +641,9 @@ pub(super) fn process_onion_failure( } else { // The node in question intentionally encoded a 0-length channel update. This is // likely due to https://github.com/ElementsProject/lightning/issues/6200. + short_channel_id = Some(failing_route_hop.short_channel_id); network_update = Some(NetworkUpdate::ChannelFailure { - short_channel_id: route_hop.short_channel_id, + short_channel_id: failing_route_hop.short_channel_id, is_permanent: false, }); }