net: Encapsulate asmap in NetGroupManager #22910

jnewbery · 2021-09-07T14:39:58Z

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.

DrahtBot · 2021-09-07T19:54:57Z

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 · 2021-09-07T21:07:33Z

Concept ACK (just judging the PR description)

practicalswift · 2021-09-08T07:55:27Z

Concept ACK

naumenkogs · 2021-09-08T07:58:15Z

Concept ACK, light code review ACK d70b354

mzumsande

Concept ACK

src/test/fuzz/addrman.cpp

853c4ed [net] Remove asmap argument from CNode::CopyStats() (John Newbery) 9fd5618 [asmap] Make DecodeAsmap() a utility function (John Newbery) bfdf4ef [asmap] Remove SanityCheckASMap() from netaddress (John Newbery) 07a9ecc [net] Remove CConnman::Options.m_asmap (John Newbery) Pull request description: These small cleanups to the asmap code are the first 4 commits from #22910. They're minor improvements that are independently useful whether or not 22910 is merged. ACKs for top commit: naumenkogs: ACK 853c4ed theStack: Concept and code-review ACK 853c4ed 🗺️ fanquake: ACK 853c4ed Tree-SHA512: 64783743182592ac165df6ff8d18870b63861e9204ed722c207fca6938687aac43232a5ac4d8228cf8b92130ab0349de1b410a2467bb5a9d60dd9a7221b3b85b

jnewbery · 2021-09-10T09:50:52Z

Rebased and fixed #22910 (comment)

jnewbery · 2021-09-10T10:53:34Z

rerebased

amitiuttarwar · 2021-09-10T15:00:49Z

concept ACK

jnewbery · 2022-04-19T10:14:27Z

Rebased

src/addrman.cpp

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

…d GetGroup() Also change parameter/variable names. This makes the next commit mostly move-only.

Reviewer hint: use: `git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`

asmap no longer needs to be exposed anywhere outside NetGroupManager.

mzumsande · 2022-04-21T12:32:10Z

Code Review ACK 36f814c

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

jnewbery · 2022-04-21T12:39:19Z

CI failure seems spurious. I rebased onto latest master to trigger a new CI run, but whilst I was doing that, mzumsande ACKed 36f814c, so I've reverted to that.

dergoegge

Code review ACK 36f814c

dergoegge · 2022-04-22T10:38:54Z

src/netaddress.cpp

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


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.

dergoegge · 2022-04-22T10:52:34Z

src/netgroup.cpp

