validation/net processing: remove the BlockChecked validation interface method

src/net_processing.cpp

@@ -430,9 +430,18 @@ static void PushNodeVersion(CNode *pnode, CConnman* connman, int64_t nTime)
     }
 }
 
-// Returns a bool indicating whether we requested this block.
-// Also used if a block was /not/ received and timed out or started with another peer
-static bool MarkBlockAsReceived(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
+/**
+ * Mark a block as no longer 'in flight' from any node. Called when the block
+ * has been received, if it's timed out, of if we're requesting it from another
+ * peer.
+ *
+ * Clear the block from the global mapBlocksInFlight map and any node's
+ * vBlocksInFlight vector.
+ *
+ * @param[in]      hash       The block hash
+ * @return         bool       Whether this block was already in flight from a peer.
+ */
+static bool MarkBlockAsNotInFlight(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
     std::map<uint256, std::pair<NodeId, std::list<QueuedBlock>::iterator> >::iterator itInFlight = mapBlocksInFlight.find(hash);
     if (itInFlight != mapBlocksInFlight.end()) {
         CNodeState *state = State(itInFlight->second.first);
@@ -455,8 +464,23 @@ static bool MarkBlockAsReceived(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs
     return false;
 }
 
-// returns false, still setting pit, if the block was already in flight from the same peer
-// pit will only be valid as long as the same cs_main lock is being held
+/**
+ * Mark a blocks as 'in flight' from a specific peer. This can be called either when
+ * we send a GETDATA(BLOCK) or GETDATA(CMPCTBLOCK) to the peer, or when we receive
+ * a CMPCTBLOCK from a peer.
+ *
+ * Add the block to the global mapBlocksInFlight map and to the node's vBlocksInFlight vector
+ *
+ * @param[in]      nodeid     The node that we send the GETDATA to or received the CMPCTBLOCKFROM
+ * @param[in]      hash       The block hash
+ * @param[in]      pindex     The block's BlockIndex
+ * @param[in/out]  pit        Pointer to iterator to QueuedBlock. Only used when this function
+ *                            is called during compact block handling. pit is only valid for
+ *                            as long as cs_main is held.
+ * @return         bool       Only used when this function is called during compact block processing
+ *                            False indicates the block was already in flight from the same node.
+ *                            Returns true otherwise.
+ */
 static bool MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlockIndex* pindex = nullptr, std::list<QueuedBlock>::iterator** pit = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
     CNodeState *state = State(nodeid);
     assert(state != nullptr);
@@ -471,7 +495,7 @@ static bool MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlock
     }
 
     // Make sure it's not listed somewhere already.
-    MarkBlockAsReceived(hash);
+    MarkBlockAsNotInFlight(hash);
 
     std::list<QueuedBlock>::iterator it = state->vBlocksInFlight.insert(state->vBlocksInFlight.end(),
             {hash, pindex, pindex != nullptr, std::unique_ptr<PartiallyDownloadedBlock>(pit ? new PartiallyDownloadedBlock(&mempool) : nullptr)});
@@ -1129,8 +1153,24 @@ PeerLogicValidation::PeerLogicValidation(CConnman* connmanIn, BanMan* banman, CS
  * block. Also save the time of the last tip update.
  */
 void PeerLogicValidation::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex, const std::vector<CTransactionRef>& vtxConflicted) {
-    LOCK(g_cs_orphans);
+    {
+        LOCK(cs_main);
 
+        const uint256 hash(pblock->GetHash());
+        std::map<uint256, std::pair<NodeId, bool>>::iterator it = mapBlockSource.find(hash);
+
+        if (it != mapBlockSource.end()) {
+            // If we're not in IBD, mark the peer that sent us this block a compact block source
+            if (!::ChainstateActive().IsInitialBlockDownload()) {
+                MaybeSetPeerAsAnnouncingHeaderAndIDs(it->second.first, connman);
+            }
+
+            // We no longer need to track the originator of this block
+            mapBlockSource.erase(it);
+        }
+    }
+
+    LOCK(g_cs_orphans);
     std::vector<uint256> vOrphanErase;
 
     for (const CTransactionRef& ptx : pblock->vtx) {
@@ -1249,37 +1289,30 @@ void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CB
 }
 
 /**
- * Handle invalid block rejection and consequent peer banning, maintain which
- * peers announce compact blocks.
+ * Handle invalid block rejection and consequent peer banning.
+ *
+ * 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());
     std::map<uint256, std::pair<NodeId, bool>>::iterator it = mapBlockSource.find(hash);
 
-    // If the block failed validation, we know where it came from and we're still connected
-    // to that peer, maybe punish.
-    if (state.IsInvalid() &&
-        it != mapBlockSource.end() &&
-        State(it->second.first)) {
+    // If we know where the block came from and we're still connected to that
+    // peer, maybe punish.
+    if (it != mapBlockSource.end()) {
+        if (State(it->second.first)) {
             MaybePunishNodeForBlock(/*nodeid=*/ it->second.first, state, /*via_compact_block=*/ !it->second.second);
-    }
-    // Check that:
-    // 1. The block is valid
-    // 2. We're not in initial block download
-    // 3. This is currently the best block we're aware of. We haven't updated
-    //    the tip yet so we have no way to check this directly here. Instead we
-    //    just check that there are currently no other blocks in flight.
-    else if (state.IsValid() &&
-             !::ChainstateActive().IsInitialBlockDownload() &&
-             mapBlocksInFlight.count(hash) == mapBlocksInFlight.size()) {
-        if (it != mapBlockSource.end()) {
-            MaybeSetPeerAsAnnouncingHeaderAndIDs(it->second.first, connman);
         }
-    }
-    if (it != mapBlockSource.end())
         mapBlockSource.erase(it);
+    }
+}
+
+void PeerLogicValidation::BlockFailedConnection(const CBlock& block, const BlockValidationState& state) {
+    ::BlockChecked(block, state, connman);
 }
 
 //////////////////////////////////////////////////////////////////////////////
