Hello,
looking at the code which responds to a getaddr-message, I discovered that addrman.GetAddr() is only called once in main.cpp (see https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L4054) to get addresses which are then queued via PushAddress().
GetAddr_ returns random 23% (ADDRMAN_GETADDR_MAX_PCT) of the addrman's stored addresses, but no more than 2500.
But PushAddress() checks again against MAX_ADDR_TO_SEND which is 1000, as receiving nodes reject addr messages with a payload larger than 1000 addresses. And it replaces random addresses if the vector already has 1000 in it.
Then, in SendMessages(), the addresses-vector is split again before sending in packages of 1000 addresses (but it should never be bigger than 1000, as PushAddress() already checked that?!).
So the number of addresses returned is 2500, then we select 1000 randomly out of these, then we check if they are more then 1000 (probably always false) and split the vector if they are, before we send them.
I propose to get rid of one of the constants (I'd say MAX_ADDR_TO_SEND), and reduce ADDRMAN_GETADDR_MAX to 1000.
What do you say? Am I missing something, or is this chain of reducing addresses just confusing bloat which can go away? See my pull request for minimal change, maybe some of the redundant checks can be removed, also?
Best Regards!