bugfix : Removed the default value of the bootnode flag to prevent it from being overridden during testnet usage #14357
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
|
|
gts2030
changed the title
james-prysm
reviewed
Aug 20, 2024
cmd/flags.go
Outdated
| @@ -101,7 +100,6 @@ var ( | |||
| BootstrapNode = &cli.StringSliceFlag{ | |||
| Name: "bootstrap-node", | |||
| Usage: "The address of bootstrap node. Beacon node will connect for peer discovery via DHT. Multiple nodes can be passed by using the flag multiple times but not comma-separated. You can also pass YAML files containing multiple nodes.", | |||
| Value: cli.NewStringSlice(params.BeaconNetworkConfig().BootstrapNodes...), | |||
There was a problem hiding this comment.
We should instead just change to if cliCtx.IsSet(cmd.BootstrapNode.Name) { instead of len(cliCtx.StringSlice(cmd.BootstrapNode.Name)) > 0 now that the library is fixed
There was a problem hiding this comment.
Oh that's a good news. Than I will fix it to use IsSet() instead of removing default value from flag.
|
thanks for the submission left a comment |
|
Bugfix to revert changes from #11166 because library is fixed now. |
gts2030
force-pushed
the
fix-bootnode-flag
branch
from
d2e3203 to
7bcd780
Compare
james-prysm
approved these changes
Aug 22, 2024
…ng overridden during testnet usage
auto-merge was automatically disabled
August 22, 2024 16:00
Head branch was pushed to by a user without write access
gts2030
force-pushed
the
fix-bootnode-flag
branch
from
2615be5 to
82cd079
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Merged
via the queue into
prysmaticlabs:develop
with commit Aug 23, 2024
50e5326
16 of 17 checks passed
james-prysm
pushed a commit
that referenced
this pull request
Sep 6, 2024
… from being overridden during testnet usage (#14357) * Removed the default value of the bootnode flag to prevent it from being overridden during testnet usage * bugfix for checking stringslice flag to use isSet
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.
What type of PR is this?
What does this PR do? Why is it needed?
By #11166, this fix made bug to always load mainnet bootnodes when just using testnet flags like --Holesky.
In
configureBeacon()at beacon-chain/node/node.go it first initialize testnet network config byConfigureBeaconChain()-> configureTestnet().Than it always overrides beaconchain network config bootnodes value as a mainnet bootnodes(which is default value for
bootstrap-nodeflag) inconfigureNetwork()because len(cmd.BootstrapNode.Name) is always >1.So this default value for
bootstrap-nodeflag should be removed to run testnet beacon node with just testnet flag.And also removing this default value doesn't disturb using mainnet flag usage.