p2p, refactor: Do not over-reserve vAddr capacity #19567

hebasto commented at 1:00 PM on July 22, 2020: member

This PR does not change behavior.

p2p, refactor: Use MAX_ADDR_TO_SEND instead of magic numbers f1012bfd5b

p2p, refactor: Do not over-reserve vAddr capacity 7aad9b2f02

fanquake added the label P2P on Jul 22, 2020

DrahtBot commented at 2:20 PM on July 22, 2020: 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:

#19503 (Add parameter feature to serialization and use it for CAddress by sipa)
#18991 (Cache responses to GETADDR to prevent topology leaks by naumenkogs)

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.

laanwj commented at 3:27 PM on July 22, 2020: member

Code review ACK 7aad9b2f02d0c62c05134e24b7274c27bb0a4624

theStack approved

theStack commented at 3:46 PM on July 22, 2020: member

Code-Review ACK https://github.com/bitcoin/bitcoin/pull/19567/commits/7aad9b2f02d0c62c05134e24b7274c27bb0a4624 :coffee:

fanquake requested review from jnewbery on Jul 22, 2020

fanquake requested review from sipa on Jul 22, 2020

in src/net_processing.cpp:3930 in 7aad9b2f02

3926 | @@ -3927,17 +3927,16 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
3927 |          if (pto->IsAddrRelayPeer() && pto->m_next_addr_send < current_time) {
3928 |              pto->m_next_addr_send = PoissonNextSend(current_time, AVG_ADDRESS_BROADCAST_INTERVAL);
3929 |              std::vector<CAddress> vAddr;
3930 | -            vAddr.reserve(pto->vAddrToSend.size());
3931 | +            vAddr.reserve(std::min(pto->vAddrToSend.size(), (size_t)MAX_ADDR_TO_SEND));

MarcoFalke commented at 6:22 AM on July 23, 2020:

style-nit: Instead of c-style casting of one or both arguments, selecting the exact template will force the type of both arguments and the return type as well. (Just casting one argument could still yield a different return type otherwise). Though, please don't change if there is nothing else to change

            vAddr.reserve(std::min<size_t>(pto->vAddrToSend.size(), MAX_ADDR_TO_SEND));

vasild commented at 7:19 AM on July 23, 2020:

Or change the type of MAX_ADDR_TO_SEND from unsigned int to size_t (std::vector::size() always returns size_t).

hebasto commented at 7:24 AM on July 23, 2020:

Or change the type of MAX_ADDR_TO_SEND from unsigned int to size_t (std::vector::size() always returns size_t).

To be pedantic, std::vector::size() always returns size_type which is

Unsigned integer type (usually std::size_t)

:)

vasild commented at 8:05 AM on July 23, 2020:

:ok_hand: :bulb:

MarcoFalke commented at 6:27 AM on July 23, 2020: member

cc @vasild

vasild approved

vasild commented at 7:11 AM on July 23, 2020: member

ACK 7aad9b2

vasild commented at 7:17 AM on July 23, 2020: member

Amazingly I have the following, almost identical, patch lurking locally! Notice that in this case there are two other magic numbers 1000 in the code which are not MAX_ADDR_TO_SEND.

diff --git a/src/net.h b/src/net.h
index a72af83ee..b059c28f8 100644
--- a/src/net.h
+++ b/src/net.h
@@ -53,13 +53,13 @@ static const int TIMEOUT_INTERVAL = 20 * 60;
 static const int FEELER_INTERVAL = 120;
 /** The maximum number of entries in an 'inv' protocol message */
 static const unsigned int MAX_INV_SZ = 50000;
 /** The maximum number of entries in a locator */
 static const unsigned int MAX_LOCATOR_SZ = 101;
 /** The maximum number of new addresses to accumulate before announcing. */
-static const unsigned int MAX_ADDR_TO_SEND = 1000;
+static constexpr size_t MAX_ADDR_TO_SEND = 1000;
 /** Maximum length of incoming protocol messages (no message over 4 MB is currently acceptable). */
 static const unsigned int MAX_PROTOCOL_MESSAGE_LENGTH = 4 * 1000 * 1000;
 /** Maximum length of the user agent string in `version` message */
 static const unsigned int MAX_SUBVERSION_LENGTH = 256;
 /** Maximum number of automatic outgoing nodes over which we'll relay everything (blocks, tx, addrs, etc) */
 static const int MAX_OUTBOUND_FULL_RELAY_CONNECTIONS = 8;
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index f63d048aa..5627910ea 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2059,13 +2059,13 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec
                     LogPrint(BCLog::NET, "ProcessMessages: advertising address %s\n", addr.ToString());
                     pfrom->PushAddress(addr, insecure_rand);
                 }
             }
 
             // Get recent addresses
