log: Avoid treating remote misbehvior as local system error #19526

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2007-logErrorVal changing 2 files +20 −16
  1. MarcoFalke commented at 9:34 AM on July 15, 2020: member

    When logging failures of CheckBlockHeader (high-hash), they are always logged as system error. This is problematic for several reasons:

    • Submitting a blockheader that fails CheckBlockHeader over RPC will result in a debug log line that starts with ERROR. Proper behaviour should be to log not anything and instead only return the failure reason to the RPC user. This pull does not fix this issue entirely, but is a good first step in the right direction.

    • A misbehaving peer that sends us an invalid block header that fails CheckBlockHeader will result in a debug log line that starts with ERROR. Proper behavior should be to log the remote peer misbehavior if logging for that category was enabled. This pull fixes this issue for CheckBlockHeader and other functions can be adjusted as well if needed in follow-ups. This should be a good first step in the right direction.

  2. MarcoFalke added the label Validation on Jul 15, 2020
  3. MarcoFalke added the label Utils/log/libs on Jul 15, 2020
  4. MarcoFalke force-pushed on Jul 15, 2020
  5. MarcoFalke force-pushed on Jul 15, 2020
  6. jonatack commented at 10:32 AM on July 15, 2020: member

    Concept ACK. A quick skim of the code LGTM.

  7. MarcoFalke force-pushed on Jul 15, 2020
  8. practicalswift commented at 11:18 AM on July 15, 2020: contributor

    ACK fa3e94a3353dbbed98a15dd538fe87c153b821e9 -- patch looks correct and misbehaving peers should not be able to cause us to write to disk

  9. MarcoFalke force-pushed on Jul 15, 2020
  10. refactor: Switch ValidationState mode to C++11 enum class fa492895b5
  11. log: Avoid treating remote misbehvior as local system error fa56eda58e
  12. MarcoFalke force-pushed on Jul 15, 2020
  13. MarcoFalke commented at 2:53 PM on July 15, 2020: member

    Please excuse the repeated force pushes. There has been a windows cross compile failure, which should now be fixed.

  14. laanwj commented at 4:01 PM on July 15, 2020: member

    Looks good to me. Validation errors, definitely for network-received transactions, should not spam the log with errors by default. I thought we had caught all of these by now but apparently not, good catch. The refactor seems unrelated but looks like a good (and zero risk) change to me. ACK fa492895b572a1196ca8652006b6fc2fa1d16951

  15. MarcoFalke commented at 4:36 PM on July 15, 2020: member

    For transactions this has been fixed in #18990 and should be complete. For blocks, this pull is the first step (incomplete)

  16. practicalswift commented at 9:30 PM on July 15, 2020: contributor

    re-ACK fa56eda58e5ec2f2345bbe14c798e83f2abb4728

  17. laanwj merged this on Jul 22, 2020
  18. laanwj closed this on Jul 22, 2020
  19. sidhujag referenced this in commit b81b14c1cd on Jul 24, 2020
  20. MarcoFalke deleted the branch on Jul 31, 2020
  21. Fabcien referenced this in commit aeb00430f7 on Sep 2, 2021
  22. DrahtBot locked this on Feb 15, 2022
Contributors
MarcoFalkejonatackpracticalswiftlaanwj
Labels
ValidationUtils/log/libs
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me