p2p: Return a max of 1000 addrs from CAddrMan::GetAddr #14154

Empact commented at 7:03 PM on September 5, 2018: member

This is the documented max size for an addr response, and was separately being limited in PushAddress to 1000 via MAX_ADDR_TO_SEND. No point in selecting a too-large random set then filtering it randomly down to a smaller set.

p2p: Return a max of 1000 addrs from CAddrMan::GetAddr

This is the documented max size for an addr response, and was separately being
limited in PushAddress to 1000 via MAX_ADDR_TO_SEND. No point in selecting a
too-large random set then filtering it randomly down to a smaller set.

e6d375b1c0

MarcoFalke commented at 7:13 PM on September 5, 2018: member

I think addrman would return a larger list that is node-agnostic and then net would filter that list taking into account the node state, no?

in src/addrman.h:175 in e6d375b1c0 outdated

 171 | @@ -172,7 +172,7 @@ class CAddrInfo : public CAddress
 172 |  #define ADDRMAN_GETADDR_MAX_PCT 23
 173 |  
 174 |  //! the maximum number of nodes to return in a getaddr call
 175 | -#define ADDRMAN_GETADDR_MAX 2500
 176 | +#define ADDRMAN_GETADDR_MAX 1000

practicalswift commented at 7:50 PM on September 5, 2018:

When we're editing this file – what about skipping the use of the preprocessor and use proper constants instead? :-)

DrahtBot commented at 8:09 PM on September 5, 2018: member

Note to reviewers: This pull request conflicts with the following ones:

#14033 (p2p: Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater by Empact)
#13115 (addrman: Add Clang thread safety annotations for variables guarded by cs_addrMan by practicalswift)

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.

Empact commented at 8:20 PM on September 5, 2018: member

@MarcoFalke the only call to GetAddr / GetAddr_ is in service of this GETADDR response: https://github.com/bitcoin/bitcoin/blob/95d731d9b38819cb822dbcd365972a14ae78d21f/src/net_processing.cpp#L2747-L2750

PushAddress just overwrites entry if it's beyond the limit: https://github.com/bitcoin/bitcoin/blob/95d731d9b38819cb822dbcd365972a14ae78d21f/src/net.h#L816-L828

fanquake added the label P2P on Sep 5, 2018

gmaxwell commented at 7:06 PM on September 6, 2018: contributor

I thought the two layer setup was so that multiple probes would get more addresses but not enumerate the whole table.

gmaxwell commented at 11:21 PM on September 8, 2018: contributor

@sipa ^

in src/net_processing.cpp:1760 in e6d375b1c0 outdated

1756 | @@ -1757,7 +1757,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
1757 |              }
1758 |  
1759 |              // Get recent addresses
1760 | -            if (pfrom->fOneShot || pfrom->nVersion >= CADDR_TIME_VERSION || connman->GetAddressCount() < 1000)
1761 | +            if (pfrom->fOneShot || pfrom->nVersion >= CADDR_TIME_VERSION || connman->GetAddressCount() < ADDRMAN_GETADDR_MAX)

laanwj commented at 6:32 AM on September 11, 2018:

I like changing these to a constant, however "maximum number of addresses in GETADDR" is a P2P layer constant, I don't think it should use addrman's internal constant. src/protocol.h would be a better place.

MarcoFalke commented at 1:36 PM on September 11, 2018: member

@Empact PushAddr will return early when addrKnown.contains it. So when vAddr is of size 1000, it might very well send less than 1000 addresses back in the response, even though there are plenty to send. No?

Empact commented at 12:38 AM on September 12, 2018: member

@MarcoFalke yep that sounds like the explanation. Thanks!

Empact closed this on Sep 12, 2018

Empact reopened this on Sep 12, 2018

Don't return known addresses from CConnman::GetAddresses

These addresses are used expressly for sending to a node in response to
GETADDR, so we may as well skip them over in the lookup soas to return the
appropriate number.

68a8acca8e

Empact force-pushed on Sep 16, 2018

Empact closed this on Sep 16, 2018

MarcoFalke locked this on Sep 8, 2021