Merge #19191: net: Extract download permission from noban

fa0540cd46eaf44d9e1a9f91c3a937986826c4fa net: Extract download permission from noban (MarcoFalke)

Pull request description:

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

ACKs for top commit:
  jonatack:
    ACK fa0540c
  Sjors:
    re-utACK fa0540cd46eaf44d9e1a9f91c3a937986826c4fa

Tree-SHA512: 255566baa43ae925d93f5d0a3aa66b475a556d1590f662a88278a4872f16a1a05739a6119ae48a293011868042e05cb264cffe5822a50fb80db7333bf44376d9

11 files changed, 43 insertions, 29 deletions

diff --git a/doc/reduce-traffic.md b/doc/reduce-traffic.md
index ce77a00dd5..ea13dd5c27 100644
--- a/doc/reduce-traffic.md
+++ b/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`)
diff --git a/doc/release-notes.md b/doc/release-notes.md
index e73bedfb10..4d2225884e 100644
--- a/doc/release-notes.md
+++ b/doc/release-notes.md
@@ -102,6 +102,11 @@ Updated settings
 - The `-debug=db` logging category, which was deprecated in 0.20 and replaced by
   `-debug=walletdb` to distinguish it from `coindb`, has been removed. (#19202)
 
+- 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. (#19191)
+
 Changes to Wallet or GUI related settings can be found in the GUI or Wallet  section below.
 
 New settings
diff --git a/src/init.cpp b/src/init.cpp
index 2e39bcd9a5..4a4f33d6ef 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -446,7 +446,7 @@ void SetupServerArgs(NodeContext& node)
     gArgs.AddArg("-maxreceivebuffer=<n>", strprintf("Maximum per-connection receive buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXRECEIVEBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     gArgs.AddArg("-maxsendbuffer=<n>", strprintf("Maximum per-connection send buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXSENDBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     gArgs.AddArg("-maxtimeadjustment", strprintf("Maximum allowed median peer time offset adjustment. Local perspective of time may be influenced by peers forward or backward by this amount. (default: %u seconds)", DEFAULT_MAX_TIME_ADJUSTMENT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
-    gArgs.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' permission. 0 = no limit (default: %d)", DEFAULT_MAX_UPLOAD_TARGET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
+    gArgs.AddArg("-maxuploadtarget=<n>", strprintf("Tries to keep outbound traffic under the given target (in 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);
     gArgs.AddArg("-onion=<ip:port>", "Use separate SOCKS5 proxy to reach peers via Tor hidden services, set -noonion to disable (default: -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     gArgs.AddArg("-onlynet=<net>", "Make outgoing connections only through network <net> (ipv4, ipv6 or onion). Incoming connections are not affected by this option. This option can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     gArgs.AddArg("-peerbloomfilters", strprintf("Support filtering of blocks and transaction with bloom filters (default: %u)", DEFAULT_PEERBLOOMFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
@@ -469,12 +469,12 @@ void SetupServerArgs(NodeContext& node)
 #else
     hidden_args.emplace_back("-upnp");
 #endif
-    gArgs.AddArg("-whitebind=<[permissions@]addr>", "Bind to given address and whitelist peers connecting to it. "
+    gArgs.AddArg("-whitebind=<[permissions@]addr>", "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.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
+        "Specify multiple permissions separated by commas (default: download,noban,mempool,relay). Can be specified multiple times.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 
-    gArgs.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 "
+    gArgs.AddArg("-whitelist=<[permissions@]IP address or network>", "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);
 
     g_wallet_init_interface.AddWalletOptions();
diff --git a/src/net.cpp b/src/net.cpp
index 0cafb57e80..05ee26f8a5 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2644,7 +2644,7 @@ void CConnman::RecordBytesSent(uint64_t bytes)
         nMaxOutboundTotalBytesSentInCycle = 0;
     }
 
-    // TODO, exclude peers with noban permission
+    // TODO, exclude peers with download permission
     nMaxOutboundTotalBytesSentInCycle += bytes;
 }
 
diff --git a/src/net_permissions.cpp b/src/net_permissions.cpp
index da09149856..a75838307c 100644
--- a/src/net_permissions.cpp
+++ b/src/net_permissions.cpp
@@ -10,10 +10,11 @@
 
 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 {
@@ -46,6 +47,7 @@ bool TryParsePermissionFlags(const std::string str, NetPermissionFlags& output,
             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.length() == 0); // Allow empty entries
@@ -72,6 +74,7 @@ std::vector<std::string> NetPermissions::ToStrings(NetPermissionFlags flags)
     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");
     return strings;
 }
 
diff --git a/src/net_permissions.h b/src/net_permissions.h
index 2cf85120fe..a9633ee2ae 100644
--- a/src/net_permissions.h
+++ b/src/net_permissions.h
@@ -14,8 +14,7 @@ struct bilingual_str;
 
 extern const std::vector<std::string> NET_PERMISSIONS_DOC;
 
-enum NetPermissionFlags
-{
+enum NetPermissionFlags {
     PF_NONE = 0,
     // Can query bloomfilter even if -peerbloomfilters is false
     PF_BLOOMFILTER = (1U << 1),
@@ -24,14 +23,16 @@ enum NetPermissionFlags
     // Always relay transactions from this peer, even if already in mempool
     // 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
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index f1a89a8936..7f89443243 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1509,7 +1509,7 @@ void static ProcessGetBlockData(CNode& pfrom, const CChainParams& chainparams, c
     if (send &&
         connman->OutboundTargetReached(true) &&
         (((pindexBestHeader != nullptr) && (pindexBestHeader->GetBlockTime() - pindex->GetBlockTime() > HISTORICAL_BLOCK_AGE)) || inv.type == MSG_FILTERED_BLOCK) &&
-        !pfrom.HasPermission(PF_NOBAN) // never disconnect nodes with the noban permission
+        !pfrom.HasPermission(PF_DOWNLOAD) // nodes with the download permission may exceed target
     ) {
         LogPrint(BCLog::NET, "historical block serving limit reached, disconnect peer=%d\n", pfrom.GetId());
 
@@ -2739,7 +2739,7 @@ void ProcessMessage(
         }
 
         LOCK(cs_main);
-        if (::ChainstateActive().IsInitialBlockDownload() && !pfrom.HasPermission(PF_NOBAN)) {
+        if (::ChainstateActive().IsInitialBlockDownload() && !pfrom.HasPermission(PF_DOWNLOAD)) {
             LogPrint(BCLog::NET, "Ignoring getheaders from peer=%d because node is in initial block download\n", pfrom.GetId());
             return;
         }
diff --git a/src/test/netbase_tests.cpp b/src/test/netbase_tests.cpp
index 0fbf257f0e..c4d5d0b6d0 100644
--- a/src/test/netbase_tests.cpp
+++ b/src/test/netbase_tests.cpp
@@ -397,12 +397,13 @@ BOOST_AUTO_TEST_CASE(netpermissions_test)
     BOOST_CHECK(NetWhitelistPermissions::TryParse("bloom,forcerelay,noban,relay,mempool@1.2.3.4/32", whitelistPermissions, 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") != strings.end());
     BOOST_CHECK(std::find(strings.begin(), strings.end(), "relay") != strings.end());
     BOOST_CHECK(std::find(strings.begin(), strings.end(), "noban") != 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(netbase_dont_resolve_strings_with_embedded_nul_characters)
diff --git a/test/functional/feature_maxuploadtarget.py b/test/functional/feature_maxuploadtarget.py
index 7eabf86cad..5056b807cb 100755
--- a/test/functional/feature_maxuploadtarget.py
+++ b/test/functional/feature_maxuploadtarget.py
@@ -137,8 +137,8 @@ class MaxUploadTest(BitcoinTestFramework):
 
         self.nodes[0].disconnect_p2ps()
 
-        self.log.info("Restarting node 0 with noban permission and 1MB maxuploadtarget")
-        self.restart_node(0, ["-whitelist=noban@127.0.0.1", "-maxuploadtarget=1"])
+        self.log.info("Restarting node 0 with download permission and 1MB maxuploadtarget")
+        self.restart_node(0, ["-whitelist=download@127.0.0.1", "-maxuploadtarget=1"])
 
         # Reconnect to self.nodes[0]
         self.nodes[0].add_p2p_connection(TestP2PConn())
@@ -151,9 +151,12 @@ class MaxUploadTest(BitcoinTestFramework):
 
         getdata_request.inv = [CInv(MSG_BLOCK, big_old_block)]
         self.nodes[0].p2p.send_and_ping(getdata_request)
-        assert_equal(len(self.nodes[0].getpeerinfo()), 1)  #node is still connected because of the noban permission
 
-        self.log.info("Peer still connected after trying to download old block (noban permission)")
+        self.log.info("Peer still connected after trying to download old block (download permission)")
+        peer_info = self.nodes[0].getpeerinfo()
+        assert_equal(len(peer_info), 1)  # node is still connected
+        assert_equal(peer_info[0]['permissions'], ['download'])
+
 
 if __name__ == '__main__':
     MaxUploadTest().main()
diff --git a/test/functional/p2p_blocksonly.py b/test/functional/p2p_blocksonly.py
index c155dda664..0865c232e8 100755
--- a/test/functional/p2p_blocksonly.py
+++ b/test/functional/p2p_blocksonly.py
@@ -65,10 +65,10 @@ class P2PBlocksOnly(BitcoinTestFramework):
         second_peer = self.nodes[0].add_p2p_connection(P2PInterface())
         peer_1_info = self.nodes[0].getpeerinfo()[0]
         assert_equal(peer_1_info['whitelisted'], True)
-        assert_equal(peer_1_info['permissions'], ['noban', 'forcerelay', 'relay', 'mempool'])
+        assert_equal(peer_1_info['permissions'], ['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'])
+        assert_equal(peer_2_info['permissions'], ['noban', 'forcerelay', 'relay', 'mempool', 'download'])
         assert_equal(self.nodes[0].testmempoolaccept([sigtx])[0]['allowed'], True)
         txid = self.nodes[0].testmempoolaccept([sigtx])[0]['txid']
 
diff --git a/test/functional/p2p_permissions.py b/test/functional/p2p_permissions.py
index bea202855d..d0656cdb49 100755
--- a/test/functional/p2p_permissions.py
+++ b/test/functional/p2p_permissions.py
@@ -39,7 +39,8 @@ class P2PPermissionsTests(BitcoinTestFramework):
         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(
@@ -51,7 +52,7 @@ class P2PPermissionsTests(BitcoinTestFramework):
         self.checkpermission(
             # relay permission removed (no specific permissions)
             ["-whitelist=127.0.0.1", "-whitelistrelay=0"],
-            ["noban", "mempool"],
+            ["noban", "mempool", "download"],
             True)
 
         self.checkpermission(
@@ -59,7 +60,7 @@ class P2PPermissionsTests(BitcoinTestFramework):
             # 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
@@ -70,32 +71,32 @@ class P2PPermissionsTests(BitcoinTestFramework):
         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, "whitebind=bloomfilter,forcerelay@" + ip_port, "bind=127.0.0.1")
 
         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)