Skip to content

Commit b5a9eab

Browse files
shaavanshaavan
committed
Pass all BroadcastChannelUpdates through pending_broadcast_message pipeline
And fix the consequent test failures. 1. Resolved issues stemming from lack of peers to broadcast `msg_events`. 2. Updated handling of `BroadcastChannelUpdate` and `HandleErrorMessage`. Introduced `connect_dummy_node` and `disconnect_dummy_node` functions in functional test utils to rectify the first type of failure. Additionally, manually adjusted other tests to align with the updated `msg_events` ordering.
1 parent 6222b1f commit b5a9eab

File tree

4 files changed

+106
-34
lines changed

4 files changed

+106
-34
lines changed

‎lightning/src/ln/chanmon_update_fail_tests.rs

+1
Original file line numberDiff line numberDiff line change
@@ -3282,6 +3282,7 @@ fn do_test_durable_preimages_on_closed_channel(close_chans_before_reload: bool,
32823282
check_spends!(bs_preimage_tx, as_closing_tx[0]);
32833283

32843284
if !close_chans_before_reload {
3285+
// Connect a dummy node to allow broadcasting the close channel event.
32853286
check_closed_broadcast(&nodes[1], 1, true);
32863287
check_closed_event(&nodes[1], 1, ClosureReason::CommitmentTxConfirmed, false, &[nodes[0].node.get_our_node_id()], 100000);
32873288
} else {

‎lightning/src/ln/channelmanager.rs

+33-20
Original file line numberDiff line numberDiff line change
@@ -892,7 +892,7 @@ pub(super) struct PeerState<SP: Deref> where SP::Target: SignerProvider {
892892
/// The peer is currently connected (i.e. we've seen a
893893
/// [`ChannelMessageHandler::peer_connected`] and no corresponding
894894
/// [`ChannelMessageHandler::peer_disconnected`].
895-
is_connected: bool,
895+
pub is_connected: bool,
896896
}
897897

898898
impl <SP: Deref> PeerState<SP> where SP::Target: SignerProvider {
@@ -1392,8 +1392,7 @@ where
13921392

13931393
pending_offers_messages: Mutex<Vec<PendingOnionMessage<OffersMessage>>>,
13941394

1395-
/// Tracks the channel_update message that were not broadcasted because
1396-
/// we were not connected to any peers.
1395+
/// Tracks the message events that are to be broadcasted when we are connected to some peer.
13971396
pending_broadcast_messages: Mutex<Vec<MessageSendEvent>>,
13981397

13991398
entropy_source: ES,
@@ -1980,7 +1979,8 @@ macro_rules! handle_error {
19801979
match $internal {
19811980
Ok(msg) => Ok(msg),
19821981
Err(MsgHandleErrInternal { err, shutdown_finish, .. }) => {
1983-
let mut msg_events = Vec::with_capacity(2);
1982+
let mut msg_event = None;
1983+
let mut broadcast_event = None;
19841984

19851985
if let Some((shutdown_res, update_option)) = shutdown_finish {
19861986
let counterparty_node_id = shutdown_res.counterparty_node_id;
@@ -1992,7 +1992,7 @@ macro_rules! handle_error {
19921992

19931993
$self.finish_close_channel(shutdown_res);
19941994
if let Some(update) = update_option {
1995-
msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
1995+
broadcast_event = Some(events::MessageSendEvent::BroadcastChannelUpdate {
19961996
msg: update
19971997
});
19981998
}
@@ -2002,17 +2002,22 @@ macro_rules! handle_error {
20022002

20032003
if let msgs::ErrorAction::IgnoreError = err.action {
20042004
} else {
2005-
msg_events.push(events::MessageSendEvent::HandleError {
2005+
msg_event = Some(events::MessageSendEvent::HandleError {
20062006
node_id: $counterparty_node_id,
20072007
action: err.action.clone()
20082008
});
20092009
}
20102010

2011-
if !msg_events.is_empty() {
2011+
if let Some(broadcast_event) = broadcast_event {
2012+
let mut pending_broadcast_messages = $self.pending_broadcast_messages.lock().unwrap();
2013+
pending_broadcast_messages.push(broadcast_event);
2014+
}
2015+
2016+
if let Some(msg_event) = msg_event {
20122017
let per_peer_state = $self.per_peer_state.read().unwrap();
20132018
if let Some(peer_state_mutex) = per_peer_state.get(&$counterparty_node_id) {
20142019
let mut peer_state = peer_state_mutex.lock().unwrap();
2015-
peer_state.pending_msg_events.append(&mut msg_events);
2020+
peer_state.pending_msg_events.push(msg_event);
20162021
}
20172022
}
20182023

@@ -2963,7 +2968,7 @@ where
29632968
};
29642969
if let Some(update) = update_opt {
29652970
// If we have some Channel Update to broadcast, we cache it and broadcast it later.
2966-
let mut pending_broadcast_messages = self.pending_broadcast_messages.lock().unwrap();
2971+
let pending_broadcast_messages = &mut self.pending_broadcast_messages.lock().unwrap();
29672972
pending_broadcast_messages.push(events::MessageSendEvent::BroadcastChannelUpdate {
29682973
msg: update
29692974
});
@@ -4042,6 +4047,7 @@ where
40424047
.ok_or_else(|| APIError::ChannelUnavailable { err: format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id) })?;
40434048
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
40444049
let peer_state = &mut *peer_state_lock;
4050+
40454051
for channel_id in channel_ids {
40464052
if !peer_state.has_channel(channel_id) {
40474053
return Err(APIError::ChannelUnavailable {
@@ -4058,7 +4064,8 @@ where
40584064
}
40594065
if let ChannelPhase::Funded(channel) = channel_phase {
40604066
if let Ok(msg) = self.get_channel_update_for_broadcast(channel) {
4061-
peer_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate { msg });
4067+
let pending_broadcast_messages = &mut self.pending_broadcast_messages.lock().unwrap();
4068+
pending_broadcast_messages.push(events::MessageSendEvent::BroadcastChannelUpdate { msg });
40624069
} else if let Ok(msg) = self.get_channel_update_for_unicast(channel) {
40634070
peer_state.pending_msg_events.push(events::MessageSendEvent::SendChannelUpdate {
40644071
node_id: channel.context.get_counterparty_node_id(),
@@ -4968,7 +4975,8 @@ where
49684975
if n >= DISABLE_GOSSIP_TICKS {
49694976
chan.set_channel_update_status(ChannelUpdateStatus::Disabled);
49704977
if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
4971-
pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
4978+
let pending_broadcast_messages = &mut self.pending_broadcast_messages.lock().unwrap();
4979+
pending_broadcast_messages.push(events::MessageSendEvent::BroadcastChannelUpdate {
49724980
msg: update
49734981
});
49744982
}
@@ -4982,7 +4990,8 @@ where
49824990
if n >= ENABLE_GOSSIP_TICKS {
49834991
chan.set_channel_update_status(ChannelUpdateStatus::Enabled);
49844992
if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
4985-
pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
4993+
let pending_broadcast_messages = &mut self.pending_broadcast_messages.lock().unwrap();
4994+
pending_broadcast_messages.push(events::MessageSendEvent::BroadcastChannelUpdate {
49864995
msg: update
49874996
});
49884997
}
@@ -6641,9 +6650,8 @@ where
66416650
}
66426651
if let Some(ChannelPhase::Funded(chan)) = chan_option {
66436652
if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
6644-
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
6645-
let peer_state = &mut *peer_state_lock;
6646-
peer_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
6653+
let pending_broadcast_messages = &mut self.pending_broadcast_messages.lock().unwrap();
6654+
pending_broadcast_messages.push(events::MessageSendEvent::BroadcastChannelUpdate {
66476655
msg: update
66486656
});
66496657
}
@@ -7303,7 +7311,8 @@ where
73037311
if let ChannelPhase::Funded(mut chan) = remove_channel_phase!(self, chan_phase_entry) {
73047312
failed_channels.push(chan.context.force_shutdown(false, ClosureReason::HolderForceClosed));
73057313
if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
7306-
pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
7314+
let pending_broadcast_messages = &mut self.pending_broadcast_messages.lock().unwrap();
7315+
pending_broadcast_messages.push(events::MessageSendEvent::BroadcastChannelUpdate {
73077316
msg: update
73087317
});
73097318
}
@@ -7488,7 +7497,8 @@ where
74887497
// We're done with this channel. We got a closing_signed and sent back
74897498
// a closing_signed with a closing transaction to broadcast.
74907499
if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
7491-
pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
7500+
let pending_broadcast_messages = &mut self.pending_broadcast_messages.lock().unwrap();
7501+
pending_broadcast_messages.push(events::MessageSendEvent::BroadcastChannelUpdate {
74927502
msg: update
74937503
});
74947504
}
@@ -8450,6 +8460,7 @@ where
84508460
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
84518461
let peer_state = &mut *peer_state_lock;
84528462
let pending_msg_events = &mut peer_state.pending_msg_events;
8463+
84538464
peer_state.channel_by_id.retain(|_, phase| {
84548465
match phase {
84558466
// Retain unfunded channels.
@@ -8522,7 +8533,8 @@ where
85228533
let reason_message = format!("{}", reason);
85238534
failed_channels.push(channel.context.force_shutdown(true, reason));
85248535
if let Ok(update) = self.get_channel_update_for_broadcast(&channel) {
8525-
pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
8536+
let pending_broadcast_messages = &mut self.pending_broadcast_messages.lock().unwrap();
8537+
pending_broadcast_messages.push(events::MessageSendEvent::BroadcastChannelUpdate {
85268538
msg: update
85278539
});
85288540
}
@@ -8969,7 +8981,9 @@ where
89698981
// Gossip
89708982
&events::MessageSendEvent::SendChannelAnnouncement { .. } => false,
89718983
&events::MessageSendEvent::BroadcastChannelAnnouncement { .. } => true,
8972-
&events::MessageSendEvent::BroadcastChannelUpdate { .. } => true,
8984+
// [`ChannelManager::pending_broadcast_events`] holds the [`BroadcastChannelUpdate`]
8985+
// This check here is to ensure exhaustivity.
8986+
&events::MessageSendEvent::BroadcastChannelUpdate { .. } => false,
89738987
&events::MessageSendEvent::BroadcastNodeAnnouncement { .. } => true,
89748988
&events::MessageSendEvent::SendChannelUpdate { .. } => false,
89758989
&events::MessageSendEvent::SendChannelRangeQuery { .. } => false,
@@ -11880,7 +11894,6 @@ mod tests {
1188011894
assert_eq!(nodes_0_lock.len(), 1);
1188111895
assert!(nodes_0_lock.contains_key(&funding_output));
1188211896
}
11883-
1188411897
{
1188511898
// At this stage, `nodes[1]` has proposed a fee for the closing transaction in the
1188611899
// `handle_closing_signed` call above. As `nodes[1]` has not yet received the signature

‎lightning/src/ln/functional_test_utils.rs

+61-6
Original file line numberDiff line numberDiff line change
@@ -1531,11 +1531,29 @@ macro_rules! check_warn_msg {
15311531
}}
15321532
}
15331533

1534+
/// Checks if at least one peer is connected.
1535+
fn is_some_peer_connected(node: &Node) -> bool {
1536+
let peer_state = node.node.per_peer_state.read().unwrap();
1537+
for (_, peer_mutex) in peer_state.iter() {
1538+
let peer = peer_mutex.lock().unwrap();
1539+
if peer.is_connected { return true; }
1540+
}
1541+
false
1542+
}
1543+
15341544
/// Check that a channel's closing channel update has been broadcasted, and optionally
15351545
/// check whether an error message event has occurred.
15361546
pub fn check_closed_broadcast(node: &Node, num_channels: usize, with_error_msg: bool) -> Vec<msgs::ErrorMessage> {
1547+
let mut dummy_connected = false;
1548+
if !is_some_peer_connected(node) {
1549+
connect_dummy_node(&node);
1550+
dummy_connected = true;
1551+
}
15371552
let msg_events = node.node.get_and_clear_pending_msg_events();
15381553
assert_eq!(msg_events.len(), if with_error_msg { num_channels * 2 } else { num_channels });
1554+
if dummy_connected {
1555+
disconnect_dummy_node(&node);
1556+
}
15391557
msg_events.into_iter().filter_map(|msg_event| {
15401558
match msg_event {
15411559
MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
@@ -3039,6 +3057,26 @@ pub fn create_network<'a, 'b: 'a, 'c: 'b>(node_count: usize, cfgs: &'b Vec<NodeC
30393057
nodes
30403058
}
30413059

3060+
pub fn connect_dummy_node<'a, 'b: 'a, 'c: 'b>(node: &Node<'a, 'b, 'c>) {
3061+
let node_id_dummy = PublicKey::from_slice(&[2; 33]).unwrap();
3062+
3063+
let mut dummy_init_features = InitFeatures::empty();
3064+
dummy_init_features.set_static_remote_key_required();
3065+
3066+
let init_dummy = msgs::Init {
3067+
features: dummy_init_features,
3068+
networks: None,
3069+
remote_network_address: None
3070+
};
3071+
3072+
node.node.peer_connected(&node_id_dummy, &init_dummy, true).unwrap();
3073+
node.onion_messenger.peer_connected(&node_id_dummy, &init_dummy, true).unwrap();
3074+
}
3075+
3076+
pub fn disconnect_dummy_node<'a, 'b: 'a, 'c: 'b>(node: &Node<'a, 'b, 'c>) {
3077+
node.node.peer_disconnected(&PublicKey::from_slice(&[2; 33]).unwrap());
3078+
}
3079+
30423080
// Note that the following only works for CLTV values up to 128
30433081
pub const ACCEPTED_HTLC_SCRIPT_WEIGHT: usize = 137; // Here we have a diff due to HTLC CLTV expiry being < 2^15 in test
30443082
pub const ACCEPTED_HTLC_SCRIPT_WEIGHT_ANCHORS: usize = 140; // Here we have a diff due to HTLC CLTV expiry being < 2^15 in test
@@ -3150,15 +3188,21 @@ pub fn check_preimage_claim<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, prev_txn: &Vec<
31503188
}
31513189

31523190
pub fn handle_announce_close_broadcast_events<'a, 'b, 'c>(nodes: &Vec<Node<'a, 'b, 'c>>, a: usize, b: usize, needs_err_handle: bool, expected_error: &str) {
3191+
let mut dummy_connected = false;
3192+
if !is_some_peer_connected(&nodes[a]) {
3193+
connect_dummy_node(&nodes[a]);
3194+
dummy_connected = true
3195+
}
3196+
31533197
let events_1 = nodes[a].node.get_and_clear_pending_msg_events();
31543198
assert_eq!(events_1.len(), 2);
3155-
let as_update = match events_1[0] {
3199+
let as_update = match events_1[1] {
31563200
MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
31573201
msg.clone()
31583202
},
31593203
_ => panic!("Unexpected event"),
31603204
};
3161-
match events_1[1] {
3205+
match events_1[0] {
31623206
MessageSendEvent::HandleError { node_id, action: msgs::ErrorAction::SendErrorMessage { ref msg } } => {
31633207
assert_eq!(node_id, nodes[b].node.get_our_node_id());
31643208
assert_eq!(msg.data, expected_error);
@@ -3175,17 +3219,24 @@ pub fn handle_announce_close_broadcast_events<'a, 'b, 'c>(nodes: &Vec<Node<'a, '
31753219
},
31763220
_ => panic!("Unexpected event"),
31773221
}
3178-
3222+
if dummy_connected {
3223+
disconnect_dummy_node(&nodes[a]);
3224+
dummy_connected = false;
3225+
}
3226+
if !is_some_peer_connected(&nodes[b]) {
3227+
connect_dummy_node(&nodes[b]);
3228+
dummy_connected = true;
3229+
}
31793230
let events_2 = nodes[b].node.get_and_clear_pending_msg_events();
31803231
assert_eq!(events_2.len(), if needs_err_handle { 1 } else { 2 });
3181-
let bs_update = match events_2[0] {
3232+
let bs_update = match events_2.last().unwrap() {
31823233
MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
31833234
msg.clone()
31843235
},
31853236
_ => panic!("Unexpected event"),
31863237
};
31873238
if !needs_err_handle {
3188-
match events_2[1] {
3239+
match events_2[0] {
31893240
MessageSendEvent::HandleError { node_id, action: msgs::ErrorAction::SendErrorMessage { ref msg } } => {
31903241
assert_eq!(node_id, nodes[a].node.get_our_node_id());
31913242
assert_eq!(msg.data, expected_error);
@@ -3197,7 +3248,11 @@ pub fn handle_announce_close_broadcast_events<'a, 'b, 'c>(nodes: &Vec<Node<'a, '
31973248
_ => panic!("Unexpected event"),
31983249
}
31993250
}
3200-
3251+
if dummy_connected {
3252+
disconnect_dummy_node(&nodes[b]);
3253+
// Commenting the assignment to remove `unused_assignments` warning.
3254+
// dummy_connected = false;
3255+
}
32013256
for node in nodes {
32023257
node.gossip_sync.handle_channel_update(&as_update).unwrap();
32033258
node.gossip_sync.handle_channel_update(&bs_update).unwrap();

‎lightning/src/ln/functional_tests.rs

+11-8
Original file line numberDiff line numberDiff line change
@@ -2371,13 +2371,13 @@ fn channel_monitor_network_test() {
23712371
connect_blocks(&nodes[3], TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS + 1);
23722372
let events = nodes[3].node.get_and_clear_pending_msg_events();
23732373
assert_eq!(events.len(), 2);
2374-
let close_chan_update_1 = match events[0] {
2374+
let close_chan_update_1 = match events[1] {
23752375
MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
23762376
msg.clone()
23772377
},
23782378
_ => panic!("Unexpected event"),
23792379
};
2380-
match events[1] {
2380+
match events[0] {
23812381
MessageSendEvent::HandleError { action: ErrorAction::DisconnectPeer { .. }, node_id } => {
23822382
assert_eq!(node_id, nodes[4].node.get_our_node_id());
23832383
},
@@ -2403,13 +2403,13 @@ fn channel_monitor_network_test() {
24032403
connect_blocks(&nodes[4], TEST_FINAL_CLTV - CLTV_CLAIM_BUFFER + 2);
24042404
let events = nodes[4].node.get_and_clear_pending_msg_events();
24052405
assert_eq!(events.len(), 2);
2406-
let close_chan_update_2 = match events[0] {
2406+
let close_chan_update_2 = match events[1] {
24072407
MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
24082408
msg.clone()
24092409
},
24102410
_ => panic!("Unexpected event"),
24112411
};
2412-
match events[1] {
2412+
match events[0] {
24132413
MessageSendEvent::HandleError { action: ErrorAction::DisconnectPeer { .. }, node_id } => {
24142414
assert_eq!(node_id, nodes[3].node.get_our_node_id());
24152415
},
@@ -4605,7 +4605,7 @@ fn test_static_spendable_outputs_preimage_tx() {
46054605
MessageSendEvent::UpdateHTLCs { .. } => {},
46064606
_ => panic!("Unexpected event"),
46074607
}
4608-
match events[1] {
4608+
match events[2] {
46094609
MessageSendEvent::BroadcastChannelUpdate { .. } => {},
46104610
_ => panic!("Unexepected event"),
46114611
}
@@ -4648,7 +4648,7 @@ fn test_static_spendable_outputs_timeout_tx() {
46484648
mine_transaction(&nodes[1], &commitment_tx[0]);
46494649
check_added_monitors!(nodes[1], 1);
46504650
let events = nodes[1].node.get_and_clear_pending_msg_events();
4651-
match events[0] {
4651+
match events[1] {
46524652
MessageSendEvent::BroadcastChannelUpdate { .. } => {},
46534653
_ => panic!("Unexpected event"),
46544654
}
@@ -5062,7 +5062,7 @@ fn test_duplicate_payment_hash_one_failure_one_success() {
50625062
MessageSendEvent::UpdateHTLCs { .. } => {},
50635063
_ => panic!("Unexpected event"),
50645064
}
5065-
match events[1] {
5065+
match events[2] {
50665066
MessageSendEvent::BroadcastChannelUpdate { .. } => {},
50675067
_ => panic!("Unexepected event"),
50685068
}
@@ -5140,7 +5140,7 @@ fn test_dynamic_spendable_outputs_local_htlc_success_tx() {
51405140
MessageSendEvent::UpdateHTLCs { .. } => {},
51415141
_ => panic!("Unexpected event"),
51425142
}
5143-
match events[1] {
5143+
match events[2] {
51445144
MessageSendEvent::BroadcastChannelUpdate { .. } => {},
51455145
_ => panic!("Unexepected event"),
51465146
}
@@ -7321,6 +7321,9 @@ fn test_announce_disable_channels() {
73217321
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
73227322
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
73237323

7324+
// Connect a dummy node for proper future events broadcasting
7325+
connect_dummy_node(&nodes[0]);
7326+
73247327
create_announced_chan_between_nodes(&nodes, 0, 1);
73257328
create_announced_chan_between_nodes(&nodes, 1, 0);
73267329
create_announced_chan_between_nodes(&nodes, 0, 1);

0 commit comments

Comments
 (0)