p2p, refactor: make NetPermissionFlags an enum class #21506

jonatack · 2021-03-22T22:17:43Z

While reviewing #20196, I noticed the NetPermissionFlags enums are frequently called as if they were scoped, yet are still global. This patch upgrades NetPermissionFlags to a scoped class enum and updates the enumerator naming, similarly to #19771. See https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#enum-enumerations for more info.

This change would eliminate the class of bugs like #20196 (comment) and #21644, as only defined operations on the flags would compile.

DrahtBot · 2021-03-22T22:57:22Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

p2p: Remove -feefilter option #21992 (Feefilter cleanups by amadeuszpawlik)
refactor: wrap accept() and extend usage of Sock #21879 (Wrap accept() and extend usage of Sock by vasild)
Make all networking code mockable #21878 (Make all networking code mockable by vasild)
p2p, rpc: enable GetAddr, GetAddresses, and getnodeaddresses by network #21843 (p2p, rpc: enable GetAddr, GetAddresses, and getnodeaddresses by network by jonatack)
net: don't bind on 0.0.0.0 if binds are restricted to Tor #20234 (net: don't extra bind for Tor if binds are restricted by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

practicalswift · 2021-03-23T16:03:44Z

Strong Concept ACK

enum class is strictly better :)

src/net.cpp

src/test/netbase_tests.cpp

kiminuo · 2021-03-24T21:05:00Z

Concept ACK, nice change

src/test/netbase_tests.cpp

jonatack · 2021-03-25T15:22:44Z

Could you please explain what was the motivation to add HasAnyFlag?

Thanks for the review @kiminuo! I extracted the NetPermissions::HasAnyFlag() addition to a separate commit with a detailed explanation in the commit message and updated with your other feedback.

jonatack · 2021-03-29T10:24:45Z

rebased

jonatack · 2021-03-30T18:33:50Z

rebased

hebasto

Approach ACK fa73a18.

The first commit "p2p, refactor: add NetPermissionFlags scopes where not already present" could be a scripted-diff:

s() { git grep -l "$1" -- 'src' ':!src/net_permissions.h' | xargs sed -i -E "s/([^:])$1/\1NetPermissionFlags::$1/"; }

s 'PF_NONE'
s 'PF_BLOOMFILTER'
s 'PF_RELAY'
s 'PF_FORCERELAY'
s 'PF_DOWNLOAD'
s 'PF_NOBAN'
s 'PF_MEMPOOL'
s 'PF_ADDR'
s 'PF_ISIMPLICIT'
s 'PF_ALL'

src/net.cpp

jonatack · 2021-04-02T13:57:33Z

The first commit "p2p, refactor: add NetPermissionFlags scopes where not already present" could be a scripted-diff:
s() { git grep -l "$1" -- 'src' ':!src/net_permissions.h' | xargs sed -i -E "s/([^:])$1/\1NetPermissionFlags::$1/"; }

Thanks! That's a co-credit :) Done in 043a4c5.

…:Bind() 36fb036 p2p: allow NetPermissions::ClearFlag() only with PF_ISIMPLICIT (Jon Atack) 4e0d578 test: add net permissions noban/download unit test coverage (Jon Atack) dde69f2 p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind() (Jon Atack) Pull request description: This is a bugfix follow-up to #16248 and #19191 that was noticed in #21506. Both v0.21 and master are affected. Since #19191, noban is a multi-flag that implies download, so the conditional in `CConnman::Bind()` using a bitwise AND on noban will return the same result for both the noban status and the download status. This means that download peers are incorrectly not being added to local addresses because they are mistakenly seen as noban peers. The second commit adds unit test coverage to illustrate and test the noban/download relationship and the `NetPermissions` operations involving them. The final commit adds documentation and disallows calling `NetPermissions::ClearFlag()` with any second param other than `NetPermissionFlags` "implicit" -- per current usage in the codebase -- because `ClearFlag()` should not be called with any second param that is a subflag of a multiflag, e.g. "relay" or "download," as that would leave the result in an invalid state corresponding to none of the existing NetPermissionFlags. Thanks to Vasil Dimov for noticing this. ACKs for top commit: theStack: re-ACK 36fb036 ☕ vasild: ACK 36fb036 hebasto: ACK 36fb036, I have reviewed the code and it looks OK, I agree it can be merged. kallewoof: Code review ACK 36fb036 Tree-SHA512: 5fbc7ddbf31d06b35bf238f4d77ef311e6b6ef2e1bb9893f32f889c1a0f65774a3710dcb21d94317fe6166df9334a9f2d42630809e7fe8cbd797dd6f6fc49491

