Do not log p2p bip61 reject messages, improve log, add tests #28429
Conversation
Rationale for not logging these messages: - they are not "unknown" and were valid in pre-v20 versions of bitcoin core - peers have been observed that send a high number of these messages, thereby flooding the log when -debug=net Rationale for improving the log output when receiving unknown commands: - logging only a peer id isn't of much help after the fact in understanding the source of the message and debugging who/what/why
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Not sure. The details are already logged at the beginning of the connection, after the verack. So this seems odd to duplicate here. In any case, it seems like something that would need to be done for the whole codebase and not just here. |
maflcko
left a comment
There was a problem hiding this comment.
Not sure. Seems better to be consistent. Also, using a remote-controlled metric as a reason for a change seems hard to maintain and measure.
|
|
||
| const auto msg_str{SanitizeString(msg_type)}; | ||
|
|
||
| // Ignore BIP61 "reject" messages. They can be very frequent, so don't log. |
There was a problem hiding this comment.
Not sure. I am sure there are other message types that used to be valid and are now logged as "unknown". They can also be frequent, so it seems odd to use that as a reason whether to log or not. Also, the net layer will still log the message type, no?
There was a problem hiding this comment.
The only "unknown" messages I've seen these past weeks are these (though they are not unknown), and they can be so high-frequency (10-20k per day) that I'm not sure what the point of logging them is.
These aren't "unknown messages", though, so it could be reasonable to treat them differently. |
Yeah, but then you'll need to include a list of all known message types, because other unhandled message types can also be frequent for someone. Not sure if this is something we want to maintain. Seems easier to adjust the log to say "unhandled" instead of "unknown"? |
|
It doesn't seem inconsistent to me to ignore them. They are messages with a well-known meaning, and we can choose to handle them just as well as every other known message - it just so happens that in this case (a) the action to take when receiving such a message is "do nothing" and (b) we choose to not send such messages ourselves. |
|
I mean consistency with other message types that are known but unhandled, like the No objection, if other reviewers are fine with this. |
Can drop the log improvement if people prefer, but it provides info not in the connection log. In my development environment I have similar peer details for all of the higher-level peer events to better observe and understand them. These allowed uncovering the issues in #28248. (If you are referring to the |
If there is something missing from the connection log, it should be added there, not here, no? Locally I have a function |
luke-jr
left a comment
There was a problem hiding this comment.
Concept NACK. Even before "reject" support was removed, we still logged every one we got anyway (see fa25f43ac5692082dba3f90456c501eb08f1b75c).
And agree with @MarcoFalke on logging peer details
...per the IRC discussion around https://www.erisian.com.au/bitcoin-core-dev/log-2023-09-03.html#l-345.
Rationale for not logging the bip61 reject messages (as unknown commands):
they are not unknown and were valid before v20; see #p2p: Remove BIP61 reject messages #15437
peers have been observed that send a high number of these messages (I've seen 10k-20k messages per day from a single peer), thereby flooding the log when
-debug=netanother option would be to continue logging bip61 reject messages, but with the
tracelog level insteadRationale for improving the log output when receiving unknown commands: