Skip to content

Commit 15792af

Browse files
committed
Re-enable the test, and fix grammatical mistake in comments
1 parent 59e6af3 commit 15792af

File tree

2 files changed

+113
-92
lines changed

2 files changed

+113
-92
lines changed

lightning/src/ln/channelmanager.rs

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -892,8 +892,9 @@ impl <SP: Deref> PeerState<SP> where SP::Target: SignerProvider {
892892
if require_disconnected && self.is_connected {
893893
return false
894894
}
895-
self.channel_by_id.iter().filter(|(_, phase)| matches!(phase, ChannelPhase::Funded(_))).count() == 0
896-
&& self.channel_by_id.iter().filter(|(_, phase)| matches!(phase, ChannelPhase::UnfundedOutboundV1(_))).count() == 0
895+
!self.channel_by_id.iter().any(|(_, phase)|
896+
matches!(phase, ChannelPhase::Funded(_) | ChannelPhase::UnfundedOutboundV1(_))
897+
)
897898
&& self.monitor_update_blocked_actions.is_empty()
898899
&& self.in_flight_monitor_updates.is_empty()
899900
}
@@ -9015,20 +9016,29 @@ where
90159016
let pending_msg_events = &mut peer_state.pending_msg_events;
90169017

90179018
for (_, phase) in peer_state.channel_by_id.iter_mut() {
9018-
if let ChannelPhase::Funded(chan) = phase {
9019-
let logger = WithChannelContext::from(&self.logger, &chan.context);
9020-
pending_msg_events.push(events::MessageSendEvent::SendChannelReestablish {
9021-
node_id: chan.context.get_counterparty_node_id(),
9022-
msg: chan.get_channel_reestablish(&&logger),
9023-
});
9024-
}
9025-
else if let ChannelPhase::UnfundedOutboundV1(chan) = phase {
9026-
pending_msg_events.push(events::MessageSendEvent::SendOpenChannel {
9027-
node_id: chan.context.get_counterparty_node_id(),
9028-
msg: chan.get_open_channel(self.chain_hash),
9029-
});
9019+
match phase {
9020+
ChannelPhase::Funded(chan) => {
9021+
let logger = WithChannelContext::from(&self.logger, &chan.context);
9022+
pending_msg_events.push(events::MessageSendEvent::SendChannelReestablish {
9023+
node_id: chan.context.get_counterparty_node_id(),
9024+
msg: chan.get_channel_reestablish(&&logger),
9025+
});
9026+
}
9027+
9028+
ChannelPhase::UnfundedOutboundV1(chan) => {
9029+
pending_msg_events.push(events::MessageSendEvent::SendOpenChannel {
9030+
node_id: chan.context.get_counterparty_node_id(),
9031+
msg: chan.get_open_channel(self.chain_hash),
9032+
});
9033+
}
9034+
9035+
ChannelPhase::UnfundedInboundV1(_) => {
9036+
// Since unfunded inbound channel maps are cleared upon disconnecting a peer, and they're not
9037+
// persisted (so won't be recovered after a crash), they shouldn't exist here and we would never
9038+
// need to worry about closing and removing them.
9039+
debug_assert!(false);
9040+
}
90309041
}
9031-
// else don't do anything if the channel is UnfundedInbound Channel.
90329042
}
90339043
}
90349044

@@ -11146,7 +11156,7 @@ mod tests {
1114611156
use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey};
1114711157
use core::sync::atomic::Ordering;
1114811158
use crate::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, ClosureReason};
11149-
use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret};
11159+
use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret};
1115011160
use crate::ln::ChannelId;
1115111161
use crate::ln::channelmanager::{create_recv_pending_htlc_info, HTLCForwardInfo, inbound_payment, PaymentId, PaymentSendFailure, RecipientOnionFields, InterceptId};
1115211162
use crate::ln::functional_test_utils::*;

lightning/src/ln/functional_tests.rs

Lines changed: 87 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -3692,13 +3692,7 @@ fn test_dup_events_on_peer_disconnect() {
36923692
expect_payment_path_successful!(nodes[0]);
36933693
}
36943694

3695-
3696-
// The following test is disabled because we no longer close the channel
3697-
// immediately after funding brodcasts. Instead we wait for some time for
3698-
// the peer to reconnect back, and only close it after it become stale for
3699-
// UNFUNDED_CHANNEL_AGE_LIMIT_TICKS
37003695
#[test]
3701-
#[ignore]
37023696
fn test_peer_disconnected_before_funding_broadcasted() {
37033697
// Test that channels are closed with `ClosureReason::DisconnectedPeer` if the peer disconnects
37043698
// before the funding transaction has been broadcasted, and doesn't reconnect back within time.
@@ -3730,12 +3724,17 @@ fn test_peer_disconnected_before_funding_broadcasted() {
37303724
assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 0);
37313725
}
37323726

