net: Encapsulate asmap in NetGroupManager

jnewbery commented at 2:39 pm on September 7, 2021: member

The asmap data is currently owned by addrman, but is used by both addrman and connman. #22791 made the data const and private (so that it can’t be updated by other components), but it is still passed out of addrman as a reference to const, and used by CNetAddress to calculate the group and AS of the net address.

This RFC PR proposes to move all asmap data and logic into a new NetGroupManager component. This is initialized at startup, and the client components addrman and connman simply call NetGroupManager::GetGroup(const CAddress&) and NetGroupManager::GetMappedAS(const CAddress&) to get the net group and AS of an address.

jnewbery force-pushed on Sep 7, 2021

DrahtBot added the label Build system on Sep 7, 2021

DrahtBot added the label P2P on Sep 7, 2021

DrahtBot added the label Utils/log/libs on Sep 7, 2021

DrahtBot commented at 7:54 pm on September 7, 2021: 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:

#24925 (refactor: make GetRand a template, remove GetRandInt by PastaPastaPasta by PastaPastaPasta)
#23531 (Add Yggdrasil support by prusnak)
#22563 (addrman: treat Tor/I2P/CJDNS as a single group by vasild)
#21878 (Make all networking code mockable by vasild)
#19860 (p2p: Improve diversification of new connections by naumenkogs)
#13462 (Make SER_GETHASH implicit for CHashWriter and SerializeHash by Empact)

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.

sipa commented at 9:07 pm on September 7, 2021: member

Concept ACK (just judging the PR description)

practicalswift commented at 7:55 am on September 8, 2021: contributor

Concept ACK

naumenkogs commented at 7:58 am on September 8, 2021: member

Concept ACK, light code review ACK d70b3541cbca6c4f5b5552ec5d3fbd59bc51749a

in src/test/fuzz/addrman.cpp:111 in d70b3541cb outdated

