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.

Sign up for GitHub

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

Jump to bottom

inclusive terms: change whitelist terms #23290

Closed
wants to merge 1 commit into from
Closed

inclusive terms: change whitelist terms #23290

wants to merge 1 commit into from

Conversation

baptiste-b-pegasys
Copy link
Contributor

@baptiste-b-pegasys baptiste-b-pegasys commented Jul 29, 2021 •
edited
Loading

split of #23283 containing only the changes for the whitelist terms.

Thanks to @holiman for the prior review.

@karalabe
Copy link
Member

This PR breaks API compatibility, so it's a no go.

karalabe
karalabe reviewed Jul 29, 2021
View reviewed changes
cmd/geth/main.go
@@ -101,7 +101,7 @@ var (
utils.UltraLightFractionFlag,
utils.UltraLightOnlyAnnounceFlag,
utils.LightNoSyncServeFlag,
utils.WhitelistFlag,
utils.AllowListFlag,
Copy link
Member

Choose a reason for hiding this comment

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

You disabled the whitelist flag.

@karalabe
Copy link
Member

Allow list is not a good enough change. The block whitelist enforces the specified hashes to be present at the remote side. It's not merely a suggestion, rather a requirement.

karalabe
karalabe reviewed Jul 29, 2021
View reviewed changes
p2p/dial.go
@@ -77,7 +77,7 @@ var (
errAlreadyDialing = errors.New("already dialing")
errAlreadyConnected = errors.New("already connected")
errRecentlyDialed = errors.New("recently dialed")
errNotWhitelisted = errors.New("not contained in netrestrict whitelist")
errNotAllowListed = errors.New("not contained in netrestrict allowList")
Copy link
Member

Choose a reason for hiding this comment

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

This sounds like a mouthfull.

Copy link
Contributor

Choose a reason for hiding this comment

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

The error name could be errNetRestrict

baptiste-b-pegasys
baptiste-b-pegasys commented Jul 29, 2021
View reviewed changes
cmd/geth/main.go
@@ -101,7 +101,7 @@ var (
utils.UltraLightFractionFlag,
utils.UltraLightOnlyAnnounceFlag,
utils.LightNoSyncServeFlag,
utils.WhitelistFlag,
utils.AllowListFlag,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
utils.AllowListFlag,
utils.WhitelistFlag,
utils.AllowListFlag,

should it work or should we use only Whitelist flag?

baptiste-b-pegasys
baptiste-b-pegasys commented Jul 29, 2021
View reviewed changes
p2p/dial.go
@@ -77,7 +77,7 @@ var (
errAlreadyDialing = errors.New("already dialing")
errAlreadyConnected = errors.New("already connected")
errRecentlyDialed = errors.New("recently dialed")
errNotWhitelisted = errors.New("not contained in netrestrict whitelist")
errNotAllowListed = errors.New("not contained in netrestrict allowList")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
errNotAllowListed = errors.New("not contained in netrestrict allowList")
errNotAllowListed = errors.New("not contained in the netrestrict allowed list")

Copy link
Contributor

Choose a reason for hiding this comment

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

"not contained in netrestrict list"

fjl
fjl requested changes Jul 29, 2021
View reviewed changes
Copy link
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

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

In general, I'm happy about the initiative to improve the terms, but unhappy about the PR changing unrelated components. I would much prefer if the changes were made individually.

node/rpcstack.go
// and then registers all of the APIs exposed by the services.
func RegisterApisFromWhitelist(apis []rpc.API, modules []string, srv *rpc.Server, exposeAll bool) error {
func RegisterApisFromAllowList(apis []rpc.API, modules []string, srv *rpc.Server, exposeAll bool) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

For this function, both terms are not appropriate. The 'list' here really just a list of API names that should be registered, so we could simply call it RegisterAPIs.

p2p/dial.go
@@ -77,7 +77,7 @@ var (
errAlreadyDialing = errors.New("already dialing")
errAlreadyConnected = errors.New("already connected")
errRecentlyDialed = errors.New("recently dialed")
errNotWhitelisted = errors.New("not contained in netrestrict whitelist")
errNotAllowListed = errors.New("not contained in netrestrict allowList")
Copy link
Contributor

Choose a reason for hiding this comment

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

"not contained in netrestrict list"

tests/init_test.go
@@ -93,7 +93,7 @@ type testMatcher struct {
failpat []testFailure
skiploadpat []*regexp.Regexp
slowpat []*regexp.Regexp
whitelistpat *regexp.Regexp
allowlistpat *regexp.Regexp
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, too, the term 'allowlist' doesn't capture the meaning in the same way that 'whitelist' did. Since we are not using this feature at all, we might as well just delete it completely.

eth/ethconfig/config.go
// Whitelist of required block number -> hash values to accept
Whitelist map[uint64]common.Hash `toml:"-"`
// AllowList of required block number -> hash values to accept
AllowList map[uint64]common.Hash `toml:"-"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This change breaks compatibility with config files, so we cannot apply it without providing some kind of translator / deprecation mechanism.

You can check how to do this in https://github.com/ethereum/go-ethereum/blob/master/cmd/geth/config.go#L238

Copy link
Contributor Author

@baptiste-b-pegasys baptiste-b-pegasys Jul 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

When I've done it, seeing the tag "-" I thought it was omitted, just here for reference not in the config file.
A team member gave me this reference https://github.com/BurntSushi/toml/blob/ebe1404fc680a89e43605f720106b16be2ba141c/encode.go#L561-L563

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I didn't notice that! That's OK then.

@baptiste-b-pegasys
Copy link
Contributor Author

thank you @karalabe and @fjl for the review, I will split this PR by feature, with the requested changes

This was referenced Jul 29, 2021
Merged
Merged
Merged
Merged
This was referenced Aug 10, 2021
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjl fjl fjl requested changes

@karalabe karalabe karalabe left review comments

@holiman holiman Awaiting requested review from holiman holiman is a code owner

@rjl493456442 rjl493456442 Awaiting requested review from rjl493456442 rjl493456442 is a code owner

@zsfelfoldi zsfelfoldi Awaiting requested review from zsfelfoldi zsfelfoldi is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@baptiste-b-pegasys @karalabe @fjl