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.

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.

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:

--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1547,15 +1547,20 @@ 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", DEFAULT_ASMAP_FILENAME);
+        if (!asmap_path.empty()) {
             if (!asmap_path.is_absolute()) {
                 asmap_path = args.GetDataDirNet() / asmap_path;
             }
-            if (!fs::exists(asmap_path)) {
+            if (!args.IsArgSet("-asmap") && !fs::exists(asmap_path)) {
+                // It is ok for asmap file not to exist if not specified.
+                asmap_path.clear();
+            } else if (!fs::exists(asmap_path)) {
                 InitError(strprintf(_("Could not find asmap file %s"), fs::quoted(fs::PathToString(asmap_path))));
                 return false;
             }
+        }
+        if (!asmap_path.empty()) {
             asmap = DecodeAsmap(asmap_path);
             if (asmap.size() == 0) {
                 InitError(strprintf(_("Could not parse asmap file %s"), fs::quoted(fs::PathToString(asmap_path))));
--- a/test/functional/feature_asmap.py
+++ b/test/functional/feature_asmap.py
@@ -49,7 +49,14 @@ class AsmapTest(BitcoinTestFramework):
             self.nodes[node_id].addpeeraddress(address=f"101.{addr}.0.0", tried=tried, port=8333)
 
     def test_without_asmap_arg(self):
-        self.log.info('Test bitcoind with no -asmap arg passed')
+        self.log.info(f'Test bitcoind with no -asmap arg passed, with {DEFAULT_ASMAP_FILENAME} present')
+        shutil.copyfile(self.asmap_raw, self.default_asmap)
+        self.stop_node(0)
+        with self.node.assert_debug_log(expected_messages(self.default_asmap)):
+            self.start_node(0)
+        os.remove(self.default_asmap)
+
+        self.log.info(f'Test bitcoind with no -asmap arg passed, without {DEFAULT_ASMAP_FILENAME} present')
         self.stop_node(0)
         with self.node.assert_debug_log(['Using /16 prefix for IP bucketing']):
             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:

--- a/src/init.cpp
+++ b/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.
@@ -532,7 +531,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>", 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);
     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);
@@ -1548,11 +1547,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.is_absolute()) {
                 asmap_path = args.GetDataDirNet() / asmap_path;
             }
-            if (!fs::exists(asmap_path)) {
+            if (!fs::is_regular_file(asmap_path)) {
                 InitError(strprintf(_("Could not find asmap file %s"), fs::quoted(fs::PathToString(asmap_path))));
                 return false;
             }
--- a/test/functional/feature_asmap.py
+++ b/test/functional/feature_asmap.py
@@ -29,7 +29,6 @@ import shutil
 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'
 
@@ -80,21 +79,18 @@ class AsmapTest(BitcoinTestFramework):
         os.remove(filename)
 
     def test_default_asmap(self):
-        shutil.copyfile(self.asmap_raw, self.default_asmap)
+        msg = f"Error: Could not find asmap file \"{self.datadir}{os.sep}\""
         for arg in ['-asmap', '-asmap=']:
             self.log.info(f'Test bitcoind {arg} (using default map file)')
             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",
@@ -102,29 +98,29 @@ class AsmapTest(BitcoinTestFramework):
             ]
         ):
             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')
         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=[f'-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,12 +129,10 @@ class AsmapTest(BitcoinTestFramework):
                 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)
 

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.

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

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.

@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?

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

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.

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

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

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.

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.

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.

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.

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.

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.

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.

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.

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:

1. bitcoind -asmap=1 => errors because it looks for a file named "1" in datadir (though -asmap works)
2. 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.

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.

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

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.

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.