Skip to content

Add test coverage for failure of inconsistent MPP parts #1451

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 29, 2022
Merged

Add test coverage for failure of inconsistent MPP parts #1451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b9d097b
Add a `get_route!()` macro for tests which only need a route
TheBlueMatt Apr 25, 2022
7a8344a
Add test coverage for failure of inconsistent MPP parts
TheBlueMatt Apr 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 18 additions & 19 deletions lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1167,6 +1167,21 @@ macro_rules! get_payment_preimage_hash {
}
}

#[macro_export]
macro_rules! get_route {
($send_node: expr, $payment_params: expr, $recv_value: expr, $cltv: expr) => {{
use $crate::chain::keysinterface::KeysInterface;
let scorer = $crate::util::test_utils::TestScorer::with_penalty(0);
let keys_manager = $crate::util::test_utils::TestKeysInterface::new(&[0u8; 32], bitcoin::network::constants::Network::Testnet);
let random_seed_bytes = keys_manager.get_secure_random_bytes();
$crate::routing::router::get_route(
&$send_node.node.get_our_node_id(), &$payment_params, &$send_node.network_graph.read_only(),
Some(&$send_node.node.list_usable_channels().iter().collect::<Vec<_>>()),
$recv_value, $cltv, $send_node.logger, &scorer, &random_seed_bytes
)
}}
}

#[cfg(test)]
#[macro_export]
macro_rules! get_route_and_payment_hash {
Expand All @@ -1176,17 +1191,9 @@ macro_rules! get_route_and_payment_hash {
$crate::get_route_and_payment_hash!($send_node, $recv_node, payment_params, $recv_value, TEST_FINAL_CLTV)
}};
($send_node: expr, $recv_node: expr, $payment_params: expr, $recv_value: expr, $cltv: expr) => {{
use $crate::chain::keysinterface::KeysInterface;
let (payment_preimage, payment_hash, payment_secret) = $crate::get_payment_preimage_hash!($recv_node, Some($recv_value));
let scorer = $crate::util::test_utils::TestScorer::with_penalty(0);
let keys_manager = $crate::util::test_utils::TestKeysInterface::new(&[0u8; 32], bitcoin::network::constants::Network::Testnet);
let random_seed_bytes = keys_manager.get_secure_random_bytes();
let route = $crate::routing::router::get_route(
&$send_node.node.get_our_node_id(), &$payment_params, &$send_node.network_graph.read_only(),
Some(&$send_node.node.list_usable_channels().iter().collect::<Vec<_>>()),
$recv_value, $cltv, $send_node.logger, &scorer, &random_seed_bytes
).unwrap();
(route, payment_hash, payment_preimage, payment_secret)
let route = $crate::get_route!($send_node, $payment_params, $recv_value, $cltv);
(route.unwrap(), payment_hash, payment_preimage, payment_secret)
}}
}