-            if (pfrom->fOneShot || pfrom->nVersion >= CADDR_TIME_VERSION || connman->GetAddressCount() < 1000)
+            if (pfrom->fOneShot || pfrom->nVersion >= CADDR_TIME_VERSION || connman->GetAddressCount() < 1000 /* is not MAX_ADDR_TO_SEND */)
             {
                 connman->PushMessage(pfrom, CNetMsgMaker(nSendVersion).Make(NetMsgType::GETADDR));
                 pfrom->fGetAddr = true;
             }
             connman->MarkAddressGood(pfrom->addr);
         }
@@ -2154,19 +2154,18 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec
 
     if (msg_type == NetMsgType::ADDR) {
         std::vector<CAddress> vAddr;
         vRecv >> vAddr;
 
         // Don't want addr from older versions unless seeding
-        if (pfrom->nVersion < CADDR_TIME_VERSION && connman->GetAddressCount() > 1000)
+        if (pfrom->nVersion < CADDR_TIME_VERSION && connman->GetAddressCount() > 1000 /* is not MAX_ADDR_TO_SEND */)
             return true;
         if (!pfrom->IsAddrRelayPeer()) {
             return true;
         }
-        if (vAddr.size() > 1000)
-        {
+        if (vAddr.size() > MAX_ADDR_TO_SEND) {
             LOCK(cs_main);
             Misbehaving(pfrom->GetId(), 20, strprintf("message addr size() = %u", vAddr.size()));
             return false;
         }
 
         // Store the new addresses
@@ -2196,13 +2195,13 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec
             }
             // Do not store addresses outside our network
             if (fReachable)
                 vAddrOk.push_back(addr);
         }
         connman->AddNewAddresses(vAddrOk, pfrom->addr, 2 * 60 * 60);
-        if (vAddr.size() < 1000)
+        if (vAddr.size() < MAX_ADDR_TO_SEND)
             pfrom->fGetAddr = false;
         if (pfrom->fOneShot)
             pfrom->fDisconnect = true;
         return true;
     }
 
