merge bitcoin#19191: Extract download permission from noban

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-19191.md

@@ -0,0 +1,7 @@
+Updated settings
+----------------
+
+- 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)

src/init.cpp

@@ -578,7 +578,7 @@ void SetupServerArgs(NodeContext& node)
     argsman.AddArg("-maxreceivebuffer=<n>", strprintf("Maximum per-connection receive buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXRECEIVEBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-maxsendbuffer=<n>", strprintf("Maximum per-connection send buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXSENDBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.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);
-    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' permission. 0 = no limit (default: %d)", DEFAULT_MAX_UPLOAD_TARGET), 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 'download' permission. 0 = no limit (default: %d)", DEFAULT_MAX_UPLOAD_TARGET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.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);
     argsman.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);
     argsman.AddArg("-peerblockfilters", strprintf("Serve compact block filters to peers per BIP 157 (default: %u)", DEFAULT_PEERBLOCKFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
@@ -607,12 +607,12 @@ void SetupServerArgs(NodeContext& node)
 #else
     hidden_args.emplace_back("-natpmp");
 #endif // USE_NATPMP
-    argsman.AddArg("-whitebind=<[permissions@]addr>", "Bind to given address and whitelist peers connecting to it. "
+    argsman.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);
 
-    argsman.AddArg("-whitelist=<[permissions@]IP address or network>", "Whitelist peers connecting from the given IP address (e.g. 1.2.3.4) or "
+    argsman.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 same permissions as "
+        "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(argsman);

src/net.cpp

@@ -3686,7 +3686,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

@@ -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;
 }
 

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

src/net_processing.cpp

@@ -1895,7 +1895,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());
 
@@ -3407,7 +3407,7 @@ void PeerManagerImpl::ProcessMessage(
         }
 
         LOCK(cs_main);
-        if (::ChainstateActive().IsInitialBlockDownload() && !pfrom.HasPermission(PF_NOBAN)) {
+        if (::ChainstateActive().IsInitialBlockDownload() && !pfrom.HasPermission(PF_DOWNLOAD)) {
             LogPrint(BCLog::NET, "Ignoring %s from peer=%d because node is in initial block download\n", msg_type, pfrom.GetId());
             return;
         }

src/test/netbase_tests.cpp

@@ -521,12 +521,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(), 5);
+    BOOST_CHECK_EQUAL(strings.size(), 6);
     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)

test/functional/feature_maxuploadtarget.py

@@ -143,8 +143,8 @@ class MaxUploadTest(BitcoinTestFramework):
 
         self.nodes[0].disconnect_p2ps()
 
-        self.log.info("Restarting node 0 with noban permission and 1MB maxuploadtarget")
+        self.log.info("Restarting node 0 with download permission and 1MB maxuploadtarget")
-        self.restart_node(0, ["-whitelist=noban@127.0.0.1", "-maxuploadtarget=1", "-blockmaxsize=999000", "-maxtipage="+str(2*60*60*24*7), "-mocktime="+str(current_mocktime)])
+        self.restart_node(0, ["-whitelist=download@127.0.0.1", "-maxuploadtarget=1", "-blockmaxsize=999000", "-maxtipage="+str(2*60*60*24*7), "-mocktime="+str(current_mocktime)])
 
         # Reconnect to self.nodes[0]
         self.nodes[0].add_p2p_connection(TestP2PConn())
@@ -157,9 +157,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()

test/functional/p2p_blocksonly.py

@@ -66,10 +66,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']
 

test/functional/p2p_permissions.py

@@ -37,7 +37,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(
@@ -49,7 +50,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(
@@ -57,7 +58,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
@@ -68,32 +69,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)