Skip to content

Commit

Permalink
Re-enable the test, improve the comments, and handle general cleanup
Browse files Browse the repository at this point in the history 
- Repurpose the test_disconnect_in_funding_batch to test that all
  channels in the batch close when one them closes
  • Loading branch information
@shaavan
shaavan committed Jan 27, 2024
1 parent 231a0bd commit 0fd3d31
Show file tree
Hide file tree
Showing 2 changed files with 118 additions and 99 deletions.
42 changes: 26 additions & 16 deletions lightning/src/ln/channelmanager.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -892,8 +892,9 @@ impl <SP: Deref> PeerState<SP> where SP::Target: SignerProvider {
if require_disconnected && self.is_connected {
return false
}
self.channel_by_id.iter().filter(|(_, phase)| matches!(phase, ChannelPhase::Funded(_))).count() == 0
&& self.channel_by_id.iter().filter(|(_, phase)| matches!(phase, ChannelPhase::UnfundedOutboundV1(_))).count() == 0
!self.channel_by_id.iter().any(|(_, phase)|
matches!(phase, ChannelPhase::Funded(_) | ChannelPhase::UnfundedOutboundV1(_))
)
&& self.monitor_update_blocked_actions.is_empty()
&& self.in_flight_monitor_updates.is_empty()
}
Expand Down Expand Up @@ -9015,20 +9016,29 @@ where
let pending_msg_events = &mut peer_state.pending_msg_events;

for (_, phase) in peer_state.channel_by_id.iter_mut() {
if let ChannelPhase::Funded(chan) = phase {
let logger = WithChannelContext::from(&self.logger, &chan.context);
pending_msg_events.push(events::MessageSendEvent::SendChannelReestablish {
node_id: chan.context.get_counterparty_node_id(),
msg: chan.get_channel_reestablish(&&logger),
});
}
else if let ChannelPhase::UnfundedOutboundV1(chan) = phase {
pending_msg_events.push(events::MessageSendEvent::SendOpenChannel {
node_id: chan.context.get_counterparty_node_id(),
msg: chan.get_open_channel(self.chain_hash),
});
match phase {
ChannelPhase::Funded(chan) => {
let logger = WithChannelContext::from(&self.logger, &chan.context);
pending_msg_events.push(events::MessageSendEvent::SendChannelReestablish {
node_id: chan.context.get_counterparty_node_id(),
msg: chan.get_channel_reestablish(&&logger),
});
}

ChannelPhase::UnfundedOutboundV1(chan) => {
pending_msg_events.push(events::MessageSendEvent::SendOpenChannel {
node_id: chan.context.get_counterparty_node_id(),
msg: chan.get_open_channel(self.chain_hash),
});
}

ChannelPhase::UnfundedInboundV1(_) => {
// Since unfunded inbound channel maps are cleared upon disconnecting a peer,
// they are not persisted and won't be recovered after a crash.
// Therefore, they shouldn't exist at this point.
debug_assert!(false);
}
}
// else don't do anything if the channel is UnfundedInbound Channel.
}
}

Expand Down Expand Up @@ -11146,7 +11156,7 @@ mod tests {
use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey};
use core::sync::atomic::Ordering;
use crate::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, ClosureReason};
use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret};
use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret};
use crate::ln::ChannelId;
use crate::ln::channelmanager::{create_recv_pending_htlc_info, HTLCForwardInfo, inbound_payment, PaymentId, PaymentSendFailure, RecipientOnionFields, InterceptId};
use crate::ln::functional_test_utils::*;
Expand Down
175 changes: 92 additions & 83 deletions lightning/src/ln/functional_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3692,13 +3692,7 @@ fn test_dup_events_on_peer_disconnect() {
expect_payment_path_successful!(nodes[0]);
}


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

// Ensure that the channel is closed after timeout with `ClosureReason::DisconnectedPeer`
// when the peers are disconnected before the funding transaction was broadcasted.
// The peers disconnect before the funding is broadcasted.
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());

check_closed_event!(&nodes[0], 2, ClosureReason::DisconnectedPeer, true
// The time for peers to reconnect expires.
for _ in 0..UNFUNDED_CHANNEL_AGE_LIMIT_TICKS {
nodes[0].node.timer_tick_occurred();
}

// Ensure that the channel is closed with `ClosureReason::HolderForceClosed`
// when the peers are disconnected and do not reconnect before the funding
// transaction is broadcasted.
check_closed_event!(&nodes[0], 2, ClosureReason::HolderForceClosed, true
, [nodes[1].node.get_our_node_id()], 1000000);
check_closed_event!(&nodes[1], 1, ClosureReason::DisconnectedPeer, false
, [nodes[0].node.get_our_node_id()], 1000000);
Expand Down Expand Up @@ -10512,22 +10513,22 @@ fn test_remove_expired_inbound_unfunded_channels() {
}

#[test]
fn test_channel_close_when_not_timely_accepted() {
// Create network of two nodes
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]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
fn test_channel_close_when_not_timely_accepted() {
// Create network of two nodes
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]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);

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

nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());

