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.

diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index 2fe61aac42f0581b5d2dbf1df06a63a7f4b092fa..540b4e4c497f7e87da935f9acf4098eceef481f5 100644 (file)
--- a/fuzz/src/chanmon_consistency.rs
+++ b/fuzz/src/chanmon_consistency.rs
@@ -840,7 +840,7 @@ pub fn do_test<Out: Output>(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 52441629aa0a5341fc08f4a0b629b29cc1d01075..af4b024a8e977547edad1ff83cce929472692e12 100644 (file)
--- 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 f92787a0a48b11ef1d405e4dfe7395eaf9bfa0c9..19d0b5773184e7f34303a02caaccb3e756c9774d 100644 (file)
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -3500,9 +3500,17 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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 6ed319ff422a68558b9744c51895e397c4ddd4f2..e747810f1b53ae03e7dc5109622518d257cd81de 100644 (file)
--- 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 84c811f748587284a4d009963a548df6ccaba190..121d8482496aef2bbc693aa6bd283bf5d7d20748 100644 (file)
--- 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 9a07603fafe7e89e90b71a0d943bba59c483a223..802cd3acab45bd4592a7f80cdb982cfadeb7ed52 100644 (file)
--- 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();