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: remove default values for p2p-tcp-address and p2p-udp-address #1248

Merged
merged 6 commits into from
Oct 12, 2022

Conversation

xenowits
Copy link
Contributor

@xenowits xenowits commented Oct 10, 2022

  • Removes default values for p2p-tcp-address and p2p-udp-address.
  • Errors if --p2p-udp-address=="" and --p2p-bootnode-relay=false and --p2p-bootnodes-from-lockfile==false.
  • Sets--p2p-bootnode-relay default value to true.
  • Output all information pertaining to a decoded ENR for charon enr --verbose.

category: feature
ticket: #998

@codecov
Copy link

codecov bot commented Oct 10, 2022

Codecov Report

Base: 53.58% // Head: 53.66% // Increases project coverage by +0.08% 🎉

Coverage data is based on head (66a3ed3) compared to base (a64b4f6).
Patch coverage: 77.33% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1248      +/-   ##
==========================================
+ Coverage   53.58%   53.66%   +0.08%     
==========================================
  Files         139      139              
  Lines       16298    16365      +67     
==========================================
+ Hits         8733     8782      +49     
- Misses       6303     6324      +21     
+ Partials     1262     1259       -3     
Impacted Files Coverage Δ
cmd/bootnode.go 31.08% <0.00%> (ø)
cmd/dkg.go 7.69% <0.00%> (ø)
p2p/discovery.go 31.72% <0.00%> (-0.88%) ⬇️
cmd/enr.go 41.30% <78.94%> (+27.97%) ⬆️
app/app.go 59.66% <100.00%> (ø)
cmd/createenr.go 71.87% <100.00%> (ø)
cmd/run.go 86.79% <100.00%> (+1.37%) ⬆️
dkg/dkg.go 49.30% <100.00%> (ø)
p2p/p2p.go 27.08% <100.00%> (-3.18%) ⬇️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

flags.StringVar(&config.Denylist, "p2p-denylist", "", "Comma-separated list of CIDR subnets for disallowing certain peer connections. Example: 192.168.0.0/16 would disallow connections to peers on your local network. The default is to accept all connections.")
func bindP2PFlags(cmd *cobra.Command, config *p2p.Config) {
cmd.Flags().StringSliceVar(&config.UDPBootnodes, "p2p-bootnodes", []string{"http://bootnode.lb.gcp.obol.tech:3640/enr"}, "Comma-separated list of discv5 bootnode URLs or ENRs.")
cmd.Flags().BoolVar(&config.BootnodeRelay, "p2p-bootnode-relay", true, "Enables using bootnodes as libp2p circuit relays. Useful if some charon nodes are not publicly accessible.")
Copy link
Contributor Author

@xenowits xenowits Oct 10, 2022

Choose a reason for hiding this comment

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

Since, now the defaults are:
--p2p-udp-address==""
--p2p-bootnode-relay=false
--p2p-bootnodes-from-lockfile==false

we need to enable anyone of the above by default. Else, bindP2PFlags will always return an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question: Do we want to enable anyone of the above flags?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, error by default is probably not good UX. And most people run with bootnode relay, so making this true is probably fine.

app/app.go Outdated Show resolved Hide resolved
p2p/p2p.go Outdated Show resolved Hide resolved
p2p/discovery.go Outdated Show resolved Hide resolved
p2p/discovery.go Outdated Show resolved Hide resolved
p2p/discovery.go Outdated Show resolved Hide resolved
@xenowits xenowits force-pushed the xenowits/remove-p2p-address-defaults branch from 0d61b8f to 6fe7ea4 Compare October 11, 2022 16:36
cmd/run.go Outdated Show resolved Hide resolved
app/app.go Outdated Show resolved Hide resolved
cmd/bootnode.go Show resolved Hide resolved
cmd/enr.go Show resolved Hide resolved
cmd.Flags().StringVar(&config.Denylist, "p2p-denylist", "", "Comma-separated list of CIDR subnets for disallowing certain peer connections. Example: 192.168.0.0/16 would disallow connections to peers on your local network. The default is to accept all connections.")

preRunE := cmd.PreRunE // Allow multiple wraps of PreRunE.
cmd.PreRunE = func(cmd *cobra.Command, args []string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is time to extract this wrapping preRunE as a function, since we do it in a few places. func wrapPreRunE(cmd, func(cmd *cobra.Command, args []string) error)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I'll raise a separate PR to extract this.

cmd/run.go Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
p2p/p2p.go Show resolved Hide resolved
@xenowits xenowits self-assigned this Oct 12, 2022
@xenowits xenowits added the merge when ready Indicates bulldozer bot may merge when all checks pass label Oct 12, 2022
@obol-bulldozer obol-bulldozer bot merged commit d0442c9 into main Oct 12, 2022
@obol-bulldozer obol-bulldozer bot deleted the xenowits/remove-p2p-address-defaults branch October 12, 2022 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants