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

fa56eda log: Avoid treating remote misbehvior as local system error (MarcoFalke)
fa49289 refactor: Switch ValidationState mode to C++11 enum class (MarcoFalke)

Pull request description:

  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.

ACKs for top commit:
  practicalswift:
    re-ACK fa56eda

Tree-SHA512: 9793191f5cb57bdff7c93926e94877e8ca2ef89dcebcf9eb155899c733961839ec7c3f9b9f001dc082ada4234fe6e75f6df431301678d6822325840771166d77

Author: laanwj

src/consensus/validation.h

@@ -75,37 +75,39 @@ enum class BlockValidationResult {
  *  by TxValidationState and BlockValidationState for validation information on transactions
  *  and blocks respectively. */
 template <typename Result>
-class ValidationState {
+class ValidationState
+{
 private:
-    enum mode_state {
-        MODE_VALID,   //!< everything ok
-        MODE_INVALID, //!< network rule violation (DoS value may be set)
-        MODE_ERROR,   //!< run-time error
-    } m_mode{MODE_VALID};
+    enum class ModeState {
+        M_VALID,   //!< everything ok
+        M_INVALID, //!< network rule violation (DoS value may be set)
+        M_ERROR,   //!< run-time error
+    } m_mode{ModeState::M_VALID};
     Result m_result{};
     std::string m_reject_reason;
     std::string m_debug_message;
+
 public:
     bool Invalid(Result result,
-                 const std::string &reject_reason="",
-                 const std::string &debug_message="")
+                 const std::string& reject_reason = "",
+                 const std::string& debug_message = "")
     {
         m_result = result;
         m_reject_reason = reject_reason;
         m_debug_message = debug_message;
-        if (m_mode != MODE_ERROR) m_mode = MODE_INVALID;
+        if (m_mode != ModeState::M_ERROR) m_mode = ModeState::M_INVALID;
         return false;
     }
     bool Error(const std::string& reject_reason)
     {
-        if (m_mode == MODE_VALID)
+        if (m_mode == ModeState::M_VALID)
             m_reject_reason = reject_reason;
-        m_mode = MODE_ERROR;
+        m_mode = ModeState::M_ERROR;
         return false;
     }
-    bool IsValid() const { return m_mode == MODE_VALID; }
-    bool IsInvalid() const { return m_mode == MODE_INVALID; }
-    bool IsError() const { return m_mode == MODE_ERROR; }
+    bool IsValid() const { return m_mode == ModeState::M_VALID; }
+    bool IsInvalid() const { return m_mode == ModeState::M_INVALID; }
+    bool IsError() const { return m_mode == ModeState::M_ERROR; }
     Result GetResult() const { return m_result; }
     std::string GetRejectReason() const { return m_reject_reason; }
     std::string GetDebugMessage() const { return m_debug_message; }

src/validation.cpp

@@ -3641,8 +3641,10 @@ bool BlockManager::AcceptBlockHeader(const CBlockHeader& block, BlockValidationS
             return true;
         }
 
-        if (!CheckBlockHeader(block, state, chainparams.GetConsensus()))
-            return error("%s: Consensus::CheckBlockHeader: %s, %s", __func__, hash.ToString(), state.ToString());
+        if (!CheckBlockHeader(block, state, chainparams.GetConsensus())) {
+            LogPrint(BCLog::VALIDATION, "%s: Consensus::CheckBlockHeader: %s, %s\n", __func__, hash.ToString(), state.ToString());
+            return false;
+        }
 
         // Get prev block index
         CBlockIndex* pindexPrev = nullptr;