Skip to content

Commit f227010

Browse files
committed
f basically rewrite the whole thing, this time with tests
1 parent 6f0354a commit f227010

File tree

1 file changed

+162
-30
lines changed

1 file changed

+162
-30
lines changed

lightning/src/ln/channelmanager.rs

+162-30
Original file line numberDiff line numberDiff line change
@@ -4251,6 +4251,38 @@ where
42514251
Ok(())
42524252
}
42534253

4254+
/// Gets the number of peers which match the given filter which have no funded or outbound
4255+
/// channels.
4256+
///
4257+
/// The filter is called for each node and provided with the number of unfunded channels the
4258+
/// node has.
4259+
///
4260+
/// If the peer in question has funded channels, however, zero will be returned.
4261+
fn peers_without_funded_channels<Filter>(&self, mut filter: Filter) -> usize
4262+
where Filter: FnMut(&PublicKey, &PeerState<<SP::Target as SignerProvider>::Signer>, usize) -> bool {
4263+
let mut peers_without_funded_channels = 0;
4264+
let best_block_height = self.best_block.read().unwrap().height();
4265+
{
4266+
let peer_state_lock = self.per_peer_state.read().unwrap();
4267+
for (node_id, peer_mtx) in peer_state_lock.iter() {
4268+
let peer = peer_mtx.lock().unwrap();
4269+
let mut num_unfunded_channels = 0;
4270+
for (_, chan) in peer.channel_by_id.iter() {
4271+
if !chan.is_outbound() && chan.minimum_depth().unwrap_or(1) != 0 &&
4272+
chan.get_funding_tx_confirmations(best_block_height) == 0
4273+
{
4274+
num_unfunded_channels += 1;
4275+
}
4276+
}
4277+
if !filter(node_id, &*peer, num_unfunded_channels) { continue; }
4278+
if num_unfunded_channels == peer.channel_by_id.len() {
4279+
peers_without_funded_channels += 1;
4280+
}
4281+
}
4282+
}
4283+
return peers_without_funded_channels;
4284+
}
4285+
42544286
fn internal_open_channel(&self, counterparty_node_id: &PublicKey, msg: &msgs::OpenChannel) -> Result<(), MsgHandleErrInternal> {
42554287
if msg.chain_hash != self.genesis_hash {
42564288
return Err(MsgHandleErrInternal::send_err_msg_no_close("Unknown genesis block hash".to_owned(), msg.temporary_channel_id.clone()));
@@ -4270,25 +4302,37 @@ where
42704302
if let None = peer_state_mutex_opt {
42714303
return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id), msg.temporary_channel_id.clone()))
42724304
}
4273-
let mut peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap();
4274-
let peer_state = &mut *peer_state_lock;
42754305