…onnman::Bind() 36fb036 p2p: allow NetPermissions::ClearFlag() only with PF_ISIMPLICIT (Jon Atack) 4e0d578 test: add net permissions noban/download unit test coverage (Jon Atack) dde69f2 p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind() (Jon Atack) Pull request description: This is a bugfix follow-up to bitcoin#16248 and bitcoin#19191 that was noticed in bitcoin#21506. Both v0.21 and master are affected. Since bitcoin#19191, noban is a multi-flag that implies download, so the conditional in `CConnman::Bind()` using a bitwise AND on noban will return the same result for both the noban status and the download status. This means that download peers are incorrectly not being added to local addresses because they are mistakenly seen as noban peers. The second commit adds unit test coverage to illustrate and test the noban/download relationship and the `NetPermissions` operations involving them. The final commit adds documentation and disallows calling `NetPermissions::ClearFlag()` with any second param other than `NetPermissionFlags` "implicit" -- per current usage in the codebase -- because `ClearFlag()` should not be called with any second param that is a subflag of a multiflag, e.g. "relay" or "download," as that would leave the result in an invalid state corresponding to none of the existing NetPermissionFlags. Thanks to Vasil Dimov for noticing this. ACKs for top commit: theStack: re-ACK 36fb036 ☕ vasild: ACK 36fb036 hebasto: ACK 36fb036, I have reviewed the code and it looks OK, I agree it can be merged. kallewoof: Code review ACK 36fb036 Tree-SHA512: 5fbc7ddbf31d06b35bf238f4d77ef311e6b6ef2e1bb9893f32f889c1a0f65774a3710dcb21d94317fe6166df9334a9f2d42630809e7fe8cbd797dd6f6fc49491

Rspigler · 2021-05-11T22:57:57Z

Mark up for grabs?

-BEGIN VERIFY SCRIPT- s() { git grep -l "$1" -- 'src' ':!src/net_permissions.h' | xargs sed -i -E "s/([^:])$1/\1NetPermissionFlags::$1/"; } s 'PF_NONE' s 'PF_BLOOMFILTER' s 'PF_RELAY' s 'PF_FORCERELAY' s 'PF_DOWNLOAD' s 'PF_NOBAN' s 'PF_MEMPOOL' s 'PF_ADDR' s 'PF_ISIMPLICIT' s 'PF_ALL' -END VERIFY SCRIPT- Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>

jonatack · 2021-05-12T09:12:11Z

Re-opening and rebased now that #21644, that this is based on, was merged.

vasild

ACK d53c70e

laanwj · 2021-05-12T13:05:35Z

src/net_permissions.h

 };
+static inline constexpr NetPermissionFlags operator|(NetPermissionFlags a, NetPermissionFlags b)


Nice! Why not do this for & as well?
(More generally, I wonder if there is a nice way in C++ to define a reusable metatype that is 'enum with bitfield properties'—it seems that this is more generally useful. Not necessarily in this PR though.)

I think the goal of this pull is to forbid the & operator, because it won't do what you think it does. (See the preceding bugfix)

That is, callers should use HasFlag instead of &.

src/net_permissions.h

maflcko

drop ALL_CAPS naming per https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-caps

TIL

maflcko · 2021-05-12T13:13:37Z

cr ACK d53c70e

and define/update operation methods to handle type conversions explicitly. See https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-oper for more info.

- drop redundant PF_ permission flags prefixes - drop ALL_CAPS naming per https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-caps - rename IsImplicit to Implicit -BEGIN VERIFY SCRIPT- s() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; } s 'PF_NONE' 'None' s 'PF_BLOOMFILTER' 'BloomFilter' s 'PF_RELAY' 'Relay' s 'PF_FORCERELAY' 'ForceRelay' s 'PF_DOWNLOAD' 'Download' s 'PF_NOBAN' 'NoBan' s 'PF_MEMPOOL' 'Mempool' s 'PF_ADDR' 'Addr' s 'PF_ISIMPLICIT' 'Implicit' s 'PF_ALL' 'All' -END VERIFY SCRIPT-

-BEGIN VERIFY SCRIPT- s() { git grep -l "$1" src/net_processing.cpp | xargs sed -i "s/$1/$2/g"; } s 'the noban permission' 'NetPermissionFlags::NoBan permission' s 'the NOBAN permission flag' 'NetPermissionFlags::NoBan permission' s 'noban permission' 'NetPermissionFlags::NoBan permission' -END VERIFY SCRIPT-

jonatack · 2021-05-12T14:16:45Z

Dropped unneeded parentheses, only change is git diff d53c70e 7075f60

diff --git a/src/net_permissions.h b/src/net_permissions.h
index 9cb59bd85f..7a158aa6c5 100644
--- a/src/net_permissions.h
+++ b/src/net_permissions.h
@@ -56,7 +56,7 @@ public:
     }
     static inline void AddFlag(NetPermissionFlags& flags, NetPermissionFlags f)
     {
-        flags = (flags | f);
+        flags = flags | f;
     }

vasild

ACK 7075f60

luke-jr

utACK

laanwj · 2021-05-19T09:51:10Z

Code review ACK 7075f60

…lass

jonatack mentioned this pull request Mar 22, 2021

net: fix GetListenPort() to derive the proper port #20196

Merged

