bitcoin · jnewbery · Nov 14, 2019 · Nov 15, 2019 · Nov 15, 2019 · Nov 15, 2019
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -1385,8 +1385,11 @@ void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CB
 /**
  * Handle invalid block rejection and consequent peer discouragement, maintain which
  * peers announce compact blocks.
+ * Called both in case of cursory DoS checks failing (implying the peer is likely
+ * sending us bogus data) and after full validation of the block fails (which may
+ * be OK if it was sent over compact blocks).
  */
-void PeerLogicValidation::BlockChecked(const CBlock& block, const BlockValidationState& state) {
+static void BlockChecked(const CBlock& block, const BlockValidationState& state, CConnman& connman) {
     LOCK(cs_main);
 
     const uint256 hash(block.GetHash());
@@ -1409,13 +1412,17 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const BlockValidatio
              !::ChainstateActive().IsInitialBlockDownload() &&
              mapBlocksInFlight.count(hash) == mapBlocksInFlight.size()) {
         if (it != mapBlockSource.end()) {
-            MaybeSetPeerAsAnnouncingHeaderAndIDs(it->second.first, m_connman);
+            MaybeSetPeerAsAnnouncingHeaderAndIDs(it->second.first, connman);
         }
     }
     if (it != mapBlockSource.end())
         mapBlockSource.erase(it);
 }
 
+void PeerLogicValidation::BlockChecked(const CBlock& block, const BlockValidationState& state) {
+    ::BlockChecked(block, state, m_connman);
+}
+
 //////////////////////////////////////////////////////////////////////////////
 //
 // Messages
@@ -2086,6 +2093,25 @@ void static ProcessOrphanTx(CConnman& connman, CTxMemPool& mempool, std::set<uin
     }
 }
 
+/**
+ *  A block has been processed. Handle potential peer punishment and housekeeping.
+ */
+void static BlockProcessed(CNode& pfrom, CConnman& connman, CBlock& pblock, BlockValidationState& state, bool new_block)
+{
+    if (!state.IsValid()) {
+        // The block failed anti-dos / mutation checks. Call BlockChecked() callback here.
+        // This clears the block from mapBlockSource.
+        BlockChecked(pblock, state, connman);
+    } else if (!new_block) {
+        // Block was valid but we've seen it before. Clear it from mapBlockSource.
+        LOCK(cs_main);
+        ::mapBlockSource.erase(pblock.GetHash());
+    } else {
+        // Block is valid and we haven't seen it before. set nLastBlockTime for this peer.
+        pfrom.nLastBlockTime = GetTime();
+    }
+}
+
 /**
  * Validation logic for compact filters request handling.
  *
@@ -3325,13 +3351,10 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty
             // we have a chain with at least nMinimumChainWork), and we ignore
             // compact blocks with less work than our tip, it is safe to treat
             // reconstructed compact blocks as having been requested.
-            m_chainman.ProcessNewBlock(chainparams, pblock, /*fForceProcessing=*/true, &fNewBlock);
-            if (fNewBlock) {
-                pfrom.nLastBlockTime = GetTime();
-            } else {
-                LOCK(cs_main);
-                mapBlockSource.erase(pblock->GetHash());
-            }
+            BlockValidationState dos_state;
+            m_chainman.ProcessNewBlock(chainparams, pblock, dos_state, /*fForceProcessing=*/true, &fNewBlock);
+            BlockProcessed(pfrom, m_connman, *pblock, dos_state, fNewBlock);
+
             LOCK(cs_main); // hold cs_main for CBlockIndex::IsValid()
             if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS)) {
                 // Clear download state for this block, which is in
@@ -3415,13 +3438,9 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty
             // disk-space attacks), but this should be safe due to the
             // protections in the compact block handler -- see related comment
             // in compact block optimistic reconstruction handling.
-            m_chainman.ProcessNewBlock(chainparams, pblock, /*fForceProcessing=*/true, &fNewBlock);
-            if (fNewBlock) {
-                pfrom.nLastBlockTime = GetTime();
-            } else {
-                LOCK(cs_main);
-                mapBlockSource.erase(pblock->GetHash());
-            }
+            BlockValidationState dos_state;
+            m_chainman.ProcessNewBlock(chainparams, pblock, dos_state, /*fForceProcessing=*/true, &fNewBlock);
+            BlockProcessed(pfrom, m_connman, *pblock, dos_state, fNewBlock);
         }
         return;
     }
