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

feat: enable NoDial should still dial static nodes #2151

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

weiihann
Copy link
Contributor

@weiihann weiihann commented Jan 10, 2024

Description

Resolves #2149

Changes

Previously, when NoDial is set to true, then the maximum dial peers is always going to be 0. Hence, the node is never going to dial static nodes.

With this code change, when NoDial is set to true, the maximum dial peers will be set to the number of static peers. With this, the dial scheduler knows that there are available slots to dial the static peers.

One concern regarding this design is that, if the node doesn't dial the static nodes successfully, will it then dial the other peers, since there are available dial slots?

Fortunately, the answer is no. Based on the code here, it's guaranteed to return the number of static nodes. Therefore, there will be no available slots to dial other peers.

@MatusKysel
Copy link
Contributor

what about VerifyNodes and TrustedNodesare we ok we are not connecting to those?

MatusKysel
MatusKysel previously approved these changes Jan 10, 2024
@weiihann
Copy link
Contributor Author

what about VerifyNodes and TrustedNodesare we ok we are not connecting to those?

I think we can include VerifyNodes, since the node is going to connect with the VerifyNodes when started right away, exactly the same as StaticNodes. For TrustedNodes, I don't think it's necessary because the scenario here targets NoDial = true, so TrustedNodes won't be connected anyways.

@du5
Copy link

du5 commented Jan 10, 2024

I think your needs should be solved using --nodiscover=true instead of modifying NoDial

[Node.P2P]
NoDiscovery = true
--nodiscover                   (default: false)
      Disables the peer discovery mechanism (manual peer addition)

However, your changes can indeed meet your needs, but I think nodiscover is more suitable for your needs. It can manually manage, add and delete

@weiihann
Copy link
Contributor Author

I think your needs should be solved using --nodiscover=true instead of modifying NoDial

[Node.P2P]
NoDiscovery = true
--nodiscover                   (default: false)
      Disables the peer discovery mechanism (manual peer addition)

However, your changes can indeed meet your needs, but I think nodiscover is more suitable for your needs. It can manually manage, add and delete

Not exactly. NoDiscovery disables the UDP port, which means that other nodes cannot discover our node at all. With this PR, enabling NoDial and StaticNodes would still allow other peers to discover and connect to our node, but our node won't be the one initiating the connection request (i.e. dialing), except for the static nodes.

Copy link
Collaborator

@zzzckck zzzckck left a comment

Choose a reason for hiding this comment

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

LGTM, I think it is reasonable that user could only want to establish connection with StaticNodes.

@zzzckck zzzckck merged commit 1c3d31c into bnb-chain:develop Jan 16, 2024
6 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.

4 participants