p2p: Log addresses of stalling peers #27761

pull mzumsande wants to merge 1 commits into bitcoin:master from mzumsande:202305_log_more_ips changing 1 files +4 −4

mzumsande commented at 7:38 PM on May 25, 2023: contributor

This was suggested in #27705 by ArmchairCryptologist. It allows node operators that have the -logips option enabled to better identify potentially misbehaving peers and maybe ban them. This is especially helpful in case of inbound peers for which (dis)connections aren't logged per default, so it's impossible to use the debug log to connect their nodeId to an address unless the very noisy net debugging is enabled. In case of outbound peers for which the address is potentially logged when establishing the connection, this just adds some convenience.
DrahtBot commented at 7:38 PM on May 25, 2023: contributor



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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK stratospher, 0xB10C, instagibbs, jamesob

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label P2P on May 25, 2023
mzumsande force-pushed on May 25, 2023
DrahtBot added the label CI failed on May 25, 2023
fanquake requested review from instagibbs on May 25, 2023
fanquake requested review from 0xB10C on May 25, 2023
fanquake requested review from ajtowns on May 25, 2023

Type	Reviewers
ACK	stratospher, 0xB10C, instagibbs, jamesob

p2p: Log addresses of stalling peers

This allows node operators that have the -logips option enabled
to better identify potentially misbehaving peers and maybe
ban them.

fb02a3cd1a

in src/net_processing.cpp:5754 in 5421304c3c outdated

5750 | @@ -5751,7 +5751,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
5751 |              // Stalling only triggers when the block download window cannot move. During normal steady state,
5752 |              // the download window should be much larger than the to-be-downloaded set of blocks, so disconnection
5753 |              // should only happen during initial block download.
5754 | -            LogPrintf("Peer=%d is stalling block download, disconnecting\n", pto->GetId());
5755 | +            LogPrintf("Peer=%d%d is stalling block download, disconnecting\n", pto->GetId(), fLogIPs ? strprintf(" addr=%s", pto->addr.ToStringAddrPort()) : "");

ajtowns commented at 4:30 AM on May 26, 2023:

Should be Peer=%d%s (the result of strprintf isn't a number) and peeraddr=%s (for better consistency with other fLogIPs logs)

mzumsande commented at 5:00 AM on May 26, 2023:

fixed, thanks.

mzumsande force-pushed on May 26, 2023
stratospher commented at 8:02 AM on May 26, 2023: contributor

tACK fb02a3c.

nice to have this! cross checked locations where we'd like inbound peer/disconnection peeraddr logging. (you may have to rebase on master for green CI.)
0xB10C approved
0xB10C commented at 11:03 AM on May 26, 2023: contributor

Untested ACK fb02a3cd1a105bdf60ca39e1858e77685be88976

Somewhat related: net logging for specific IP subnets/addresses could also be useful for debugging something like #27705. Probably a bigger change though.
instagibbs approved
instagibbs commented at 3:07 PM on May 26, 2023: member

utACK fb02a3cd1a105bdf60ca39e1858e77685be88976
jamesob commented at 3:59 PM on May 26, 2023: member

github ACK https://github.com/bitcoin/bitcoin/commit/fb02a3cd1a105bdf60ca39e1858e77685be88976
fanquake merged this on May 26, 2023
fanquake closed this on May 26, 2023
sidhujag referenced this in commit 7dbcde8cc5 on May 26, 2023
mzumsande deleted the branch on May 30, 2023
Sjors commented at 2:31 PM on June 16, 2023: member

May I suggest a fine pairing? #27826
mzumsande commented at 2:53 PM on June 16, 2023: contributor

Yes, I'll review soon! It's been on my list anyway but I got distracted by other things...
luke-jr referenced this in commit adbdff5443 on Aug 16, 2023
bitcoin locked this on Jun 15, 2024