@@ -3478,13 +3497,9 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty
             mapBlockSource.emplace(hash, std::make_pair(pfrom.GetId(), true));
         }
         bool fNewBlock = false;
-        m_chainman.ProcessNewBlock(chainparams, pblock, forceProcessing, &fNewBlock);
-        if (fNewBlock) {
-            pfrom.nLastBlockTime = GetTime();
-        } else {
-            LOCK(cs_main);
-            mapBlockSource.erase(pblock->GetHash());
-        }
+        BlockValidationState dos_state;
+        m_chainman.ProcessNewBlock(chainparams, pblock, dos_state, forceProcessing, &fNewBlock);
+        BlockProcessed(pfrom, m_connman, *pblock, dos_state, fNewBlock);
         return;
     }
 

diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
@@ -126,7 +126,8 @@ static bool GenerateBlock(ChainstateManager& chainman, CBlock& block, uint64_t&
     }
 
     std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(block);
-    if (!chainman.ProcessNewBlock(chainparams, shared_pblock, true, nullptr)) {
+    BlockValidationState state;
+    if (!chainman.ProcessNewBlock(chainparams, shared_pblock, state, true, nullptr)) {
         throw JSONRPCError(RPC_INTERNAL_ERROR, "ProcessNewBlock, block not accepted");
     }
 
@@ -960,11 +961,15 @@ static UniValue submitblock(const JSONRPCRequest& request)
     bool new_block;
     auto sc = std::make_shared<submitblock_StateCatcher>(block.GetHash());
     RegisterSharedValidationInterface(sc);
-    bool accepted = EnsureChainman(request.context).ProcessNewBlock(Params(), blockptr, /* fForceProcessing */ true, /* fNewBlock */ &new_block);
+    BlockValidationState dos_state;
+    bool accepted = EnsureChainman(request.context).ProcessNewBlock(Params(), blockptr, dos_state, /* fForceProcessing */ true, /* fNewBlock */ &new_block);
     UnregisterSharedValidationInterface(sc);
     if (!new_block && accepted) {
         return "duplicate";
     }
+    if (!dos_state.IsValid()) {
+        return BIP22ValidationResult(dos_state);
+    }
     if (!sc->found) {
         return "inconclusive";
     }

diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp
@@ -171,7 +171,9 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, BuildChainTestingSetup)
     uint256 chainA_last_header = last_header;
     for (size_t i = 0; i < 2; i++) {
         const auto& block = chainA[i];
-        BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(Params(), block, true, nullptr));
+        BlockValidationState dos_state;
+        BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(Params(), block, dos_state, true, nullptr));
+        BOOST_REQUIRE(dos_state.IsValid());
     }
     for (size_t i = 0; i < 2; i++) {
         const auto& block = chainA[i];
@@ -189,7 +191,9 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, BuildChainTestingSetup)
     uint256 chainB_last_header = last_header;
     for (size_t i = 0; i < 3; i++) {
         const auto& block = chainB[i];
-        BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(Params(), block, true, nullptr));
+        BlockValidationState dos_state;
+        BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(Params(), block, dos_state, true, nullptr));
+        BOOST_REQUIRE(dos_state.IsValid());
     }
     for (size_t i = 0; i < 3; i++) {
         const auto& block = chainB[i];
@@ -218,10 +222,12 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, BuildChainTestingSetup)
     }
 
     // Reorg back to chain A.
-     for (size_t i = 2; i < 4; i++) {
-         const auto& block = chainA[i];
-         BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(Params(), block, true, nullptr));
-     }
+    for (size_t i = 2; i < 4; i++) {
+        const auto& block = chainA[i];
+        BlockValidationState dos_state;
+        BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(Params(), block, dos_state, true, nullptr));
+        BOOST_REQUIRE(dos_state.IsValid());
+    }
 
      // Check that chain A and B blocks can be retrieved.
      chainA_last_header = last_header;

diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp
@@ -7,6 +7,7 @@
 #include <consensus/consensus.h>
 #include <consensus/merkle.h>
 #include <consensus/tx_verify.h>
+#include <consensus/validation.h>
 #include <miner.h>
 #include <policy/policy.h>
 #include <script/standard.h>
@@ -253,7 +254,9 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
             pblock->nNonce = blockinfo[i].nonce;
         }
         std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(*pblock);