@@ -1880,6 +1913,27 @@ void static ProcessOrphanTx(CConnman* connman, std::set<uint256>& orphan_work_se
     }
 }
 
+/**
+ *  A block has been processed. If the block was failed anti-dos / mutation checks, then
+ *  call BlockChecked() to maybe punish peer. If this is the first time we've seen the block,
+ *  update the node's nLastBlockTime. Otherwise erase it from mapBlockSource.
+ */
+void static BlockProcessed(CNode* pfrom, CConnman* connman, std::shared_ptr<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();
+    }
+}
+
 bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStream& vRecv, int64_t nTimeReceived, const CChainParams& chainparams, CConnman* connman, BanMan* banman, const std::atomic<bool>& interruptMsgProc)
 {
     LogPrint(BCLog::NET, "received: %s (%u bytes) peer=%d\n", SanitizeString(strCommand), vRecv.size(), pfrom->GetId());
@@ -2730,7 +2784,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
                 PartiallyDownloadedBlock& partialBlock = *(*queuedBlockIt)->partialBlock;
                 ReadStatus status = partialBlock.InitData(cmpctblock, vExtraTxnForCompact);
                 if (status == READ_STATUS_INVALID) {
-                    MarkBlockAsReceived(pindex->GetBlockHash()); // Reset in-flight state in case of whitelist
+                    MarkBlockAsNotInFlight(pindex->GetBlockHash()); // Reset in-flight state in case of whitelist
                     Misbehaving(pfrom->GetId(), 100, strprintf("Peer %d sent us invalid compact block\n", pfrom->GetId()));
                     return true;
                 } else if (status == READ_STATUS_FAILED) {
@@ -2818,20 +2872,20 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
             // 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.
-            ProcessNewBlock(chainparams, pblock, /*fForceProcessing=*/true, &fNewBlock);
-            if (fNewBlock) {
-                pfrom->nLastBlockTime = GetTime();
-            } else {
+            BlockValidationState dos_state;
+            ProcessNewBlock(chainparams, pblock, dos_state, /*fForceProcessing=*/true, &fNewBlock);
+            BlockProcessed(pfrom, connman, pblock, dos_state, fNewBlock);
+
+            if (dos_state.IsValid()) {
+                // This block has passed anti-dos / mutation checks in
+                // ProcessNewBlock(). Clear the download state for the block,
+                // which is in flight from some other peer.
+                //
+                // We do this after checking for mutation so that a mutated
+                // cmpctblock announcement can't be used to interfere with
+                // block relay.
                 LOCK(cs_main);
-                mapBlockSource.erase(pblock->GetHash());
-            }
-            LOCK(cs_main); // hold cs_main for CBlockIndex::IsValid()
-            if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS)) {
-                // Clear download state for this block, which is in
-                // process from some other peer.  We do this after calling
-                // ProcessNewBlock so that a malleated cmpctblock announcement
-                // can't be used to interfere with block relay.
-                MarkBlockAsReceived(pblock->GetHash());
+                MarkBlockAsNotInFlight(pblock->GetHash());
             }
         }
         return true;