Expand Down Expand Up @@ -1650,15 +1657,7 @@ pub const TEST_FINAL_CLTV: u32 = 70;
pub fn route_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], recv_value: u64) -> (PaymentPreimage, PaymentHash, PaymentSecret) {
let payment_params = PaymentParameters::from_node_id(expected_route.last().unwrap().node.get_our_node_id())
.with_features(InvoiceFeatures::known());
let network_graph = origin_node.network_graph.read_only();
let scorer = test_utils::TestScorer::with_penalty(0);
let seed = [0u8; 32];
let keys_manager = test_utils::TestKeysInterface::new(&seed, Network::Testnet);
let random_seed_bytes = keys_manager.get_secure_random_bytes();
let route = get_route(
&origin_node.node.get_our_node_id(), &payment_params, &network_graph,
Some(&origin_node.node.list_usable_channels().iter().collect::<Vec<_>>()),
recv_value, TEST_FINAL_CLTV, origin_node.logger, &scorer, &random_seed_bytes).unwrap();
let route = get_route!(origin_node, payment_params, recv_value, TEST_FINAL_CLTV).unwrap();
assert_eq!(route.paths.len(), 1);
assert_eq!(route.paths[0].len(), expected_route.len());
for (node, hop) in expected_route.iter().zip(route.paths[0].iter()) {
Expand Down
171 changes: 144 additions & 27 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9435,12 +9435,7 @@ fn test_forwardable_regen() {
claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage_2);
}

#[test]
fn test_dup_htlc_second_fail_panic() {
// Previously, if we received two HTLCs back-to-back, where the second overran the expected
// value for the payment, we'd fail back both HTLCs after generating a `PaymentReceived` event.
// Then, if the user failed the second payment, they'd hit a "tried to fail an already failed
// HTLC" debug panic. This tests for this behavior, checking that only one HTLC is auto-failed.
fn do_test_dup_htlc_second_rejected(test_for_second_fail_panic: bool) {
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]);
Expand All @@ -9450,14 +9445,9 @@ fn test_dup_htlc_second_fail_panic() {

let payment_params = PaymentParameters::from_node_id(nodes[1].node.get_our_node_id())
.with_features(InvoiceFeatures::known());
let scorer = test_utils::TestScorer::with_penalty(0);
let random_seed_bytes = chanmon_cfgs[1].keys_manager.get_secure_random_bytes();
let route = get_route(
&nodes[0].node.get_our_node_id(), &payment_params, &nodes[0].network_graph.read_only(),
Some(&nodes[0].node.list_usable_channels().iter().collect::<Vec<_>>()),
10_000, TEST_FINAL_CLTV, nodes[0].logger, &scorer, &random_seed_bytes).unwrap();
let route = get_route!(nodes[0], payment_params, 10_000, TEST_FINAL_CLTV).unwrap();

let (_, our_payment_hash, our_payment_secret) = get_payment_preimage_hash!(&nodes[1]);
let (our_payment_preimage, our_payment_hash, our_payment_secret) = get_payment_preimage_hash!(&nodes[1]);

{
nodes[0].node.send_payment(&route, our_payment_hash, &Some(our_payment_secret)).unwrap();
Expand Down Expand Up @@ -9485,26 +9475,153 @@ fn test_dup_htlc_second_fail_panic() {
// the first HTLC delivered above.
}

// Now we go fail back the first HTLC from the user end.
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
nodes[1].node.process_pending_htlc_forwards();
nodes[1].node.fail_htlc_backwards(&our_payment_hash);

expect_pending_htlcs_forwardable_ignore!(nodes[1]);
nodes[1].node.process_pending_htlc_forwards();
if test_for_second_fail_panic {
// Now we go fail back the first HTLC from the user end.
nodes[1].node.fail_htlc_backwards(&our_payment_hash);

check_added_monitors!(nodes[1], 1);
let fail_updates_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
assert_eq!(fail_updates_1.update_fail_htlcs.len(), 2);
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
nodes[1].node.process_pending_htlc_forwards();

check_added_monitors!(nodes[1], 1);
let fail_updates_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
assert_eq!(fail_updates_1.update_fail_htlcs.len(), 2);

nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_updates_1.update_fail_htlcs[0]);
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_updates_1.update_fail_htlcs[1]);
commitment_signed_dance!(nodes[0], nodes[1], fail_updates_1.commitment_signed, false);

let failure_events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(failure_events.len(), 2);
if let Event::PaymentPathFailed { .. } = failure_events[0] {} else { panic!(); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it appears to me here that with the second fail we're actually not panicking. Is the boolean a misnomer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, its a bit confusing its really "test that the second fail doesnt panic". I force-pushed a rename to test_for_second_fail_panic to make it clearer.

if let Event::PaymentPathFailed { .. } = failure_events[1] {} else { panic!(); }
} else {
// Let the second HTLC fail and claim the first
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
nodes[1].node.process_pending_htlc_forwards();

check_added_monitors!(nodes[1], 1);
let fail_updates_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_updates_1.update_fail_htlcs[0]);
commitment_signed_dance!(nodes[0], nodes[1], fail_updates_1.commitment_signed, false);

expect_payment_failed_conditions!(nodes[0], our_payment_hash, true, PaymentFailedConditions::new().mpp_parts_remain());

claim_payment(&nodes[0], &[&nodes[1]], our_payment_preimage);
}
}

#[test]
fn test_dup_htlc_second_fail_panic() {
// Previously, if we received two HTLCs back-to-back, where the second overran the expected
// value for the payment, we'd fail back both HTLCs after generating a `PaymentReceived` event.
// Then, if the user failed the second payment, they'd hit a "tried to fail an already failed
// HTLC" debug panic. This tests for this behavior, checking that only one HTLC is auto-failed.
do_test_dup_htlc_second_rejected(true);
}

#[test]
fn test_dup_htlc_second_rejected() {
// Test that if we receive a second HTLC for an MPP payment that overruns the payment amount we
// simply reject the second HTLC but are still able to claim the first HTLC.
do_test_dup_htlc_second_rejected(false);
}

#[test]
fn test_inconsistent_mpp_params() {
// Test that if we recieve two HTLCs with different payment parameters we fail back the first
// such HTLC and allow the second to stay.
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);

