init: Split file path handling out of -asmap option

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

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

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
User requested bot ignore	fanquake

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:

#33920 (Export embedded ASMap RPC by fjahr)
#33878 (refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation by fjahr)
#33770 (init: Require explicit -asmap filename by ryanofsky)
#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#28792 (build: Embedded ASMap [3/3]: Build 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.

fjahr renamed this:
~~init: Split file path handling out of -asmap~~
init: Split file path handling out of -asmap option
on Oct 14, 2025

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

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.

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.

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.

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.

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.

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?

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.)

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.

ryanofsky referenced this in commit 90db796049 on Nov 3, 2025

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.

ryanofsky referenced this in commit cfeb160bae on Nov 3, 2025

ryanofsky referenced this in commit 44717da29b on Nov 5, 2025

ryanofsky referenced this in commit deac31dd77 on Nov 13, 2025

fanquake commented at 11:49 am on November 19, 2025: member

Is this still relevant, given you’ve ACK’d #33770?

fjahr commented at 1:12 pm on November 19, 2025: contributor

Is this still relevant, given you’ve ACK’d #33770?

Yes, these are solving two separate reasons people were unhappy with the -asmap behavior and I think both changes have support and should ideally go into the same release, so users of the option only have to adapt to a new behavior once.

Even though @ryanofsky didn’t explicitly review this PR here yet my understanding is that he didn’t intend #33770 as a replacement of this either, but please correct me if I am wrong.

ryanofsky commented at 2:04 pm on November 19, 2025: contributor

Even though @ryanofsky didn’t explicitly review this PR here yet my understanding is that he didn’t intend #33770 as a replacement of this either, but please correct me if I am wrong.

I did intend to #33770 to be an alternative this PR and as far I I know there wouldn’t be any benefits to this PR if #33770 is merged, unless I’m missing something. Both this PR and #33770 are addressing the problem of -asmap default value not matching its default behavior. #33770 addresses that by making the defaults consistent, this PR addresses it by splitting the two defaults into two options.

ryanofsky referenced this in commit f6ec3519a3 on Nov 19, 2025

fjahr commented at 2:23 pm on November 19, 2025: contributor

Even though @ryanofsky didn’t explicitly review this PR here yet my understanding is that he didn’t intend #33770 as a replacement of this either, but please correct me if I am wrong.

I did intend to #33770 to be an alternative this PR and as far I I know there wouldn’t be any benefits to this PR if #33770 is merged, unless I’m missing something. Both this PR and #33770 are addressing the problem of -asmap default value not matching its default behavior. #33770 addresses that by making the defaults consistent, this PR addresses it by splitting the two defaults into two options.

Ok, then I misunderstood this but I still think that these two topics are orthogonal and at CoreDev and here people have voiced support for having the bool option split from the file path. Especially in the context of having embedded data this is much clearer since #33770 doesn’t fix the messy ambiguity between bool and path usage of the parameter.

Also, we now could be in a temporary state with where using -asmap=1 doesn’t work under any circumstances unless given a file path but when we get the embedded data then -asmap=1 would work again. All this switching back and forth is more confusing to users than the potential confusion #33770 seeks to fix IMO.

ryanofsky commented at 3:12 pm on November 19, 2025: contributor

Thanks for explaining. Now I see why you still want this PR. In the short term with #33770 there are no problems, and in the long terms with embedded asmap turned on by default there are no problems. But in the medium turn with embedded asmap data present but not turned on by default, you dislike the fact that -asmap=1 would load a file called 1 if we don’t take additional steps to prevent this.

To clarify:

In the short term, with #33770 the situation is simple. No asmap file is loaded by default unless you specify -asmap=<filename>.
In the long term, with asmap data embedded and turned on by default, the situation is also simple: asmap data is loaded by default, but it can be replaced with -asmap=<filename> or disabled with -noasmap.
However, in the medium term when embedded data is present but not used by default, you need to provide some way to enable it. IMO, you should just accept a bare -asmap option to enable it, and this should be sufficient and have no ambiguity. Alternately, you could accept -asmap=1 which would introduce a little ambiguity but be easy to detect with GetBoolArg("-asmap", false). I think this would be fine, but I can understand why some people might not like it. And finally, this PR would provide a third alternative solution. IMO it would just not be a very good solution because it would be introducing complexity and inconsistency with other file options to solve a temporary and minor problem. Having to use two settings to load one file just seems like a cumbersome UX to me.

fjahr commented at 10:11 pm on November 19, 2025: contributor

But in the medium turn with embedded asmap data present but not turned on by default, you dislike the fact that -asmap=1 would load a file called 1 if we don’t take additional steps to prevent this.

Yes, this is what has bothered me most about the arg for a long time and why I had included my minimal fix for this in the original embedding PR already, just implemented a bit differently than you suggest but with the same effect. I think it’s just not acceptable that you couldn’t even use the embedded asmap data by setting the option from the conf file because of this. This was also noted in #33386.

In the long term, with asmap data embedded and turned on by default, the situation is also simple: asmap data is loaded by default, but it can be replaced with -asmap= or disabled with -noasmap.

Right, but we don’t know yet how long the long term means here. I am happy to see new reviewers of asmap PRs now but I think there may still be some additional convincing necessary to enable it by default. This is different from BIP324 where, as far as I remember, it was accepted pretty early that one release would be enough before it could be enabled by default. Given how scarce review has been I don’t want to open this discussion for asmap before the embedding has actually been merged, to not create more distractions for those interested in the topic.