107@@ -101,7 +108,7 @@ class CAddrManDeterministic : public CAddrMan
108                 if (n > 0 && mapInfo.size() % n == 0 && mapAddr.find(addr) == mapAddr.end()) {
109                     // Add to the "tried" table (if the bucket slot is free).
110                     const CAddrInfo dummy{addr, source};
111-                    const int bucket = dummy.GetTriedBucket(nKey, m_asmap);
112+                    const int bucket = dummy.GetTriedBucket(nKey, *g_setup->m_node.netgroupman);

mzumsande commented at 10:35 pm on September 8, 2021:

I think this should use m_netgroupman which may have been fuzzed prior to calling Fill(), instead of the empty *g_setup->m_node.netgroupman, see CI fuzz test fail.

jnewbery commented at 9:50 am on September 10, 2021:

You’re absolutely right, of course. Now fixed.

mzumsande commented at 10:38 pm on September 8, 2021: member

Concept ACK

fanquake referenced this in commit 5446070418 on Sep 10, 2021

jnewbery force-pushed on Sep 10, 2021

jnewbery commented at 9:50 am on September 10, 2021: member

Rebased and fixed #22910 (review)

jnewbery force-pushed on Sep 10, 2021

jnewbery commented at 10:53 am on September 10, 2021: member

rerebased

MarcoFalke removed the label Build system on Sep 10, 2021

amitiuttarwar commented at 3:00 pm on September 10, 2021: contributor

concept ACK

jnewbery commented at 1:01 pm on September 28, 2021: member

I’m planning to leave this as a draft PR until #22950 is merged, since I think that’s more important to get done.

jnewbery force-pushed on Oct 7, 2021

jnewbery commented at 4:19 pm on October 7, 2021: member

Rebased on master

in src/bench/addrman.cpp:25 in 4d080b386e outdated

19@@ -19,6 +20,13 @@ static constexpr size_t NUM_ADDRESSES_PER_SOURCE = 256;
20 static std::vector<CAddress> g_sources;
21 static std::vector<std::vector<CAddress>> g_addresses;
22 
23+static NetGroupManager g_netgroupman{std::vector<bool>()};
24+
25+static CAddrMan CreateAddrMan()

MarcoFalke commented at 8:23 am on October 8, 2021:

remove C

0static AddrMan CreateAddrMan()

jnewbery commented at 12:56 pm on October 8, 2021:

Oops. Pesky benches. Fixed.

jnewbery force-pushed on Oct 8, 2021

jnewbery commented at 1:09 pm on October 8, 2021: member

rebased on master

jnewbery force-pushed on Oct 8, 2021

DrahtBot added the label Needs rebase on Dec 13, 2021

jnewbery force-pushed on Dec 22, 2021

jnewbery commented at 10:45 pm on December 22, 2021: member

I’ve rebased this on top of #23826, since they conflict quite heavily. Leaving as draft for now.

DrahtBot removed the label Needs rebase on Dec 23, 2021

DrahtBot added the label Needs rebase on Jan 4, 2022

jnewbery force-pushed on Jan 5, 2022

jnewbery commented at 3:54 pm on January 5, 2022: member

Rebased on master

DrahtBot removed the label Needs rebase on Jan 5, 2022

jnewbery force-pushed on Jan 6, 2022

jnewbery marked this as ready for review on Jan 6, 2022

jnewbery force-pushed on Jan 6, 2022

jnewbery commented at 11:02 am on January 6, 2022: member

I’ve made some very minor style/comment changes to comments, and marked this as ready for review. I was concerned that commit 76339d1750 [net] Only use public CNetAddr functions and data in GetMappedAS() and GetGroup() could potentially introduce some subtle behaviour changes, but I’ve re-reviewed and think that it’s safe. Reviewers should pay attention to the change from m_addr to addr_bytes = address.GetAddrBytes() in that commit.

DrahtBot added the label Needs rebase on Jan 17, 2022

jnewbery force-pushed on Jan 20, 2022

jnewbery commented at 5:27 pm on January 20, 2022: member

Rebased

DrahtBot removed the label Needs rebase on Jan 20, 2022

jnewbery renamed this:
~~[RFC] Encapsulate asmap in NetGroupManager~~
net: Encapsulate asmap in NetGroupManager
on Jan 23, 2022

naumenkogs commented at 8:01 am on March 10, 2022: member

ACK a3def07ce573fd216e6b8b41d93bb8218024ba74

oops, see below

MarcoFalke commented at 8:16 am on March 10, 2022: member

(This doesn’t compile when rebased on current master, see CI)

jnewbery force-pushed on Mar 10, 2022

jnewbery commented at 9:28 pm on March 10, 2022: member

Thanks @MarcoFalke. I’ve rebased on latest master and resolved the silent conflict.

in src/Makefile.am:183 in 6b78d8e077 outdated

179@@ -180,6 +180,7 @@ BITCOIN_CORE_H = \
180   net_types.h \
181   netaddress.h \
182   netbase.h \
183+  netgroup.h \

fjahr commented at 12:39 pm on March 12, 2022:

nit: looking at other file naming I think I would have named the file netgroupman?

jnewbery commented at 2:39 pm on March 14, 2022:

I’m indifferent. Happy to rename this to netgroupman if other people agree and I need to touch this branch again.

in src/netgroup.h:16 in b710751542 outdated

 9+
10 /**
11  * Netgroup manager
12  */
13-class NetGroupManager {};
14+class NetGroupManager {

fjahr commented at 2:28 pm on March 12, 2022:

I feel like I am missing a bit of context on the naming decision of this class. The name is pretty generic so is the goal to add more functionality not related to ASMap? If not I would have probably opted for AsmapMan or so. If there is more to add here in the future maybe it would be good to add some more context in the comment above. Or maybe NetGroup is already a term used in this context and I am not aware of this?

jnewbery commented at 2:42 pm on March 14, 2022:

I think NetGroupManager makes more sense than AsmapManager, since this class will also calculate the netgroup for addresses when no asmap is present. In fact, the other components (net, addrman), do not even need to know about the existence of asmaps after this change - netgroupman hides that detail from them.

fjahr commented at 2:48 pm on March 12, 2022: member

Code looks good from the first pass, just some naming bickering that is probably not that important. I am running an asmap node with this PR and will leave it on for at least 24 hours total before finishing the review.

fjahr commented at 10:34 pm on March 13, 2022: member

ACK 5d2af606cc6c4f5decd078e86d860ea2d8009b69

Took another code review pass and didn’t see any issues with the node running this code.

jnewbery commented at 2:42 pm on March 14, 2022: member

Thanks for the review @fjahr. I’ve responded to your comments. Let me know if anything is still unclear.

in src/init.cpp:1230 in fe5b91a2ed outdated

1224@@ -1223,8 +1225,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1225     const bool ignores_incoming_txs{args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)};
1226 
1227     {
1228-        // Initialize addrman
1229-        assert(!node.addrman);
1230+        // Initialize netgroup manager
1231+        assert(!node.netgroupman);
1232+        node.netgroupman = std::make_unique<NetGroupManager>();

mzumsande commented at 7:48 pm on March 14, 2022:

node.netgroupman is introduced in fe5b91a2ed4e4a10fda17d79203c3dc2b2b59eb1 and then moved below the asmap parsing section in the following commit b710751542eb5995a358ef83eab2832cb3bae739 (while the assert stays at its original place).

Any reason not to introduce it there in the first place, and keep it together with the assert?

jnewbery commented at 7:15 am on March 28, 2022:

I agree. I’ve updated the commits so this doesn’t move around. Thanks!

in src/netaddress.cpp:821 in 5c2fd3ce06 outdated

815@@ -811,13 +816,14 @@ std::vector<unsigned char> CNetAddr::GetGroup(const std::vector<bool> &asmap) co
816     }
817 
818     // Push our address onto vchRet.
819+    auto addr_bytes = address.GetAddrBytes();
820     const size_t num_bytes = nBits / 8;
821-    vchRet.insert(vchRet.end(), m_addr.begin(), m_addr.begin() + num_bytes);
822+    vchRet.insert(vchRet.end(), addr_bytes.begin() + nStartByte, addr_bytes.begin() + nStartByte + num_bytes);

mzumsande commented at 9:52 pm on March 14, 2022:

It took me some time to understand why this doesn’t change behavior: GetAddrBytes(), which is now used instead of m_addr, adds a prefix only for IPv4 and Internal addresses. For IPv4, we return early and never get here, so we’d only need to reverse the prefix for Internal addresses. So I think this is correct.

Adding a prefix just to remove it again seems a bit complicated though, have you considered the alternative of adding a function to netaddress that exposes the unaltered m_addr instead of using GetAddrBytes?

jnewbery commented at 4:03 pm on March 26, 2022:

I did consider that, but went with this approach, which is equivalent to what we were doing before commit 102867c587f5f7954232fb8ed8e85cda78bb4d32 to avoid having to expose the internal m_addr bytes in a public method.

mzumsande commented at 10:04 pm on March 14, 2022: member

Reviewed the code, and it looks good to me - two small comments before I Ack.

DrahtBot added the label Needs rebase on Mar 25, 2022

jnewbery commented at 4:03 pm on March 26, 2022: member

Rebased and responded to @mzumsande’s review comments.

jnewbery force-pushed on Mar 26, 2022

DrahtBot removed the label Needs rebase on Mar 26, 2022

DrahtBot added the label Needs rebase on Apr 4, 2022

jnewbery commented at 8:50 pm on April 4, 2022: member

rebased

jnewbery force-pushed on Apr 4, 2022

DrahtBot removed the label Needs rebase on Apr 4, 2022

DrahtBot added the label Needs rebase on Apr 16, 2022

[build] Add netgroup.cpp|h

These aren't used yet.

9b3836710b

[init] Add netgroupman to node.context

This is constructed before addrman and connman, and destructed afterwards.

netgroupman does not currently do anything, but will have functionality added in future commits.

17c24d4580

jnewbery force-pushed on Apr 19, 2022

jnewbery commented at 10:14 am on April 19, 2022: member

Rebased

DrahtBot removed the label Needs rebase on Apr 19, 2022

in src/addrman.cpp:949 in ab0ae657d7 outdated

944@@ -945,16 +945,16 @@ std::optional<AddressPosition> AddrManImpl::FindAddressEntry_(const CAddress& ad
945     if (!addr_info) return std::nullopt;
946 
947     if(addr_info->fInTried) {
948-        int bucket{addr_info->GetTriedBucket(nKey, m_asmap)};
949-        return AddressPosition(/*tried_in=*/true,
950-                               /*multiplicity_in=*/1,
951-                               /*bucket_in=*/bucket,
952+        int bucket{addr_info->GetTriedBucket(nKey, m_netgroupman.GetAsmap())};
953+        return AddressPosition(/*tried=*/true,

mzumsande commented at 8:47 pm on April 19, 2022:

This is a small rebase error, reverting #24665 (see CI fail).

jnewbery commented at 1:36 pm on April 20, 2022:

Thanks! Now fixed.

[net] Move asmap into NetGroupManager 19431560e3

[netgroupman] Add GetMappedAS() and GetGroup()

These currently call through to the CNetAddr methods. The logic will be moved in a future commit.

6b2268162e

[net] Only use public CNetAddr functions and data in GetMappedAS() and GetGroup()

Also change parameter/variable names. This makes the next commit mostly
move-only.

ddb4101e63

[netgroupman] Move GetMappedAS() and GetGroup() logic to NetGroupManager

Reviewer hint: use:

`git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`

1b978a7e8c

[netgroupman] Move asmap checksum calculation to NetGroupManager 4709fc2019

[netgroupman] Remove NetGroupManager::GetAsmap()

asmap no longer needs to be exposed anywhere outside NetGroupManager.

36f814c0e8

jnewbery force-pushed on Apr 20, 2022

mzumsande commented at 12:32 pm on April 21, 2022: member

Code Review ACK 36f814c0e84d009c0e0aa26981a20ac4cf338a85

Did some light sanity testing, and didn’t run into any problems.

fanquake requested review from fjahr on Apr 21, 2022

fanquake requested review from dergoegge on Apr 21, 2022

fanquake requested review from naumenkogs on Apr 21, 2022

jnewbery force-pushed on Apr 21, 2022

jnewbery commented at 12:39 pm on April 21, 2022: member

CI failure seems spurious. I rebased onto latest master to trigger a new CI run, but whilst I was doing that, mzumsande ACKed https://github.com/bitcoin/bitcoin/commit/36f814c0e84d009c0e0aa26981a20ac4cf338a85, so I’ve reverted to that.

in src/netaddress.cpp:792 in ddb4101e63 outdated

791-    } else if (IsInternal()) {
792-        // all internal-usage addresses get their own group
793+    } else if (address.IsInternal()) {
794+        // All internal-usage addresses get their own group.
795+        // Skip over the INTERNAL_IN_IPV6_PREFIX returned by CAddress::GetAddrBytes().
796+        nStartByte = INTERNAL_IN_IPV6_PREFIX.size();

dergoegge commented at 10:38 am on April 22, 2022:

nit: i think it would also be possible to keep the prefix and set nBits = (INTERNAL_IN_IPV6_PREFIX.size() + ADDR_INTERNAL_SIZE) * 8;. Then we wouldn’t need to reintroduce nStartByte, which was previously also used for other address types.

in src/netgroup.cpp:74 in 36f814c0e8

69+    vchRet.insert(vchRet.end(), addr_bytes.begin() + nStartByte, addr_bytes.begin() + nStartByte + num_bytes);
70+    nBits %= 8;
71+    // ...for the last byte, push nBits and for the rest of the byte push 1's
72+    if (nBits > 0) {
73+        assert(num_bytes < addr_bytes.size());
74+        vchRet.push_back(addr_bytes[num_bytes] | ((1 << (8 - nBits)) - 1));

dergoegge commented at 10:52 am on April 22, 2022:

If nStartByte is ever used for other address types again it might be necessary to do the following:

0        vchRet.push_back(addr_bytes[num_bytes + nStartByte] | ((1 << (8 - nBits)) - 1));

See: https://github.com/bitcoin/bitcoin/blob/1017b8a960d8bca41adebdad9982511a479689ce/src/netbase.cpp#L996

dergoegge commented at 10:59 am on April 22, 2022: member

Code review ACK 36f814c0e84d009c0e0aa26981a20ac4cf338a85

fanquake commented at 1:42 pm on April 22, 2022: member

@dergoegge you might want to open a followup with any suggestions / changes. @jnewbery fyi I’ve edited your comment above to remove the @. Otherwise the ACK was being picked up and included in the merge message.

fanquake merged this on Apr 22, 2022

fanquake closed this on Apr 22, 2022

sidhujag referenced this in commit f411b06c58 on Apr 22, 2022

jnewbery deleted the branch on Apr 22, 2022

fjahr commented at 1:28 pm on April 24, 2022: member

post-merge code review ACK 36f814c0e84d009c0e0aa26981a20ac4cf338a85

fanquake referenced this in commit 33aaf434af on May 4, 2022

sidhujag referenced this in commit 9fdd5ac90c on May 4, 2022

kouloumos referenced this in commit fc13feb3cf on Feb 1, 2023

DrahtBot locked this on Apr 24, 2023

net: Encapsulate asmap in NetGroupManager #22910

Conflicts