Skip to content

Commit cb952f6

Browse files
ViktorT-11ViktorT-11
committed
Expect pending_msg_events to be in random peer order in tests
1 parent 6cb9853 commit cb952f6

File tree

5 files changed

+167
-44
lines changed

5 files changed

+167
-44
lines changed

‎lightning/src/ln/chanmon_update_fail_tests.rs‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -935,7 +935,8 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
935935

936936
// Note that the ordering of the events for different nodes is non-prescriptive, though the
937937
// ordering of the two events that both go to nodes[2] have to stay in the same order.
938-
let messages_a = match events_3.pop().unwrap() {
938+
let (nodes_0_event, events_3) = remove_first_msg_event_to_node(&nodes[0].node.get_our_node_id(), &events_3);
939+
let messages_a = match nodes_0_event {
939940
MessageSendEvent::UpdateHTLCs { node_id, mut updates } => {
940941
assert_eq!(node_id, nodes[0].node.get_our_node_id());
941942
assert!(updates.update_fulfill_htlcs.is_empty());
@@ -947,17 +948,21 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
947948
},
948949
_ => panic!("Unexpected event type!"),
949950
};
951+
952+
let (nodes_2_event, events_3) = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &events_3);
953+
let send_event_b = SendEvent::from_event(nodes_2_event);
954+
assert_eq!(send_event_b.node_id, nodes[2].node.get_our_node_id());
955+
950956
let raa = if test_ignore_second_cs {
951-
match events_3.remove(1) {
957+
let (nodes_2_event, _events_3) = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &events_3);
958+
match nodes_2_event {
952959
MessageSendEvent::SendRevokeAndACK { node_id, msg } => {
953960
assert_eq!(node_id, nodes[2].node.get_our_node_id());
954961
Some(msg.clone())
955962
},
956963
_ => panic!("Unexpected event"),
957964
}
958965
} else { None };
959-
let send_event_b = SendEvent::from_event(events_3.remove(0));
960-
assert_eq!(send_event_b.node_id, nodes[2].node.get_our_node_id());
961966

962967
// Now deliver the new messages...
963968

@@ -991,6 +996,7 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
991996
check_added_monitors!(nodes[2], 1);
992997

