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

5912 only static peers #7447

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ssonthal
Copy link
Contributor

Fixes #5912

Changes

  • Adding DiscoveryEnabled field in INetworkConfig and NetworkConfig classes.
  • Setting DiscoveryEnabled filed as !OnlyStaticPeers to ensure it gets set correctly at the beginning.
  • Adding an if condition based on DiscoveryEnabled value in initializeAllPeers method.

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

@ssonthal
Copy link
Contributor Author

@smartprogrammer93 please let me know if these changes look good.

@smartprogrammer93
Copy link
Contributor

@shubham-sonthalia nope, that is not what is needed here. There is already a DiscoveryEnabled config in InitConfigs, so you need to migrate that to Network. Then, set DiscoveryEnabled=false when OnlyStaticPeers=true. Also, you need to reject any incoming connections from peers that are not in the static list when OnlyStaticPeers=true.

@ssonthal
Copy link
Contributor Author

ssonthal commented Sep 17, 2024

Hey @smartprogrammer93, I copied the DiscoveryEnabled field from InitConfig and set it to false if OnlyStaticPeer = true.

For rejecting any incoming request from peer, I couldn't pinpoint the exact place. But through some analysis, I have added it in the AddNodes() method in the NodeTable class. Let me know if this looks good.

EDIT: I have moved the rejecting of incoming request part to PoolManager class -> ProcessIncomingRequest method.

@ssonthal
Copy link
Contributor Author

Hey @smartprogrammer93 did you get a chance to go over the PR? Have I missed anything here?

@smartprogrammer93
Copy link
Contributor

@shubham-sonthalia yeah there is a couple. Changing the value of discovery based on static peers this way is incorrect. Also, migrating a sertting needs to not break existing setups. You need to deal with the old variable and migrate it to the correct place. And change all the usages.

@ssonthal
Copy link
Contributor Author

@smartprogrammer93 so should I remove the field DiscoveryEnabled from IInitConfig and then wherever this field was used - instead of IInitConfig, it should come from INetworkConfig?

@ssonthal
Copy link
Contributor Author

ssonthal commented Oct 5, 2024

@smartprogrammer93 can you please comment on the above query?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants