p2p: ip bucketing in addrman via asmap #3229

xanimo · 2023-02-26T22:20:39Z

This is a backport of bitcoin's 16702 pull request with a minimal subset of other cherry-picks needed in order to get it up and running.

patricklodder · 2023-02-26T22:41:14Z

@xanimo am I correct in my assumption that this PR also has a peer.dat downgrade issue as we discussed in #3149 (comment)?

I think this PR would give us an additional reason to make sure that we release a patch to 1.14.6 once we're done with the build system upgrade - correct?

xanimo · 2023-02-26T22:47:03Z

@patricklodder i've not tested that actually so will need to do that when I get home however yes I agree on the patch post build system upgrade.

xanimo · 2023-02-27T00:25:37Z

@patricklodder Hmm not certain if I'm doing it right but it would appear there are no errors from 1.14.6 to 1.14.7 and vice versa with regards to the peers.dat issue found in #3149.

patricklodder · 2023-02-27T00:48:59Z

Thank you, that is hopeful. I'll make it part of testing to be sure.

xanimo · 2023-02-27T01:18:06Z

Tested from this branch:

2023-02-27 01:08:52 init message: Loading addresses...
2023-02-27 01:08:52 ERROR: Read: Failed to open file /home/bluezr/.dogecoin/peers.dat
2023-02-27 01:08:52 Invalid or missing peers.dat; recreating

to 1.14.6 with debugging:

2023-02-27 01:09:33 init message: Loading addresses...
2023-02-27 01:09:33 Loaded 4828 addresses from peers.dat  25ms

and vice versa (1.14.6):

2023-02-27 01:14:22 init message: Loading addresses...
2023-02-27 01:14:22 ERROR: Read: Failed to open file /home/bluezr/.dogecoin/peers.dat
2023-02-27 01:14:22 Invalid or missing peers.dat; recreating

to this branch:

2023-02-27 01:15:26 init message: Loading addresses...
2023-02-27 01:15:26 Loaded 1767 addresses from peers.dat  8ms

Am still not sure if I'm doing it right though 🤷 anyway hope this helps.

patricklodder · 2023-06-28T21:43:11Z

@xanimo would you mind rebasing this on top of the current 1.14.7-dev (502a8a9 or later)?

xanimo · 2023-06-28T21:44:00Z

@xanimo would you mind rebasing this on top of the current 1.14.7-dev (502a8a9 or later)?

absolutely, will do shortly!

edtubbs · 2023-07-28T20:30:53Z

Initial tests with make check and qa/pull-tester/rpc-tests.py look good. Need to complete the review of code changes and tests. Very cool!

patricklodder · 2024-02-28T16:07:31Z

Are you resubmitting this later?

I was still reviewing this and trying to find solutions for observed problems. Spent over a month on it so far.

xanimo · 2024-02-28T18:34:29Z

Are you resubmitting this later?

I was still reviewing this and trying to find solutions for observed problems. Spent over a month on it so far.

Ah my apologies. I'll reopen and rebase on the next applicable dev branch later.

patricklodder · 2024-02-28T18:40:15Z

No worries. Apologies from my end for not giving quick feedback. The puzzle on this one has been real.

I'm currently at the point of backtracking everything I did to make it work because I was all over the place. Will comment when I've untangled my mess.

xanimo · 2024-02-28T18:44:57Z

Sounds good to me, keep me posted. I'll reopen shortly.

patricklodder

Test results

I tested this extensively and there are a number of bugs that Bitcoin Core has fixed that need to be backported, because the proposed code contains a bug that jumps too far sometimes, with the result that I saw AWS nodes reported as if they are ran by the US military (which is how I spotted this because I was wondering why Naval bases on both the East and West coast were running dogecoind.)

There are also some known vulnerabilities in the code as ported still.

Commits from upstream that I'd like to see back ported into this, so that we're not in limbo with bad AS detection (that peering decisions are made upon):

Prereqs

At least 2 things backported here should be separated out, I think:

c943a69 (orig: 19e36bb) introduces an fs wrapper that is best used throughout the codebase: I suggest we split this off and implement the whole change, not just this commit
eec1ef5 and 7325b05 are also part of a bigger change and would benefit from separate and full implementation, see:

