net_processing: reorder the code that handles the VERSION message

[vasild]

Change the order in which code snippets are executed as a result of receiving the VERSION message. Move the snippets that do MakeAndPushMessage() near the end. This makes it easier to interrupt the execution when no messages should be sent as a response to the VERSION messages, in private broadcast connections.

This is a non-functional change.

---

This is part of #29415 Broadcast own transactions only via short-lived Tor or I2P connections. Putting it in its own PR to reduce the size of #29415 and because it does not depend on the other commits from there.

[DrahtBot]

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33792.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	w0xlt, andrewtoth, mzumsande

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)

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.

[vasild]

This is just moving code around. Easier reviewed with ~/.gitconfig like:

[diff]
        colorMoved = dimmed-zebra
        colorMovedWS = allow-indentation-change

[fanquake]

cc @ajtowns

[w0xlt]

The reordering of WTXIDRELAY and SENDADDRV2 looks safe to me and keeps them before the tx‑reconciliation announcement.

What’s the rationale for moving m_addrman.Good(pfrom.addr) earlier ?

[vasild]

Good question, I guess it is a bit obscure. Let me elaborate. So, when we receive the VERSION message, its handling (inside the if (msg_type == NetMsgType::VERSION) {) currently goes like this:

if (...)
    A
if (...)
    MakeAndPushMessage()
if (...)
    B
if (...)
    MakeAndPushMessage()
if (...)
    some stuff that better be avoided for private broadcast connections

For private broadcast we don't want to push any extra messages. To do that I took the approach to reorder the code like:

if (...)
    A
if (...)
    B

if this is private broadcast connection // newly added condition
    return, end here

if (...)
    MakeAndPushMessage()
if (...)
    MakeAndPushMessage()
if (...)
    some stuff that better be avoided for private broadcast connections

another option would be to add "is private broadcast" check on every of the ifs, like:

if (...)
    A
if (... and not private broadcast)
    MakeAndPushMessage()
if (...)
    B
if (... and not private broadcast)
    MakeAndPushMessage()
if (... and not private broadcast)
    some stuff that better be avoided for private broadcast connections

or something else? I guess this can be achieved in more than one way. No strong opinion, the aim is to avoid calling some of the code.

Btw, MakeAndPushMessage() calls CConnman::PushMessage() which ignores messages other than the needed ones for private broadcast connections.

[vasild]

What’s the rationale for moving m_addrman.Good(pfrom.addr) earlier ?

Hmm, wait! m_addrman.Good() need not be earlier, it is one of the "some stuff that better be avoided". Then this can be simplified? 💡

[vasild]

431dd57d67...52149b0e2b: drop some hunks from the diff, m_addrman.Good() need not be moved around. Thanks, @w0xlt!

[w0xlt]

Thank you for the detailed explanation.
ACK 52149b0

[andrewtoth]

ACK 52149b0

Moving the WTXIDRELAY and SENDADDRV2 MakeAndPushMessages doesn't change the order of messages pushed. Nothing between below where they are moved from and above where they are moved to has an effect on any of the members of pfrom accessed in MakeAndPushMessage.

Moving mapped_as and the subsequent log above does not change anything with mapped_as either, since the pfrom.addr is not modified in any of the lines between where it is moved. None of the log parameters miss any changes being moved up either.

[mzumsande]

Code Review ACK 52149b0