@@ -3591,22 +3590,22 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
         //
         // Message: addr
         //
         if (pto->IsAddrRelayPeer() && pto->nNextAddrSend < nNow) {
             pto->nNextAddrSend = PoissonNextSend(nNow, AVG_ADDRESS_BROADCAST_INTERVAL);
             std::vector<CAddress> vAddr;
-            vAddr.reserve(pto->vAddrToSend.size());
+            vAddr.reserve(std::min(pto->vAddrToSend.size(), MAX_ADDR_TO_SEND));
             assert(pto->m_addr_known);
             for (const CAddress& addr : pto->vAddrToSend)
             {
                 if (!pto->m_addr_known->contains(addr.GetKey()))
                 {
                     pto->m_addr_known->insert(addr.GetKey());
                     vAddr.push_back(addr);
-                    // receiver rejects addr messages larger than 1000
-                    if (vAddr.size() >= 1000)
+                    // Receiver would reject addr messages with too many addresses in them.
+                    if (vAddr.size() >= MAX_ADDR_TO_SEND)
                     {
                         connman->PushMessage(pto, msgMaker.Make(NetMsgType::ADDR, vAddr));
                         vAddr.clear();
                     }
                 }
             }

</details>

promag commented at 8:18 AM on July 23, 2020: member

Code review ACK 7aad9b2f02d0c62c05134e24b7274c27bb0a4624.

jonatack commented at 9:13 AM on July 23, 2020: member

Amazingly I have the following, almost identical, patch lurking locally! Notice that in this case there are two other magic numbers 1000 in the code which are not MAX_ADDR_TO_SEND.

Same! :smile: Therefore, concept ACK modulo @MarcoFalke's suggestion and @vasild's added comments.

jnewbery changes_requested

jnewbery commented at 9:24 AM on July 23, 2020: member

This issue is fixed by #18991, which I think is just about ready for merge. Therefore instead of merging this and causing a rebase of that PR, I think we close this in favour of #18991.

cc @naumenkogs

hebasto commented at 7:51 AM on July 24, 2020: member

@jnewbery

This issue is fixed by #18991...

Actually, the 7aad9b2f02d0c62c05134e24b7274c27bb0a4624 "p2p, refactor: Do not over-reserve vAddr capacity" commit is a fix in this PR, and this line is not touched in #18991.

To avoid #18991 rebasing I can see two possibilities:

drop the first commit in this PR, or
cherry-pick the second commit into #18991

Which variant is preferable?

jnewbery commented at 8:11 AM on July 24, 2020: member

I've looked a bit more closely at this PR, and I don't understand what it achieves. The supposed functional change is:

-            vAddr.reserve(pto->vAddrToSend.size());
+            vAddr.reserve(std::min(pto->vAddrToSend.size(), (size_t)MAX_ADDR_TO_SEND));

but vAddrToSend can never grow larger than MAX_ADDR_TO_SEND (see PushAddress() which is the only place that elements are added to vAddrToSend), so this doesn't seem to change anything.

hebasto commented at 8:21 AM on July 24, 2020: member

@jnewbery Correct. Closing.

hebasto closed this on Jul 24, 2020

jnewbery commented at 8:36 AM on July 24, 2020: member

Thanks @hebasto . The code here is confusing, mostly due to the fact that AddrMan and net_processing are both setting their own limits on the size of of getaddr responses (ADDRMAN_GETADDR_MAX and MAX_ADDR_TO_SEND respectively). The size check inside the for loop is also unnecessary and confusing since vAddr never grows larger than 1000:

                    // receiver rejects addr messages larger than 1000
                    if (vAddr.size() >= 1000)
                    {
                        connman->PushMessage(pto, msgMaker.Make(NetMsgType::ADDR, vAddr));
                        vAddr.clear();
                    }

I have a branch which makes this a bit clearer here: https://github.com/jnewbery/bitcoin/tree/2020-07-addrman-get which could be PR'ed after #18991 is in.

vasild commented at 8:39 AM on July 24, 2020: member

I guess the reason for the confusion is this code:

            for (const CAddress& addr : pto->vAddrToSend)
            { 
...
                    vAddr.push_back(addr);
                    // receiver rejects addr messages larger than 1000
                    if (vAddr.size() >= 1000)

which would be better if something like:

diff --git i/src/net_processing.cpp w/src/net_processing.cpp
index 7a58de35d..aa8f1be0e 100644
--- i/src/net_processing.cpp
+++ w/src/net_processing.cpp
@@ -3924,31 +3924,30 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
         //
         // Message: addr
         //
         if (pto->IsAddrRelayPeer() && pto->m_next_addr_send < current_time) {
             pto->m_next_addr_send = PoissonNextSend(current_time, AVG_ADDRESS_BROADCAST_INTERVAL);
             std::vector<CAddress> vAddr;
-            vAddr.reserve(pto->vAddrToSend.size());
+            // Do not reserve vAddr as that is unproven micro optimization and
+            // we could also end up over-reserving because not all addresses
+            // from vAddrToSend are added to vAddr
+            //vAddr.reserve(pto->vAddrToSend.size());
             assert(pto->m_addr_known);
             for (const CAddress& addr : pto->vAddrToSend)
             {
                 if (!pto->m_addr_known->contains(addr.GetKey()))
                 {
                     pto->m_addr_known->insert(addr.GetKey());
                     vAddr.push_back(addr);
-                    // receiver rejects addr messages larger than 1000
-                    if (vAddr.size() >= 1000)
-                    {
-                        connman->PushMessage(pto, msgMaker.Make(NetMsgType::ADDR, vAddr));
-                        vAddr.clear();
-                    }
                 }
             }
             pto->vAddrToSend.clear();
-            if (!vAddr.empty())
+            if (!vAddr.empty()) {
+                assert(vAddr.size() <= MAX_ADDR_TO_SEND);
                 connman->PushMessage(pto, msgMaker.Make(NetMsgType::ADDR, vAddr));
+            }
             // we only send the big addr message once
             if (pto->vAddrToSend.capacity() > 40)
                 pto->vAddrToSend.shrink_to_fit();
         }
 
         // Start block sync

</details>

jnewbery commented at 9:22 AM on July 24, 2020: member

@vasild I agree with removing the if block from within the loop, and adding the assert. However, I think the reserve is still useful. It ensures that we only ever have to do one allocation.

Even better would be to move the vector out of vAddrToSend (since it gets cleared afterwards anyway) and use remove-erase to filter out the unwanted addresses.

jnewbery commented at 2:47 PM on July 25, 2020: member

Even better would be to move the vector out of vAddrToSend (since it gets cleared afterwards anyway) and use remove-erase to filter out the unwanted addresses.

I've implemented this in the Refactor MaybeSendAddr() commit of https://github.com/jnewbery/bitcoin/tree/2020-06-cs-main-split. @hebasto - if you agree that's the right change, feel free to either take that commit (everything between [net] Remove unnecessary cs_sendProcessing lock annotation and [net processing] Refactor MaybeSendAddr() can be taken independently from the rest of that branch) or help by reviewing #19583 :)

DrahtBot locked this on Feb 15, 2022