Connect to network discv5 boot enrs on start up #3672
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dadepo
requested review from
ColinSchwarz,
dapplion,
g11tech,
mpetrunic,
RyRy79261,
twoeths and
wemeetagain
as code owners
Codecov Report
@@ Coverage Diff @@
## master #3672 +/- ##
==========================================
+ Coverage 37.16% 37.52% +0.36%
==========================================
Files 321 322 +1
Lines 8689 8818 +129
Branches 1346 1379 +33
==========================================
+ Hits 3229 3309 +80
- Misses 5317 5368 +51
+ Partials 143 141 -2 |
|
Code Climate has analyzed commit a8efda7 and detected 0 issues on this pull request. View more on Code Climate. |
Performance Report✔️ no performance regression detected Full benchmark results
|
wemeetagain
requested changes
Feb 1, 2022
docs/usage/local.md
Outdated
|
|
||
| ```bash | ||
| ./lodestar dev --genesisValidators 8 --reset | ||
| ./lodestar dev --genesisValidators 8 --genesisTime 1578787200 --network prater --rootDir </path/to/node1> --reset |
There was a problem hiding this comment.
maybe remove --network prater here and below
I think this is a little confusing as an example in the docs, folks may think they are actually connecting to the prater testnet
There was a problem hiding this comment.
may be network option can be removed from the dev cli options, so that it will throw error if someone even tries to specify
There was a problem hiding this comment.
may be
networkoption can be removed from the dev cli options, so that it will throw error if someone even tries to specify
Good tip. I'll see to that
There was a problem hiding this comment.
@g11tech so I looked into your suggestion. The network option is a global option and hence the changes that will be required to make it not applicable when the dev cli option is used feels a bit out of scope to this PR. So I created a separate issue: #3695 for the suggestion to be implemented separately.
| @@ -127,6 +127,8 @@ export class PeerDiscovery { | |||
| await this.discv5.start(); | |||
| this.discv5StartMs = Date.now(); | |||
| this.discv5.on("discovered", this.onDiscovered); | |||
| // on start, dial the discv5.bootEnrs that has been added to the routing table | |||
There was a problem hiding this comment.
I think we have two choices here:
- only do this if network.connectToDiscv5Bootnodes is true
- remove the network.connectToDiscv5Bootnodes option entirely
There was a problem hiding this comment.
We are not supposed to connect to the Mainnet bootnodes, this has to be behind a cli flag and used only for local devnets
There was a problem hiding this comment.
I have added it behind the network.connectToDiscv5Bootnodes cli flag
dapplion
reviewed
Feb 4, 2022
g11tech
reviewed
Feb 4, 2022
| --port 9001 \ | ||
| --api.rest.port 9597 \ | ||
| --network.discv5.bootEnrs <enr value> | ||
| --reset |
There was a problem hiding this comment.
may be this will now need connectToDiscv5Bootnodes arg as well?
g11tech
previously approved these changes
Feb 4, 2022
wemeetagain
reviewed
Feb 4, 2022
wemeetagain
approved these changes
Feb 4, 2022
g11tech
approved these changes
Feb 4, 2022
dadepo
deleted the
dadepo/connect-to-network-discv5-bootEnrs-on-start-up
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
While using the dev command it was realised that the documentation on how to start two nodes that connects to each other is outdated. This changes updates the documentation and also fixes the code to ensures nodes are connected as peers based on supplied discv5.boot.enrs values
Description
Updated the documentation for using the dev command.
Updated the code to ensure nodes are connected as peers based on supplied discv5.boot.enrs values.
Closes #3568