p2p, refactor: Extract logic of announcing a local address into a private method #21534

The motivation of this PR is to extract and encapsulate logic in SendMessages() into smaller and focused methods, also motivated by a brief comment during a review of #21509.

This PR extracts the logic to maybe announce the nodes local address to a peer.

If there is sufficient support for this type of refactoring, then the intention would be that further changes should be made in separate PRs to extract each area of logic from SendMessages().

The idea would be for the changes to eventually look something like this:

bool PeerManagerImpl::SendMessages(CNode* pto) {
    ...
    {
        LOCK(cs_main);
        ...
        MaybeAnnounceLocalAddress(...);
        SendAddr(...);
        SendGetHeadersBlockSync(...);
        SendBlockAnnouncements(...);
        SendInv(...);
        ...
    }
    ...
}

The argument for this change would be, that it would be easier to read and understand the high level logic of SendMessages(). There other reason would be to separate and encapsulate each area of logic according to narrow responsibilities.

Since the logic in SendMessages() is important, I am concerned that moving the code around for style and readability may introduce unintended bugs, for example implementing safe and correct method signatures. I was also unable to find tests that specifically target announcing local addresses while in IBD or if the peer is block-relay-only.

Any feedback would greatly appreciated.

Thanks for this @ccdle12. It goes without saying that I'm very supportive of such changes. SendMessages() is a monster. It's very difficult to know whether their are unintended interactions between the different parts of the code. It's also impossible to isolate and test the functionality for the units of code.

The idea would be for the changes to eventually look something like this:

bool PeerManagerImpl::SendMessages(CNode* pto) {
    ...
    {
        LOCK(cs_main);
        ...
        MaybeAnnounceLocalAddress(...);
        SendAddr(...);
        SendGetHeadersBlockSync(...);
        SendBlockAnnouncements(...);
        SendInv(...);
        ...
    }
    ...
}

Or perhaps even better would be for each peer to have a list of tasks with times, and then pop tasks off the front when the current time has passed the task time. That'd stop us from making many redundant time comparisons in each SendMessages() invocation.

I've already done something very similar to this PR in #21236. Do you mind taking a look at that and seeing if it achieves your goals?

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #21528 ([net_processing] Reduce addr blackholes by amitiuttarwar)
• #21236 (Net processing: Extract addr send functionality into MaybeSendAddr() by jnewbery)

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.

Or perhaps even better would be for each peer to have a list of tasks with times, and then pop tasks off the front when the current time has passed the task time. That'd stop us from making many redundant time comparisons in each SendMessages() invocation.

I've already done something very similar to this PR in #21236. Do you mind taking a look at that and seeing if it achieves your goals?

Thanks @jnewbery, appreciate the feedback and taking a look at this. I'll check out #21236 and see if I can help anywhere.

closing since the changes proposed are covered in #21236