Skip to content

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

Merged
fanquake merged 9 commits intobitcoin:masterfrom
jonatack:feature-asmap
Mar 5, 2020
Merged

config, net, test: asmap feature refinements and functional tests#17812
fanquake merged 9 commits intobitcoin:masterfrom
jonatack:feature-asmap

Conversation

@jonatack
Copy link
Member

@jonatack jonatack commented Dec 28, 2019 •
edited
Loading

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 p2p: supplying and using asmap to improve IP bucketing in addrman #16702 (comment), 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 jonatack force-pushed the feature-asmap branch 2 times, most recently from 8fc5170 to 734d531 Compare December 28, 2019 13:22
@fanquake fanquake added the P2P label Dec 28, 2019
@jonatack jonatack force-pushed the feature-asmap branch 2 times, most recently from d60253f to 91258da Compare December 28, 2019 14:50
@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 28, 2019 •
edited
Loading

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

Conflicts

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.

@fanquake
Copy link
Member

fanquake commented Dec 28, 2019

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 fanquake changed the title net, test: asmap functional tests and feature refinements test: asmap functional tests and feature refinements Dec 28, 2019
@fanquake fanquake added the Tests label Dec 28, 2019
@jonatack
Copy link
Member Author

jonatack commented Dec 28, 2019 •
edited
Loading

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 jonatack changed the title test: asmap functional tests and feature refinements config, test: asmap functional tests and feature refinements Dec 30, 2019
@jonatack jonatack mentioned this pull request Dec 30, 2019
Merged
@jonatack jonatack force-pushed the feature-asmap branch 2 times, most recently from 6f7bb4c to 7cf4926 Compare December 30, 2019 20:06
@jonatack jonatack closed this Jan 6, 2020
@jonatack jonatack reopened this Jan 6, 2020
@laanwj
Copy link
Member

laanwj commented Jan 15, 2020

+1 for allowing absolute asmap paths

@jonatack
Copy link
Member Author

jonatack commented Jan 30, 2020

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

@fanquake
Copy link
Member

fanquake commented Jan 31, 2020

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
Copy link
Contributor

gmaxwell commented Jan 31, 2020

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

@gmaxwell
Copy link
Contributor

gmaxwell commented Jan 31, 2020

Oh, nevermind, I just saw #18033

@practicalswift
Copy link
Contributor

practicalswift commented Mar 4, 2020 •
edited
Loading

In the sentence …

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

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

@jonatack
Copy link
Member Author

jonatack commented Mar 5, 2020

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

@practicalswift
Copy link
Contributor

practicalswift commented Mar 5, 2020

@jonatack Sure! :)

ACK 1ba3e1c -- diff looks correct

fanquake
fanquake approved these changes Mar 5, 2020
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 1ba3e1c

@fanquake fanquake merged commit d0601e6 into bitcoin:master Mar 5, 2020
@jonatack jonatack deleted the feature-asmap branch March 5, 2020 12:18
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 30, 2020
@jonatack @Fabcien
config, net, test: asmap feature refinements and functional tests
930510c 
Summary:
```
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.
```

Backport of core [[bitcoin/bitcoin#17812 | PR17812]].

Depends on D8198.

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, jasonbcox

Reviewed By: #bitcoin_abc, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D8200
@jonatack jonatack mentioned this pull request Feb 15, 2021
Closed
@kwvg kwvg mentioned this pull request Mar 8, 2021
Closed
@kwvg kwvg mentioned this pull request Mar 9, 2021
Merged
@furszy furszy mentioned this pull request Jun 7, 2021
Merged
@furszy furszy mentioned this pull request Jul 15, 2021
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sipa sipa sipa left review comments

@fanquake fanquake fanquake approved these changes

+1 more reviewer

@pinheadmz pinheadmz pinheadmz left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@jonatack @DrahtBot @fanquake @laanwj @gmaxwell @pinheadmz @naumenkogs @maflcko @practicalswift @sipa