Merge #17812: config, net, test: asmap feature refinements and functional tests

1ba3e1cc21 init: move asmap code earlier in init process (Jon Atack)
5ba829e12e rpc: fix getpeerinfo RPCResult `mapped_as` type (Jon Atack)
c90b9a2399 net: extract conditional to bool CNetAddr::IsHeNet (Jon Atack)
819fb5549b logging: asmap logging and #include fixups (Jon Atack)
dcaf543ba0 test: add functional test for an empty, unparsable asmap (Jon Atack)
b8d0412b21 config: separate the asmap finding and parsing checks (Jon Atack)
81c38a2497 config: enable passing -asmap an absolute file path (Jon Atack)
fbe9b024f0 config: use default value in -asmap config (Jon Atack)
08b992675c test: add feature_asmap functional tests (Jon Atack)

Pull request description:

  This PR builds on PR #16702 to add functional tests / sanity checks and user-facing refinements for passing `-asmap` to configure ASN-based IP bucketing in addrman. As per our review discussion in that PR, the idea here is to handle aspects like functional tests and config arg handling that can help the PR be merged while enabling the author to focus on the bucketing itself.

  - [x] add feature functional tests to verify node behaviour and debug log output when launching

    - `bitcoind` with no `-asmap` arg

    - `bitcoind -asmap=RELATIVE_FILENAME` to the unit test data skeleton asmap

    - `bitcoind -asmap` with no filename specified using the default asmap file

    - `bitcoind -asmap` with no filename specified and a missing default asmap file

  - [x] add the ability to pass absolute path filenames to the `-asmap` config arg in addition to datadir-relative path filenames as per https://github.com/bitcoin/bitcoin/pull/16702#discussion_r361300447, and add test coverage

  - [x] separate the asmap file finding and parsing checks, which allows adding tests for the case of a found but unparseable or empty asmap

  - [x] add test for an empty asmap

  - [x] various asmap fixups

  - [x] move the asmap init code earlier in the init process to provide immediate feedback when passing an  `-asmap` config arg. This speeds up the `feature_asmap` functional test from 60 to 5 seconds! Credit to Wladimir J. van der Laan for the suggestion.

ACKs for top commit:
  practicalswift:
    ACK 1ba3e1cc21 -- diff looks correct
  fanquake:
    ACK 1ba3e1cc21

Tree-SHA512: e9094460a597ac5597449acfe631c87b71d3ede6a12c7ae61b26d1161b3eefed8e7e25c4fb0505864cebd89300b7c4cf9378060aa9155441029315df15fa3283

src/addrman.cpp

@@ -6,8 +6,8 @@
 #include <addrman.h>
 
 #include <hash.h>
-#include <serialize.h>
 #include <logging.h>
