From f76f60ff855ff286fc5c71704ea74430cdfb7d86 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 7 Sep 2022 20:02:04 +0000 Subject: [PATCH 1/2] 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 3b83726958b..6f8ba7b39a4 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 c41d5402ff3..826a2c31aa9 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 a2fccbbc388..58c2e526f8e 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 315f76635c2..6fd1c2dc2eb 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 e4b916c9345..0440e213514 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) { From c57bb42204a569fce5fb90d7f1b1367320124ae1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 7 Sep 2022 20:58:05 +0000 Subject: [PATCH 2/2] Rename `rejected_by_dest` -> `payment_failed_permanently` The `rejected_by_dest` field of the `PaymentPathFailed` event has always been a bit of a misnomer, as its really more about retry than where a payment failed. Now is as good a time as any to rename it. --- lightning-invoice/src/payment.rs | 40 +++++++++---------- lightning/src/ln/chanmon_update_fail_tests.rs | 4 +- lightning/src/ln/channelmanager.rs | 6 +-- lightning/src/ln/functional_test_utils.rs | 22 +++++----- lightning/src/ln/functional_tests.rs | 30 +++++++------- lightning/src/ln/onion_route_tests.rs | 4 +- lightning/src/routing/gossip.rs | 6 +-- lightning/src/util/events.rs | 12 +++--- 8 files changed, 62 insertions(+), 62 deletions(-) diff --git a/lightning-invoice/src/payment.rs b/lightning-invoice/src/payment.rs index e7cc43508d9..a97bf2fced4 100644 --- a/lightning-invoice/src/payment.rs +++ b/lightning-invoice/src/payment.rs @@ -717,7 +717,7 @@ where match event { Event::PaymentPathFailed { - payment_id, payment_hash, rejected_by_dest, path, short_channel_id, retry, .. + payment_id, payment_hash, payment_failed_permanently, path, short_channel_id, retry, .. } => { if let Some(short_channel_id) = short_channel_id { let path = path.iter().collect::>(); @@ -726,7 +726,7 @@ where if payment_id.is_none() { log_trace!(self.logger, "Payment {} has no id; not retrying", log_bytes!(payment_hash.0)); - } else if *rejected_by_dest { + } else if *payment_failed_permanently { log_trace!(self.logger, "Payment {} rejected by destination; not retrying", log_bytes!(payment_hash.0)); self.payer.abandon_payment(payment_id.unwrap()); } else if retry.is_none() { @@ -916,7 +916,7 @@ mod tests { payment_id, payment_hash, network_update: None, - rejected_by_dest: false, + payment_failed_permanently: false, all_paths_failed: false, path: TestRouter::path_for_value(final_value_msat), short_channel_id: None, @@ -981,7 +981,7 @@ mod tests { payment_id, payment_hash, network_update: None, - rejected_by_dest: false, + payment_failed_permanently: false, all_paths_failed: false, path: TestRouter::path_for_value(final_value_msat), short_channel_id: None, @@ -1028,7 +1028,7 @@ mod tests { payment_id, payment_hash: PaymentHash(invoice.payment_hash().clone().into_inner()), network_update: None, - rejected_by_dest: false, + payment_failed_permanently: false, all_paths_failed: true, path: TestRouter::path_for_value(final_value_msat), short_channel_id: None, @@ -1042,7 +1042,7 @@ mod tests { payment_id, payment_hash: PaymentHash(invoice.payment_hash().clone().into_inner()), network_update: None, - rejected_by_dest: false, + payment_failed_permanently: false, all_paths_failed: false, path: TestRouter::path_for_value(final_value_msat / 2), short_channel_id: None, @@ -1088,7 +1088,7 @@ mod tests { payment_id, payment_hash: PaymentHash(invoice.payment_hash().clone().into_inner()), network_update: None, - rejected_by_dest: false, + payment_failed_permanently: false, all_paths_failed: true, path: TestRouter::path_for_value(final_value_msat), short_channel_id: None, @@ -1128,7 +1128,7 @@ mod tests { payment_id, payment_hash: PaymentHash(invoice.payment_hash().clone().into_inner()), network_update: None, - rejected_by_dest: false, + payment_failed_permanently: false, all_paths_failed: false, path: vec![], short_channel_id: None, @@ -1189,7 +1189,7 @@ mod tests { payment_id, payment_hash: PaymentHash(invoice.payment_hash().clone().into_inner()), network_update: None, - rejected_by_dest: false, + payment_failed_permanently: false, all_paths_failed: false, path: vec![], short_channel_id: None, @@ -1226,7 +1226,7 @@ mod tests { payment_id, payment_hash: PaymentHash(invoice.payment_hash().clone().into_inner()), network_update: None, - rejected_by_dest: false, + payment_failed_permanently: false, all_paths_failed: false, path: TestRouter::path_for_value(final_value_msat / 2), short_channel_id: None, @@ -1260,7 +1260,7 @@ mod tests { payment_id, payment_hash: PaymentHash(invoice.payment_hash().clone().into_inner()), network_update: None, - rejected_by_dest: true, + payment_failed_permanently: true, all_paths_failed: false, path: vec![], short_channel_id: None, @@ -1309,7 +1309,7 @@ mod tests { payment_id, payment_hash, network_update: None, - rejected_by_dest: false, + payment_failed_permanently: false, all_paths_failed: false, path: vec![], short_channel_id: None, @@ -1444,7 +1444,7 @@ mod tests { payment_id, payment_hash, network_update: None, - rejected_by_dest: false, + payment_failed_permanently: false, all_paths_failed: false, path: vec![], short_channel_id: None, @@ -1490,7 +1490,7 @@ mod tests { payment_id, payment_hash, network_update: None, - rejected_by_dest: false, + payment_failed_permanently: false, all_paths_failed: false, path, short_channel_id, @@ -1680,7 +1680,7 @@ mod tests { payment_id, payment_hash, network_update: None, - rejected_by_dest: false, + payment_failed_permanently: false, all_paths_failed: false, path: TestRouter::path_for_value(final_value_msat), short_channel_id: None, @@ -1692,7 +1692,7 @@ mod tests { payment_id, payment_hash, network_update: None, - rejected_by_dest: false, + payment_failed_permanently: false, all_paths_failed: false, path: TestRouter::path_for_value(final_value_msat / 2), short_channel_id: None, @@ -2393,8 +2393,8 @@ mod tests { // treated this as "HTLC complete" and dropped the retry counter, causing us to retry again // if the final HTLC failed. expected_events.borrow_mut().push_back(&|ev: &Event| { - if let Event::PaymentPathFailed { rejected_by_dest, all_paths_failed, .. } = ev { - assert!(!rejected_by_dest); + if let Event::PaymentPathFailed { payment_failed_permanently, all_paths_failed, .. } = ev { + assert!(!payment_failed_permanently); assert!(all_paths_failed); } else { panic!("Unexpected event"); } }); @@ -2411,8 +2411,8 @@ mod tests { commitment_signed_dance!(nodes[0], nodes[1], &bs_fail_update.commitment_signed, false, true); expected_events.borrow_mut().push_back(&|ev: &Event| { - if let Event::PaymentPathFailed { rejected_by_dest, all_paths_failed, .. } = ev { - assert!(!rejected_by_dest); + if let Event::PaymentPathFailed { payment_failed_permanently, all_paths_failed, .. } = ev { + assert!(!payment_failed_permanently); assert!(all_paths_failed); } else { panic!("Unexpected event"); } }); diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index f5977abd4fc..794ac0db722 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -1744,9 +1744,9 @@ fn test_monitor_update_on_pending_forwards() { let events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 2); - if let Event::PaymentPathFailed { payment_hash, rejected_by_dest, .. } = events[0] { + if let Event::PaymentPathFailed { payment_hash, payment_failed_permanently, .. } = events[0] { assert_eq!(payment_hash, payment_hash_1); - assert!(rejected_by_dest); + assert!(payment_failed_permanently); } else { panic!("Unexpected event!"); } match events[1] { Event::PendingHTLCsForwardable { .. } => { }, diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 6f8ba7b39a4..2d1a3c7efdc 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3844,7 +3844,7 @@ impl ChannelMana pending_events.push(events::Event::PaymentPathFailed { payment_id: Some(payment_id), payment_hash, - rejected_by_dest: false, + payment_failed_permanently: false, network_update: None, all_paths_failed: payment.get().remaining_parts() == 0, path: path.clone(), @@ -3959,7 +3959,7 @@ impl ChannelMana events::Event::PaymentPathFailed { payment_id: Some(payment_id), payment_hash: payment_hash.clone(), - rejected_by_dest: !payment_retryable, + payment_failed_permanently: !payment_retryable, network_update, all_paths_failed, path: path.clone(), @@ -3990,7 +3990,7 @@ impl ChannelMana events::Event::PaymentPathFailed { payment_id: Some(payment_id), payment_hash: payment_hash.clone(), - rejected_by_dest: false, + payment_failed_permanently: false, network_update: None, all_paths_failed, path: path.clone(), diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 39efab5fd2d..11f61cad502 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1580,9 +1580,9 @@ impl<'a> PaymentFailedConditions<'a> { #[cfg(test)] macro_rules! expect_payment_failed_with_update { - ($node: expr, $expected_payment_hash: expr, $rejected_by_dest: expr, $scid: expr, $chan_closed: expr) => { + ($node: expr, $expected_payment_hash: expr, $payment_failed_permanently: expr, $scid: expr, $chan_closed: expr) => { $crate::ln::functional_test_utils::expect_payment_failed_conditions( - &$node, $expected_payment_hash, $rejected_by_dest, + &$node, $expected_payment_hash, $payment_failed_permanently, $crate::ln::functional_test_utils::PaymentFailedConditions::new() .blamed_scid($scid).blamed_chan_closed($chan_closed)); } @@ -1590,28 +1590,28 @@ macro_rules! expect_payment_failed_with_update { #[cfg(test)] macro_rules! expect_payment_failed { - ($node: expr, $expected_payment_hash: expr, $rejected_by_dest: expr $(, $expected_error_code: expr, $expected_error_data: expr)*) => { + ($node: expr, $expected_payment_hash: expr, $payment_failed_permanently: expr $(, $expected_error_code: expr, $expected_error_data: expr)*) => { #[allow(unused_mut)] let mut conditions = $crate::ln::functional_test_utils::PaymentFailedConditions::new(); $( conditions = conditions.expected_htlc_error_data($expected_error_code, &$expected_error_data); )* - $crate::ln::functional_test_utils::expect_payment_failed_conditions(&$node, $expected_payment_hash, $rejected_by_dest, conditions); + $crate::ln::functional_test_utils::expect_payment_failed_conditions(&$node, $expected_payment_hash, $payment_failed_permanently, conditions); }; } pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>( node: &'a Node<'b, 'c, 'd>, payment_failed_event: Event, expected_payment_hash: PaymentHash, - expected_rejected_by_dest: bool, conditions: PaymentFailedConditions<'e> + expected_payment_failed_permanently: bool, conditions: PaymentFailedConditions<'e> ) { let expected_payment_id = match payment_failed_event { - Event::PaymentPathFailed { payment_hash, rejected_by_dest, path, retry, payment_id, network_update, short_channel_id, + Event::PaymentPathFailed { payment_hash, payment_failed_permanently, path, retry, payment_id, network_update, short_channel_id, #[cfg(test)] error_code, #[cfg(test)] error_data, .. } => { assert_eq!(payment_hash, expected_payment_hash, "unexpected payment_hash"); - assert_eq!(rejected_by_dest, expected_rejected_by_dest, "unexpected rejected_by_dest value"); + assert_eq!(payment_failed_permanently, expected_payment_failed_permanently, "unexpected payment_failed_permanently value"); assert!(retry.is_some(), "expected retry.is_some()"); assert_eq!(retry.as_ref().unwrap().final_value_msat, path.last().unwrap().fee_msat, "Retry amount should match last hop in path"); assert_eq!(retry.as_ref().unwrap().payment_params.payee_pubkey, path.last().unwrap().pubkey, "Retry payee node_id should match last hop in path"); @@ -1668,12 +1668,12 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>( } pub fn expect_payment_failed_conditions<'a, 'b, 'c, 'd, 'e>( - node: &'a Node<'b, 'c, 'd>, expected_payment_hash: PaymentHash, expected_rejected_by_dest: bool, + node: &'a Node<'b, 'c, 'd>, expected_payment_hash: PaymentHash, expected_payment_failed_permanently: bool, conditions: PaymentFailedConditions<'e> ) { let mut events = node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); - expect_payment_failed_conditions_event(node, events.pop().unwrap(), expected_payment_hash, expected_rejected_by_dest, conditions); + expect_payment_failed_conditions_event(node, events.pop().unwrap(), expected_payment_hash, expected_payment_failed_permanently, conditions); } pub fn send_along_route_with_secret<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, route: Route, expected_paths: &[&[&Node<'a, 'b, 'c>]], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: PaymentSecret) -> PaymentId { @@ -2017,9 +2017,9 @@ pub fn pass_failed_payment_back<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expe let events = origin_node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); let expected_payment_id = match events[0] { - Event::PaymentPathFailed { payment_hash, rejected_by_dest, all_paths_failed, ref path, ref payment_id, .. } => { + Event::PaymentPathFailed { payment_hash, payment_failed_permanently, all_paths_failed, ref path, ref payment_id, .. } => { assert_eq!(payment_hash, our_payment_hash); - assert!(rejected_by_dest); + assert!(payment_failed_permanently); assert_eq!(all_paths_failed, i == expected_paths.len() - 1); for (idx, hop) in expected_route.iter().enumerate() { assert_eq!(hop.node.get_our_node_id(), path[idx].pubkey); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 826a2c31aa9..214e7cdad52 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -3266,7 +3266,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use let events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 3); match events[0] { - Event::PaymentPathFailed { ref payment_hash, rejected_by_dest: _, ref network_update, .. } => { + Event::PaymentPathFailed { ref payment_hash, ref network_update, .. } => { assert!(failed_htlcs.insert(payment_hash.0)); // If we delivered B's RAA we got an unknown preimage error, not something // that we should update our routing table for. @@ -3277,14 +3277,14 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use _ => panic!("Unexpected event"), } match events[1] { - Event::PaymentPathFailed { ref payment_hash, rejected_by_dest: _, ref network_update, .. } => { + Event::PaymentPathFailed { ref payment_hash, ref network_update, .. } => { assert!(failed_htlcs.insert(payment_hash.0)); assert!(network_update.is_some()); }, _ => panic!("Unexpected event"), } match events[2] { - Event::PaymentPathFailed { ref payment_hash, rejected_by_dest: _, ref network_update, .. } => { + Event::PaymentPathFailed { ref payment_hash, ref network_update, .. } => { assert!(failed_htlcs.insert(payment_hash.0)); assert!(network_update.is_some()); }, @@ -3614,9 +3614,9 @@ fn test_simple_peer_disconnect() { _ => panic!("Unexpected event"), } match events[1] { - Event::PaymentPathFailed { payment_hash, rejected_by_dest, .. } => { + Event::PaymentPathFailed { payment_hash, payment_failed_permanently, .. } => { assert_eq!(payment_hash, payment_hash_5); - assert!(rejected_by_dest); + assert!(payment_failed_permanently); }, _ => panic!("Unexpected event"), } @@ -5715,12 +5715,12 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno let mut as_failds = HashSet::new(); let mut as_updates = 0; for event in as_events.iter() { - if let &Event::PaymentPathFailed { ref payment_hash, ref rejected_by_dest, ref network_update, .. } = event { + if let &Event::PaymentPathFailed { ref payment_hash, ref payment_failed_permanently, ref network_update, .. } = event { assert!(as_failds.insert(*payment_hash)); if *payment_hash != payment_hash_2 { - assert_eq!(*rejected_by_dest, deliver_last_raa); + assert_eq!(*payment_failed_permanently, deliver_last_raa); } else { - assert!(!rejected_by_dest); + assert!(!payment_failed_permanently); } if network_update.is_some() { as_updates += 1; @@ -5740,12 +5740,12 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno let mut bs_failds = HashSet::new(); let mut bs_updates = 0; for event in bs_events.iter() { - if let &Event::PaymentPathFailed { ref payment_hash, ref rejected_by_dest, ref network_update, .. } = event { + if let &Event::PaymentPathFailed { ref payment_hash, ref payment_failed_permanently, ref network_update, .. } = event { assert!(bs_failds.insert(*payment_hash)); if *payment_hash != payment_hash_1 && *payment_hash != payment_hash_5 { - assert_eq!(*rejected_by_dest, deliver_last_raa); + assert_eq!(*payment_failed_permanently, deliver_last_raa); } else { - assert!(!rejected_by_dest); + assert!(!payment_failed_permanently); } if network_update.is_some() { bs_updates += 1; @@ -6264,10 +6264,10 @@ fn test_fail_holding_cell_htlc_upon_free() { let events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match &events[0] { - &Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref rejected_by_dest, ref network_update, ref all_paths_failed, ref short_channel_id, ref error_code, ref error_data, .. } => { + &Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref all_paths_failed, ref short_channel_id, ref error_code, ref error_data, .. } => { assert_eq!(our_payment_id, *payment_id.as_ref().unwrap()); assert_eq!(our_payment_hash.clone(), *payment_hash); - assert_eq!(*rejected_by_dest, false); + assert_eq!(*payment_failed_permanently, false); assert_eq!(*all_paths_failed, true); assert_eq!(*network_update, None); assert_eq!(*short_channel_id, None); @@ -6350,10 +6350,10 @@ fn test_free_and_fail_holding_cell_htlcs() { let events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match &events[0] { - &Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref rejected_by_dest, ref network_update, ref all_paths_failed, ref short_channel_id, ref error_code, ref error_data, .. } => { + &Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref all_paths_failed, ref short_channel_id, ref error_code, ref error_data, .. } => { assert_eq!(payment_id_2, *payment_id.as_ref().unwrap()); assert_eq!(payment_hash_2.clone(), *payment_hash); - assert_eq!(*rejected_by_dest, false); + assert_eq!(*payment_failed_permanently, false); assert_eq!(*all_paths_failed, true); assert_eq!(*network_update, None); assert_eq!(*short_channel_id, None); diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index e2c6432337f..4223b4cba06 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -167,8 +167,8 @@ fn run_onion_failure_test_with_fail_intercept(_name: &str, test_case: let events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); - if let &Event::PaymentPathFailed { ref rejected_by_dest, ref network_update, ref all_paths_failed, ref short_channel_id, ref error_code, .. } = &events[0] { - assert_eq!(*rejected_by_dest, !expected_retryable); + if let &Event::PaymentPathFailed { ref payment_failed_permanently, ref network_update, ref all_paths_failed, ref short_channel_id, ref error_code, .. } = &events[0] { + assert_eq!(*payment_failed_permanently, !expected_retryable); assert_eq!(*all_paths_failed, true); assert_eq!(*error_code, expected_error_code); if expected_channel_update.is_some() { diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index 637a8c046ed..28067838f78 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -2279,7 +2279,7 @@ mod tests { network_graph.handle_event(&Event::PaymentPathFailed { payment_id: None, payment_hash: PaymentHash([0; 32]), - rejected_by_dest: false, + payment_failed_permanently: false, all_paths_failed: true, path: vec![], network_update: Some(NetworkUpdate::ChannelUpdateMessage { @@ -2306,7 +2306,7 @@ mod tests { network_graph.handle_event(&Event::PaymentPathFailed { payment_id: None, payment_hash: PaymentHash([0; 32]), - rejected_by_dest: false, + payment_failed_permanently: false, all_paths_failed: true, path: vec![], network_update: Some(NetworkUpdate::ChannelFailure { @@ -2331,7 +2331,7 @@ mod tests { network_graph.handle_event(&Event::PaymentPathFailed { payment_id: None, payment_hash: PaymentHash([0; 32]), - rejected_by_dest: false, + payment_failed_permanently: false, all_paths_failed: true, path: vec![], network_update: Some(NetworkUpdate::ChannelFailure { diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index e86eae3c813..47e0c88f549 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -376,7 +376,7 @@ pub enum Event { /// Indicates the payment was rejected for some reason by the recipient. This implies that /// the payment has failed, not just the route in question. If this is not set, you may /// retry the payment via a different route. - rejected_by_dest: bool, + payment_failed_permanently: bool, /// Any failure information conveyed via the Onion return packet by a node along the failed /// payment route. /// @@ -643,7 +643,7 @@ impl Writeable for Event { }); }, &Event::PaymentPathFailed { - ref payment_id, ref payment_hash, ref rejected_by_dest, ref network_update, + ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref all_paths_failed, ref path, ref short_channel_id, ref retry, #[cfg(test)] ref error_code, @@ -658,7 +658,7 @@ impl Writeable for Event { write_tlv_fields!(writer, { (0, payment_hash, required), (1, network_update, option), - (2, rejected_by_dest, required), + (2, payment_failed_permanently, required), (3, all_paths_failed, required), (5, path, vec_type), (7, short_channel_id, option), @@ -827,7 +827,7 @@ impl MaybeReadable for Event { #[cfg(test)] let error_data = Readable::read(reader)?; let mut payment_hash = PaymentHash([0; 32]); - let mut rejected_by_dest = false; + let mut payment_failed_permanently = false; let mut network_update = None; let mut all_paths_failed = Some(true); let mut path: Option> = Some(vec![]); @@ -837,7 +837,7 @@ impl MaybeReadable for Event { read_tlv_fields!(reader, { (0, payment_hash, required), (1, network_update, ignorable), - (2, rejected_by_dest, required), + (2, payment_failed_permanently, required), (3, all_paths_failed, option), (5, path, vec_type), (7, short_channel_id, option), @@ -847,7 +847,7 @@ impl MaybeReadable for Event { Ok(Some(Event::PaymentPathFailed { payment_id, payment_hash, - rejected_by_dest, + payment_failed_permanently, network_update, all_paths_failed: all_paths_failed.unwrap(), path: path.unwrap(),