From: Matt Corallo Date: Tue, 19 Apr 2022 21:46:44 +0000 (+0000) Subject: Do additional pre-flight checks before claiming a payment X-Git-Tag: v0.0.107~12^2~3 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=11c2f12baaeb4fb8fcd3965eb1aaea7b8aa21722;p=rust-lightning Do additional pre-flight checks before claiming a payment 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. --- diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 03e076713..f92787a0a 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3825,14 +3825,42 @@ impl 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; diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 458601542..6ed319ff4 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -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()); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index d033f85f6..84c811f74 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -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