From: Matt Corallo Date: Wed, 19 Jan 2022 03:52:56 +0000 (+0000) Subject: Fix a debug panic caused by receiving MPP parts after a failure X-Git-Tag: v0.0.105~12^2 X-Git-Url: http://git.bitcoin.ninja/?a=commitdiff_plain;h=refs%2Fheads%2F2022-01-fix-double-fail-panic;p=rust-lightning 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 0ae0f8c3e..0b24503cc 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3191,7 +3191,6 @@ impl 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 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 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 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 ad416fbff..3c7174a9d 100644 --- 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::>()), + 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);