From a12d37e0633d94f1651cd24b656024b84f68e3cb Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 19 Apr 2022 22:06:50 +0000 Subject: [PATCH] Drop return value from `fail_htlc_backwards`, clarify docs `ChannelManager::fail_htlc_backwards`' bool return value is quite confusing - just because it returns false doesn't mean the payment wasn't (already) failed. Worse, in some race cases around shutdown where a payment was claimed before an unclean shutdown and then retried on startup, `fail_htlc_backwards` could return true even though (a duplicate copy of the same payment) was claimed, but the claim event has not been seen by the user yet. While its possible to use it correctly, its somewhat confusing to have a return value at all, and definitely lends itself to misuse. Instead, we should push users towards a model where they don't care if `fail_htlc_backwards` succeeds - either they've locally marked the payment as failed (prior to seeing any `PaymentReceived` events) and will fail any attempts to pay it, or they have not and the payment is still receivable until its timeout time is reached. We can revisit this decision based on user feedback, but will need to very carefully document the potential failure modes here if we do. --- fuzz/src/chanmon_consistency.rs | 2 +- lightning/src/ln/chanmon_update_fail_tests.rs | 6 ++--- lightning/src/ln/channelmanager.rs | 17 +++++++++----- lightning/src/ln/functional_test_utils.rs | 4 ++-- lightning/src/ln/functional_tests.rs | 22 +++++++++---------- lightning/src/ln/onion_route_tests.rs | 2 +- 6 files changed, 30 insertions(+), 23 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 2fe61aac4..540b4e4c4 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -840,7 +840,7 @@ pub fn do_test(data: &[u8], underlying_out: Out) { events::Event::PaymentReceived { payment_hash, .. } => { if claim_set.insert(payment_hash.0) { if $fail { - assert!(nodes[$node].fail_htlc_backwards(&payment_hash)); + nodes[$node].fail_htlc_backwards(&payment_hash); } else { nodes[$node].claim_funds(PaymentPreimage(payment_hash.0)); } diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index 52441629a..af4b024a8 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -831,7 +831,7 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) { let (_, payment_hash_1, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1000000); // Fail the payment backwards, failing the monitor update on nodes[1]'s receipt of the RAA - assert!(nodes[2].node.fail_htlc_backwards(&payment_hash_1)); + nodes[2].node.fail_htlc_backwards(&payment_hash_1); expect_pending_htlcs_forwardable!(nodes[2]); check_added_monitors!(nodes[2], 1); @@ -1696,7 +1696,7 @@ fn test_monitor_update_on_pending_forwards() { send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 5000000); let (_, payment_hash_1, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1000000); - assert!(nodes[2].node.fail_htlc_backwards(&payment_hash_1)); + nodes[2].node.fail_htlc_backwards(&payment_hash_1); expect_pending_htlcs_forwardable!(nodes[2]); check_added_monitors!(nodes[2], 1); @@ -2468,7 +2468,7 @@ fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim, second_f payment_preimage, }; if second_fails { - assert!(nodes[2].node.fail_htlc_backwards(&payment_hash)); + nodes[2].node.fail_htlc_backwards(&payment_hash); expect_pending_htlcs_forwardable!(nodes[2]); check_added_monitors!(nodes[2], 1); get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id()); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index f92787a0a..19d0b5773 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3500,9 +3500,17 @@ impl ChannelMana /// Indicates that the preimage for payment_hash is unknown or the received amount is incorrect /// after a PaymentReceived event, failing the HTLC back to its origin and freeing resources /// along the path (including in our own channel on which we received it). - /// Returns false if no payment was found to fail backwards, true if the process of failing the - /// HTLC backwards has been started. - pub fn fail_htlc_backwards(&self, payment_hash: &PaymentHash) -> bool { + /// + /// Note that in some cases around unclean shutdown, it is possible the payment may have + /// already been claimed by you via [`ChannelManager::claim_funds`] prior to you seeing (a + /// second copy of) the [`events::Event::PaymentReceived`] event. Alternatively, the payment + /// may have already been failed automatically by LDK if it was nearing its expiration time. + /// + /// While LDK will never claim a payment automatically on your behalf (i.e. without you calling + /// [`ChannelManager::claim_funds`]), you should still monitor for + /// [`events::Event::PaymentClaimed`] events even for payments you intend to fail, especially on + /// startup during which time claims that were in-progress at shutdown may be replayed. + pub fn fail_htlc_backwards(&self, payment_hash: &PaymentHash) { let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier); let mut channel_state = Some(self.channel_state.lock().unwrap()); @@ -3517,8 +3525,7 @@ impl ChannelMana HTLCSource::PreviousHopData(htlc.prev_hop), payment_hash, HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: htlc_msat_height_data }); } - true - } else { false } + } } /// Gets an HTLC onion failure code and error data for an `UPDATE` error, given the error code diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 6ed319ff4..e747810f1 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1739,7 +1739,7 @@ pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expe 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)); + 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); @@ -1845,7 +1845,7 @@ pub fn pass_failed_payment_back<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expe } // Ensure that fail_htlc_backwards is idempotent. - assert!(!expected_paths[0].last().unwrap().node.fail_htlc_backwards(&our_payment_hash)); + expected_paths[0].last().unwrap().node.fail_htlc_backwards(&our_payment_hash); assert!(expected_paths[0].last().unwrap().node.get_and_clear_pending_events().is_empty()); assert!(expected_paths[0].last().unwrap().node.get_and_clear_pending_msg_events().is_empty()); check_added_monitors!(expected_paths[0].last().unwrap(), 0); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 84c811f74..121d84824 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -3089,7 +3089,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use let (_, second_payment_hash, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], value); let (_, third_payment_hash, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], value); - assert!(nodes[2].node.fail_htlc_backwards(&first_payment_hash)); + nodes[2].node.fail_htlc_backwards(&first_payment_hash); expect_pending_htlcs_forwardable!(nodes[2]); check_added_monitors!(nodes[2], 1); let updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id()); @@ -3102,7 +3102,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use let bs_raa = commitment_signed_dance!(nodes[1], nodes[2], updates.commitment_signed, false, true, false, true); // Drop the last RAA from 3 -> 2 - assert!(nodes[2].node.fail_htlc_backwards(&second_payment_hash)); + nodes[2].node.fail_htlc_backwards(&second_payment_hash); expect_pending_htlcs_forwardable!(nodes[2]); check_added_monitors!(nodes[2], 1); let updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id()); @@ -3119,7 +3119,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa); check_added_monitors!(nodes[2], 1); - assert!(nodes[2].node.fail_htlc_backwards(&third_payment_hash)); + nodes[2].node.fail_htlc_backwards(&third_payment_hash); expect_pending_htlcs_forwardable!(nodes[2]); check_added_monitors!(nodes[2], 1); let updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id()); @@ -5518,10 +5518,10 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno // Now fail back three of the over-dust-limit and three of the under-dust-limit payments in one go. // Fail 0th below-dust, 4th above-dust, 8th above-dust, 10th below-dust HTLCs - assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_1)); - assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_3)); - assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_5)); - assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_6)); + nodes[4].node.fail_htlc_backwards(&payment_hash_1); + nodes[4].node.fail_htlc_backwards(&payment_hash_3); + nodes[4].node.fail_htlc_backwards(&payment_hash_5); + nodes[4].node.fail_htlc_backwards(&payment_hash_6); check_added_monitors!(nodes[4], 0); expect_pending_htlcs_forwardable!(nodes[4]); check_added_monitors!(nodes[4], 1); @@ -5534,8 +5534,8 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno commitment_signed_dance!(nodes[3], nodes[4], four_removes.commitment_signed, false); // Fail 3rd below-dust and 7th above-dust HTLCs - assert!(nodes[5].node.fail_htlc_backwards(&payment_hash_2)); - assert!(nodes[5].node.fail_htlc_backwards(&payment_hash_4)); + nodes[5].node.fail_htlc_backwards(&payment_hash_2); + nodes[5].node.fail_htlc_backwards(&payment_hash_4); check_added_monitors!(nodes[5], 0); expect_pending_htlcs_forwardable!(nodes[5]); check_added_monitors!(nodes[5], 1); @@ -5960,7 +5960,7 @@ fn do_htlc_claim_previous_remote_commitment_only(use_dust: bool, check_revoke_no // actually revoked. let htlc_value = if use_dust { 50000 } else { 3000000 }; let (_, our_payment_hash, _) = route_payment(&nodes[0], &[&nodes[1]], htlc_value); - assert!(nodes[1].node.fail_htlc_backwards(&our_payment_hash)); + nodes[1].node.fail_htlc_backwards(&our_payment_hash); expect_pending_htlcs_forwardable!(nodes[1]); check_added_monitors!(nodes[1], 1); @@ -7136,7 +7136,7 @@ fn do_test_failure_delay_dust_htlc_local_commitment(announce_latest: bool) { let as_prev_commitment_tx = get_local_commitment_txn!(nodes[0], chan.2); // Fail one HTLC to prune it in the will-be-latest-local commitment tx - assert!(nodes[1].node.fail_htlc_backwards(&payment_hash_2)); + nodes[1].node.fail_htlc_backwards(&payment_hash_2); check_added_monitors!(nodes[1], 0); expect_pending_htlcs_forwardable!(nodes[1]); check_added_monitors!(nodes[1], 1); diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index 9a07603fa..802cd3aca 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -1155,7 +1155,7 @@ fn test_phantom_failure_reject_payment() { expect_pending_htlcs_forwardable_ignore!(nodes[1]); nodes[1].node.process_pending_htlc_forwards(); expect_payment_received!(nodes[1], payment_hash, payment_secret, recv_amt_msat); - assert!(nodes[1].node.fail_htlc_backwards(&payment_hash)); + nodes[1].node.fail_htlc_backwards(&payment_hash); expect_pending_htlcs_forwardable_ignore!(nodes[1]); nodes[1].node.process_pending_htlc_forwards(); -- 2.39.5