993998
let bs_revoke_and_commit = nodes[2].node.get_and_clear_pending_msg_events();
999+
// As both messages are for nodes[1], they're in order.
9941000
assert_eq!(bs_revoke_and_commit.len(), 2);
9951001
match bs_revoke_and_commit[0] {
9961002
MessageSendEvent::SendRevokeAndACK { ref node_id, ref msg } => {

‎lightning/src/ln/functional_test_utils.rs‎

Lines changed: 95 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -558,6 +558,84 @@ macro_rules! get_htlc_update_msgs {
558558
}
559559
}
560560

561+
/// Fetches the first `msg_event` to the passed `node_id` in the passed `msg_events` vec.
562+
/// Returns the `msg_event`, along with an updated `msg_events` vec with the message removed.
563+
///
564+
/// Note that even though `BroadcastChannelAnnouncement` and `BroadcastChannelUpdate`
565+
/// `msg_events` are stored under specific peers, this function does not fetch such `msg_events` as
566+
/// such messages are intended to all peers.
567+
pub fn remove_first_msg_event_to_node(msg_node_id: &PublicKey, msg_events: &Vec<MessageSendEvent>) -> (MessageSendEvent, Vec<MessageSendEvent>) {
568+
let ev_index = msg_events.iter().position(|e| { match e {
569+
MessageSendEvent::SendAcceptChannel { node_id, .. } => {
570+
node_id == msg_node_id
571+
},
572+
MessageSendEvent::SendOpenChannel { node_id, .. } => {
573+
node_id == msg_node_id
574+
},
575+
MessageSendEvent::SendFundingCreated { node_id, .. } => {
576+
node_id == msg_node_id
577+
},
578+
MessageSendEvent::SendFundingSigned { node_id, .. } => {
579+
node_id == msg_node_id
580+
},
581+
MessageSendEvent::SendChannelReady { node_id, .. } => {
582+
node_id == msg_node_id
583+
},
584+
MessageSendEvent::SendAnnouncementSignatures { node_id, .. } => {
585+
node_id == msg_node_id
586+
},
587+
MessageSendEvent::UpdateHTLCs { node_id, .. } => {
588+
node_id == msg_node_id
589+
},
590+
MessageSendEvent::SendRevokeAndACK { node_id, .. } => {
591+
node_id == msg_node_id
592+
},
593+
MessageSendEvent::SendClosingSigned { node_id, .. } => {
594+
node_id == msg_node_id
595+
},
596+
MessageSendEvent::SendShutdown { node_id, .. } => {
597+
node_id == msg_node_id
598+
},
599+
MessageSendEvent::SendChannelReestablish { node_id, .. } => {
600+
node_id == msg_node_id
601+
},
602+
MessageSendEvent::SendChannelAnnouncement { node_id, .. } => {
603+
node_id == msg_node_id
604+
},
605+
MessageSendEvent::BroadcastChannelAnnouncement { .. } => {
606+
false
607+
},
608+
MessageSendEvent::BroadcastChannelUpdate { .. } => {
609+
false
610+
},
611+
MessageSendEvent::SendChannelUpdate { node_id, .. } => {
612+
node_id == msg_node_id
613+
},
614+
MessageSendEvent::HandleError { node_id, .. } => {
615+
node_id == msg_node_id
616+
},
617+
MessageSendEvent::SendChannelRangeQuery { node_id, .. } => {
618+
node_id == msg_node_id
619+
},
620+
MessageSendEvent::SendShortIdsQuery { node_id, .. } => {
621+
node_id == msg_node_id
622+
},
623+
MessageSendEvent::SendReplyChannelRange { node_id, .. } => {
624+
node_id == msg_node_id
625+
},
626+
MessageSendEvent::SendGossipTimestampFilter { node_id, .. } => {
627+
node_id == msg_node_id
628+
},
629+
}});
630+
if ev_index.is_some() {
631+
let mut updated_msg_events = msg_events.to_vec();
632+
let ev = updated_msg_events.remove(ev_index.unwrap());
633+
(ev, updated_msg_events)
634+
} else {
635+
panic!("Couldn't find any MessageSendEvent to the node!")
636+
}
637+
}
638+
561639
#[cfg(test)]
562640
macro_rules! get_channel_ref {
563641
($node: expr, $counterparty_node: expr, $per_peer_state_lock: ident, $peer_state_lock: ident, $channel_id: expr) => {
@@ -1276,13 +1354,14 @@ macro_rules! commitment_signed_dance {
12761354
let (bs_revoke_and_ack, extra_msg_option) = {
12771355
let events = $node_b.node.get_and_clear_pending_msg_events();
12781356
assert!(events.len() <= 2);
1279-
(match events[0] {
1357+
let (node_a_event, events) = remove_first_msg_event_to_node(&$node_a.node.get_our_node_id(), &events);
1358+
(match node_a_event {
12801359
MessageSendEvent::SendRevokeAndACK { ref node_id, ref msg } => {
12811360
assert_eq!(*node_id, $node_a.node.get_our_node_id());
12821361
(*msg).clone()
12831362
},
12841363
_ => panic!("Unexpected event"),
1285-
}, events.get(1).map(|e| e.clone()))
1364+
}, events.get(0).map(|e| e.clone()))
12861365
};
12871366
check_added_monitors!($node_b, 1);
12881367
if $fail_backwards {
@@ -1833,7 +1912,9 @@ pub fn pass_along_path<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_path
18331912
pub fn pass_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&[&Node<'a, 'b, 'c>]], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: PaymentSecret) {
18341913
let mut events = origin_node.node.get_and_clear_pending_msg_events();
18351914
assert_eq!(events.len(), expected_route.len());
1836-
for (path_idx, (ev, expected_path)) in events.drain(..).zip(expected_route.iter()).enumerate() {
1915+
for (path_idx, expected_path) in expected_route.iter().enumerate() {
1916+
let (ev, updated_events) = remove_first_msg_event_to_node(&expected_path[0].node.get_our_node_id(), &events);
1917+
events = updated_events;
18371918
// Once we've gotten through all the HTLCs, the last one should result in a
18381919
// PaymentClaimable (but each previous one should not!), .
18391920
let expect_payment = path_idx == expected_route.len() - 1;
@@ -1884,10 +1965,18 @@ pub fn do_claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>,
18841965
}
18851966
}
18861967
let mut per_path_msgs: Vec<((msgs::UpdateFulfillHTLC, msgs::CommitmentSigned), PublicKey)> = Vec::with_capacity(expected_paths.len());
1887-
let events = expected_paths[0].last().unwrap().node.get_and_clear_pending_msg_events();
1968+
let mut events = expected_paths[0].last().unwrap().node.get_and_clear_pending_msg_events();
18881969
assert_eq!(events.len(), expected_paths.len());
1889-
for ev in events.iter() {
1890-
per_path_msgs.push(msgs_from_ev!(ev));
1970+
1971+
if events.len() == 1 {
1972+
per_path_msgs.push(msgs_from_ev!(&events[0]));
1973+
} else {
1974+
for expected_path in expected_paths.iter() {
1975+
// For MPP payments, we always want the message to the first node in the path.
1976+
let (ev, updated_events) = remove_first_msg_event_to_node(&expected_path[0].node.get_our_node_id(), &events);
1977+
per_path_msgs.push(msgs_from_ev!(&ev));
1978+
events = updated_events;
1979+
}
18911980
}
18921981

18931982
for (expected_route, (path_msgs, next_hop)) in expected_paths.iter().zip(per_path_msgs.drain(..)) {

‎lightning/src/ln/functional_tests.rs‎

Lines changed: 52 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -2756,16 +2756,16 @@ fn test_htlc_on_chain_success() {
27562756
added_monitors.clear();
27572757
}
27582758
assert_eq!(events.len(), 3);
2759-
match events[0] {
2760-
MessageSendEvent::BroadcastChannelUpdate { .. } => {},
2761-
_ => panic!("Unexpected event"),
2762-
}
2763-
match events[1] {
2759+
2760+
let (nodes_2_event, events) = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &events);
2761+
let (nodes_0_event, events) = remove_first_msg_event_to_node(&nodes[0].node.get_our_node_id(), &events);
2762+
2763+
match nodes_2_event {
27642764
MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { .. }, node_id: _ } => {},
27652765
_ => panic!("Unexpected event"),
27662766
}
27672767

2768-
match events[2] {
2768+
match nodes_0_event {
27692769
MessageSendEvent::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fail_htlcs, ref update_fulfill_htlcs, ref update_fail_malformed_htlcs, .. } } => {
27702770
assert!(update_add_htlcs.is_empty());
27712771
assert!(update_fail_htlcs.is_empty());
@@ -2775,6 +2775,13 @@ fn test_htlc_on_chain_success() {
27752775
},
27762776
_ => panic!("Unexpected event"),
27772777
};
2778+
2779+
// Ensure that the last remaining message event is the BroadcastChannelUpdate msg for chan_2
2780+
match events[0] {
2781+
MessageSendEvent::BroadcastChannelUpdate { .. } => {},
2782+
_ => panic!("Unexpected event"),
2783+
}
2784+
27782785
macro_rules! check_tx_local_broadcast {
27792786
($node: expr, $htlc_offered: expr, $commitment_tx: expr) => { {
27802787
let mut node_txn = $node.tx_broadcaster.txn_broadcasted.lock().unwrap();
@@ -3192,21 +3199,12 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
31923199
nodes[1].node.process_pending_htlc_forwards();
31933200
check_added_monitors!(nodes[1], 1);
31943201

3195-
let events = nodes[1].node.get_and_clear_pending_msg_events();
3202+
let mut events = nodes[1].node.get_and_clear_pending_msg_events();
31963203
assert_eq!(events.len(), if deliver_bs_raa { 4 } else { 3 });
3197-
match events[if deliver_bs_raa { 1 } else { 0 }] {
3198-
MessageSendEvent::BroadcastChannelUpdate { msg: msgs::ChannelUpdate { .. } } => {},
3199-
_ => panic!("Unexpected event"),
3200-
}
3201-
match events[if deliver_bs_raa { 2 } else { 1 }] {
3202-
MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { msg: msgs::ErrorMessage { channel_id, ref data } }, node_id: _ } => {
3203-
assert_eq!(channel_id, chan_2.2);
3204-
assert_eq!(data.as_str(), "Channel closed because commitment or closing transaction was confirmed on chain.");
3205-
},
3206-
_ => panic!("Unexpected event"),
3207-
}
3208-
if deliver_bs_raa {
3209-
match events[0] {
3204+
3205+
let events = if deliver_bs_raa {
3206+
let (nodes_2_event, events) = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &events);
3207+
match nodes_2_event {
32103208
MessageSendEvent::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fail_htlcs, ref update_fulfill_htlcs, ref update_fail_malformed_htlcs, .. } } => {
32113209
assert_eq!(nodes[2].node.get_our_node_id(), *node_id);
32123210
assert_eq!(update_add_htlcs.len(), 1);
@@ -3216,8 +3214,20 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
32163214
},
32173215
_ => panic!("Unexpected event"),
32183216
}
3217+
events
3218+
} else { events };
3219+
3220+
let (nodes_2_event, events) = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &events);
3221+
match nodes_2_event {
3222+
MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { msg: msgs::ErrorMessage { channel_id, ref data } }, node_id: _ } => {
3223+
assert_eq!(channel_id, chan_2.2);
3224+
assert_eq!(data.as_str(), "Channel closed because commitment or closing transaction was confirmed on chain.");
3225+
},
3226+
_ => panic!("Unexpected event"),
32193227
}
3220-
match events[if deliver_bs_raa { 3 } else { 2 }] {
3228+
3229+
let (nodes_0_event, events) = remove_first_msg_event_to_node(&nodes[0].node.get_our_node_id(), &events);
3230+
match nodes_0_event {
32213231
MessageSendEvent::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fail_htlcs, ref update_fulfill_htlcs, ref update_fail_malformed_htlcs, ref commitment_signed, .. } } => {
32223232
assert!(update_add_htlcs.is_empty());
32233233
assert_eq!(update_fail_htlcs.len(), 3);
@@ -3262,6 +3272,12 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
32623272
_ => panic!("Unexpected event"),
32633273
}
32643274

3275+
// Ensure that the last remaining message event is the BroadcastChannelUpdate msg for chan_2
3276+
match events[0] {
3277+
MessageSendEvent::BroadcastChannelUpdate { msg: msgs::ChannelUpdate { .. } } => {},
3278+
_ => panic!("Unexpected event"),
3279+
}
3280+
32653281
assert!(failed_htlcs.contains(&first_payment_hash.0));
32663282
assert!(failed_htlcs.contains(&second_payment_hash.0));
32673283
assert!(failed_htlcs.contains(&third_payment_hash.0));
@@ -4627,15 +4643,15 @@ fn test_onchain_to_onchain_claim() {
46274643
check_added_monitors!(nodes[1], 1);
46284644
let msg_events = nodes[1].node.get_and_clear_pending_msg_events();
46294645
assert_eq!(msg_events.len(), 3);
4630-
match msg_events[0] {
4631-
MessageSendEvent::BroadcastChannelUpdate { .. } => {},
4632-
_ => panic!("Unexpected event"),
4633-
}
4634-
match msg_events[1] {
4646+
let (nodes_2_event, msg_events) = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &msg_events);
4647+
let (nodes_0_event, msg_events) = remove_first_msg_event_to_node(&nodes[0].node.get_our_node_id(), &msg_events);
4648+
4649+
match nodes_2_event {
46354650
MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { .. }, node_id: _ } => {},
46364651
_ => panic!("Unexpected event"),
46374652
}
4638-
match msg_events[2] {
4653+
4654+
match nodes_0_event {
46394655
MessageSendEvent::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, .. } } => {
46404656
assert!(update_add_htlcs.is_empty());
46414657
assert!(update_fail_htlcs.is_empty());
@@ -4645,6 +4661,13 @@ fn test_onchain_to_onchain_claim() {
46454661
},
46464662
_ => panic!("Unexpected event"),
46474663
};
4664+
4665+
// Ensure that the last remaining message event is the BroadcastChannelUpdate msg for chan_2
4666+
match msg_events[0] {
4667+
MessageSendEvent::BroadcastChannelUpdate { .. } => {},
4668+
_ => panic!("Unexpected event"),
4669+
}
4670+
46484671
// Broadcast A's commitment tx on B's chain to see if we are able to claim inbound HTLC with our HTLC-Success tx
46494672
let commitment_tx = get_local_commitment_txn!(nodes[0], chan_1.2);
46504673
mine_transaction(&nodes[1], &commitment_tx[0]);
@@ -9254,7 +9277,8 @@ fn test_double_partial_claim() {
92549277

92559278
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
92569279
assert_eq!(events.len(), 2);
9257-
pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], 15_000_000, payment_hash, Some(payment_secret), events.drain(..).next().unwrap(), false, None);
9280+
let (node_1_msgs, _events) = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &events);
9281+
pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], 15_000_000, payment_hash, Some(payment_secret), node_1_msgs, false, None);
92589282

92599283
// At this point nodes[3] has received one half of the payment, and the user goes to handle
92609284
// that PaymentClaimable event they got hours ago and never handled...we should refuse to claim.

‎lightning/src/ln/payment_tests.rs‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -146,11 +146,11 @@ fn mpp_retry() {
146146
assert_eq!(events.len(), 2);
147147

148148
// Pass half of the payment along the success path.
149-
let success_path_msgs = events.remove(0);
149+
let (success_path_msgs, mut events) = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &events);
150150
pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], 2_000_000, payment_hash, Some(payment_secret), success_path_msgs, false, None);
151151

