jonatack
commented at 1:15 pm on December 28, 2019:
member
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.
jonatack force-pushed
on Dec 28, 2019
jonatack force-pushed
on Dec 28, 2019
fanquake added the label
P2P
on Dec 28, 2019
jonatack force-pushed
on Dec 28, 2019
jonatack force-pushed
on Dec 28, 2019
DrahtBot
commented at 5:30 pm on December 28, 2019:
member
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
jonatack force-pushed
on Dec 28, 2019
fanquake
commented at 9:49 pm on December 28, 2019:
member
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.
fanquake renamed this:
net, test: asmap functional tests and feature refinements
test: asmap functional tests and feature refinements
on Dec 28, 2019
fanquake added the label
Tests
on Dec 28, 2019
jonatack
commented at 10:18 pm on December 28, 2019:
member
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.
jonatack force-pushed
on Dec 30, 2019
jonatack renamed this:
test: asmap functional tests and feature refinements
config, test: asmap functional tests and feature refinements
on Dec 30, 2019
jonatack force-pushed
on Dec 30, 2019
jonatack force-pushed
on Dec 30, 2019
jonatack force-pushed
on Dec 30, 2019
jonatack closed this
on Jan 6, 2020
jonatack reopened this
on Jan 6, 2020
laanwj
commented at 1:10 pm on January 15, 2020:
member
+1 for allowing absolute asmap paths
DrahtBot added the label
Needs rebase
on Jan 29, 2020
jonatack force-pushed
on Jan 30, 2020
DrahtBot removed the label
Needs rebase
on Jan 30, 2020
DrahtBot added the label
Needs rebase
on Jan 30, 2020
jonatack force-pushed
on Jan 30, 2020
jonatack
commented at 11:11 pm on January 30, 2020:
member
fanquake removed the label
Needs rebase
on Jan 30, 2020
fanquake
commented at 0:17 am on January 31, 2020:
member
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.
gmaxwell
commented at 0:18 am on January 31, 2020:
contributor
Has anyone fuzzed the asmap file reader? I anticipate users accepting asmaps from third parties…
gmaxwell
commented at 0:19 am on January 31, 2020:
contributor
jonatack
commented at 10:55 am on January 31, 2020:
member
@fanquake thanks for having a look and the suggestions. Done, squashed all those down into e30a4691bacc232a984e049fb887b08860fa7c94.
jonatack force-pushed
on Jan 31, 2020
jonatack force-pushed
on Jan 31, 2020
laanwj
commented at 1:38 pm on February 5, 2020:
member
Needs rebase (due to sipa’s asmap changes in #18023, I think)
DrahtBot added the label
Needs rebase
on Feb 5, 2020
jonatack force-pushed
on Feb 6, 2020
jonatack force-pushed
on Feb 6, 2020
jonatack renamed this:
config, test: asmap functional tests and feature refinements
config, net, test: asmap functional tests and feature refinements
on Feb 6, 2020
jonatack
commented at 4:11 pm on February 6, 2020:
member
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’.
DrahtBot removed the label
Needs rebase
on Feb 6, 2020
in
src/init.cpp:412
in
614b082b01outdated
408@@ -409,7 +409,7 @@ void SetupServerArgs()
409 ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
410411 gArgs.AddArg("-addnode=<ip>", "Add a node to connect to and attempt to keep the connection open (see the `addnode` RPC command help for more info). This option can be specified multiple times to add multiple nodes.", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
412- gArgs.AddArg("-asmap=<file>", strprintf("Specify asn mapping used for bucketing of the peers (default: %s). Path should be relative to the -datadir path.", DEFAULT_ASMAP_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
413+ 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);
jonatack
commented at 8:36 am on February 7, 2020:
Done.
in
src/rpc/net.cpp:86
in
1d0c754f8aoutdated
82@@ -83,7 +83,7 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
83 " \"addr\":\"host:port\", (string) The IP address and port of the peer\n"
84 " \"addrbind\":\"ip:port\", (string) Bind address of the connection to the peer\n"
85 " \"addrlocal\":\"ip:port\", (string) Local address as reported by the peer\n"
86- " \"mapped_as\":\"mapped_as\", (string) The AS in the BGP route to the peer used for diversifying peer selection\n"
87+ " \"mapped_as\":\"mapped_as\", (numeric) The AS in the BGP route to the peer used for diversifying peer selection\n"
If this is numeric, the \" around mapped_as should go as well.
jonatack
commented at 8:36 am on February 7, 2020:
Good catch – done. Replaced with \"mapped_as\": n,
jonatack force-pushed
on Feb 7, 2020
jonatack
commented at 8:48 am on February 7, 2020:
member
Thank you @sipa for reviewing, updated as per git diff 11a1028..1e26a1a
0diff --git a/src/addrman.cpp b/src/addrman.cpp
1index ef34edc0a0..2f8a3a0bd5 100644
2- LogPrint(BCLog::NET, "IP %s mapped to AS %i belongs to tried bucket %i\n", ToStringIP(), mapped_as, tried_bucket);
3+ LogPrint(BCLog::NET, "IP %s mapped to AS%i belongs to tried bucket %i\n", ToStringIP(), mapped_as, tried_bucket);
4 5- LogPrint(BCLog::NET, "IP %s mapped to AS %i belongs to new bucket %i\n", ToStringIP(), mapped_as, new_bucket);
6+ LogPrint(BCLog::NET, "IP %s mapped to AS%i belongs to new bucket %i\n", ToStringIP(), mapped_as, new_bucket);
7 8diff --git a/src/init.cpp b/src/init.cpp
9index 6591852a7f..7b5e536b8f 100644
10- 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);
11+ 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);
1213diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
14index 59d299e749..2f66fb9299 100644
15- " \"mapped_as\":\"mapped_as\", (numeric) The AS in the BGP route to the peer used for diversifying peer selection\n"
16+ " \"mapped_as\": n, (numeric) The AS in the BGP route to the peer used for diversifying peer selection\n"
17+ " (only available if the asmap config flag is set)\n"
in
test/functional/feature_asmap.py:60
in
1e26a1af96outdated
pinheadmz
commented at 3:59 pm on February 11, 2020:
Are you sure it’s ok to have a space in a command line argument? Some OSes/shells might interpret this as -asmap=ASN and stop parsing, but I’m not sure
jonatack
commented at 4:05 pm on February 11, 2020:
The CI builds didn’t have an issue, but since there are no ACKs yet to lose, I can change it.
in
test/functional/feature_asmap.py:86
in
1e26a1af96outdated
81+
82+ def test_empty_asmap(self):
83+ self.log.info('Test bitcoind -asmap with empty map file')
84+ self.stop_node(0)
85+ with open(self.default_asmap, "w", encoding="utf-8") as f:
86+ f.write("")
pinheadmz
commented at 4:08 pm on February 11, 2020:
I tried running this test with junk data here (f.write("helloworld")) bitcoind didn’t mind interpreting that junk as a valid asmap file, didn’t throw the expected parsing error. I suspect that’s because the map file is just raw bytes, is there no way to check it for integrity?
jonatack
commented at 4:28 pm on February 11, 2020:
I think that’s outside the scope of this PR; perhaps as a follow-up.
pinheadmz
commented at 4:39 pm on February 11, 2020:
member
ACK1e26a1af96438edf96ecb2268569d5bec4a7f43e
optional nit #17812 (review) happy to reack if you decide to address that
jonatack force-pushed
on Feb 11, 2020
jonatack
commented at 7:18 pm on February 11, 2020:
member
Thanks @pinheadmz for reviewing, updated as per git diff 1e26a1a..dccc6bf:
0test/functional/feature_asmap.py
1- name = 'ASN map'
2+ name = 'ASN_map'
pinheadmz
commented at 7:43 pm on February 11, 2020:
member
ACk dccc6bfc04e43bbdf8db35e0fb7288231b05973f
naumenkogs
commented at 7:02 pm on February 13, 2020:
member
Great work, thanks! Especially those tests.
ACKdccc6bfc04e43bbdf8db35e0fb7288231b05973f
luke-jr referenced this in commit
75798ad061
on Feb 16, 2020
luke-jr referenced this in commit
44a2a4c35c
on Feb 16, 2020
luke-jr referenced this in commit
5ac01f769f
on Feb 16, 2020
luke-jr referenced this in commit
821809b64c
on Feb 16, 2020
luke-jr referenced this in commit
96a9bdba15
on Feb 16, 2020
luke-jr referenced this in commit
306f89acee
on Feb 16, 2020
luke-jr referenced this in commit
7a552eb8f5
on Feb 16, 2020
luke-jr referenced this in commit
58a0c9dcc2
on Feb 16, 2020
luke-jr referenced this in commit
e8bb88a82d
on Feb 16, 2020
MarcoFalke
commented at 1:54 am on February 17, 2020:
member
0< " \"addr\":\"host:port\", (string) The IP address and port of the peer\n"
1< " \"addrbind\":\"ip:port\", (string) Bind address of the connection to the peer\n"
2< " \"addrlocal\":\"ip:port\", (string) Local address as reported by the peer\n"
3< - " \"mapped_as\":\"mapped_as\", (string) The AS in the BGP route to the peer used for diversifying peer selection\n"
4< + " \"mapped_as\": n, (numeric) The AS in the BGP route to the peer used for diversifying peer selection\n"
5< + " (only available if the asmap config flag is set)\n"
6< " \"services\":\"xxxxxxxxxxxxxxxx\", (string) The services offered\n"
7< " \"servicesnames\":[ (array) the services offered, in human-readable form\n"
8---
9> " \"addr\" : \"host:port\", (string) The IP address and port of the peer\n"
10> " \"addrbind\" : \"ip:port\", (string) Bind address of the connection to the peer\n"
11> " \"addrlocal\" : \"ip:port\", (string) Local address as reported by the peer\n"
12> - " \"mapped_as\" : \"mapped_as\", (string) The AS in the BGP route to the peer used for diversifying peer selection\n"
13> + " \"mapped_as\" : n, (numeric) The AS in the BGP route to the peer used for diversifying peer selection\n"
14> + " (only available if the asmap config flag is set)\n"
15> " \"services\" : \"xxxxxxxxxxxxxxxx\", (string) The services offered\n"
16> " \"servicesnames\" : [ (json array) the services offered, in human-readable form\n"
to verify node behaviour and debug log when launching bitcoind in these cases:
1. `bitcoind` with no -asmap arg, using /16 prefix for IP bucketing
2. `bitcoind -asmap=<relative path>`, using the unit test skeleton asmap
3. `bitcoind -asmap/-asmap=` with no file specified, using the default asmap
4. `bitcoind -asmap` with no file specified, and a missing default asmap file
The tests are order-independent. The slowest test (missing default asmap file)
is placed last.
08b992675c
config: use default value in -asmap config
and move to sorted position
fbe9b024f0
config: enable passing -asmap an absolute file path
- allow passing an absolute file path to the -asmap config arg
- update the -asmap config help
- add a functional test in feature_asmap.py
81c38a2497
config: separate the asmap finding and parsing checks
and update the tests.
b8d0412b21
test: add functional test for an empty, unparsable asmap
This is now testable after separating the asmap finding and parsing checks in
the previous commit.
dcaf543ba0
logging: asmap logging and #include fixups
- move asmap #includes to sorted positions in addrman and init (move-only)
- remove redundant quotes in asmap InitError, update test
- remove full stops from asmap logging to be consistent with debug logging,
update tests
819fb5549b
net: extract conditional to bool CNetAddr::IsHeNet
and remove redundant public declaration
c90b9a2399
rpc: fix getpeerinfo RPCResult `mapped_as` type
and mention it is only available if the asmap config flag is set.
5ba829e12e
init: move asmap code earlier in init process
and update feature_asmap.py and test_runner.py
This commit moves the asmap init.cpp code from the end of "Step 12: start node"
to "Step 6: network initialization" to provide feedback on passing an -asmap
config arg much more quickly. This change speeds up the feature_asmap.py
functional test file from 60 to 5 seconds by accelerating the 2 tests that use
`assert_start_raises_init_error`.
Credit to Wladimir J. van der Laan for the suggestion.
1ba3e1cc21
jonatack force-pushed
on Mar 4, 2020
jonatack
commented at 2:00 pm on March 4, 2020:
member
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-12-18 09:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