fanquake added the P2P label Mar 22, 2021

This was referenced Mar 23, 2021

p2p: Refactor sock to add I2P fuzz and unit tests #21387

Merged

net/net processing: Move tx inventory into net_processing #21160

Merged

net: don't bind on 0.0.0.0 if binds are restricted to Tor #20234

Merged

maflcko added the Refactoring label Mar 23, 2021

This was referenced Mar 23, 2021

Allow whitelisting outgoing connections #17167

Closed

p2p: Don't send FEEFILTER in blocksonly mode. #21509

Merged

maflcko reviewed Mar 23, 2021

View reviewed changes

src/net.cpp Show resolved Hide resolved

jonatack force-pushed the NetPermissionFlags-enum-class branch from 4eb2ada to 02270a1 Compare March 24, 2021 12:50

kiminuo reviewed Mar 24, 2021

View reviewed changes

src/test/netbase_tests.cpp Outdated Show resolved Hide resolved

kiminuo reviewed Mar 24, 2021

View reviewed changes

src/test/netbase_tests.cpp Outdated Show resolved Hide resolved

jonatack force-pushed the NetPermissionFlags-enum-class branch from 02270a1 to a29876a Compare March 25, 2021 15:22

DrahtBot added the Needs rebase label Mar 29, 2021

jonatack force-pushed the NetPermissionFlags-enum-class branch from a29876a to 717108b Compare March 29, 2021 10:24

DrahtBot added Needs rebase and removed Needs rebase labels Mar 29, 2021

jonatack force-pushed the NetPermissionFlags-enum-class branch from 717108b to fa73a18 Compare March 30, 2021 18:33

DrahtBot removed the Needs rebase label Mar 30, 2021

hebasto reviewed Apr 1, 2021

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

jonatack force-pushed the NetPermissionFlags-enum-class branch from fa73a18 to fad51cb Compare April 2, 2021 13:56

DrahtBot mentioned this pull request Apr 3, 2021

Erlay: bandwidth-efficient transaction relay protocol #21515

Closed

jonatack and others added 2 commits May 12, 2021 10:50

p2p: NetPermissions::HasFlag() pass flags param by value

7b55a94

jonatack reopened this May 12, 2021

jonatack force-pushed the NetPermissionFlags-enum-class branch from 24b294a to d53c70e Compare May 12, 2021 09:26

vasild approved these changes May 12, 2021

View reviewed changes

laanwj reviewed May 12, 2021

View reviewed changes

src/net_permissions.h Outdated Show resolved Hide resolved

maflcko approved these changes May 12, 2021

View reviewed changes

maflcko added this to the 22.0 milestone May 12, 2021

jonatack added 3 commits May 12, 2021 16:13

p2p, refactor: make NetPermissionFlags a uint32 enum class

810d092

and define/update operation methods to handle type conversions explicitly. See https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-oper for more info.

jonatack force-pushed the NetPermissionFlags-enum-class branch from d53c70e to 7075f60 Compare May 12, 2021 14:14

vasild approved these changes May 12, 2021

View reviewed changes

luke-jr approved these changes May 13, 2021

View reviewed changes

DrahtBot mentioned this pull request May 19, 2021

p2p: Remove -feefilter option #21992

Merged

laanwj merged commit 4da26fb into bitcoin:master May 19, 2021

jonatack deleted the NetPermissionFlags-enum-class branch May 19, 2021 10:04

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 19, 2021

Merge bitcoin#21506: p2p, refactor: make NetPermissionFlags an enum c…

41ce7f7

…lass

kiminuo mentioned this pull request Aug 24, 2021

refactor: Clarify and disable unused ArgsManager flags #22766

Merged

ryanofsky mentioned this pull request Sep 15, 2021

RFC: Replacing ArgsManager enum with enum class #22977

Closed

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 4da26fb into merged_master (Bitcoin PR bitcoin/bitcoin#21506)

3074da4

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

		};
		static inline constexpr NetPermissionFlags operator\|(NetPermissionFlags a, NetPermissionFlags b)

p2p, refactor: make NetPermissionFlags an enum class #21506

p2p, refactor: make NetPermissionFlags an enum class #21506

Uh oh!

Conversation

jonatack commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift commented Mar 23, 2021

Uh oh!

Uh oh!

Uh oh!

kiminuo commented Mar 24, 2021

Uh oh!

Uh oh!

jonatack commented Mar 25, 2021

Uh oh!

jonatack commented Mar 29, 2021

Uh oh!

jonatack commented Mar 30, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Apr 2, 2021

Uh oh!

Rspigler commented May 11, 2021

Uh oh!

jonatack commented May 12, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko May 12, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko May 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented May 12, 2021

Uh oh!

jonatack commented May 12, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented May 19, 2021

Uh oh!

Uh oh!

jonatack commented Mar 22, 2021 •

edited

Loading

DrahtBot commented Mar 22, 2021 •

edited

Loading

laanwj May 12, 2021 •

edited

Loading