config, net, test: asmap feature refinements and functional tests #17812

This PR builds on PR #16702 to add functional tests / sanity checks and user-facing refinements for passing -asmap to configure ASN-based IP bucketing in addrman. As per our review discussion in that PR, the idea here is to handle aspects like functional tests and config arg handling that can help the PR be merged while enabling the author to focus on the bucketing itself.

• add feature functional tests to verify node behaviour and debug log output when launching

  • bitcoind with no -asmap arg

  • bitcoind -asmap=RELATIVE_FILENAME to the unit test data skeleton asmap

  • bitcoind -asmap with no filename specified using the default asmap file

  • bitcoind -asmap with no filename specified and a missing default asmap file

• add the ability to pass absolute path filenames to the -asmap config arg in addition to datadir-relative path filenames as per #16702 (review), and add test coverage

• separate the asmap file finding and parsing checks, which allows adding tests for the case of a found but unparseable or empty asmap

• add test for an empty asmap

• various asmap fixups

• move the asmap init code earlier in the init process to provide immediate feedback when passing an -asmap config arg. This speeds up the feature_asmap functional test from 60 to 5 seconds! Credit to Wladimir J. van der Laan for the suggestion.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #18038 (P2P: Mempool tracks locally submitted transactions to improve privacy by amitiuttarwar)
• #16224 (gui: Bilingual GUI error messages by hebasto)

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.

Shouldn't e4f5aa4b09a0f0c51def6b859609aee01b61d2df, b380e9a48e5c6cb7e1ed6558e7fbbde5feb0069b and eeb1a42341c9a5a9517a244506dd2a609962ee5c just be left as feedback/review on #16702? Especially if one of those commits is required for the CI to pass.

Yes, those can be pulled into the PR if helpful. I have more important review questions on that PR that I'd like to concentrate on (the unit tests and actual bucketing) without adding further nit review at the moment.

The idea here, as per our review discussion in that PR, is to handle other aspects (functional tests, config arg handling, user-facing interface) that can help it be merged without the author needing to bother with it.

+1 for allowing absolute asmap paths

Rebased and added #16702 (comment) and #16702 (review) in the last 2 commits.

Can you squash a bunch of these? cd6d6d138f4844d35537f07c6c106e42dc0d8581 and b5cf2db1a4cd20175f37fc039c2bc0e15ccef1e4 could be combined. I don't think we need separate commits just to remove full stops and then add spaces to logging. 8554077e5532c26b28be95249992d96b68feebd7 reorders includes, which generally we don't do by itself, maybe combine into e44e8842b4da2829bbdcd982c83bb5ffb665b472.

Has anyone fuzzed the asmap file reader? I anticipate users accepting asmaps from third parties...

Oh, nevermind, I just saw #18033

@fanquake thanks for having a look and the suggestions. Done, squashed all those down into e30a4691bacc232a984e049fb887b08860fa7c94.