3733-
// Ensure that the channel is closed after timeout with `ClosureReason::DisconnectedPeer`
3727+
// Ensure that the channel is closed if the peer does not reconnect within the timeout period
37343728
// when the peers are disconnected before the funding transaction was broadcasted.
37353729
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
37363730
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
37373731

3738-
check_closed_event!(&nodes[0], 2, ClosureReason::DisconnectedPeer, true
3732+
// After the time expires for allowing peer to connect back
3733+
for _ in 0..UNFUNDED_CHANNEL_AGE_LIMIT_TICKS {
3734+
nodes[0].node.timer_tick_occurred();
3735+
}
3736+
3737+
check_closed_event!(&nodes[0], 2, ClosureReason::HolderForceClosed, true
37393738
, [nodes[1].node.get_our_node_id()], 1000000);
37403739
check_closed_event!(&nodes[1], 1, ClosureReason::DisconnectedPeer, false
37413740
, [nodes[0].node.get_our_node_id()], 1000000);
@@ -10512,88 +10511,100 @@ fn test_remove_expired_inbound_unfunded_channels() {
1051210511
}
1051310512

1051410513
#[test]
10515-
fn test_channel_close_when_not_timely_accepted() {
10516-
// Create network of two nodes
10517-
let chanmon_cfgs = create_chanmon_cfgs(2);
10518-
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
10519-
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
10520-
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
10514+
fn test_channel_close_when_not_timely_accepted() {
10515+
// Create network of two nodes
10516+
let chanmon_cfgs = create_chanmon_cfgs(2);
10517+
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
10518+
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
10519+
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
1052110520

10522-
// Simulate peer-diconnects mid-handshake
10523-
// The channel is initiated from the node 0 side,
10524-
// But the nodes disconnects before node 1 could send accept channel
10525-
let create_chan_id = nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None, None).unwrap();
10526-
let open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
10527-
assert_eq!(open_channel_msg.temporary_channel_id, create_chan_id);
10521+
// Simulate peer-disconnects mid-handshake
10522+
// The channel is initiated from the node 0 side,
10523+
// but the nodes disconnect before node 1 could send accept channel
10524+
let create_chan_id = nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None, None).unwrap();
10525+
let open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
10526+
assert_eq!(open_channel_msg.temporary_channel_id, create_chan_id);
1052810527

10529-
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
10530-
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
10528+
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
10529+
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
1053110530

10532-
{
10533-
// Make sure that we have not removed the OutboundV1Channel from node[0] immediately.
10534-
let node_0_per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
10535-
let per_peer_state = node_0_per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
10536-
assert_eq!(per_peer_state.channel_by_id.len(), 1);
10537-
}
10531+
{
10532+
// Make sure that we have not removed the OutboundV1Channel from node[0] immediately.
10533+
let node_0_per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
10534+
let per_peer_state = node_0_per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
10535+
assert_eq!(per_peer_state.channel_by_id.len(), 1);
10536+
}
1053810537

10539-
// In the meantime, some time passes.
10540-
for _ in 0..UNFUNDED_CHANNEL_AGE_LIMIT_TICKS {
10541-
nodes[0].node.timer_tick_occurred();
10542-
}
10538+
{
10539+
// Since channel was inbound from node[1] perspective, it should have been immediately dropped.
10540+
let node_1_per_peer_state = nodes[1].node.per_peer_state.read().unwrap();
10541+
let per_peer_state = node_1_per_peer_state.get(&nodes[0].node.get_our_node_id());
10542+
assert!(per_peer_state.is_none());
10543+
}
1054310544

10544-
// Since we disconnected from peer and did not connect back within time
10545-
// We should have forced-closed the channel by now.
10546-
check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed, [nodes[1].node.get_our_node_id()], 100000);
10545+
// In the meantime, some time passes.
10546+
for _ in 0..UNFUNDED_CHANNEL_AGE_LIMIT_TICKS {
10547+
nodes[0].node.timer_tick_occurred();
10548+
}
1054710549

10548-
{
10549-
// Since accept channel message was never received
10550-
// The channel should be forced close by now from node 0 side
10551-
// and the peer removed from per_peer_state
10552-
let node_0_per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
10553-
assert_eq!(node_0_per_peer_state.len(), 0);
10554-
}
10550+
// Since we disconnected from peer and did not connect back within time,
10551+
// we should have forced-closed the channel by now.
10552+
check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed, [nodes[1].node.get_our_node_id()], 100000);
1055510553