@@ -2863,7 +2917,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
             PartiallyDownloadedBlock& partialBlock = *it->second.second->partialBlock;
             ReadStatus status = partialBlock.FillBlock(*pblock, resp.txn);
             if (status == READ_STATUS_INVALID) {
-                MarkBlockAsReceived(resp.blockhash); // Reset in-flight state in case of whitelist
+                MarkBlockAsNotInFlight(resp.blockhash); // Reset in-flight state in case of whitelist
                 Misbehaving(pfrom->GetId(), 100, strprintf("Peer %d sent us invalid compact block/non-matching block transactions\n", pfrom->GetId()));
                 return true;
             } else if (status == READ_STATUS_FAILED) {
@@ -2889,7 +2943,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
                 // though the block was successfully read, and rely on the
                 // handling in ProcessNewBlock to ensure the block index is
                 // updated, etc.
-                MarkBlockAsReceived(resp.blockhash); // it is now an empty pointer
+                MarkBlockAsNotInFlight(resp.blockhash); // it is now an empty pointer
                 fBlockRead = true;
                 // mapBlockSource is used for potentially punishing peers and
                 // updating which peers send us compact blocks, so the race
@@ -2908,13 +2962,9 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
             // 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.
-            ProcessNewBlock(chainparams, pblock, /*fForceProcessing=*/true, &fNewBlock);
-            if (fNewBlock) {
-                pfrom->nLastBlockTime = GetTime();
-            } else {
-                LOCK(cs_main);
-                mapBlockSource.erase(pblock->GetHash());
-            }
+            BlockValidationState dos_state;
+            ProcessNewBlock(chainparams, pblock, dos_state, /*fForceProcessing=*/true, &fNewBlock);
+            BlockProcessed(pfrom, connman, pblock, dos_state, fNewBlock);
         }
         return true;
     }
@@ -2964,20 +3014,16 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
             LOCK(cs_main);
             // Also always process if we requested the block explicitly, as we may
             // need it even though it is not a candidate for a new best tip.
-            forceProcessing |= MarkBlockAsReceived(hash);
+            forceProcessing |= MarkBlockAsNotInFlight(hash);
             // mapBlockSource is only used for punishing peers and setting
             // which peers send us compact blocks, so the race between here and
             // cs_main in ProcessNewBlock is fine.
             mapBlockSource.emplace(hash, std::make_pair(pfrom->GetId(), true));
         }
         bool fNewBlock = false;
-        ProcessNewBlock(chainparams, pblock, forceProcessing, &fNewBlock);
-        if (fNewBlock) {
-            pfrom->nLastBlockTime = GetTime();
-        } else {
-            LOCK(cs_main);
-            mapBlockSource.erase(pblock->GetHash());
-        }
+        BlockValidationState dos_state;
+        ProcessNewBlock(chainparams, pblock, dos_state, forceProcessing, &fNewBlock);
+        BlockProcessed(pfrom, connman, pblock, dos_state, fNewBlock);
         return true;
     }
 

src/net_processing.h

