Merge bitcoin/bitcoin#21644: p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind()

36fb036d25e2a3016b36873456e5a9e6251ffef8 p2p: allow NetPermissions::ClearFlag() only with PF_ISIMPLICIT (Jon Atack)
4e0d5788ba5771c81bc0ff2e6523cf9accddae46 test: add net permissions noban/download unit test coverage (Jon Atack)
dde69f20a01acca64ac21cb13993c6e4f8709f23 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 36fb036d25e2a3016b36873456e5a9e6251ffef8 ☕
  vasild:
    ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8
  hebasto:
    ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8, I have reviewed the code and it looks OK, I agree it can be merged.
  kallewoof:
    Code review ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8

Tree-SHA512: 5fbc7ddbf31d06b35bf238f4d77ef311e6b6ef2e1bb9893f32f889c1a0f65774a3710dcb21d94317fe6166df9334a9f2d42630809e7fe8cbd797dd6f6fc49491

4 files changed, 36 insertions, 5 deletions

diff --git a/src/net.cpp b/src/net.cpp
index ef4521d38a..ea5d11da18 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2399,8 +2399,9 @@ NodeId CConnman::GetNewNodeId()
 
 
 bool CConnman::Bind(const CService &addr, unsigned int flags, NetPermissionFlags permissions) {
-    if (!(flags & BF_EXPLICIT) && !IsReachable(addr))
+    if (!(flags & BF_EXPLICIT) && !IsReachable(addr)) {
         return false;
+    }
     bilingual_str strError;
     if (!BindListenPort(addr, strError, permissions)) {
         if ((flags & BF_REPORT_ERROR) && clientInterface) {
@@ -2409,7 +2410,7 @@ bool CConnman::Bind(const CService &addr, unsigned int flags, NetPermissionFlags
         return false;
     }
 
-    if (addr.IsRoutable() && fDiscover && !(flags & BF_DONT_ADVERTISE) && !(permissions & PF_NOBAN)) {
+    if (addr.IsRoutable() && fDiscover && !(flags & BF_DONT_ADVERTISE) && !NetPermissions::HasFlag(permissions, NetPermissionFlags::PF_NOBAN)) {
         AddLocal(addr, LOCAL_BIND);
     }
 
diff --git a/src/net_permissions.h b/src/net_permissions.h
index bba0ea1695..142b317bf6 100644
--- a/src/net_permissions.h
+++ b/src/net_permissions.h
@@ -51,8 +51,14 @@ public:
     {
         flags = static_cast<NetPermissionFlags>(flags | f);
     }
+    //! ClearFlag is only called with `f` == NetPermissionFlags::PF_ISIMPLICIT.
+    //! 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
+    //! 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);
     }
 };
diff --git a/src/test/fuzz/net_permissions.cpp b/src/test/fuzz/net_permissions.cpp
index 544a33047b..6fdf4b653c 100644
--- a/src/test/fuzz/net_permissions.cpp
+++ b/src/test/fuzz/net_permissions.cpp
@@ -25,7 +25,7 @@ FUZZ_TARGET(net_permissions)
         (void)NetPermissions::ToStrings(net_whitebind_permissions.m_flags);
         (void)NetPermissions::AddFlag(net_whitebind_permissions.m_flags, net_permission_flags);
         assert(NetPermissions::HasFlag(net_whitebind_permissions.m_flags, net_permission_flags));
-        (void)NetPermissions::ClearFlag(net_whitebind_permissions.m_flags, net_permission_flags);
+        (void)NetPermissions::ClearFlag(net_whitebind_permissions.m_flags, NetPermissionFlags::PF_ISIMPLICIT);
         (void)NetPermissions::ToStrings(net_whitebind_permissions.m_flags);
     }
 
@@ -35,7 +35,7 @@ FUZZ_TARGET(net_permissions)
         (void)NetPermissions::ToStrings(net_whitelist_permissions.m_flags);
         (void)NetPermissions::AddFlag(net_whitelist_permissions.m_flags, net_permission_flags);
         assert(NetPermissions::HasFlag(net_whitelist_permissions.m_flags, net_permission_flags));
-        (void)NetPermissions::ClearFlag(net_whitelist_permissions.m_flags, net_permission_flags);
+        (void)NetPermissions::ClearFlag(net_whitelist_permissions.m_flags, NetPermissionFlags::PF_ISIMPLICIT);
         (void)NetPermissions::ToStrings(net_whitelist_permissions.m_flags);
     }
 }