4276-
let mut unfunded_channels = 0;
4277-
let best_block_height = self.best_block.read().unwrap().height();
4278-
for (_, chan) in peer_state.channel_by_id.iter() {
4279-
if chan.get_funding_tx_confirmations(best_block_height) == 0 && chan.minimum_depth() != 0 {
4280-
unfunded_channels += 1;
4281-
}
4282-
}
4283-
if unfunded_channels >= MAX_UNFUNDED_CHANS_PER_PEER {
4306+
// Get the number of peers with channels, but without funded ones. We don't care too much
4307+
// about peers that never open a channel, so we filter by peers that have at least one
4308+
// channel, and then limit the number of those with unfunded channels.
4309+
let mut this_node_unfunded_channels = 0;
4310+
let peers_with_unfunded_channels = self.peers_without_funded_channels(
4311+
|node_id, node, unfunded_channels| {
4312+
if node_id == counterparty_node_id {
4313+
this_node_unfunded_channels = unfunded_channels;
4314+
}
4315+
!node.channel_by_id.is_empty()
4316+
});
4317+
if this_node_unfunded_channels >= MAX_UNFUNDED_CHANS_PER_PEER {
42844318
return Err(MsgHandleErrInternal::send_err_msg_no_close(
42854319
format!("Refusing more than {} unfunded channels.", MAX_UNFUNDED_CHANS_PER_PEER),
42864320
msg.temporary_channel_id.clone()));
42874321
}
4322+
let mut peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap();
4323+
let peer_state = &mut *peer_state_lock;
4324+
// If this peer already has some channels, a new channel won't increase our number of peers
4325+
// with unfunded channels, so as long as we aren't over the maximum number of unfunded
4326+
// channels per-peer we can accept channels from a peer with existing ones.
4327+
if peer_state.channel_by_id.is_empty() && peers_with_unfunded_channels >= MAX_NO_CHANNEL_PEERS {
4328+
return Err(MsgHandleErrInternal::send_err_msg_no_close(
4329+
"Have too many peers with unfunded channels, not accepting new ones".to_owned(),
4330+
msg.temporary_channel_id.clone()));
4331+
}
42884332

42894333
let mut channel = match Channel::new_from_req(&self.fee_estimator, &self.entropy_source, &self.signer_provider,
42904334
counterparty_node_id.clone(), &self.channel_type_features(), &peer_state.latest_features, msg, user_channel_id,
4291-
&self.default_configuration, best_block_height, &self.logger, outbound_scid_alias)
4335+
&self.default_configuration, self.best_block.read().unwrap().height(), &self.logger, outbound_scid_alias)
42924336
{
42934337
Err(e) => {
42944338
self.outbound_scid_aliases.lock().unwrap().remove(&outbound_scid_alias);
@@ -6241,26 +6285,19 @@ where
62416285

62426286
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
62436287

6244-
let mut peers_with_unfunded_channels = 0;
6245-
let mut peer_has_channels = false;
6246-
let best_block_height = self.best_block.read().unwrap().height();
6247-
{
6248-
let peer_state_lock = self.per_peer_state.read().unwrap();
6249-
for (node_id, peer) in peer_state_lock.iter() {
6250-
if node_id == counterparty_node_id {
6251-
peer_has_channels = true;
6252-
break;
6253-
}
6254-
let mut has_funded_channels = false;
6255-
for (_, chan) in peer.lock().unwrap().channel_by_id.iter() {
6256-
if chan.get_funding_tx_confirmations(best_block_height) != 0 || chan.minimum_depth() == 0 {
6257-
has_funded_channels = true;
6258-
}
6288+
// If we have too many peers connected which don't have funded channels, disconnect the
6289+
// peer immediately. If we have a bunch of unfunded channels taking up space in memory for
6290+
// disconnected peers, we still let new peers connect, but we'll reject new channels from
6291+
// them.
6292+
let mut this_peer_has_funded_channels = false;
6293+
let connected_peers_without_funded_channels = self.peers_without_funded_channels(
6294+
|node_id, node, num_unfunded_channels| {
6295+
if node_id == counterparty_node_id && num_unfunded_channels != node.channel_by_id.len() {
6296+
this_peer_has_funded_channels = true;
62596297
}
6260-
if !has_funded_channels { peers_with_unfunded_channels += 1 }
6261-
}
6262-
}
6263-
if !peer_has_channels && inbound && peers_with_unfunded_channels >= MAX_NO_CHANNEL_PEERS {
6298+
node.is_connected
6299+
});
6300+
if inbound && !this_peer_has_funded_channels && connected_peers_without_funded_channels >= MAX_NO_CHANNEL_PEERS {
62646301
return Err(());
62656302
}
62666303

@@ -8449,6 +8486,101 @@ mod tests {
84498486
nodes[1].node.handle_update_fee(&unkown_public_key, &update_fee_msg);
84508487
}
84518488

8489+
#[test]
8490+
fn test_connection_limiting() {
8491+
// Test that we limit un-channel'd peers and un-funded channels properly.
8492+
let chanmon_cfgs = create_chanmon_cfgs(2);
8493+
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
8494+
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
8495+
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
8496+
8497+
// Note that create_network connects the nodes together for us
8498+
8499+
nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100_000, 0, 42, None).unwrap();
8500+
let mut open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
8501+
8502+
let mut funding_tx = None;
8503+
for (idx, _) in (0..super::MAX_UNFUNDED_CHANS_PER_PEER).enumerate() {
8504+
nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_channel_msg);
8505+
let accept_channel = get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id());
8506+
8507+
if idx == 0 {
8508+
nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), &accept_channel);
8509+
let (temporary_channel_id, tx, _) = create_funding_transaction(&nodes[0], &nodes[1].node.get_our_node_id(), 100_000, 42);
8510+
funding_tx = Some(tx.clone());
8511+
nodes[0].node.funding_transaction_generated(&temporary_channel_id, &nodes[1].node.get_our_node_id(), tx).unwrap();
8512+
let funding_created_msg = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
8513+
8514+
nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created_msg);
8515+
check_added_monitors!(nodes[1], 1);
8516+
let funding_signed = get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id());
8517+
8518+
nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &funding_signed);
8519+
check_added_monitors!(nodes[0], 1);
8520+
}
8521+
open_channel_msg.temporary_channel_id = nodes[0].keys_manager.get_secure_random_bytes();
8522+
}
8523+
8524+
// A MAX_UNFUNDED_CHANS_PER_PEER + 1 channel will be summarily rejected
8525+
open_channel_msg.temporary_channel_id = nodes[0].keys_manager.get_secure_random_bytes();
8526+
nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_channel_msg);
8527+
assert_eq!(get_err_msg!(nodes[1], nodes[0].node.get_our_node_id()).channel_id,
8528+
open_channel_msg.temporary_channel_id);
8529+
8530+
// Further, because all of our channels with nodes[0] are inbound, and none of them funded,
8531+
// it doesn't count as a "protected" peer, counting towards the MAX_NO_CHANNEL_PEERS limit.
8532+
let mut peer_pks = Vec::with_capacity(super::MAX_NO_CHANNEL_PEERS);
8533+
for _ in 1..super::MAX_NO_CHANNEL_PEERS {
8534+
let random_pk = PublicKey::from_secret_key(&nodes[0].node.secp_ctx,
8535+
&SecretKey::from_slice(&nodes[1].keys_manager.get_secure_random_bytes()).unwrap());
8536+
peer_pks.push(random_pk);
8537+
nodes[1].node.peer_connected(&random_pk, &msgs::Init {
8538+
features: nodes[0].node.init_features(), remote_network_address: None }, true).unwrap();
8539+
}
8540+
let last_random_pk = PublicKey::from_secret_key(&nodes[0].node.secp_ctx,
8541+
&SecretKey::from_slice(&nodes[1].keys_manager.get_secure_random_bytes()).unwrap());
8542+
nodes[1].node.peer_connected(&last_random_pk, &msgs::Init {
8543+
features: nodes[0].node.init_features(), remote_network_address: None }, true).unwrap_err();
8544+
8545+
// Also importantly, because nodes[0] isn't "protected", we will refuse a reconnection from
8546+
// them if we have too many un-channel'd peers.
8547+
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
8548+
let chan_closed_events = nodes[1].node.get_and_clear_pending_events();
8549+
assert_eq!(chan_closed_events.len(), super::MAX_UNFUNDED_CHANS_PER_PEER - 1);
8550+
for ev in chan_closed_events {
8551+
if let Event::ChannelClosed { .. } = ev { } else { panic!(); }
8552+
}
8553+
nodes[1].node.peer_connected(&last_random_pk, &msgs::Init {
8554+
features: nodes[0].node.init_features(), remote_network_address: None }, true).unwrap();
8555+
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init {
8556+
features: nodes[0].node.init_features(), remote_network_address: None }, true).unwrap_err();
8557+
8558+
// Now nodes[0] is disconnected but still has a pending, un-funded channel lying around.
8559+
// Even though we accept one more connection from new peers, we won't actually let them
8560+
// open channels.
8561+
assert_eq!(peer_pks.len(), super::MAX_NO_CHANNEL_PEERS - 1);
8562+
for peer_pk in peer_pks {
8563+
nodes[1].node.handle_open_channel(&peer_pk, &open_channel_msg);
8564+
get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, peer_pk);
8565+
open_channel_msg.temporary_channel_id = nodes[0].keys_manager.get_secure_random_bytes();
8566+
}
8567+
nodes[1].node.handle_open_channel(&last_random_pk, &open_channel_msg);
8568+
assert_eq!(get_err_msg!(nodes[1], last_random_pk).channel_id,
8569+
open_channel_msg.temporary_channel_id);
8570+
8571+
// If we fund the first channel, nodes[0] has a live on-chain channel with us, it is now
8572+
// "protected" and can connect again.
8573+
mine_transaction(&nodes[1], funding_tx.as_ref().unwrap());
8574+
nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init {
8575+
features: nodes[0].node.init_features(), remote_network_address: None }, true).unwrap();
8576+
get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
8577+
8578+
// Further, because the first channel was funded, we can open another channel with
8579+
// last_random_pk.
8580+
nodes[1].node.handle_open_channel(&last_random_pk, &open_channel_msg);
8581+
get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, last_random_pk);
8582+
}
8583+
84528584
#[cfg(anchors)]
84538585
#[test]
84548586
fn test_anchors_zero_fee_htlc_tx_fallback() {

0 commit comments

Comments
 (0)