-        BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlock(chainparams, shared_pblock, true, nullptr));
+        BlockValidationState dos_state;
+        BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlock(chainparams, shared_pblock, dos_state, true, nullptr));
+        BOOST_CHECK(dos_state.IsValid());
         pblock->hashPrevBlock = pblock->GetHash();
     }
 

diff --git a/src/test/util/mining.cpp b/src/test/util/mining.cpp
@@ -6,6 +6,7 @@
 
 #include <chainparams.h>
 #include <consensus/merkle.h>
+#include <consensus/validation.h>
 #include <key_io.h>
 #include <miner.h>
 #include <node/context.h>
@@ -32,8 +33,10 @@ CTxIn MineBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
         assert(block->nNonce);
     }
 
-    bool processed{Assert(node.chainman)->ProcessNewBlock(Params(), block, true, nullptr)};
+    BlockValidationState dos_state;
+    bool processed{Assert(node.chainman)->ProcessNewBlock(Params(), block, dos_state, true, nullptr)};
     assert(processed);
+    assert(dos_state.IsValid());
 
     return CTxIn{block->vtx[0]->GetHash(), 0};
 }

diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
@@ -235,7 +235,8 @@ CBlock TestChain100Setup::CreateAndProcessBlock(const std::vector<CMutableTransa
     while (!CheckProofOfWork(block.GetHash(), block.nBits, chainparams.GetConsensus())) ++block.nNonce;
 
     std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(block);
-    Assert(m_node.chainman)->ProcessNewBlock(chainparams, shared_pblock, true, nullptr);
+    BlockValidationState dos_state;
+    Assert(m_node.chainman)->ProcessNewBlock(chainparams, shared_pblock, dos_state, true, nullptr);
 
     CBlock result = block;
     return result;

diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp
@@ -166,7 +166,8 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
     BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlockHeaders(headers, state, Params()));
 
     // Connect the genesis block and drain any outstanding events
-    BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlock(Params(), std::make_shared<CBlock>(Params().GenesisBlock()), true, &ignored));
+    BlockValidationState dos_state;
+    BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlock(Params(), std::make_shared<CBlock>(Params().GenesisBlock()), dos_state, true, &ignored));
     SyncWithValidationInterfaceQueue();
 
     // subscribe to events (this subscriber will validate event ordering)
@@ -188,14 +189,16 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
             FastRandomContext insecure;
             for (int i = 0; i < 1000; i++) {
                 auto block = blocks[insecure.randrange(blocks.size() - 1)];
-                Assert(m_node.chainman)->ProcessNewBlock(Params(), block, true, &ignored);
+                Assert(m_node.chainman)->ProcessNewBlock(Params(), block, dos_state, true, &ignored);
             }
 
             // to make sure that eventually we process the full chain - do it here
             for (auto block : blocks) {
                 if (block->vtx.size() == 1) {
-                    bool processed = Assert(m_node.chainman)->ProcessNewBlock(Params(), block, true, &ignored);
+                    BlockValidationState dos_state;
+                    bool processed = Assert(m_node.chainman)->ProcessNewBlock(Params(), block, dos_state, true, &ignored);
                     assert(processed);
+                    assert(dos_state.IsValid());
                 }
             }
         });
@@ -233,7 +236,8 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
 {
     bool ignored;
     auto ProcessBlock = [&](std::shared_ptr<const CBlock> block) -> bool {
-        return Assert(m_node.chainman)->ProcessNewBlock(Params(), block, /* fForceProcessing */ true, /* fNewBlock */ &ignored);
+        BlockValidationState dos_state;
+        return Assert(m_node.chainman)->ProcessNewBlock(Params(), block, dos_state, /* fForceProcessing */ true, /* fNewBlock */ &ignored) && dos_state.IsValid();
     };
 
     // Process all mined blocks

diff --git a/src/validation.cpp b/src/validation.cpp
@@ -3828,37 +3828,35 @@ bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, Block
     return true;
 }
 
