Merge bitcoin/bitcoin#21506: p2p, refactor: make NetPermissionFlags an enum class

7075f60 scripted-diff: update noban documentation in net_processing.cpp (Jon Atack)
a95540c scripted-diff: rename NetPermissionFlags enumerators (Jon Atack)
810d092 p2p, refactor: make NetPermissionFlags a uint32 enum class (Jon Atack)
7b55a94 p2p: NetPermissions::HasFlag() pass flags param by value (Jon Atack)
91f6e6e scripted-diff: add NetPermissionFlags scopes where not already present (Jon Atack)

Pull request description:

  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 bitcoin/bitcoin#20196 (comment) and #21644, as only defined operations on the flags would compile.

ACKs for top commit:
  laanwj:
    Code review ACK 7075f60
  vasild:
    ACK 7075f60

Tree-SHA512: 7fcea66ee499f059efc78c934b5f729b3c8573fe304dee2c27c837c2f662b89324790568246d75b2a574cf9f059b42d3551d928996862f4358055eb43521e6f4

Author: laanwj

src/net.cpp

@@ -1005,7 +1005,7 @@ bool CConnman::AttemptToEvictConnection()
 
         LOCK(cs_vNodes);
         for (const CNode* node : vNodes) {
-            if (node->HasPermission(PF_NOBAN))
+            if (node->HasPermission(NetPermissionFlags::NoBan))
                 continue;
             if (!node->IsInboundConn())
                 continue;
@@ -1062,7 +1062,7 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
 
     const CAddress addr_bind = GetBindAddress(hSocket);
 
-    NetPermissionFlags permissionFlags = NetPermissionFlags::PF_NONE;
+    NetPermissionFlags permissionFlags = NetPermissionFlags::None;
     hListenSocket.AddSocketPermissionFlags(permissionFlags);
 
     CreateNodeFromAcceptedSocket(hSocket, permissionFlags, addr_bind, addr);
@@ -1077,12 +1077,12 @@ void CConnman::CreateNodeFromAcceptedSocket(SOCKET hSocket,
     int nMaxInbound = nMaxConnections - m_max_outbound;
 
     AddWhitelistPermissionFlags(permissionFlags, addr);
-    if (NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_ISIMPLICIT)) {
-        NetPermissions::ClearFlag(permissionFlags, PF_ISIMPLICIT);
-        if (gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) NetPermissions::AddFlag(permissionFlags, PF_FORCERELAY);
-        if (gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) NetPermissions::AddFlag(permissionFlags, PF_RELAY);
-        NetPermissions::AddFlag(permissionFlags, PF_MEMPOOL);
-        NetPermissions::AddFlag(permissionFlags, PF_NOBAN);
+    if (NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::Implicit)) {
+        NetPermissions::ClearFlag(permissionFlags, NetPermissionFlags::Implicit);
+        if (gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) NetPermissions::AddFlag(permissionFlags, NetPermissionFlags::ForceRelay);
+        if (gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) NetPermissions::AddFlag(permissionFlags, NetPermissionFlags::Relay);
+        NetPermissions::AddFlag(permissionFlags, NetPermissionFlags::Mempool);
+        NetPermissions::AddFlag(permissionFlags, NetPermissionFlags::NoBan);
     }
 
     {
@@ -1111,7 +1111,7 @@ void CConnman::CreateNodeFromAcceptedSocket(SOCKET hSocket,
 
     // Don't accept connections from banned peers.
     bool banned = m_banman && m_banman->IsBanned(addr);
-    if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && banned)
+    if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::NoBan) && banned)
     {
         LogPrint(BCLog::NET, "connection from %s dropped (banned)\n", addr.ToString());
         CloseSocket(hSocket);
@@ -1120,7 +1120,7 @@ void CConnman::CreateNodeFromAcceptedSocket(SOCKET hSocket,
 
     // Only accept connections from discouraged peers if our inbound slots aren't (almost) full.
     bool discouraged = m_banman && m_banman->IsDiscouraged(addr);
-    if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && nInbound + 1 >= nMaxInbound && discouraged)
+    if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::NoBan) && nInbound + 1 >= nMaxInbound && discouraged)
     {
         LogPrint(BCLog::NET, "connection from %s dropped (discouraged)\n", addr.ToString());
         CloseSocket(hSocket);
@@ -1141,7 +1141,7 @@ void CConnman::CreateNodeFromAcceptedSocket(SOCKET hSocket,
     uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize();
 
     ServiceFlags nodeServices = nLocalServices;
-    if (NetPermissions::HasFlag(permissionFlags, PF_BLOOMFILTER)) {
+    if (NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::BloomFilter)) {
         nodeServices = static_cast<ServiceFlags>(nodeServices | NODE_BLOOM);
     }
 
@@ -2253,7 +2253,7 @@ void CConnman::ThreadI2PAcceptIncoming()
             continue;
         }
 
-        CreateNodeFromAcceptedSocket(conn.sock->Release(), NetPermissionFlags::PF_NONE,
+        CreateNodeFromAcceptedSocket(conn.sock->Release(), NetPermissionFlags::None,
                                      CAddress{conn.me, NODE_NONE}, CAddress{conn.peer, NODE_NONE});
     }
 }
