From f76f60ff855ff286fc5c71704ea74430cdfb7d86 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 7 Sep 2022 20:02:04 +0000 Subject: [PATCH] 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. --- lightning/src/ln/channelmanager.rs | 2 +- lightning/src/ln/functional_tests.rs | 20 ++++++++++---------- lightning/src/ln/monitor_tests.rs | 22 +++++++++++----------- lightning/src/ln/payment_tests.rs | 4 ++-- lightning/src/ln/reorg_tests.rs | 2 +- 5 files changed, 25 insertions(+), 25 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 3b8372695..6f8ba7b39 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3990,7 +3990,7 @@ impl 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 c41d5402f..826a2c31a 100644 --- 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 a2fccbbc3..58c2e526f 100644 --- 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::::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 315f76635..6fd1c2dc2 100644 --- 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 e4b916c93..0440e2135 100644 --- 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) { -- 2.39.5