Skip to content

Commit 9bdce47

Browse files
TheBlueMattTheBlueMatt
authored
Merge pull request #1451 from TheBlueMatt/2022-04-moar-mpp-fail-test
Add test coverage for failure of inconsistent MPP parts
2 parents dc8479a + 7a8344a commit 9bdce47

File tree

2 files changed

+162
-46
lines changed

2 files changed

+162
-46
lines changed

‎lightning/src/ln/functional_test_utils.rs

+18-19
Original file line numberDiff line numberDiff line change
@@ -1167,6 +1167,21 @@ macro_rules! get_payment_preimage_hash {
11671167
}
11681168
}
11691169

1170+
#[macro_export]
1171+
macro_rules! get_route {
1172+
($send_node: expr, $payment_params: expr, $recv_value: expr, $cltv: expr) => {{
1173+
use $crate::chain::keysinterface::KeysInterface;
1174+
let scorer = $crate::util::test_utils::TestScorer::with_penalty(0);
1175+
let keys_manager = $crate::util::test_utils::TestKeysInterface::new(&[0u8; 32], bitcoin::network::constants::Network::Testnet);
1176+
let random_seed_bytes = keys_manager.get_secure_random_bytes();
1177+
$crate::routing::router::get_route(
1178+
&$send_node.node.get_our_node_id(), &$payment_params, &$send_node.network_graph.read_only(),
1179+
Some(&$send_node.node.list_usable_channels().iter().collect::<Vec<_>>()),
1180+
$recv_value, $cltv, $send_node.logger, &scorer, &random_seed_bytes
1181+
)
1182+
}}
1183+
}
1184+
11701185
#[cfg(test)]
11711186
#[macro_export]
11721187
macro_rules! get_route_and_payment_hash {
@@ -1176,17 +1191,9 @@ macro_rules! get_route_and_payment_hash {
11761191
$crate::get_route_and_payment_hash!($send_node, $recv_node, payment_params, $recv_value, TEST_FINAL_CLTV)
11771192
}};
11781193
($send_node: expr, $recv_node: expr, $payment_params: expr, $recv_value: expr, $cltv: expr) => {{
1179-
use $crate::chain::keysinterface::KeysInterface;
11801194
let (payment_preimage, payment_hash, payment_secret) = $crate::get_payment_preimage_hash!($recv_node, Some($recv_value));
1181-
let scorer = $crate::util::test_utils::TestScorer::with_penalty(0);
1182-
let keys_manager = $crate::util::test_utils::TestKeysInterface::new(&[0u8; 32], bitcoin::network::constants::Network::Testnet);
1183-
let random_seed_bytes = keys_manager.get_secure_random_bytes();
1184-
let route = $crate::routing::router::get_route(
1185-
&$send_node.node.get_our_node_id(), &$payment_params, &$send_node.network_graph.read_only(),
1186-
Some(&$send_node.node.list_usable_channels().iter().collect::<Vec<_>>()),
1187-
$recv_value, $cltv, $send_node.logger, &scorer, &random_seed_bytes
1188-
).unwrap();
1189-
(route, payment_hash, payment_preimage, payment_secret)
1195+
let route = $crate::get_route!($send_node, $payment_params, $recv_value, $cltv);
1196+
(route.unwrap(), payment_hash, payment_preimage, payment_secret)
11901197
}}
11911198
}
11921199

