Unified number constants of getaddr return addrs #5919

See #5917

@tnull: i think it would be good to give a more detailed commit message.

ACK on replacing 1000 with a constant where appropriate.

NACK on the naming. MAX_ADDR_TO_SEND in net.h is clear, using ADDRMAN_GETADDR_MAX everywhere less so.

As for unifying them, I tend to say no unless there is a clear problem. Remember that there are various filtering steps that reduce the size of the list, for example to remove addresses already known, so starting off with 1000 entries may result in much less than 1000 being returned.

@jonasschnelli Amended the commit to have a better message. @laanwj Thats true, MAX_ADDR_TO_SEND is better, however, I didn't want to introduce circular dependencies, or modify many files for this first minimal patch, before I didn't hear from the developers. MAX_ADDR_TO_SEND can't be used in addrman.h/.cpp, while ADDRMAN_GETADDR_MAX is available in net.h, if we include addrman.h in it (instead of net.cpp). On less than 1000 addresses being returned: the same can be true for 2500...

Additionally, there is an other problem in the filtering I discovered: When nodes announce their addr for the first time, don't get elected trickle node, and then want to answer a getaddr-message, couldn't it happen, that their queued addr-message gets overwritten and therefore never gets flooded?

On less than 1000 addresses being returned: the same can be true for 2500...

But with a smaller probability.

Also, the 1000 number is part of the protocol implementation, the 2500 is from addrman. I don't think that changing a number in addrman should have an effect on protocol behaviour, so it seems weird to use addrman's constant inside net.

But with a smaller probability.

Of course! I argue that the concept of checking and reducing the set of addrs multiple times is flawed and since it's creating a new vector every time should introduce a good amount of overhead. But probably you're right, if the concept stays the same, it'll be better to leave it at 2500.

Well, the concept is not changing here, just the number, so I'm commenting on that :)

Have you measured the overhead? Without measurements from a profiler that show that this is actually a big time sink for a node, I wouldn't worry about it.

I don't think that changing a number in addrman should have an effect on protocol behaviour, so it seems weird to use addrman's constant inside net.

True again. Maybe this request can be closed, maybe I'll send another one just replacing some of the hardcoded "1000" number with MAX_ADDR_TO_SEND constant.

Does anyone have thoughts on the issue of the possibility that initial addr-messages get overwritten in the queue by PushAddress() and therefore never get sent?

You mean that the node's self-advertisement may get lost? I don't think that is a huge issue. It will re-advertise and for some peer, at some point it's bound to get through. Some unpredictability in getaddr may even be desirable to break assumptions about the returned data.

Ok, I understand if you think this is no huge issue. However, I fail to see how this unpredictability would be desirable (on top of randomizing returned addresses). Closing this request now, thanks for your time! :)