net: Encapsulate asmap in NetGroupManager #22910

pull jnewbery wants to merge 8 commits into bitcoin:master from jnewbery:2021-08-netgroupmgr changing 24 files +370 −302
  1. 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.

  2. jnewbery cross-referenced this on Sep 7, 2021 from issue [net] Minor cleanups to asmap by jnewbery
  3. jnewbery force-pushed on Sep 7, 2021
  4. jnewbery cross-referenced this on Sep 7, 2021 from issue init: Fix asmap/addrman initialization order bug by jnewbery
  5. jnewbery force-pushed on Sep 7, 2021
  6. DrahtBot added the label Build system on Sep 7, 2021
  7. DrahtBot added the label P2P on Sep 7, 2021
  8. DrahtBot added the label Utils/log/libs on Sep 7, 2021
  9. DrahtBot commented at 7:54 PM on September 7, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  10. DrahtBot cross-referenced this on Sep 7, 2021 from issue log: improve addrman logging by mzumsande
  11. sipa commented at 9:07 PM on September 7, 2021: member

    Concept ACK (just judging the PR description)

  12. DrahtBot cross-referenced this on Sep 7, 2021 from issue Raise InitError when peers.dat is invalid or corrupted by MarcoFalke
  13. DrahtBot cross-referenced this on Sep 7, 2021 from issue addrman: treat Tor/I2P/CJDNS as a single group by vasild
  14. DrahtBot cross-referenced this on Sep 8, 2021 from issue Add fee_est tool for debugging fee estimation code by ryanofsky
  15. practicalswift commented at 7:55 AM on September 8, 2021: contributor

    Concept ACK

  16. naumenkogs commented at 7:58 AM on September 8, 2021: member

    Concept ACK, light code review ACK d70b3541cbca6c4f5b5552ec5d3fbd59bc51749a

  17. DrahtBot cross-referenced this on Sep 8, 2021 from issue Remove confusing CAddrDB by MarcoFalke
  18. 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.

  19. mzumsande commented at 10:38 PM on September 8, 2021: contributor

    Concept ACK

  20. fanquake referenced this in commit 5446070418 on Sep 10, 2021
  21. jnewbery force-pushed on Sep 10, 2021
  22. jnewbery force-pushed on Sep 10, 2021
  23. jnewbery commented at 9:50 AM on September 10, 2021: member

    Rebased and fixed #22910 (review)

  24. jnewbery force-pushed on Sep 10, 2021
  25. jnewbery commented at 10:53 AM on September 10, 2021: member

    rerebased

  26. MarcoFalke removed the label Build system on Sep 10, 2021
  27. amitiuttarwar commented at 3:00 PM on September 10, 2021: contributor

    concept ACK

  28. DrahtBot cross-referenced this on Sep 11, 2021 from issue fuzz: Move all addrman fuzz targets to one file by MarcoFalke
  29. DrahtBot cross-referenced this on Sep 11, 2021 from issue [p2p] Pimpl AddrMan to abstract implementation details by amitiuttarwar
  30. DrahtBot cross-referenced this on Sep 14, 2021 from issue addrman: Improve performance of Good by mzumsande
  31. DrahtBot cross-referenced this on Sep 15, 2021 from issue p2p: Improve diversification of new connections by naumenkogs
  32. DrahtBot cross-referenced this on Sep 15, 2021 from issue scripted-diff: Rename overloaded int GetArg to GetIntArg by ryanofsky
  33. DrahtBot cross-referenced this on Sep 16, 2021 from issue addrman: Avoid crash on corrupt data, Force Check after deserialize by MarcoFalke
  34. DrahtBot cross-referenced this on Sep 16, 2021 from issue refactor: Forbid calling unsafe fs::path(std::string) constructor and fs::path::string() method by ryanofsky
  35. DrahtBot cross-referenced this on Sep 19, 2021 from issue bench: update nanobench add `-min_time` by martinus
  36. Bossday cross-referenced this on Sep 21, 2021 from issue . by Bossday
  37. 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.

  38. jnewbery force-pushed on Oct 7, 2021
  39. jnewbery commented at 4:19 PM on October 7, 2021: member

    Rebased on master

  40. DrahtBot cross-referenced this on Oct 7, 2021 from issue p2p: Use mocktime for ping timeout by MarcoFalke
  41. DrahtBot cross-referenced this on Oct 7, 2021 from issue Full CJDNS support by vasild
  42. DrahtBot cross-referenced this on Oct 7, 2021 from issue [fuzz] Use public methods in addrman fuzz tests by jnewbery
  43. 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

    static AddrMan CreateAddrMan()
    jnewbery commented at 12:56 PM on October 8, 2021:

    Oops. Pesky benches. Fixed.

  44. jnewbery force-pushed on Oct 8, 2021
  45. jnewbery commented at 1:09 PM on October 8, 2021: member

    rebased on master

  46. jnewbery force-pushed on Oct 8, 2021
  47. DrahtBot cross-referenced this on Oct 9, 2021 from issue Make all networking code mockable by vasild
  48. DrahtBot added the label Needs rebase on Dec 13, 2021
  49. jnewbery force-pushed on Dec 22, 2021
  50. 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.

  51. DrahtBot removed the label Needs rebase on Dec 23, 2021
  52. DrahtBot cross-referenced this on Dec 23, 2021 from issue docs: avoid C-style casts; use modern C++ casts by PastaPastaPasta
  53. DrahtBot cross-referenced this on Dec 23, 2021 from issue p2p: Remove GetAdjustedTime() from AddrMan by w0xlt
  54. DrahtBot cross-referenced this on Dec 23, 2021 from issue scripted-diff: Use clang-tidy syntax for C++ named arguments by MarcoFalke
  55. DrahtBot cross-referenced this on Dec 23, 2021 from issue Add Yggdrasil support by prusnak
  56. DrahtBot cross-referenced this on Dec 23, 2021 from issue test: Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change by vasild
  57. DrahtBot cross-referenced this on Dec 24, 2021 from issue test: Make AddrMan unit tests use public interface, extend coverage by mzumsande
  58. DrahtBot added the label Needs rebase on Jan 4, 2022
  59. jnewbery force-pushed on Jan 5, 2022
  60. jnewbery commented at 3:54 PM on January 5, 2022: member

    Rebased on master

  61. DrahtBot removed the label Needs rebase on Jan 5, 2022
  62. jnewbery force-pushed on Jan 6, 2022
  63. jnewbery marked this as ready for review on Jan 6, 2022
  64. jnewbery force-pushed on Jan 6, 2022
  65. 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.

  66. DrahtBot added the label Needs rebase on Jan 17, 2022
  67. jnewbery force-pushed on Jan 20, 2022
  68. jnewbery force-pushed on Jan 20, 2022
  69. jnewbery commented at 5:27 PM on January 20, 2022: member

    Rebased

  70. DrahtBot removed the label Needs rebase on Jan 20, 2022
  71. jnewbery renamed this:
    [RFC] Encapsulate asmap in NetGroupManager
    net: Encapsulate asmap in NetGroupManager
    on Jan 23, 2022
  72. DrahtBot cross-referenced this on Jan 31, 2022 from issue refactor: use Span in random.* by PastaPastaPasta
  73. DrahtBot cross-referenced this on Mar 1, 2022 from issue Make SER_GETHASH implicit for CHashWriter and SerializeHash by Empact
  74. naumenkogs commented at 8:01 AM on March 10, 2022: member

    ACK a3def07ce573fd216e6b8b41d93bb8218024ba74

    oops, see below

  75. MarcoFalke commented at 8:16 AM on March 10, 2022: member

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

  76. jnewbery force-pushed on Mar 10, 2022
  77. jnewbery commented at 9:28 PM on March 10, 2022: member

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

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

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

  80. fjahr commented at 2:48 PM on March 12, 2022: contributor

    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.

  81. DrahtBot cross-referenced this on Mar 12, 2022 from issue BIP324: Enable v2 P2P encrypted transport by dhruv
  82. fjahr commented at 10:34 PM on March 13, 2022: contributor

    ACK 5d2af606cc6c4f5decd078e86d860ea2d8009b69

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

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

  84. 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!

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

  86. mzumsande commented at 10:04 PM on March 14, 2022: contributor

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

  87. DrahtBot added the label Needs rebase on Mar 25, 2022
  88. jnewbery commented at 4:03 PM on March 26, 2022: member

    Rebased and responded to @mzumsande's review comments.

  89. jnewbery force-pushed on Mar 26, 2022
  90. DrahtBot removed the label Needs rebase on Mar 26, 2022
  91. DrahtBot cross-referenced this on Mar 27, 2022 from issue refactor: Use clang-tidy syntax for C++ named arguments by fanquake
  92. DrahtBot added the label Needs rebase on Apr 4, 2022
  93. jnewbery commented at 8:50 PM on April 4, 2022: member

    rebased

  94. jnewbery force-pushed on Apr 4, 2022
  95. DrahtBot removed the label Needs rebase on Apr 4, 2022
  96. DrahtBot cross-referenced this on Apr 5, 2022 from issue test/BIP324: functional tests for v2 P2P encryption by stratospher
  97. DrahtBot cross-referenced this on Apr 13, 2022 from issue test: fix connman UB by calling derived constructor by chinggg
  98. DrahtBot added the label Needs rebase on Apr 16, 2022
  99. [build] Add netgroup.cpp|h
    These aren't used yet.
    9b3836710b
  100. [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
  101. jnewbery force-pushed on Apr 19, 2022
  102. jnewbery commented at 10:14 AM on April 19, 2022: member

    Rebased

  103. DrahtBot removed the label Needs rebase on Apr 19, 2022
  104. 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.

  105. [net] Move asmap into NetGroupManager 19431560e3
  106. [netgroupman] Add GetMappedAS() and GetGroup()
    These currently call through to the CNetAddr methods. The logic will be moved in a future commit.
    6b2268162e
  107. [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
  108. [netgroupman] Move GetMappedAS() and GetGroup() logic to NetGroupManager
    Reviewer hint: use:

`git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
    1b978a7e8c
  109. [netgroupman] Move asmap checksum calculation to NetGroupManager 4709fc2019
  110. [netgroupman] Remove NetGroupManager::GetAsmap()
    asmap no longer needs to be exposed anywhere outside NetGroupManager.
    36f814c0e8
  111. jnewbery force-pushed on Apr 20, 2022
  112. DrahtBot cross-referenced this on Apr 20, 2022 from issue refactor: make GetRand a template, remove GetRandInt by PastaPastaPasta
  113. mzumsande commented at 12:32 PM on April 21, 2022: contributor

    Code Review ACK 36f814c0e84d009c0e0aa26981a20ac4cf338a85

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

  114. fanquake requested review from fjahr on Apr 21, 2022
  115. fanquake requested review from dergoegge on Apr 21, 2022
  116. fanquake requested review from naumenkogs on Apr 21, 2022
  117. jnewbery force-pushed on Apr 21, 2022
  118. jnewbery force-pushed on Apr 21, 2022
  119. 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.

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

  121. 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:

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

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

  122. dergoegge commented at 10:59 AM on April 22, 2022: member

    Code review ACK 36f814c0e84d009c0e0aa26981a20ac4cf338a85

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

  124. fanquake merged this on Apr 22, 2022
  125. fanquake closed this on Apr 22, 2022
  126. sidhujag referenced this in commit f411b06c58 on Apr 22, 2022
  127. jnewbery deleted the branch on Apr 22, 2022
  128. fjahr commented at 1:28 PM on April 24, 2022: contributor

    post-merge code review ACK 36f814c0e84d009c0e0aa26981a20ac4cf338a85

  129. dergoegge cross-referenced this on Apr 25, 2022 from issue netgroup: Follow-up for #22910 by dergoegge
  130. fanquake referenced this in commit 33aaf434af on May 4, 2022
  131. sidhujag referenced this in commit 9fdd5ac90c on May 4, 2022
  132. kouloumos referenced this in commit fc13feb3cf on Feb 1, 2023
  133. bitcoin locked this on Apr 24, 2023
Contributors
jnewberyDrahtBotsipapracticalswiftnaumenkogsmzumsandeamitiuttarwarMarcoFalkefjahrdergoeggefanquake


fjahrnaumenkogs

Labels
P2PUtils/log/libs
Linked (view graph)
#22791 init: Fix asmap/addrman initialization order bug#22911 [net] Minor cleanups to asmap#23055 .#23826 test: Make AddrMan unit tests use public interface, extend coverage#24976 netgroup: Follow-up for #22910
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 04:53 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me