Currently, if -asmap is specified without a filename bitcoind tries to load ip_asn.map data file.
This change now requires -asmap=ip_asn.map or another filename to be specified explicitly.
The change is intended to make behavior of the option explicit and avoid confusion reported #33386 where documentation specifies a default file which is not actually loaded by default. It was originally implemented in #33631 (comment) and various alternatives are discussed there.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33770.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | jurraca |
| Stale ACK | brunoerg, TheCharlatan, pablomartin4btc |
| User requested bot ignore | fjahr |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
530@@ -532,7 +531,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
531 ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
532
533 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);
534- 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);
535+ 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);
(default: none) to maintain consistency with other =<file> options?
re: #33770 (review)
cfeb160: nit: Perhaps we could remove this
(default: none)to maintain consistency with other=<file>options?
Thanks for the suggestion. I looked into it and initially it seemed like a good simplification, but then I noticed that this is not the only (default: none) mention in help since there is also -i2psam. Also since PR is changing behavior, and we know behavior will change again in the future #33631 (comment), it seems like it might be better to be explicit about what exactly the default is right now.
If you disagree, or think default: none could be confusing in some way, or replaced with something clearer, please let me know. I’m just not sure it makes sense to drop completely.
78@@ -80,51 +79,48 @@ def test_asmap_with_relative_path(self):
79 os.remove(filename)
80
81 def test_default_asmap(self):
82- shutil.copyfile(self.asmap_raw, self.default_asmap)
83+ msg = f"Error: Could not find asmap file \"{self.datadir}{os.sep}\""
84 for arg in ['-asmap', '-asmap=']:
85 self.log.info(f'Test bitcoind {arg} (using default map file)')
nit:
0 self.log.info(f'Test bitcoind {arg} (using a map file)')
utACK cfeb160baec1369452c42d05c51f2a6af76ed077
If it won’t be a default asmap filename, perhaps we could remove “default” from the test function names in test/functional/feature_asmap.py?
Left another nit…
Currently, if `-asmap` is specified without a filename bitcoind tries to load
`ip_asn.map` data file.
This change now requires `-asmap=ip_asn.map` or another filename to be
specified explicitly.
The change is intended to make behavior of the option explicit avoid confusion
reported https://github.com/bitcoin/bitcoin/issues/33386 where documentation
specifies a default file which is not actually loaded by default. It was
originally implemented in
https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3410302383 and
various alternatives are discussed there.
Thanks for the suggestions and reviews! Implemented some test cleanup below.
I do want to wait for feedback from @fjahr before going ahead with this PR since he expressed a preference for keeping behavior unchanged until #28792 is merged (“I am again leaning towards keeping the current behavior for the intermediary step” from #33631 (comment)) and I think that would be reasonable. But I do think this PR is an improvement in the short term, and resolution to #33386, and good preparation for #28792 since it makes stops making -asmap load file data like #28792 will also do.
Updated cfeb160baec1369452c42d05c51f2a6af76ed077 -> 44717da29b626bfdcb20a32318689d74c4113b7b (pr/asmapd.2 -> pr/asmapd.3, compare) cleaning up test as suggested
Thanks @ryanofsky for the ping and for moving the conversation forward.
I am -0 on this change for now because in the recollection of conversations about ASMap usage whenever the conversation touched this (which was rarely) then either people were indifferent about the default path or found it nice to have. I don’t recollect anyone expressing negativity about it, as in it doesn’t make sense or it won’t be used. I can not point at specific people or conversations that were documented for this, so take it with a grain of salt. Also, I think the maintenance burden of the code is minimal and the inconsistencies are not as big of deal to me as to others.
As I laid out in #33631 (comment) I convinced myself that the current behavior is as consistent as we can get currently and in the context of embedded asmap data being added but being off by default. And when we switch to on by default eventually the behavior should be fully consistent. So not making a change is still my slight preference based on the sentiment I mentioned above.
However, this is an absolutely valid and certainly also the easiest solution. I think it is also not unreasonable to assume that the demand for this feature goes down when we have the embedded data available. The code looks good to me as well on first glance.
I will reach out to a few people today that have been using asmap files consistently in the past and ask them to weigh in if they prefer to use asmap with this option. If there isn’t anyone speaking out in favor of keeping it within a few days it should be fine to remove it and I would change my -0 to +0. But if this PR gets overwhelming support and is merged earlier, I would also be fine with it.
[P]eople were indifferent about the default path or found it nice to have. I don’t recollect anyone expressing negativity about it, as in it doesn’t make sense or it won’t be used
To be sure, I think it could make sense to have a default path that is loaded by default. I think it just to confusing to have a default path that is not loaded by default, and I think #33386 shows I’m not the only one who finds the current situation confusing.
Also if #28792 will require -asmap=<file> to load file data, then it would seem to make sense to also require that now, instead of providing two ways of loading file data now (-asmap=<file> and -asmap) and then taking one away later.
To be sure, I think it could make sense to have a default path that is loaded by default. I think it just to confusing to have a default path that is not loaded by default, and I think #33386 shows I’m not the only one who finds the current situation confusing.
Sure I don’t question that there is a potential confusion, we just disagree on whether the confusion is bad enough to take away the feature. I would like to go with whatever the users want which might be in favor of keeping the option, but if I can’t confirm that hunch in the short term then happy to ACK removing it. I have pinged the OP of #33386 there as well to clarify where they stand on this.
Also if #28792 will require
-asmap=<file>to load file data, then it would seem to make sense to also require that now, instead of providing two ways of loading file data now (-asmap=<file>and-asmap) and then taking one away later.
Review on #28792 is extremely slow, so I do expect this discussion here and #33631 to be resolved or merged before #28792 is close to getting merged. So I am not so concerned with ordering.
ACK 44717da29b626bfdcb20a32318689d74c4113b7b
I think it’s a better pattern to force users to specify the filename, but I can’t tell whether users currently depend on the default.
I’m neutral on this PR.
I do use the asmap.dat loading by default, but (a) don’t care about changing that to specify it explicitly and (b) I hope #28794 happens soonish so it becomes a non-issue (as I will switch to built-in asmap files rather than explicit ones in the datadir).
