init: Split file path handling out of -asmap option #33631

1. 
   fjahr commented at 10:25 pm on October 14, 2025: contributor

   Note: There is an alternative change that leaves the behavior of -asmap as is but makes slight improvements and adds better documentation: #33632 Please indicate with your conceptual review which option you prefer, thanks!

   This change adds a new option -asmapfile which allows to set the asmap file, while -asmap now is simply a bool option. Also fixes the asmap functional test docs, since then had become out of sync with the actual content of the test.

   The current handling of -asmap is a bit confusing and atypical of our usual options behavior. The -asmap option can be used as a boolean which turns on the feature and looks for the file in the default location. But it can be alternatively be used to pass a path to a file which is not in the default location, which also turns on the feature implicitly.

   See #33386 for further discussion. Closes #33386

2. init: Split file path handling out of -asmap

   This adds a new option -asmapfile which allows to set the asmap file, while -asmap now is simply a bool option.

   e46d057991
3. 
   DrahtBot commented at 10:25 pm on October 14, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33631.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	laanwj

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #33770 (init: Require explicit -asmap filename by ryanofsky)
   • #28792 (Embed default ASMap as binary dump header file by fjahr)
   • #17783 (common: Disallow calling IsArgSet() on ALLOW_LIST options by ryanofsky)
   • #17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
   • #17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
   • #17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

4. fjahr renamed this:
   init: Split file path handling out of -asmap
   init: Split file path handling out of -asmap option
   on Oct 14, 2025
