Drop forwarded HTLCs which were still pending at persist-time

If, after forwarding an intercepted payment to our counterparty, we
restart with a ChannelMonitor update having been persisted, but the
corresponding ChannelManager update not having been persisted,
we'll still have the intercepted HTLC in the
`pending_intercepted_htlcs` map on start (and potentially a pending
`HTLCIntercepted` event). This will cause us to allow the user to
handle the forwarded HTLC twice, potentially double-forwarding it.

This builds on 0bb87ddad71d2e33199ebad79e9f709f869f2130, which
provided a preemptive fix for the general relay case (though it was
not an actual issue at the time). We simply check for the HTLCs
having been forwarded on startup and remove them from the map.

Fixes #1858

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 16c5ec68a7debd1dbd7c262883e5109f534ac899..b8372ad9b10302738882fcc7a35665135cee8e73 100644 (file)
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -7527,17 +7527,19 @@ impl<'a, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
                                        }
                                        for (htlc_source, htlc) in monitor.get_all_current_outbound_htlcs() {
                                                if let HTLCSource::PreviousHopData(prev_hop_data) = htlc_source {
+                                                       let pending_forward_matches_htlc = |info: &PendingAddHTLCInfo| {
+                                                               info.prev_funding_outpoint == prev_hop_data.outpoint &&
+                                                                       info.prev_htlc_id == prev_hop_data.htlc_id
+                                                       };
                                                        // The ChannelMonitor is now responsible for this HTLC's
                                                        // failure/success and will let us know what its outcome is. If we
-                                                       // still have an entry for this HTLC in `forward_htlcs`, we were
-                                                       // apparently not persisted after the monitor was when forwarding
-                                                       // the payment.
+                                                       // still have an entry for this HTLC in `forward_htlcs` or
+                                                       // `pending_intercepted_htlcs`, we were apparently not persisted after
+                                                       // the monitor was when forwarding the payment.
                                                        forward_htlcs.retain(|_, forwards| {
                                                                forwards.retain(|forward| {
                                                                        if let HTLCForwardInfo::AddHTLC(htlc_info) = forward {
-                                                                               if htlc_info.prev_short_channel_id == prev_hop_data.short_channel_id &&
-                                                                                       htlc_info.prev_htlc_id == prev_hop_data.htlc_id
-                                                                               {
+                                                                               if pending_forward_matches_htlc(&htlc_info) {
                                                                                        log_info!(args.logger, "Removing pending to-forward HTLC with hash {} as it was forwarded to the closed channel {}",
                                                                                                log_bytes!(htlc.payment_hash.0), log_bytes!(monitor.get_funding_txo().0.to_channel_id()));
                                                                                        false
@@ -7545,7 +7547,19 @@ impl<'a, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
                                                                        } else { true }
                                                                });
                                                                !forwards.is_empty()
-                                                       })
+                                                       });
+                                                       pending_intercepted_htlcs.as_mut().unwrap().retain(|intercepted_id, htlc_info| {
+                                                               if pending_forward_matches_htlc(&htlc_info) {
+                                                                       log_info!(args.logger, "Removing pending intercepted HTLC with hash {} as it was forwarded to the closed channel {}",
+                                                                               log_bytes!(htlc.payment_hash.0), log_bytes!(monitor.get_funding_txo().0.to_channel_id()));
+                                                                       pending_events_read.retain(|event| {
+                                                                               if let Event::HTLCIntercepted { intercept_id: ev_id, .. } = event {
+                                                                                       intercepted_id != ev_id
+                                                                               } else { true }
+                                                                       });
+                                                                       false
+                                                               } else { true }
+                                                       });
                                                }
                                        }
                                }

diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs
index 2c7e8e72bbee663fd824a7f3aad5a27b1c118792..6c04cbe0ca292b11ae313732aea71664e7a1e41f 100644 (file)
--- a/lightning/src/ln/reload_tests.rs
+++ b/lightning/src/ln/reload_tests.rs
@@ -19,6 +19,7 @@ use crate::ln::msgs;
 use crate::ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, ErrorAction};
 use crate::util::enforcing_trait_impls::EnforcingSigner;
 use crate::util::test_utils;
+use crate::util::errors::APIError;
 use crate::util::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
 use crate::util::ser::{Writeable, ReadableArgs};
 use crate::util::config::UserConfig;
@@ -814,7 +815,7 @@ fn test_partial_claim_before_restart() {
        do_test_partial_claim_before_restart(true);
 }
 
-fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_htlc: bool) {
+fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_htlc: bool, use_intercept: bool) {
        if !use_cs_commitment { assert!(!claim_htlc); }
        // If we go to forward a payment, and the ChannelMonitor persistence completes, but the
        // ChannelManager does not, we shouldn't try to forward the payment again, nor should we fail
@@ -822,7 +823,9 @@ fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_ht
        // This was never an issue, but it may be easy to regress here going forward.
        let chanmon_cfgs = create_chanmon_cfgs(3);
        let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
-       let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
+       let mut intercept_forwards_config = test_default_channel_config();
+       intercept_forwards_config.accept_intercept_htlcs = true;
+       let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, Some(intercept_forwards_config), None]);
 
        let persister;
        let new_chain_monitor;
@@ -833,7 +836,13 @@ fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_ht
        let chan_id_1 = create_announced_chan_between_nodes(&nodes, 0, 1, channelmanager::provided_init_features(), channelmanager::provided_init_features()).2;
        let chan_id_2 = create_announced_chan_between_nodes(&nodes, 1, 2, channelmanager::provided_init_features(), channelmanager::provided_init_features()).2;
 
-       let (route, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], 1_000_000);
+       let intercept_scid = nodes[1].node.get_intercept_scid();
+
+       let (mut route, payment_hash, payment_preimage, payment_secret) =
+               get_route_and_payment_hash!(nodes[0], nodes[2], 1_000_000);
+       if use_intercept {
+               route.paths[0][1].short_channel_id = intercept_scid;
+       }
        let payment_id = PaymentId(nodes[0].keys_manager.backing.get_secure_random_bytes());
        let htlc_expiry = nodes[0].best_block_info().1 + TEST_FINAL_CLTV;
        nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret), payment_id).unwrap();
@@ -843,8 +852,27 @@ fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_ht
        nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
        commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false);
 
+       // Store the `ChannelManager` before handling the `PendingHTLCsForwardable`/`HTLCIntercepted`
+       // events, expecting either event (and the HTLC itself) to be missing on reload even though its
+       // present when we serialized.
        let node_encoded = nodes[1].node.encode();
 
+       let mut intercept_id = None;
+       let mut expected_outbound_amount_msat = None;
+       if use_intercept {
+               let events = nodes[1].node.get_and_clear_pending_events();
+               assert_eq!(events.len(), 1);
+               match events[0] {
+                       Event::HTLCIntercepted { intercept_id: ev_id, expected_outbound_amount_msat: ev_amt, .. } => {
+                               intercept_id = Some(ev_id);
+                               expected_outbound_amount_msat = Some(ev_amt);
+                       },
+                       _ => panic!()
+               }
+               nodes[1].node.forward_intercepted_htlc(intercept_id.unwrap(), &chan_id_2,
+                       nodes[2].node.get_our_node_id(), expected_outbound_amount_msat.unwrap()).unwrap();
+       }
+
        expect_pending_htlcs_forwardable!(nodes[1]);
 
        let payment_event = SendEvent::from_node(&nodes[1]);
@@ -872,8 +900,20 @@ fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_ht
        let chan_1_monitor_serialized = get_monitor!(nodes[1], chan_id_2).encode();
        reload_node!(nodes[1], node_encoded, &[&chan_0_monitor_serialized, &chan_1_monitor_serialized], persister, new_chain_monitor, nodes_1_deserialized);
 
+       // Note that this checks that this is the only event on nodes[1], implying the
+       // `HTLCIntercepted` event has been removed in the `use_intercept` case.
        check_closed_event!(nodes[1], 1, ClosureReason::OutdatedChannelManager);
 
+       if use_intercept {
+               // Attempt to forward the HTLC back out over nodes[1]' still-open channel, ensuring we get
+               // a intercept-doesn't-exist error.
+               let forward_err = nodes[1].node.forward_intercepted_htlc(intercept_id.unwrap(), &chan_id_1,
+                       nodes[0].node.get_our_node_id(), expected_outbound_amount_msat.unwrap()).unwrap_err();
+               assert_eq!(forward_err, APIError::APIMisuseError {
+                       err: format!("Payment with intercept id {} not found", log_bytes!(intercept_id.unwrap().0))
+               });
+       }
+
        let bs_commitment_tx = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
        assert_eq!(bs_commitment_tx.len(), 1);
 
@@ -929,9 +969,16 @@ fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_ht
 
 #[test]
 fn forwarded_payment_no_manager_persistence() {
-       do_forwarded_payment_no_manager_persistence(true, true);
-       do_forwarded_payment_no_manager_persistence(true, false);
-       do_forwarded_payment_no_manager_persistence(false, false);
+       do_forwarded_payment_no_manager_persistence(true, true, false);
+       do_forwarded_payment_no_manager_persistence(true, false, false);
+       do_forwarded_payment_no_manager_persistence(false, false, false);
+}
+
+#[test]
+fn intercepted_payment_no_manager_persistence() {
+       do_forwarded_payment_no_manager_persistence(true, true, true);
+       do_forwarded_payment_no_manager_persistence(true, false, true);
+       do_forwarded_payment_no_manager_persistence(false, false, true);
 }
 
 #[test]