Mark failed counterparty-is-destination HTLCs retryable

When our counterparty is the payment destination and we receive
an `HTLCFailReason::Reason` in `fail_htlc_backwards_internal` we
currently always set `rejected_by_dest` in the `PaymentPathFailed`
event, implying the HTLC should *not* be retried.

There are a number of cases where we use `HTLCFailReason::Reason`,
but most should reasonably be treated as retryable even if our
counterparty was the destination (i.e. `!rejected_by_dest`):
 * If an HTLC times out on-chain, this doesn't imply that the
   payment is no longer retryable, though the peer may well be
   offline so retrying may not be very useful,
 * If a commitment transaction "containing" a dust HTLC is
   confirmed on-chain, this definitely does not imply the payment
   is no longer retryable
 * If the channel we intended to relay over was closed (or
   force-closed) we should retry over another path,
 * If the channel we intended to relay over did not have enough
   capacity we should retry over another path,
 * If we received a update_fail_malformed_htlc message from our
   peer, we likely should *not* retry, however this should be
   exceedingly rare, and appears to nearly never appear in practice

Thus, this commit simply disables the behavior here, opting to
treat all `HTLCFailReason::Reason` errors as retryable.

Note that prior to 93e645daf46f85949ae0edf60d36bf21e9fde8af this
change would not have made sense as it would have resulted in us
retrying the payment over the same channel in some cases, however
we now "blame" our own channel and will avoid it when routing for
the same payment.

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 3b83726958b3a655b681f9a821d114b5a97e7c8a..6f8ba7b39a4834afd538864c8bde87167af35e1c 100644 (file)
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -3990,7 +3990,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                events::Event::PaymentPathFailed {
                                                        payment_id: Some(payment_id),
                                                        payment_hash: payment_hash.clone(),
-                                                       rejected_by_dest: path.len() == 1,
+                                                       rejected_by_dest: false,
                                                        network_update: None,
                                                        all_paths_failed,
                                                        path: path.clone(),

diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index c41d5402ff3861e2dafaff10da98d9d4b49c25d6..826a2c31aa94afa99ea5c6ab2d6ad3972fb7d073 100644 (file)
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -2560,7 +2560,7 @@ fn claim_htlc_outputs_shared_tx() {
                // ANTI_REORG_DELAY confirmations.
                mine_transaction(&nodes[1], &node_txn[0]);
                connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
-               expect_payment_failed!(nodes[1], payment_hash_2, true);
+               expect_payment_failed!(nodes[1], payment_hash_2, false);
        }
        get_announce_close_broadcast_events(&nodes, 0, 1);
        assert_eq!(nodes[0].node.list_channels().len(), 0);
@@ -2642,7 +2642,7 @@ fn claim_htlc_outputs_single_tx() {
                mine_transaction(&nodes[1], &node_txn[3]);
                mine_transaction(&nodes[1], &node_txn[4]);
                connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
-               expect_payment_failed!(nodes[1], payment_hash_2, true);
+               expect_payment_failed!(nodes[1], payment_hash_2, false);
        }
        get_announce_close_broadcast_events(&nodes, 0, 1);
        assert_eq!(nodes[0].node.list_channels().len(), 0);
@@ -4960,7 +4960,7 @@ fn test_static_spendable_outputs_timeout_tx() {
        mine_transaction(&nodes[1], &node_txn[1]);
        check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed);
        connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
-       expect_payment_failed!(nodes[1], our_payment_hash, true);
+       expect_payment_failed!(nodes[1], our_payment_hash, false);
 
        let spend_txn = check_spendable_outputs!(nodes[1], node_cfgs[1].keys_manager);
        assert_eq!(spend_txn.len(), 3); // SpendableOutput: remote_commitment_tx.to_remote, timeout_tx.output
@@ -5818,7 +5818,7 @@ fn test_dynamic_spendable_outputs_local_htlc_timeout_tx() {
 
        mine_transaction(&nodes[0], &htlc_timeout);
        connect_blocks(&nodes[0], BREAKDOWN_TIMEOUT as u32 - 1);
-       expect_payment_failed!(nodes[0], our_payment_hash, true);
+       expect_payment_failed!(nodes[0], our_payment_hash, false);
 
        // Verify that A is able to spend its own HTLC-Timeout tx thanks to spendable output event given back by its ChannelMonitor
        let spend_txn = check_spendable_outputs!(nodes[0], node_cfgs[0].keys_manager);
@@ -5900,7 +5900,7 @@ fn test_key_derivation_params() {
 
        mine_transaction(&nodes[0], &htlc_timeout);
        connect_blocks(&nodes[0], BREAKDOWN_TIMEOUT as u32 - 1);
-       expect_payment_failed!(nodes[0], our_payment_hash, true);
+       expect_payment_failed!(nodes[0], our_payment_hash, false);
 
        // Verify that A is able to spend its own HTLC-Timeout tx thanks to spendable output event given back by its ChannelMonitor
        let new_keys_manager = test_utils::TestKeysInterface::new(&seed, Network::Testnet);
@@ -7304,7 +7304,7 @@ fn do_test_sweep_outbound_htlc_failure_update(revoked: bool, local: bool) {
                mine_transaction(&nodes[0], &as_commitment_tx[0]);
                check_closed_event!(nodes[0], 1, ClosureReason::CommitmentTxConfirmed);
                connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
-               expect_payment_failed!(nodes[0], dust_hash, true);
+               expect_payment_failed!(nodes[0], dust_hash, false);
 
                connect_blocks(&nodes[0], TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS - ANTI_REORG_DELAY);
                check_closed_broadcast!(nodes[0], true);
@@ -7316,7 +7316,7 @@ fn do_test_sweep_outbound_htlc_failure_update(revoked: bool, local: bool) {
                assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 0);
                mine_transaction(&nodes[0], &timeout_tx[0]);
                connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
-               expect_payment_failed!(nodes[0], non_dust_hash, true);
+               expect_payment_failed!(nodes[0], non_dust_hash, false);
        } else {
                // We fail dust-HTLC 1 by broadcast of remote commitment tx. If revoked, fail also non-dust HTLC
                mine_transaction(&nodes[0], &bs_commitment_tx[0]);
@@ -7331,7 +7331,7 @@ fn do_test_sweep_outbound_htlc_failure_update(revoked: bool, local: bool) {
                check_spends!(timeout_tx[0], bs_commitment_tx[0]);
                // For both a revoked or non-revoked commitment transaction, after ANTI_REORG_DELAY the
                // dust HTLC should have been failed.
-               expect_payment_failed!(nodes[0], dust_hash, true);
+               expect_payment_failed!(nodes[0], dust_hash, false);
 
                if !revoked {
                        assert_eq!(timeout_tx[0].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
@@ -7342,7 +7342,7 @@ fn do_test_sweep_outbound_htlc_failure_update(revoked: bool, local: bool) {
                mine_transaction(&nodes[0], &timeout_tx[0]);
                assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 0);
                connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
-               expect_payment_failed!(nodes[0], non_dust_hash, true);
+               expect_payment_failed!(nodes[0], non_dust_hash, false);
        }
 }
 
@@ -9042,7 +9042,7 @@ fn test_htlc_no_detection() {
        let header_201 = BlockHeader { version: 0x20000000, prev_blockhash: nodes[0].best_block_hash(), merkle_root: TxMerkleNode::all_zeros(), time: 42, bits: 42, nonce: 42 };
        connect_block(&nodes[0], &Block { header: header_201, txdata: vec![htlc_timeout.clone()] });
        connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
-       expect_payment_failed!(nodes[0], our_payment_hash, true);
+       expect_payment_failed!(nodes[0], our_payment_hash, false);
 }
 
 fn do_test_onchain_htlc_settlement_after_close(broadcast_alice: bool, go_onchain_before_fulfill: bool) {

diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs
index a2fccbbc3883d6bb1e4d7f97ff89021b979483be..58c2e526f8e439df0b18dc82d7dca464e4728acd 100644 (file)
--- a/lightning/src/ln/monitor_tests.rs
+++ b/lightning/src/ln/monitor_tests.rs
@@ -141,7 +141,7 @@ fn revoked_output_htlc_resolution_timing() {
        assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 
        connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
-       expect_payment_failed!(nodes[1], payment_hash_1, true);
+       expect_payment_failed!(nodes[1], payment_hash_1, false);
 }
 
 #[test]
@@ -429,7 +429,7 @@ fn do_test_claim_value_force_close(prev_commitment_tx: bool) {
                sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances()));
 
        connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
-       expect_payment_failed!(nodes[0], dust_payment_hash, true);
+       expect_payment_failed!(nodes[0], dust_payment_hash, false);
        connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
 
        // After ANTI_REORG_DELAY, A will consider its balance fully spendable and generate a
@@ -509,7 +509,7 @@ fn do_test_claim_value_force_close(prev_commitment_tx: bool) {
        connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
        assert_eq!(Vec::<Balance>::new(),
                nodes[0].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances());
-       expect_payment_failed!(nodes[0], timeout_payment_hash, true);
+       expect_payment_failed!(nodes[0], timeout_payment_hash, false);
 
        test_spendable_output(&nodes[0], &a_broadcast_txn[2]);
 
@@ -724,7 +724,7 @@ fn test_balances_on_local_commitment_htlcs() {
        // panicked as described in the test introduction. This will remove the "maybe claimable"
        // spendable output as nodes[1] has fully claimed the second HTLC.
        connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
-       expect_payment_failed!(nodes[0], payment_hash, true);
+       expect_payment_failed!(nodes[0], payment_hash, false);
 
        assert_eq!(sorted_vec(vec![Balance::ClaimableAwaitingConfirmations {
                        claimable_amount_satoshis: 1_000_000 - 10_000 - 20_000 - chan_feerate *
@@ -923,7 +923,7 @@ fn test_no_preimage_inbound_htlc_balances() {
        // Once as_htlc_timeout_claim[0] reaches ANTI_REORG_DELAY confirmations, we should get a
        // payment failure event.
        connect_blocks(&nodes[0], ANTI_REORG_DELAY - 2);
-       expect_payment_failed!(nodes[0], to_b_failed_payment_hash, true);
+       expect_payment_failed!(nodes[0], to_b_failed_payment_hash, false);
 
        connect_blocks(&nodes[0], 1);
        assert_eq!(sorted_vec(vec![Balance::ClaimableAwaitingConfirmations {
@@ -972,7 +972,7 @@ fn test_no_preimage_inbound_htlc_balances() {
                sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances()));
 
        connect_blocks(&nodes[1], ANTI_REORG_DELAY - 2);
-       expect_payment_failed!(nodes[1], to_a_failed_payment_hash, true);
+       expect_payment_failed!(nodes[1], to_a_failed_payment_hash, false);
 
        assert_eq!(vec![Balance::MaybePreimageClaimableHTLC {
                        claimable_amount_satoshis: 10_000,
@@ -1216,21 +1216,21 @@ fn do_test_revoked_counterparty_commitment_balances(confirm_htlc_spend_first: bo
 
        let mut payment_failed_events = nodes[1].node.get_and_clear_pending_events();
        expect_payment_failed_conditions_event(&nodes[1], payment_failed_events.pop().unwrap(),
-               dust_payment_hash, true, PaymentFailedConditions::new());
+               dust_payment_hash, false, PaymentFailedConditions::new());
        expect_payment_failed_conditions_event(&nodes[1], payment_failed_events.pop().unwrap(),
-               missing_htlc_payment_hash, true, PaymentFailedConditions::new());
+               missing_htlc_payment_hash, false, PaymentFailedConditions::new());
        assert!(payment_failed_events.is_empty());
 
        connect_blocks(&nodes[1], 1);
        test_spendable_output(&nodes[1], &claim_txn[if confirm_htlc_spend_first { 2 } else { 3 }]);
        connect_blocks(&nodes[1], 1);
        test_spendable_output(&nodes[1], &claim_txn[if confirm_htlc_spend_first { 3 } else { 2 }]);
-       expect_payment_failed!(nodes[1], live_payment_hash, true);
+       expect_payment_failed!(nodes[1], live_payment_hash, false);
        connect_blocks(&nodes[1], 1);
        test_spendable_output(&nodes[1], &claim_txn[0]);
        connect_blocks(&nodes[1], 1);
        test_spendable_output(&nodes[1], &claim_txn[1]);
-       expect_payment_failed!(nodes[1], timeout_payment_hash, true);
+       expect_payment_failed!(nodes[1], timeout_payment_hash, false);
        assert_eq!(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances(), Vec::new());
 }
 
@@ -1626,7 +1626,7 @@ fn test_revoked_counterparty_aggregated_claims() {
        assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); // We shouldn't fail the payment until we spend the output
 
        connect_blocks(&nodes[1], 5);
-       expect_payment_failed!(nodes[1], revoked_payment_hash, true);
+       expect_payment_failed!(nodes[1], revoked_payment_hash, false);
        test_spendable_output(&nodes[1], &claim_txn_2[0]);
        assert!(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances().is_empty());
 }

diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs
index 315f76635c2bc77a4cafc8350aefa7cf7cd4dfd1..6fd1c2dc2eb848cab8a6571807f19aee28194a8b 100644 (file)
--- a/lightning/src/ln/payment_tests.rs
+++ b/lightning/src/ln/payment_tests.rs
@@ -894,7 +894,7 @@ fn do_test_dup_htlc_onchain_fails_on_reload(persist_manager_post_event: bool, co
                nodes[0].chain_monitor.chain_monitor.channel_monitor_updated(funding_txo, update).unwrap();
        }
        if payment_timeout {
-               expect_payment_failed!(nodes[0], payment_hash, true);
+               expect_payment_failed!(nodes[0], payment_hash, false);
        } else {
                expect_payment_sent!(nodes[0], payment_preimage);
        }
@@ -938,7 +938,7 @@ fn do_test_dup_htlc_onchain_fails_on_reload(persist_manager_post_event: bool, co
        if persist_manager_post_event {
                assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
        } else if payment_timeout {
-               expect_payment_failed!(nodes[0], payment_hash, true);
+               expect_payment_failed!(nodes[0], payment_hash, false);
        } else {
                expect_payment_sent!(nodes[0], payment_preimage);
        }

diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs
index e4b916c9345d59c7aecd92ab536541a042534ad3..0440e2135144c6efe27a53e12c0ab6d3ac836011 100644 (file)
--- a/lightning/src/ln/reorg_tests.rs
+++ b/lightning/src/ln/reorg_tests.rs
@@ -256,7 +256,7 @@ fn test_counterparty_revoked_reorg() {
 
        // Connect blocks to confirm the unrevoked commitment transaction
        connect_blocks(&nodes[1], ANTI_REORG_DELAY - 2);
-       expect_payment_failed!(nodes[1], payment_hash_4, true);
+       expect_payment_failed!(nodes[1], payment_hash_4, false);
 }
 
 fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_unconfirmed: bool, connect_style: ConnectStyle) {