[Net] asmap to improve IP bucketing in addrman - backports by furszy · Pull Request #2480 · PIVX-Project/PIVX

furszy · 2021-07-15T19:53:37Z

Decoupled from #2411, built on top of #2479. Probably the last decouple from the "road to Tor" work.

Focused on porting the ASN nodes bucketing functionality. The hearth of this work is bitcoin#16702.

Providing an asmap file that contains the IP->ASN mapping, nodes will be bucketed by AS they belong to, in order to make impossible for a node to connect to several nodes hosted in a single AS.
This is done in response to Erebus attack, but also to generally diversify the connections every node creates, especially useful when a large fraction of nodes operate under a couple of cloud providers.

List of PRs:

PRs for a follow up PR:

furszy · 2021-07-21T18:24:28Z

rebased on master, ready to go.

random-zebra

Light code review ACK (with the last 2 test cases of feature_asmap fixed).
Will do some more testing.

furszy · 2021-07-23T14:01:20Z

Updated per zebra's feedback 👌, fixed the two commented test cases :).

random-zebra

Tested (also with demo.map from sipa's repo) ACK 97e04978971d163c44e518739818af50c5bc6562

Nit:
Commit 63b6f995991c3d9a3aa3cd522ea73d0b6c634757 should be split/squashed into 1e9a8568e4948fdc6a172efcd9bc9cd844cdff94 + a69b82d4e7427419087f5847bc7ac999e8a64633

furszy · 2021-07-26T14:20:44Z

done, squashed. I missed it between all of the open works.

random-zebra

utACK a7138cfa150e8c3743bbe98588208a1eab436063

Fuzzbawls · 2021-07-27T10:22:15Z

IMHO, I think eb3eba03ebca443f254cea39465620b811ff8eb8 should be dropped and the issue fixed directly in upstream.

The extra indentation goes against the styling policies/guidelines for both us and upstream, and I would prefer to avoid intentionally (re-)introducing a policy violation that had been explicitly fixed in the past for a marginal improvement in ease of cherry-picking, especially being relatively close to upstream's HEAD in this particular file; which, given a quick glance, seems to be one of, if not the only file that has this styling discrepancy.

Fuzzbawls · 2021-07-27T12:24:26Z

Upstream PR addressing the indentation issue can be found at bitcoin#22562

furszy · 2021-07-27T22:32:06Z

yeah, it's the only file with text indentation discrepancies.

I made it because after, literally, more than two hundred commits back ported and adapted to our sources, it was becoming a pain in the ass to struggle against the same text indentation issue over and over, (with literally no benefits as it's just text indentation that can be re-added all at once at the end). Conflicting most of the file most of the times.

Saying that, now that the road to Tor path is clear (In netaddress.h history this is at 12 commits from the HEAD in #2411), it shouldn't take me much to drop the indentation change and apply the changes to every posterior commit. Will give it a quick look. It's one of those "meh" changes that no one in upstream really cared about for all of this years that we can have first for sure.

furszy · 2021-07-27T23:00:58Z

done, #2411 rebase took me a bit more but all looking good there too. Nothing to worry about.

…s corrected.

The scripts for creating a compact IP->ASN mapping are here: https://github.com/sipa/asmap Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>

Instead of using /16 netgroups to bucket nodes in Addrman for connection diversification, ASN, which better represents an actor in terms of network-layer infrastructure, is used. For testing, asmap.raw is used. It represents a minimal asmap needed for testing purposes. Adaptation of btc@ec45646de9e62b3d42c85716bfeb06d8f2b507dc with the latest version of the asmap.raw file.

-BEGIN VERIFY SCRIPT- sed --in-place'' --expression='s/NET_TOR/NET_ONION/g' $(git grep -I --files-with-matches 'NET_TOR') -END VERIFY SCRIPT- The --in-place'' hack is required for sed on macOS to edit files in-place without passing a backup extension.

If ASN bucketing is used, return a corresponding AS used in bucketing for a given peer.

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.

- allow passing an absolute file path to the -asmap config arg - update the -asmap config help - add a functional test in feature_asmap.py

and update the tests.

This is now testable after separating the asmap finding and parsing checks in the previous commit.

- 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

Remove redundant public declaration and mention if mapped_as is only available if the asmap config flag is set.

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.

furszy · 2021-07-28T13:43:54Z

rebased again, conflict solved. Ready to go.

random-zebra

re-utACK 16791f2

Fuzzbawls

ACK 16791f2

furszy self-assigned this Jul 15, 2021

furszy added Network P2P labels Jul 15, 2021

furszy added this to the 6.0.0 milestone Jul 15, 2021

random-zebra added Refactoring Tests Upstream labels Jul 17, 2021

furszy mentioned this pull request Jul 18, 2021

Road to Tor v3 support (BIP155) #2411

Merged

furszy force-pushed the 2021_ASN_IP_based_bucketing branch 2 times, most recently from 33ae75b to 7fead2c Compare July 21, 2021 16:39

furszy requested a review from Fuzzbawls July 22, 2021 14:09

random-zebra added the RPC label Jul 23, 2021

random-zebra reviewed Jul 23, 2021

View reviewed changes

Comment thread src/serialize.h Outdated

Comment thread test/functional/feature_asmap.py Outdated

Comment thread test/functional/feature_asmap.py Outdated

random-zebra added the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Jul 23, 2021

furszy force-pushed the 2021_ASN_IP_based_bucketing branch from 7fead2c to 334df2e Compare July 23, 2021 13:59

furszy force-pushed the 2021_ASN_IP_based_bucketing branch 2 times, most recently from 13915de to 97e0497 Compare July 26, 2021 09:11

random-zebra previously approved these changes Jul 26, 2021

View reviewed changes

furszy dismissed random-zebra’s stale review via a7138cf July 26, 2021 14:19

furszy force-pushed the 2021_ASN_IP_based_bucketing branch from 97e0497 to a7138cf Compare July 26, 2021 14:19

random-zebra previously approved these changes Jul 26, 2021

View reviewed changes

furszy dismissed random-zebra’s stale review via 011d033 July 27, 2021 22:37

furszy force-pushed the 2021_ASN_IP_based_bucketing branch from a7138cf to 011d033 Compare July 27, 2021 22:37

furszy and others added 22 commits July 28, 2021 10:40

addrman.h: CAddrInfo inline members default values, plus several typo…

d2a8baf

…s corrected.

CAddrMan::Deserialize handle corrupt serializations better.

e986ed0

Add asmap utility which queries a mapping

8d01cbd

The scripts for creating a compact IP->ASN mapping are here: https://github.com/sipa/asmap Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>

Tests: address placement should be deterministic by default

fd51941

CAddrManTest: remove redundant MakeDeterministic call.

718f1df

Return mapped AS in RPC call getpeerinfo

2fe5a05

If ASN bucketing is used, return a corresponding AS used in bucketing for a given peer.

Add extra logging of asmap use and bucketing

cb698fb

Avoid asmap copies in initialization

868a6ed

Mark asmap const in statistics code

9a28bc0

Use ASNs for mapped IPv4 addresses correctly

e527e04

Make asmap Interpret tolerant of malicious map data

4290d3f

config: use default value in -asmap config

8c7bdbe

config: enable passing -asmap an absolute file path

618b8d1

- allow passing an absolute file path to the -asmap config arg - update the -asmap config help - add a functional test in feature_asmap.py

config: separate the asmap finding and parsing checks

6545656

and update the tests.

test: add functional test for an empty, unparsable asmap

0c9efb8

This is now testable after separating the asmap finding and parsing checks in the previous commit.

logging: asmap logging and #include fixups

2fc1f37

- 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

net: extract conditional to bool CNetAddr::IsHeNet

65cd143

Remove redundant public declaration and mention if mapped_as is only available if the asmap config flag is set.

CMakeLists tests: add raw files generation.

16791f2

furszy force-pushed the 2021_ASN_IP_based_bucketing branch from 011d033 to 16791f2 Compare July 28, 2021 13:42

random-zebra approved these changes Jul 28, 2021

View reviewed changes

Fuzzbawls approved these changes Jul 30, 2021

View reviewed changes

random-zebra merged commit 85f000e into PIVX-Project:master Jul 30, 2021

random-zebra mentioned this pull request Jul 30, 2021

[Tests] Require exact match in assert_raises_init_error #2489

Merged

random-zebra removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Aug 24, 2021

furszy deleted the 2021_ASN_IP_based_bucketing branch November 29, 2022 14:33

Uh oh!

Conversation

furszy commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

List of PRs:

Uh oh!

furszy commented Jul 21, 2021

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy commented Jul 23, 2021

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

furszy commented Jul 26, 2021

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Fuzzbawls commented Jul 27, 2021

Uh oh!

Fuzzbawls commented Jul 27, 2021

Uh oh!

furszy commented Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy commented Jul 27, 2021

Uh oh!

furszy commented Jul 28, 2021

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Fuzzbawls left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

furszy commented Jul 15, 2021 •

edited

Loading

furszy commented Jul 27, 2021 •

edited

Loading