10554+
{
10555+
// Since accept channel message was never received
10556+
// The channel should be forced close by now from node 0 side
10557+
// and the peer removed from per_peer_state
10558+
let node_0_per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
10559+
assert_eq!(node_0_per_peer_state.len(), 0);
1055610560
}
10561+
}
1055710562

10558-
#[test]
10559-
fn test_rebroadcast_open_channel_when_reconnect_mid_handshake() {
10560-
// Create network of two nodes
10561-
let chanmon_cfgs = create_chanmon_cfgs(2);
10562-
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
10563-
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
10564-
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
10565-
10566-
// Simulate peer-diconnects mid-handshake
10567-
// The channel is initiated from the node 0 side,
10568-
// But the nodes disconnects before node 1 could send accept channel
10569-
let create_chan_id = nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None, None).unwrap();
10570-
let open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
10571-
assert_eq!(open_channel_msg.temporary_channel_id, create_chan_id);
10563+
#[test]
10564+
fn test_rebroadcast_open_channel_when_reconnect_mid_handshake() {
10565+
// Create network of two nodes
10566+
let chanmon_cfgs = create_chanmon_cfgs(2);
10567+
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
10568+
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
10569+
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
1057210570

10573-
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
10574-
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
10571+
// Simulate peer-disconnects mid-handshake
10572+
// The channel is initiated from the node 0 side,
10573+
// but the nodes disconnect before node 1 could send accept channel
10574+
let create_chan_id = nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None, None).unwrap();
10575+
let open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
10576+
assert_eq!(open_channel_msg.temporary_channel_id, create_chan_id);
1057510577

10576-
{
10577-
// Make sure that we have not removed the OutboundV1Channel from node[0] immediately.
10578-
let node_0_per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
10579-
let per_peer_state = node_0_per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
10580-
assert_eq!(per_peer_state.channel_by_id.len(), 1);
10581-
}
10578+
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
10579+
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
1058210580

10583-
// The peers now reconnect
10584-
nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init {
10585-
features: nodes[1].node.init_features(), networks: None, remote_network_address: None
10586-
}, true).unwrap();
10587-
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init {
10588-
features: nodes[0].node.init_features(), networks: None, remote_network_address: None
10589-
}, false).unwrap();
10581+
{
10582+
// Make sure that we have not removed the OutboundV1Channel from node[0] immediately.
10583+
let node_0_per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
10584+
let per_peer_state = node_0_per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
10585+
assert_eq!(per_peer_state.channel_by_id.len(), 1);
10586+
}
1059010587

10591-
// Make sure the SendOpenChannel message is added to
10592-
// node_0 pending message events
10593-
let events = nodes[0].node.get_and_clear_pending_msg_events();
10594-
assert_eq!(events.len(), 1);
10588+
{
10589+
// Since channel was inbound from node[1] perspective, it should have been immediately dropped.
10590+
let node_1_per_peer_state = nodes[1].node.per_peer_state.read().unwrap();
10591+
let per_peer_state = node_1_per_peer_state.get(&nodes[0].node.get_our_node_id());
10592+
assert!(per_peer_state.is_none());
1059510593
}
1059610594

10595+
// The peers now reconnect
10596+
nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init {
10597+
features: nodes[1].node.init_features(), networks: None, remote_network_address: None
10598+
}, true).unwrap();
10599+
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init {
10600+
features: nodes[0].node.init_features(), networks: None, remote_network_address: None
10601+
}, false).unwrap();
10602+
10603+
// Make sure the SendOpenChannel message is added to
10604+
// node_0 pending message events
10605+
let events = nodes[0].node.get_and_clear_pending_msg_events();
10606+
assert_eq!(events.len(), 1);
10607+
}
1059710608

1059810609
fn do_test_multi_post_event_actions(do_reload: bool) {
1059910610
// Tests handling multiple post-Event actions at once.
@@ -10783,7 +10794,7 @@ fn test_disconnect_in_funding_batch() {
1078310794
nodes[0].node.timer_tick_occurred();
1078410795
}
1078510796

10786-
// The channels in the batch will close immediately.
10797+
// The channels in the batch will close after timeout.
1078710798
let funding_txo_1 = OutPoint { txid: tx.txid(), index: 0 };
1078810799
let funding_txo_2 = OutPoint { txid: tx.txid(), index: 1 };
1078910800
let channel_id_1 = funding_txo_1.to_channel_id();

0 commit comments

Comments
 (0)