Skip to content

Commit

Permalink
Introduce new code changes properly handling this new behavior
Browse files Browse the repository at this point in the history
- Do not remove channel immediately when peer_disconnect, instead
  removed it after some time if peer doesn't reconnect soon (handled in
previous commit).
- Do not mark per ok_to_remove if we have some OutboundV1Channels too.
- Rebroadcast SendOpenChannel for outboundV1Channel when peer
  reconnects.
- Also, update the relevant tests to account for the new behavior change
  • Loading branch information
shaavan committed Dec 12, 2023
1 parent 5c14929 commit 5e6e3bc
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 19 deletions.
41 changes: 25 additions & 16 deletions lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -874,6 +874,7 @@ impl <SP: Deref> PeerState<SP> where SP::Target: SignerProvider {
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.monitor_update_blocked_actions.is_empty()
&& self.in_flight_monitor_updates.is_empty()
}
Expand Down Expand Up @@ -8734,10 +8735,12 @@ where
}
&mut chan.context
},
// Unfunded channels will always be removed.
ChannelPhase::UnfundedOutboundV1(chan) => {
&mut chan.context
// We retain UnfundedOutboundV1 channel for some time in case
// peer unexpectedly disconnects, and intends to reconnect again.
ChannelPhase::UnfundedOutboundV1(_) => {
return true;
},
// Unfunded inbound channels will always be removed.
ChannelPhase::UnfundedInboundV1(chan) => {
&mut chan.context
},
Expand Down Expand Up @@ -8877,21 +8880,27 @@ where
let peer_state = &mut *peer_state_lock;
let pending_msg_events = &mut peer_state.pending_msg_events;

peer_state.channel_by_id.iter_mut().filter_map(|(_, phase)|
if let ChannelPhase::Funded(chan) = phase { Some(chan) } else {
// Since unfunded channel maps are cleared upon disconnecting a peer, and they're not persisted
// (so won't be recovered after a crash), they shouldn't exist here and we would never need to
// worry about closing and removing them.
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),
});
}
else {
// Since unfunded inbound channel maps are cleared upon disconnecting a peer, and they're not
// persisted (so won't be recovered after a crash), they shouldn't exist here and we would never
// need to worry about closing and removing them.
debug_assert!(false);
None
}
).for_each(|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),
});
});
}
}

return NotifyOption::SkipPersistHandleEvents;
Expand Down
14 changes: 11 additions & 3 deletions lightning/src/ln/functional_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3692,7 +3692,7 @@ fn test_dup_events_on_peer_disconnect() {
#[test]
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.
// before the funding transaction has been broadcasted, and doesn't reconnect back within time.
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]);
Expand Down Expand Up @@ -3721,11 +3721,15 @@ 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 with `ClosureReason::DisconnectedPeer` when the peers are
// disconnected before the funding transaction was broadcasted.
// Ensure that the channel is closed after timeout with `ClosureReason::DisconnectedPeer`
// when the peers are disconnected before the funding transaction was 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());

// The timer ticks while waiting for peer to connect back
nodes[0].node.timer_tick_occurred();
nodes[0].node.timer_tick_occurred();

check_closed_event!(&nodes[0], 2, ClosureReason::DisconnectedPeer, true
, [nodes[1].node.get_our_node_id()], 1000000);
check_closed_event!(&nodes[1], 1, ClosureReason::DisconnectedPeer, false
Expand Down Expand Up @@ -10526,6 +10530,10 @@ fn test_disconnect_in_funding_batch() {
// 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
nodes[0].node.timer_tick_occurred();
nodes[0].node.timer_tick_occurred();

// The channels in the batch will close immediately.
let channel_id_1 = OutPoint { txid: tx.txid(), index: 0 }.to_channel_id();
let channel_id_2 = OutPoint { txid: tx.txid(), index: 1 }.to_channel_id();
Expand Down

0 comments on commit 5e6e3bc

Please sign in to comment.