152152
// Add the HTLC along the first hop.
153-
let fail_path_msgs_1 = events.remove(0);
153+
let (fail_path_msgs_1, _events) = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &events);
154154
let (update_add, commitment_signed) = match fail_path_msgs_1 {
155155
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 } } => {
156156
assert_eq!(update_add_htlcs.len(), 1);
@@ -230,7 +230,8 @@ fn do_mpp_receive_timeout(send_partial_mpp: bool) {
230230
assert_eq!(events.len(), 2);
231231

232232
// Pass half of the payment along the first path.
233-
pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], 200_000, payment_hash, Some(payment_secret), events.remove(0), false, None);
233+
let (node_1_msgs, mut events) = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &events);
234+
pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], 200_000, payment_hash, Some(payment_secret), node_1_msgs, false, None);
234235

235236
if send_partial_mpp {
236237
// Time out the partial MPP
@@ -257,7 +258,8 @@ fn do_mpp_receive_timeout(send_partial_mpp: bool) {
257258
expect_payment_failed_conditions(&nodes[0], payment_hash, false, PaymentFailedConditions::new().mpp_parts_remain().expected_htlc_error_data(23, &[][..]));
258259
} else {
259260
// Pass half of the payment along the second path.
260-
pass_along_path(&nodes[0], &[&nodes[2], &nodes[3]], 200_000, payment_hash, Some(payment_secret), events.remove(0), true, None);
261+
let (node_2_msgs, _events) = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &events);
262+
pass_along_path(&nodes[0], &[&nodes[2], &nodes[3]], 200_000, payment_hash, Some(payment_secret), node_2_msgs, true, None);
261263

262264
// Even after MPP_TIMEOUT_TICKS we should not timeout the MPP if we have all the parts
263265
for _ in 0..MPP_TIMEOUT_TICKS {

‎lightning/src/ln/reload_tests.rs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -699,8 +699,10 @@ fn do_test_partial_claim_before_restart(persist_both_monitors: bool) {
699699
// Send the payment through to nodes[3] *without* clearing the PaymentClaimable event
700700
let mut send_events = nodes[0].node.get_and_clear_pending_msg_events();
701701
assert_eq!(send_events.len(), 2);
702-
do_pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], 15_000_000, payment_hash, Some(payment_secret), send_events[0].clone(), true, false, None);
703-
do_pass_along_path(&nodes[0], &[&nodes[2], &nodes[3]], 15_000_000, payment_hash, Some(payment_secret), send_events[1].clone(), true, false, None);
702+
let (node_1_msgs, mut send_events) = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &send_events);
703+
let (node_2_msgs, _send_events) = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &send_events);
704+
do_pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], 15_000_000, payment_hash, Some(payment_secret), node_1_msgs, true, false, None);
705+
do_pass_along_path(&nodes[0], &[&nodes[2], &nodes[3]], 15_000_000, payment_hash, Some(payment_secret), node_2_msgs, true, false, None);
704706

705707
// Now that we have an MPP payment pending, get the latest encoded copies of nodes[3]'s
706708
// monitors and ChannelManager, for use later, if we don't want to persist both monitors.

0 commit comments

Comments
 (0)