+#include <serialize.h>
 
 int CAddrInfo::GetTriedBucket(const uint256& nKey, const std::vector<bool> &asmap) const
 {
@@ -15,7 +15,7 @@ int CAddrInfo::GetTriedBucket(const uint256& nKey, const std::vector<bool> &asma
     uint64_t hash2 = (CHashWriter(SER_GETHASH, 0) << nKey << GetGroup(asmap) << (hash1 % ADDRMAN_TRIED_BUCKETS_PER_GROUP)).GetCheapHash();
     int tried_bucket = hash2 % ADDRMAN_TRIED_BUCKET_COUNT;
     uint32_t mapped_as = GetMappedAS(asmap);
-    LogPrint(BCLog::NET, "IP %s mapped to AS%i belongs to tried bucket %i.\n", ToStringIP(), mapped_as, tried_bucket);
+    LogPrint(BCLog::NET, "IP %s mapped to AS%i belongs to tried bucket %i\n", ToStringIP(), mapped_as, tried_bucket);
     return tried_bucket;
 }
 
@@ -26,7 +26,7 @@ int CAddrInfo::GetNewBucket(const uint256& nKey, const CNetAddr& src, const std:
     uint64_t hash2 = (CHashWriter(SER_GETHASH, 0) << nKey << vchSourceGroupKey << (hash1 % ADDRMAN_NEW_BUCKETS_PER_SOURCE_GROUP)).GetCheapHash();
     int new_bucket = hash2 % ADDRMAN_NEW_BUCKET_COUNT;
     uint32_t mapped_as = GetMappedAS(asmap);
-    LogPrint(BCLog::NET, "IP %s mapped to AS%i belongs to new bucket %i.\n", ToStringIP(), mapped_as, new_bucket);
+    LogPrint(BCLog::NET, "IP %s mapped to AS%i belongs to new bucket %i\n", ToStringIP(), mapped_as, new_bucket);
     return new_bucket;
 }
 
@@ -630,12 +630,12 @@ std::vector<bool> CAddrMan::DecodeAsmap(fs::path path)
     FILE *filestr = fsbridge::fopen(path, "rb");
     CAutoFile file(filestr, SER_DISK, CLIENT_VERSION);
     if (file.IsNull()) {
-        LogPrintf("Failed to open asmap file from disk.\n");
+        LogPrintf("Failed to open asmap file from disk\n");
         return bits;
     }
     fseek(filestr, 0, SEEK_END);
     int length = ftell(filestr);
-    LogPrintf("Opened asmap file %s (%d bytes) from disk.\n", path, length);
+    LogPrintf("Opened asmap file %s (%d bytes) from disk\n", path, length);
     fseek(filestr, 0, SEEK_SET);
     char cur_byte;
     for (int i = 0; i < length; ++i) {

src/addrman.h

@@ -6,23 +6,22 @@
 #ifndef BITCOIN_ADDRMAN_H
 #define BITCOIN_ADDRMAN_H
 
+#include <clientversion.h>
 #include <netaddress.h>
 #include <protocol.h>
 #include <random.h>
 #include <sync.h>
 #include <timedata.h>
 #include <util/system.h>
-#include <clientversion.h>
 
+#include <fs.h>
+#include <hash.h>
+#include <iostream>
 #include <map>
 #include <set>
 #include <stdint.h>
-#include <vector>
-#include <iostream>
 #include <streams.h>
-#include <fs.h>
-#include <hash.h>
-
+#include <vector>
 
 /**
  * Extended statistics about a CAddress

src/init.cpp

@@ -47,11 +47,11 @@
 #include <txdb.h>
 #include <txmempool.h>
 #include <ui_interface.h>
+#include <util/asmap.h>
 #include <util/moneystr.h>
 #include <util/system.h>
 #include <util/threadnames.h>
 #include <util/translation.h>
-#include <util/asmap.h>
 #include <validation.h>
 #include <hash.h>
 
@@ -408,6 +408,7 @@ void SetupServerArgs()
                  ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 
     gArgs.AddArg("-addnode=<ip>", "Add a node to connect to and attempt to keep the connection open (see the `addnode` RPC command help for more info). This option can be specified multiple times to add multiple nodes.", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
+    gArgs.AddArg("-asmap=<file>", strprintf("Specify asn mapping used for bucketing of the peers (default: %s). Relative paths will be prefixed by the net-specific datadir location.", DEFAULT_ASMAP_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     gArgs.AddArg("-banscore=<n>", strprintf("Threshold for disconnecting misbehaving peers (default: %u)", DEFAULT_BANSCORE_THRESHOLD), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     gArgs.AddArg("-bantime=<n>", strprintf("Number of seconds to keep misbehaving peers from reconnecting (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     gArgs.AddArg("-bind=<addr>", "Bind to given address and always listen on it. Use [host]:port notation for IPv6", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
@@ -436,7 +437,6 @@ void SetupServerArgs()
     gArgs.AddArg("-peertimeout=<n>", strprintf("Specify p2p connection timeout in seconds. This option determines the amount of time a peer may be inactive before the connection to it is dropped. (minimum: 1, default: %d)", DEFAULT_PEER_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
     gArgs.AddArg("-torcontrol=<ip>:<port>", strprintf("Tor control port to use if onion listening enabled (default: %s)", DEFAULT_TOR_CONTROL), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     gArgs.AddArg("-torpassword=<pass>", "Tor control port password (default: empty)", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE, OptionsCategory::CONNECTION);
-    gArgs.AddArg("-asmap=<file>", "Specify asn mapping used for bucketing of the peers. Path should be relative to the -datadir path.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 #ifdef USE_UPNP
 #if USE_UPNP
     gArgs.AddArg("-upnp", "Use UPnP to map the listening port (default: 1 when listening and no -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
@@ -1418,6 +1418,31 @@ bool AppInitMain(NodeContext& node)
             return InitError(ResolveErrMsg("externalip", strAddr));
     }
 
+    // Read asmap file if configured
+    if (gArgs.IsArgSet("-asmap")) {
+        fs::path asmap_path = fs::path(gArgs.GetArg("-asmap", ""));
+        if (asmap_path.empty()) {
+            asmap_path = DEFAULT_ASMAP_FILENAME;
+        }
+        if (!asmap_path.is_absolute()) {
+            asmap_path = GetDataDir() / asmap_path;
+        }
+        if (!fs::exists(asmap_path)) {
+            InitError(strprintf(_("Could not find asmap file %s").translated, asmap_path));
+            return false;
+        }
+        std::vector<bool> asmap = CAddrMan::DecodeAsmap(asmap_path);
+        if (asmap.size() == 0) {
+            InitError(strprintf(_("Could not parse asmap file %s").translated, asmap_path));
+            return false;
+        }
+        const uint256 asmap_version = SerializeHash(asmap);
+        node.connman->SetAsmap(std::move(asmap));
+        LogPrintf("Using asmap version %s for IP bucketing\n", asmap_version.ToString());
+    } else {
+        LogPrintf("Using /16 prefix for IP bucketing\n");
+    }
+
 #if ENABLE_ZMQ
     g_zmq_notification_interface = CZMQNotificationInterface::Create();
 
@@ -1825,25 +1850,6 @@ bool AppInitMain(NodeContext& node)
         return false;
     }
 
-    // Read asmap file if configured
-    if (gArgs.IsArgSet("-asmap")) {
-        std::string asmap_file = gArgs.GetArg("-asmap", "");
-        if (asmap_file.empty()) {
-            asmap_file = DEFAULT_ASMAP_FILENAME;
-        }
-        const fs::path asmap_path = GetDataDir() / asmap_file;
-        std::vector<bool> asmap = CAddrMan::DecodeAsmap(asmap_path);
-        if (asmap.size() == 0) {
-            InitError(strprintf(_("Could not find or parse specified asmap: '%s'").translated, asmap_path));
-            return false;
-        }
-        const uint256 asmap_version = SerializeHash(asmap);
-        node.connman->SetAsmap(std::move(asmap));
-        LogPrintf("Using asmap version %s for IP bucketing.\n", asmap_version.ToString());
-    } else {
-        LogPrintf("Using /16 prefix for IP bucketing.\n");
-    }
-
     // ********************************************************* Step 13: finished
 
     SetRPCWarmupFinished();

src/netaddress.cpp

@@ -210,6 +210,11 @@ bool CNetAddr::IsRFC7343() const
     return (GetByte(15) == 0x20 && GetByte(14) == 0x01 && GetByte(13) == 0x00 && (GetByte(12) & 0xF0) == 0x20);
 }
 
+bool CNetAddr::IsHeNet() const
+{
+    return (GetByte(15) == 0x20 && GetByte(14) == 0x01 && GetByte(13) == 0x04 && GetByte(12) == 0x70);
+}
+
 /**
  * @returns Whether or not this is a dummy address that maps an onion address
  *          into IPv6.
@@ -516,7 +521,7 @@ std::vector<unsigned char> CNetAddr::GetGroup(const std::vector<bool> &asmap) co
     } else if (IsTor()) {
         nStartByte = 6;
         nBits = 4;
-    } else if (GetByte(15) == 0x20 && GetByte(14) == 0x01 && GetByte(13) == 0x04 && GetByte(12) == 0x70) {
+    } else if (IsHeNet()) {
         // for he.net, use /36 groups
         nBits = 36;
     } else {

src/netaddress.h

@@ -45,7 +45,6 @@ class CNetAddr
          */
         void SetRaw(Network network, const uint8_t *data);
 
-    public:
         bool SetInternal(const std::string& name);
 
         bool SetSpecial(const std::string &strName); // for Tor addresses
@@ -66,6 +65,7 @@ class CNetAddr
         bool IsRFC4862() const; // IPv6 autoconfig (FE80::/64)
         bool IsRFC6052() const; // IPv6 well-known prefix for IPv4-embedded address (64:FF9B::/96)
         bool IsRFC6145() const; // IPv6 IPv4-translated address (::FFFF:0:0:0/96) (actually defined in RFC2765)
+        bool IsHeNet() const;   // IPv6 Hurricane Electric - https://he.net (2001:0470::/36)
         bool IsTor() const;
         bool IsLocal() const;
         bool IsRoutable() const;

src/rpc/net.cpp

@@ -86,7 +86,8 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
                             {RPCResult::Type::STR, "addr", "(host:port) The IP address and port of the peer"},
                             {RPCResult::Type::STR, "addrbind", "(ip:port) Bind address of the connection to the peer"},
                             {RPCResult::Type::STR, "addrlocal", "(ip:port) Local address as reported by the peer"},
-                            {RPCResult::Type::STR, "mapped_as", "The AS in the BGP route to the peer used for diversifying peer selection"},
+                            {RPCResult::Type::NUM, "mapped_as", "The AS in the BGP route to the peer used for diversifying\n"
+                                                                "peer selection (only available if the asmap config flag is set)"},
                             {RPCResult::Type::STR_HEX, "services", "The services offered"},
                             {RPCResult::Type::ARR, "servicesnames", "the services offered, in human-readable form",
                             {

test/functional/feature_asmap.py

@@ -0,0 +1,106 @@
+#!/usr/bin/env python3
+# Copyright (c) 2020 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test asmap config argument for ASN-based IP bucketing.
+
+Verify node behaviour and debug log when launching bitcoind in these cases:
+
+1. `bitcoind` with no -asmap arg, using /16 prefix for IP bucketing
+
+2. `bitcoind -asmap=<absolute path>`, using the unit test skeleton asmap
+
+3. `bitcoind -asmap=<relative path>`, using the unit test skeleton asmap
+
+4. `bitcoind -asmap/-asmap=` with no file specified, using the default asmap
+
+5. `bitcoind -asmap` with no file specified and a missing default asmap file
+
+6. `bitcoind -asmap` with an empty (unparsable) default asmap file
+
+The tests are order-independent.
+
+"""
+import os
+import shutil
+
+from test_framework.test_framework import BitcoinTestFramework
+
+DEFAULT_ASMAP_FILENAME = 'ip_asn.map' # defined in src/init.cpp
+ASMAP = '../../src/test/data/asmap.raw' # path to unit test skeleton asmap
+VERSION = 'fec61fa21a9f46f3b17bdcd660d7f4cd90b966aad3aec593c99b35f0aca15853'
+
+def expected_messages(filename):
+    return ['Opened asmap file "{}" (59 bytes) from disk'.format(filename),
+            'Using asmap version {} for IP bucketing'.format(VERSION)]
+
+class AsmapTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = False
+        self.num_nodes = 1
+
+    def test_without_asmap_arg(self):
+        self.log.info('Test bitcoind with no -asmap arg passed')
+        self.stop_node(0)
+        with self.node.assert_debug_log(['Using /16 prefix for IP bucketing']):
+            self.start_node(0)
+
+    def test_asmap_with_absolute_path(self):
+        self.log.info('Test bitcoind -asmap=<absolute path>')
+        self.stop_node(0)
+        filename = os.path.join(self.datadir, 'my-map-file.map')
+        shutil.copyfile(self.asmap_raw, filename)
+        with self.node.assert_debug_log(expected_messages(filename)):
+            self.start_node(0, ['-asmap={}'.format(filename)])
+        os.remove(filename)
+
+    def test_asmap_with_relative_path(self):
+        self.log.info('Test bitcoind -asmap=<relative path>')
+        self.stop_node(0)
+        name = 'ASN_map'
+        filename = os.path.join(self.datadir, name)
+        shutil.copyfile(self.asmap_raw, filename)
+        with self.node.assert_debug_log(expected_messages(filename)):
+            self.start_node(0, ['-asmap={}'.format(name)])
+        os.remove(filename)
+
+    def test_default_asmap(self):
+        shutil.copyfile(self.asmap_raw, self.default_asmap)
+        for arg in ['-asmap', '-asmap=']:
+            self.log.info('Test bitcoind {} (using default map file)'.format(arg))
+            self.stop_node(0)
+            with self.node.assert_debug_log(expected_messages(self.default_asmap)):
+                self.start_node(0, [arg])
+        os.remove(self.default_asmap)
+
+    def test_default_asmap_with_missing_file(self):
+        self.log.info('Test bitcoind -asmap with missing default map file')
+        self.stop_node(0)
+        msg = "Error: Could not find asmap file \"{}\"".format(self.default_asmap)
+        self.node.assert_start_raises_init_error(extra_args=['-asmap'], expected_msg=msg)
+
+    def test_empty_asmap(self):
+        self.log.info('Test bitcoind -asmap with empty map file')
+        self.stop_node(0)
+        with open(self.default_asmap, "w", encoding="utf-8") as f:
+            f.write("")
+        msg = "Error: Could not parse asmap file \"{}\"".format(self.default_asmap)
+        self.node.assert_start_raises_init_error(extra_args=['-asmap'], expected_msg=msg)
+        os.remove(self.default_asmap)
+
+    def run_test(self):
+        self.node = self.nodes[0]
+        self.datadir = os.path.join(self.node.datadir, self.chain)
+        self.default_asmap = os.path.join(self.datadir, DEFAULT_ASMAP_FILENAME)
+        self.asmap_raw = os.path.join(os.path.dirname(os.path.realpath(__file__)), ASMAP)
+
+        self.test_without_asmap_arg()
+        self.test_asmap_with_absolute_path()
+        self.test_asmap_with_relative_path()
+        self.test_default_asmap()
+        self.test_default_asmap_with_missing_file()
+        self.test_empty_asmap()
+
+
+if __name__ == '__main__':
+    AsmapTest().main()

test/functional/test_runner.py

@@ -206,6 +206,7 @@ BASE_SCRIPTS = [
     'p2p_dos_header_tree.py',
     'p2p_unrequested_blocks.py',
     'feature_includeconf.py',
+    'feature_asmap.py',
     'rpc_deriveaddresses.py',
     'rpc_deriveaddresses.py --usecli',
     'rpc_scantxoutset.py',