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.
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.
Co-authored-by: Fabian Jahr <fjahr@protonmail.com>
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33770.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | brunoerg, fjahr, vostrnad |
| Stale ACK | TheCharlatan, pablomartin4btc, jurraca |
| User requested bot ignore | achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
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);
cfeb160baec1369452c42d05c51f2a6af76ed077: nit: Perhaps we could remove this (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.
I also don't like the inconsistent (default: none). I think it's pretty clear that if no default is mentioned, the feature is disabled. The help text for -i2psam should be fixed.
nit: strprintf isn't needed anymore.
re: #33770 (review)
I also don't like the inconsistent
(default: none). I think it's pretty clear that if no default is mentioned, the feature is disabled. The help text for-i2psamshould be fixed.
Thanks, I still think it is nice to be explicit about the default behavior, but I get that if you think about it, any other default behavior would be surprising, so it's not really necessary to mention. Dropped (default: none) from both -i2psam and -asmap descriptions as suggested.
code review ACK cfeb160baec1369452c42d05c51f2a6af76ed077
ACK cfeb160baec1369452c42d05c51f2a6af76ed077
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:
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...
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.
<!-- begin push-3 -->
Updated cfeb160baec1369452c42d05c51f2a6af76ed077 -> 44717da29b626bfdcb20a32318689d74c4113b7b (pr/asmapd.2 -> pr/asmapd.3, compare)<!-- end --> 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).
1546 | @@ -1548,11 +1547,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) 1547 | // Read asmap file if configured 1548 | std::vector<bool> asmap; 1549 | if (args.IsArgSet("-asmap") && !args.IsArgNegated("-asmap")) { 1550 | - fs::path asmap_path = args.GetPathArg("-asmap", DEFAULT_ASMAP_FILENAME); 1551 | + fs::path asmap_path = args.GetPathArg("-asmap");
Effectively the arg is not allowed anymore without a path to a file, so I think it would make sense to return an explicit error here which gives the user that feedback. The current error for the empty -asmap is "Could not find asmap file /path/to/datadir" which suggests there still is a default location where the file (without a name) could not be found. This error message would also be more helpful for anyone who might still try to use the default location feature with the next release.
re: #33770 (review)
Thanks, added a more specific error message for this case.
65 | @@ -79,52 +66,49 @@ def test_asmap_with_relative_path(self): 66 | self.start_node(0, [f'-asmap={name}']) 67 | os.remove(filename) 68 | 69 | - def test_default_asmap(self): 70 | - shutil.copyfile(self.asmap_raw, self.default_asmap) 71 | + def test_unspecified_asmap(self): 72 | + msg = f"Error: Could not find asmap file \"{self.datadir}{os.sep}\""
nit: I was going to say that the unspecified file and the missing file test could be deduplicated since they are currently similar but I think the better change would be to have an explicit error message for the empty -asmap arg.
re: #33770 (review)
nit: I was going to say that the unspecified file and the missing file test could be deduplicated since they are currently similar but I think the better change would be to have an explicit error message for the empty
-asmaparg.
Make sense, implemented the separate error message
Approach ACK
While not everyone I asked has gotten back to me yet, almost all responders did already use an explicit file path and the few that had used the option of the default file had similar sentiment to @sipa.
The file should probably also be dropped from files.md then since there is nothing special about that name/location anymore, e.g. https://github.com/fjahr/bitcoin/commit/e0283bb8c133c8cae19c48c8d78cfe95dab664ee
Thanks for the reviews! Made all suggested changes
<!-- begin push-4 -->
Updated 44717da29b626bfdcb20a32318689d74c4113b7b -> deac31dd779d28a8c01f1c3153650f5827cfe75d (pr/asmapd.3 -> pr/asmapd.4, compare)<!-- end --> updating files.md and adding a more specific error message for a missing filename
ACK deac31dd779d28a8c01f1c3153650f5827cfe75d
Reviewed code and tested locally. I had grepped for other relevant mentions of the default location previously.
Suggested by Vojtěch Strnad (vostrnad)
https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2529117675
Thanks for the reviews!
<!-- begin push-5 -->
Updated deac31dd779d28a8c01f1c3153650f5827cfe75d -> 288b8c30be42f2879d1a1f3d5ec3cac13f87ace4 (pr/asmapd.4 -> pr/asmapd.5, compare)<!-- end --> making suggested documentation tweaks (since previous ACKs were stale anyway)
reACK 288b8c30be42f2879d1a1f3d5ec3cac13f87ace4
Verified it removed the strprintf and (default: none).
reACK 288b8c30be42f2879d1a1f3d5ec3cac13f87ace4
utACK 288b8c30be42f2879d1a1f3d5ec3cac13f87ace4
@fjahr DrahtBot is ignoring your ACK, is that intended? If you mean to withdraw your ACK, I think editing your comment with a strikethrough and explanation is the typical way people do that.
It falsely interpreted this comment as an ACK previously and that's when I put the ignore: #33770 (comment) I have removed the emoji reaction, not sure if that makes it reconsider my latest ACK but that is still valid.
re-ACK 288b8c30be42f2879d1a1f3d5ec3cac13f87ace4
Only minor fixes since last review.
ACK 288b8c30be42f2879d1a1f3d5ec3cac13f87ace4