-bool ChainstateManager::ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock> pblock, bool fForceProcessing, bool* fNewBlock)
+bool ChainstateManager::ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock> pblock, BlockValidationState& dos_state, bool fForceProcessing, bool* fNewBlock)
 {
     AssertLockNotHeld(cs_main);
 
     {
         CBlockIndex *pindex = nullptr;
         if (fNewBlock) *fNewBlock = false;
-        BlockValidationState state;
 
         // CheckBlock() does not support multi-threaded block validation because CBlock::fChecked can cause data race.
         // Therefore, the following critical section must include the CheckBlock() call as well.
         LOCK(cs_main);
 
         // Ensure that CheckBlock() passes before calling AcceptBlock, as
         // belt-and-suspenders.
-        bool ret = CheckBlock(*pblock, state, chainparams.GetConsensus());
+        bool ret = CheckBlock(*pblock, dos_state, chainparams.GetConsensus());
         if (ret) {
             // Store to disk
-            ret = ::ChainstateActive().AcceptBlock(pblock, state, chainparams, &pindex, fForceProcessing, nullptr, fNewBlock);
+            ret = ::ChainstateActive().AcceptBlock(pblock, dos_state, chainparams, &pindex, fForceProcessing, nullptr, fNewBlock);
         }
         if (!ret) {
-            GetMainSignals().BlockChecked(*pblock, state);
-            return error("%s: AcceptBlock FAILED (%s)", __func__, state.ToString());
+            return error("%s: AcceptBlock FAILED (%s)", __func__, dos_state.ToString());
         }
     }
 
     NotifyHeaderTip();
 
-    BlockValidationState state; // Only used to report errors, not invalidity - ignore it
-    if (!::ChainstateActive().ActivateBestChain(state, chainparams, pblock))
-        return error("%s: ActivateBestChain failed (%s)", __func__, state.ToString());
+    BlockValidationState dummy_state; // Only used to report errors, not invalidity - ignore it
+    if (!::ChainstateActive().ActivateBestChain(dummy_state, chainparams, pblock))
+        return error("%s: ActivateBestChain failed (%s)", __func__, dummy_state.ToString());
 
     return true;
 }

diff --git a/src/validation.h b/src/validation.h
@@ -862,22 +862,34 @@ class ChainstateManager
      * block is made active. Note that it does not, however, guarantee that the
      * specific block passed to it has been checked for validity!
      *
-     * If you want to *possibly* get feedback on whether pblock is valid, you must
-     * install a CValidationInterface (see validationinterface.h) - this will have
-     * its BlockChecked method called whenever *any* block completes validation.
+     * Performs initial sanity checks using the provided BlockValidationState before
+     * connecting any block(s). If you want to *possibly* get feedback on whether
+     * pblock is valid beyond just cursory mutation/DoS checks, you must install
+     * a CValidationInterface (see validationinterface.h) - this will have its
+     * BlockChecked method called whenever *any* block completes validation (note
+     * that any invalidity returned via state will *not* also be provided via
+     * BlockChecked). There is, of course, no guarantee that any given block which
+     * is not a part of the eventual best chain will ever be checked.
      *
-     * Note that we guarantee that either the proof-of-work is valid on pblock, or
-     * (and possibly also) BlockChecked will have been called.
+     * If the block pblock is built on is in our header tree, and pblock is a
+     * candidate for accepting to disk (either because it is a candidate for the
+     * best chain soon, or fForceProcessing is set), but pblock has been mutated,
+     * state is guaranteed to be some non-IsValid() state.
      *
-     * May not be called in a
-     * validationinterface callback.
+     * If fForceProcessing is set (or fNewBlock returns true), and state.IsValid(),
+     * barring pruning and a desire to re-download a pruned block, there should
+     * never be any reason to re-ProcessNewBlock any block with the same hash.
+     *
+     * May not be called in a validationinterface callback.
      *
-     * @param[in]   pblock  The block we want to process.
-     * @param[in]   fForceProcessing Process this block even if unrequested; used for non-network block sources.
-     * @param[out]  fNewBlock A boolean which is set to indicate if the block was first received via this call
-     * @returns     If the block was processed, independently of block validity
+     * @param[in]   pblock            The block we want to process.
+     * @param[out]  state             Only used for failures in CheckBlock/AcceptBlock. For failure in block connection,
+     *                                a CValidationInterface BlockChecked callback is used to notify clients of validity.
+     * @param[in]   fForceProcessing  This block was requested from the peer or came from a non-network source.
+     * @param[out]  fNewBlock         A boolean which is set to indicate if the block was first received via this call
+     * @returns     bool              If the block was processed, independently of block validity
      */
-    bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock> pblock, bool fForceProcessing, bool* fNewBlock) LOCKS_EXCLUDED(cs_main);
+    bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock> pblock, BlockValidationState& state, bool fForceProcessing, bool* fNewBlock) LOCKS_EXCLUDED(cs_main);
 
     /**
      * Process incoming block headers.