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.

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

Bootstrapper Mode #268

Merged
merged 5 commits into from
Nov 28, 2024
Merged

Bootstrapper Mode #268

merged 5 commits into from
Nov 28, 2024

Conversation

aszepieniec
Copy link
Contributor

We are observing that people would like to participate in the network but can't because the two nodes listed in the README.md refuse connections. They do this because their max number of peers has already been reached.

This PR adds functionality for running a node in "bootstrapper mode", wherein it will drop the longest-lived peer connection whenever the maximum is reached. This mode is activated through a CLI argument. Whenever an incoming connection is established, if this mode is set, and if the max number of peers has been reached, the peer loop will send a DisconnectFromLongestLivedPeer message to the main loop. The main loop iterates over all peers, filters out the peers given as CLI arguments, selects the one with the earliest connection established date, and instructs that peer's peer loop to close the connection.

Whenever we establish a connection with a new peer, the first thing to
do after the handshake is now to ask the peer for its list of peers.
This change is motivated by topological robustness in the context of
adversaries who deploy sybil or other attacks. In such a context, the
connection is not guaranteed to last and so we want to prioritize the
information that affects network topology -- blockchain data may be
gathered from other nodes.
This commit
 - adds a CLI argument `bootstrap` that instructs the client to run in
   bootstrapper mode, which:
 - kills the connection with the longest-lived peer when the max num
   peers is reached, such that:
 - a new incoming connection can always be established.

The number of peers is tested against the maximum prior to entering
the peer loop. If the maximum is about to be reached, a message is
sent to the main loop, instructing it to select a peer and disconnect
from it.
@aszepieniec aszepieniec requested a review from dan-da November 27, 2024 15:42
Copy link
Member

@Sword-Smith Sword-Smith left a comment

Choose a reason for hiding this comment

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

I think you're missing a check on the bootstrap value in a branch. I think you're not using the bootstrap field at all. Otherwise, the logic looks excellent.

@@ -134,10 +134,20 @@ async fn check_if_connection_is_allowed(
return ConnectionStatus::Refused(ConnectionRefusedReason::IncompatibleVersion);
}

// If this connection touches the maximum number of peer connections, say
// so with special OK code.
if cli_arguments.max_num_peers as usize == global_state.net.peer_map.len() + 1 {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't you also check that the node is running as a bootstrapper node here? It seems to me that all nodes will display this "disconnect" behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • All nodes (bootstrapper or not) run through the highlighted logic.
  • The P2P message ConnectionStatus is only ever Refused(..) or Accepted; the value AcceptedMaxReached is filtered out on line 260.
  • Reading the CLI flag should happen on line 279. Will add that momentarily.

// get all peers
let all_peers = global_state.net.peer_map.iter();

// filter out CLI peers
Copy link
Member

Choose a reason for hiding this comment

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

Excellent that you're allowing CLI arguments to preserve peers that will never be disconnected.

@Sword-Smith Sword-Smith self-requested a review November 27, 2024 17:11
@aszepieniec aszepieniec merged commit 69c4f46 into master Nov 28, 2024
5 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants