Bugfix: net_processing: Restore missing comma between peer and peeraddr in "receive version message" and "New ___ peer connected" #34293

pull luke-jr wants to merge 1 commits into bitcoin:master from luke-jr:fix_vermsg_missing_comma changing 1 files +6 −4

luke-jr commented at 8:34 PM on January 14, 2026: member

Versions prior to 29.x all included a comma here, and the general format of the message still includes commas between all other fields
DrahtBot commented at 8:34 PM on January 14, 2026: 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/34293.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK l0rinc, vasild, 0xB10C

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #34444 (refactor: rename remaining BIP14 subversion identifiers to user agent by l0rinc)
- #34389 (net/log: standardize peer+addr log formatting via LogPeer by l0rinc)
- #34181 (refactor: [p2p] Make ProcessMessage private again, Use references when non-null by maflcko)
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.
polespinasa commented at 9:43 PM on January 14, 2026: member

Trying to test this but in my logs I don't see a peeraddr after peer. How can I test this?
in src/net_processing.cpp:3675 in 9e689f5cb5 outdated
```
3672 |                    peer->m_starting_height, addrMe.ToStringAddrPort(), fRelay, pfrom.GetId(),
3673 | -                  pfrom.LogIP(fLogIPs), (mapped_as ? strprintf(", mapped_as=%d", mapped_as) : ""));
3674 | +                  fLogIPs ? "," : "", pfrom.LogIP(fLogIPs), (mapped_as ? strprintf(", mapped_as=%d", mapped_as) : ""));
3675 |  
3676 |          if (pfrom.IsPrivateBroadcastConn()) {
3677 |              if (fRelay) {
```
l0rinc commented at 11:01 PM on January 14, 2026:

Seems the original contained the leading comma: https://github.com/bitcoin/bitcoin/pull/28521/changes#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3L3570 (cc: @Sjors)

Edit: Can you please fix https://github.com/bitcoin/bitcoin/blob/b0b65336e76153f81c782e2a940ee6f925b32048/src/net_processing.cpp#L3803, too?

luke-jr commented at 3:31 AM on January 23, 2026:

Good catch, done.
l0rinc approved
l0rinc commented at 11:01 PM on January 14, 2026: contributor

code review ACK 9e689f5cb5271c668610cf9cd21ba04cc8a755b4
Bugfix: net_processing: Restore missing comma between peer and peeraddr in "receive version message" and "New ___ peer connected"
Versions prior to 29.x all included a comma here, and the general format of the message still includes commas between all other fields
ffd09f8a0d
luke-jr force-pushed on Jan 23, 2026
luke-jr commented at 3:31 AM on January 23, 2026: member

Trying to test this but in my logs I don't see a peeraddr after peer. How can I test this?

-logips option
luke-jr renamed this:
~~Bugfix: net_processing: Restore missing comma between peer and peeraddr in "receive version message"~~
Bugfix: net_processing: Restore missing comma between peer and peeraddr in "receive version message" and "New ___ peer connected"
on Jan 23, 2026
l0rinc commented at 12:12 PM on January 23, 2026: contributor

ACK ffd09f8a0d024423d55c07223a588d35c0a97c0c

This does fix an existing issue introduced in https://github.com/bitcoin/bitcoin/pull/28521/changes#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3L3570. I think we should fix all remaining LogPeer instances as well in a more systematic way, not just two of the instances, so I opened an alternative in #34389, added @luke-jr as a coauthor.
vasild approved
vasild commented at 11:19 AM on February 5, 2026: contributor

ACK ffd09f8a0d024423d55c07223a588d35c0a97c0c

Seems like this code would benefit from further improvements (in a followup?) to avoid repeated "peer=%s%d%Sss%%!*&*", fLogIPs ? "," : "", pfrom.LogIP(fLogIPs)

Thanks!
0xB10C commented at 12:41 PM on February 5, 2026: contributor

ACK ffd09f8a0d024423d55c07223a588d35c0a97c0c

I had noticed this while touching the New %s peer connected log in #34008 but it didn't bother me enough to fix it.
l0rinc commented at 12:51 PM on February 5, 2026: contributor

Note that this only fixes 2 occurrences, but there's an alternative where I've adjusted them in a more systematic way, see: https://github.com/bitcoin/bitcoin/pull/34389
DrahtBot added the label Needs rebase on Feb 7, 2026
DrahtBot commented at 1:33 AM on February 7, 2026: contributor



🐙 This pull request conflicts with the target branch and needs rebase.
fanquake commented at 11:02 AM on February 20, 2026: member

@luke-jr are you going to rebase this?
l0rinc commented at 11:07 AM on February 20, 2026: contributor

#34389 contains these changes already and applies it consistently to all other cases and is rebased. I have added @luke-jr as a coauthor to it.
sedited referenced this in commit e96d9e6492 on Mar 11, 2026
fanquake commented at 1:35 PM on March 11, 2026: member

This has been done as part of #34389.
fanquake closed this on Mar 11, 2026