@@ -2411,7 +2411,7 @@ bool CConnman::Bind(const CService &addr, unsigned int flags, NetPermissionFlags
         return false;
     }
 
-    if (addr.IsRoutable() && fDiscover && !(flags & BF_DONT_ADVERTISE) && !NetPermissions::HasFlag(permissions, NetPermissionFlags::PF_NOBAN)) {
+    if (addr.IsRoutable() && fDiscover && !(flags & BF_DONT_ADVERTISE) && !NetPermissions::HasFlag(permissions, NetPermissionFlags::NoBan)) {
         AddLocal(addr, LOCAL_BIND);
     }
 
@@ -2425,7 +2425,7 @@ bool CConnman::InitBinds(
 {
     bool fBound = false;
     for (const auto& addrBind : binds) {
-        fBound |= Bind(addrBind, (BF_EXPLICIT | BF_REPORT_ERROR), NetPermissionFlags::PF_NONE);
+        fBound |= Bind(addrBind, (BF_EXPLICIT | BF_REPORT_ERROR), NetPermissionFlags::None);
     }
     for (const auto& addrBind : whiteBinds) {
         fBound |= Bind(addrBind.m_service, (BF_EXPLICIT | BF_REPORT_ERROR), addrBind.m_flags);
@@ -2434,12 +2434,12 @@ bool CConnman::InitBinds(
         struct in_addr inaddr_any;
         inaddr_any.s_addr = htonl(INADDR_ANY);
         struct in6_addr inaddr6_any = IN6ADDR_ANY_INIT;
-        fBound |= Bind(CService(inaddr6_any, GetListenPort()), BF_NONE, NetPermissionFlags::PF_NONE);
-        fBound |= Bind(CService(inaddr_any, GetListenPort()), !fBound ? BF_REPORT_ERROR : BF_NONE, NetPermissionFlags::PF_NONE);
+        fBound |= Bind(CService(inaddr6_any, GetListenPort()), BF_NONE, NetPermissionFlags::None);
+        fBound |= Bind(CService(inaddr_any, GetListenPort()), !fBound ? BF_REPORT_ERROR : BF_NONE, NetPermissionFlags::None);
     }
 
     for (const auto& addr_bind : onion_binds) {
-        fBound |= Bind(addr_bind, BF_EXPLICIT | BF_DONT_ADVERTISE, NetPermissionFlags::PF_NONE);
+        fBound |= Bind(addr_bind, BF_EXPLICIT | BF_DONT_ADVERTISE, NetPermissionFlags::None);
     }
 
     return fBound;

src/net.h

@@ -402,7 +402,7 @@ class CNode
     std::unique_ptr<TransportDeserializer> m_deserializer;
     std::unique_ptr<TransportSerializer> m_serializer;
 
-    NetPermissionFlags m_permissionFlags{PF_NONE};
+    NetPermissionFlags m_permissionFlags{NetPermissionFlags::None};
     std::atomic<ServiceFlags> nServices{NODE_NONE};
     SOCKET hSocket GUARDED_BY(cs_hSocket);
     /** Total size of all vSendMsg entries */

src/net_permissions.cpp

@@ -23,12 +23,12 @@ namespace {
 // The parse the following format "perm1,perm2@xxxxxx"
 bool TryParsePermissionFlags(const std::string str, NetPermissionFlags& output, size_t& readen, bilingual_str& error)
 {
-    NetPermissionFlags flags = PF_NONE;
+    NetPermissionFlags flags = NetPermissionFlags::None;
     const auto atSeparator = str.find('@');
 
     // if '@' is not found (ie, "xxxxx"), the caller should apply implicit permissions
     if (atSeparator == std::string::npos) {
-        NetPermissions::AddFlag(flags, PF_ISIMPLICIT);
+        NetPermissions::AddFlag(flags, NetPermissionFlags::Implicit);
         readen = 0;
     }
     // else (ie, "perm1,perm2@xxxxx"), let's enumerate the permissions by splitting by ',' and calculate the flags
@@ -44,14 +44,14 @@ bool TryParsePermissionFlags(const std::string str, NetPermissionFlags& output,
             readen += len; // We read "perm1"
             if (commaSeparator != std::string::npos) readen++; // We read ","
 
-            if (permission == "bloomfilter" || permission == "bloom") NetPermissions::AddFlag(flags, PF_BLOOMFILTER);
-            else if (permission == "noban") NetPermissions::AddFlag(flags, PF_NOBAN);
-            else if (permission == "forcerelay") NetPermissions::AddFlag(flags, PF_FORCERELAY);
-            else if (permission == "mempool") NetPermissions::AddFlag(flags, PF_MEMPOOL);
-            else if (permission == "download") NetPermissions::AddFlag(flags, PF_DOWNLOAD);
-            else if (permission == "all") NetPermissions::AddFlag(flags, PF_ALL);
-            else if (permission == "relay") NetPermissions::AddFlag(flags, PF_RELAY);
-            else if (permission == "addr") NetPermissions::AddFlag(flags, PF_ADDR);
+            if (permission == "bloomfilter" || permission == "bloom") NetPermissions::AddFlag(flags, NetPermissionFlags::BloomFilter);
+            else if (permission == "noban") NetPermissions::AddFlag(flags, NetPermissionFlags::NoBan);
+            else if (permission == "forcerelay") NetPermissions::AddFlag(flags, NetPermissionFlags::ForceRelay);
+            else if (permission == "mempool") NetPermissions::AddFlag(flags, NetPermissionFlags::Mempool);
+            else if (permission == "download") NetPermissions::AddFlag(flags, NetPermissionFlags::Download);
+            else if (permission == "all") NetPermissions::AddFlag(flags, NetPermissionFlags::All);
+            else if (permission == "relay") NetPermissions::AddFlag(flags, NetPermissionFlags::Relay);
+            else if (permission == "addr") NetPermissions::AddFlag(flags, NetPermissionFlags::Addr);
             else if (permission.length() == 0); // Allow empty entries
             else {
                 error = strprintf(_("Invalid P2P permission: '%s'"), permission);
@@ -71,13 +71,13 @@ bool TryParsePermissionFlags(const std::string str, NetPermissionFlags& output,
 std::vector<std::string> NetPermissions::ToStrings(NetPermissionFlags flags)
 {
     std::vector<std::string> strings;
-    if (NetPermissions::HasFlag(flags, PF_BLOOMFILTER)) strings.push_back("bloomfilter");
-    if (NetPermissions::HasFlag(flags, PF_NOBAN)) strings.push_back("noban");
-    if (NetPermissions::HasFlag(flags, PF_FORCERELAY)) strings.push_back("forcerelay");
-    if (NetPermissions::HasFlag(flags, PF_RELAY)) strings.push_back("relay");
-    if (NetPermissions::HasFlag(flags, PF_MEMPOOL)) strings.push_back("mempool");
-    if (NetPermissions::HasFlag(flags, PF_DOWNLOAD)) strings.push_back("download");
-    if (NetPermissions::HasFlag(flags, PF_ADDR)) strings.push_back("addr");
+    if (NetPermissions::HasFlag(flags, NetPermissionFlags::BloomFilter)) strings.push_back("bloomfilter");
+    if (NetPermissions::HasFlag(flags, NetPermissionFlags::NoBan)) strings.push_back("noban");
+    if (NetPermissions::HasFlag(flags, NetPermissionFlags::ForceRelay)) strings.push_back("forcerelay");
+    if (NetPermissions::HasFlag(flags, NetPermissionFlags::Relay)) strings.push_back("relay");
+    if (NetPermissions::HasFlag(flags, NetPermissionFlags::Mempool)) strings.push_back("mempool");
+    if (NetPermissions::HasFlag(flags, NetPermissionFlags::Download)) strings.push_back("download");
+    if (NetPermissions::HasFlag(flags, NetPermissionFlags::Addr)) strings.push_back("addr");
     return strings;
 }
 

src/net_permissions.h

@@ -5,6 +5,7 @@
 #include <netaddress.h>
 
 #include <string>
+#include <type_traits>
 #include <vector>
 
 #ifndef BITCOIN_NET_PERMISSIONS_H
@@ -14,52 +15,59 @@ struct bilingual_str;
 
 extern const std::vector<std::string> NET_PERMISSIONS_DOC;
 
-enum NetPermissionFlags {
-    PF_NONE = 0,
+enum class NetPermissionFlags : uint32_t {
+    None = 0,
     // Can query bloomfilter even if -peerbloomfilters is false
-    PF_BLOOMFILTER = (1U << 1),
+    BloomFilter = (1U << 1),
     // Relay and accept transactions from this peer, even if -blocksonly is true
     // This peer is also not subject to limits on how many transaction INVs are tracked
-    PF_RELAY = (1U << 3),
+    Relay = (1U << 3),
     // Always relay transactions from this peer, even if already in mempool
     // Keep parameter interaction: forcerelay implies relay
-    PF_FORCERELAY = (1U << 2) | PF_RELAY,
+    ForceRelay = (1U << 2) | Relay,
     // Allow getheaders during IBD and block-download after maxuploadtarget limit
-    PF_DOWNLOAD = (1U << 6),
+    Download = (1U << 6),
     // Can't be banned/disconnected/discouraged for misbehavior
-    PF_NOBAN = (1U << 4) | PF_DOWNLOAD,
+    NoBan = (1U << 4) | Download,
     // Can query the mempool
-    PF_MEMPOOL = (1U << 5),
+    Mempool = (1U << 5),
     // Can request addrs without hitting a privacy-preserving cache
-    PF_ADDR = (1U << 7),
+    Addr = (1U << 7),
 
     // True if the user did not specifically set fine grained permissions
-    PF_ISIMPLICIT = (1U << 31),
-    PF_ALL = PF_BLOOMFILTER | PF_FORCERELAY | PF_RELAY | PF_NOBAN | PF_MEMPOOL | PF_DOWNLOAD | PF_ADDR,
+    Implicit = (1U << 31),
+    All = BloomFilter | ForceRelay | Relay | NoBan | Mempool | Download | Addr,
 };
+static inline constexpr NetPermissionFlags operator|(NetPermissionFlags a, NetPermissionFlags b)
+{
+    using t = typename std::underlying_type<NetPermissionFlags>::type;
+    return static_cast<NetPermissionFlags>(static_cast<t>(a) | static_cast<t>(b));
+}
 
 class NetPermissions
 {
 public:
     NetPermissionFlags m_flags;
     static std::vector<std::string> ToStrings(NetPermissionFlags flags);
-    static inline bool HasFlag(const NetPermissionFlags& flags, NetPermissionFlags f)
+    static inline bool HasFlag(NetPermissionFlags flags, NetPermissionFlags f)
     {
-        return (flags & f) == f;
+        using t = typename std::underlying_type<NetPermissionFlags>::type;
+        return (static_cast<t>(flags) & static_cast<t>(f)) == static_cast<t>(f);
     }
     static inline void AddFlag(NetPermissionFlags& flags, NetPermissionFlags f)
     {
-        flags = static_cast<NetPermissionFlags>(flags | f);
+        flags = flags | f;
     }
-    //! ClearFlag is only called with `f` == NetPermissionFlags::PF_ISIMPLICIT.
+    //! ClearFlag is only called with `f` == NetPermissionFlags::Implicit.
     //! If that should change in the future, be aware that ClearFlag should not
-    //! be called with a subflag of a multiflag, e.g. NetPermissionFlags::PF_RELAY
-    //! or NetPermissionFlags::PF_DOWNLOAD, as that would leave `flags` in an
+    //! be called with a subflag of a multiflag, e.g. NetPermissionFlags::Relay
+    //! or NetPermissionFlags::Download, as that would leave `flags` in an
     //! invalid state corresponding to none of the existing flags.
     static inline void ClearFlag(NetPermissionFlags& flags, NetPermissionFlags f)
     {
-        assert(f == NetPermissionFlags::PF_ISIMPLICIT);
-        flags = static_cast<NetPermissionFlags>(flags & ~f);
+        assert(f == NetPermissionFlags::Implicit);
+        using t = typename std::underlying_type<NetPermissionFlags>::type;
+        flags = static_cast<NetPermissionFlags>(static_cast<t>(flags) & ~static_cast<t>(f));
     }
 };