Merge bitcoin/bitcoin#33770: init: Require explicit -asmap filename

288b8c3 doc: Drop (default: none) from -i2psam description (Ryan Ofsky)
f6ec351 init: Require explicit -asmap filename (Ryan Ofsky)

Pull request description:

  Currently, if `-asmap` is specified without a filename bitcoind tries to load `ip_asn.map` data file.

  This change now requires `-asmap=ip_asn.map` or another filename to be specified explicitly.

  The change is intended to make behavior of the option explicit and avoid confusion reported bitcoin/bitcoin#33386 where documentation specifies a default file which is not actually loaded by default. It was originally implemented in
  bitcoin/bitcoin#33631 (comment) and various alternatives are discussed there.

ACKs for top commit:
  brunoerg:
    reACK 288b8c3
  fjahr:
    re-ACK 288b8c3
  vostrnad:
    utACK 288b8c3
  achow101:
    ACK 288b8c3

Tree-SHA512: 11a38a03892a58d6ccc1505cfbf915f58a86df9891761d89dc54b92d40593ee3cbb2d7c7bdbb922b871b3529072ef7f34cc98393aff6e8f0633b56352315b27c

Author: achow101

doc/files.md

@@ -66,7 +66,6 @@ Subdirectory       | File(s)               | Description
 `./`               | `debug.log`           | Contains debug information and general logging generated by `bitcoind` or `bitcoin-qt`; can be specified by `-debuglogfile` option
 `./`               | `fee_estimates.dat`   | Stores statistics used to estimate minimum transaction fees required for confirmation
 `./`               | `guisettings.ini.bak` | Backup of former [GUI settings](#gui-settings) after `-resetguisettings` option is used
-`./`               | `ip_asn.map`          | IP addresses to Autonomous System Numbers (ASNs) mapping used for bucketing of the peers; path can be specified with the `-asmap` option
 `./`               | `mempool.dat`         | Dump of the mempool's transactions
 `./`               | `onion_v3_private_key` | Cached Tor onion service private key for `-listenonion` option
 `./`               | `i2p_private_key`     | Private key that corresponds to our I2P address. When `-i2psam=` is specified the contents of this file is used to identify ourselves for making outgoing connections to I2P peers and possibly accepting incoming ones. Automatically generated if it does not exist.

doc/release-notes-33770.md

@@ -0,0 +1,4 @@
+`-asmap` requires explicit filename
+-----------------------------------
+
+In previous releases, if `-asmap` was specified without a filename, this would try to load an `ip_asn.map` data file. Now loading an asmap file requires an explicit filename like `-asmap=ip_asn.map`. This change was made to make the option easier to understand, because it was confusing for there to be a default filename not actually loaded by default (https://github.com/bitcoin/bitcoin/issues/33386). Also this change makes the option more future-proof, because in upcoming releases, specifying `-asmap` will load embedded asmap data instead of an external file (https://github.com/bitcoin/bitcoin/pull/28792).

src/init.cpp

@@ -158,7 +158,6 @@ static constexpr bool DEFAULT_STOPAFTERBLOCKIMPORT{false};
 #endif
 
 static constexpr int MIN_CORE_FDS = MIN_LEVELDB_FDS + NUM_FDS_MESSAGE_CAPTURE;
-static const char* DEFAULT_ASMAP_FILENAME="ip_asn.map";
 
 /**
  * The PID file facilities.
@@ -530,7 +529,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
                  ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 
     argsman.AddArg("-addnode=<ip>", strprintf("Add a node to connect to and attempt to keep the connection open (see the addnode RPC help for more info). This option can be specified multiple times to add multiple nodes; connections are limited to %u at a time and are counted separately from the -maxconnections limit.", MAX_ADDNODE_CONNECTIONS), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
-    argsman.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);
+    argsman.AddArg("-asmap=<file>", "Specify asn mapping used for bucketing of the peers. Relative paths will be prefixed by the net-specific datadir location.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-bantime=<n>", strprintf("Default duration (in seconds) of manually configured bans (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-bind=<addr>[:<port>][=onion]", strprintf("Bind to given address and always listen on it (default: 0.0.0.0). Use [host]:port notation for IPv6. Append =onion to tag any incoming connections to that address and port as incoming Tor connections (default: 127.0.0.1:%u=onion, testnet3: 127.0.0.1:%u=onion, testnet4: 127.0.0.1:%u=onion, signet: 127.0.0.1:%u=onion, regtest: 127.0.0.1:%u=onion)", defaultChainParams->GetDefaultPort() + 1, testnetChainParams->GetDefaultPort() + 1, testnet4ChainParams->GetDefaultPort() + 1, signetChainParams->GetDefaultPort() + 1, regtestChainParams->GetDefaultPort() + 1), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
     argsman.AddArg("-cjdnsreachable", "If set, then this host is configured for CJDNS (connecting to fc00::/8 addresses would lead us to the CJDNS network, see doc/cjdns.md) (default: 0)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
@@ -552,7 +551,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
 #else
     argsman.AddArg("-onion=<ip:port>", "Use separate SOCKS5 proxy to reach peers via Tor onion services, set -noonion to disable (default: -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 #endif
-    argsman.AddArg("-i2psam=<ip:port>", "I2P SAM proxy to reach I2P peers and accept I2P connections (default: none)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
+    argsman.AddArg("-i2psam=<ip:port>", "I2P SAM proxy to reach I2P peers and accept I2P connections", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-i2pacceptincoming", strprintf("Whether to accept inbound I2P connections (default: %i). Ignored if -i2psam is not set. Listening for inbound I2P connections is done through the SAM proxy, not by binding to a local address and port.", DEFAULT_I2P_ACCEPT_INCOMING), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-onlynet=<net>", "Make automatic outbound connections only to network <net> (" + Join(GetNetworkNames(), ", ") + "). Inbound and manual connections are not affected by this option. It can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-v2transport", strprintf("Support v2 transport (default: %u)", DEFAULT_V2_TRANSPORT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
@@ -1541,7 +1540,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         // Read asmap file if configured
         std::vector<bool> asmap;
         if (args.IsArgSet("-asmap") && !args.IsArgNegated("-asmap")) {
-            fs::path asmap_path = args.GetPathArg("-asmap", DEFAULT_ASMAP_FILENAME);
+            fs::path asmap_path = args.GetPathArg("-asmap");
+            if (asmap_path.empty()) {
+                InitError(_("-asmap requires a file path. Use -asmap=<file>."));
+                return false;
+            }
             if (!asmap_path.is_absolute()) {
                 asmap_path = args.GetDataDirNet() / asmap_path;
             }

test/functional/feature_asmap.py

@@ -4,21 +4,9 @@
 # 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` restart with an addrman containing new and tried entries
-
-6. `bitcoind -asmap` with no file specified and a missing default asmap file
-
-7. `bitcoind -asmap` with an empty (unparsable) default asmap file
+Verify node behaviour and debug log when launching bitcoind with different
+`-asmap` and `-noasmap` arg values, including absolute and relative paths, and
+with missing and unparseable files.
 
 The tests are order-independent.
 
@@ -29,7 +17,6 @@
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal
 
-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'
 
@@ -79,52 +66,48 @@ def test_asmap_with_relative_path(self):
             self.start_node(0, [f'-asmap={name}'])
         os.remove(filename)
 
-    def test_default_asmap(self):
-        shutil.copyfile(self.asmap_raw, self.default_asmap)
+    def test_unspecified_asmap(self):
+        msg = "Error: -asmap requires a file path. Use -asmap=<file>."
         for arg in ['-asmap', '-asmap=']:
-            self.log.info(f'Test bitcoind {arg} (using default map file)')
+            self.log.info(f'Test bitcoind {arg} (and no filename specified)')
             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)
+            self.node.assert_start_raises_init_error(extra_args=[arg], expected_msg=msg)
 
     def test_asmap_interaction_with_addrman_containing_entries(self):
         self.log.info("Test bitcoind -asmap restart with addrman containing new and tried entries")
         self.stop_node(0)
-        shutil.copyfile(self.asmap_raw, self.default_asmap)
-        self.start_node(0, ["-asmap", "-checkaddrman=1", "-test=addrman"])
+        self.start_node(0, [f"-asmap={self.asmap_raw}", "-checkaddrman=1", "-test=addrman"])
         self.fill_addrman(node_id=0)
-        self.restart_node(0, ["-asmap", "-checkaddrman=1", "-test=addrman"])
+        self.restart_node(0, [f"-asmap={self.asmap_raw}", "-checkaddrman=1", "-test=addrman"])
         with self.node.assert_debug_log(
             expected_msgs=[
                 "CheckAddrman: new 2, tried 2, total 4 started",
                 "CheckAddrman: completed",
             ]
         ):
             self.node.getnodeaddresses()  # getnodeaddresses re-runs the addrman checks
-        os.remove(self.default_asmap)
 
-    def test_default_asmap_with_missing_file(self):
-        self.log.info('Test bitcoind -asmap with missing default map file')
+    def test_asmap_with_missing_file(self):
+        self.log.info('Test bitcoind -asmap with missing map file')
         self.stop_node(0)
-        msg = f"Error: Could not find asmap file \"{self.default_asmap}\""
-        self.node.assert_start_raises_init_error(extra_args=['-asmap'], expected_msg=msg)
+        msg = f"Error: Could not find asmap file \"{self.datadir}{os.sep}missing\""
+        self.node.assert_start_raises_init_error(extra_args=['-asmap=missing'], 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:
+        empty_asmap = os.path.join(self.datadir, "ip_asn.map")
+        with open(empty_asmap, "w", encoding="utf-8") as f:
             f.write("")
-        msg = f"Error: Could not parse asmap file \"{self.default_asmap}\""
-        self.node.assert_start_raises_init_error(extra_args=['-asmap'], expected_msg=msg)
-        os.remove(self.default_asmap)
+        msg = f"Error: Could not parse asmap file \"{empty_asmap}\""
+        self.node.assert_start_raises_init_error(extra_args=[f'-asmap={empty_asmap}'], expected_msg=msg)
+        os.remove(empty_asmap)
 
     def test_asmap_health_check(self):
         self.log.info('Test bitcoind -asmap logs ASMap Health Check with basic stats')
-        shutil.copyfile(self.asmap_raw, self.default_asmap)
         msg = "ASMap Health Check: 4 clearnet peers are mapped to 3 ASNs with 0 peers being unmapped"
         with self.node.assert_debug_log(expected_msgs=[msg]):
-            self.start_node(0, extra_args=['-asmap'])
+            self.start_node(0, extra_args=[f'-asmap={self.asmap_raw}'])
         raw_addrman = self.node.getrawaddrman()
         asns = []
         for _, entries in raw_addrman.items():
@@ -133,22 +116,20 @@ def test_asmap_health_check(self):
                 if asn not in asns:
                     asns.append(asn)
         assert_equal(len(asns), 3)
-        os.remove(self.default_asmap)
 
     def run_test(self):
         self.node = self.nodes[0]
         self.datadir = self.node.chain_path
-        self.default_asmap = os.path.join(self.datadir, DEFAULT_ASMAP_FILENAME)
         base_dir = self.config["environment"]["SRCDIR"]
         self.asmap_raw = os.path.join(base_dir, ASMAP)
 
         self.test_without_asmap_arg()
         self.test_noasmap_arg()
         self.test_asmap_with_absolute_path()
         self.test_asmap_with_relative_path()
-        self.test_default_asmap()
+        self.test_unspecified_asmap()
         self.test_asmap_interaction_with_addrman_containing_entries()
-        self.test_default_asmap_with_missing_file()
+        self.test_asmap_with_missing_file()
         self.test_empty_asmap()
         self.test_asmap_health_check()