From: Jeffrey Czyz Date: Thu, 12 Aug 2021 20:30:53 +0000 (-0500) Subject: Refactor PaymentFailureNetworkUpdate event X-Git-Tag: v0.0.101~20^2~3 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=eff9a47075445a3d4f3bf15ef68723c2ea6348e8;p=rust-lightning Refactor PaymentFailureNetworkUpdate event MessageSendEvent::PaymentFailureNetworkUpdate served as a hack to pass an HTLCFailChannelUpdate from ChannelManager to NetGraphMsgHandler via PeerManager. Instead, remove the event entirely and move the contained data (renamed NetworkUpdate) to Event::PaymentFailed to be processed by an event handler. --- diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 88826d655..6e726a947 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -594,7 +594,6 @@ pub fn do_test(data: &[u8], out: Out) { }, events::MessageSendEvent::SendFundingLocked { .. } => continue, events::MessageSendEvent::SendAnnouncementSignatures { .. } => continue, - events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => continue, events::MessageSendEvent::SendChannelUpdate { ref node_id, ref msg } => { assert_eq!(msg.contents.flags & 2, 0); // The disable bit must never be set! if Some(*node_id) == expect_drop_id { panic!("peer_disconnected should drop msgs bound for the disconnected peer"); } @@ -727,10 +726,6 @@ pub fn do_test(data: &[u8], out: Out) { events::MessageSendEvent::SendAnnouncementSignatures { .. } => { // Can be generated as a reestablish response }, - events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => { - // Can be generated due to a payment forward being rejected due to a - // channel having previously failed a monitor update - }, events::MessageSendEvent::SendChannelUpdate { ref msg, .. } => { // When we reconnect we will resend a channel_update to make sure our // counterparty has the latest parameters for receiving payments @@ -769,7 +764,6 @@ pub fn do_test(data: &[u8], out: Out) { events::MessageSendEvent::SendChannelReestablish { .. } => {}, events::MessageSendEvent::SendFundingLocked { .. } => {}, events::MessageSendEvent::SendAnnouncementSignatures { .. } => {}, - events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => {}, events::MessageSendEvent::SendChannelUpdate { ref msg, .. } => { assert_eq!(msg.contents.flags & 2, 0); // The disable bit must never be set! }, @@ -787,7 +781,6 @@ pub fn do_test(data: &[u8], out: Out) { events::MessageSendEvent::SendChannelReestablish { .. } => {}, events::MessageSendEvent::SendFundingLocked { .. } => {}, events::MessageSendEvent::SendAnnouncementSignatures { .. } => {}, - events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => {}, events::MessageSendEvent::SendChannelUpdate { ref msg, .. } => { assert_eq!(msg.contents.flags & 2, 0); // The disable bit must never be set! }, diff --git a/lightning-net-tokio/src/lib.rs b/lightning-net-tokio/src/lib.rs index bf411211f..13a4c0d93 100644 --- a/lightning-net-tokio/src/lib.rs +++ b/lightning-net-tokio/src/lib.rs @@ -492,7 +492,6 @@ mod tests { fn handle_node_announcement(&self, _msg: &NodeAnnouncement) -> Result { Ok(false) } fn handle_channel_announcement(&self, _msg: &ChannelAnnouncement) -> Result { Ok(false) } fn handle_channel_update(&self, _msg: &ChannelUpdate) -> Result { Ok(false) } - fn handle_htlc_fail_channel_update(&self, _update: &HTLCFailChannelUpdate) { } fn get_next_channel_announcements(&self, _starting_point: u64, _batch_amount: u8) -> Vec<(ChannelAnnouncement, Option, Option)> { Vec::new() } fn get_next_node_announcements(&self, _starting_point: Option<&PublicKey>, _batch_amount: u8) -> Vec { Vec::new() } fn sync_routing_table(&self, _their_node_id: &PublicKey, _init_msg: &Init) { } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 35535c058..b772c5e9a 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2822,6 +2822,7 @@ impl ChannelMana events::Event::PaymentFailed { payment_hash, rejected_by_dest: false, + network_update: None, #[cfg(test)] error_code: None, #[cfg(test)] @@ -2866,23 +2867,17 @@ impl ChannelMana match &onion_error { &HTLCFailReason::LightningError { ref err } => { #[cfg(test)] - let (channel_update, payment_retryable, onion_error_code, onion_error_data) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone()); + let (network_update, payment_retryable, onion_error_code, onion_error_data) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone()); #[cfg(not(test))] - let (channel_update, payment_retryable, _, _) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone()); + let (network_update, payment_retryable, _, _) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone()); // TODO: If we decided to blame ourselves (or one of our channels) in // process_onion_failure we should close that channel as it implies our // next-hop is needlessly blaming us! - if let Some(update) = channel_update { - self.channel_state.lock().unwrap().pending_msg_events.push( - events::MessageSendEvent::PaymentFailureNetworkUpdate { - update, - } - ); - } self.pending_events.lock().unwrap().push( events::Event::PaymentFailed { payment_hash: payment_hash.clone(), rejected_by_dest: !payment_retryable, + network_update, #[cfg(test)] error_code: onion_error_code, #[cfg(test)] @@ -2897,7 +2892,7 @@ impl ChannelMana ref data, .. } => { // we get a fail_malformed_htlc from the first hop - // TODO: We'd like to generate a PaymentFailureNetworkUpdate for temporary + // TODO: We'd like to generate a NetworkUpdate for temporary // failures here, but that would be insufficient as get_route // generally ignores its view of our own channels as we provide them via // ChannelDetails. @@ -2907,6 +2902,7 @@ impl ChannelMana events::Event::PaymentFailed { payment_hash: payment_hash.clone(), rejected_by_dest: path.len() == 1, + network_update: None, #[cfg(test)] error_code: Some(*failure_code), #[cfg(test)] @@ -4670,7 +4666,6 @@ impl &events::MessageSendEvent::BroadcastChannelUpdate { .. } => true, &events::MessageSendEvent::SendChannelUpdate { ref node_id, .. } => node_id != counterparty_node_id, &events::MessageSendEvent::HandleError { ref node_id, .. } => node_id != counterparty_node_id, - &events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => true, &events::MessageSendEvent::SendChannelRangeQuery { .. } => false, &events::MessageSendEvent::SendShortIdsQuery { .. } => false, &events::MessageSendEvent::SendReplyChannelRange { .. } => false, diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 4e7ca676d..96a6ae86d 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1036,22 +1036,27 @@ macro_rules! expect_payment_forwarded { } #[cfg(test)] -macro_rules! expect_payment_failure_chan_update { - ($node: expr, $scid: expr, $chan_closed: expr) => { - let events = $node.node.get_and_clear_pending_msg_events(); +macro_rules! expect_payment_failed_with_update { + ($node: expr, $expected_payment_hash: expr, $rejected_by_dest: expr, $scid: expr, $chan_closed: expr) => { + let events = $node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match events[0] { - MessageSendEvent::PaymentFailureNetworkUpdate { ref update } => { - match update { - &HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg } if !$chan_closed => { + Event::PaymentFailed { ref payment_hash, rejected_by_dest, ref network_update, ref error_code, ref error_data } => { + assert_eq!(*payment_hash, $expected_payment_hash, "unexpected payment_hash"); + assert_eq!(rejected_by_dest, $rejected_by_dest, "unexpected rejected_by_dest value"); + assert!(error_code.is_some(), "expected error_code.is_some() = true"); + assert!(error_data.is_some(), "expected error_data.is_some() = true"); + match network_update { + &Some(NetworkUpdate::ChannelUpdateMessage { ref msg }) if !$chan_closed => { assert_eq!(msg.contents.short_channel_id, $scid); assert_eq!(msg.contents.flags & 2, 0); }, - &HTLCFailChannelUpdate::ChannelClosed { short_channel_id, is_permanent } if $chan_closed => { + &Some(NetworkUpdate::ChannelClosed { short_channel_id, is_permanent }) if $chan_closed => { assert_eq!(short_channel_id, $scid); assert!(is_permanent); }, - _ => panic!("Unexpected update type"), + Some(_) => panic!("Unexpected update type"), + None => panic!("Expected update"), } }, _ => panic!("Unexpected event"), @@ -1065,7 +1070,7 @@ macro_rules! expect_payment_failed { let events = $node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match events[0] { - Event::PaymentFailed { ref payment_hash, rejected_by_dest, ref error_code, ref error_data } => { + Event::PaymentFailed { ref payment_hash, rejected_by_dest, network_update: _, ref error_code, ref error_data } => { assert_eq!(*payment_hash, $expected_payment_hash, "unexpected payment_hash"); assert_eq!(rejected_by_dest, $rejected_by_dest, "unexpected rejected_by_dest value"); assert!(error_code.is_some(), "expected error_code.is_some() = true"); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index eb3d3cb3b..e1a57cd7c 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -24,10 +24,10 @@ use ln::channel::{Channel, ChannelError}; use ln::{chan_utils, onion_utils}; use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT; use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route, get_keysend_route}; -use routing::network_graph::RoutingFees; +use routing::network_graph::{NetworkUpdate, RoutingFees}; use ln::features::{ChannelFeatures, InitFeatures, InvoiceFeatures, NodeFeatures}; use ln::msgs; -use ln::msgs::{ChannelMessageHandler,RoutingMessageHandler,HTLCFailChannelUpdate, ErrorAction}; +use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, ErrorAction}; use util::enforcing_trait_impls::EnforcingSigner; use util::{byte_utils, test_utils}; use util::events::{Event, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose}; @@ -1050,8 +1050,7 @@ fn holding_cell_htlc_counting() { nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_fail_updates.update_fail_htlcs[0]); commitment_signed_dance!(nodes[0], nodes[1], bs_fail_updates.commitment_signed, false, true); - expect_payment_failure_chan_update!(nodes[0], chan_2.0.contents.short_channel_id, false); - expect_payment_failed!(nodes[0], payment_hash_2, false); + expect_payment_failed_with_update!(nodes[0], payment_hash_2, false, chan_2.0.contents.short_channel_id, false); // Now forward all the pending HTLCs and claim them back nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &initial_payment_event.msgs[0]); @@ -2833,8 +2832,7 @@ fn test_simple_commitment_revoked_fail_backward() { nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &update_fail_htlcs[0]); commitment_signed_dance!(nodes[0], nodes[1], commitment_signed, false, true); - expect_payment_failure_chan_update!(nodes[0], chan_2.0.contents.short_channel_id, true); - expect_payment_failed!(nodes[0], payment_hash, false); + expect_payment_failed_with_update!(nodes[0], payment_hash, false, chan_2.0.contents.short_channel_id, true); }, _ => panic!("Unexpected event"), } @@ -3020,33 +3018,30 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use commitment_signed_dance!(nodes[0], nodes[1], commitment_signed, false, true); - let events = nodes[0].node.get_and_clear_pending_msg_events(); - // If we delivered B's RAA we got an unknown preimage error, not something - // that we should update our routing table for. - assert_eq!(events.len(), if deliver_bs_raa { 2 } else { 3 }); - for event in events { - match event { - MessageSendEvent::PaymentFailureNetworkUpdate { .. } => {}, - _ => panic!("Unexpected event"), - } - } let events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 3); match events[0] { - Event::PaymentFailed { ref payment_hash, .. } => { + Event::PaymentFailed { ref payment_hash, rejected_by_dest: _, ref network_update, .. } => { assert!(failed_htlcs.insert(payment_hash.0)); + // If we delivered B's RAA we got an unknown preimage error, not something + // that we should update our routing table for. + if !deliver_bs_raa { + assert!(network_update.is_some()); + } }, _ => panic!("Unexpected event"), } match events[1] { - Event::PaymentFailed { ref payment_hash, .. } => { + Event::PaymentFailed { ref payment_hash, rejected_by_dest: _, ref network_update, .. } => { assert!(failed_htlcs.insert(payment_hash.0)); + assert!(network_update.is_some()); }, _ => panic!("Unexpected event"), } match events[2] { - Event::PaymentFailed { ref payment_hash, .. } => { + Event::PaymentFailed { ref payment_hash, rejected_by_dest: _, ref network_update, .. } => { assert!(failed_htlcs.insert(payment_hash.0)); + assert!(network_update.is_some()); }, _ => panic!("Unexpected event"), } @@ -4001,8 +3996,7 @@ fn do_test_holding_cell_htlc_add_timeouts(forwarded_htlc: bool) { }, _ => unreachable!(), } - expect_payment_failed!(nodes[0], second_payment_hash, false); - expect_payment_failure_chan_update!(nodes[0], chan_2.0.contents.short_channel_id, false); + expect_payment_failed_with_update!(nodes[0], second_payment_hash, false, chan_2.0.contents.short_channel_id, false); } else { expect_payment_failed!(nodes[1], second_payment_hash, true); } @@ -5106,9 +5100,8 @@ fn test_duplicate_payment_hash_one_failure_one_success() { assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); { commitment_signed_dance!(nodes[0], nodes[1], &htlc_updates.commitment_signed, false, true); - expect_payment_failure_chan_update!(nodes[0], chan_2.0.contents.short_channel_id, true); } - expect_payment_failed!(nodes[0], duplicate_payment_hash, false); + expect_payment_failed_with_update!(nodes[0], duplicate_payment_hash, false, chan_2.0.contents.short_channel_id, true); // Solve 2nd HTLC by broadcasting on B's chain HTLC-Success Tx from C // Note that the fee paid is effectively double as the HTLC value (including the nodes[1] fee @@ -5375,14 +5368,18 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno let as_events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(as_events.len(), if announce_latest { 5 } else { 3 }); let mut as_failds = HashSet::new(); + let mut as_updates = 0; for event in as_events.iter() { - if let &Event::PaymentFailed { ref payment_hash, ref rejected_by_dest, .. } = event { + if let &Event::PaymentFailed { ref payment_hash, ref rejected_by_dest, ref network_update, .. } = event { assert!(as_failds.insert(*payment_hash)); if *payment_hash != payment_hash_2 { assert_eq!(*rejected_by_dest, deliver_last_raa); } else { assert!(!rejected_by_dest); } + if network_update.is_some() { + as_updates += 1; + } } else { panic!("Unexpected event"); } } assert!(as_failds.contains(&payment_hash_1)); @@ -5396,14 +5393,18 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno let bs_events = nodes[1].node.get_and_clear_pending_events(); assert_eq!(bs_events.len(), if announce_latest { 4 } else { 3 }); let mut bs_failds = HashSet::new(); + let mut bs_updates = 0; for event in bs_events.iter() { - if let &Event::PaymentFailed { ref payment_hash, ref rejected_by_dest, .. } = event { + if let &Event::PaymentFailed { ref payment_hash, ref rejected_by_dest, ref network_update, .. } = event { assert!(bs_failds.insert(*payment_hash)); if *payment_hash != payment_hash_1 && *payment_hash != payment_hash_5 { assert_eq!(*rejected_by_dest, deliver_last_raa); } else { assert!(!rejected_by_dest); } + if network_update.is_some() { + bs_updates += 1; + } } else { panic!("Unexpected event"); } } assert!(bs_failds.contains(&payment_hash_1)); @@ -5414,20 +5415,11 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno assert!(bs_failds.contains(&payment_hash_5)); // For each HTLC which was not failed-back by normal process (ie deliver_last_raa), we should - // get a PaymentFailureNetworkUpdate. A should have gotten 4 HTLCs which were failed-back due - // to unknown-preimage-etc, B should have gotten 2. Thus, in the - // announce_latest && deliver_last_raa case, we should have 5-4=1 and 4-2=2 - // PaymentFailureNetworkUpdates. - let as_msg_events = nodes[0].node.get_and_clear_pending_msg_events(); - assert_eq!(as_msg_events.len(), if deliver_last_raa { 1 } else if !announce_latest { 3 } else { 5 }); - let bs_msg_events = nodes[1].node.get_and_clear_pending_msg_events(); - assert_eq!(bs_msg_events.len(), if deliver_last_raa { 2 } else if !announce_latest { 3 } else { 4 }); - for event in as_msg_events.iter().chain(bs_msg_events.iter()) { - match event { - &MessageSendEvent::PaymentFailureNetworkUpdate { .. } => {}, - _ => panic!("Unexpected event"), - } - } + // get a NetworkUpdate. A should have gotten 4 HTLCs which were failed-back due to + // unknown-preimage-etc, B should have gotten 2. Thus, in the + // announce_latest && deliver_last_raa case, we should have 5-4=1 and 4-2=2 NetworkUpdates. + assert_eq!(as_updates, if deliver_last_raa { 1 } else if !announce_latest { 3 } else { 5 }); + assert_eq!(bs_updates, if deliver_last_raa { 2 } else if !announce_latest { 3 } else { 4 }); } #[test] @@ -5921,9 +5913,10 @@ fn test_fail_holding_cell_htlc_upon_free() { let events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match &events[0] { - &Event::PaymentFailed { ref payment_hash, ref rejected_by_dest, ref error_code, ref error_data } => { + &Event::PaymentFailed { ref payment_hash, ref rejected_by_dest, ref network_update, ref error_code, ref error_data } => { assert_eq!(our_payment_hash.clone(), *payment_hash); assert_eq!(*rejected_by_dest, false); + assert_eq!(*network_update, None); assert_eq!(*error_code, None); assert_eq!(*error_data, None); }, @@ -6006,9 +5999,10 @@ fn test_free_and_fail_holding_cell_htlcs() { let events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match &events[0] { - &Event::PaymentFailed { ref payment_hash, ref rejected_by_dest, ref error_code, ref error_data } => { + &Event::PaymentFailed { ref payment_hash, ref rejected_by_dest, ref network_update, ref error_code, ref error_data } => { assert_eq!(payment_hash_2.clone(), *payment_hash); assert_eq!(*rejected_by_dest, false); + assert_eq!(*network_update, None); assert_eq!(*error_code, None); assert_eq!(*error_data, None); }, @@ -6189,8 +6183,7 @@ fn test_fail_holding_cell_htlc_upon_free_multihop() { _ => panic!("Unexpected event"), }; nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &raa); - expect_payment_failure_chan_update!(nodes[0], chan_1_2.0.contents.short_channel_id, false); - expect_payment_failed!(nodes[0], our_payment_hash, false); + expect_payment_failed_with_update!(nodes[0], our_payment_hash, false, chan_1_2.0.contents.short_channel_id, false); check_added_monitors!(nodes[0], 1); } @@ -7455,8 +7448,7 @@ fn test_priv_forwarding_rejection() { nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &htlc_fail_updates.update_fail_htlcs[0]); commitment_signed_dance!(nodes[0], nodes[1], htlc_fail_updates.commitment_signed, true, true); - expect_payment_failed!(nodes[0], our_payment_hash, false); - expect_payment_failure_chan_update!(nodes[0], nodes[2].node.list_channels()[0].short_channel_id.unwrap(), true); + expect_payment_failed_with_update!(nodes[0], our_payment_hash, false, nodes[2].node.list_channels()[0].short_channel_id.unwrap(), true); // Now disconnect nodes[1] from its peers and restart with accept_forwards_to_priv_channels set // to true. Sadly there is currently no way to change it at runtime. @@ -9012,8 +9004,7 @@ fn do_test_tx_confirmed_skipping_blocks_immediate_broadcast(test_height_before_t assert!(updates.update_fee.is_none()); nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]); commitment_signed_dance!(nodes[0], nodes[1], updates.commitment_signed, true, true); - expect_payment_failed!(nodes[0], payment_hash, false); - expect_payment_failure_chan_update!(nodes[0], chan_announce.contents.short_channel_id, true); + expect_payment_failed_with_update!(nodes[0], payment_hash, false, chan_announce.contents.short_channel_id, true); } } diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 8fdf28e57..2d3c8f59f 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -12,8 +12,9 @@ use chain::channelmonitor::ANTI_REORG_DELAY; use ln::{PaymentPreimage, PaymentHash}; use ln::features::InitFeatures; -use ln::msgs::{ChannelMessageHandler, HTLCFailChannelUpdate, ErrorAction}; +use ln::msgs::{ChannelMessageHandler, ErrorAction}; use util::events::{Event, MessageSendEvent, MessageSendEventsProvider}; +use routing::network_graph::NetworkUpdate; use routing::router::get_route; use bitcoin::hashes::sha256::Hash as Sha256; @@ -76,6 +77,5 @@ fn chanmon_fail_from_stale_commitment() { nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_updates.update_fail_htlcs[0]); commitment_signed_dance!(nodes[0], nodes[1], fail_updates.commitment_signed, true, true); - expect_payment_failed!(nodes[0], payment_hash, false); - expect_payment_failure_chan_update!(nodes[0], update_a.contents.short_channel_id, true); + expect_payment_failed_with_update!(nodes[0], payment_hash, false, update_a.contents.short_channel_id, true); } diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 5b49f43b1..7c2647916 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -745,35 +745,6 @@ pub struct CommitmentUpdate { pub commitment_signed: CommitmentSigned, } -/// The information we received from a peer along the route of a payment we originated. This is -/// returned by ChannelMessageHandler::handle_update_fail_htlc to be passed into -/// RoutingMessageHandler::handle_htlc_fail_channel_update to update our network map. -#[derive(Clone, Debug, PartialEq)] -pub enum HTLCFailChannelUpdate { - /// We received an error which included a full ChannelUpdate message. - ChannelUpdateMessage { - /// The unwrapped message we received - msg: ChannelUpdate, - }, - /// We received an error which indicated only that a channel has been closed - ChannelClosed { - /// The short_channel_id which has now closed. - short_channel_id: u64, - /// when this true, this channel should be permanently removed from the - /// consideration. Otherwise, this channel can be restored as new channel_update is received - is_permanent: bool, - }, - /// We received an error which indicated only that a node has failed - NodeFailure { - /// The node_id that has failed. - node_id: PublicKey, - /// when this true, node should be permanently removed from the - /// consideration. Otherwise, the channels connected to this node can be - /// restored as new channel_update is received - is_permanent: bool, - } -} - /// Messages could have optional fields to use with extended features /// As we wish to serialize these differently from Options (Options get a tag byte, but /// OptionalFeild simply gets Present if there are enough bytes to read into it), we have a @@ -868,8 +839,6 @@ pub trait RoutingMessageHandler : MessageSendEventsProvider { /// Handle an incoming channel_update message, returning true if it should be forwarded on, /// false or returning an Err otherwise. fn handle_channel_update(&self, msg: &ChannelUpdate) -> Result; - /// Handle some updates to the route graph that we learned due to an outbound failed payment. - fn handle_htlc_fail_channel_update(&self, update: &HTLCFailChannelUpdate); /// Gets a subset of the channel announcements and updates required to dump our routing table /// to a remote node, starting at the short_channel_id indicated by starting_point and /// including the batch_amount entries immediately higher in numerical value than starting_point. diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index 1006d9da1..bcc05fdb0 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -16,9 +16,10 @@ use ln::{PaymentPreimage, PaymentHash, PaymentSecret}; use ln::channelmanager::{HTLCForwardInfo, CLTV_FAR_FAR_AWAY}; use ln::onion_utils; use routing::router::{Route, get_route}; +use routing::network_graph::NetworkUpdate; use ln::features::{InitFeatures, InvoiceFeatures}; use ln::msgs; -use ln::msgs::{ChannelMessageHandler, ChannelUpdate, HTLCFailChannelUpdate, OptionalField}; +use ln::msgs::{ChannelMessageHandler, ChannelUpdate, OptionalField}; use util::test_utils; use util::events::{Event, MessageSendEvent, MessageSendEventsProvider}; use util::ser::{Writeable, Writer}; @@ -39,7 +40,7 @@ use core::default::Default; use ln::functional_test_utils::*; -fn run_onion_failure_test(_name: &str, test_case: u8, nodes: &Vec, route: &Route, payment_hash: &PaymentHash, payment_secret: &PaymentSecret, callback_msg: F1, callback_node: F2, expected_retryable: bool, expected_error_code: Option, expected_channel_update: Option) +fn run_onion_failure_test(_name: &str, test_case: u8, nodes: &Vec, route: &Route, payment_hash: &PaymentHash, payment_secret: &PaymentSecret, callback_msg: F1, callback_node: F2, expected_retryable: bool, expected_error_code: Option, expected_channel_update: Option) where F1: for <'a> FnMut(&'a mut msgs::UpdateAddHTLC), F2: FnMut(), { @@ -53,7 +54,7 @@ fn run_onion_failure_test(_name: &str, test_case: u8, nodes: &Vec, // 3: final node fails backward (but tamper onion payloads from node0) // 100: trigger error in the intermediate node and tamper returning fail_htlc // 200: trigger error in the final node and tamper returning fail_htlc -fn run_onion_failure_test_with_fail_intercept(_name: &str, test_case: u8, nodes: &Vec, route: &Route, payment_hash: &PaymentHash, payment_secret: &PaymentSecret, mut callback_msg: F1, mut callback_fail: F2, mut callback_node: F3, expected_retryable: bool, expected_error_code: Option, expected_channel_update: Option) +fn run_onion_failure_test_with_fail_intercept(_name: &str, test_case: u8, nodes: &Vec, route: &Route, payment_hash: &PaymentHash, payment_secret: &PaymentSecret, mut callback_msg: F1, mut callback_fail: F2, mut callback_node: F3, expected_retryable: bool, expected_error_code: Option, expected_channel_update: Option) where F1: for <'a> FnMut(&'a mut msgs::UpdateAddHTLC), F2: for <'a> FnMut(&'a mut msgs::UpdateFailHTLC), F3: FnMut(), @@ -162,34 +163,27 @@ fn run_onion_failure_test_with_fail_intercept(_name: &str, test_case: let events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); - if let &Event::PaymentFailed { payment_hash:_, ref rejected_by_dest, ref error_code, error_data: _ } = &events[0] { + if let &Event::PaymentFailed { payment_hash:_, ref rejected_by_dest, ref network_update, ref error_code, error_data: _ } = &events[0] { assert_eq!(*rejected_by_dest, !expected_retryable); assert_eq!(*error_code, expected_error_code); - } else { - panic!("Uexpected event"); - } - - let events = nodes[0].node.get_and_clear_pending_msg_events(); - if expected_channel_update.is_some() { - assert_eq!(events.len(), 1); - match events[0] { - MessageSendEvent::PaymentFailureNetworkUpdate { ref update } => { - match update { - &HTLCFailChannelUpdate::ChannelUpdateMessage { .. } => { - if let HTLCFailChannelUpdate::ChannelUpdateMessage { .. } = expected_channel_update.unwrap() {} else { + if expected_channel_update.is_some() { + match network_update { + Some(update) => match update { + &NetworkUpdate::ChannelUpdateMessage { .. } => { + if let NetworkUpdate::ChannelUpdateMessage { .. } = expected_channel_update.unwrap() {} else { panic!("channel_update not found!"); } }, - &HTLCFailChannelUpdate::ChannelClosed { ref short_channel_id, ref is_permanent } => { - if let HTLCFailChannelUpdate::ChannelClosed { short_channel_id: ref expected_short_channel_id, is_permanent: ref expected_is_permanent } = expected_channel_update.unwrap() { + &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() { assert!(*short_channel_id == *expected_short_channel_id); assert!(*is_permanent == *expected_is_permanent); } else { panic!("Unexpected message event"); } }, - &HTLCFailChannelUpdate::NodeFailure { ref node_id, ref is_permanent } => { - if let HTLCFailChannelUpdate::NodeFailure { node_id: ref expected_node_id, is_permanent: ref expected_is_permanent } = expected_channel_update.unwrap() { + &NetworkUpdate::NodeFailure { ref node_id, ref is_permanent } => { + if let NetworkUpdate::NodeFailure { node_id: ref expected_node_id, is_permanent: ref expected_is_permanent } = expected_channel_update.unwrap() { assert!(*node_id == *expected_node_id); assert!(*is_permanent == *expected_is_permanent); } else { @@ -197,11 +191,13 @@ fn run_onion_failure_test_with_fail_intercept(_name: &str, test_case: } }, } - }, - _ => panic!("Unexpected message event"), + None => panic!("Expected channel update"), + } + } else { + assert!(network_update.is_none()); } } else { - assert_eq!(events.len(), 0); + panic!("Unexpected event"); } } @@ -275,7 +271,7 @@ fn test_fee_failures() { let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]); run_onion_failure_test("fee_insufficient", 0, &nodes, &route, &payment_hash, &payment_secret, |msg| { msg.amount_msat -= 1; - }, || {}, true, Some(UPDATE|12), Some(msgs::HTLCFailChannelUpdate::ChannelClosed { short_channel_id: channels[0].0.contents.short_channel_id, is_permanent: true})); + }, || {}, true, Some(UPDATE|12), Some(NetworkUpdate::ChannelClosed { short_channel_id: channels[0].0.contents.short_channel_id, is_permanent: true})); // In an earlier version, we spuriously failed to forward payments if the expected feerate // changed between the channel open and the payment. @@ -336,7 +332,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(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true}));//XXX incremented channels idx here + }, ||{}, true, Some(PERM|22), Some(NetworkUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true})); // final node failure run_onion_failure_test("invalid_realm", 3, &nodes, &route, &payment_hash, &payment_secret, |msg| { @@ -352,7 +348,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(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true})); + }, ||{}, false, Some(PERM|22), Some(NetworkUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true})); // the following three with run_onion_failure_test_with_fail_intercept() test only the origin node // receiving simulated fail messages @@ -365,7 +361,7 @@ fn test_onion_failure() { 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(); msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[0].shared_secret[..], NODE|2, &[0;0]); - }, ||{}, true, Some(NODE|2), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.paths[0][0].pubkey, is_permanent: false})); + }, ||{}, true, Some(NODE|2), Some(NetworkUpdate::NodeFailure{node_id: route.paths[0][0].pubkey, is_permanent: false})); // final node failure run_onion_failure_test_with_fail_intercept("temporary_node_failure", 200, &nodes, &route, &payment_hash, &payment_secret, |_msg| {}, |msg| { @@ -375,7 +371,7 @@ fn test_onion_failure() { msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[1].shared_secret[..], NODE|2, &[0;0]); }, ||{ nodes[2].node.fail_htlc_backwards(&payment_hash); - }, true, Some(NODE|2), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.paths[0][1].pubkey, is_permanent: false})); + }, true, Some(NODE|2), Some(NetworkUpdate::NodeFailure{node_id: route.paths[0][1].pubkey, is_permanent: false})); let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]); // intermediate node failure @@ -385,7 +381,7 @@ fn test_onion_failure() { 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(); msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[0].shared_secret[..], PERM|NODE|2, &[0;0]); - }, ||{}, true, Some(PERM|NODE|2), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.paths[0][0].pubkey, is_permanent: true})); + }, ||{}, true, Some(PERM|NODE|2), Some(NetworkUpdate::NodeFailure{node_id: route.paths[0][0].pubkey, is_permanent: true})); // final node failure run_onion_failure_test_with_fail_intercept("permanent_node_failure", 200, &nodes, &route, &payment_hash, &payment_secret, |_msg| {}, |msg| { @@ -394,7 +390,7 @@ fn test_onion_failure() { msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[1].shared_secret[..], PERM|NODE|2, &[0;0]); }, ||{ nodes[2].node.fail_htlc_backwards(&payment_hash); - }, false, Some(PERM|NODE|2), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.paths[0][1].pubkey, is_permanent: true})); + }, false, Some(PERM|NODE|2), Some(NetworkUpdate::NodeFailure{node_id: route.paths[0][1].pubkey, is_permanent: true})); let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]); // intermediate node failure @@ -406,7 +402,7 @@ fn test_onion_failure() { msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[0].shared_secret[..], PERM|NODE|3, &[0;0]); }, ||{ nodes[2].node.fail_htlc_backwards(&payment_hash); - }, true, Some(PERM|NODE|3), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.paths[0][0].pubkey, is_permanent: true})); + }, true, Some(PERM|NODE|3), Some(NetworkUpdate::NodeFailure{node_id: route.paths[0][0].pubkey, is_permanent: true})); // final node failure run_onion_failure_test_with_fail_intercept("required_node_feature_missing", 200, &nodes, &route, &payment_hash, &payment_secret, |_msg| {}, |msg| { @@ -415,7 +411,7 @@ fn test_onion_failure() { msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[1].shared_secret[..], PERM|NODE|3, &[0;0]); }, ||{ nodes[2].node.fail_htlc_backwards(&payment_hash); - }, false, Some(PERM|NODE|3), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.paths[0][1].pubkey, is_permanent: true})); + }, false, Some(PERM|NODE|3), Some(NetworkUpdate::NodeFailure{node_id: route.paths[0][1].pubkey, is_permanent: true})); let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]); run_onion_failure_test("invalid_onion_version", 0, &nodes, &route, &payment_hash, &payment_secret, |msg| { msg.onion_routing_packet.version = 1; }, ||{}, true, @@ -433,7 +429,7 @@ fn test_onion_failure() { 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(); msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[0].shared_secret[..], UPDATE|7, &ChannelUpdate::dummy().encode_with_len()[..]); - }, ||{}, true, Some(UPDATE|7), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()})); + }, ||{}, true, Some(UPDATE|7), Some(NetworkUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()})); run_onion_failure_test_with_fail_intercept("permanent_channel_failure", 100, &nodes, &route, &payment_hash, &payment_secret, |msg| { msg.amount_msat -= 1; @@ -442,7 +438,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[..], PERM|8, &[0;0]); // short_channel_id from the processing node - }, ||{}, true, Some(PERM|8), Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true})); + }, ||{}, true, Some(PERM|8), Some(NetworkUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true})); run_onion_failure_test_with_fail_intercept("required_channel_feature_missing", 100, &nodes, &route, &payment_hash, &payment_secret, |msg| { msg.amount_msat -= 1; @@ -451,18 +447,18 @@ 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[..], PERM|9, &[0;0]); // short_channel_id from the processing node - }, ||{}, true, Some(PERM|9), Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true})); + }, ||{}, true, Some(PERM|9), Some(NetworkUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true})); let mut bogus_route = route.clone(); bogus_route.paths[0][1].short_channel_id -= 1; run_onion_failure_test("unknown_next_peer", 0, &nodes, &bogus_route, &payment_hash, &payment_secret, |_| {}, ||{}, true, Some(PERM|10), - Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: bogus_route.paths[0][1].short_channel_id, is_permanent:true})); + Some(NetworkUpdate::ChannelClosed{short_channel_id: bogus_route.paths[0][1].short_channel_id, is_permanent:true})); let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_counterparty_htlc_minimum_msat() - 1; let mut bogus_route = route.clone(); let route_len = bogus_route.paths[0].len(); bogus_route.paths[0][route_len-1].fee_msat = amt_to_forward; - run_onion_failure_test("amount_below_minimum", 0, &nodes, &bogus_route, &payment_hash, &payment_secret, |_| {}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()})); + run_onion_failure_test("amount_below_minimum", 0, &nodes, &bogus_route, &payment_hash, &payment_secret, |_| {}, ||{}, true, Some(UPDATE|11), Some(NetworkUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()})); // Test a positive test-case with one extra msat, meeting the minimum. bogus_route.paths[0][route_len-1].fee_msat = amt_to_forward + 1; @@ -473,19 +469,19 @@ fn test_onion_failure() { //invalid channel_update cases. run_onion_failure_test("fee_insufficient", 0, &nodes, &route, &payment_hash, &payment_secret, |msg| { msg.amount_msat -= 1; - }, || {}, true, Some(UPDATE|12), Some(msgs::HTLCFailChannelUpdate::ChannelClosed { short_channel_id: channels[0].0.contents.short_channel_id, is_permanent: true})); + }, || {}, true, Some(UPDATE|12), Some(NetworkUpdate::ChannelClosed { short_channel_id: channels[0].0.contents.short_channel_id, is_permanent: true})); 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(msgs::HTLCFailChannelUpdate::ChannelClosed { short_channel_id: channels[0].0.contents.short_channel_id, is_permanent: true})); + }, || {}, true, Some(UPDATE|13), Some(NetworkUpdate::ChannelClosed { short_channel_id: channels[0].0.contents.short_channel_id, is_permanent: true})); run_onion_failure_test("expiry_too_soon", 0, &nodes, &route, &payment_hash, &payment_secret, |msg| { let height = msg.cltv_expiry - CLTV_CLAIM_BUFFER - LATENCY_GRACE_PERIOD_BLOCKS + 1; connect_blocks(&nodes[0], height - nodes[0].best_block_info().1); connect_blocks(&nodes[1], height - nodes[1].best_block_info().1); connect_blocks(&nodes[2], height - nodes[2].best_block_info().1); - }, ||{}, true, Some(UPDATE|14), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()})); + }, ||{}, true, Some(UPDATE|14), Some(NetworkUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()})); run_onion_failure_test("unknown_payment_hash", 2, &nodes, &route, &payment_hash, &payment_secret, |_| {}, || { nodes[2].node.fail_htlc_backwards(&payment_hash); @@ -528,7 +524,7 @@ fn test_onion_failure() { // disconnect event to the channel between nodes[1] ~ nodes[2] nodes[1].node.peer_disconnected(&nodes[2].node.get_our_node_id(), false); nodes[2].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false); - }, true, Some(UPDATE|20), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()})); + }, true, Some(UPDATE|20), Some(NetworkUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()})); reconnect_nodes(&nodes[1], &nodes[2], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false)); run_onion_failure_test("expiry_too_far", 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 4886168dd..f6c62cb83 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -10,6 +10,7 @@ use ln::{PaymentHash, PaymentPreimage, PaymentSecret}; use ln::channelmanager::HTLCSource; use ln::msgs; +use routing::network_graph::NetworkUpdate; use routing::router::RouteHop; use util::chacha20::ChaCha20; use util::errors::{self, APIError}; @@ -330,7 +331,7 @@ pub(super) fn build_first_hop_failure_packet(shared_secret: &[u8], failure_type: /// OutboundRoute). /// Returns update, a boolean indicating that the payment itself failed, and the error code. #[inline] -pub(super) fn process_onion_failure(secp_ctx: &Secp256k1, logger: &L, htlc_source: &HTLCSource, mut packet_decrypted: Vec) -> (Option, bool, Option, Option>) where L::Target: Logger { +pub(super) fn process_onion_failure(secp_ctx: &Secp256k1, logger: &L, htlc_source: &HTLCSource, mut packet_decrypted: Vec) -> (Option, bool, Option, Option>) where L::Target: Logger { if let &HTLCSource::OutboundRoute { ref path, ref session_priv, ref first_hop_htlc_msat } = htlc_source { let mut res = None; let mut htlc_msat = *first_hop_htlc_msat; @@ -382,13 +383,13 @@ pub(super) fn process_onion_failure(secp_ctx: & } && is_from_final_node) // PERM bit observed below even this error is from the intermediate nodes || error_code == 21; // Special case error 21 as the Route object is bogus, TODO: Maybe fail the node if the CLTV was reasonable? - let mut fail_channel_update = None; + let mut network_update = None; if error_code & NODE == NODE { - fail_channel_update = Some(msgs::HTLCFailChannelUpdate::NodeFailure { node_id: route_hop.pubkey, is_permanent: error_code & PERM == PERM }); + network_update = Some(NetworkUpdate::NodeFailure { node_id: route_hop.pubkey, is_permanent: error_code & PERM == PERM }); } else if error_code & PERM == PERM { - fail_channel_update = if payment_failed {None} else {Some(msgs::HTLCFailChannelUpdate::ChannelClosed { + network_update = if payment_failed { None } else { Some(NetworkUpdate::ChannelClosed { short_channel_id: path[next_route_hop_ix - if next_route_hop_ix == path.len() { 1 } else { 0 }].short_channel_id, is_permanent: true, })}; @@ -412,25 +413,25 @@ pub(super) fn process_onion_failure(secp_ctx: & 20 => chan_update.contents.flags & 2 == 0, _ => false, // unknown error code; take channel_update as valid }; - fail_channel_update = if is_chan_update_invalid { + network_update = if is_chan_update_invalid { // This probably indicates the node which forwarded // to the node in question corrupted something. - Some(msgs::HTLCFailChannelUpdate::ChannelClosed { + Some(NetworkUpdate::ChannelClosed { short_channel_id: route_hop.short_channel_id, is_permanent: true, }) } else { - Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { + Some(NetworkUpdate::ChannelUpdateMessage { msg: chan_update, }) }; } } } - if fail_channel_update.is_none() { + if network_update.is_none() { // They provided an UPDATE which was obviously bogus, not worth // trying to relay through them anymore. - fail_channel_update = Some(msgs::HTLCFailChannelUpdate::NodeFailure { + network_update = Some(NetworkUpdate::NodeFailure { node_id: route_hop.pubkey, is_permanent: true, }); @@ -439,7 +440,7 @@ pub(super) fn process_onion_failure(secp_ctx: & // We can't understand their error messages and they failed to // forward...they probably can't understand our forwards so its // really not worth trying any further. - fail_channel_update = Some(msgs::HTLCFailChannelUpdate::NodeFailure { + network_update = Some(NetworkUpdate::NodeFailure { node_id: route_hop.pubkey, is_permanent: true, }); @@ -448,7 +449,7 @@ pub(super) fn process_onion_failure(secp_ctx: & // TODO: Here (and a few other places) we assume that BADONION errors // are always "sourced" from the node previous to the one which failed // to decode the onion. - res = Some((fail_channel_update, !(error_code & PERM == PERM && is_from_final_node))); + res = Some((network_update, !(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 { @@ -460,7 +461,7 @@ pub(super) fn process_onion_failure(secp_ctx: & } else { // Useless packet that we can't use but it passed HMAC, so it // definitely came from the peer in question - res = Some((Some(msgs::HTLCFailChannelUpdate::NodeFailure { + res = Some((Some(NetworkUpdate::NodeFailure { node_id: route_hop.pubkey, is_permanent: true, }), !is_from_final_node)); diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index 34bcda3a0..60d7a8750 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/lightning/src/ln/peer_handler.rs @@ -66,7 +66,6 @@ impl RoutingMessageHandler for IgnoringMessageHandler { fn handle_node_announcement(&self, _msg: &msgs::NodeAnnouncement) -> Result { Ok(false) } fn handle_channel_announcement(&self, _msg: &msgs::ChannelAnnouncement) -> Result { Ok(false) } fn handle_channel_update(&self, _msg: &msgs::ChannelUpdate) -> Result { Ok(false) } - fn handle_htlc_fail_channel_update(&self, _update: &msgs::HTLCFailChannelUpdate) {} fn get_next_channel_announcements(&self, _starting_point: u64, _batch_amount: u8) -> Vec<(msgs::ChannelAnnouncement, Option, Option)> { Vec::new() } fn get_next_node_announcements(&self, _starting_point: Option<&PublicKey>, _batch_amount: u8) -> Vec { Vec::new() } @@ -1318,9 +1317,6 @@ impl P let peer = get_peer_for_forwarding!(node_id); peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg))); }, - MessageSendEvent::PaymentFailureNetworkUpdate { ref update } => { - self.message_handler.route_handler.handle_htlc_fail_channel_update(update); - }, MessageSendEvent::HandleError { ref node_id, ref action } => { match *action { msgs::ErrorAction::DisconnectPeer { ref msg } => { diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index bbdb5bfac..0db30b817 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/lightning/src/ln/reorg_tests.rs @@ -14,7 +14,8 @@ use chain::transaction::OutPoint; use chain::{Confirm, Watch}; use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs}; use ln::features::InitFeatures; -use ln::msgs::{ChannelMessageHandler, ErrorAction, HTLCFailChannelUpdate}; +use ln::msgs::{ChannelMessageHandler, ErrorAction}; +use routing::network_graph::NetworkUpdate; use util::enforcing_trait_impls::EnforcingSigner; use util::events::{Event, MessageSendEvent, MessageSendEventsProvider}; use util::test_utils; @@ -163,12 +164,7 @@ fn do_test_onchain_htlc_reorg(local_commitment: bool, claim: bool) { if claim { expect_payment_sent!(nodes[0], our_payment_preimage); } else { - let events = nodes[0].node.get_and_clear_pending_msg_events(); - assert_eq!(events.len(), 1); - if let MessageSendEvent::PaymentFailureNetworkUpdate { update: HTLCFailChannelUpdate::ChannelClosed { ref is_permanent, .. } } = events[0] { - assert!(is_permanent); - } else { panic!("Unexpected event!"); } - expect_payment_failed!(nodes[0], our_payment_hash, false); + expect_payment_failed_with_update!(nodes[0], our_payment_hash, false, chan_2.0.contents.short_channel_id, true); } } diff --git a/lightning/src/ln/shutdown_tests.rs b/lightning/src/ln/shutdown_tests.rs index c1140ac43..26d39fbad 100644 --- a/lightning/src/ln/shutdown_tests.rs +++ b/lightning/src/ln/shutdown_tests.rs @@ -13,6 +13,7 @@ use chain::keysinterface::KeysInterface; use chain::transaction::OutPoint; use ln::{PaymentPreimage, PaymentHash}; use ln::channelmanager::PaymentSendFailure; +use routing::network_graph::NetworkUpdate; use routing::router::get_route; use ln::features::{InitFeatures, InvoiceFeatures}; use ln::msgs; @@ -192,18 +193,11 @@ fn htlc_fail_async_shutdown() { nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates_2.update_fail_htlcs[0]); commitment_signed_dance!(nodes[0], nodes[1], updates_2.commitment_signed, false, true); - expect_payment_failed!(nodes[0], our_payment_hash, false); + expect_payment_failed_with_update!(nodes[0], our_payment_hash, false, chan_2.0.contents.short_channel_id, true); let msg_events = nodes[0].node.get_and_clear_pending_msg_events(); - assert_eq!(msg_events.len(), 2); - match msg_events[0] { - MessageSendEvent::PaymentFailureNetworkUpdate { update: msgs::HTLCFailChannelUpdate::ChannelClosed { short_channel_id, is_permanent }} => { - assert_eq!(short_channel_id, chan_2.0.contents.short_channel_id); - assert!(is_permanent); - }, - _ => panic!("Unexpected event"), - } - let node_0_closing_signed = match msg_events[1] { + assert_eq!(msg_events.len(), 1); + let node_0_closing_signed = match msg_events[0] { MessageSendEvent::SendClosingSigned { ref node_id, ref msg } => { assert_eq!(*node_id, nodes[1].node.get_our_node_id()); (*msg).clone() diff --git a/lightning/src/routing/network_graph.rs b/lightning/src/routing/network_graph.rs index 61e94e049..1a149b3bd 100644 --- a/lightning/src/routing/network_graph.rs +++ b/lightning/src/routing/network_graph.rs @@ -64,6 +64,52 @@ pub struct ReadOnlyNetworkGraph<'a> { nodes: RwLockReadGuard<'a, BTreeMap>, } +/// Update to the [`NetworkGraph`] based on payment failure information conveyed via the Onion +/// return packet by a node along the route. See [BOLT #4] for details. +/// +/// [BOLT #4]: https://github.com/lightningnetwork/lightning-rfc/blob/master/04-onion-routing.md +#[derive(Clone, Debug, PartialEq)] +pub enum NetworkUpdate { + /// An error indicating a `channel_update` messages should be applied via + /// [`NetworkGraph::update_channel`]. + ChannelUpdateMessage { + /// 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 { + /// 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`]. + NodeFailure { + /// The node id of the failed node. + node_id: PublicKey, + /// Whether the node should be permanently removed from consideration or can be restored + /// when a new `channel_update` message is received. + is_permanent: bool, + } +} + +impl_writeable_tlv_based_enum_upgradable!(NetworkUpdate, + (0, ChannelUpdateMessage) => { + (0, msg, required), + }, + (2, ChannelClosed) => { + (0, short_channel_id, required), + (2, is_permanent, required), + }, + (4, NodeFailure) => { + (0, node_id, required), + (2, is_permanent, required), + }, +); + /// Receives and validates network updates from peers, /// stores authentic and relevant data as a network graph. /// This network graph is then used for routing payments. @@ -152,24 +198,6 @@ impl RoutingMessageHandler for NetGraphMsgHandler wh Ok(msg.contents.excess_data.len() <= MAX_EXCESS_BYTES_FOR_RELAY) } - fn handle_htlc_fail_channel_update(&self, update: &msgs::HTLCFailChannelUpdate) { - match update { - &msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg } => { - let chan_enabled = msg.contents.flags & (1 << 1) != (1 << 1); - log_debug!(self.logger, "Updating channel with channel_update from a payment failure. Channel {} is {}abled.", msg.contents.short_channel_id, if chan_enabled { "en" } else { "dis" }); - let _ = self.network_graph.update_channel(msg, &self.secp_ctx); - }, - &msgs::HTLCFailChannelUpdate::ChannelClosed { short_channel_id, is_permanent } => { - log_debug!(self.logger, "{} channel graph entry for {} due to a payment failure.", if is_permanent { "Removing" } else { "Disabling" }, short_channel_id); - self.network_graph.close_channel_from_update(short_channel_id, is_permanent); - }, - &msgs::HTLCFailChannelUpdate::NodeFailure { ref node_id, is_permanent } => { - log_debug!(self.logger, "{} node graph entry for {} due to a payment failure.", if is_permanent { "Removing" } else { "Disabling" }, node_id); - self.network_graph.fail_node(node_id, is_permanent); - }, - } - } - fn handle_channel_update(&self, msg: &msgs::ChannelUpdate) -> Result { self.network_graph.update_channel(msg, &self.secp_ctx)?; Ok(msg.contents.excess_data.len() <= MAX_EXCESS_BYTES_FOR_RELAY) @@ -898,7 +926,8 @@ impl NetworkGraph { } } - fn fail_node(&self, _node_id: &PublicKey, is_permanent: bool) { + /// Marks a node in the graph as failed. + pub fn fail_node(&self, _node_id: &PublicKey, is_permanent: bool) { if is_permanent { // TODO: Wholly remove the node } else { @@ -1090,7 +1119,7 @@ mod tests { use ln::features::{ChannelFeatures, InitFeatures, NodeFeatures}; use routing::network_graph::{NetGraphMsgHandler, NetworkGraph, MAX_EXCESS_BYTES_FOR_RELAY}; use ln::msgs::{Init, OptionalField, RoutingMessageHandler, UnsignedNodeAnnouncement, NodeAnnouncement, - UnsignedChannelAnnouncement, ChannelAnnouncement, UnsignedChannelUpdate, ChannelUpdate, HTLCFailChannelUpdate, + UnsignedChannelAnnouncement, ChannelAnnouncement, UnsignedChannelUpdate, ChannelUpdate, ReplyChannelRange, ReplyShortChannelIdsEnd, QueryChannelRange, QueryShortChannelIds, MAX_VALUE_MSAT}; use util::test_utils; use util::logger::Logger; @@ -1671,12 +1700,7 @@ mod tests { }; } - let channel_close_msg = HTLCFailChannelUpdate::ChannelClosed { - short_channel_id, - is_permanent: false - }; - - net_graph_msg_handler.handle_htlc_fail_channel_update(&channel_close_msg); + net_graph_msg_handler.network_graph.close_channel_from_update(short_channel_id, false); // Non-permanent closing just disables a channel { @@ -1689,12 +1713,7 @@ mod tests { }; } - let channel_close_msg = HTLCFailChannelUpdate::ChannelClosed { - short_channel_id, - is_permanent: true - }; - - net_graph_msg_handler.handle_htlc_fail_channel_update(&channel_close_msg); + net_graph_msg_handler.network_graph.close_channel_from_update(short_channel_id, true); // Permanent closing deletes a channel { @@ -1703,7 +1722,7 @@ mod tests { // Nodes are also deleted because there are no associated channels anymore assert_eq!(network.read_only().nodes().len(), 0); } - // TODO: Test HTLCFailChannelUpdate::NodeFailure, which is not implemented yet. + // TODO: Test NetworkUpdate::NodeFailure, which is not implemented yet. } #[test] diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index d4288164c..bde7d8c44 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -14,9 +14,10 @@ //! future, as well as generate and broadcast funding transactions handle payment preimages and a //! few other things. +use chain::keysinterface::SpendableOutputDescriptor; use ln::msgs; use ln::{PaymentPreimage, PaymentHash, PaymentSecret}; -use chain::keysinterface::SpendableOutputDescriptor; +use routing::network_graph::NetworkUpdate; use util::ser::{Writeable, Writer, MaybeReadable, Readable, VecReadWrapper, VecWriteWrapper}; use bitcoin::blockdata::script::Script; @@ -128,6 +129,12 @@ pub enum Event { /// the payment has failed, not just the route in question. If this is not set, you may /// retry the payment via a different route. rejected_by_dest: bool, + /// Any failure information conveyed via the Onion return packet by a node along the failed + /// payment route. Should be applied to the [`NetworkGraph`] so that routing decisions can + /// take into account the update. + /// + /// [`NetworkGraph`]: crate::routing::network_graph::NetworkGraph + network_update: Option, #[cfg(test)] error_code: Option, #[cfg(test)] @@ -211,7 +218,7 @@ impl Writeable for Event { (0, payment_preimage, required), }); }, - &Event::PaymentFailed { ref payment_hash, ref rejected_by_dest, + &Event::PaymentFailed { ref payment_hash, ref rejected_by_dest, ref network_update, #[cfg(test)] ref error_code, #[cfg(test)] @@ -224,6 +231,7 @@ impl Writeable for Event { error_data.write(writer)?; write_tlv_fields!(writer, { (0, payment_hash, required), + (1, network_update, option), (2, rejected_by_dest, required), }); }, @@ -307,13 +315,16 @@ impl MaybeReadable for Event { let error_data = Readable::read(reader)?; let mut payment_hash = PaymentHash([0; 32]); let mut rejected_by_dest = false; + let mut network_update = None; read_tlv_fields!(reader, { (0, payment_hash, required), + (1, network_update, ignorable), (2, rejected_by_dest, required), }); Ok(Some(Event::PaymentFailed { payment_hash, rejected_by_dest, + network_update, #[cfg(test)] error_code, #[cfg(test)] @@ -485,12 +496,6 @@ pub enum MessageSendEvent { /// The action which should be taken. action: msgs::ErrorAction }, - /// When a payment fails we may receive updates back from the hop where it failed. In such - /// cases this event is generated so that we can inform the network graph of this information. - PaymentFailureNetworkUpdate { - /// The channel/node update which should be sent to NetGraphMsgHandler - update: msgs::HTLCFailChannelUpdate, - }, /// Query a peer for channels with funding transaction UTXOs in a block range. SendChannelRangeQuery { /// The node_id of this message recipient diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 64b88acb0..6dd5836c4 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -349,7 +349,6 @@ impl msgs::RoutingMessageHandler for TestRoutingMessageHandler { self.chan_upds_recvd.fetch_add(1, Ordering::AcqRel); Err(msgs::LightningError { err: "".to_owned(), action: msgs::ErrorAction::IgnoreError }) } - fn handle_htlc_fail_channel_update(&self, _update: &msgs::HTLCFailChannelUpdate) {} fn get_next_channel_announcements(&self, starting_point: u64, batch_amount: u8) -> Vec<(msgs::ChannelAnnouncement, Option, Option)> { let mut chan_anns = Vec::new(); const TOTAL_UPDS: u64 = 100;