diff --git a/src/test/netbase_tests.cpp b/src/test/netbase_tests.cpp
index 33b56624a8..b316a37c6e 100644
--- a/src/test/netbase_tests.cpp
+++ b/src/test/netbase_tests.cpp
@@ -395,6 +395,28 @@ BOOST_AUTO_TEST_CASE(netpermissions_test)
     BOOST_CHECK(NetWhitebindPermissions::TryParse("@1.2.3.4:32", whitebindPermissions, error));
     BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_NONE);
 
+    NetWhitebindPermissions noban, noban_download, download_noban, download;
+
+    // "noban" implies "download"
+    BOOST_REQUIRE(NetWhitebindPermissions::TryParse("noban@1.2.3.4:32", noban, error));
+    BOOST_CHECK_EQUAL(noban.m_flags, NetPermissionFlags::PF_NOBAN);
+    BOOST_CHECK(NetPermissions::HasFlag(noban.m_flags, NetPermissionFlags::PF_DOWNLOAD));
+    BOOST_CHECK(NetPermissions::HasFlag(noban.m_flags, NetPermissionFlags::PF_NOBAN));
+
+    // "noban,download" is equivalent to "noban"
+    BOOST_REQUIRE(NetWhitebindPermissions::TryParse("noban,download@1.2.3.4:32", noban_download, error));
+    BOOST_CHECK_EQUAL(noban_download.m_flags, noban.m_flags);
+
+    // "download,noban" is equivalent to "noban"
+    BOOST_REQUIRE(NetWhitebindPermissions::TryParse("download,noban@1.2.3.4:32", download_noban, error));
+    BOOST_CHECK_EQUAL(download_noban.m_flags, noban.m_flags);
+
+    // "download" excludes (does not imply) "noban"
+    BOOST_REQUIRE(NetWhitebindPermissions::TryParse("download@1.2.3.4:32", download, error));
+    BOOST_CHECK_EQUAL(download.m_flags, NetPermissionFlags::PF_DOWNLOAD);
+    BOOST_CHECK(NetPermissions::HasFlag(download.m_flags, NetPermissionFlags::PF_DOWNLOAD));
+    BOOST_CHECK(!NetPermissions::HasFlag(download.m_flags, NetPermissionFlags::PF_NOBAN));
+
     // Happy path, can parse flags
     BOOST_CHECK(NetWhitebindPermissions::TryParse("bloom,forcerelay@1.2.3.4:32", whitebindPermissions, error));
     // forcerelay should also activate the relay permission
@@ -407,7 +429,7 @@ BOOST_AUTO_TEST_CASE(netpermissions_test)
 
     // Allow dups
     BOOST_CHECK(NetWhitebindPermissions::TryParse("bloom,relay,noban,noban@1.2.3.4:32", whitebindPermissions, error));
-    BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_BLOOMFILTER | PF_RELAY | PF_NOBAN);
+    BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_BLOOMFILTER | PF_RELAY | PF_NOBAN | PF_DOWNLOAD); // "noban" implies "download"
 
     // Allow empty
     BOOST_CHECK(NetWhitebindPermissions::TryParse("bloom,relay,,noban@1.2.3.4:32", whitebindPermissions, error));
@@ -428,6 +450,8 @@ BOOST_AUTO_TEST_CASE(netpermissions_test)
     // Happy path for whitelist parsing
     BOOST_CHECK(NetWhitelistPermissions::TryParse("noban@1.2.3.4", whitelistPermissions, error));
     BOOST_CHECK_EQUAL(whitelistPermissions.m_flags, PF_NOBAN);
+    BOOST_CHECK(NetPermissions::HasFlag(whitelistPermissions.m_flags, NetPermissionFlags::PF_NOBAN));
+
     BOOST_CHECK(NetWhitelistPermissions::TryParse("bloom,forcerelay,noban,relay@1.2.3.4/32", whitelistPermissions, error));
     BOOST_CHECK_EQUAL(whitelistPermissions.m_flags, PF_BLOOMFILTER | PF_FORCERELAY | PF_NOBAN | PF_RELAY);
     BOOST_CHECK(error.empty());