Refactor PaymentFailureNetworkUpdate event
authorJeffrey Czyz <jkczyz@gmail.com>
Thu, 12 Aug 2021 20:30:53 +0000 (15:30 -0500)
committerJeffrey Czyz <jkczyz@gmail.com>
Wed, 15 Sep 2021 16:50:27 +0000 (11:50 -0500)
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.

15 files changed:
fuzz/src/chanmon_consistency.rs
lightning-net-tokio/src/lib.rs
lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_test_utils.rs
lightning/src/ln/functional_tests.rs
lightning/src/ln/monitor_tests.rs
lightning/src/ln/msgs.rs
lightning/src/ln/onion_route_tests.rs
lightning/src/ln/onion_utils.rs
lightning/src/ln/peer_handler.rs
lightning/src/ln/reorg_tests.rs
lightning/src/ln/shutdown_tests.rs
lightning/src/routing/network_graph.rs
lightning/src/util/events.rs
lightning/src/util/test_utils.rs

index 88826d65570fb94c7cfda47de7cfa58c53e5027e..6e726a9475265f774ef97a2f839200726849529c 100644 (file)
@@ -594,7 +594,6 @@ pub fn do_test<Out: test_logger::Output>(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<Out: test_logger::Output>(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<Out: test_logger::Output>(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<Out: test_logger::Output>(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!
                                                        },
index bf411211fdbb81b1e077c75eab92889e6bb5e54b..13a4c0d934fe264fa5be936d440951c5c5bee1dd 100644 (file)
@@ -492,7 +492,6 @@ mod tests {
                fn handle_node_announcement(&self, _msg: &NodeAnnouncement) -> Result<bool, LightningError> { Ok(false) }
                fn handle_channel_announcement(&self, _msg: &ChannelAnnouncement) -> Result<bool, LightningError> { Ok(false) }
                fn handle_channel_update(&self, _msg: &ChannelUpdate) -> Result<bool, LightningError> { 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<ChannelUpdate>, Option<ChannelUpdate>)> { Vec::new() }
                fn get_next_node_announcements(&self, _starting_point: Option<&PublicKey>, _batch_amount: u8) -> Vec<NodeAnnouncement> { Vec::new() }
                fn sync_routing_table(&self, _their_node_id: &PublicKey, _init_msg: &Init) { }
index 35535c058803c7c71a63ecc5f1928f2e59d89fe1..b772c5e9ac75789f0876e2664bb87fdf2c8129a4 100644 (file)
@@ -2822,6 +2822,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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<Signer: Sign, M: Deref , T: Deref , K: Deref , F: Deref , L: Deref >
                                        &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,
index 4e7ca676d9f136c0b2b0943c9a8944c0f35be724..96a6ae86d6d104d2c85c999bf1644d77eaafcbc7 100644 (file)
@@ -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");
index eb3d3cb3bf0562e72ed24c13bda45729d3e9de7d..e1a57cd7cc1533e084c955a69d6b3820043fa8a7 100644 (file)
@@ -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);
        }
 }
 
index 8fdf28e578b835342c498ba39d4132ec5ab9bd0b..2d3c8f59fb63e3f89fd9741c0f457cd3c98e1a26 100644 (file)
@@ -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);
 }
