Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable incoming light-client connections for minimal relay node #2202

Merged
merged 2 commits into from
Nov 7, 2023
Merged

Disable incoming light-client connections for minimal relay node #2202

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c3325b5
Disable incoming light-client connections
skunert Nov 6, 2023
2f09667
Merge branch 'master' into skunert/network-traffic-fix
skunert Nov 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions cumulus/client/relay-chain-minimal-node/src/network.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ use sp_runtime::traits::{Block as BlockT, NumberFor};

use sc_network::{
config::{
NonDefaultSetConfig, NonReservedPeerMode, NotificationHandshake, ProtocolId, SetConfig,
NetworkConfiguration, NonDefaultSetConfig, NonReservedPeerMode, NotificationHandshake,
ProtocolId, SetConfig,
},
peer_store::PeerStore,
NetworkService,
Expand All @@ -35,7 +36,7 @@ use std::{iter, sync::Arc};
/// Build the network service, the network status sinks and an RPC sender.
pub(crate) fn build_collator_network(
config: &Configuration,
network_config: FullNetworkConfiguration,
mut full_network_config: FullNetworkConfiguration,
spawn_handle: SpawnTaskHandle,
genesis_hash: Hash,
best_header: Header,
Expand All @@ -53,8 +54,12 @@ pub(crate) fn build_collator_network(
genesis_hash,
);

// Since this node has no syncing, we do not want light-clients to connect to it.
// Here we set any potential light-client slots to 0.
adjust_network_config_light_in_peers(&mut full_network_config.network_config);

let peer_store = PeerStore::new(
network_config
full_network_config
.network_config
.boot_nodes
.iter()
Expand All @@ -75,7 +80,7 @@ pub(crate) fn build_collator_network(
})
},
fork_id: None,
network_config,
network_config: full_network_config,
peer_store: peer_store_handle,
genesis_hash,
protocol_id,
Expand Down Expand Up @@ -114,6 +119,18 @@ pub(crate) fn build_collator_network(
Ok((network_service, network_starter, Box::new(SyncOracle {})))
}

fn adjust_network_config_light_in_peers(config: &mut NetworkConfiguration) {
let light_client_in_peers = (config.default_peers_set.in_peers +
config.default_peers_set.out_peers)
.saturating_sub(config.default_peers_set_num_full);
Comment on lines +123 to +125
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though it looks ugly, it seems there is no other way to estimate in_peers_light using current implementation of NetworkConfiguration...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this should probably become a proper field in the config some day.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is something I don't understand, maybe due to some limited domain knowledge in the networking subsystem. How reducing the in_peers number just trims the light clients connections?
I mean, aren't the full clients connections affected as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit tricky, when users specify --in-peers-light we actually add them to the normal in_peers in config construction:

polkadot-sdk/substrate/client/cli/src/params/network_params.rs

Lines 235 to 245 in b44de38

default_peers_set: SetConfig {
in_peers: self.in_peers + self.in_peers_light,
out_peers: self.out_peers,
reserved_nodes: self.reserved_nodes.clone(),
non_reserved_mode: if self.reserved_only {
NonReservedPeerMode::Deny
} else {
NonReservedPeerMode::Accept
},
},
default_peers_set_num_full: self.in_peers + self.out_peers,
.

And here is the logic that checks for the light client slots actually occupied:

polkadot-sdk/substrate/client/network/sync/src/engine.rs

Lines 1035 to 1041 in b44de38

if status.roles.is_light() &&
(self.peers.len() - self.chain_sync.num_peers()) >= self.default_peers_set_num_light
{
// Make sure that not all slots are occupied by light clients.
log::debug!(target: LOG_TARGET, "Too many light nodes, rejecting {peer_id}");
return Err(())
}

The whole concept of light client slots is kind of implicit currently. But in the end you can still calculate the light client slots with by calculating the difference as in the code.

Copy link
Contributor

@dmitry-markin dmitry-markin Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to comment, but @skunert beat me. Could just add that it's the limitation of light peer slots bookkeeping, we calculate the light peer slot count in SyncingEngine from in_peers, out_peers, full_peers:

polkadot-sdk/substrate/client/network/sync/src/engine.rs

Lines 422 to 426 in 4caa3d8

let default_peers_set_num_light = {
let total = net_config.network_config.default_peers_set.out_peers +
net_config.network_config.default_peers_set.in_peers;
total.saturating_sub(net_config.network_config.default_peers_set_num_full) as usize
};

So it's enough to update config.default_peers_set.in_peers for light peer slots to be correctly calculated later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clear Ty

if light_client_in_peers > 0 {
tracing::debug!(target: crate::LOG_TARGET, "Detected {light_client_in_peers} peer slots for light clients. Since this minimal node does support\
neither syncing nor light-client request/response, we are setting them to 0.");
}
config.default_peers_set.in_peers =
config.default_peers_set.in_peers.saturating_sub(light_client_in_peers);
}

struct SyncOracle;

impl sp_consensus::SyncOracle for SyncOracle {
Expand Down
Loading