net: Extract download permission from noban

Summary:
```
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.
```

Backport of [[bitcoin/bitcoin#19191 | core#19191]].

Depends on D9324 and D9325.

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Subscribers: majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9326

doc/reduce-traffic.md

@@ -23,7 +23,7 @@ longer serving historic blocks (blocks older than one week).
 Keep in mind that new nodes require other nodes that are willing to serve
 historic blocks.
 
-Peers with the `noban` permission will never be disconnected, although their traffic counts for
+Peers with the `download` permission will never be disconnected, although their traffic counts for
 calculating the target.
 
 ## 2. Disable "listening" (`-listen=0`)

doc/release-notes.md

@@ -5,3 +5,7 @@ Bitcoin ABC version 0.23.0 is now available from:
   <https://download.bitcoinabc.org/0.23.0/>
 
 This release includes the following features and fixes:
+ - A `download` permission has been extracted from the `noban` permission. For
+   compatibility, `noban` implies the `download` permission, but this may change
+   in future releases. Refer to the help of the affected settings `-whitebind`
+   and `-whitelist` for more details.

src/init.cpp

@@ -783,24 +783,26 @@ void SetupServerArgs(NodeContext &node) {
 #endif
     argsman.AddArg(
         "-whitebind=<[permissions@]addr>",
-        "Bind to given address and whitelist peers connecting to it."
+        "Bind to the given address and add permission flags to the peers "
+        "connecting to it."
         "Use [host]:port notation for IPv6. Allowed permissions: " +
             Join(NET_PERMISSIONS_DOC, ", ") +
             ". "
             "Specify multiple permissions separated by commas (default: "
-            "noban,mempool,relay). Can be specified multiple times.",
+            "download,noban,mempool,relay). Can be specified multiple times.",
         ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 
     argsman.AddArg("-whitelist=<[permissions@]IP address or network>",
-                   "Whitelist peers connecting from the given IP address "
-                   "(e.g. 1.2.3.4) or CIDR notated network(e.g. 1.2.3.0/24). "
-                   "Uses same permissions as -whitebind. Can be specified "
+                   "Add permission flags to the peers connecting from the "
+                   "given IP address (e.g. 1.2.3.4) or CIDR-notated network "
+                   "(e.g. 1.2.3.0/24). "
+                   "Uses the same permissions as -whitebind. Can be specified "
                    "multiple times.",
                    ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg(
         "-maxuploadtarget=<n>",
         strprintf("Tries to keep outbound traffic under the given target (in "
-                  "MiB per 24h). Limit does not apply to peers with 'noban' "
+                  "MiB per 24h). Limit does not apply to peers with 'download' "
                   "permission. 0 = no limit (default: %d)",
                   DEFAULT_MAX_UPLOAD_TARGET),
         ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

src/net.cpp

@@ -2884,7 +2884,7 @@ void CConnman::RecordBytesSent(uint64_t bytes) {
         nMaxOutboundTotalBytesSentInCycle = 0;
     }
 
-    // TODO, exclude peers with noban permission
+    // TODO, exclude peers with download permission
     nMaxOutboundTotalBytesSentInCycle += bytes;
 }
 

src/net_permissions.cpp

@@ -11,11 +11,13 @@
 
 const std::vector<std::string> NET_PERMISSIONS_DOC{
     "bloomfilter (allow requesting BIP37 filtered blocks and transactions)",
-    "noban (do not ban for misbehavior)",
+    "noban (do not ban for misbehavior; implies download)",
     "forcerelay (relay transactions that are already in the mempool; implies "
     "relay)",
     "relay (relay even in -blocksonly mode)",
     "mempool (allow requesting BIP35 mempool contents)",
+    "download (allow getheaders during IBD, no disconnect after "
+    "maxuploadtarget limit)",
 };
 
 namespace {
@@ -60,6 +62,8 @@ bool TryParsePermissionFlags(const std::string str, NetPermissionFlags &output,
                 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") {
@@ -99,6 +103,9 @@ std::vector<std::string> NetPermissions::ToStrings(NetPermissionFlags flags) {
     if (NetPermissions::HasFlag(flags, PF_MEMPOOL)) {
         strings.push_back("mempool");
     }
+    if (NetPermissions::HasFlag(flags, PF_DOWNLOAD)) {
+        strings.push_back("download");
+    }
     return strings;
 }
 

src/net_permissions.h

@@ -23,14 +23,18 @@ enum NetPermissionFlags {
     // Always relay transactions from this peer, even if already in mempool or
     // rejected from policy Keep parameter interaction: forcerelay implies relay
     PF_FORCERELAY = (1U << 2) | PF_RELAY,
+    // Allow getheaders during IBD and block-download after maxuploadtarget
+    // limit
+    PF_DOWNLOAD = (1U << 6),
     // Can't be banned/disconnected/discouraged for misbehavior
-    PF_NOBAN = (1U << 4),
+    PF_NOBAN = (1U << 4) | PF_DOWNLOAD,
     // Can query the mempool
     PF_MEMPOOL = (1U << 5),
 
     // 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_ALL = PF_BLOOMFILTER | PF_FORCERELAY | PF_RELAY | PF_NOBAN | PF_MEMPOOL |
+             PF_DOWNLOAD,
 };
 
 class NetPermissions {

src/net_processing.cpp

@@ -1858,8 +1858,8 @@ static void ProcessGetBlockData(const Config &config, CNode &pfrom,
           (pindexBestHeader->GetBlockTime() - pindex->GetBlockTime() >
            HISTORICAL_BLOCK_AGE)) ||
          inv.type == MSG_FILTERED_BLOCK) &&
-        /* never disconnect nodes with the noban permission */
-        !pfrom.HasPermission(PF_NOBAN)) {
+        // nodes with the download permission may exceed target
+        !pfrom.HasPermission(PF_DOWNLOAD)) {
         LogPrint(BCLog::NET,
                  "historical block serving limit reached, disconnect peer=%d\n",
                  pfrom.GetId());
@@ -3286,7 +3286,7 @@ void PeerManager::ProcessMessage(const Config &config, CNode &pfrom,
 
         LOCK(cs_main);
         if (::ChainstateActive().IsInitialBlockDownload() &&
-            !pfrom.HasPermission(PF_NOBAN)) {
+            !pfrom.HasPermission(PF_DOWNLOAD)) {
             LogPrint(BCLog::NET,
                      "Ignoring getheaders from peer=%d because node is in "
                      "initial block download\n",

src/test/netbase_tests.cpp

@@ -487,7 +487,7 @@ BOOST_AUTO_TEST_CASE(netpermissions_test) {
         error));
 
     const auto strings = NetPermissions::ToStrings(PF_ALL);
-    BOOST_CHECK_EQUAL(strings.size(), 5U);
+    BOOST_CHECK_EQUAL(strings.size(), 6U);
     BOOST_CHECK(std::find(strings.begin(), strings.end(), "bloomfilter") !=
                 strings.end());
     BOOST_CHECK(std::find(strings.begin(), strings.end(), "forcerelay") !=
@@ -498,6 +498,8 @@ BOOST_AUTO_TEST_CASE(netpermissions_test) {
                 strings.end());
     BOOST_CHECK(std::find(strings.begin(), strings.end(), "mempool") !=
                 strings.end());
+    BOOST_CHECK(std::find(strings.begin(), strings.end(), "download") !=
+                strings.end());
 }
 
 BOOST_AUTO_TEST_CASE(

test/functional/feature_maxuploadtarget.py

@@ -150,9 +150,9 @@ def run_test(self):
 
         self.nodes[0].disconnect_p2ps()
 
-        self.log.info("Restarting node 0 with noban permission"
+        self.log.info("Restarting node 0 with download permission"
                       " and 1MB maxuploadtarget")
-        self.restart_node(0, ["-whitelist=noban@127.0.0.1",
+        self.restart_node(0, ["-whitelist=download@127.0.0.1",
                               "-maxuploadtarget=1", "-blockmaxsize=999000"])
 
         # Reconnect to self.nodes[0]
@@ -167,12 +167,13 @@ def run_test(self):
 
         getdata_request.inv = [CInv(MSG_BLOCK, big_old_block)]
         self.nodes[0].p2p.send_and_ping(getdata_request)
-        # node is still connected because of the noban permission
-        assert_equal(len(self.nodes[0].getpeerinfo()), 1)
 
         self.log.info(
-            "Peer still connected after trying to download old block "
-            "(noban permission)")
+            "Peer still connected after trying to download old block (download permission)")
+        peer_info = self.nodes[0].getpeerinfo()
+        # node is still connected
+        assert_equal(len(peer_info), 1)
+        assert_equal(peer_info[0]['permissions'], ['download'])
 
 
 if __name__ == '__main__':

test/functional/p2p_blocksonly.py

@@ -80,12 +80,12 @@ def run_test(self):
         assert_equal(peer_1_info['whitelisted'], True)
         assert_equal(
             peer_1_info['permissions'], [
-                'noban', 'forcerelay', 'relay', 'mempool'])
+                'noban', 'forcerelay', 'relay', 'mempool', 'download'])
         peer_2_info = self.nodes[0].getpeerinfo()[1]
         assert_equal(peer_2_info['whitelisted'], True)
         assert_equal(
             peer_2_info['permissions'], [
-                'noban', 'forcerelay', 'relay', 'mempool'])
+                'noban', 'forcerelay', 'relay', 'mempool', 'download'])
         assert_equal(
             self.nodes[0].testmempoolaccept(
                 [sigtx])[0]['allowed'], True)

test/functional/p2p_permissions.py

@@ -27,21 +27,23 @@ def run_test(self):
         self.checkpermission(
             # default permissions (no specific permissions)
             ["-whitelist=127.0.0.1"],
-            ["relay", "noban", "mempool"],
+            # Make sure the default values in the command line documentation
+            # match the ones here
+            ["relay", "noban", "mempool", "download"],
             True)
 
         self.checkpermission(
             # relay permission removed (no specific permissions)
             ["-whitelist=127.0.0.1", "-whitelistrelay=0"],
-            ["noban", "mempool"],
+            ["noban", "mempool", "download"],
             True)
 
         self.checkpermission(
             # forcerelay and relay permission added
             # Legacy parameter interaction which set whitelistrelay to true
             # if whitelistforcerelay is true
             ["-whitelist=127.0.0.1", "-whitelistforcerelay"],
-            ["forcerelay", "relay", "noban", "mempool"],
+            ["forcerelay", "relay", "noban", "mempool", "download"],
             True)
 
         # Let's make sure permissions are merged correctly
@@ -56,7 +58,7 @@ def run_test(self):
         self.checkpermission(
             ["-whitelist=noban@127.0.0.1"],
             # Check parameter interaction forcerelay should activate relay
-            ["noban", "bloomfilter", "forcerelay", "relay"],
+            ["noban", "bloomfilter", "forcerelay", "relay", "download"],
             False)
         self.replaceinconfig(
             1,
@@ -67,25 +69,25 @@ def run_test(self):
         self.checkpermission(
             # legacy whitelistrelay should be ignored
             ["-whitelist=noban,mempool@127.0.0.1", "-whitelistrelay"],
-            ["noban", "mempool"],
+            ["noban", "mempool", "download"],
             False)
 
         self.checkpermission(
             # legacy whitelistforcerelay should be ignored
             ["-whitelist=noban,mempool@127.0.0.1", "-whitelistforcerelay"],
-            ["noban", "mempool"],
+            ["noban", "mempool", "download"],
             False)
 
         self.checkpermission(
             # missing mempool permission to be considered legacy whitelisted
             ["-whitelist=noban@127.0.0.1"],
-            ["noban"],
+            ["noban", "download"],
             False)
 
         self.checkpermission(
             # all permission added
             ["-whitelist=all@127.0.0.1"],
-            ["forcerelay", "noban", "mempool", "bloomfilter", "relay"],
+            ["forcerelay", "noban", "mempool", "bloomfilter", "relay", "download"],
             False)
 
         self.stop_node(1)