net: Avoid redundant and confusing FAILED log #19293

1. 
   MarcoFalke commented at 10:59 am on June 16, 2020: member

   Remove a redundant and confusing “FAILED” log message and gets rid of the unused return type in ProcessMessage
2. net: Avoid redundant and confusing FAILED log

   Every `return false` is preceeded by a detailed debug log message to
   explain that a disconnect or misbehavior happened. Logging another
   generic "FAILED" message seems redundant.
   
   Also, the size of the message and the message type has already been
   logged and is thus redundant as well.
   
   Finally, claiming that message processing FAILED seems odd, because the
   message was fully processed to the point where it was concluded that the
   peer should be either disconnected or marked as misbehaving.

   fac12ebf4f
3. net: Remove dead logging code

   fRet is never false, so the dead code can be removed and the return type
   can be made void

   fa1904e5f0
4. MarcoFalke added the label Utils/log/libs on Jun 16, 2020
5. 
   MarcoFalke commented at 11:19 am on June 16, 2020: member

   The refactor commit to remove dead code can be reviewed with --word-diff-regex=. -U0
6. 
   practicalswift commented at 2:44 pm on June 16, 2020: contributor

   Concept ACK: higher signal to noise in our logging is welcome
7. 
   troygiorshev commented at 5:50 pm on June 16, 2020: contributor

   ACK fa1904e5f0d164fbcf41398f9ebbaafe82c28419

   Reviewed, ran tests, checked that no tests relied on the removed log message.

8. 
   glozow commented at 9:49 pm on June 16, 2020: member

   utACK https://github.com/bitcoin/bitcoin/commit/fa1904e5f0d164fbcf41398f9ebbaafe82c28419 Everywhere that fails will log from Misbehaving or right before returning anyway.
9. 
   DrahtBot commented at 9:55 pm on June 16, 2020: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #19306 (refactor: Replace RecursiveMutex with Mutex in CTxMemPool by hebasto)
   • #19272 (net, test: invalid p2p messages and test framework improvements by jonatack)
   • #19204 (p2p: Reduce inv traffic during IBD by MarcoFalke)
   • #19174 (refactor: replace CConnman/BanMan pointers by references in net_processing.cpp by theStack)
   • #19107 (p2p: Refactor, move all header verification into the network layer, without changing behavior by troygiorshev)
   • #18638 (net: Use mockable time for ping/pong, add tests by MarcoFalke)

   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.

10. fanquake requested review from jnewbery on Jun 17, 2020
11. 
    hebasto commented at 11:38 am on June 17, 2020: member

    Concept ACK.
12. 
    jnewbery commented at 3:04 pm on June 17, 2020: member

    utACK fa1904e5f0d164fbcf41398f9ebbaafe82c28419

    The “return false to log” pattern is inconsistently applied, so we should just make it ProcessMessage’s responsibility to log in all cases.

    There are just a couple of places where this could make logging a little less clear: those where no log string is passed to the Misbehaving() call and we were previously returning false to log the message type and size. No need to fix that in this PR. Perhaps a future PR could remove the default message parameter from Misbehaving() and make sure that all calls include a helpful log message.

13. fanquake requested review from naumenkogs on Jun 19, 2020
14. 
    naumenkogs commented at 8:31 am on June 19, 2020: member

    utACK fa1904e
15. fanquake merged this on Jun 19, 2020
16. fanquake closed this on Jun 19, 2020

    ---

17. 
    fanquake commented at 9:19 am on June 19, 2020: member

    @amitiuttarwar you may also have been interested here.
18. MarcoFalke deleted the branch on Jun 19, 2020
19. sidhujag referenced this in commit ce1acf0098 on Jul 7, 2020
20. Fabcien referenced this in commit 25d0b1e38a on Jan 6, 2021
21. DrahtBot locked this on Feb 15, 2022

Contributors
MarcoFalke practicalswift troygiorshev glozow DrahtBot hebasto jnewbery naumenkogs fanquake

Review Requested
jnewbery naumenkogs

Labels
Utils/log/libs