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.

Author: TheBlueMatt

lightning/src/ln/channelmanager.rs

@@ -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;

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());

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