+    // ...for the last byte, push nBits and for the rest of the byte push 1's
+    if (nBits > 0) {
+        assert(num_bytes < addr_bytes.size());
+        vchRet.push_back(addr_bytes[num_bytes] | ((1 << (8 - nBits)) - 1));


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

Suggested change

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

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

See:

bitcoin/src/netbase.cpp

Line 996 in 1017b8a

vchRet.push_back(GetByte(15 - nStartByte) | ((1 << (8 - nBits)) - 1));

fanquake · 2022-04-22T13:42:40Z

@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.

fjahr · 2022-04-24T13:28:41Z

post-merge code review ACK 36f814c

e5d1831 [netgroup] Use nStartByte as offset for the last byte of the group (dergoegge) Pull request description: This addresses my review [comments](bitcoin/bitcoin#22910 (comment)) I left on #22910. This has no effect on the current logic as `nStartByte` is only used for internal addresses which only ever add 10 whole bytes to the returned group. However to avoid future bugs, I think we should use `nStartByte` as offset for the last byte as well, in case we ever add a new address type that makes makes use of `nStartByte` and adds fractional bytes to the group. ACKs for top commit: jnewbery: Code review ACK e5d1831 theStack: Concept and code-review ACK e5d1831 Tree-SHA512: 4c08c7d6cb38b553e998798b3e3b790177aaa2141a48e277dfd538e01a7fccadf644329e93c5b0fb5e7e4037494c8dfe061b94eb52c6b31dc21bdf99eb0e311a

e5d1831 [netgroup] Use nStartByte as offset for the last byte of the group (dergoegge) Pull request description: This addresses my review [comments](bitcoin#22910 (comment)) I left on bitcoin#22910. This has no effect on the current logic as `nStartByte` is only used for internal addresses which only ever add 10 whole bytes to the returned group. However to avoid future bugs, I think we should use `nStartByte` as offset for the last byte as well, in case we ever add a new address type that makes makes use of `nStartByte` and adds fractional bytes to the group. ACKs for top commit: jnewbery: Code review ACK e5d1831 theStack: Concept and code-review ACK e5d1831 Tree-SHA512: 4c08c7d6cb38b553e998798b3e3b790177aaa2141a48e277dfd538e01a7fccadf644329e93c5b0fb5e7e4037494c8dfe061b94eb52c6b31dc21bdf99eb0e311a

- add NetGroupManager as part of node p2p components after bitcoin/bitcoin#22910 - update AddrMan description

jnewbery mentioned this pull request Sep 7, 2021

[net] Minor cleanups to asmap #22911

Merged

jnewbery force-pushed the 2021-08-netgroupmgr branch from 64ba743 to 2789177 Compare September 7, 2021 14:48

jnewbery mentioned this pull request Sep 7, 2021

init: Fix asmap/addrman initialization order bug #22791

Merged

jnewbery force-pushed the 2021-08-netgroupmgr branch from 2789177 to d70b354 Compare September 7, 2021 14:54

DrahtBot added Build system P2P Utils/log/libs labels Sep 7, 2021

DrahtBot mentioned this pull request Sep 7, 2021

log: improve addrman logging #22839

Merged

This was referenced Sep 7, 2021

Raise InitError when peers.dat is invalid or corrupted #22762

Merged

addrman: treat Tor/I2P/CJDNS as a single group #22563

Closed

Add fee_est tool for debugging fee estimation code #10443

Closed

DrahtBot mentioned this pull request Sep 8, 2021

Remove confusing CAddrDB #22915

Merged

mzumsande reviewed Sep 8, 2021

View reviewed changes

src/test/fuzz/addrman.cpp Outdated Show resolved Hide resolved

jnewbery force-pushed the 2021-08-netgroupmgr branch 2 times, most recently from 54f6a6a to 3de73f1 Compare September 10, 2021 09:49

jnewbery force-pushed the 2021-08-netgroupmgr branch from 3de73f1 to 4f89a79 Compare September 10, 2021 10:49

maflcko removed the Build system label Sep 10, 2021

jnewbery force-pushed the 2021-08-netgroupmgr branch from 14ec5f5 to 3149a8c Compare April 19, 2022 10:08

DrahtBot removed the Needs rebase label Apr 19, 2022

mzumsande reviewed Apr 19, 2022

View reviewed changes

src/addrman.cpp Outdated Show resolved Hide resolved

jnewbery added 6 commits April 20, 2022 14:29

[net] Move asmap into NetGroupManager

1943156

[netgroupman] Add GetMappedAS() and GetGroup()

6b22681

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

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

ddb4101

…d GetGroup() Also change parameter/variable names. This makes the next commit mostly move-only.

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

1b978a7

Reviewer hint: use: `git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`

[netgroupman] Move asmap checksum calculation to NetGroupManager

4709fc2

[netgroupman] Remove NetGroupManager::GetAsmap()

36f814c

asmap no longer needs to be exposed anywhere outside NetGroupManager.

jnewbery force-pushed the 2021-08-netgroupmgr branch from 3149a8c to 36f814c Compare April 20, 2022 13:36

DrahtBot mentioned this pull request Apr 20, 2022

refactor: make GetRand a template, remove GetRandInt #24925

Merged

fanquake requested review from fjahr, dergoegge and naumenkogs April 21, 2022 12:32

jnewbery force-pushed the 2021-08-netgroupmgr branch 2 times, most recently from fa4c923 to 36f814c Compare April 21, 2022 12:37

dergoegge reviewed Apr 22, 2022

View reviewed changes

fanquake merged commit 505ba39 into bitcoin:master Apr 22, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 22, 2022

Merge bitcoin#22910: net: Encapsulate asmap in NetGroupManager

f411b06

jnewbery deleted the 2021-08-netgroupmgr branch April 22, 2022 21:33

dergoegge mentioned this pull request Apr 25, 2022

netgroup: Follow-up for #22910 #24976

Merged

kouloumos added a commit to kouloumos/onboarding-to-bitcoin-core that referenced this pull request Feb 1, 2023

add NetGroupManager

fc13feb

- add NetGroupManager as part of node p2p components after bitcoin/bitcoin#22910 - update AddrMan description

bitcoin locked and limited conversation to collaborators Apr 24, 2023

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

net: Encapsulate asmap in NetGroupManager #22910

net: Encapsulate asmap in NetGroupManager #22910

Uh oh!

Conversation

jnewbery commented Sep 7, 2021

Uh oh!

DrahtBot commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

sipa commented Sep 7, 2021

Uh oh!

practicalswift commented Sep 8, 2021

Uh oh!

naumenkogs commented Sep 8, 2021

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery commented Sep 10, 2021

Uh oh!

jnewbery commented Sep 10, 2021

Uh oh!

amitiuttarwar commented Sep 10, 2021

Uh oh!

jnewbery commented Apr 19, 2022

Uh oh!

Uh oh!

mzumsande commented Apr 21, 2022

Uh oh!

jnewbery commented Apr 21, 2022 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dergoegge left a comment

Choose a reason for hiding this comment

Uh oh!

dergoegge Apr 22, 2022

Choose a reason for hiding this comment

Uh oh!

dergoegge Apr 22, 2022

Choose a reason for hiding this comment

Uh oh!

fanquake commented Apr 22, 2022

Uh oh!

fjahr commented Apr 24, 2022

Uh oh!

Uh oh!

DrahtBot commented Sep 7, 2021 •

edited

Loading

jnewbery commented Apr 21, 2022 •

edited by fanquake

Loading