net_processing: reorder the code that handles the VERSION message #33792

1. 
   vasild commented at 3:01 pm on November 5, 2025: contributor

   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.

2. 
   DrahtBot commented at 3:01 pm on November 5, 2025: contributor

   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.

3. 
   vasild commented at 3:02 pm on November 5, 2025: contributor

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

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

4. 
   fanquake commented at 12:26 pm on November 7, 2025: member

   cc @ajtowns
5. laanwj added the label P2P on Nov 11, 2025
6. 
   w0xlt commented at 7:09 pm on November 12, 2025: contributor

   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 ?

7. 
   vasild commented at 11:15 am on November 13, 2025: contributor

   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:

   0if (...)
   1    A
   2if (...)
   3    MakeAndPushMessage()
   4if (...)
   5    B
   6if (...)
   7    MakeAndPushMessage()
   8if (...)
   9    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:

    0if (...)
    1    A
    2if (...)
    3    B
    4
    5if this is private broadcast connection // newly added condition
    6    return, end here
    7
    8if (...)
    9    MakeAndPushMessage()
   10if (...)
   11    MakeAndPushMessage()
   12if (...)
   13    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:

   0if (...)
   1    A
   2if (... and not private broadcast)
   3    MakeAndPushMessage()
   4if (...)
   5    B
   6if (... and not private broadcast)
   7    MakeAndPushMessage()
   8if (... and not private broadcast)
   9    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.

8. 
   vasild commented at 11:30 am on November 13, 2025: contributor

   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? :bulb:

9. net_processing: reorder the code that handles the VERSION message

   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.

   52149b0e2b
10. vasild force-pushed on Nov 13, 2025
11. 
    vasild commented at 5:48 pm on November 13, 2025: contributor

    431dd57d67...52149b0e2b: drop some hunks from the diff, m_addrman.Good() need not be moved around. Thanks, @w0xlt!
12. 
    w0xlt commented at 7:13 pm on November 13, 2025: contributor

    Thank you for the detailed explanation. ACK https://github.com/bitcoin/bitcoin/pull/33792/commits/52149b0e2b7897cc2ea059a9b50c4cb3b9fcd722
13. andrewtoth approved
14. 
    andrewtoth commented at 5:15 pm on November 14, 2025: contributor

    ACK 52149b0e2b7897cc2ea059a9b50c4cb3b9fcd722

    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.

15. 
    mzumsande commented at 4:29 pm on November 17, 2025: contributor

    Code Review ACK https://github.com/bitcoin/bitcoin/commit/52149b0e2b7897cc2ea059a9b50c4cb3b9fcd722

Contributors
vasild DrahtBot fanquake w0xlt andrewtoth mzumsande

Labels
P2P