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 (review) and #21644, as only defined operations on the flags would compile.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
Strong Concept ACK
967@@ -968,7 +968,7 @@ bool CConnman::AttemptToEvictConnection()
968
969 LOCK(cs_vNodes);
970 for (const CNode* node : vNodes) {
971- if (node->HasPermission(NetPermissionFlags::PF_NOBAN))
972+ if (node->HasPermission(NetPermissionFlags::NoBan))
392+ BOOST_CHECK(NetPermissions::HasFlag(whitebindPermissions.m_flags, NetPermissionFlags::Implicit));
393+ BOOST_CHECK(NetPermissions::HasAnyFlag(whitebindPermissions.m_flags, NetPermissionFlags::Implicit));
394+
395+ NetPermissions::ClearFlag(whitebindPermissions.m_flags, NetPermissionFlags::Implicit);
396+ BOOST_CHECK(!NetPermissions::HasFlag(whitebindPermissions.m_flags, NetPermissionFlags::Implicit));
397+ BOOST_CHECK(!NetPermissions::HasAnyFlag(whitebindPermissions.m_flags, NetPermissionFlags::Implicit));;
0 BOOST_CHECK(!NetPermissions::HasAnyFlag(whitebindPermissions.m_flags, NetPermissionFlags::Implicit));
?
438+ BOOST_CHECK(NetPermissions::HasFlag(whitelistPermissions.m_flags, NetPermissionFlags::NoBan));
439+ BOOST_CHECK(NetPermissions::HasAnyFlag(whitelistPermissions.m_flags, NetPermissionFlags::NoBan));
440+
441+ NetPermissions::ClearFlag(whitelistPermissions.m_flags, NetPermissionFlags::Download);
442+ BOOST_CHECK(!NetPermissions::HasFlag(whitelistPermissions.m_flags, NetPermissionFlags::Download));
443+ BOOST_CHECK(!NetPermissions::HasFlag(whitelistPermissions.m_flags, NetPermissionFlags::NoBan));
HasFlag and HasAnyFlag differ.
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.
Approach ACK fa73a18ad874cc076a9201678ff25686766c24d0.
The first commit “p2p, refactor: add NetPermissionFlags scopes where not already present” could be a scripted-diff:
0s() { git grep -l "$1" -- 'src' ':!src/net_permissions.h' | xargs sed -i -E "s/([^:])$1/\1NetPermissionFlags::$1/"; }
1
2s 'PF_NONE'
3s 'PF_BLOOMFILTER'
4s 'PF_RELAY'
5s 'PF_FORCERELAY'
6s 'PF_DOWNLOAD'
7s 'PF_NOBAN'
8s 'PF_MEMPOOL'
9s 'PF_ADDR'
10s 'PF_ISIMPLICIT'
11s 'PF_ALL'
2413@@ -2410,7 +2414,7 @@ bool CConnman::Bind(const CService &addr, unsigned int flags, NetPermissionFlags
2414 return false;
2415 }
2416
2417- if (addr.IsRoutable() && fDiscover && !(flags & BF_DONT_ADVERTISE) && !(permissions & PF_NOBAN)) {
2418+ if (addr.IsRoutable() && fDiscover && !(flags & BF_DONT_ADVERTISE) && !NetPermissions::HasAnyFlag(permissions, NetPermissionFlags::NoBan)) {
NetPermissions::HasFlag ?
Hm, the tests pass with HasFlag() but ISTM that would represent a potential change in behavior in a refactoring pull, as HasFlag() returns (flags & f) == f instead of flags & f and NetPermissionFlags::NoBan is a multi-flag (NoBan = (1U << 4) | Download). LMK if I’m confused here though.
Aside, this line was last changed in commit bb145c90502:
0- if (addr.IsRoutable() && fDiscover && (permissions & PF_NOBAN) == 0) {
1+ if (addr.IsRoutable() && fDiscover && !(flags & BF_DONT_ADVERTISE) && !(permissions & PF_NOBAN)) {
You are right. Your change is equivalent to the current code. But this raises additional questions:
pass with
HasFlag()
HasAnyFlag(permissions, flag_a | flag_b) is much easier to read and reason about than HasAnyFlag(permissions, fancy_multiflag)
in general, with named multiflags, negation of HasFlag could have ambiguous meaning. Does this code do what it is supposed to do in all cases: https://github.com/bitcoin/bitcoin/blob/590e49ccf2af27c6c1f1e0eb8be3a4bf4d92ce8b/src/net.cpp#L1110-L1117 ?
After looking at #16248 and #19191, the current code seems correct.
This line was changed in e5b26deaaa6, which pre-dates the download permission being extracted from noban in #19191:
0- if (addrBind.IsRoutable() && fDiscover && !fWhitelisted)
1+ if (addrBind.IsRoutable() && fDiscover && (permissions & PF_NOBAN) == 0)
The intent of adding the Download flag in #19191 was: “It should be possible to grant nodes in a local network (e.g. home, university, enterprise, …) permission to download blocks even after the maxuploadtarget is hit. Currently this is only possible by setting the noban permission, which has some adverse effects, especially if the peers can’t be fully trusted. Fix this by extracting a download permission from noban.”
So, in reply to your questions:
(If I understand correctly) the existing lone bitwise AND here, e.g. HasAnyFlag(Noban) is correct (but the precise behavior is not covered by tests)
I guess we could replace HasAnyFlag(Noban) with the more verbose HasFlag(Noban) || HasFlag(Download) if that seems clearer
Elsewhere, HasFlag() appears to be what we want in the cases where it is used, AFAICT
Ugh, looks like this is a bug that I introduced in #19191
(The download permission set shouldn’t affect places where noban is checked for)
The first commit “p2p, refactor: add NetPermissionFlags scopes where not already present” could be a scripted-diff:
0s() { 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 043a4c5163909d0.
There is still the following “strange” behavior:
0// expected to have "noban" and "download" in flags, true
1flags = TryParse("noban,download");
2
3// expected to have just "noban" in flags, given that "download" has been removed, not true :(
4ClearFlag(flags, PF_DOWNLOAD);
5
6// what happens actually is that now flags is in a "bricked" state
7// where it does not equal any of the PF_* flags or a bitwise-OR of them.
Could it be avoided to have the download flag included in the noban flag (i.e. to have each PF_* flag have exactly one bit set) and #19191 resolved in some other way?
I don’t disagree, though it might be out of scope for this change (which has indeed exposed some issues!)
ClearFlag is currently only used in src/net.cpp::CConnman::CreateNodeFromAcceptedSocket() to clear the NetPermissionFlags implicit flag.
Edit: addressed for now in #21644.
47- PF_ISIMPLICIT = (1U << 31),
48- PF_ALL = PF_BLOOMFILTER | PF_FORCERELAY | PF_RELAY | PF_NOBAN | PF_MEMPOOL | PF_DOWNLOAD | PF_ADDR,
49+ Implicit = (1U << 31),
50+ All = BloomFilter | ForceRelay | Relay | NoBan | Mempool | Download | Addr,
51 };
52+static inline constexpr NetPermissionFlags operator|(NetPermissionFlags a, NetPermissionFlags b) {
{ should be on a new line
60 NetPermissionFlags m_flags;
61 static std::vector<std::string> ToStrings(NetPermissionFlags flags);
62- static inline bool HasFlag(const NetPermissionFlags& flags, NetPermissionFlags f)
63- {
64- return (flags & f) == f;
65+ static inline bool HasFlag(NetPermissionFlags flags, NetPermissionFlags f) {
{ should be on a new line
-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>47- PF_ISIMPLICIT = (1U << 31),
48- PF_ALL = PF_BLOOMFILTER | PF_FORCERELAY | PF_RELAY | PF_NOBAN | PF_MEMPOOL | PF_DOWNLOAD | PF_ADDR,
49+ Implicit = (1U << 31),
50+ All = BloomFilter | ForceRelay | Relay | NoBan | Mempool | Download | Addr,
51 };
52+static inline constexpr NetPermissionFlags operator|(NetPermissionFlags a, NetPermissionFlags b)
& 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.)
& operator, because it won’t do what you think it does. (See the preceding bugfix)
HasFlag instead of &.
68+ return (static_cast<t>(flags) & static_cast<t>(f)) == static_cast<t>(f);
69 }
70 static inline void AddFlag(NetPermissionFlags& flags, NetPermissionFlags f)
71 {
72- flags = static_cast<NetPermissionFlags>(flags | f);
73+ flags = (flags | f);
drop ALL_CAPS naming per https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-caps
TIL
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-Dropped unneeded parentheses, only change is git diff d53c70e 7075f60
0diff --git a/src/net_permissions.h b/src/net_permissions.h
1index 9cb59bd85f..7a158aa6c5 100644
2--- a/src/net_permissions.h
3+++ b/src/net_permissions.h
4@@ -56,7 +56,7 @@ public:
5 }
6 static inline void AddFlag(NetPermissionFlags& flags, NetPermissionFlags f)
7 {
8- flags = (flags | f);
9+ flags = flags | f;
10 }
Labels
Refactoring
P2P
Milestone
22.0