From 8f37ea509e4e6f531d894acef9f9be1d6f4eceb5 Mon Sep 17 00:00:00 2001
From: Glenn Willen <gwillen@nerdnet.org>
Date: Mon, 29 Jun 2020 23:07:41 -0700
Subject: [PATCH] Improve logging of failures during block signature validation

Pass in a ValidationState object to CheckProof and friends, so that we can
provide helpful error messages when validation fails.
---
 src/block_proof.cpp | 34 +++++++++++++++++++++++++---------
 src/block_proof.h   |  3 ++-
 src/rpc/mining.cpp  |  3 ++-
 src/txdb.cpp        |  7 +++++--
 src/validation.cpp  | 12 ++++++------
 5 files changed, 40 insertions(+), 19 deletions(-)

diff --git a/src/block_proof.cpp b/src/block_proof.cpp
index 28e9e98dca..7d1b010c62 100644
--- a/src/block_proof.cpp
+++ b/src/block_proof.cpp
@@ -6,9 +6,12 @@
 #include <pow.h>
 
 #include <chain.h>
+#include <consensus/validation.h>
 #include <primitives/block.h>
 #include <script/interpreter.h>
 #include <script/generic.hpp>
+#include <util/system.h>
+#include <validation.h>
 
 bool CheckChallenge(const CBlockHeader& block, const CBlockIndex& indexLast, const Consensus::Params& params)
 {
@@ -19,7 +22,7 @@ bool CheckChallenge(const CBlockHeader& block, const CBlockIndex& indexLast, con
     }
 }
 
