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 846487555556d8465c5b7b811f976e78f265c48f, 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.

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 0ae0f8c3e8096e34970bd6f2e42452365d86c1cf..0b24503cc92fc49c79d560cfe91e5f9c019c0fb8 100644 (file)
--- a/lightning/src/ln/channelmanager.rs
+++ b/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
                                                                        }}

diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index ad416fbffc34177b29e9d0005c0f5a8b101f0ea1..3c7174a9d71a63a8f74116229ee46f6da5f42f4a 100644 (file)
--- a/lightning/src/ln/functional_tests.rs
+++ b/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);