create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 0, InitFeatures::known(), InitFeatures::known());
create_announced_chan_between_nodes_with_value(&nodes, 0, 2, 100_000, 0, InitFeatures::known(), InitFeatures::known());
create_announced_chan_between_nodes_with_value(&nodes, 1, 3, 100_000, 0, InitFeatures::known(), InitFeatures::known());
create_announced_chan_between_nodes_with_value(&nodes, 2, 3, 100_000, 0, InitFeatures::known(), InitFeatures::known());

let payment_params = PaymentParameters::from_node_id(nodes[3].node.get_our_node_id())
.with_features(InvoiceFeatures::known());
let mut route = get_route!(nodes[0], payment_params, 15_000_000, TEST_FINAL_CLTV).unwrap();
assert_eq!(route.paths.len(), 2);
route.paths.sort_by(|path_a, _| {
// Sort the path so that the path through nodes[1] comes first
if path_a[0].pubkey == nodes[1].node.get_our_node_id() {
core::cmp::Ordering::Less } else { core::cmp::Ordering::Greater }
});
let payment_params_opt = Some(payment_params);

let (our_payment_preimage, our_payment_hash, our_payment_secret) = get_payment_preimage_hash!(&nodes[3]);

let cur_height = nodes[0].best_block_info().1;
let payment_id = PaymentId([42; 32]);
{
nodes[0].node.send_payment_along_path(&route.paths[0], &payment_params_opt, &our_payment_hash, &Some(our_payment_secret), 15_000_000, cur_height, payment_id, &None).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[3]], 15_000_000, our_payment_hash, Some(our_payment_secret), events.pop().unwrap(), false, None);
}
assert!(nodes[3].node.get_and_clear_pending_events().is_empty());

{
nodes[0].node.send_payment_along_path(&route.paths[1], &payment_params_opt, &our_payment_hash, &Some(our_payment_secret), 14_000_000, cur_height, payment_id, &None).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 payment_event = SendEvent::from_event(events.pop().unwrap());

nodes[2].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
commitment_signed_dance!(nodes[2], nodes[0], payment_event.commitment_msg, false);

nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_updates_1.update_fail_htlcs[0]);
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_updates_1.update_fail_htlcs[1]);
commitment_signed_dance!(nodes[0], nodes[1], fail_updates_1.commitment_signed, false);
expect_pending_htlcs_forwardable!(nodes[2]);
check_added_monitors!(nodes[2], 1);

let mut events = nodes[2].node.get_and_clear_pending_msg_events();
assert_eq!(events.len(), 1);
let payment_event = SendEvent::from_event(events.pop().unwrap());

nodes[3].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &payment_event.msgs[0]);
check_added_monitors!(nodes[3], 0);
commitment_signed_dance!(nodes[3], nodes[2], payment_event.commitment_msg, true, true);

// At this point, nodes[3] should notice the two HTLCs don't contain the same total payment
// amount. It will assume the second is a privacy attack (no longer particularly relevant
// post-payment_secrets) and fail back the new HTLC.
}
expect_pending_htlcs_forwardable_ignore!(nodes[3]);
nodes[3].node.process_pending_htlc_forwards();
expect_pending_htlcs_forwardable_ignore!(nodes[3]);
nodes[3].node.process_pending_htlc_forwards();

check_added_monitors!(nodes[3], 1);

let fail_updates_1 = get_htlc_update_msgs!(nodes[3], nodes[2].node.get_our_node_id());
nodes[2].node.handle_update_fail_htlc(&nodes[3].node.get_our_node_id(), &fail_updates_1.update_fail_htlcs[0]);
commitment_signed_dance!(nodes[2], nodes[3], fail_updates_1.commitment_signed, false);

expect_pending_htlcs_forwardable!(nodes[2]);
check_added_monitors!(nodes[2], 1);

let fail_updates_2 = get_htlc_update_msgs!(nodes[2], nodes[0].node.get_our_node_id());
nodes[0].node.handle_update_fail_htlc(&nodes[2].node.get_our_node_id(), &fail_updates_2.update_fail_htlcs[0]);
commitment_signed_dance!(nodes[0], nodes[2], fail_updates_2.commitment_signed, false);

expect_payment_failed_conditions!(nodes[0], our_payment_hash, true, PaymentFailedConditions::new().mpp_parts_remain());

nodes[0].node.send_payment_along_path(&route.paths[1], &payment_params_opt, &our_payment_hash, &Some(our_payment_secret), 15_000_000, cur_height, payment_id, &None).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]], 15_000_000, our_payment_hash, Some(our_payment_secret), events.pop().unwrap(), true, None);

let failure_events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(failure_events.len(), 2);
if let Event::PaymentPathFailed { .. } = failure_events[0] {} else { panic!(); }
if let Event::PaymentPathFailed { .. } = failure_events[1] {} else { panic!(); }
claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, our_payment_preimage);
}

#[test]
Expand Down