Needs rebase (due to sipa's asmap changes in #18023, I think)

Rebased, added 366aab9 for items found during review of other now-merged asmap PRs, and updated the new tests to use self.chain rather than 'regtest'.

Thank you @sipa for reviewing, updated as per git diff 11a1028..1e26a1a

diff --git a/src/addrman.cpp b/src/addrman.cpp
index ef34edc0a0..2f8a3a0bd5 100644
-    LogPrint(BCLog::NET, "IP %s mapped to AS %i belongs to tried bucket %i\n", ToStringIP(), mapped_as, tried_bucket);
+    LogPrint(BCLog::NET, "IP %s mapped to AS%i belongs to tried bucket %i\n", ToStringIP(), mapped_as, tried_bucket);
 
-    LogPrint(BCLog::NET, "IP %s mapped to AS %i belongs to new bucket %i\n", ToStringIP(), mapped_as, new_bucket);
+    LogPrint(BCLog::NET, "IP %s mapped to AS%i belongs to new bucket %i\n", ToStringIP(), mapped_as, new_bucket);
 
diff --git a/src/init.cpp b/src/init.cpp
index 6591852a7f..7b5e536b8f 100644
-    gArgs.AddArg("-asmap=<file>", strprintf("Specify asn mapping used for bucketing of the peers (default: %s). Relative paths will be prefixed by a net-specific datadir location.", DEFAULT_ASMAP_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
+    gArgs.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);

diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 59d299e749..2f66fb9299 100644
-            "    \"mapped_as\":\"mapped_as\", (numeric) The AS in the BGP route to the peer used for diversifying peer selection\n"
+            "    \"mapped_as\": n,            (numeric) The AS in the BGP route to the peer used for diversifying peer selection\n"
+            "                                         (only available if the asmap config flag is set)\n"

ACK 1e26a1af96438edf96ecb2268569d5bec4a7f43e optional nit #17812 (review) happy to reack if you decide to address that

Thanks @pinheadmz for reviewing, updated as per git diff 1e26a1a..dccc6bf:

test/functional/feature_asmap.py
-        name = 'ASN map'
+        name = 'ASN_map'

ACk dccc6bfc04e43bbdf8db35e0fb7288231b05973f

Great work, thanks! Especially those tests. ACK dccc6bfc04e43bbdf8db35e0fb7288231b05973f

ACK dccc6bfc04 🐔

<details><summary>Show signature and timestamp</summary>

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK dccc6bfc04 🐔
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUh6Cgv+M0g7bjlY1yqXorkNkyNeTx6YJpaqnM+w5VCirwJxVpH7hXGo8USv2t6d
N2u0oyAslzwupoD83ucutnx8yxbZ1cqDXvRasDX1I2pFACgKjSmd9NqkKOlpO6Qi
8KSSsvFeGBv2IO5dmIoRpDdUsebewBMnCdbJXh0g59+hkKo0GUU0NLcHIpdBgR1C
Un/ffCCmnF2KlvZAi1jje9fDc0ws2oMUzjb8NP7Zw2jtSEFev22j8lPpVX3efiWe
0l6+9eJ+La5B5OxwHx2afhEjybmlKSnEM5BidpYdDJRlN6/8GdkWACvCi9yr+uCV
NMfSVSV5lGXAGArf+lFH5j8ru1VelBy/GB2291q42b/wo/PvGMOn59IurgK4sfGr
/lzT66I+8Z8WtU4o8IDIShObkt4nJtHlosROun7Rx6XFC4enA5b82HkDnPfpY0vb
19r3DaJwTxQ22gEo1EhFAcKKTJThPbcwMtoNpsOMi+hzdBLunNTcCVbC2SN9ymoh
/PXwblvP
=zKAr
-----END PGP SIGNATURE-----

Timestamp of file with hash 9e9a24d432005b3d1790df265d0b15fa6227860ab58337e3ed405bd9b7b94cba -

</details>

Rebased. Thank you to the reviewers. Mind re-ACKing?

reACK d3ec323d151c5b19a7c4fa78000087266c9c6d2e

Confirming rebase didn't change anything:

$ git range-diff 712b7d9b4..dccc6bfc0  36f42e1bf..d3ec323d1
 1:  c4dc420cb =  1:  610cc93e9 test: add feature_asmap functional tests
 2:  919f4d7dd =  2:  a8a82d5bf config: use default value in -asmap config
 3:  429129440 =  3:  d7b1a5b04 config: enable passing -asmap an absolute file path
 4:  eacb2458e =  4:  b321be999 config: separate the asmap finding and parsing checks
 5:  0c3b1499b =  5:  366600e83 test: add functional test for an empty, unparsable asmap
 6:  89177df7a =  6:  d6e44f0b9 logging: asmap logging and #include fixups
 7:  392798830 =  7:  91ff62ff0 net: extract conditional to bool CNetAddr::IsHeNet
 8:  317bf6aea <  -:  --------- rpc: fix getpeerinfo RPCResult `mapped_as` type
 -:  --------- >  8:  ab8a6cbcb rpc: fix getpeerinfo RPCResult `mapped_as` type
 9:  dccc6bfc0 =  9:  d3ec323d1 init: move asmap code earlier in init process

<              "    \"addr\":\"host:port\",      (string) The IP address and port of the peer\n"
<              "    \"addrbind\":\"ip:port\",    (string) Bind address of the connection to the peer\n"
<              "    \"addrlocal\":\"ip:port\",   (string) Local address as reported by the peer\n"
< -            "    \"mapped_as\":\"mapped_as\", (string) The AS in the BGP route to the peer used for diversifying peer selection\n"
< +            "    \"mapped_as\": n,            (numeric) The AS in the BGP route to the peer used for diversifying peer selection\n"
< +            "                                         (only available if the asmap config flag is set)\n"
<              "    \"services\":\"xxxxxxxxxxxxxxxx\",   (string) The services offered\n"
<              "    \"servicesnames\":[              (array) the services offered, in human-readable form\n"
---
>              "    \"addr\" : \"host:port\",      (string) The IP address and port of the peer\n"
>              "    \"addrbind\" : \"ip:port\",    (string) Bind address of the connection to the peer\n"
>              "    \"addrlocal\" : \"ip:port\",   (string) Local address as reported by the peer\n"
> -            "    \"mapped_as\" : \"mapped_as\", (string) The AS in the BGP route to the peer used for diversifying peer selection\n"
> +            "    \"mapped_as\" : n,             (numeric) The AS in the BGP route to the peer used for diversifying peer selection\n"
> +            "                                           (only available if the asmap config flag is set)\n"
>              "    \"services\" : \"xxxxxxxxxxxxxxxx\",   (string) The services offered\n"
>              "    \"servicesnames\" : [              (json array) the services offered, in human-readable form\n"

reACK d3ec323d1

<details><summary>Sig:</summary>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

reACK d3ec323d1
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAl5MCRYACgkQ5+KYS2KJ
yToEbA//eNfaa06Lt1kslQn4CgV6QBILrLFCcHct6/SMFDCGO1nHUAF+2m70j7Ro
PbgUaOKyuSZHreeVeoVfZtxJxjJnWhKUOugvEPCI0sMfezxznx9xSuYXxVCyJ1GG
e+sY9IWkTT/99blMqlSNAj1WVjr0p8LZlfBNDpbxJPd/zdTuqdFk0lMLsFA2/mdl
i9uDLsF9WxjfDdHb4rIL3CjUe22j+9idQuetkVZY/0BqwuQEjuSpVGeE/9ROkvyF
2RBUrIQfjcylq5NUv7NXBEX75xcvefsxv/fTKBbAU1yksKLckEAu87PLS5KscwCf
fCK5KydD/5f2bRE5Fi5t9VOmlTmj8WyMrvcd/Y3Wu/89hRQ0J70I8RYHP1xr+7TR
wgGL+Dwk4VPy34KEbZk9MZRvREm2xpqsKFl4/F8lUzYo4CYzYG/dhf2avpUUjS3O
198pydlsf6mIBXbWo2Qczzz5rzl334kY+g4T8I7B1IcZ/MlOd5RODOwCIBRXrFIN
xNJQ92S5lesLz+IkNFq0gMXp7ncPsSKEvdqyLeKD4BrKoSiS7T858fGgu/f1jnch
G+RZCXJf8fd2kF4bcu2fA/h8SiGD14NyULzPYvAog/bzqVwPBluSWr3dxRmpv6mr
sk31H1CbGGaFv31fNaVVv1zjZc1H32IpMty8TKhq0IQKqbac52c=
=s/w5
-----END PGP SIGNATURE-----

</details>

rebased

reACK 0b7ad4aba1c6a850d5ba927766ccd38cc003571c

Rebase looks cool

git range-diff 36f42e1bf..d3ec323d1 54a7ef612..0b7ad4aba
 1:  610cc93e9 =  1:  1fa86bb61 test: add feature_asmap functional tests
 2:  a8a82d5bf =  2:  499ff12f3 config: use default value in -asmap config
 3:  d7b1a5b04 =  3:  ca845a1aa config: enable passing -asmap an absolute file path
 4:  b321be999 =  4:  0ca73d89c config: separate the asmap finding and parsing checks
 5:  366600e83 =  5:  dc69b77e4 test: add functional test for an empty, unparsable asmap
 6:  d6e44f0b9 !  6:  74dea5777 logging: asmap logging and #include fixups
    @@ src/init.cpp
      #include <util/system.h>
      #include <util/threadnames.h>
      #include <util/translation.h>
    - #include <util/validation.h>
     -#include <util/asmap.h>
      #include <validation.h>
      #include <hash.h>
 7:  91ff62ff0 =  7:  295cd2b96 net: extract conditional to bool CNetAddr::IsHeNet
 8:  ab8a6cbcb =  8:  9ee5e8b0a rpc: fix getpeerinfo RPCResult `mapped_as` type
 9:  d3ec323d1 =  9:  0b7ad4aba init: move asmap code earlier in init process

reACK 0b7ad4aba1c6a850d5ba927766ccd38cc003571c

<details><summary>Sig</summary>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

reACK 0b7ad4aba1c6a850d5ba927766ccd38cc003571c
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAl5dcNAACgkQ5+KYS2KJ
yTqiug//WdU8pZVxuxH2eF+Ky8+VUPynP7btq2cwqmhtPUcp/WMRRS+i8NAPcUWm
PP4DXaNaDbb5k/+/HmxWqju1v4kJRDxNVFzsK7KZ+515YKu6Vn3Zor/uV/XIKlwm
hOwNpC3w7ypoot2r8AbjZ8kWwPvUCdPqgXKt4p3VedxQ8XGsxnVmQ0/CpyI6Szb9
A4AmjoRW+VfEp8hOuhFTsE12BHfPJqGcskxLDiKVdyV31nE7U69yEj4T83lvBLUA
BoGysXQJKjuNbMYU9W7oGJsO/iX4Gu0hdRhYpbOjgCDM3p9AqSqDHDoYDNoHLA1c
Jk6qhwsK6HKde9Xd4tDrFxN6i2iTJ3d+Maw72Fnd2MKyapoWfQIefXU8pUnQN/A7
Kz6EpfTH+Oph089tQzJhRIkMapopV7gTVsGe1d9BL5/vMW9gQ5RjevOBPgY1pPwu
zxXNYdPeewF3Pb3qQ6Qq25RC8AHCE9OYuLl2F3i0UldDTRGY69AWNN7vxjYwCGPc
3EjGCKbzjas+iSaPyTwwcioZ+KN+N/ZVJC4f98RtcSCjeYOeESVWmt7mixen6QpH
3RrrCPPiJtmF9uB313xszgKEY+mDvv1iIeLj56eYvY4q4m/pJqCelOvS3X79E/OV
W1ngpN7Yq07x6zmlsa1H00bVBn68v7HIcIjwS49xxBPgHvLV2e4=
=9FZk
-----END PGP SIGNATURE-----

</details>

rebased due to #17809

In the sentence …

"The AS in the BGP route to the peer …"

… isn't the correct technical term "AS path" rather than "BGP route"? :)

Thanks @practicalswift, taking note for a follow-up (asmap release notes), at this point I prefer to preserve the 3 ACKs. Mind acking?

@jonatack Sure! :)

ACK 1ba3e1cc21150abe632a5b82a1a38998b33815dc -- diff looks correct

ACK 1ba3e1cc21150abe632a5b82a1a38998b33815dc