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

fa56eda58e log: Avoid treating remote misbehvior as local system error (MarcoFalke)
fa492895b5 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 fa56eda58e

Tree-SHA512: 9793191f5cb57bdff7c93926e94877e8ca2ef89dcebcf9eb155899c733961839ec7c3f9b9f001dc082ada4234fe6e75f6df431301678d6822325840771166d77
This commit is contained in:
Wladimir J. van der Laan 2020-07-22 19:28:09 +02:00
commit 1397afc5ec
No known key found for this signature in database
GPG key ID: 1E4AED62986CD25D
2 changed files with 20 additions and 16 deletions

View file

@ -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; }

View file

@ -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;