p2p: clean up Misbehaving() #19583
p2p: clean up Misbehaving() #19583
Conversation
|
This is the next PR in the #19398 sequence to move application data into net_processing. |
Based on review comments from Marco Falke and Jon Atack.
Misbehaving() could optionally take a debug string for printing to the log file. Make this mandatory and always provide the string. A couple of additional minor changes: - remove the unnecessary forward declaration of Misbehaving() - don't include the nodeid or newline in the passed debug message. Misbehaving() adds these itself.
- Make const things const. - Replace conditional return with assert. - Don't log the peer's IP address. - Log the name Misbehaving directly instead of relying on __func__.
2b5057d to
a8865f8
Compare
|
Thanks for the quick review @jonatack
Done |
|
ACK a8865f8 |
|
Thanks for the review @promag!
We shouldn't be logging the peer IP inside net processing, and the I believe the only other place we do is during the version handshake. See #19472 (comment). |
|
Concept ACK. |
| { | ||
| if (howmuch == 0) | ||
| return; | ||
| assert(howmuch > 0); |
There was a problem hiding this comment.
Bugs me that I can't find a way to check this at compile time (like static_assert), being that we're always giving this parameter an integer literal :/
There was a problem hiding this comment.
If you make it a templated function, you can pass in howmuch as a template parameter, enabling static_asserts on it. That may be a bit overkill, though.
There was a problem hiding this comment.
That may be a bit overkill, though.
And also would cause bloat in the built executable.
It's easy enough to grep all instances of Misbehaving. We don't need to automate everything :)
a8865f8 [net processing] Tidy up Misbehaving() (John Newbery) d15b3af [net processing] Always supply debug message to Misbehaving() (John Newbery) 634144a [net processing] Fixup MaybeDiscourageAndDisconnect() style (John Newbery) Pull request description: This PR makes a few minor clean-ups to `Misbehaving()` in preparation to move it out of the cs_main lock. There are very minor logging changes but otherwise no functional changes. ACKs for top commit: troygiorshev: tACK a8865f8 jonatack: ACK a8865f8 fjahr: Code review ACK a8865f8 promag: Code review ACK a8865f8. Tree-SHA512: 98fb4f5f76399715545a1ea19290dcebfc8cb4eff72a1d3555dd3de6e184040bb8668c9651dab21db0dfd8e674e53a5977105ef76547146c9f6fa6b4b9d2ba59
Summary: ``` This PR makes a few minor clean-ups to Misbehaving() in preparation to move it out of the cs_main lock. There are very minor logging changes but otherwise no functional changes. ``` Backport of core [[bitcoin/bitcoin#19583 | PR19583]]. Depends on D8792. Some of our Misbehaving messages are updated to match core messages. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D8793
This PR makes a few minor clean-ups to
Misbehaving()in preparation to move it out of the cs_main lock.There are very minor logging changes but otherwise no functional changes.