Do additional pre-flight checks before claiming a payment
authorMatt Corallo <git@bluematt.me>
Tue, 19 Apr 2022 21:46:44 +0000 (21:46 +0000)
committerMatt Corallo <git@bluematt.me>
Sat, 28 May 2022 00:02:49 +0000 (00:02 +0000)
As additional sanity checks, before claiming a payment, we check
that we have the full amount available in `claimable_htlcs` that
the payment should be for. Concretely, this prevents one
somewhat-absurd edge case where a user may receive an MPP payment,
wait many *blocks* before claiming it, allowing us to fail the
pending HTLCs and the sender to retry some subset of the payment
before we go to claim. More generally, this is just good
belt-and-suspenders against any edge cases we may have missed.

lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_test_utils.rs
lightning/src/ln/functional_tests.rs

index 03e07671360880304d7d76952e3373d5c4167144..f92787a0a48b11ef1d405e4dfe7395eaf9bfa0c9 100644 (file)
@@ -3825,14 +3825,42 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                        // provide the preimage, so worrying too much about the optimal handling isn't worth
                        // it.
                        let mut claimable_amt_msat = 0;
+                       let mut expected_amt_msat = None;
                        let mut valid_mpp = true;
                        for htlc in sources.iter() {
                                if let None = channel_state.as_ref().unwrap().short_to_id.get(&htlc.prev_hop.short_channel_id) {
                                        valid_mpp = false;
                                        break;
                                }
+                               if expected_amt_msat.is_some() && expected_amt_msat != Some(htlc.total_msat) {
+                                       log_error!(self.logger, "Somehow ended up with an MPP payment with different total amounts - this should not be reachable!");
+                                       debug_assert!(false);
+                                       valid_mpp = false;
+                                       break;
+                               }
+                               expected_amt_msat = Some(htlc.total_msat);
+                               if let OnionPayload::Spontaneous(_) = &htlc.onion_payload {
+                                       // We don't currently support MPP for spontaneous payments, so just check
+                                       // that there's one payment here and move on.
+                                       if sources.len() != 1 {
+                                               log_error!(self.logger, "Somehow ended up with an MPP spontaneous payment - this should not be reachable!");
+                                               debug_assert!(false);
+                                               valid_mpp = false;
+                                               break;
+                                       }
+                               }
+
                                claimable_amt_msat += htlc.value;
                        }
+                       if sources.is_empty() || expected_amt_msat.is_none() {
+                               log_info!(self.logger, "Attempted to claim an incomplete payment which no longer had any available HTLCs!");
+                               return;
+                       }
+                       if claimable_amt_msat != expected_amt_msat.unwrap() {
+                               log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",
+                                       expected_amt_msat.unwrap(), claimable_amt_msat);
+                               return;
+                       }
 
                        let mut errs = Vec::new();
                        let mut claimed_any_htlcs = false;
index 458601542ca149bb2944ddf888eb7d3e0f5f1172..6ed319ff422a68558b9744c51895e397c4ddd4f2 100644 (file)
@@ -1735,13 +1735,18 @@ pub fn send_payment<'a, 'b, 'c>(origin: &Node<'a, 'b, 'c>, expected_route: &[&No
        claim_payment(&origin, expected_route, our_payment_preimage);
 }
 
-pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths_slice: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_hash: PaymentHash)  {
-       let mut expected_paths: Vec<_> = expected_paths_slice.iter().collect();
+pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_hash: PaymentHash) {
        for path in expected_paths.iter() {
                assert_eq!(path.last().unwrap().node.get_our_node_id(), expected_paths[0].last().unwrap().node.get_our_node_id());
        }
        assert!(expected_paths[0].last().unwrap().node.fail_htlc_backwards(&our_payment_hash));
        expect_pending_htlcs_forwardable!(expected_paths[0].last().unwrap());
+
+       pass_failed_payment_back(origin_node, expected_paths, skip_last, our_payment_hash);
+}
+
+pub fn pass_failed_payment_back<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths_slice: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_hash: PaymentHash) {
+       let mut expected_paths: Vec<_> = expected_paths_slice.iter().collect();
        check_added_monitors!(expected_paths[0].last().unwrap(), expected_paths.len());
 
        let mut per_path_msgs: Vec<((msgs::UpdateFailHTLC, msgs::CommitmentSigned), PublicKey)> = Vec::with_capacity(expected_paths.len());
index d033f85f6f77b98af44105ab8a8061e9b9cba122..84c811f748587284a4d009963a548df6ccaba190 100644 (file)
@@ -9870,6 +9870,57 @@ fn test_keysend_payments_to_private_node() {
        claim_payment(&nodes[0], &path, test_preimage);
 }
 
+#[test]
+fn test_double_partial_claim() {
+       // Test what happens if a node receives a payment, generates a PaymentReceived event, the HTLCs
+       // time out, the sender resends only some of the MPP parts, then the user processes the
+       // PaymentReceived event, ensuring they don't inadvertently claim only part of the full payment
+       // amount.
+       let chanmon_cfgs = create_chanmon_cfgs(4);
+       let node_cfgs = create_node_cfgs(4, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]);
+       let nodes = create_network(4, &node_cfgs, &node_chanmgrs);
+
+       create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 0, InitFeatures::known(), InitFeatures::known());
+       create_announced_chan_between_nodes_with_value(&nodes, 0, 2, 100_000, 0, InitFeatures::known(), InitFeatures::known());
+       create_announced_chan_between_nodes_with_value(&nodes, 1, 3, 100_000, 0, InitFeatures::known(), InitFeatures::known());
+       create_announced_chan_between_nodes_with_value(&nodes, 2, 3, 100_000, 0, InitFeatures::known(), InitFeatures::known());
+
+       let (mut route, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[3], 15_000_000);
+       assert_eq!(route.paths.len(), 2);
+       route.paths.sort_by(|path_a, _| {
+               // Sort the path so that the path through nodes[1] comes first
+               if path_a[0].pubkey == nodes[1].node.get_our_node_id() {
+                       core::cmp::Ordering::Less } else { core::cmp::Ordering::Greater }
+       });
+
+       send_along_route_with_secret(&nodes[0], route.clone(), &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], 15_000_000, payment_hash, payment_secret);
+       // nodes[3] has now received a PaymentReceived event...which it will take some (exorbitant)
+       // amount of time to respond to.
+
+       // Connect some blocks to time out the payment
+       connect_blocks(&nodes[3], TEST_FINAL_CLTV);
+       connect_blocks(&nodes[0], TEST_FINAL_CLTV); // To get the same height for sending later
+
+       expect_pending_htlcs_forwardable!(nodes[3]);
+
+       pass_failed_payment_back(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, payment_hash);
+
+       // nodes[1] now retries one of the two paths...
+       nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap();
+       check_added_monitors!(nodes[0], 2);
+
+       let mut events = nodes[0].node.get_and_clear_pending_msg_events();
+       assert_eq!(events.len(), 2);
+       pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], 15_000_000, payment_hash, Some(payment_secret), events.drain(..).next().unwrap(), false, None);
+
+       // At this point nodes[3] has received one half of the payment, and the user goes to handle
+       // that PaymentReceived event they got hours ago and never handled...we should refuse to claim.
+       nodes[3].node.claim_funds(payment_preimage);
+       check_added_monitors!(nodes[3], 0);
+       assert!(nodes[3].node.get_and_clear_pending_msg_events().is_empty());
+}
+
 fn do_test_partial_claim_before_restart(persist_both_monitors: bool) {
        // Test what happens if a node receives an MPP payment, claims it, but crashes before
        // persisting the ChannelManager. If `persist_both_monitors` is false, also crash after only