p2p: Avoid allocating memory for addrKnown where we don’t need it #17164

pull naumenkogs wants to merge 3 commits into bitcoin:master from naumenkogs:addr_relay_optimization_4 changing 4 files +11 −13
  1. naumenkogs commented at 5:48 pm on October 16, 2019: member

    We should allocate memory for addrKnown filter only for those peers which are expected to participate in address relay.

    Currently, we do it for all peers (including SPV and block-relay-only), which results in extra RAM where it’s not needed.

    Upd: In future, we would still allow SPVs to ask for addrs, so allocation still will be done by default. However, they will be able to opt-out via this proposal and then we could save some more memory. This PR still saves memory for block-relay-only peers immediately after merging.

  2. fanquake added the label P2P on Oct 16, 2019
  3. fanquake added the label Resource usage on Oct 16, 2019
  4. in src/bloom.h:121 in e65af6e1cf outdated
    117@@ -118,6 +118,7 @@ class CRollingBloomFilter
    118     // A random bloom filter calls GetRand() at creation time.
    119     // Don't create global CRollingBloomFilter objects, as they may be
    120     // constructed before the randomizer is properly initialized.
    121+    CRollingBloomFilter(): nEntriesPerGeneration(0), nEntriesThisGeneration(0), nGeneration(0),  nTweak(0), nHashFuncs(0) {};
    


    MarcoFalke commented at 5:53 pm on October 16, 2019:

    why is this needed?

    I removed it and I can still compile.

  5. in src/bloom.h:120 in e65af6e1cf outdated
    117@@ -118,6 +118,7 @@ class CRollingBloomFilter
    118     // A random bloom filter calls GetRand() at creation time.
    119     // Don't create global CRollingBloomFilter objects, as they may be
    120     // constructed before the randomizer is properly initialized.
    


    MarcoFalke commented at 5:53 pm on October 16, 2019:

    This comment no longer applies and can be removed

    Randomness is initialized on first use

  6. MarcoFalke commented at 5:56 pm on October 16, 2019: member
    This is only relevant for nodes that accept incoming connections? And those nodes are assumed to have a ton of resources. Though, it can’t hurt to same some bits, if it can be done conveniently.
  7. naumenkogs commented at 6:06 pm on October 16, 2019: member

    This is only relevant for nodes that accept incoming connections?

    Not really. To all the nodes with block-relay-only peers.

  8. naumenkogs force-pushed on Oct 16, 2019
  9. GChuf commented at 6:51 pm on October 16, 2019: contributor
    Concept ACK. Respectfully, assuming nodes have a ton of resources is dangerous - every optimization is potentially important (talking as a multiple nodes operator here).
  10. in src/bloom.h:120 in 90407e6fab outdated
    114@@ -115,9 +115,6 @@ class CBloomFilter
    115 class CRollingBloomFilter
    116 {
    117 public:
    118-    // A random bloom filter calls GetRand() at creation time.
    119-    // Don't create global CRollingBloomFilter objects, as they may be
    120-    // constructed before the randomizer is properly initialized.
    


    dongcarl commented at 7:01 pm on October 16, 2019:
    Is there a particular reason this comment was removed?

    naumenkogs commented at 7:04 pm on October 16, 2019:
    @MarcoFalke says it doesn’t apply anymore.

    dongcarl commented at 7:08 pm on October 16, 2019:
    Ah sorry, didn’t see!

    MarcoFalke commented at 7:28 pm on October 16, 2019:
    note to other reviewers: The comment no longer applies after 05fde14e3afe6f7156ebb6df6cd0e3ae12635b89
  11. MarcoFalke commented at 7:47 pm on October 16, 2019: member

    I did some testing with massif, and this saves about 263.3 KiB for 10 peers that connect, but don’t have address relay enabled. Note that the tx relay rolling bloom filter takes up 5.1 MB, which sort of dwarfs the gains here.

    ACK 90407e6fab74cd849d474f0c5dea19b9954d4196

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3ACK 90407e6fab74cd849d474f0c5dea19b9954d4196
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUj6UAv/S2YrC/LwxnSHSRI8/CTG/D4keXVDin7cH//FaaKlFa3yLaiDU6Fiwnj0
     8VuaOdGdxxfxrBRmFI3afL1mpNwbA24ny7pdrHbMVQJ6vLnPWOyXespyT6M7idrLZ
     9xYP2yt7RIufDgLkLV15PlI4YahsTo/fnLfMs3bOA+3/zc8g3q4YLxThBrk5P8fZJ
    10iYaM3uENRl+/7Ny5N1Wfkdl8tXDqbC6/2pYwBI7BNTPSrTOdERymW5EHvfG/IWSE
    11v8VELXxdt2LBWLMODEz2Q4Huib5MeoYJenZvGwnauOun17gHSYi0Ksy8Q8OA0MTU
    12IYZ8J4olbEjJsWlOmKeS774r+s4nf4iwSIw0lx/58ITLECsuGxJbsquYTyB+vP6q
    13RBaNFBoTHshue1c7nH/aHYftfdlNThc+d3Rqas78mIGBFbynJ3a10S1cl37TkApy
    14oyDzr7BnQV7ZdUV0WcVudrNnBVRAky7PmOndvAbStI79TLU5GwGXessK/7lgZibV
    15TcsFz/y0
    16=gtc/
    17-----END PGP SIGNATURE-----
    

    Timestamp of file with hash 283bf14cba6f831bf8491da4b8a98092329778b2783d5ed8f6bbf7462772ff0f -

  12. in src/net.h:732 in 90407e6fab outdated
    728@@ -729,7 +729,7 @@ class CNode
    729 
    730     // flood relay
    731     std::vector<CAddress> vAddrToSend;
    732-    CRollingBloomFilter addrKnown;
    733+    std::unique_ptr<CRollingBloomFilter> addrKnown;
    


    MarcoFalke commented at 7:47 pm on October 16, 2019:
    0    std::unique_ptr<CRollingBloomFilter> m_addr_known;
    

    style-nit: Should use new style for new members

  13. GChuf commented at 8:28 pm on October 16, 2019: contributor
    @MarcoFalke so, the result is actually … an extra ~4.8 MB RAM usage?
  14. naumenkogs commented at 8:42 pm on October 16, 2019: member

    @GChuf no, Marco’s just saying that this update probably saves only 5% of the overall memory we dedicate per peer (or maybe even a bit less due to other stuff).

    My code does not introduce any extra overhead.

  15. p2p: Avoid allocating memory for addrKnown where we don't need it 090b75c14b
  16. naumenkogs force-pushed on Oct 16, 2019
  17. GChuf commented at 9:24 pm on October 16, 2019: contributor
    @naumenkogs thanks for clarifying. Just had to ask because I wasn’t sure I understand. Anyway like I said, I think every improvement is important, no matter how small.
  18. MarcoFalke commented at 5:58 pm on October 17, 2019: member

    re-ACK 090b75c14be6b9ba2efe38a17d141c6e6af575cb

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3re-ACK 090b75c14be6b9ba2efe38a17d141c6e6af575cb
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUjXJgwAqkfJNeBc+WA9VZaMmaiR1ZLmyt/oKW8fcehMV1FS0Vf0sm32JsuUYsqH
     8t/E4L4Hg2lG/7d+wNmQcwXW+gPYAubpBDkZjJw2jCjqptKim9wWnUa1/twgpCfFZ
     9/bBowXhvRuI5wVZhlP7GyKLFdSZqp16goEqYi1IbpjROUFHIvEqXexs5FKmNK9EF
    10ySMMDLgazsmCYsVNtWOh8GbxUuALPX0Sbh3Wv9Lt9YFSVwRFim+b3emO8nGmovSI
    11wkclHFUpmTpYEOmZC+B+/ZR2P9EdFkWzUIgoqiFpkEHCqgMowjF2lGPiMqvcbO6i
    12m/6zWTLtgMxNm1VG2HrAhqbj81NzqO6BYH3T1sO959aSVdSu+VDi5xd3gh6jrpX5
    13WNqxIAWfWu0ISnnWtz0kcTRjLcpqI0QQfzcUXeSWfodS3q4pRNV4abvo8Hb8b6/Q
    14TLJuGRFTQbMGSF7eU9FUDgV8NpCzXbu7Pu1L+FgDgBQfpxgYvGXyitgfaGPLX2TJ
    15ZrlPc95E
    16=Toyw
    17-----END PGP SIGNATURE-----
    

    Timestamp of file with hash 7806780bf7d730ec30b25732f4026f506b9c7fbff0e3b47a2803142242aea1b6 -

  19. DrahtBot commented at 6:53 pm on October 17, 2019: member

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

    Conflicts

    No conflicts as of last run.

  20. laanwj commented at 6:15 pm on October 24, 2019: member
    I think there should be NULL pointer checks or assertions at the locations that now use m_addr_known, just in case.
  21. added asserts to check m_addr_known when it's used a552e8477c
  22. laanwj commented at 3:49 pm on October 28, 2019: member
    ACK a552e8477c5bcd22a5457f4f73a2fd6db8acd2c2
  23. MarcoFalke commented at 8:37 pm on October 28, 2019: member

    re-ACK a552e8477c5bcd22a5457f4f73a2fd6db8acd2c2

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3re-ACK a552e8477c5bcd22a5457f4f73a2fd6db8acd2c2
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUibEQwAr22ss9KohgyamVhNe0/FeIO1vHR9dSdSknvqKD6DoGkCzgctdooQJhiW
     84QKzssocyI/i1Jbg9O3Ckl14IcfFyhnYBLmA+u01Oqxp/rEWtVPHS32mw7xj+LPk
     90ABS7uZSEdzkFcupONG/4zFoC60LokZi1DszYUNRfpxRbgMFlWSJtaMuBJn5Bp8y
    10tq2Sn9h2IojdUyHvdWpwpMFHmN6fP0PORI6XvHVSIB7Vi1c7+sAuDOaEVS7E6PW2
    11cPuX8VV/rYO0K67bmDY4tS2r07UEuBCwDS59NxHDoGpVBH0fKINbMEhu0ETNIPZ9
    12avrIkaaYcehiv5qn6J3ocpZT4wnoAXjes+GMcCTbnhdB+2+fWlk+mWYRFTYT7UEg
    13Q06HINGY7amistUcZRNrScIDWfsReoxMx7HihrcxcYgpT4t2OVWYtRewDx1f99iE
    14ZO4xfqgDd2S/grEsV5rp0Nv6Bb6D0MIG2kjYg3kCG2h1ASPHvcWLsAcupDHajIpy
    15rPPybuft
    16=/ueI
    17-----END PGP SIGNATURE-----
    

    Timestamp of file with hash 0ab34613ea38e4bc0035a5ffbe8b4c57fd1cbe756bb708ed3a66369644787338 -

  24. dongcarl commented at 4:55 pm on October 30, 2019: member

    ACK a552e8477c5bcd22a5457f4f73a2fd6db8acd2c2


    Potential Followups

    • Add comments to these functions that describe their assumptions about preconditions (pto->IsAddrRelayPeer())
    • Perhaps tweak the API so that there doesn’t need to be preconditions like this (no idea how to do)

    Review Notes

    Looked through to see that the new asserts won’t crash the daemon where previously it might have been a silent failure…

    The assert in bool PeerLogicValidation::SendMessages(CNode* pto) is guaranteed to succeed because of the pto->IsAddrRelayPeer() check a few lines up: https://github.com/bitcoin/bitcoin/blob/a552e8477c5bcd22a5457f4f73a2fd6db8acd2c2/src/net_processing.cpp#L3560-L3564

    The assert in void AddAddressKnown(const CAddress& _addr) is guaranteed to succeed because its only call-site is pre-empted by an early return if !pfrom->IsAddrRelayPeer() https://github.com/bitcoin/bitcoin/blob/a552e8477c5bcd22a5457f4f73a2fd6db8acd2c2/src/net_processing.cpp#L2101-L2128

    For the assert in void PushAddress(const CAddress& _addr, FastRandomContext &insecure_rand), there are 5 call-sites:

    1 of them in static void RelayAddress(const CAddress& addr, bool fReachable, CConnman* connman), where it is called in pushfunc, which is only called on CNode *s that are pnode->IsAddrRelayPeer()

    https://github.com/bitcoin/bitcoin/blob/a552e8477c5bcd22a5457f4f73a2fd6db8acd2c2/src/net_processing.cpp#L1326-L1345

    3 of them in bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStream& vRecv, int64_t nTimeReceived, const CChainParams& chainparams, CConnman* connman, const std::atomic<bool>& interruptMsgProc) where they are guarded by pfrom->IsAddrRelayPeer()

    https://github.com/bitcoin/bitcoin/blob/a552e8477c5bcd22a5457f4f73a2fd6db8acd2c2/src/net_processing.cpp#L1985-L1999

    https://github.com/bitcoin/bitcoin/blob/a552e8477c5bcd22a5457f4f73a2fd6db8acd2c2/src/net_processing.cpp#L2964-L2982

    The last one in void AdvertiseLocal(CNode *pnode), whose only call-site is guarded by a pto->IsAddrRelayPeer() in bool PeerLogicValidation::SendMessages(CNode* pto)

    https://github.com/bitcoin/bitcoin/blob/a552e8477c5bcd22a5457f4f73a2fd6db8acd2c2/src/net_processing.cpp#L3552-L3553

  25. in src/net.h:732 in a552e8477c outdated
    728@@ -729,7 +729,7 @@ class CNode
    729 
    730     // flood relay
    731     std::vector<CAddress> vAddrToSend;
    732-    CRollingBloomFilter addrKnown;
    733+    std::unique_ptr<CRollingBloomFilter> m_addr_known;
    


    MarcoFalke commented at 7:27 pm on October 30, 2019:
    0    const std::unique_ptr<CRollingBloomFilter> m_addr_known;
    

    in the first commit:

    On a second thought, this should be made const and m_addr_relay_peer should be removed:

    • Slightly less code
    • Easier to review, because equivalence of m_addr_relay_peer == bool{m_addr_known} is obviously true
    • 4 bytes less memory per peer

    MarcoFalke commented at 7:27 pm on October 30, 2019:

    For the full proposed diff:

     0diff --git a/src/net.cpp b/src/net.cpp
     1index 3cea184ac3..84692d2a79 100644
     2--- a/src/net.cpp
     3+++ b/src/net.cpp
     4@@ -2669,7 +2669,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn
     5     // Don't relay addr messages to peers that we connect to as block-relay-only
     6     // peers (to prevent adversaries from inferring these links from addr
     7     // traffic).
     8-    m_addr_relay_peer(!block_relay_only),
     9+    m_addr_known{block_relay_only ? nullptr : MakeUnique<CRollingBloomFilter>(5000, 0.001)},
    10     id(idIn),
    11     nLocalHostNonce(nLocalHostNonceIn),
    12     nLocalServices(nLocalServicesIn),
    13@@ -2682,10 +2682,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn
    14         m_tx_relay = MakeUnique<TxRelay>();
    15     }
    16 
    17-    if (m_addr_relay_peer) {
    18-        m_addr_known = MakeUnique<CRollingBloomFilter>(5000, 0.001);
    19-    }
    20-
    21     for (const std::string &msg : getAllNetMessageTypes())
    22         mapRecvBytesPerMsgCmd[msg] = 0;
    23     mapRecvBytesPerMsgCmd[NET_MESSAGE_COMMAND_OTHER] = 0;
    24diff --git a/src/net.h b/src/net.h
    25index 327c06a306..d71e30377a 100644
    26--- a/src/net.h
    27+++ b/src/net.h
    28@@ -776,13 +776,12 @@ public:
    29 
    30     // flood relay
    31     std::vector<CAddress> vAddrToSend;
    32-    std::unique_ptr<CRollingBloomFilter> m_addr_known;
    33+    const std::unique_ptr<CRollingBloomFilter> m_addr_known;
    34     bool fGetAddr{false};
    35     int64_t nNextAddrSend GUARDED_BY(cs_sendProcessing){0};
    36     int64_t nNextLocalAddrSend GUARDED_BY(cs_sendProcessing){0};
    37 
    38-    const bool m_addr_relay_peer;
    39-    bool IsAddrRelayPeer() const { return m_addr_relay_peer; }
    40+    bool IsAddrRelayPeer() const { return bool{m_addr_known}; }
    41 
    42     // List of block ids we still have announce.
    43     // There is no final sorting before sending, as they are always sent immediately
    
  26. naumenkogs commented at 8:21 pm on October 30, 2019: member
    I agree. At least right now m_addr_known strongly correlates with m_addr_relay_peer. We can merge 2 variables. It’s also very unlikely that these 2 variables would ever mean different things.
  27. Minor refactoring to remove implied m_addr_relay_peer.
    Co-authored-by: MarcoFalke <falke.marco@gmail.com>
    b6d2183858
  28. naumenkogs force-pushed on Oct 31, 2019
  29. luke-jr commented at 4:02 pm on November 4, 2019: member

    This is only relevant for nodes that accept incoming connections? And those nodes are assumed to have a ton of resources.

    What? No. All nodes ideally should accept incoming connections, it doesn’t imply “a ton of resources”!

  30. MarcoFalke referenced this in commit 8f9df2ed88 on Nov 4, 2019
  31. MarcoFalke merged this on Nov 4, 2019
  32. MarcoFalke closed this on Nov 4, 2019

  33. sidhujag referenced this in commit 81143d9dd0 on Nov 7, 2019
  34. laanwj commented at 4:18 pm on May 25, 2020: member

    @naumenkogs Please set a different name than User in your commits if you want to be credited correctly in the future (I’ll fix it up manually for 0.20).

    0commit b6d2183858975abc961207c125c15791e531edcc
    1Author:     User <n…@gmail.com>
    2AuthorDate: Thu Oct 31 13:42:02 2019 -0400
    3Commit:     User <n…@gmail.com>
    4CommitDate: Thu Oct 31 13:42:02 2019 -0400
    5
    6    Minor refactoring to remove implied m_addr_relay_peer. 
    7    Co-authored-by: MarcoFalke <falke.marco@gmail.com>
    
  35. jasonbcox referenced this in commit 59d4627bec on Nov 3, 2020
  36. jasonbcox referenced this in commit 84f2368418 on Nov 3, 2020
  37. jasonbcox referenced this in commit a8b03bcc45 on Nov 3, 2020
  38. sidhujag referenced this in commit d3bf0e7c6b on Nov 10, 2020
  39. MarcoFalke locked this on Feb 15, 2022

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: 2024-10-30 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me