5. 
   ryanofsky commented at 10:55 am on October 16, 2025: contributor

   Note: There is an alternative change that leaves the behavior of -asmap as is but makes slight improvements and adds better documentation: #33632 Please indicate with your conceptual review which option you prefer, thanks!

   IMO #33631 and #33632 are both reasonable ways of patching up a confusing UI and making it slightly better, and either would be an improvement over current behavior.

   But neither of these PRs seem to solve the fundamental problem here: that if a user goes through the trouble of saving ip_asn.map file in their data directory, the software treats this file differently from other data files and ignores it. This seems like an obvious footgun, made worse by the fact that if the file is present and software chooses to ignore it, it doesn’t warn about the unused file. These choices seem likely to result in situations where a user thinks they  have provided asmap data but it just sits there unused. It seems to me like a much more natural solution would be to use the data by default if it is present:

    0--- a/src/init.cpp
    1+++ b/src/init.cpp
    2@@ -1547,15 +1547,20 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
    3 
    4         // Read asmap file if configured
    5         std::vector<bool> asmap;
    6-        if (args.IsArgSet("-asmap") && !args.IsArgNegated("-asmap")) {
    7-            fs::path asmap_path = args.GetPathArg("-asmap", DEFAULT_ASMAP_FILENAME);
    8+        fs::path asmap_path = args.GetPathArg("-asmap", DEFAULT_ASMAP_FILENAME);
    9+        if (!asmap_path.empty()) {
   10             if (!asmap_path.is_absolute()) {
   11                 asmap_path = args.GetDataDirNet() / asmap_path;
   12             }
   13-            if (!fs::exists(asmap_path)) {
   14+            if (!args.IsArgSet("-asmap") && !fs::exists(asmap_path)) {
   15+                // It is ok for asmap file not to exist if not specified.
   16+                asmap_path.clear();
   17+            } else if (!fs::exists(asmap_path)) {
   18                 InitError(strprintf(_("Could not find asmap file %s"), fs::quoted(fs::PathToString(asmap_path))));
   19                 return false;
   20             }
   21+        }
   22+        if (!asmap_path.empty()) {
   23             asmap = DecodeAsmap(asmap_path);
   24             if (asmap.size() == 0) {
   25                 InitError(strprintf(_("Could not parse asmap file %s"), fs::quoted(fs::PathToString(asmap_path))));
   26--- a/test/functional/feature_asmap.py
   27+++ b/test/functional/feature_asmap.py
   28@@ -49,7 +49,14 @@ class AsmapTest(BitcoinTestFramework):
   29             self.nodes[node_id].addpeeraddress(address=f"101.{addr}.0.0", tried=tried, port=8333)
   30 
   31     def test_without_asmap_arg(self):
   32-        self.log.info('Test bitcoind with no -asmap arg passed')
   33+        self.log.info(f'Test bitcoind with no -asmap arg passed, with {DEFAULT_ASMAP_FILENAME} present')
   34+        shutil.copyfile(self.asmap_raw, self.default_asmap)
   35+        self.stop_node(0)
   36+        with self.node.assert_debug_log(expected_messages(self.default_asmap)):
   37+            self.start_node(0)
   38+        os.remove(self.default_asmap)
   39+
   40+        self.log.info(f'Test bitcoind with no -asmap arg passed, without {DEFAULT_ASMAP_FILENAME} present')
   41         self.stop_node(0)
   42         with self.node.assert_debug_log(['Using /16 prefix for IP bucketing']):
   43             self.start_node(0)
   

   Alternately, if it is unsafe to load this data by default unless the node is explicitly configured to load it, then it is unnecessarily confusing to define a default filename that is not used by default. It would make more sense to make the default value empty:

     0--- a/src/init.cpp
     1+++ b/src/init.cpp
     2@@ -158,7 +158,6 @@ static constexpr bool DEFAULT_STOPAFTERBLOCKIMPORT{false};
     3 #endif
     4 
     5 static constexpr int MIN_CORE_FDS = MIN_LEVELDB_FDS + NUM_FDS_MESSAGE_CAPTURE;
     6-static const char* DEFAULT_ASMAP_FILENAME="ip_asn.map";
     7 
     8 /**
     9  * The PID file facilities.
    10@@ -532,7 +531,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
    11                  ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
    12 
    13     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);
    14-    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);
    15+    argsman.AddArg("-asmap=<file>", strprintf("Specify asn mapping used for bucketing of the peers (default: none). Relative paths will be prefixed by the net-specific datadir location."), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
    16     argsman.AddArg("-bantime=<n>", strprintf("Default duration (in seconds) of manually configured bans (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
    17     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);
    18     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);
    19@@ -1548,11 +1547,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
    20         // Read asmap file if configured
    21         std::vector<bool> asmap;
    22         if (args.IsArgSet("-asmap") && !args.IsArgNegated("-asmap")) {
    23-            fs::path asmap_path = args.GetPathArg("-asmap", DEFAULT_ASMAP_FILENAME);
    24+            fs::path asmap_path = args.GetPathArg("-asmap");
    25             if (!asmap_path.is_absolute()) {
    26                 asmap_path = args.GetDataDirNet() / asmap_path;
    27             }
    28-            if (!fs::exists(asmap_path)) {
    29+            if (!fs::is_regular_file(asmap_path)) {
    30                 InitError(strprintf(_("Could not find asmap file %s"), fs::quoted(fs::PathToString(asmap_path))));
    31                 return false;
    32             }
    33--- a/test/functional/feature_asmap.py
    34+++ b/test/functional/feature_asmap.py
    35@@ -29,7 +29,6 @@ import shutil
    36 from test_framework.test_framework import BitcoinTestFramework
    37 from test_framework.util import assert_equal
    38 
    39-DEFAULT_ASMAP_FILENAME = 'ip_asn.map' # defined in src/init.cpp
    40 ASMAP = 'src/test/data/asmap.raw' # path to unit test skeleton asmap
    41 VERSION = 'fec61fa21a9f46f3b17bdcd660d7f4cd90b966aad3aec593c99b35f0aca15853'
    42 
    43@@ -80,21 +79,18 @@ class AsmapTest(BitcoinTestFramework):
    44         os.remove(filename)
    45 
    46     def test_default_asmap(self):
    47-        shutil.copyfile(self.asmap_raw, self.default_asmap)
    48+        msg = f"Error: Could not find asmap file \"{self.datadir}{os.sep}\""
    49         for arg in ['-asmap', '-asmap=']:
    50             self.log.info(f'Test bitcoind {arg} (using default map file)')
    51             self.stop_node(0)
    52-            with self.node.assert_debug_log(expected_messages(self.default_asmap)):
    53-                self.start_node(0, [arg])
    54-        os.remove(self.default_asmap)
    55+            self.node.assert_start_raises_init_error(extra_args=[arg], expected_msg=msg)
    56 
    57     def test_asmap_interaction_with_addrman_containing_entries(self):
    58         self.log.info("Test bitcoind -asmap restart with addrman containing new and tried entries")
    59         self.stop_node(0)
    60-        shutil.copyfile(self.asmap_raw, self.default_asmap)
    61-        self.start_node(0, ["-asmap", "-checkaddrman=1", "-test=addrman"])
    62+        self.start_node(0, [f"-asmap={self.asmap_raw}", "-checkaddrman=1", "-test=addrman"])
    63         self.fill_addrman(node_id=0)
    64-        self.restart_node(0, ["-asmap", "-checkaddrman=1", "-test=addrman"])
    65+        self.restart_node(0, [f"-asmap={self.asmap_raw}", "-checkaddrman=1", "-test=addrman"])
    66         with self.node.assert_debug_log(
    67             expected_msgs=[
    68                 "CheckAddrman: new 2, tried 2, total 4 started",
    69@@ -102,29 +98,29 @@ class AsmapTest(BitcoinTestFramework):
    70             ]
    71         ):
    72             self.node.getnodeaddresses()  # getnodeaddresses re-runs the addrman checks
    73-        os.remove(self.default_asmap)
    74 
    75     def test_default_asmap_with_missing_file(self):
    76         self.log.info('Test bitcoind -asmap with missing default map file')
    77         self.stop_node(0)
    78-        msg = f"Error: Could not find asmap file \"{self.default_asmap}\""
    79-        self.node.assert_start_raises_init_error(extra_args=['-asmap'], expected_msg=msg)
    80+        msg = f"Error: Could not find asmap file \"{self.datadir}{os.sep}missing\""
    81+        self.node.assert_start_raises_init_error(extra_args=[f'-asmap=missing'], expected_msg=msg)
    82 
    83     def test_empty_asmap(self):
    84         self.log.info('Test bitcoind -asmap with empty map file')
    85         self.stop_node(0)
    86-        with open(self.default_asmap, "w", encoding="utf-8") as f:
    87+
    88+        empty_asmap = os.path.join(self.datadir, "ip_asn.map")
    89+        with open(empty_asmap, "w", encoding="utf-8") as f:
    90             f.write("")
    91-        msg = f"Error: Could not parse asmap file \"{self.default_asmap}\""
    92-        self.node.assert_start_raises_init_error(extra_args=['-asmap'], expected_msg=msg)
    93-        os.remove(self.default_asmap)
    94+        msg = f"Error: Could not parse asmap file \"{empty_asmap}\""
    95+        self.node.assert_start_raises_init_error(extra_args=[f'-asmap={empty_asmap}'], expected_msg=msg)
    96+        os.remove(empty_asmap)
    97 
    98     def test_asmap_health_check(self):
    99         self.log.info('Test bitcoind -asmap logs ASMap Health Check with basic stats')
   100-        shutil.copyfile(self.asmap_raw, self.default_asmap)
   101         msg = "ASMap Health Check: 4 clearnet peers are mapped to 3 ASNs with 0 peers being unmapped"
   102         with self.node.assert_debug_log(expected_msgs=[msg]):
   103-            self.start_node(0, extra_args=['-asmap'])
   104+            self.start_node(0, extra_args=[f'-asmap={self.asmap_raw}'])
   105         raw_addrman = self.node.getrawaddrman()
   106         asns = []
   107         for _, entries in raw_addrman.items():
   108@@ -133,12 +129,10 @@ class AsmapTest(BitcoinTestFramework):
   109                 if asn not in asns:
   110                     asns.append(asn)
   111         assert_equal(len(asns), 3)
   112-        os.remove(self.default_asmap)
   113 
   114     def run_test(self):
   115         self.node = self.nodes[0]
   116         self.datadir = self.node.chain_path
   117-        self.default_asmap = os.path.join(self.datadir, DEFAULT_ASMAP_FILENAME)
   118         base_dir = self.config["environment"]["SRCDIR"]
   119         self.asmap_raw = os.path.join(base_dir, ASMAP)
   120 
   

6. in src/init.cpp:536 in e46d057991

   531@@ -532,7 +532,8 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
   532                  ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
   533 
   534     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);
   535-    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);
   536+    argsman.AddArg("-asmap", "Use AS mapping for IP bucketing (default: 0)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
   537+    argsman.AddArg("-asmapfile=<file>", strprintf("Specify asn mapping file 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);
   

   ---

   ---

   

   laanwj commented at 11:37 am on October 22, 2025:

   nit: AS mapping versus “asn mapping”, let’s settle on one spelling.
7. 
   laanwj commented at 11:39 am on October 22, 2025: member

   Concept ACK. Splitting it to a separate filename option makes sense. imo it’s better to avoid overloading the type (boolean or path) of options and introducing ambiguity. This is conceptually simpler for the user and simplifies the code.
8. 
   ryanofsky commented at 2:48 pm on October 22, 2025: contributor

   @laanwj do you agree this PR would be unnecessary if the ip_asn.map file were loaded by default (or if asmap data were embedded in the binary and enabled by default)? Those seem like cleaner solutions to me.

   Or if it is not safe to load asmap data be default, it would seem cleaner not to have a default’filename that is not actually used by default.

9. 
   laanwj commented at 1:39 pm on October 23, 2025: member

   @laanwj do you agree this PR would be unnecessary if the ip_asn.map file were loaded by default (or if asmap data were embedded in the binary and enabled by default)? Those seem like cleaner solutions to me.

   The reason i like this PR is that i dislike how the current code interprets an argument as either a boolean or a string. This is not compatible with stricter option parsing.

   If reading a file by default or embedding it into the binary gets rid of that, that’s fine with me too. But i guess even in that case, one might want to either disable the functionality, or override the path? It seems partly orthogonal to these options.

   Or if it is not safe to load asmap data be default, it would seem cleaner not to have a default’filename that is not actually used by default.

   Right.

10. 
    ryanofsky commented at 5:29 pm on October 29, 2025: contributor

    If reading a file by default or embedding it into the binary gets rid of that, that’s fine with me too. But i guess even in that case, one might want to either disable the functionality, or override the path?

    Yes, these things are done with -noasmap and -asmap=<filename> and covered in existing feature_asmap.py tests.

    It seems partly orthogonal to these options.

    I do think the issues are pretty directly related, not orthogonal. The basic problem here is that this option’s default value (ip_asn.map) and default behavior (not loading ip_asn.map) do not match.

    If the default behavior is changed to match the default value (as in the first diff), or if the default value is changed to match the default behavior (as in the second diff) no issues should remain.

    By contrast, this PR fixes the mismatched defaults by splitting them into two options, making the interface more complicated and requiring multiple parameters just to specify a single file. It also makes the way asmap files are specified different than the way other files are specified (-conf, -settings, -pid, etc), when it would seem good to have a consistent way to specify, enable, and disable files.

11. 
    fjahr commented at 4:02 pm on October 31, 2025: contributor

    @ryanofsky @laanwj Thank you the in-depth discussion here. I previously thought it might be best to leave the default file behavior untouched but if people are unhappy with it then now, in this PR, is certainly the right time to fix it and I am not married to the idea of keeping it the same. I have thought about asmap more than most in the last couple of years so I’ve simply gotten used to how things were.

    The basic problem here is that this option’s default value (ip_asn.map) and default behavior (not loading ip_asn.map) do not match.

    I do think it’s a bit unsafe to simply introduce loading and using the default file just because it’s present. I don’t know how well people manage their datadirs but it could be that someone has a very old default file there from when the feature was first developed or some limited example file from a workshop or so.

    So the two options I would feel comfortable with right now would be: 1. Use the default file when it’s present but rename the default file once in the same step. For example ip_asn.map becomes asmap.dat. This mitigates my main concern with old default files laying in the datadir which people forgot about. 2. Remove the default file behavior.

    I have a slight tendency to the first option because some people seem to like the convenience of this feature. What do you think?

12. 
    ryanofsky commented at 4:32 pm on October 31, 2025: contributor

    I have a slight tendency to the first option because some people seem to like the convenience of this feature. What do you think?

    I think I like both equally, and it feels like whichever default you pick shouldn’t matter too much because it will change again soon when asmap data is embedded? (I’m assuming when asmap data is embedded, the default will be to use embedded data unless you specify -noasmap or a filename.)

13. 
    fjahr commented at 11:59 pm on October 31, 2025: contributor

    and it feels like whichever default you pick shouldn’t matter too much because it will change again soon when asmap data is embedded? (I’m assuming when asmap data is embedded, the default will be to use embedded data unless you specify -noasmap or a filename.)

    The long term plan is to have the embedded data used by default, however first the idea was to have a release or two with the data embedded but the usage still off by default. This is a cautious intermediate step similar to how assumeutxo was merged without a mainnet param and how v2 transport was off-by-default in the beginning. While I am personally very confident in these matters there are still regularly the same set of questions raised about the data that is embedded and how it degrades after 6 or 12 months. Also concerns about nodes from crowded ASs having trouble finding all the connections they want. The intermediary step helps to build confidence further over that time that on-by-default is truly effective and safe. Just giving some background but to discuss this in particular I think #28792 is the right place for that.

    That said, I think it makes sense to aim the design primarily for the future when embedded data is used by default. So for that future version we would have data from an explicit file path, the default file path or the embedded data and with any of those available the feature is used unless -noasmap is set. I think we agree that this is consistent and we should eventually end up in that state.

    For the intermediate time period we have the options of the current behavior of this PR, or the options you mentioned above with my modification suggestion. Now that I have laid it out like this I am again leaning towards keeping the current behavior for the intermediary step: Data can be provided with explicit path, the default path or (with #28792) from the embedded data but no matter where the data is from, the feature isn’t used unless -asmap is also specified. This would mean that we can simply switch the -asmap from on by default to off by default and nothing else needs to change. I would still want to change the name of the default file though, either now or then. The inconsistency still remains with the other options that is using a default file when present (-pid etc. as mentioned above) but this would go away when asmap is turned on by default overall. And I think I prefer this kind of inconsistency across different args with the alternative of being inconsistent within asmap (default file presence turns feature on, embedded data doesn’t turn feature on) which is what option 1 would do for the period of the intermediate step.

    And of course removing the default file just gets rid of this problem entirely.

14. ryanofsky referenced this in commit 90db796049 on Nov 3, 2025
15. 
    ryanofsky commented at 1:33 pm on November 3, 2025: contributor

    #33631 (comment)

    The long term plan is to have the embedded data used by default, however first the idea was to have a release or two with the data embedded but the usage still off by default.

    This makes sense, but I think if it is the plan, it should rule out option “1. Use the default file when it’s present” so bitcoind will consistently just not load asmap data, instead of loading a file by default but not loading embedded data by default.

    So I opened PR #33770 implementing option “2. Remove the default file behavior.” I think it’s a straightforward improvement with no real downsides that’s less confusing than current behavior and more compatible with your short & long term plans. But if you do prefer current behavior over #33770 that’s also fine and I’m happy to close it. I agree current behavior seems ok as an intermediate state.

    I was also thinking more about the problem you mentioned of “old default files laying in the datadir” being loaded. This seems like a general problem that will always exist as long as external asmap files can be loaded. A good solution could be have a timestamp in asmap files reflecting when the data was collected, and have bitcoind refuse to load external asmap files with timestamps less than the timestamp in their embedded asmap file.

16. ryanofsky referenced this in commit cfeb160bae on Nov 3, 2025
17. ryanofsky referenced this in commit 44717da29b on Nov 5, 2025

Contributors
fjahr DrahtBot ryanofsky laanwj