@@ -1650,15 +1657,7 @@ pub const TEST_FINAL_CLTV: u32 = 70;
16501657
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) {
16511658
let payment_params = PaymentParameters::from_node_id(expected_route.last().unwrap().node.get_our_node_id())
16521659
.with_features(InvoiceFeatures::known());
1653-
let network_graph = origin_node.network_graph.read_only();
1654-
let scorer = test_utils::TestScorer::with_penalty(0);
1655-
let seed = [0u8; 32];
1656-
let keys_manager = test_utils::TestKeysInterface::new(&seed, Network::Testnet);
1657-
let random_seed_bytes = keys_manager.get_secure_random_bytes();
1658-
let route = get_route(
1659-
&origin_node.node.get_our_node_id(), &payment_params, &network_graph,
1660-
Some(&origin_node.node.list_usable_channels().iter().collect::<Vec<_>>()),
1661-
recv_value, TEST_FINAL_CLTV, origin_node.logger, &scorer, &random_seed_bytes).unwrap();
1660+
let route = get_route!(origin_node, payment_params, recv_value, TEST_FINAL_CLTV).unwrap();
16621661
assert_eq!(route.paths.len(), 1);
16631662
assert_eq!(route.paths[0].len(), expected_route.len());
16641663
for (node, hop) in expected_route.iter().zip(route.paths[0].iter()) {

‎lightning/src/ln/functional_tests.rs

+144-27
Original file line numberDiff line numberDiff line change
@@ -9457,12 +9457,7 @@ fn test_forwardable_regen() {
94579457
claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage_2);
94589458
}
94599459

9460-
#[test]
9461-
fn test_dup_htlc_second_fail_panic() {
9462-
// Previously, if we received two HTLCs back-to-back, where the second overran the expected
9463-
// value for the payment, we'd fail back both HTLCs after generating a `PaymentReceived` event.
9464-
// Then, if the user failed the second payment, they'd hit a "tried to fail an already failed
9465-
// HTLC" debug panic. This tests for this behavior, checking that only one HTLC is auto-failed.
9460+
fn do_test_dup_htlc_second_rejected(test_for_second_fail_panic: bool) {
94669461
let chanmon_cfgs = create_chanmon_cfgs(2);
94679462
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
94689463
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
@@ -9472,14 +9467,9 @@ fn test_dup_htlc_second_fail_panic() {
94729467

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

9482-
let (_, our_payment_hash, our_payment_secret) = get_payment_preimage_hash!(&nodes[1]);
9472+
let (our_payment_preimage, our_payment_hash, our_payment_secret) = get_payment_preimage_hash!(&nodes[1]);
94839473

94849474
{
94859475
nodes[0].node.send_payment(&route, our_payment_hash, &Some(our_payment_secret)).unwrap();
@@ -9507,26 +9497,153 @@ fn test_dup_htlc_second_fail_panic() {
95079497
// the first HTLC delivered above.
95089498
}
95099499

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

9515-
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
9516-
nodes[1].node.process_pending_htlc_forwards();
9503+
if test_for_second_fail_panic {
9504+
// Now we go fail back the first HTLC from the user end.
9505+
nodes[1].node.fail_htlc_backwards(&our_payment_hash);
95179506

9518-
check_added_monitors!(nodes[1], 1);
9519-
let fail_updates_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
9520-
assert_eq!(fail_updates_1.update_fail_htlcs.len(), 2);
9507+
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
9508+
nodes[1].node.process_pending_htlc_forwards();
9509+
9510+
check_added_monitors!(nodes[1], 1);
9511+
let fail_updates_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
9512+
assert_eq!(fail_updates_1.update_fail_htlcs.len(), 2);
9513+
9514+
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_updates_1.update_fail_htlcs[0]);
9515+
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_updates_1.update_fail_htlcs[1]);
9516+
commitment_signed_dance!(nodes[0], nodes[1], fail_updates_1.commitment_signed, false);
9517+
9518+
let failure_events = nodes[0].node.get_and_clear_pending_events();
9519+
assert_eq!(failure_events.len(), 2);
9520+
if let Event::PaymentPathFailed { .. } = failure_events[0] {} else { panic!(); }
9521+
if let Event::PaymentPathFailed { .. } = failure_events[1] {} else { panic!(); }
9522+
} else {
9523+
// Let the second HTLC fail and claim the first
9524+
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
9525+
nodes[1].node.process_pending_htlc_forwards();
9526+
9527+
check_added_monitors!(nodes[1], 1);
9528+
let fail_updates_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
9529+
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_updates_1.update_fail_htlcs[0]);
9530+
commitment_signed_dance!(nodes[0], nodes[1], fail_updates_1.commitment_signed, false);
9531+
9532+
expect_payment_failed_conditions!(nodes[0], our_payment_hash, true, PaymentFailedConditions::new().mpp_parts_remain());
9533+
9534+
claim_payment(&nodes[0], &[&nodes[1]], our_payment_preimage);
9535+
}
9536+
}
9537+
9538+
#[test]
9539+
fn test_dup_htlc_second_fail_panic() {
9540+
// Previously, if we received two HTLCs back-to-back, where the second overran the expected
9541+
// value for the payment, we'd fail back both HTLCs after generating a `PaymentReceived` event.
9542+
// Then, if the user failed the second payment, they'd hit a "tried to fail an already failed
9543+
// HTLC" debug panic. This tests for this behavior, checking that only one HTLC is auto-failed.
9544+
do_test_dup_htlc_second_rejected(true);
9545+
}
9546+
9547+
#[test]
9548+
fn test_dup_htlc_second_rejected() {
9549+
// Test that if we receive a second HTLC for an MPP payment that overruns the payment amount we
9550+
// simply reject the second HTLC but are still able to claim the first HTLC.
9551+
do_test_dup_htlc_second_rejected(false);
9552+
}
9553+
9554+
#[test]
9555+
fn test_inconsistent_mpp_params() {
9556+
// Test that if we recieve two HTLCs with different payment parameters we fail back the first
9557+
// such HTLC and allow the second to stay.
9558+
let chanmon_cfgs = create_chanmon_cfgs(4);
9559+
let node_cfgs = create_node_cfgs(4, &chanmon_cfgs);
9560+
let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]);
9561+
let nodes = create_network(4, &node_cfgs, &node_chanmgrs);
9562+
9563+
create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 0, InitFeatures::known(), InitFeatures::known());
9564+
create_announced_chan_between_nodes_with_value(&nodes, 0, 2, 100_000, 0, InitFeatures::known(), InitFeatures::known());
9565+
create_announced_chan_between_nodes_with_value(&nodes, 1, 3, 100_000, 0, InitFeatures::known(), InitFeatures::known());
9566+
create_announced_chan_between_nodes_with_value(&nodes, 2, 3, 100_000, 0, InitFeatures::known(), InitFeatures::known());
9567+
9568+
let payment_params = PaymentParameters::from_node_id(nodes[3].node.get_our_node_id())
9569+
.with_features(InvoiceFeatures::known());
9570+
let mut route = get_route!(nodes[0], payment_params, 15_000_000, TEST_FINAL_CLTV).unwrap();
9571+
assert_eq!(route.paths.len(), 2);
9572+
route.paths.sort_by(|path_a, _| {
9573+
// Sort the path so that the path through nodes[1] comes first
9574+
if path_a[0].pubkey == nodes[1].node.get_our_node_id() {
9575+
core::cmp::Ordering::Less } else { core::cmp::Ordering::Greater }
9576+
});
9577+
let payment_params_opt = Some(payment_params);
9578+
9579+
let (our_payment_preimage, our_payment_hash, our_payment_secret) = get_payment_preimage_hash!(&nodes[3]);
9580+
9581+
let cur_height = nodes[0].best_block_info().1;
9582+
let payment_id = PaymentId([42; 32]);
9583+
{
9584+
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();
9585+
check_added_monitors!(nodes[0], 1);
9586+
9587+
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
9588+
assert_eq!(events.len(), 1);
9589+
pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], 15_000_000, our_payment_hash, Some(our_payment_secret), events.pop().unwrap(), false, None);
9590+
}
9591+
assert!(nodes[3].node.get_and_clear_pending_events().is_empty());
9592+
9593+
{
9594+
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();
9595+
check_added_monitors!(nodes[0], 1);
9596+
9597+
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
9598+
assert_eq!(events.len(), 1);
9599+
let payment_event = SendEvent::from_event(events.pop().unwrap());
9600+
9601+
nodes[2].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
9602+
commitment_signed_dance!(nodes[2], nodes[0], payment_event.commitment_msg, false);
95219603

9522-
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_updates_1.update_fail_htlcs[0]);
9523-
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_updates_1.update_fail_htlcs[1]);
9524-
commitment_signed_dance!(nodes[0], nodes[1], fail_updates_1.commitment_signed, false);
9604+
expect_pending_htlcs_forwardable!(nodes[2]);
9605+
check_added_monitors!(nodes[2], 1);
9606+
9607+
let mut events = nodes[2].node.get_and_clear_pending_msg_events();
9608+
assert_eq!(events.len(), 1);
9609+
let payment_event = SendEvent::from_event(events.pop().unwrap());
9610+
9611+
nodes[3].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &payment_event.msgs[0]);
9612+
check_added_monitors!(nodes[3], 0);
9613+
commitment_signed_dance!(nodes[3], nodes[2], payment_event.commitment_msg, true, true);
9614+
9615+
// At this point, nodes[3] should notice the two HTLCs don't contain the same total payment
9616+
// amount. It will assume the second is a privacy attack (no longer particularly relevant
9617+
// post-payment_secrets) and fail back the new HTLC.
9618+
}
9619+
expect_pending_htlcs_forwardable_ignore!(nodes[3]);
9620+
nodes[3].node.process_pending_htlc_forwards();
9621+
expect_pending_htlcs_forwardable_ignore!(nodes[3]);
9622+
nodes[3].node.process_pending_htlc_forwards();
9623+
9624+
check_added_monitors!(nodes[3], 1);
9625+
9626+
let fail_updates_1 = get_htlc_update_msgs!(nodes[3], nodes[2].node.get_our_node_id());
9627+
nodes[2].node.handle_update_fail_htlc(&nodes[3].node.get_our_node_id(), &fail_updates_1.update_fail_htlcs[0]);
9628+
commitment_signed_dance!(nodes[2], nodes[3], fail_updates_1.commitment_signed, false);
9629+
9630+
expect_pending_htlcs_forwardable!(nodes[2]);
9631+
check_added_monitors!(nodes[2], 1);
9632+
9633+
let fail_updates_2 = get_htlc_update_msgs!(nodes[2], nodes[0].node.get_our_node_id());
9634+
nodes[0].node.handle_update_fail_htlc(&nodes[2].node.get_our_node_id(), &fail_updates_2.update_fail_htlcs[0]);
9635+
commitment_signed_dance!(nodes[0], nodes[2], fail_updates_2.commitment_signed, false);
9636+
9637+
expect_payment_failed_conditions!(nodes[0], our_payment_hash, true, PaymentFailedConditions::new().mpp_parts_remain());
9638+
9639+
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();
9640+
check_added_monitors!(nodes[0], 1);
9641+
9642+
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
9643+
assert_eq!(events.len(), 1);
9644+
pass_along_path(&nodes[0], &[&nodes[2], &nodes[3]], 15_000_000, our_payment_hash, Some(our_payment_secret), events.pop().unwrap(), true, None);
95259645

9526-
let failure_events = nodes[0].node.get_and_clear_pending_events();
9527-
assert_eq!(failure_events.len(), 2);
9528-
if let Event::PaymentPathFailed { .. } = failure_events[0] {} else { panic!(); }
9529-
if let Event::PaymentPathFailed { .. } = failure_events[1] {} else { panic!(); }
9646+
claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, our_payment_preimage);
95309647
}
95319648

95329649
#[test]

0 commit comments

Comments
 (0)