index 5b49f43b1188013c5f44822e59622409590aefb3..7c2647916e50cd18d68597e6ddc2142cd1f649df 100644 (file)
@@ -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 Option<T>s (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<bool, LightningError>;
-       /// 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.
index 1006d9da133e2e21426a4fe1e45f9ed2f28973c1..bcc05fdb0300f89c415736a7e0272e6e043dc8fe 100644 (file)
@@ -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<F1,F2>(_name: &str, test_case: u8, nodes: &Vec<Node>, route: &Route, payment_hash: &PaymentHash, payment_secret: &PaymentSecret, callback_msg: F1, callback_node: F2, expected_retryable: bool, expected_error_code: Option<u16>, expected_channel_update: Option<HTLCFailChannelUpdate>)
+fn run_onion_failure_test<F1,F2>(_name: &str, test_case: u8, nodes: &Vec<Node>, route: &Route, payment_hash: &PaymentHash, payment_secret: &PaymentSecret, callback_msg: F1, callback_node: F2, expected_retryable: bool, expected_error_code: Option<u16>, expected_channel_update: Option<NetworkUpdate>)
        where F1: for <'a> FnMut(&'a mut msgs::UpdateAddHTLC),
                                F2: FnMut(),
 {
@@ -53,7 +54,7 @@ fn run_onion_failure_test<F1,F2>(_name: &str, test_case: u8, nodes: &Vec<Node>,
 // 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<F1,F2,F3>(_name: &str, test_case: u8, nodes: &Vec<Node>, 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<u16>, expected_channel_update: Option<HTLCFailChannelUpdate>)
+fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case: u8, nodes: &Vec<Node>, 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<u16>, expected_channel_update: Option<NetworkUpdate>)
        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<F1,F2,F3>(_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<F1,F2,F3>(_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| {
index 4886168dd16cc277adfd536fcedbca83ba03ac1a..f6c62cb83cac0de409a8bc3770b8722dca4b4896 100644 (file)
@@ -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<T: secp256k1::Signing, L: Deref>(secp_ctx: &Secp256k1<T>, logger: &L, htlc_source: &HTLCSource, mut packet_decrypted: Vec<u8>) -> (Option<msgs::HTLCFailChannelUpdate>, bool, Option<u16>, Option<Vec<u8>>) where L::Target: Logger {
+pub(super) fn process_onion_failure<T: secp256k1::Signing, L: Deref>(secp_ctx: &Secp256k1<T>, logger: &L, htlc_source: &HTLCSource, mut packet_decrypted: Vec<u8>) -> (Option<NetworkUpdate>, bool, Option<u16>, Option<Vec<u8>>) 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<T: secp256k1::Signing, L: Deref>(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<T: secp256k1::Signing, L: Deref>(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<T: secp256k1::Signing, L: Deref>(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<T: secp256k1::Signing, L: Deref>(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<T: secp256k1::Signing, L: Deref>(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));
index 34bcda3a009a850b780bfffd6d4c087eb0256b28..60d7a8750d84eea4b2bebe54a9c63771fc275a46 100644 (file)
@@ -66,7 +66,6 @@ impl RoutingMessageHandler for IgnoringMessageHandler {
        fn handle_node_announcement(&self, _msg: &msgs::NodeAnnouncement) -> Result<bool, LightningError> { Ok(false) }
        fn handle_channel_announcement(&self, _msg: &msgs::ChannelAnnouncement) -> Result<bool, LightningError> { Ok(false) }
        fn handle_channel_update(&self, _msg: &msgs::ChannelUpdate) -> Result<bool, LightningError> { 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<msgs::ChannelUpdate>, Option<msgs::ChannelUpdate>)> { Vec::new() }
        fn get_next_node_announcements(&self, _starting_point: Option<&PublicKey>, _batch_amount: u8) -> Vec<msgs::NodeAnnouncement> { Vec::new() }
@@ -1318,9 +1317,6 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref, CMH: Deref> 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 } => {
index bbdb5bfac6bae9270e30bb99434c818020a3d475..0db30b8178cdc6bb8dc6113168d93d56ecb7f762 100644 (file)
@@ -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);
        }
 }
 
index c1140ac43e9f420c1187cc818d1de4d2f210f86e..26d39fbad9df08a4d54b507a9ad4126dbdd9db8c 100644 (file)
@@ -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()
index 61e94e0494c4e48cbe67c489b3f9b93315042026..1a149b3bd76cf580951c2cf2599da7c7924048d2 100644 (file)
@@ -64,6 +64,52 @@ pub struct ReadOnlyNetworkGraph<'a> {
        nodes: RwLockReadGuard<'a, BTreeMap<PublicKey, NodeInfo>>,
 }
 
+/// 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<C: Deref , L: Deref > RoutingMessageHandler for NetGraphMsgHandler<C, L> 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<bool, LightningError> {
                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]
index d4288164cd972e23230ac86af64d42c6bf254952..bde7d8c441301251cd8d2278c3837348ce2e3cbc 100644 (file)
 //! 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<NetworkUpdate>,
 #[cfg(test)]
                error_code: Option<u16>,
 #[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
index 64b88acb0081487495f997ac2706f9783dd62d9c..6dd5836c4809ec4d9691f1d926457ab2aa985836 100644 (file)
@@ -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<msgs::ChannelUpdate>, Option<msgs::ChannelUpdate>)> {
                let mut chan_anns = Vec::new();
                const TOTAL_UPDS: u64 = 100;