Fix a debug panic caused by receiving MPP parts after a failure

Prior to cryptographic payment secrets, when we process a received
payment in `process_pending_htlc_fowards` we'd remove its entry
from the `pending_inbound_payments` map and give the user a
`PaymentReceived` event.

Thereafter, if a second HTLC came in with the same payment hash, it
would find no entry in the `pending_inbound_payments` map and be
immediately failed in `process_pending_htlc_forwards`.

Thus, each HTLC will either result in a `PaymentReceived` event or
be failed, with no possibility for both.

As of 8464875, we no longer
materially have a pending-inbound-payments map, and thus
more-than-happily accept a second payment with the same payment
hash even if we just failed a previous one for having mis-matched
payment data.

This can cause an issue if the two HTLCs are received back-to-back,
with the first being accepted as valid, generating a
`PaymentReceived` event. Then, when the second comes in we'll hit
the "total value {} ran over expected value" condition and fail
*all* pending HTLCs with the same payment hash. At this point,
we'll have a pending failure for both HTLCs, as well as a
`PaymentReceived` event for the user.

Thereafter, if the user attempts to fail the HTLC in response to
the `PaymentReceived`, they'll get a debug panic at channel.rs:1657
'Tried to fail an HTLC that was already failed'.

The solution is to avoid bulk-failing all pending HTLCs for a
payment. This feels like the right thing to do anyway - if a sender
accidentally sends an extra HTLC after a payment has ben fully
paid, we shouldn't fail the entire payment.

Found by the `chanmon_consistency` fuzz test.

Author: TheBlueMatt

lightning/src/ln/channelmanager.rs

@@ -3191,7 +3191,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 
 								macro_rules! check_total_value {
 									($payment_data_total_msat: expr, $payment_secret: expr, $payment_preimage: expr) => {{
-										let mut total_value = 0;
 										let mut payment_received_generated = false;
 										let htlcs = channel_state.claimable_htlcs.entry(payment_hash)
 											.or_insert(Vec::new());
@@ -3202,7 +3201,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 												continue
 											}
 										}
-										htlcs.push(claimable_htlc);
+										let mut total_value = claimable_htlc.value;
 										for htlc in htlcs.iter() {
 											total_value += htlc.value;
 											match &htlc.onion_payload {
@@ -3220,10 +3219,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 										if total_value >= msgs::MAX_VALUE_MSAT || total_value > $payment_data_total_msat {
 											log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the total value {} ran over expected value {} (or HTLCs were inconsistent)",
 												log_bytes!(payment_hash.0), total_value, $payment_data_total_msat);
-											for htlc in htlcs.iter() {
-												fail_htlc!(htlc);
-											}
+											fail_htlc!(claimable_htlc);
 										} else if total_value == $payment_data_total_msat {
+											htlcs.push(claimable_htlc);
 											new_events.push(events::Event::PaymentReceived {
 												payment_hash,
 												purpose: events::PaymentPurpose::InvoicePayment {
@@ -3237,6 +3235,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 											// Nothing to do - we haven't reached the total
 											// payment value yet, wait until we receive more
 											// MPP parts.
+											htlcs.push(claimable_htlc);
 										}
 										payment_received_generated
 									}}

lightning/src/ln/functional_tests.rs

@@ -9601,6 +9601,77 @@ fn test_forwardable_regen() {
 	claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage_2);
 }
 
+#[test]
+fn test_dup_htlc_second_fail_panic() {
+	// Previously, if we received two HTLCs back-to-back, where the second overran the expected
+	// value for the payment, we'd fail back both HTLCs after generating a `PaymentReceived` event.
+	// Then, if the user failed the second payment, they'd hit a "tried to fail an already failed
+	// HTLC" debug panic. This tests for this behavior, checking that only one HTLC is auto-failed.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let _chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 10001, InitFeatures::known(), InitFeatures::known());
+
+	let payment_params = PaymentParameters::from_node_id(nodes[1].node.get_our_node_id())
+		.with_features(InvoiceFeatures::known());
+	let scorer = test_utils::TestScorer::with_penalty(0);
+	let route = get_route(
+		&nodes[0].node.get_our_node_id(), &payment_params, &nodes[0].network_graph,
+		Some(&nodes[0].node.list_usable_channels().iter().collect::<Vec<_>>()),
+		10_000, TEST_FINAL_CLTV, nodes[0].logger, &scorer).unwrap();
+
+	let (_, our_payment_hash, our_payment_secret) = get_payment_preimage_hash!(&nodes[1]);
+
+	{
+		nodes[0].node.send_payment(&route, our_payment_hash, &Some(our_payment_secret)).unwrap();
+		check_added_monitors!(nodes[0], 1);
+		let mut events = nodes[0].node.get_and_clear_pending_msg_events();
+		assert_eq!(events.len(), 1);
+		let mut payment_event = SendEvent::from_event(events.pop().unwrap());
+		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);
+	}
+	expect_pending_htlcs_forwardable!(nodes[1]);
+	expect_payment_received!(nodes[1], our_payment_hash, our_payment_secret, 10_000);
+
+	{
+		nodes[0].node.send_payment(&route, our_payment_hash, &Some(our_payment_secret)).unwrap();
+		check_added_monitors!(nodes[0], 1);
+		let mut events = nodes[0].node.get_and_clear_pending_msg_events();
+		assert_eq!(events.len(), 1);
+		let mut payment_event = SendEvent::from_event(events.pop().unwrap());
+		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);
+		// At this point, nodes[1] would notice it has too much value for the payment. It will
+		// assume the second is a privacy attack (no longer particularly relevant
+		// post-payment_secrets) and fail back the new HTLC. Previously, it'd also have failed back
+		// the first HTLC delivered above.
+	}
+
+	// Now we go fail back the first HTLC from the user end.
+	expect_pending_htlcs_forwardable_ignore!(nodes[1]);
+	nodes[1].node.process_pending_htlc_forwards();
+	nodes[1].node.fail_htlc_backwards(&our_payment_hash);
+
+	expect_pending_htlcs_forwardable_ignore!(nodes[1]);
+	nodes[1].node.process_pending_htlc_forwards();
+
+	check_added_monitors!(nodes[1], 1);
+	let fail_updates_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
+	assert_eq!(fail_updates_1.update_fail_htlcs.len(), 2);
+
+	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_updates_1.update_fail_htlcs[0]);
+	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_updates_1.update_fail_htlcs[1]);
+	commitment_signed_dance!(nodes[0], nodes[1], fail_updates_1.commitment_signed, false);
+
+	let failure_events = nodes[0].node.get_and_clear_pending_events();
+	assert_eq!(failure_events.len(), 2);
+	if let Event::PaymentPathFailed { .. } = failure_events[0] {} else { panic!(); }
+	if let Event::PaymentPathFailed { .. } = failure_events[1] {} else { panic!(); }
+}
+
 #[test]
 fn test_keysend_payments_to_public_node() {
 	let chanmon_cfgs = create_chanmon_cfgs(2);