// Make sure that we have not removed the OutboundV1Channel from node[0] immediately.
assert_eq!(nodes[0].node.list_channels().len(), 1);
Expand All @@ -10539,42 +10540,41 @@ fn test_remove_expired_inbound_unfunded_channels() {
assert!(per_peer_state.is_none());
}

// In the meantime, some time passes.
for _ in 0..UNFUNDED_CHANNEL_AGE_LIMIT_TICKS {
nodes[0].node.timer_tick_occurred();
}

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

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

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

#[test]
fn test_rebroadcast_open_channel_when_reconnect_mid_handshake() {
// Create network of two nodes
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]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
#[test]
fn test_rebroadcast_open_channel_when_reconnect_mid_handshake() {
// Create network of two nodes
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]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);

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

nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());

// Make sure that we have not removed the OutboundV1Channel from node[0] immediately.
assert_eq!(nodes[0].node.list_channels().len(), 1);
Expand All @@ -10586,13 +10586,13 @@ fn test_remove_expired_inbound_unfunded_channels() {
assert!(per_peer_state.is_none());
}

// The peers now reconnect
nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init {
features: nodes[1].node.init_features(), networks: None, remote_network_address: None
}, true).unwrap();
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init {
features: nodes[0].node.init_features(), networks: None, remote_network_address: None
}, false).unwrap();
// The peers now reconnect
nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init {
features: nodes[1].node.init_features(), networks: None, remote_network_address: None
}, true).unwrap();
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init {
features: nodes[0].node.init_features(), networks: None, remote_network_address: None
}, false).unwrap();

// Make sure the SendOpenChannel message is added to node_0 pending message events
let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
Expand Down Expand Up @@ -10759,7 +10759,9 @@ fn test_batch_channel_open() {
}

#[test]
fn test_disconnect_in_funding_batch() {
fn test_close_in_funding_batch() {
// This test ensures that if one of the channels
// in the batch closes, the complete batch will close.
let chanmon_cfgs = create_chanmon_cfgs(3);
let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
Expand All @@ -10783,19 +10785,39 @@ fn test_disconnect_in_funding_batch() {
// The transaction should not have been broadcast before all channels are ready.
assert_eq!(nodes[0].tx_broadcaster.txn_broadcast().len(), 0);

// The remaining peer in the batch disconnects.
nodes[0].node.peer_disconnected(&nodes[2].node.get_our_node_id());

// After the time expires for allowing peer to connect back
for _ in 0..UNFUNDED_CHANNEL_AGE_LIMIT_TICKS {
nodes[0].node.timer_tick_occurred();
}

// The channels in the batch will close immediately.
// Force-close the channel for which we've completed the initial monitor.
let funding_txo_1 = OutPoint { txid: tx.txid(), index: 0 };
let funding_txo_2 = OutPoint { txid: tx.txid(), index: 1 };
let channel_id_1 = funding_txo_1.to_channel_id();
let channel_id_2 = funding_txo_2.to_channel_id();

nodes[0].node.force_close_broadcasting_latest_txn(&channel_id_1, &nodes[1].node.get_our_node_id()).unwrap();

// The monitor should become closed.
check_added_monitors(&nodes[0], 1);
{
let mut monitor_updates = nodes[0].chain_monitor.monitor_updates.lock().unwrap();
let monitor_updates_1 = monitor_updates.get(&channel_id_1).unwrap();
assert_eq!(monitor_updates_1.len(), 1);
assert_eq!(monitor_updates_1[0].update_id, CLOSED_CHANNEL_UPDATE_ID);
}

let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
match msg_events[0] {
MessageSendEvent::HandleError { .. } => (),
_ => panic!("Unexpected message."),
}

// We broadcast the commitment transaction as part of the force-close.
{
let broadcasted_txs = nodes[0].tx_broadcaster.txn_broadcast();
assert_eq!(broadcasted_txs.len(), 1);
assert!(broadcasted_txs[0].txid() != tx.txid());
assert_eq!(broadcasted_txs[0].input.len(), 1);
assert_eq!(broadcasted_txs[0].input[0].previous_output.txid, tx.txid());
}

// All channels in the batch should close immediately.
check_closed_events(&nodes[0], &[
ExpectedCloseEvent {
channel_id: Some(channel_id_1),
Expand All @@ -10813,19 +10835,6 @@ fn test_disconnect_in_funding_batch() {
},
]);

// The monitor should become closed.
check_added_monitors(&nodes[0], 1);
{
let mut monitor_updates = nodes[0].chain_monitor.monitor_updates.lock().unwrap();
let monitor_updates_1 = monitor_updates.get(&channel_id_1).unwrap();
assert_eq!(monitor_updates_1.len(), 1);
assert_eq!(monitor_updates_1[0].update_id, CLOSED_CHANNEL_UPDATE_ID);
}

// The funding transaction should not have been broadcast, and therefore, we don't need
// to broadcast a force-close transaction for the closed monitor.
assert_eq!(nodes[0].tx_broadcaster.txn_broadcast().len(), 0);

// Ensure the channels don't exist anymore.
assert!(nodes[0].node.list_channels().is_empty());
}
Expand Down

0 comments on commit 0fd3d31

Please sign in to comment.