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

cmd/utils: fix parsing BootstrapNodes from BootnodesFlag #28095

Merged
merged 3 commits into from Sep 25, 2023

Conversation

buddh0
Copy link
Contributor

@buddh0 buddh0 commented Sep 12, 2023

bug description

conditions

  1. BootstrapNodes is set in config.toml, no matter empty or not, like
[Node.P2P]
MaxPeers = 30
NoDiscovery = false
BootstrapNodes = []
StaticNodes = []
ListenAddr = ":30311"
EnableMsgEvents = false
  1. start geth with flag BootnodesFlag

result

bootnode in BootnodesFlag is ignored

expected hehavior

reset the bootnode from BootnodesFlag

if cfg.BootstrapNodes != nil {
return
case cfg.BootstrapNodesV5 != nil:
return // already set, don't apply defaults.
Copy link
Member

Choose a reason for hiding this comment

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

The issue was introduced in this PR #25174, I think this PR is wrong is the first place.

But your fix is also not correct, it will reset the bootstrap nodes in the config file. I will suggest this approach.

// resolveBootstrapNodes resolves the URLs for configuring the bootstrap
// nodes. If the bootstrap nodes are already configured in the configuration
// file, they will only be overridden if the command line flag is specified.
func resolveBootstrapNodes(ctx *cli.Context, cfg *p2p.Config) []string {
	if cfg.BootstrapNodes != nil {
		return SplitAndTrim(ctx.String(BootnodesFlag.Name))
	}
	urls := params.MainnetBootnodes
	switch {
	case ctx.IsSet(BootnodesFlag.Name):
		urls = SplitAndTrim(ctx.String(BootnodesFlag.Name))
	case ctx.Bool(HoleskyFlag.Name):
		urls = params.HoleskyBootnodes
	case ctx.Bool(SepoliaFlag.Name):
		urls = params.SepoliaBootnodes
	case ctx.Bool(GoerliFlag.Name):
		urls = params.GoerliBootnodes
	}
	return urls
}

// setBootstrapNodes creates a list of bootstrap nodes from the command line
// flags, reverting to pre-configured ones if none have been specified.
func setBootstrapNodes(ctx *cli.Context, cfg *p2p.Config) {
	urls := resolveBootstrapNodes(ctx, cfg)
	if len(urls) == 0 {
		return
	}
	cfg.BootstrapNodes = make([]*enode.Node, 0, len(urls))
	for _, url := range urls {
		if url != "" {
			node, err := enode.Parse(enode.ValidSchemes, url)
			if err != nil {
				log.Crit("Bootstrap URL invalid", "enode", url, "err", err)
				continue
			}
			cfg.BootstrapNodes = append(cfg.BootstrapNodes, node)
		}
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, I'm agree with your approach.

@buddh0 buddh0 closed this Sep 13, 2023
@buddh0 buddh0 reopened this Sep 19, 2023
@fjl fjl self-requested a review September 25, 2023 13:56
@fjl fjl merged commit f6f64cc into ethereum:master Sep 25, 2023
2 checks passed
@fjl fjl added this to the 1.13.2 milestone Sep 25, 2023
tyler-smith pushed a commit to blocknative/go-ethereum that referenced this pull request Oct 9, 2023
This fixes an issue where the --bootnodes flag was overridden by the config file.

---------

Co-authored-by: NathanBSC <Nathan.l@nodereal.io>
Co-authored-by: Felix Lange <fjl@twurst.com>
tyler-smith pushed a commit to blocknative/go-ethereum that referenced this pull request Oct 12, 2023
This fixes an issue where the --bootnodes flag was overridden by the config file.

---------

Co-authored-by: NathanBSC <Nathan.l@nodereal.io>
Co-authored-by: Felix Lange <fjl@twurst.com>
tyler-smith pushed a commit to blocknative/go-ethereum that referenced this pull request Oct 12, 2023
This fixes an issue where the --bootnodes flag was overridden by the config file.

---------

Co-authored-by: NathanBSC <Nathan.l@nodereal.io>
Co-authored-by: Felix Lange <fjl@twurst.com>
siosw pushed a commit to SpecularL2/go-ethereum that referenced this pull request Oct 16, 2023
This fixes an issue where the --bootnodes flag was overridden by the config file.

---------

Co-authored-by: NathanBSC <Nathan.l@nodereal.io>
Co-authored-by: Felix Lange <fjl@twurst.com>
0x366 pushed a commit to Dexcelerate/go-ethereum that referenced this pull request Nov 8, 2023
This fixes an issue where the --bootnodes flag was overridden by the config file.

---------

Co-authored-by: NathanBSC <Nathan.l@nodereal.io>
Co-authored-by: Felix Lange <fjl@twurst.com>
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
This fixes an issue where the --bootnodes flag was overridden by the config file.

---------

Co-authored-by: NathanBSC <Nathan.l@nodereal.io>
Co-authored-by: Felix Lange <fjl@twurst.com>
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
@buddh0 buddh0 deleted the bugfix_parse_BootnodesFlag branch March 4, 2024 02:24
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.

None yet

4 participants