patricklodder · 2023-12-23T20:04:15Z

src/test/addrman_tests.cpp

+
+BOOST_AUTO_TEST_CASE(addrman_serialization)
+{
+    std::vector<bool> asmap1 = FromBytes(asmap, sizeof(asmap) * 8);


asmap here has a scope issue with gcc 12 / boost 1.74, should be: asmap_raw

patricklodder · 2023-12-23T20:04:35Z

src/test/addrman_tests.cpp

    uint256 nKey2 = (uint256)(CHashWriter(SER_GETHASH, 0) << 2).GetHash();

-    BOOST_CHECK(info1.GetNewBucket(nKey1) == 786);
+    std::vector<bool> asmap1 = FromBytes(asmap, sizeof(asmap) * 8);


asmap here has a scope issue with gcc 12 / boost 1.74, should be: asmap_raw

patricklodder · 2023-12-23T20:05:31Z

src/test/addrman_tests.cpp

    uint256 nKey1 = (uint256)(CHashWriter(SER_GETHASH, 0) << 1).GetHash();
    uint256 nKey2 = (uint256)(CHashWriter(SER_GETHASH, 0) << 2).GetHash();

+    std::vector<bool> asmap1 = FromBytes(asmap, sizeof(asmap) * 8);


asmap here has a scope issue with gcc 12 / boost 1.74, should be: asmap_raw

xanimo · 2024-03-22T19:08:22Z

Awesome I'll get to work on this over the weekend. Thanks!

Also very interesting re east/west coast naval bases/military running dogecoind...

patricklodder · 2024-03-22T19:24:15Z

If you like, I can pick up the fs wrapper in parallel - it also helps with std++17 transition.

xanimo · 2024-04-05T19:14:55Z

Force pushed after rebase of #3487 and #3511.

patricklodder · 2024-06-19T00:38:55Z

I suspect that after merging #3521, this PR should be fully enabled (from 82f372d onward)? Is there anything else needed as a prerequisite?

xanimo · 2024-06-19T00:42:49Z

I don't believe there are any remaining prerequisites. Going to rebase off #3521 and upstream/1.15.0-dev to verify.

xanimo · 2024-06-19T01:02:15Z

Just passed unit tests on x86_64-pc-linux-gnu so I think we're good to go 🤞

patricklodder · 2024-06-19T01:48:31Z

Test plan:

Spend some time with fjahr/kartograf and build a more reliable asmap file
Test concurrently with less reliable asmap files built with other scripts in previous rounds
Parse peers.dat and look up how reliably AS are recognized with this version
Upgrade/downgrade re-checks

I'll share files/results.

We currently do two resolves for dns seeds: one for the results, and one to serve in addrman as the source for those addresses. There's no requirement that the source hostname resolves to the stored identifier, only that the mapping is unique. So rather than incurring the second lookup, combine a private subnet with a hash of the hostname. The resulting v6 ip is guaranteed not to be publicy routable, and has only a negligible chance of colliding with a user's internal network (which would be of no consequence anyway). Cherry-picked from: 7f31762

Cherry-picked from: 92528c2

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> Cherry-picked from: 8feb4e4

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. Cherry-picked from: ec45646

If ASN bucketing is used, return a corresponding AS used in bucketing for a given peer. Cherry-picked from: e4658aa

Cherry-picked from: 3c1bc40

patricklodder added this to the 1.14.7 milestone Feb 26, 2023

patricklodder added the enhancement label Feb 26, 2023

patricklodder requested a review from a team February 26, 2023 22:24

xanimo force-pushed the 1.14.7-dev-asmap branch from 672455c to cf79f16 Compare February 26, 2023 23:41

patricklodder added the rebase needed label Jun 28, 2023

xanimo force-pushed the 1.14.7-dev-asmap branch from cf79f16 to 4a3993d Compare June 28, 2023 21:58

patricklodder removed the rebase needed label Jun 28, 2023

eztub approved these changes Aug 16, 2023

View reviewed changes

patricklodder removed this from the 1.14.7 milestone Feb 16, 2024

xanimo closed this Feb 28, 2024

xanimo deleted the 1.14.7-dev-asmap branch February 28, 2024 09:16

xanimo restored the 1.14.7-dev-asmap branch February 28, 2024 18:52

xanimo reopened this Feb 28, 2024

xanimo changed the base branch from 1.14.7-dev to 1.15.0-dev February 28, 2024 18:54

patricklodder added this to the 1.15.0 milestone Mar 9, 2024

patricklodder reviewed Mar 22, 2024

View reviewed changes

xanimo force-pushed the 1.14.7-dev-asmap branch 2 times, most recently from dff6167 to f1f6062 Compare April 6, 2024 20:19

xanimo force-pushed the 1.14.7-dev-asmap branch from f1f6062 to ffe68e0 Compare April 15, 2024 19:38

patricklodder added the rebase needed label Jun 19, 2024

xanimo force-pushed the 1.14.7-dev-asmap branch from ffe68e0 to 3730899 Compare June 19, 2024 01:00

patricklodder changed the base branch from 1.15.0-dev to master August 9, 2024 00:49

patricklodder removed this from the 1.15.0 milestone Oct 13, 2024

patricklodder moved this from 🚀 needs review to 🔨needs rework in Review & merge board Mar 31, 2025

theuni and others added 14 commits July 5, 2025 18:29

Support serialization of std::vector<bool>

578870f

Cherry-picked from: 92528c2

Add asmap utility which queries a mapping

ccd0ef9

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> Cherry-picked from: 8feb4e4

Return mapped AS in RPC call getpeerinfo

abc5089

If ASN bucketing is used, return a corresponding AS used in bucketing for a given peer. Cherry-picked from: e4658aa

Add extra logging of asmap use and bucketing

9446b0d

Cherry-picked from: 3c1bc40

Avoid asmap copies in initialization

330b659

Mark asmap const in statistics code

412433a

Make asmap Interpret tolerant of malicious map data

0507bab

Introduce Instruction enum in asmap

716172c

Deal with decoding failures explicitly in asmap Interpret

cd1a8bd

Improve asmap Interpret checks and document failures

ba8890d

Add asmap sanity checker

7406278

Add additional effiency checks to sanity checker

bdb4884

xanimo force-pushed the 1.14.7-dev-asmap branch from 3730899 to bdb4884 Compare July 6, 2025 03:44

patricklodder moved this from 🔨needs rework to 🚀 needs review in Review & merge board Jul 22, 2025

patricklodder removed the rebase needed label Jul 22, 2025

p2p: ip bucketing in addrman via asmap #3229

Are you sure you want to change the base?

p2p: ip bucketing in addrman via asmap #3229

Uh oh!

Conversation

xanimo commented Feb 26, 2023

Uh oh!

patricklodder commented Feb 26, 2023

Uh oh!

xanimo commented Feb 26, 2023

Uh oh!

xanimo commented Feb 27, 2023

Uh oh!

patricklodder commented Feb 27, 2023

Uh oh!

xanimo commented Feb 27, 2023

Uh oh!

patricklodder commented Jun 28, 2023

Uh oh!

xanimo commented Jun 28, 2023

Uh oh!

edtubbs commented Jul 28, 2023

Uh oh!

patricklodder commented Feb 28, 2024

Uh oh!

xanimo commented Feb 28, 2024

Uh oh!

patricklodder commented Feb 28, 2024

Uh oh!

xanimo commented Feb 28, 2024

Uh oh!

patricklodder left a comment

Choose a reason for hiding this comment

Test results

Prereqs

Uh oh!

patricklodder Dec 23, 2023

Choose a reason for hiding this comment

Uh oh!

patricklodder Dec 23, 2023

Choose a reason for hiding this comment

Uh oh!

patricklodder Dec 23, 2023

Choose a reason for hiding this comment

Uh oh!

xanimo commented Mar 22, 2024

Uh oh!

patricklodder commented Mar 22, 2024

Uh oh!

xanimo commented Apr 5, 2024

Uh oh!

patricklodder commented Jun 19, 2024

Uh oh!

xanimo commented Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xanimo commented Jun 19, 2024

Uh oh!

patricklodder commented Jun 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

xanimo commented Jun 19, 2024 •

edited

Loading