@@ -40,7 +40,7 @@ class PeerLogicValidation final : public CValidationInterface, public NetEventsI
     /**
      * Overridden from CValidationInterface.
      */
-    void BlockChecked(const CBlock& block, const BlockValidationState& state) override;
+    void BlockFailedConnection(const CBlock& block, const BlockValidationState& state) override;
     /**
      * Overridden from CValidationInterface.
      */

src/rpc/mining.cpp

@@ -135,7 +135,8 @@ static UniValue generateBlocks(const CScript& coinbase_script, int nGenerate, ui
             continue;
         }
         std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(*pblock);
-        if (!ProcessNewBlock(Params(), shared_pblock, true, nullptr))
+        BlockValidationState state;
+        if (!ProcessNewBlock(Params(), shared_pblock, state, true, nullptr) || !state.IsValid())
             throw JSONRPCError(RPC_INTERNAL_ERROR, "ProcessNewBlock, block not accepted");
         ++nHeight;
         blockHashes.push_back(pblock->GetHash().GetHex());
@@ -717,12 +718,17 @@ class submitblock_StateCatcher : public CValidationInterface
     explicit submitblock_StateCatcher(const uint256 &hashIn) : hash(hashIn), found(false), state() {}
 
 protected:
-    void BlockChecked(const CBlock& block, const BlockValidationState& stateIn) override {
+    void BlockFailedConnection(const CBlock& block, const BlockValidationState& stateIn) override {
         if (block.GetHash() != hash)
             return;
         found = true;
         state = stateIn;
     }
+
+    void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex, const std::vector<CTransactionRef>& vtxConflicted) override {
+        if (pblock->GetHash() != hash) return;
+        found = true;
+    }
 };
 
 static UniValue submitblock(const JSONRPCRequest& request)
@@ -777,11 +783,18 @@ static UniValue submitblock(const JSONRPCRequest& request)
     bool new_block;
     submitblock_StateCatcher sc(block.GetHash());
     RegisterValidationInterface(&sc);
-    bool accepted = ProcessNewBlock(Params(), blockptr, /* fForceProcessing */ true, /* fNewBlock */ &new_block);
+    BlockValidationState dos_state;
+    bool accepted = ProcessNewBlock(Params(), blockptr, dos_state, /* fForceProcessing */ true, /* fNewBlock */ &new_block);
+
+    // Make sure the validation interface queue has drained (so we get any relevant BlockConnected/BlockFailedConnection callbacks)
+    SyncWithValidationInterfaceQueue();
     UnregisterValidationInterface(&sc);
     if (!new_block && accepted) {
         return "duplicate";
     }
+    if (!dos_state.IsValid()) {
+        return BIP22ValidationResult(dos_state);
+    }
     if (!sc.found) {
         return "inconclusive";
     }

src/test/blockfilter_index_tests.cpp

@@ -164,7 +164,9 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, TestChain100Setup)
     uint256 chainA_last_header = last_header;
     for (size_t i = 0; i < 2; i++) {
         const auto& block = chainA[i];
-        BOOST_REQUIRE(ProcessNewBlock(Params(), block, true, nullptr));
+        BlockValidationState dos_state;
+        BOOST_REQUIRE(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];
@@ -182,7 +184,9 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, TestChain100Setup)
     uint256 chainB_last_header = last_header;
     for (size_t i = 0; i < 3; i++) {
         const auto& block = chainB[i];
-        BOOST_REQUIRE(ProcessNewBlock(Params(), block, true, nullptr));
+        BlockValidationState dos_state;
+        BOOST_REQUIRE(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];
@@ -211,10 +215,12 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, TestChain100Setup)
     }
 
     // Reorg back to chain A.
-     for (size_t i = 2; i < 4; i++) {
-         const auto& block = chainA[i];
-         BOOST_REQUIRE(ProcessNewBlock(Params(), block, true, nullptr));
-     }
+    for (size_t i = 2; i < 4; i++) {
+        const auto& block = chainA[i];
+        BlockValidationState dos_state;
+        BOOST_REQUIRE(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;