From 3b3a4ba0a6e60e90792bf78d2c5d9cd5c59a5ddb Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Fri, 5 Nov 2021 12:55:25 -0500 Subject: [PATCH] Rename ChannelClosed to ChannelFailure A NetworkUpdate indicating ChannelClosed actually corresponds to a channel failure as described in BOLT 4: 0x2000 (NODE): node failure (otherwise channel) Rename the enum variant to ChannelFailure and rename NetworkGraph methods close_channel_from_update and fail_node to channel_failed and node_failed, respectively. --- fuzz/src/router.rs | 2 +- lightning/src/ln/functional_test_utils.rs | 2 +- lightning/src/ln/onion_route_tests.rs | 20 ++++++++-------- lightning/src/ln/onion_utils.rs | 4 ++-- lightning/src/routing/network_graph.rs | 28 +++++++++++------------ 5 files changed, 28 insertions(+), 28 deletions(-) diff --git a/fuzz/src/router.rs b/fuzz/src/router.rs index bb6ba2c6..9f4fd512 100644 --- a/fuzz/src/router.rs +++ b/fuzz/src/router.rs @@ -196,7 +196,7 @@ pub fn do_test(data: &[u8], out: Out) { }, 4 => { let short_channel_id = slice_to_be64(get_slice!(8)); - net_graph.close_channel_from_update(short_channel_id, false); + net_graph.channel_failed(short_channel_id, false); }, _ if node_pks.is_empty() => {}, _ => { diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 8929a977..7b653c89 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1512,7 +1512,7 @@ macro_rules! expect_payment_failed_conditions { } assert_eq!(msg.contents.flags & 2, 0); }, - &Some($crate::routing::network_graph::NetworkUpdate::ChannelClosed { short_channel_id, is_permanent }) if chan_closed => { + &Some($crate::routing::network_graph::NetworkUpdate::ChannelFailure { short_channel_id, is_permanent }) if chan_closed => { if let Some(scid) = $conditions.expected_blamed_scid { assert_eq!(short_channel_id, scid); } diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index 802cd3ac..1c3203ed 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -177,8 +177,8 @@ fn run_onion_failure_test_with_fail_intercept(_name: &str, test_case: panic!("channel_update not found!"); } }, - &NetworkUpdate::ChannelClosed { ref short_channel_id, ref is_permanent } => { - if let NetworkUpdate::ChannelClosed { short_channel_id: ref expected_short_channel_id, is_permanent: ref expected_is_permanent } = expected_channel_update.unwrap() { + &NetworkUpdate::ChannelFailure { ref short_channel_id, ref is_permanent } => { + if let NetworkUpdate::ChannelFailure { short_channel_id: ref expected_short_channel_id, is_permanent: ref expected_is_permanent } = expected_channel_update.unwrap() { assert!(*short_channel_id == *expected_short_channel_id); assert!(*is_permanent == *expected_is_permanent); } else { @@ -283,7 +283,7 @@ fn test_fee_failures() { let short_channel_id = channels[0].0.contents.short_channel_id; run_onion_failure_test("fee_insufficient", 0, &nodes, &route, &payment_hash, &payment_secret, |msg| { msg.amount_msat -= 1; - }, || {}, true, Some(UPDATE|12), Some(NetworkUpdate::ChannelClosed { short_channel_id, is_permanent: true}), Some(short_channel_id)); + }, || {}, true, Some(UPDATE|12), Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent: true}), Some(short_channel_id)); // In an earlier version, we spuriously failed to forward payments if the expected feerate // changed between the channel open and the payment. @@ -343,7 +343,7 @@ fn test_onion_failure() { // describing a length-1 TLV payload, which is obviously bogus. new_payloads[0].data[0] = 1; msg.onion_routing_packet = onion_utils::construct_onion_packet_bogus_hopdata(new_payloads, onion_keys, [0; 32], &payment_hash); - }, ||{}, true, Some(PERM|22), Some(NetworkUpdate::ChannelClosed{short_channel_id, is_permanent: true}), Some(short_channel_id)); + }, ||{}, true, Some(PERM|22), Some(NetworkUpdate::ChannelFailure{short_channel_id, is_permanent: true}), Some(short_channel_id)); // final node failure let short_channel_id = channels[1].0.contents.short_channel_id; @@ -360,7 +360,7 @@ fn test_onion_failure() { // length-1 TLV payload, which is obviously bogus. new_payloads[1].data[0] = 1; msg.onion_routing_packet = onion_utils::construct_onion_packet_bogus_hopdata(new_payloads, onion_keys, [0; 32], &payment_hash); - }, ||{}, false, Some(PERM|22), Some(NetworkUpdate::ChannelClosed{short_channel_id, is_permanent: true}), Some(short_channel_id)); + }, ||{}, false, Some(PERM|22), Some(NetworkUpdate::ChannelFailure{short_channel_id, is_permanent: true}), Some(short_channel_id)); // the following three with run_onion_failure_test_with_fail_intercept() test only the origin node // receiving simulated fail messages @@ -471,7 +471,7 @@ fn test_onion_failure() { let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap(); msg.reason = onion_utils::build_first_hop_failure_packet(onion_keys[0].shared_secret.as_ref(), PERM|8, &[0;0]); // short_channel_id from the processing node - }, ||{}, true, Some(PERM|8), Some(NetworkUpdate::ChannelClosed{short_channel_id, is_permanent: true}), Some(short_channel_id)); + }, ||{}, true, Some(PERM|8), Some(NetworkUpdate::ChannelFailure{short_channel_id, is_permanent: true}), Some(short_channel_id)); let short_channel_id = channels[1].0.contents.short_channel_id; run_onion_failure_test_with_fail_intercept("required_channel_feature_missing", 100, &nodes, &route, &payment_hash, &payment_secret, |msg| { @@ -481,13 +481,13 @@ fn test_onion_failure() { let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap(); msg.reason = onion_utils::build_first_hop_failure_packet(onion_keys[0].shared_secret.as_ref(), PERM|9, &[0;0]); // short_channel_id from the processing node - }, ||{}, true, Some(PERM|9), Some(NetworkUpdate::ChannelClosed{short_channel_id, is_permanent: true}), Some(short_channel_id)); + }, ||{}, true, Some(PERM|9), Some(NetworkUpdate::ChannelFailure{short_channel_id, is_permanent: true}), Some(short_channel_id)); let mut bogus_route = route.clone(); bogus_route.paths[0][1].short_channel_id -= 1; let short_channel_id = bogus_route.paths[0][1].short_channel_id; run_onion_failure_test("unknown_next_peer", 0, &nodes, &bogus_route, &payment_hash, &payment_secret, |_| {}, ||{}, true, Some(PERM|10), - Some(NetworkUpdate::ChannelClosed{short_channel_id, is_permanent:true}), Some(short_channel_id)); + Some(NetworkUpdate::ChannelFailure{short_channel_id, is_permanent:true}), Some(short_channel_id)); let short_channel_id = channels[1].0.contents.short_channel_id; let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_counterparty_htlc_minimum_msat() - 1; @@ -511,13 +511,13 @@ fn test_onion_failure() { let short_channel_id = channels[0].0.contents.short_channel_id; run_onion_failure_test("fee_insufficient", 0, &nodes, &route, &payment_hash, &payment_secret, |msg| { msg.amount_msat -= 1; - }, || {}, true, Some(UPDATE|12), Some(NetworkUpdate::ChannelClosed { short_channel_id, is_permanent: true}), Some(short_channel_id)); + }, || {}, true, Some(UPDATE|12), Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent: true}), Some(short_channel_id)); let short_channel_id = channels[0].0.contents.short_channel_id; run_onion_failure_test("incorrect_cltv_expiry", 0, &nodes, &route, &payment_hash, &payment_secret, |msg| { // need to violate: cltv_expiry - cltv_expiry_delta >= outgoing_cltv_value msg.cltv_expiry -= 1; - }, || {}, true, Some(UPDATE|13), Some(NetworkUpdate::ChannelClosed { short_channel_id, is_permanent: true}), Some(short_channel_id)); + }, || {}, true, Some(UPDATE|13), Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent: true}), Some(short_channel_id)); let short_channel_id = channels[1].0.contents.short_channel_id; run_onion_failure_test("expiry_too_soon", 0, &nodes, &route, &payment_hash, &payment_secret, |msg| { diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 9e4ae058..02676b73 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -395,7 +395,7 @@ pub(super) fn process_onion_failure(secp_ctx: & } else if error_code & PERM == PERM { if !payment_failed { - network_update = Some(NetworkUpdate::ChannelClosed { + network_update = Some(NetworkUpdate::ChannelFailure { short_channel_id: failing_route_hop.short_channel_id, is_permanent: true, }); @@ -440,7 +440,7 @@ pub(super) fn process_onion_failure(secp_ctx: & if is_chan_update_invalid { // This probably indicates the node which forwarded // to the node in question corrupted something. - network_update = Some(NetworkUpdate::ChannelClosed { + network_update = Some(NetworkUpdate::ChannelFailure { short_channel_id: route_hop.short_channel_id, is_permanent: true, }); diff --git a/lightning/src/routing/network_graph.rs b/lightning/src/routing/network_graph.rs index 76af50bd..7cbe373f 100644 --- a/lightning/src/routing/network_graph.rs +++ b/lightning/src/routing/network_graph.rs @@ -162,17 +162,17 @@ pub enum NetworkUpdate { /// The update to apply via [`NetworkGraph::update_channel`]. msg: ChannelUpdate, }, - /// An error indicating only that a channel has been closed, which should be applied via - /// [`NetworkGraph::close_channel_from_update`]. - ChannelClosed { + /// An error indicating that a channel failed to route a payment, which should be applied via + /// [`NetworkGraph::channel_failed`]. + ChannelFailure { /// The short channel id of the closed channel. short_channel_id: u64, /// Whether the channel should be permanently removed or temporarily disabled until a new /// `channel_update` message is received. is_permanent: bool, }, - /// An error indicating only that a node has failed, which should be applied via - /// [`NetworkGraph::fail_node`]. + /// An error indicating that a node failed to route a payment, which should be applied via + /// [`NetworkGraph::node_failed`]. NodeFailure { /// The node id of the failed node. node_id: PublicKey, @@ -186,7 +186,7 @@ impl_writeable_tlv_based_enum_upgradable!(NetworkUpdate, (0, ChannelUpdateMessage) => { (0, msg, required), }, - (2, ChannelClosed) => { + (2, ChannelFailure) => { (0, short_channel_id, required), (2, is_permanent, required), }, @@ -282,15 +282,15 @@ where C::Target: chain::Access, L::Target: Logger log_debug!(self.logger, "Updating channel with channel_update from a payment failure. Channel {} is {}.", short_channel_id, status); let _ = self.network_graph.update_channel(msg, &self.secp_ctx); }, - NetworkUpdate::ChannelClosed { short_channel_id, is_permanent } => { + NetworkUpdate::ChannelFailure { short_channel_id, is_permanent } => { let action = if is_permanent { "Removing" } else { "Disabling" }; log_debug!(self.logger, "{} channel graph entry for {} due to a payment failure.", action, short_channel_id); - self.network_graph.close_channel_from_update(short_channel_id, is_permanent); + self.network_graph.channel_failed(short_channel_id, is_permanent); }, NetworkUpdate::NodeFailure { ref node_id, is_permanent } => { let action = if is_permanent { "Removing" } else { "Disabling" }; log_debug!(self.logger, "{} node graph entry for {} due to a payment failure.", action, node_id); - self.network_graph.fail_node(node_id, is_permanent); + self.network_graph.node_failed(node_id, is_permanent); }, } } @@ -1328,11 +1328,11 @@ impl NetworkGraph { self.add_channel_between_nodes(msg.short_channel_id, chan_info, utxo_value) } - /// Close a channel if a corresponding HTLC fail was sent. + /// Marks a channel in the graph as failed if a corresponding HTLC fail was sent. /// If permanent, removes a channel from the local storage. /// May cause the removal of nodes too, if this was their last channel. /// If not permanent, makes channels unavailable for routing. - pub fn close_channel_from_update(&self, short_channel_id: u64, is_permanent: bool) { + pub fn channel_failed(&self, short_channel_id: u64, is_permanent: bool) { let mut channels = self.channels.write().unwrap(); if is_permanent { if let Some(chan) = channels.remove(&short_channel_id) { @@ -1352,7 +1352,7 @@ impl NetworkGraph { } /// Marks a node in the graph as failed. - pub fn fail_node(&self, _node_id: &PublicKey, is_permanent: bool) { + pub fn node_failed(&self, _node_id: &PublicKey, is_permanent: bool) { if is_permanent { // TODO: Wholly remove the node } else { @@ -2120,7 +2120,7 @@ mod tests { rejected_by_dest: false, all_paths_failed: true, path: vec![], - network_update: Some(NetworkUpdate::ChannelClosed { + network_update: Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent: false, }), @@ -2145,7 +2145,7 @@ mod tests { rejected_by_dest: false, all_paths_failed: true, path: vec![], - network_update: Some(NetworkUpdate::ChannelClosed { + network_update: Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent: true, }), -- 2.30.2