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 93e645d 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.

Author: TheBlueMatt

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(),

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) {

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());
 }

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);
 	}

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) {