-static bool CheckProofGeneric(const CBlockHeader& block, const uint32_t max_block_signature_size, const CScript& challenge, const CScript& scriptSig, const CScriptWitness& witness)
+static bool CheckProofGeneric(const CBlockHeader& block, CValidationState& state, const uint32_t max_block_signature_size, const CScript& challenge, const CScript& scriptSig, const CScriptWitness& witness)
 {
     // Legacy blocks have empty witness, dynafed blocks have empty scriptSig
     bool is_dyna = !witness.stack.empty();
@@ -27,10 +30,10 @@ static bool CheckProofGeneric(const CBlockHeader& block, const uint32_t max_bloc
     // Check signature limits for blocks
     if (scriptSig.size() > max_block_signature_size) {
         assert(!is_dyna);
-        return false;
+        return state.DoS(50, false, REJECT_INVALID, "block-signature-too-large");
     } else if (witness.GetSerializedSize() > max_block_signature_size) {
         assert(is_dyna);
-        return false;
+        return state.DoS(50, false, REJECT_INVALID, "block-signature-too-large");
     }
 
     // Some anti-DoS flags, though max_block_signature_size caps the possible
@@ -44,28 +47,41 @@ static bool CheckProofGeneric(const CBlockHeader& block, const uint32_t max_bloc
         | SCRIPT_VERIFY_LOW_S // Stop easiest signature fiddling
         | SCRIPT_VERIFY_WITNESS // Witness and to enforce cleanstack
         | (is_dyna ? 0 : SCRIPT_NO_SIGHASH_BYTE); // Non-dynafed blocks do not have sighash byte
-    return GenericVerifyScript(scriptSig, witness, challenge, proof_flags, block);
+    if(!GenericVerifyScript(scriptSig, witness, challenge, proof_flags, block)) {
+        return state.DoS(50, false, REJECT_INVALID, "block-signature-verify-failed");
+    } else {
+        return true;
+    }
 }
 
-bool CheckProof(const CBlockHeader& block, const Consensus::Params& params)
+bool CheckProof(const CBlockHeader& block, CValidationState& state, const Consensus::Params& params)
 {
     if (g_signed_blocks) {
         const DynaFedParams& dynafed_params = block.m_dynafed_params;
         if (dynafed_params.IsNull()) {
-            return CheckProofGeneric(block, params.max_block_signature_size, params.signblockscript, block.proof.solution, CScriptWitness());
+            return CheckProofGeneric(block, state, params.max_block_signature_size, params.signblockscript, block.proof.solution, CScriptWitness());
         } else {
-            return CheckProofGeneric(block, dynafed_params.m_current.m_signblock_witness_limit, dynafed_params.m_current.m_signblockscript, CScript(), block.m_signblock_witness);
+            return CheckProofGeneric(block, state, dynafed_params.m_current.m_signblock_witness_limit, dynafed_params.m_current.m_signblockscript, CScript(), block.m_signblock_witness);
         }
     } else {
-        return CheckProofOfWork(block.GetHash(), block.nBits, params);
+        if(!CheckProofOfWork(block.GetHash(), block.nBits, params)) {
+            return state.DoS(50, false, REJECT_INVALID, "high-hash", false, "proof of work failed");
+        } else {
+            return true;
+        }
     }
 }
 
 bool CheckProofSignedParent(const CBlockHeader& block, const Consensus::Params& params)
 {
+    CValidationState state;
     const DynaFedParams& dynafed_params = block.m_dynafed_params;
     if (dynafed_params.IsNull()) {
-        return CheckProofGeneric(block, params.max_block_signature_size, params.parent_chain_signblockscript, block.proof.solution, CScriptWitness());
+        if(!CheckProofGeneric(block, state, params.max_block_signature_size, params.parent_chain_signblockscript, block.proof.solution, CScriptWitness())) {
+            return error("%s: CheckProofGeneric: %s", __func__, FormatStateMessage(state));
+        } else {
+            return true;
+        }
     } else {
         // Dynamic federations means we cannot validate the signer set
         // at least without tracking the parent chain more directly.
diff --git a/src/block_proof.h b/src/block_proof.h
index 273699f37f..24c424f4ce 100644
--- a/src/block_proof.h
+++ b/src/block_proof.h
@@ -14,11 +14,12 @@ class CBlockHeader;
 class CBlockIndex;
 class CProof;
 class CScript;
+class CValidationState;
 
 // Elements signed chain functionality
 
 /** Check on header proof, depending on chain type, PoW or signed **/
-bool CheckProof(const CBlockHeader& block, const Consensus::Params&);
+bool CheckProof(const CBlockHeader& block, CValidationState& state, const Consensus::Params&);
 bool CheckProofSignedParent(const CBlockHeader& block, const Consensus::Params&);
 bool CheckChallenge(const CBlockHeader& block, const CBlockIndex& indexLast, const Consensus::Params&);
 
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index 5de6759943..2803921fa9 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -1189,7 +1189,8 @@ UniValue combineblocksigs(const JSONRPCRequest& request)
     ssBlock << block;
     UniValue result(UniValue::VOBJ);
     result.pushKV("hex", HexStr(ssBlock.begin(), ssBlock.end()));
-    result.pushKV("complete", CheckProof(block, params));
+    CValidationState state;  // ignored
+    result.pushKV("complete", CheckProof(block, state, params));
     return result;
 }
 
diff --git a/src/txdb.cpp b/src/txdb.cpp
index 9694e5bf66..7f0263d896 100644
--- a/src/txdb.cpp
+++ b/src/txdb.cpp
@@ -6,6 +6,7 @@
 #include <txdb.h>
 
 #include <chainparams.h>
+#include <consensus/validation.h>
 #include <hash.h>
 #include <random.h>
 #include <pow.h>
@@ -13,6 +14,7 @@
 #include <uint256.h>
 #include <util/system.h>
 #include <ui_interface.h>
+#include <validation.h>
 
 #include <stdint.h>
 
@@ -323,10 +325,11 @@ bool CBlockTreeDB::LoadBlockIndexGuts(const Consensus::Params& consensusParams,
 
                 const uint256 block_hash = pindexNew->GetBlockHash();
                 // Only validate one of every 1000 block header for sanity check
+                CValidationState state;
                 if (pindexNew->nHeight % 1000 == 0 &&
                         block_hash != consensusParams.hashGenesisBlock &&
-                        !CheckProof(pindexNew->GetBlockHeader(), consensusParams)) {
-                    return error("%s: CheckProof: %s, %s", __func__, block_hash.ToString(), pindexNew->ToString());
+                        !CheckProof(pindexNew->GetBlockHeader(), state, consensusParams)) {
+                    return error("%s: CheckProof: (%s, %s): %s", __func__, block_hash.ToString(), pindexNew->ToString(), FormatStateMessage(state));
                 }
                 pcursor->Next();
             } else {
diff --git a/src/validation.cpp b/src/validation.cpp
index b28fc1764f..34ba6ef0ee 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1159,9 +1159,10 @@ bool ReadBlockFromDisk(CBlock& block, const CDiskBlockPos& pos, const Consensus:
 
     // Check the header
     const uint256 block_hash = block.GetHash();
+    CValidationState state;
     if (block_hash != consensusParams.hashGenesisBlock &&
-            !CheckProof(block, consensusParams)) {
-        return error("ReadBlockFromDisk: Errors in block header at %s", pos.ToString());
+            !CheckProof(block, state, consensusParams)) {
+        return error("ReadBlockFromDisk: Errors in block header at %s: %s", pos.ToString(), FormatStateMessage(state));
     }
 
     return true;
@@ -3304,9 +3305,8 @@ static bool FindUndoPos(CValidationState &state, int nFile, CDiskBlockPos &pos,
 static bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true)
 {
     // Check proof of work matches claimed amount
-    if (fCheckPOW && block.GetHash() != consensusParams.hashGenesisBlock
-            && !CheckProof(block, consensusParams)) {
-        return state.DoS(50, false, REJECT_INVALID, g_signed_blocks ? "block-proof-invalid" : "high-hash", false, "proof of work failed");
+    if (fCheckPOW && block.GetHash() != consensusParams.hashGenesisBlock) {
+        return CheckProof(block, state, consensusParams);
     }
     return true;
 }
@@ -3389,7 +3389,7 @@ bool IsNullDummyEnabled(const CBlockIndex* pindexPrev, const Consensus::Params&
 bool IsDynaFedEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& params)
 {
     LOCK(cs_main);
-    return (VersionBitsState(pindexPrev, params, Consensus::DEPLOYMENT_DYNA_FED, versionbitscache) == ThresholdState::ACTIVE);
+    return VersionBitsState(pindexPrev, params, Consensus::DEPLOYMENT_DYNA_FED, versionbitscache) == ThresholdState::ACTIVE;
 }
 
 // Compute at which vout of the block's coinbase transaction the witness