IMO, you should just accept a bare -asmap option to enable it, and this should be sufficient and have no ambiguity.

Sure, this is basically what I had suggested with #33632 just implemented differently, you had already commented on this there in earlier. I was asked to make a decision for which way to go between these different approaches to not drag out the discussion and I chose this one because the approach is cleaner even though it’s more verbose and because I sensed slightly more support for it. I understand that you would prefer #33632 to this approach here and if more reviewers agree with you I am open to switch the approach. But for now I will keep going with this because of the rationale above and @laanwj had already expressed preference for this approach earlier: #33631 (comment)

Lastly I want to add this for anyone else who reads this and wonders, why not stack this change on top of #28792 because -asmap=1 isn’t possible until that arrives. My rationale for not doing this is that I would like for the changes of the asmap arg UX to go in as close together as possible so that users don’t have to adapt their behavior twice, so this should ideally go together with #33770 IMO.

achow101 commented at 10:27 pm on November 21, 2025: member

I’m slightly confused as to what the plan is between this and #33770. This PR both has code conflicts and logical conflicts with #33770. 33770 redefines -asmap in one way, while this redefines it in the opposite way.

Since 33770 seems more likely to be merged soon, how would this PR change to resolve those conflicts.

fjahr commented at 10:48 pm on November 21, 2025: contributor

I’m slightly confused as to what the plan is between this and #33770. This PR both has code conflicts and logical conflicts with #33770. 33770 redefines -asmap in one way, while this redefines it in the opposite way.

It is not the opposite way, it’s an orthogonal issue. #33770 removes the usage of the default file and this PR splits the -asmap usage from being a path arg (maybe interpreted as a bool) to a bool arg and a path arg seperately which is much cleaner compared to master (with default file) and also when we get embedded data but it is not very relevant while #33770 is merged and embedded is not. When #33770 is merged I will obviously rebase this here and keep it open, even if people fail to see the appeal until embedded asmap is merged. I think they will get it when they can’t use the embedded asmap through their config file ;) I just think it would be better for users to change the -asmap UX once and then stay with it rather than doing #33770 and then this (or #33632 if more people speak up for it) in the next release or so and that’s why I am not basing it on the embedding PR as explained above.

sipa commented at 10:56 pm on November 21, 2025: member

I think having two options to control such a minor feature is unintuive and overkill. If that’s what people like, no objection as it really isn’t important. But my preference would be to put everything in the -asmap option:

-asmap=netgroups (and for compatibility, -asmap=0) to get the /16 based splitting.
-asmap=builtin to get the file built-in to the binary, or fall back to netgroups if nothing was built in.
-asmap=<filename> to get the specified filename if it exists, and error otherwise.
-asmap=default (and for compatibility, -asmap=1) to get ip_asn.dat if it exists, and fall back to builtin otherwise.

If #33770 goes in, and that’s behavior we want to keep, -asmap=default is unnecessary and the default can be -asmap=builtin.

fjahr commented at 10:58 pm on November 21, 2025: contributor

I guess there are too many walls of text here and elsewhere so I will try to make it more concise, this PR here (and #33632 with a different approach) fixes two things:

bitcoind -asmap=1 => errors because it looks for a file named “1” in datadir (though -asmap works)
You can not use asmap=1 in the config file at all for the same reason

#33770 doesn’t fix this, it just gets rid of the current reason anyone would use -asmap=1 (to activate usage of the default location). You always need to give an actual path with it, otherwise you can’t get the asmap data. Once we have embedded data we have a new reason to use -asmap=1 so then we have the same problems again.

achow101 referenced this in commit 0690514d4f on Nov 21, 2025

ryanofsky commented at 0:29 am on November 22, 2025: contributor

Yes conflict with #33770 is superficial, and both could be merged. They both solved #33386 in different ways, but are otherwise independent.

I don’t think this PR is a good idea, but don’t oppose it if others like it. I don’t think separating -asmap and -asmapfile options is user-friendly or consistent, even though I understand the problem this is trying to solve. I just think the problem is minor and temporary.

Right now since #33770 is merged there’s no issue with the -asmap option. After embedded asmap data is added, we can support -asmap to enable the embedded data, -noasmap to disable the data, and -asmap=<file> to load an external file. As long as embedded data is enabled by default this syntax is totally sufficient, but if there is a temporary period where asmap data is embedded but not enabled by default we may want to allow other syntaxes like -asmap=1 or maybe -asmap=builtin like sipa suggested so users do not have to awkwardly write asmap= in bitcoin.conf to test the feature before it is fully rolled out. These solutions seem ok to me so I don’t think having a separate option is justified, but again I think this change is fine if others want it.

DrahtBot added the label Needs rebase on Nov 22, 2025

DrahtBot commented at 1:41 am on November 22, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

fanquake commented at 11:44 am on November 25, 2025: member

I think having two options to control such a minor feature is unintuive and overkill. If that’s what people like, no objection as it really isn’t important. But my preference would be to put everything in the -asmap option:

I agree.

fjahr commented at 1:06 pm on November 25, 2025: contributor

Seems like there is now more opposition to this approach, so I will close this and consider an alternative as a follow-up when the embedding is merged.

fjahr closed this on Nov 25, 2025

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

Code Coverage & Benchmarks

Reviews

Conflicts