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

1ba3e1cc21150abe632a5b82a1a38998b33815dc init: move asmap code earlier in init process (Jon Atack)
5ba829e12e99f119df56cab422f827b9be03fe57 rpc: fix getpeerinfo RPCResult `mapped_as` type (Jon Atack)
c90b9a2399f4cead37bad39f388ce1255e123dc4 net: extract conditional to bool CNetAddr::IsHeNet (Jon Atack)
819fb5549b0d02477f47b3c40338071f37b6d885 logging: asmap logging and #include fixups (Jon Atack)
dcaf543ba0241f9219cea70b67c7b066d4c9ca9b test: add functional test for an empty, unparsable asmap (Jon Atack)
b8d0412b213df18f23bf8677ab94068c6cca9f51 config: separate the asmap finding and parsing checks (Jon Atack)
81c38a24975f34e5894efe3d1aaf45ff6a8cee4a config: enable passing -asmap an absolute file path (Jon Atack)
fbe9b024f01c29153afe494fed74b623ce3ffefa config: use default value in -asmap config (Jon Atack)
08b992675cf8d946db19b7bea747fa1085fdb2a2 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 1ba3e1cc21150abe632a5b82a1a38998b33815dc -- diff looks correct
  fanquake:
    ACK 1ba3e1cc21150abe632a5b82a1a38998b33815dc

Tree-SHA512: e9094460a597ac5597449acfe631c87b71d3ede6a12c7ae61b26d1161b3eefed8e7e25c4fb0505864cebd89300b7c4cf9378060aa9155441029315df15fa3283

8 files changed, 153 insertions, 35 deletions

diff --git a/src/addrman.cpp b/src/addrman.cpp
index 121ae4bf7e..2f8a3a0bd5 100644
--- a/src/addrman.cpp
+++ b/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) {
diff --git a/src/addrman.h b/src/addrman.h
index 5901611bee..8e82020df0 100644
--- a/src/addrman.h
+++ b/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
diff --git a/src/init.cpp b/src/init.cpp
index 1c9faec803..14d489617c 100644
--- a/src/init.cpp
+++ b/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();
diff --git a/src/netaddress.cpp b/src/netaddress.cpp
index 1cac57a817..228caf74a9 100644
--- a/src/netaddress.cpp
+++ b/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 {
diff --git a/src/netaddress.h b/src/netaddress.h
index b300b709f3..b7381c1eb4 100644
--- a/src/netaddress.h
+++ b/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;
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 482c689285..085921095e 100644
--- a/src/rpc/net.cpp
+++ b/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",
                             {
diff --git a/test/functional/feature_asmap.py b/test/functional/feature_asmap.py
new file mode 100755
index 0000000000..2c6553fbe2
--- /dev/null
+++ b/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()
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 06d939afb7..2036d20852 100755
--- a/test/functional/test_runner.py
+++ b/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',