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

naumenkogs · 2019-10-16T17:48:47Z

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.

maflcko

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.

src/bloom.h

naumenkogs · 2019-10-16T18:06:41Z

This is only relevant for nodes that accept incoming connections?

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

GChuf · 2019-10-16T18:51:17Z

Concept ACK.
Respectfully, assuming nodes have a ton of resources is dangerous - every optimization is potentially important (talking as a multiple nodes operator here).

src/bloom.h

maflcko · 2019-10-16T19:28:10Z

src/bloom.h

@@ -115,9 +115,6 @@ class CBloomFilter
 class CRollingBloomFilter
 {
 public:
-    // A random bloom filter calls GetRand() at creation time.
-    // Don't create global CRollingBloomFilter objects, as they may be
-    // constructed before the randomizer is properly initialized.


note to other reviewers: The comment no longer applies after 05fde14

maflcko · 2019-10-16T19:47:01Z

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 90407e6

Show signature and timestamp

Signature:

Timestamp of file with hash 283bf14cba6f831bf8491da4b8a98092329778b2783d5ed8f6bbf7462772ff0f -

src/net.h

GChuf · 2019-10-16T20:28:39Z

@MarcoFalke so, the result is actually ... an extra ~4.8 MB RAM usage?

naumenkogs · 2019-10-16T20:42:39Z

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

GChuf · 2019-10-16T21:24:38Z

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

maflcko · 2019-10-17T17:58:08Z

re-ACK 090b75c

Show signature and timestamp

Signature:

Timestamp of file with hash 7806780bf7d730ec30b25732f4026f506b9c7fbff0e3b47a2803142242aea1b6 -

DrahtBot · 2019-10-17T18:53:00Z

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

Conflicts

No conflicts as of last run.

laanwj · 2019-10-24T18:15:24Z

I think there should be NULL pointer checks or assertions at the locations that now use m_addr_known, just in case.

laanwj · 2019-10-28T15:49:32Z

ACK a552e84

maflcko · 2019-10-28T20:37:58Z

re-ACK a552e84

Show signature and timestamp

Signature:

Timestamp of file with hash 0ab34613ea38e4bc0035a5ffbe8b4c57fd1cbe756bb708ed3a66369644787338 -

dongcarl · 2019-10-30T16:55:14Z

ACK a552e84

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:

bitcoin/src/net_processing.cpp

Lines 3560 to 3564 in a552e84

    
           if (pto->IsAddrRelayPeer() && pto->nNextAddrSend < nNow) { 
        
               pto->nNextAddrSend = PoissonNextSend(nNow, AVG_ADDRESS_BROADCAST_INTERVAL); 
        
               std::vector<CAddress> vAddr; 
        
               vAddr.reserve(pto->vAddrToSend.size()); 
        
               assert(pto->m_addr_known);

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()

bitcoin/src/net_processing.cpp

Lines 2101 to 2128 in a552e84

    
           if (!pfrom->IsAddrRelayPeer()) { 
        
               return true; 
        
           } 
        
           if (vAddr.size() > 1000) 
        
           { 
        
               LOCK(cs_main); 
        
               Misbehaving(pfrom->GetId(), 20, strprintf("message addr size() = %u", vAddr.size())); 
        
               return false; 
        
           } 
        
           // Store the new addresses 
        
           std::vector<CAddress> vAddrOk; 
        
           int64_t nNow = GetAdjustedTime(); 
        
           int64_t nSince = nNow - 10 * 60; 
        
           for (CAddress& addr : vAddr) 
        
           { 
        
               if (interruptMsgProc) 
        
                   return true; 
        
               // We only bother storing full nodes, though this may include 
        
               // things which we would not make an outbound connection to, in 
        
               // part because we may make feeler connections to them. 
        
               if (!MayHaveUsefulAddressDB(addr.nServices) && !HasAllDesirableServiceFlags(addr.nServices)) 
        
                   continue; 
        
               if (addr.nTime <= 100000000 || addr.nTime > nNow + 10 * 60) 
        
                   addr.nTime = nNow - 5 * 24 * 60 * 60; 
        
               pfrom->AddAddressKnown(addr);

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()

bitcoin/src/net_processing.cpp

Lines 1326 to 1345 in a552e84

    
           auto sortfunc = [&best, &hasher, nRelayNodes](CNode* pnode) { 
        
               if (pnode->nVersion >= CADDR_TIME_VERSION && pnode->IsAddrRelayPeer()) { 
        
                   uint64_t hashKey = CSipHasher(hasher).Write(pnode->GetId()).Finalize(); 
        
                   for (unsigned int i = 0; i < nRelayNodes; i++) { 
        
                        if (hashKey > best[i].first) { 
        
                            std::copy(best.begin() + i, best.begin() + nRelayNodes - 1, best.begin() + i + 1); 
        
                            best[i] = std::make_pair(hashKey, pnode); 
        
                            break; 
        
                        } 
        
                   } 
        
               } 
        
           }; 
        
           auto pushfunc = [&addr, &best, nRelayNodes, &insecure_rand] { 
        
               for (unsigned int i = 0; i < nRelayNodes && best[i].first != 0; i++) { 
        
                   best[i].second->PushAddress(addr, insecure_rand); 
        
               } 
        
           }; 
        
           connman->ForEachNodeThen(std::move(sortfunc), std::move(pushfunc));

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()

bitcoin/src/net_processing.cpp

Lines 1985 to 1999 in a552e84

    
           if (!pfrom->fInbound && pfrom->IsAddrRelayPeer()) 
        
           { 
        
               // Advertise our address 
        
               if (fListen && !::ChainstateActive().IsInitialBlockDownload()) 
        
               { 
        
                   CAddress addr = GetLocalAddress(&pfrom->addr, pfrom->GetLocalServices()); 
        
                   FastRandomContext insecure_rand; 
        
                   if (addr.IsRoutable()) 
        
                   { 
        
                       LogPrint(BCLog::NET, "ProcessMessages: advertising address %s\n", addr.ToString()); 
        
                       pfrom->PushAddress(addr, insecure_rand); 
        
                   } else if (IsPeerAddrLocalGood(pfrom)) { 
        
                       addr.SetIP(addrMe); 
        
                       LogPrint(BCLog::NET, "ProcessMessages: advertising address %s\n", addr.ToString()); 
        
                       pfrom->PushAddress(addr, insecure_rand);

bitcoin/src/net_processing.cpp

Lines 2964 to 2982 in a552e84

    
           if (!pfrom->IsAddrRelayPeer()) { 
        
               LogPrint(BCLog::NET, "Ignoring \"getaddr\" from block-relay-only connection. peer=%d\n", pfrom->GetId()); 
        
               return true; 
        
           } 
        
           // Only send one GetAddr response per connection to reduce resource waste 
        
           //  and discourage addr stamping of INV announcements. 
        
           if (pfrom->fSentAddr) { 
        
               LogPrint(BCLog::NET, "Ignoring repeated \"getaddr\". peer=%d\n", pfrom->GetId()); 
        
               return true; 
        
           } 
        
           pfrom->fSentAddr = true; 
        
           pfrom->vAddrToSend.clear(); 
        
           std::vector<CAddress> vAddr = connman->GetAddresses(); 
        
           FastRandomContext insecure_rand; 
        
           for (const CAddress &addr : vAddr) { 
        
               if (!g_banman->IsBanned(addr)) { 
        
                   pfrom->PushAddress(addr, insecure_rand);

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

bitcoin/src/net_processing.cpp

Lines 3552 to 3553 in a552e84

    
           if (pto->IsAddrRelayPeer() && !::ChainstateActive().IsInitialBlockDownload() && pto->nNextLocalAddrSend < nNow) { 
        
               AdvertiseLocal(pto);

src/net.h

naumenkogs · 2019-10-30T20:21:58Z

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.

Co-authored-by: MarcoFalke <falke.marco@gmail.com>

luke-jr · 2019-11-04T16:02:53Z

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"!

…'t need it b6d2183 Minor refactoring to remove implied m_addr_relay_peer. (User) a552e84 added asserts to check m_addr_known when it's used (User) 090b75c p2p: Avoid allocating memory for addrKnown where we don't need it (User) Pull request description: 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](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-October/017428.html) and then we could save some more memory. This PR still saves memory for block-relay-only peers immediately after merging. Top commit has no ACKs. Tree-SHA512: e84d93b2615556d466f5ca0e543580fde763911a3bfea3127c493ddfaba8f05c8605cb94ff795d165af542b594400995a2c51338185c298581408687e7812463

… we don't need it b6d2183 Minor refactoring to remove implied m_addr_relay_peer. (User) a552e84 added asserts to check m_addr_known when it's used (User) 090b75c p2p: Avoid allocating memory for addrKnown where we don't need it (User) Pull request description: 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](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-October/017428.html) and then we could save some more memory. This PR still saves memory for block-relay-only peers immediately after merging. Top commit has no ACKs. Tree-SHA512: e84d93b2615556d466f5ca0e543580fde763911a3bfea3127c493ddfaba8f05c8605cb94ff795d165af542b594400995a2c51338185c298581408687e7812463

laanwj · 2020-05-25T16:18:59Z

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

commit b6d2183858975abc961207c125c15791e531edcc
Author:     User <n…@gmail.com>
AuthorDate: Thu Oct 31 13:42:02 2019 -0400
Commit:     User <n…@gmail.com>
CommitDate: Thu Oct 31 13:42:02 2019 -0400

    Minor refactoring to remove implied m_addr_relay_peer. 
    Co-authored-by: MarcoFalke <falke.marco@gmail.com>

Summary: > 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. We use `std::make_unique` instead of `MakeUnique` (see D2463) This is a backport of Core [[bitcoin/bitcoin#17164 | PR17164]] [1/3] bitcoin/bitcoin@090b75c Test Plan: `ninja all check-all` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8238

Summary: This is a backport of Core [[bitcoin/bitcoin#17164 | PR17164]] [2/3] bitcoin/bitcoin@a552e84 Depends on D8238 Test Plan: `ninja all check-all` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8239

Summary: Co-authored-by: MarcoFalke <falke.marco@gmail.com> This is a backport of Core [[bitcoin/bitcoin#17164 | PR17164]] [3/3] bitcoin/bitcoin@b6d2183 Depends on D8239 Test Plan: `ninja all check-all` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8240

… we don't need it b6d2183 Minor refactoring to remove implied m_addr_relay_peer. (User) a552e84 added asserts to check m_addr_known when it's used (User) 090b75c p2p: Avoid allocating memory for addrKnown where we don't need it (User) Pull request description: 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](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-October/017428.html) and then we could save some more memory. This PR still saves memory for block-relay-only peers immediately after merging. Top commit has no ACKs. Tree-SHA512: e84d93b2615556d466f5ca0e543580fde763911a3bfea3127c493ddfaba8f05c8605cb94ff795d165af542b594400995a2c51338185c298581408687e7812463

fanquake added the P2P label Oct 16, 2019

naumenkogs mentioned this pull request Oct 16, 2019

p2p: Avoid relaying ADDR messages to SPV clients #17163

Closed

fanquake added the Resource usage label Oct 16, 2019

maflcko reviewed Oct 16, 2019

View reviewed changes

src/bloom.h Outdated Show resolved Hide resolved

src/bloom.h Outdated Show resolved Hide resolved

naumenkogs force-pushed the addr_relay_optimization_4 branch from e65af6e to 90407e6 Compare October 16, 2019 18:10

dongcarl reviewed Oct 16, 2019

View reviewed changes

src/bloom.h Show resolved Hide resolved

maflcko reviewed Oct 16, 2019

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

p2p: Avoid allocating memory for addrKnown where we don't need it

090b75c

naumenkogs force-pushed the addr_relay_optimization_4 branch from 26802de to 090b75c Compare October 16, 2019 21:06

added asserts to check m_addr_known when it's used

a552e84

maflcko reviewed Oct 30, 2019

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

Minor refactoring to remove implied m_addr_relay_peer.

b6d2183

Co-authored-by: MarcoFalke <falke.marco@gmail.com>

naumenkogs force-pushed the addr_relay_optimization_4 branch from ccf5aa7 to b6d2183 Compare October 31, 2019 17:42

maflcko merged commit b6d2183 into bitcoin:master Nov 4, 2019

maflcko mentioned this pull request Mar 31, 2020

BIP37: 'getdata' request for filtered blocks is answered with 'merkleblock's even if no filter is set #18483

Closed

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

maflcko mentioned this pull request May 27, 2020

Change UpdateForDescendants to use Epochs #18191

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Uh oh!

Conversation

naumenkogs commented Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

naumenkogs commented Oct 16, 2019

Uh oh!

GChuf commented Oct 16, 2019

Uh oh!

Uh oh!

maflcko Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 16, 2019

Uh oh!

Uh oh!

GChuf commented Oct 16, 2019

Uh oh!

naumenkogs commented Oct 16, 2019

Uh oh!

GChuf commented Oct 16, 2019

Uh oh!

maflcko commented Oct 17, 2019

Uh oh!

DrahtBot commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Oct 24, 2019

Uh oh!

laanwj commented Oct 28, 2019

Uh oh!

maflcko commented Oct 28, 2019

Uh oh!

dongcarl commented Oct 30, 2019

Potential Followups

Review Notes

Uh oh!

Uh oh!

naumenkogs commented Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Nov 4, 2019

Uh oh!

laanwj commented May 25, 2020

Uh oh!

Uh oh!

naumenkogs commented Oct 16, 2019 •

edited

Loading

DrahtBot commented Oct 17, 2019 •

edited

Loading

naumenkogs commented Oct 30, 2019 •

edited

Loading