From bb4ff74da13dcfded20d29aff7d0f8d4afd5505d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 5 Oct 2021 19:10:42 +0000 Subject: [PATCH 1/3] Move tests of payment retries into a new module --- lightning/src/ln/functional_tests.rs | 198 ----------------------- lightning/src/ln/mod.rs | 3 + lightning/src/ln/payment_tests.rs | 227 +++++++++++++++++++++++++++ 3 files changed, 230 insertions(+), 198 deletions(-) create mode 100644 lightning/src/ln/payment_tests.rs diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index be6de19463c..30652e47864 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -4183,204 +4183,6 @@ fn mpp_failure() { fail_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, payment_hash); } -#[test] -fn mpp_retry() { - let chanmon_cfgs = create_chanmon_cfgs(4); - let node_cfgs = create_node_cfgs(4, &chanmon_cfgs); - let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]); - let nodes = create_network(4, &node_cfgs, &node_chanmgrs); - - let chan_1_id = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; - let chan_2_id = create_announced_chan_between_nodes(&nodes, 0, 2, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; - let chan_3_id = create_announced_chan_between_nodes(&nodes, 1, 3, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; - let chan_4_id = create_announced_chan_between_nodes(&nodes, 3, 2, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; - let logger = test_utils::TestLogger::new(); - // Rebalance - send_payment(&nodes[3], &vec!(&nodes[2])[..], 1_500_000); - - let (payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash!(&nodes[3]); - let net_graph_msg_handler = &nodes[0].net_graph_msg_handler; - let mut route = get_route(&nodes[0].node.get_our_node_id(), &net_graph_msg_handler.network_graph, &nodes[3].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &[], 1_000_000, TEST_FINAL_CLTV, &logger).unwrap(); - let path = route.paths[0].clone(); - route.paths.push(path); - route.paths[0][0].pubkey = nodes[1].node.get_our_node_id(); - route.paths[0][0].short_channel_id = chan_1_id; - route.paths[0][1].short_channel_id = chan_3_id; - route.paths[1][0].pubkey = nodes[2].node.get_our_node_id(); - route.paths[1][0].short_channel_id = chan_2_id; - route.paths[1][1].short_channel_id = chan_4_id; - - // Initiate the MPP payment. - let payment_id = nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap(); - check_added_monitors!(nodes[0], 2); // one monitor per path - let mut events = nodes[0].node.get_and_clear_pending_msg_events(); - assert_eq!(events.len(), 2); - - // Pass half of the payment along the success path. - let success_path_msgs = events.remove(0); - pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], 2_000_000, payment_hash, Some(payment_secret), success_path_msgs, false, None); - - // Add the HTLC along the first hop. - let fail_path_msgs_1 = events.remove(0); - let (update_add, commitment_signed) = match fail_path_msgs_1 { - MessageSendEvent::UpdateHTLCs { node_id: _, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, ref update_fee, ref commitment_signed } } => { - assert_eq!(update_add_htlcs.len(), 1); - assert!(update_fail_htlcs.is_empty()); - assert!(update_fulfill_htlcs.is_empty()); - assert!(update_fail_malformed_htlcs.is_empty()); - assert!(update_fee.is_none()); - (update_add_htlcs[0].clone(), commitment_signed.clone()) - }, - _ => panic!("Unexpected event"), - }; - nodes[2].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add); - commitment_signed_dance!(nodes[2], nodes[0], commitment_signed, false); - - // Attempt to forward the payment and complete the 2nd path's failure. - expect_pending_htlcs_forwardable!(&nodes[2]); - expect_pending_htlcs_forwardable!(&nodes[2]); - let htlc_updates = get_htlc_update_msgs!(nodes[2], nodes[0].node.get_our_node_id()); - assert!(htlc_updates.update_add_htlcs.is_empty()); - assert_eq!(htlc_updates.update_fail_htlcs.len(), 1); - assert!(htlc_updates.update_fulfill_htlcs.is_empty()); - assert!(htlc_updates.update_fail_malformed_htlcs.is_empty()); - check_added_monitors!(nodes[2], 1); - nodes[0].node.handle_update_fail_htlc(&nodes[2].node.get_our_node_id(), &htlc_updates.update_fail_htlcs[0]); - commitment_signed_dance!(nodes[0], nodes[2], htlc_updates.commitment_signed, false); - expect_payment_failed!(nodes[0], payment_hash, false); - - // Rebalance the channel so the second half of the payment can succeed. - send_payment(&nodes[3], &vec!(&nodes[2])[..], 1_500_000); - - // Make sure it errors as expected given a too-large amount. - if let Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { err })) = nodes[0].node.retry_payment(&route, payment_id) { - assert!(err.contains("over total_payment_amt_msat")); - } else { panic!("Unexpected error"); } - - // Make sure it errors as expected given the wrong payment_id. - if let Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { err })) = nodes[0].node.retry_payment(&route, PaymentId([0; 32])) { - assert!(err.contains("not found")); - } else { panic!("Unexpected error"); } - - // Retry the second half of the payment and make sure it succeeds. - let mut path = route.clone(); - path.paths.remove(0); - nodes[0].node.retry_payment(&path, payment_id).unwrap(); - check_added_monitors!(nodes[0], 1); - let mut events = nodes[0].node.get_and_clear_pending_msg_events(); - assert_eq!(events.len(), 1); - pass_along_path(&nodes[0], &[&nodes[2], &nodes[3]], 2_000_000, payment_hash, Some(payment_secret), events.pop().unwrap(), true, None); - claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, payment_preimage); -} - -#[test] -fn retry_single_path_payment() { - let chanmon_cfgs = create_chanmon_cfgs(3); - let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); - let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]); - let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); - - let _chan_0 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); - let _chan_1 = create_announced_chan_between_nodes(&nodes, 2, 1, InitFeatures::known(), InitFeatures::known()); - // Rebalance to find a route - send_payment(&nodes[2], &vec!(&nodes[1])[..], 3_000_000); - - let logger = test_utils::TestLogger::new(); - let (payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]); - let net_graph_msg_handler = &nodes[0].net_graph_msg_handler; - let route = get_route(&nodes[0].node.get_our_node_id(), &net_graph_msg_handler.network_graph, &nodes[2].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 100_000, TEST_FINAL_CLTV, &logger).unwrap(); - - // Rebalance so that the first hop fails. - send_payment(&nodes[1], &vec!(&nodes[2])[..], 2_000_000); - - // Make sure the payment fails on the first hop. - let payment_id = nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap(); - check_added_monitors!(nodes[0], 1); - let mut events = nodes[0].node.get_and_clear_pending_msg_events(); - assert_eq!(events.len(), 1); - let mut payment_event = SendEvent::from_event(events.pop().unwrap()); - nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]); - check_added_monitors!(nodes[1], 0); - commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false); - expect_pending_htlcs_forwardable!(nodes[1]); - expect_pending_htlcs_forwardable!(&nodes[1]); - let htlc_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); - assert!(htlc_updates.update_add_htlcs.is_empty()); - assert_eq!(htlc_updates.update_fail_htlcs.len(), 1); - assert!(htlc_updates.update_fulfill_htlcs.is_empty()); - assert!(htlc_updates.update_fail_malformed_htlcs.is_empty()); - check_added_monitors!(nodes[1], 1); - nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &htlc_updates.update_fail_htlcs[0]); - commitment_signed_dance!(nodes[0], nodes[1], htlc_updates.commitment_signed, false); - expect_payment_failed!(nodes[0], payment_hash, false); - - // Rebalance the channel so the retry succeeds. - send_payment(&nodes[2], &vec!(&nodes[1])[..], 3_000_000); - - // Mine two blocks (we expire retries after 3, so this will check that we don't expire early) - connect_blocks(&nodes[0], 2); - - // Retry the payment and make sure it succeeds. - nodes[0].node.retry_payment(&route, payment_id).unwrap(); - check_added_monitors!(nodes[0], 1); - let mut events = nodes[0].node.get_and_clear_pending_msg_events(); - assert_eq!(events.len(), 1); - pass_along_path(&nodes[0], &[&nodes[1], &nodes[2]], 100_000, payment_hash, Some(payment_secret), events.pop().unwrap(), true, None); - claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], false, payment_preimage); -} - -#[test] -fn retry_expired_payment() { - let chanmon_cfgs = create_chanmon_cfgs(3); - let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); - let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]); - let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); - - let _chan_0 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); - let _chan_1 = create_announced_chan_between_nodes(&nodes, 2, 1, InitFeatures::known(), InitFeatures::known()); - // Rebalance to find a route - send_payment(&nodes[2], &vec!(&nodes[1])[..], 3_000_000); - - let logger = test_utils::TestLogger::new(); - let (_payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]); - let net_graph_msg_handler = &nodes[0].net_graph_msg_handler; - let route = get_route(&nodes[0].node.get_our_node_id(), &net_graph_msg_handler.network_graph, &nodes[2].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 100_000, TEST_FINAL_CLTV, &logger).unwrap(); - - // Rebalance so that the first hop fails. - send_payment(&nodes[1], &vec!(&nodes[2])[..], 2_000_000); - - // Make sure the payment fails on the first hop. - let payment_id = nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap(); - check_added_monitors!(nodes[0], 1); - let mut events = nodes[0].node.get_and_clear_pending_msg_events(); - assert_eq!(events.len(), 1); - let mut payment_event = SendEvent::from_event(events.pop().unwrap()); - nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]); - check_added_monitors!(nodes[1], 0); - commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false); - expect_pending_htlcs_forwardable!(nodes[1]); - expect_pending_htlcs_forwardable!(&nodes[1]); - let htlc_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); - assert!(htlc_updates.update_add_htlcs.is_empty()); - assert_eq!(htlc_updates.update_fail_htlcs.len(), 1); - assert!(htlc_updates.update_fulfill_htlcs.is_empty()); - assert!(htlc_updates.update_fail_malformed_htlcs.is_empty()); - check_added_monitors!(nodes[1], 1); - nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &htlc_updates.update_fail_htlcs[0]); - commitment_signed_dance!(nodes[0], nodes[1], htlc_updates.commitment_signed, false); - expect_payment_failed!(nodes[0], payment_hash, false); - - // Mine blocks so the payment will have expired. - connect_blocks(&nodes[0], 3); - - // Retry the payment and make sure it errors as expected. - if let Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { err })) = nodes[0].node.retry_payment(&route, payment_id) { - assert!(err.contains("not found")); - } else { - panic!("Unexpected error"); - } -} - #[test] fn test_dup_htlc_onchain_fails_on_reload() { // When a Channel is closed, any outbound HTLCs which were relayed through it are simply diff --git a/lightning/src/ln/mod.rs b/lightning/src/ln/mod.rs index b5e433270a5..a2a0b4efee2 100644 --- a/lightning/src/ln/mod.rs +++ b/lightning/src/ln/mod.rs @@ -51,6 +51,9 @@ pub mod wire; mod functional_tests; #[cfg(test)] #[allow(unused_mut)] +mod payment_tests; +#[cfg(test)] +#[allow(unused_mut)] mod chanmon_update_fail_tests; #[cfg(test)] #[allow(unused_mut)] diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs new file mode 100644 index 00000000000..f79b5da51cd --- /dev/null +++ b/lightning/src/ln/payment_tests.rs @@ -0,0 +1,227 @@ +// This file is Copyright its original authors, visible in version control +// history. +// +// This file is licensed under the Apache License, Version 2.0 or the MIT license +// , at your option. +// You may not use this file except in accordance with one or both of these +// licenses. + +//! Tests that test the payment retry logic in ChannelManager, including various edge-cases around +//! serialization ordering between ChannelManager/ChannelMonitors and ensuring we can still retry +//! payments thereafter. + +use ln::{PaymentPreimage, PaymentHash}; +use ln::channelmanager::{PaymentId, PaymentSendFailure}; +use routing::router::get_route; +use ln::features::{InitFeatures, InvoiceFeatures}; +use ln::msgs; +use ln::msgs::ChannelMessageHandler; +use util::test_utils; +use util::events::{Event, MessageSendEvent, MessageSendEventsProvider}; +use util::errors::APIError; + +use bitcoin::hashes::sha256::Hash as Sha256; +use bitcoin::hashes::Hash; + +use prelude::*; + +use ln::functional_test_utils::*; + +#[test] +fn retry_single_path_payment() { + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]); + let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + let _chan_0 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); + let _chan_1 = create_announced_chan_between_nodes(&nodes, 2, 1, InitFeatures::known(), InitFeatures::known()); + // Rebalance to find a route + send_payment(&nodes[2], &vec!(&nodes[1])[..], 3_000_000); + + let logger = test_utils::TestLogger::new(); + let (payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]); + let net_graph_msg_handler = &nodes[0].net_graph_msg_handler; + let route = get_route(&nodes[0].node.get_our_node_id(), &net_graph_msg_handler.network_graph, &nodes[2].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 100_000, TEST_FINAL_CLTV, &logger).unwrap(); + + // Rebalance so that the first hop fails. + send_payment(&nodes[1], &vec!(&nodes[2])[..], 2_000_000); + + // Make sure the payment fails on the first hop. + let payment_id = nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap(); + check_added_monitors!(nodes[0], 1); + let mut events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 1); + let mut payment_event = SendEvent::from_event(events.pop().unwrap()); + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]); + check_added_monitors!(nodes[1], 0); + commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false); + expect_pending_htlcs_forwardable!(nodes[1]); + expect_pending_htlcs_forwardable!(&nodes[1]); + let htlc_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + assert!(htlc_updates.update_add_htlcs.is_empty()); + assert_eq!(htlc_updates.update_fail_htlcs.len(), 1); + assert!(htlc_updates.update_fulfill_htlcs.is_empty()); + assert!(htlc_updates.update_fail_malformed_htlcs.is_empty()); + check_added_monitors!(nodes[1], 1); + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &htlc_updates.update_fail_htlcs[0]); + commitment_signed_dance!(nodes[0], nodes[1], htlc_updates.commitment_signed, false); + expect_payment_failed!(nodes[0], payment_hash, false); + + // Rebalance the channel so the retry succeeds. + send_payment(&nodes[2], &vec!(&nodes[1])[..], 3_000_000); + + // Mine two blocks (we expire retries after 3, so this will check that we don't expire early) + connect_blocks(&nodes[0], 2); + + // Retry the payment and make sure it succeeds. + nodes[0].node.retry_payment(&route, payment_id).unwrap(); + check_added_monitors!(nodes[0], 1); + let mut events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 1); + pass_along_path(&nodes[0], &[&nodes[1], &nodes[2]], 100_000, payment_hash, Some(payment_secret), events.pop().unwrap(), true, None); + claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], false, payment_preimage); +} + +#[test] +fn mpp_retry() { + let chanmon_cfgs = create_chanmon_cfgs(4); + let node_cfgs = create_node_cfgs(4, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]); + let nodes = create_network(4, &node_cfgs, &node_chanmgrs); + + let chan_1_id = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; + let chan_2_id = create_announced_chan_between_nodes(&nodes, 0, 2, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; + let chan_3_id = create_announced_chan_between_nodes(&nodes, 1, 3, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; + let chan_4_id = create_announced_chan_between_nodes(&nodes, 3, 2, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; + let logger = test_utils::TestLogger::new(); + // Rebalance + send_payment(&nodes[3], &vec!(&nodes[2])[..], 1_500_000); + + let (payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash!(&nodes[3]); + let net_graph_msg_handler = &nodes[0].net_graph_msg_handler; + let mut route = get_route(&nodes[0].node.get_our_node_id(), &net_graph_msg_handler.network_graph, &nodes[3].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &[], 1_000_000, TEST_FINAL_CLTV, &logger).unwrap(); + let path = route.paths[0].clone(); + route.paths.push(path); + route.paths[0][0].pubkey = nodes[1].node.get_our_node_id(); + route.paths[0][0].short_channel_id = chan_1_id; + route.paths[0][1].short_channel_id = chan_3_id; + route.paths[1][0].pubkey = nodes[2].node.get_our_node_id(); + route.paths[1][0].short_channel_id = chan_2_id; + route.paths[1][1].short_channel_id = chan_4_id; + + // Initiate the MPP payment. + let payment_id = nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap(); + check_added_monitors!(nodes[0], 2); // one monitor per path + let mut events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 2); + + // Pass half of the payment along the success path. + let success_path_msgs = events.remove(0); + pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], 2_000_000, payment_hash, Some(payment_secret), success_path_msgs, false, None); + + // Add the HTLC along the first hop. + let fail_path_msgs_1 = events.remove(0); + let (update_add, commitment_signed) = match fail_path_msgs_1 { + MessageSendEvent::UpdateHTLCs { node_id: _, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, ref update_fee, ref commitment_signed } } => { + assert_eq!(update_add_htlcs.len(), 1); + assert!(update_fail_htlcs.is_empty()); + assert!(update_fulfill_htlcs.is_empty()); + assert!(update_fail_malformed_htlcs.is_empty()); + assert!(update_fee.is_none()); + (update_add_htlcs[0].clone(), commitment_signed.clone()) + }, + _ => panic!("Unexpected event"), + }; + nodes[2].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add); + commitment_signed_dance!(nodes[2], nodes[0], commitment_signed, false); + + // Attempt to forward the payment and complete the 2nd path's failure. + expect_pending_htlcs_forwardable!(&nodes[2]); + expect_pending_htlcs_forwardable!(&nodes[2]); + let htlc_updates = get_htlc_update_msgs!(nodes[2], nodes[0].node.get_our_node_id()); + assert!(htlc_updates.update_add_htlcs.is_empty()); + assert_eq!(htlc_updates.update_fail_htlcs.len(), 1); + assert!(htlc_updates.update_fulfill_htlcs.is_empty()); + assert!(htlc_updates.update_fail_malformed_htlcs.is_empty()); + check_added_monitors!(nodes[2], 1); + nodes[0].node.handle_update_fail_htlc(&nodes[2].node.get_our_node_id(), &htlc_updates.update_fail_htlcs[0]); + commitment_signed_dance!(nodes[0], nodes[2], htlc_updates.commitment_signed, false); + expect_payment_failed!(nodes[0], payment_hash, false); + + // Rebalance the channel so the second half of the payment can succeed. + send_payment(&nodes[3], &vec!(&nodes[2])[..], 1_500_000); + + // Make sure it errors as expected given a too-large amount. + if let Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { err })) = nodes[0].node.retry_payment(&route, payment_id) { + assert!(err.contains("over total_payment_amt_msat")); + } else { panic!("Unexpected error"); } + + // Make sure it errors as expected given the wrong payment_id. + if let Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { err })) = nodes[0].node.retry_payment(&route, PaymentId([0; 32])) { + assert!(err.contains("not found")); + } else { panic!("Unexpected error"); } + + // Retry the second half of the payment and make sure it succeeds. + let mut path = route.clone(); + path.paths.remove(0); + nodes[0].node.retry_payment(&path, payment_id).unwrap(); + check_added_monitors!(nodes[0], 1); + let mut events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 1); + pass_along_path(&nodes[0], &[&nodes[2], &nodes[3]], 2_000_000, payment_hash, Some(payment_secret), events.pop().unwrap(), true, None); + claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, payment_preimage); +} + +#[test] +fn retry_expired_payment() { + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]); + let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + let _chan_0 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); + let _chan_1 = create_announced_chan_between_nodes(&nodes, 2, 1, InitFeatures::known(), InitFeatures::known()); + // Rebalance to find a route + send_payment(&nodes[2], &vec!(&nodes[1])[..], 3_000_000); + + let logger = test_utils::TestLogger::new(); + let (_payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]); + let net_graph_msg_handler = &nodes[0].net_graph_msg_handler; + let route = get_route(&nodes[0].node.get_our_node_id(), &net_graph_msg_handler.network_graph, &nodes[2].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 100_000, TEST_FINAL_CLTV, &logger).unwrap(); + + // Rebalance so that the first hop fails. + send_payment(&nodes[1], &vec!(&nodes[2])[..], 2_000_000); + + // Make sure the payment fails on the first hop. + let payment_id = nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap(); + check_added_monitors!(nodes[0], 1); + let mut events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 1); + let mut payment_event = SendEvent::from_event(events.pop().unwrap()); + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]); + check_added_monitors!(nodes[1], 0); + commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false); + expect_pending_htlcs_forwardable!(nodes[1]); + expect_pending_htlcs_forwardable!(&nodes[1]); + let htlc_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + assert!(htlc_updates.update_add_htlcs.is_empty()); + assert_eq!(htlc_updates.update_fail_htlcs.len(), 1); + assert!(htlc_updates.update_fulfill_htlcs.is_empty()); + assert!(htlc_updates.update_fail_malformed_htlcs.is_empty()); + check_added_monitors!(nodes[1], 1); + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &htlc_updates.update_fail_htlcs[0]); + commitment_signed_dance!(nodes[0], nodes[1], htlc_updates.commitment_signed, false); + expect_payment_failed!(nodes[0], payment_hash, false); + + // Mine blocks so the payment will have expired. + connect_blocks(&nodes[0], 3); + + // Retry the payment and make sure it errors as expected. + if let Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { err })) = nodes[0].node.retry_payment(&route, payment_id) { + assert!(err.contains("not found")); + } else { + panic!("Unexpected error"); + } +} From 928bfb12d2d0d02ae0b5a68a81a6b4a651561c69 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 4 Oct 2021 04:47:33 +0000 Subject: [PATCH 2/3] Move pending payment tracking to after the new HTLC flies If we attempt to send a payment, but the HTLC cannot be send due to local channel limits, we'll provide the user an error but end up with an entry in our pending payment map. This will result in a memory leak as we'll never reclaim the pending payment map entry. --- lightning/src/ln/channelmanager.rs | 44 ++++++++++++++++++------------ lightning/src/ln/payment_tests.rs | 27 ++++++++++++++++++ 2 files changed, 54 insertions(+), 17 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index b29199e8624..ad8603312fc 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -402,7 +402,7 @@ struct PendingInboundPayment { /// Stores the session_priv for each part of a payment that is still pending. For versions 0.0.102 /// and later, also stores information for retrying the payment. -enum PendingOutboundPayment { +pub(crate) enum PendingOutboundPayment { Legacy { session_privs: HashSet<[u8; 32]>, }, @@ -1951,16 +1951,6 @@ impl ChannelMana let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, prng_seed, payment_hash); let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier); - let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap(); - let payment = pending_outbounds.entry(payment_id).or_insert_with(|| PendingOutboundPayment::Retryable { - session_privs: HashSet::new(), - pending_amt_msat: 0, - payment_hash: *payment_hash, - payment_secret: *payment_secret, - starting_block_height: self.best_block.read().unwrap().height(), - total_msat: total_value, - }); - assert!(payment.insert(session_priv_bytes, path.last().unwrap().fee_msat)); let err: Result<(), _> = loop { let mut channel_lock = self.channel_state.lock().unwrap(); @@ -1978,12 +1968,27 @@ impl ChannelMana if !chan.get().is_live() { return Err(APIError::ChannelUnavailable{err: "Peer for first hop currently disconnected/pending monitor update!".to_owned()}); } - break_chan_entry!(self, chan.get_mut().send_htlc_and_commit(htlc_msat, payment_hash.clone(), htlc_cltv, HTLCSource::OutboundRoute { - path: path.clone(), - session_priv: session_priv.clone(), - first_hop_htlc_msat: htlc_msat, - payment_id, - }, onion_packet, &self.logger), channel_state, chan) + let send_res = break_chan_entry!(self, chan.get_mut().send_htlc_and_commit( + htlc_msat, payment_hash.clone(), htlc_cltv, HTLCSource::OutboundRoute { + path: path.clone(), + session_priv: session_priv.clone(), + first_hop_htlc_msat: htlc_msat, + payment_id, + }, onion_packet, &self.logger), + channel_state, chan); + + let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap(); + let payment = pending_outbounds.entry(payment_id).or_insert_with(|| PendingOutboundPayment::Retryable { + session_privs: HashSet::new(), + pending_amt_msat: 0, + payment_hash: *payment_hash, + payment_secret: *payment_secret, + starting_block_height: self.best_block.read().unwrap().height(), + total_msat: total_value, + }); + assert!(payment.insert(session_priv_bytes, path.last().unwrap().fee_msat)); + + send_res } { Some((update_add, commitment_signed, monitor_update)) => { if let Err(e) = self.chain_monitor.update_channel(chan.get().get_funding_txo().unwrap(), monitor_update) { @@ -4383,6 +4388,11 @@ impl ChannelMana self.process_pending_events(&event_handler); events.into_inner() } + + #[cfg(test)] + pub fn has_pending_payments(&self) -> bool { + !self.pending_outbound_payments.lock().unwrap().is_empty() + } } impl MessageSendEventsProvider for ChannelManager diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index f79b5da51cd..f8ff35f09ac 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -225,3 +225,30 @@ fn retry_expired_payment() { panic!("Unexpected error"); } } + +#[test] +fn no_pending_leak_on_initial_send_failure() { + // In an earlier version of our payment tracking, we'd have a retry entry even when the initial + // HTLC for payment failed to send due to local channel errors (e.g. peer disconnected). In this + // case, the user wouldn't have a PaymentId to retry the payment with, but we'd think we have a + // pending payment forever and never time it out. + // Here we test exactly that - retrying a payment when a peer was disconnected on the first + // try, and then check that no pending payment is being tracked. + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); + + let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], 100_000); + + nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false); + nodes[1].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false); + + unwrap_send_err!(nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)), + true, APIError::ChannelUnavailable { ref err }, + assert_eq!(err, "Peer for first hop currently disconnected/pending monitor update!")); + + assert!(!nodes[0].node.has_pending_payments()); +} From a58c617c4291076eb68d910ac59e577743acce8c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 5 Oct 2021 22:28:32 +0000 Subject: [PATCH 3/3] Correct error returned when `retry_payment` doesn't have a payment --- lightning/src/ln/channelmanager.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index ad8603312fc..152de61fd8e 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2178,7 +2178,7 @@ impl ChannelMana } } else { return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { - err: "Payment with ID {} not found".to_string() + err: format!("Payment with ID {} not found", log_bytes!(payment_id.0)), })) } };