From 8a50e9a3163079a5d251ae46d75f5abbf3dfc6e2 Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Tue, 11 Jun 2019 20:10:25 -0400
Subject: [PATCH 01/13] [tests] Remove unnecessary cs_mains in
 denialofservice_tests

9fdf05d70cac4a62d1aeeb4299e2c3a9a866f8af resolved some lock
inversion warnings in denialofservice_tests, but left in a number
of cs_main locks that are unnecessary (introducing lock inversion
warnings in future changes).
---
 src/test/denialofservice_tests.cpp | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
index 632151793ec0..21c8c34323bb 100644
--- a/src/test/denialofservice_tests.cpp
+++ b/src/test/denialofservice_tests.cpp
@@ -98,11 +98,11 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
 
     // Test starts here
     {
-        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        LOCK(dummyNode1.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); // should result in getheaders
     }
     {
-        LOCK2(cs_main, dummyNode1.cs_vSend);
+        LOCK(dummyNode1.cs_vSend);
         BOOST_CHECK(dummyNode1.vSendMsg.size() > 0);
         dummyNode1.vSendMsg.clear();
     }
@@ -111,17 +111,17 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
     // Wait 21 minutes
     SetMockTime(nStartTime+21*60);
     {
-        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        LOCK(dummyNode1.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); // should result in getheaders
     }
     {
-        LOCK2(cs_main, dummyNode1.cs_vSend);
+        LOCK(dummyNode1.cs_vSend);
         BOOST_CHECK(dummyNode1.vSendMsg.size() > 0);
     }
     // Wait 3 more minutes
     SetMockTime(nStartTime+24*60);
     {
-        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        LOCK(dummyNode1.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); // should result in disconnect
     }
     BOOST_CHECK(dummyNode1.fDisconnect == true);
@@ -235,7 +235,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
         Misbehaving(dummyNode1.GetId(), 100); // Should get banned
     }
     {
-        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        LOCK(dummyNode1.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode1));
     }
     BOOST_CHECK(banman->IsBanned(addr1));
@@ -252,7 +252,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
         Misbehaving(dummyNode2.GetId(), 50);
     }
     {
-        LOCK2(cs_main, dummyNode2.cs_sendProcessing);
+        LOCK(dummyNode2.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode2));
     }
     BOOST_CHECK(!banman->IsBanned(addr2)); // 2 not banned yet...
@@ -262,7 +262,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
         Misbehaving(dummyNode2.GetId(), 50);
     }
     {
-        LOCK2(cs_main, dummyNode2.cs_sendProcessing);
+        LOCK(dummyNode2.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode2));
     }
     BOOST_CHECK(banman->IsBanned(addr2));
@@ -291,7 +291,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
         Misbehaving(dummyNode1.GetId(), 100);
     }
     {
-        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        LOCK(dummyNode1.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode1));
     }
     BOOST_CHECK(!banman->IsBanned(addr1));
@@ -300,7 +300,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
         Misbehaving(dummyNode1.GetId(), 10);
     }
     {
-        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        LOCK(dummyNode1.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode1));
     }
     BOOST_CHECK(!banman->IsBanned(addr1));
@@ -309,7 +309,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
         Misbehaving(dummyNode1.GetId(), 1);
     }
     {
-        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        LOCK(dummyNode1.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode1));
     }
     BOOST_CHECK(banman->IsBanned(addr1));
@@ -341,7 +341,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
         Misbehaving(dummyNode.GetId(), 100);
     }
     {
-        LOCK2(cs_main, dummyNode.cs_sendProcessing);
+        LOCK(dummyNode.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode));
     }
     BOOST_CHECK(banman->IsBanned(addr));

From 8cd7501997228031e0021aa9f19a7fb1227f5b72 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Thu, 14 Nov 2019 14:47:55 -0500
Subject: [PATCH 02/13] [net processing] Deduplicate post-block-processing code

Co-authored-by: Carl Dong <contact@carldong.me>
---
 src/net_processing.cpp | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index f42a26ca3ea4..9e9377fbf5ed 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1880,6 +1880,20 @@ void static ProcessOrphanTx(CConnman* connman, std::set<uint256>& orphan_work_se
     }
 }
 
+/**
+ *  A block has been processed. 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, std::shared_ptr<CBlock> pblock, bool new_block)
+{
+    if (new_block) {
+        pfrom->nLastBlockTime = GetTime();
+    } else {
+        LOCK(cs_main);
+        ::mapBlockSource.erase(pblock->GetHash());
+    }
+}
+
 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());
@@ -2819,12 +2833,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
             // 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 {
-                LOCK(cs_main);
-                mapBlockSource.erase(pblock->GetHash());
-            }
+            BlockProcessed(pfrom, pblock, 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
@@ -2909,12 +2919,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
             // 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());
-            }
+            BlockProcessed(pfrom, pblock, fNewBlock);
         }
         return true;
     }
@@ -2972,12 +2977,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
         }
         bool fNewBlock = false;
         ProcessNewBlock(chainparams, pblock, forceProcessing, &fNewBlock);
-        if (fNewBlock) {
-            pfrom->nLastBlockTime = GetTime();
-        } else {
-            LOCK(cs_main);
-            mapBlockSource.erase(pblock->GetHash());
-        }
+        BlockProcessed(pfrom, pblock, fNewBlock);
         return true;
     }
 

From 57782d5f70c072cfc70568a3db58a04831454576 Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Fri, 15 Nov 2019 13:26:28 -0500
Subject: [PATCH 03/13] [validation] Add BlockValidationState inout param to
 ProcessNewBlock

This is a pure refactor commit.

This commit enables the caller of ProcessNewBlock to access the final
BlockValidationState passed around between CheckBlock(), AcceptBlock(),
and BlockChecked() inside ProcessNewBlock(). This is useful because in a
future commit, we will move the BlockChecked() call out of
ProcessNewBlock(), and BlockChecked() still needs to be able to access
the BlockValidationState.

Co-authored-by: John Newbery <john@johnnewbery.com>
Co-authored-by: Carl Dong <contact@carldong.me>
---
 src/net_processing.cpp               |  9 ++++++---
 src/rpc/mining.cpp                   |  6 ++++--
 src/test/blockfilter_index_tests.cpp | 15 +++++++++------
 src/test/miner_tests.cpp             |  4 +++-
 src/test/util.cpp                    |  4 +++-
 src/test/util/setup_common.cpp       |  3 ++-
 src/test/validation_block_tests.cpp  | 12 ++++++++----
 src/validation.cpp                   | 11 +++++------
 src/validation.h                     | 11 ++++++-----
 9 files changed, 46 insertions(+), 29 deletions(-)

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 9e9377fbf5ed..ffbc850ae12e 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2832,7 +2832,8 @@ 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);
+            BlockValidationState dos_state;
+            ProcessNewBlock(chainparams, pblock, dos_state, /*fForceProcessing=*/true, &fNewBlock);
             BlockProcessed(pfrom, pblock, fNewBlock);
 
             LOCK(cs_main); // hold cs_main for CBlockIndex::IsValid()
@@ -2918,7 +2919,8 @@ 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);
+            BlockValidationState dos_state;
+            ProcessNewBlock(chainparams, pblock, dos_state, /*fForceProcessing=*/true, &fNewBlock);
             BlockProcessed(pfrom, pblock, fNewBlock);
         }
         return true;
@@ -2976,7 +2978,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
             mapBlockSource.emplace(hash, std::make_pair(pfrom->GetId(), true));
         }
         bool fNewBlock = false;
-        ProcessNewBlock(chainparams, pblock, forceProcessing, &fNewBlock);
+        BlockValidationState dos_state;
+        ProcessNewBlock(chainparams, pblock, dos_state, forceProcessing, &fNewBlock);
         BlockProcessed(pfrom, pblock, fNewBlock);
         return true;
     }
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index ab22155651e1..c4b3a7ea343a 100644
--- a/src/rpc/mining.cpp
+++ b/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))
             throw JSONRPCError(RPC_INTERNAL_ERROR, "ProcessNewBlock, block not accepted");
         ++nHeight;
         blockHashes.push_back(pblock->GetHash().GetHex());
@@ -777,7 +778,8 @@ 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);
     UnregisterValidationInterface(&sc);
     if (!new_block && accepted) {
         return "duplicate";
diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp
index 2b616f4c2ba7..aecd02dd2369 100644
--- a/src/test/blockfilter_index_tests.cpp
+++ b/src/test/blockfilter_index_tests.cpp
@@ -164,7 +164,8 @@ 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));
     }
     for (size_t i = 0; i < 2; i++) {
         const auto& block = chainA[i];
@@ -182,7 +183,8 @@ 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));
     }
     for (size_t i = 0; i < 3; i++) {
         const auto& block = chainB[i];
@@ -211,10 +213,11 @@ 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));
+    }
 
      // 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
index 6ed7350ea237..2bd09321cf98 100644
--- 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>
@@ -247,7 +248,8 @@ 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(ProcessNewBlock(chainparams, shared_pblock, true, nullptr));
+        BlockValidationState dos_state;
+        BOOST_CHECK(ProcessNewBlock(chainparams, shared_pblock, dos_state, true, nullptr));
         pblock->hashPrevBlock = pblock->GetHash();
     }
 
diff --git a/src/test/util.cpp b/src/test/util.cpp
index ed031270f2da..fa6951ec24b2 100644
--- a/src/test/util.cpp
+++ b/src/test/util.cpp
@@ -6,6 +6,7 @@
 
 #include <chainparams.h>
 #include <consensus/merkle.h>
+#include <consensus/validation.h>
 #include <key_io.h>
 #include <miner.h>
 #include <outputtype.h>
@@ -63,7 +64,8 @@ CTxIn MineBlock(const CScript& coinbase_scriptPubKey)
         assert(block->nNonce);
     }
 
-    bool processed{ProcessNewBlock(Params(), block, true, nullptr)};
+    BlockValidationState dos_state;
+    bool processed{ProcessNewBlock(Params(), block, dos_state, true, nullptr)};
     assert(processed);
 
     return CTxIn{block->vtx[0]->GetHash(), 0};
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 0c6ecdf69d50..e5b8e8727c4a 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -190,7 +190,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);
-    ProcessNewBlock(chainparams, shared_pblock, true, nullptr);
+    BlockValidationState dos_state;
+    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
index b7cf82906ab8..34a6aef28bee 100644
--- a/src/test/validation_block_tests.cpp
+++ b/src/test/validation_block_tests.cpp
@@ -156,7 +156,8 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
     BOOST_CHECK(ProcessNewBlockHeaders(headers, state, Params()));
 
     // Connect the genesis block and drain any outstanding events
-    BOOST_CHECK(ProcessNewBlock(Params(), std::make_shared<CBlock>(Params().GenesisBlock()), true, &ignored));
+    BlockValidationState dos_state;
+    BOOST_CHECK(ProcessNewBlock(Params(), std::make_shared<CBlock>(Params().GenesisBlock()), dos_state, true, &ignored));
     SyncWithValidationInterfaceQueue();
 
     // subscribe to events (this subscriber will validate event ordering)
@@ -178,13 +179,15 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
             FastRandomContext insecure;
             for (int i = 0; i < 1000; i++) {
                 auto block = blocks[insecure.randrange(blocks.size() - 1)];
-                ProcessNewBlock(Params(), block, true, &ignored);
+                BlockValidationState dos_state;
+                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 = ProcessNewBlock(Params(), block, true, &ignored);
+                    BlockValidationState dos_state;
+                    bool processed = ProcessNewBlock(Params(), block, dos_state, true, &ignored);
                     assert(processed);
                 }
             }
@@ -225,7 +228,8 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
 {
     bool ignored;
     auto ProcessBlock = [&ignored](std::shared_ptr<const CBlock> block) -> bool {
-        return ProcessNewBlock(Params(), block, /* fForceProcessing */ true, /* fNewBlock */ &ignored);
+        BlockValidationState dos_state;
+        return ProcessNewBlock(Params(), block, dos_state, /* fForceProcessing */ true, /* fNewBlock */ &ignored);
     };
 
     // Process all mined blocks
diff --git a/src/validation.cpp b/src/validation.cpp
index cc7687ae059e..56262af58ed6 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3760,14 +3760,13 @@ bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, Block
     return true;
 }
 
-bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock> pblock, bool fForceProcessing, bool *fNewBlock)
+bool 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.
@@ -3775,14 +3774,14 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons
 
         // 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__, FormatStateMessage(state));
+            GetMainSignals().BlockChecked(*pblock, dos_state);
+            return error("%s: AcceptBlock FAILED (%s)", __func__, FormatStateMessage(dos_state));
         }
     }
 
diff --git a/src/validation.h b/src/validation.h
index 31721cfbf7e5..faea14d2aa24 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -209,12 +209,13 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
  * 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 and whitelisted peers.
- * @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             Currently unused.
+ * @param[in]   fForceProcessing  Process this block even if unrequested; used for non-network block sources and whitelisted peers.
+ * @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.

From bbc1bca53d2db873a1699566b4c6f682b15d26e3 Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Fri, 15 Nov 2019 14:51:21 -0500
Subject: [PATCH 04/13] [net processing] Make BlockChecked a standalone, static
 function

This is a pure refactor commit.

Since BlockChecked() doesn't actually depend on all of
PeerLogicValidation but just PeerLogicValidation's CConnman, we can make
a standalone, static function that simply has an extra CConnman
parameter and have the non-static version call the static one.

This also means that, in a future commit, when we move the
BlockChecked() call out of ProcessNewBlock(), the caller of
ProcessNewBlock() can call BlockChecked() directly even if they only
have a CConnman.

Co-authored-by: John Newbery <john@johnnewbery.com>
Co-authored-by: Carl Dong <contact@carldong.me>
---
 src/net_processing.cpp | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index ffbc850ae12e..1ba7c8d8dacf 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1251,8 +1251,11 @@ void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CB
 /**
  * Handle invalid block rejection and consequent peer banning, 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());
@@ -1282,6 +1285,10 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const BlockValidatio
         mapBlockSource.erase(it);
 }
 
+void PeerLogicValidation::BlockChecked(const CBlock& block, const BlockValidationState& state) {
+    ::BlockChecked(block, state, connman);
+}
+
 //////////////////////////////////////////////////////////////////////////////
 //
 // Messages

From a49a153f7cab063ce4f9ae898c9ffa94e8fcf510 Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Fri, 15 Nov 2019 14:56:02 -0500
Subject: [PATCH 05/13] [validation] Return the AcceptBlock
 BlockValidationState directly in ProcessNewBlock

Net processing now passes a BlockValidationState object into
ProcessNewBlock(). If CheckBlock() or AcceptBlock() fails, then PNB
returns to net processing without calling the (asynchronous)
BlockChecked Validation Interface method. net processing can use the
invalid BlockValidationState returned to punish peers.

CheckBlock() and AcceptBlock() represent the DoS checks on a block (ie
PoW and malleability). Net processing wants to know about those failed
checks immediately and shouldn't have to wait on a callback. Other
validation interface clients don't care about net processing submitting
bogus malleated blocks to validation, so they don't need to be notified
of BlockChecked.

Furthermore, if PNB returns a valid BlockValidationState, we never need
to try to process (non-malleated) copies of the block from other peers.
That makes it much easier to move the best chain activation logic to a
background thread in future work.

Co-authored-by: John Newbery <john@johnnewbery.com>
Co-authored-by: Carl Dong <contact@carldong.me>
---
 src/net_processing.cpp | 23 +++++++++++++++--------
 src/rpc/mining.cpp     |  3 +++
 src/validation.cpp     |  1 -
 src/validation.h       | 27 +++++++++++++++++++--------
 4 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 1ba7c8d8dacf..a69e8f04160f 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1888,16 +1888,23 @@ void static ProcessOrphanTx(CConnman* connman, std::set<uint256>& orphan_work_se
 }
 
 /**
- *  A block has been processed. If this is the first time we've seen the block,
+ *  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, std::shared_ptr<CBlock> pblock, bool new_block)
+void static BlockProcessed(CNode* pfrom, CConnman* connman, std::shared_ptr<CBlock> pblock, BlockValidationState& state, bool new_block)
 {
-    if (new_block) {
-        pfrom->nLastBlockTime = GetTime();
-    } else {
+    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();
     }
 }
 
@@ -2841,7 +2848,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
             // reconstructed compact blocks as having been requested.
             BlockValidationState dos_state;
             ProcessNewBlock(chainparams, pblock, dos_state, /*fForceProcessing=*/true, &fNewBlock);
-            BlockProcessed(pfrom, pblock, fNewBlock);
+            BlockProcessed(pfrom, connman, pblock, dos_state, fNewBlock);
 
             LOCK(cs_main); // hold cs_main for CBlockIndex::IsValid()
             if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS)) {
@@ -2928,7 +2935,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
             // in compact block optimistic reconstruction handling.
             BlockValidationState dos_state;
             ProcessNewBlock(chainparams, pblock, dos_state, /*fForceProcessing=*/true, &fNewBlock);
-            BlockProcessed(pfrom, pblock, fNewBlock);
+            BlockProcessed(pfrom, connman, pblock, dos_state, fNewBlock);
         }
         return true;
     }
@@ -2987,7 +2994,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
         bool fNewBlock = false;
         BlockValidationState dos_state;
         ProcessNewBlock(chainparams, pblock, dos_state, forceProcessing, &fNewBlock);
-        BlockProcessed(pfrom, pblock, fNewBlock);
+        BlockProcessed(pfrom, connman, pblock, dos_state, fNewBlock);
         return true;
     }
 
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index c4b3a7ea343a..7427b4d70d47 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -784,6 +784,9 @@ static UniValue submitblock(const JSONRPCRequest& request)
     if (!new_block && accepted) {
         return "duplicate";
     }
+    if (!dos_state.IsValid()) {
+        return BIP22ValidationResult(dos_state);
+    }
     if (!sc.found) {
         return "inconclusive";
     }
diff --git a/src/validation.cpp b/src/validation.cpp
index 56262af58ed6..19791f75f4da 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3780,7 +3780,6 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons
             ret = ::ChainstateActive().AcceptBlock(pblock, dos_state, chainparams, &pindex, fForceProcessing, nullptr, fNewBlock);
         }
         if (!ret) {
-            GetMainSignals().BlockChecked(*pblock, dos_state);
             return error("%s: AcceptBlock FAILED (%s)", __func__, FormatStateMessage(dos_state));
         }
     }
diff --git a/src/validation.h b/src/validation.h
index faea14d2aa24..86c2dc7c8923 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -199,18 +199,29 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
  * 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[out]  state             Currently unused.
+ * @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  Process this block even if unrequested; used for non-network block sources and whitelisted peers.
  * @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

From 91d26915cd98e53c235d4b493b326f781837f1d7 Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Fri, 15 Nov 2019 14:56:45 -0500
Subject: [PATCH 06/13] [validation] trivial: Rename state to dummy_state for
 clarity

This is a pure refactor commit.

Co-authored-by: John Newbery <john@johnnewbery.com>
Co-authored-by: Carl Dong <contact@carldong.me>
---
 src/validation.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/validation.cpp b/src/validation.cpp
index 19791f75f4da..c8893a21be05 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3786,9 +3786,9 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons
 
     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__, FormatStateMessage(state));
+    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__, FormatStateMessage(dummy_state));
 
     return true;
 }

From 0bf87d2034902e23cc4b58c2f6ae0b0ae4af48cc Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Fri, 15 Nov 2019 14:58:23 -0500
Subject: [PATCH 07/13] [test/rpc] Additional checks for dos_state validity

Co-authored-by: John Newbery <john@johnnewbery.com>
Co-authored-by: Carl Dong <contact@carldong.me>
---
 src/rpc/mining.cpp                   | 2 +-
 src/test/blockfilter_index_tests.cpp | 3 +++
 src/test/miner_tests.cpp             | 1 +
 src/test/util.cpp                    | 1 +
 src/test/validation_block_tests.cpp  | 3 ++-
 5 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index 7427b4d70d47..6f713206a823 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -136,7 +136,7 @@ static UniValue generateBlocks(const CScript& coinbase_script, int nGenerate, ui
         }
         std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(*pblock);
         BlockValidationState state;
-        if (!ProcessNewBlock(Params(), shared_pblock, state, true, nullptr))
+        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());
diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp
index aecd02dd2369..e85234a2040e 100644
--- a/src/test/blockfilter_index_tests.cpp
+++ b/src/test/blockfilter_index_tests.cpp
@@ -166,6 +166,7 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, TestChain100Setup)
         const auto& block = chainA[i];
         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];
@@ -185,6 +186,7 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, TestChain100Setup)
         const auto& block = chainB[i];
         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];
@@ -217,6 +219,7 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, TestChain100Setup)
         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.
diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp
index 2bd09321cf98..b1451420df95 100644
--- a/src/test/miner_tests.cpp
+++ b/src/test/miner_tests.cpp
@@ -250,6 +250,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
         std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(*pblock);
         BlockValidationState dos_state;
         BOOST_CHECK(ProcessNewBlock(chainparams, shared_pblock, dos_state, true, nullptr));
+        BOOST_CHECK(dos_state.IsValid());
         pblock->hashPrevBlock = pblock->GetHash();
     }
 
diff --git a/src/test/util.cpp b/src/test/util.cpp
index fa6951ec24b2..f6939352a803 100644
--- a/src/test/util.cpp
+++ b/src/test/util.cpp
@@ -67,6 +67,7 @@ CTxIn MineBlock(const CScript& coinbase_scriptPubKey)
     BlockValidationState dos_state;
     bool processed{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/validation_block_tests.cpp b/src/test/validation_block_tests.cpp
index 34a6aef28bee..f7f970e0f963 100644
--- a/src/test/validation_block_tests.cpp
+++ b/src/test/validation_block_tests.cpp
@@ -189,6 +189,7 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
                     BlockValidationState dos_state;
                     bool processed = ProcessNewBlock(Params(), block, dos_state, true, &ignored);
                     assert(processed);
+                    assert(dos_state.IsValid());
                 }
             }
         });
@@ -229,7 +230,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
     bool ignored;
     auto ProcessBlock = [&ignored](std::shared_ptr<const CBlock> block) -> bool {
         BlockValidationState dos_state;
-        return ProcessNewBlock(Params(), block, dos_state, /* fForceProcessing */ true, /* fNewBlock */ &ignored);
+        return ProcessNewBlock(Params(), block, dos_state, /* fForceProcessing */ true, /* fNewBlock */ &ignored) && dos_state.IsValid();
     };
 
     // Process all mined blocks

From e6a70e5b8f76a4261c47521e92979eebc9a46f4b Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Thu, 14 Nov 2019 17:18:55 -0500
Subject: [PATCH 08/13] [net processing] Improve comments for
 MarkBlockAsInFlight and MarkBlockAsReceived

---
 src/net_processing.cpp | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index a69e8f04160f..b07cbf2dc180 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -430,8 +430,17 @@ 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
+/**
+ * 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 MarkBlockAsReceived(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()) {
@@ -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);

From 367ce754aa8b4a95697254ac5fdbdd7628efa4c6 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Thu, 14 Nov 2019 17:19:29 -0500
Subject: [PATCH 09/13] [net processing] rename MarkBlockAsReceived ->
 MarkBlockAsNotInFlight

The previous name was misleading, since we can call the function even
when the block has not been received.
---
 src/net_processing.cpp | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index b07cbf2dc180..0d1c0a93e4ef 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -441,7 +441,7 @@ static void PushNodeVersion(CNode *pnode, CConnman* connman, int64_t nTime)
  * @param[in]      hash       The block hash
  * @return         bool       Whether this block was already in flight from a peer.
  */
-static bool MarkBlockAsReceived(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
+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);
@@ -495,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)});
@@ -2782,7 +2782,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) {
@@ -2880,7 +2880,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
                 // 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;
@@ -2912,7 +2912,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) {
@@ -2938,7 +2938,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
@@ -3009,7 +3009,7 @@ 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.

From 11b7abdb42656f194b82eee46412ab2691f2efa4 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Thu, 14 Nov 2019 16:25:08 -0500
Subject: [PATCH 10/13] [net processing] Don't reach into CBlockIndex to check
 for block mutation

If a CMPCTBLOCK is in flight from peer A and we then succesfully
reconstruct it during CMPCTBLOCK processing from peer B, we need to
clear the in-flight state for the block from peer A.

We can only do that once we've ensured that the block hasn't been
mutated (otherwise peer B could interfere with our block relay from peer
A by providing a mutated block).

Mutation-checking used to be done indirectly by checking that the block
had been writted to disk by checking the CBlockIndex. Now that
ProcessNewBlock returns a BlockValidationState, we can check that state
directly to determine whether to mark the block as no longer in-flight.
---
 src/net_processing.cpp | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 0d1c0a93e4ef..90a4c03b1cbb 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2874,12 +2874,15 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
             ProcessNewBlock(chainparams, pblock, dos_state, /*fForceProcessing=*/true, &fNewBlock);
             BlockProcessed(pfrom, 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
-                // 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.
+            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);
                 MarkBlockAsNotInFlight(pblock->GetHash());
             }
         }

From e297b7f20ee31e091bf31e9a1875bb9cc875399a Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Fri, 22 Nov 2019 15:11:46 -0500
Subject: [PATCH 11/13] [validation] Only call BlockChecked if block connection
 fails

The BlockChecked callback is now only used by net_processing for the
following purposes:

1. banning peers that serve us invalid blocks
2. marking peers that send us valid blocks as compact block peers
3. removing blocks from the mapBlockSource map

To disentangle the meaning of BlockChecked, we move (2) to the
BlockConnected callback in net_processing, and only call BlockChecked if
the block fails to connect.
---
 src/net_processing.cpp | 46 ++++++++++++++++++++++--------------------
 src/validation.cpp     |  5 +++--
 2 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 90a4c03b1cbb..a68616c38ae6 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1153,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) {
@@ -1273,8 +1289,8 @@ 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).
@@ -1285,28 +1301,14 @@ static void BlockChecked(const CBlock& block, const BlockValidationState& state,
     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::BlockChecked(const CBlock& block, const BlockValidationState& state) {
diff --git a/src/validation.cpp b/src/validation.cpp
index c8893a21be05..f3d38370b379 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2558,10 +2558,11 @@ bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& ch
     {
         CCoinsViewCache view(&CoinsTip());
         bool rv = ConnectBlock(blockConnecting, state, pindexNew, view, chainparams);
-        GetMainSignals().BlockChecked(blockConnecting, state);
         if (!rv) {
-            if (state.IsInvalid())
+            if (state.IsInvalid()) {
+                GetMainSignals().BlockChecked(blockConnecting, state);
                 InvalidBlockFound(pindexNew, state);
+            }
             return error("%s: ConnectBlock %s failed, %s", __func__, pindexNew->GetBlockHash().ToString(), FormatStateMessage(state));
         }
         nTime3 = GetTimeMicros(); nTimeConnectTotal += nTime3 - nTime2;

From 59378abcece667e28bf8bb858b39341627ff8d70 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Fri, 22 Nov 2019 15:50:27 -0500
Subject: [PATCH 12/13] [validation interface] Rename BlockChecked to
 BlockFailedConnection

BlockChecked is only called in cases where the block fails ConnectBlock().
Rename the method to be more acurate.
---
 src/net_processing.cpp      |  2 +-
 src/net_processing.h        |  2 +-
 src/rpc/mining.cpp          |  2 +-
 src/validation.cpp          |  2 +-
 src/validation.h            |  6 +++---
 src/validationinterface.cpp | 10 +++++-----
 src/validationinterface.h   | 12 ++++++------
 7 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index a68616c38ae6..c5e9cab5689c 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1311,7 +1311,7 @@ static void BlockChecked(const CBlock& block, const BlockValidationState& state,
     }
 }
 
-void PeerLogicValidation::BlockChecked(const CBlock& block, const BlockValidationState& state) {
+void PeerLogicValidation::BlockFailedConnection(const CBlock& block, const BlockValidationState& state) {
     ::BlockChecked(block, state, connman);
 }
 
diff --git a/src/net_processing.h b/src/net_processing.h
index 4adb7d3a2151..9a34e2e55f75 100644
--- a/src/net_processing.h
+++ b/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.
      */
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index 6f713206a823..aa593aa24e71 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -718,7 +718,7 @@ 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;
diff --git a/src/validation.cpp b/src/validation.cpp
index f3d38370b379..6af0976a2c13 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2560,7 +2560,7 @@ bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& ch
         bool rv = ConnectBlock(blockConnecting, state, pindexNew, view, chainparams);
         if (!rv) {
             if (state.IsInvalid()) {
-                GetMainSignals().BlockChecked(blockConnecting, state);
+                GetMainSignals().BlockFailedConnection(blockConnecting, state);
                 InvalidBlockFound(pindexNew, state);
             }
             return error("%s: ConnectBlock %s failed, %s", __func__, pindexNew->GetBlockHash().ToString(), FormatStateMessage(state));
diff --git a/src/validation.h b/src/validation.h
index 86c2dc7c8923..d396bbfa3037 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -203,9 +203,9 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
  * 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
+ * BlockFailedConnection 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
+ * BlockFailedConnection). There is, of course, no guarantee that any given block which
  * is not a part of the eventual best chain will ever be checked.
  *
  * If the block pblock is built on is in our header tree, and pblock is a
@@ -221,7 +221,7 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
  *
  * @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.
+ *                                a CValidationInterface BlockFailedConnection callback is used to notify clients of validity.
  * @param[in]   fForceProcessing  Process this block even if unrequested; used for non-network block sources and whitelisted peers.
  * @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
diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp
index 663308bae951..e4fa3582faac 100644
--- a/src/validationinterface.cpp
+++ b/src/validationinterface.cpp
@@ -21,7 +21,7 @@ struct ValidationInterfaceConnections {
     boost::signals2::scoped_connection BlockDisconnected;
     boost::signals2::scoped_connection TransactionRemovedFromMempool;
     boost::signals2::scoped_connection ChainStateFlushed;
-    boost::signals2::scoped_connection BlockChecked;
+    boost::signals2::scoped_connection BlockFailedConnection;
     boost::signals2::scoped_connection NewPoWValidBlock;
 };
 
@@ -32,7 +32,7 @@ struct MainSignalsInstance {
     boost::signals2::signal<void (const std::shared_ptr<const CBlock>&, const CBlockIndex* pindex)> BlockDisconnected;
     boost::signals2::signal<void (const CTransactionRef &)> TransactionRemovedFromMempool;
     boost::signals2::signal<void (const CBlockLocator &)> ChainStateFlushed;
-    boost::signals2::signal<void (const CBlock&, const BlockValidationState&)> BlockChecked;
+    boost::signals2::signal<void (const CBlock&, const BlockValidationState&)> BlockFailedConnection;
     boost::signals2::signal<void (const CBlockIndex *, const std::shared_ptr<const CBlock>&)> NewPoWValidBlock;
 
     // We are not allowed to assume the scheduler only runs in one thread,
@@ -95,7 +95,7 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) {
     conns.BlockDisconnected = g_signals.m_internals->BlockDisconnected.connect(std::bind(&CValidationInterface::BlockDisconnected, pwalletIn, std::placeholders::_1, std::placeholders::_2));
     conns.TransactionRemovedFromMempool = g_signals.m_internals->TransactionRemovedFromMempool.connect(std::bind(&CValidationInterface::TransactionRemovedFromMempool, pwalletIn, std::placeholders::_1));
     conns.ChainStateFlushed = g_signals.m_internals->ChainStateFlushed.connect(std::bind(&CValidationInterface::ChainStateFlushed, pwalletIn, std::placeholders::_1));
-    conns.BlockChecked = g_signals.m_internals->BlockChecked.connect(std::bind(&CValidationInterface::BlockChecked, pwalletIn, std::placeholders::_1, std::placeholders::_2));
+    conns.BlockFailedConnection = g_signals.m_internals->BlockFailedConnection.connect(std::bind(&CValidationInterface::BlockFailedConnection, pwalletIn, std::placeholders::_1, std::placeholders::_2));
     conns.NewPoWValidBlock = g_signals.m_internals->NewPoWValidBlock.connect(std::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, std::placeholders::_1, std::placeholders::_2));
 }
 
@@ -169,8 +169,8 @@ void CMainSignals::ChainStateFlushed(const CBlockLocator &locator) {
     });
 }
 
-void CMainSignals::BlockChecked(const CBlock& block, const BlockValidationState& state) {
-    m_internals->BlockChecked(block, state);
+void CMainSignals::BlockFailedConnection(const CBlock& block, const BlockValidationState& state) {
+    m_internals->BlockFailedConnection(block, state);
 }
 
 void CMainSignals::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock> &block) {
diff --git a/src/validationinterface.h b/src/validationinterface.h
index 6a8059a6a01e..ae4f29f2a7d0 100644
--- a/src/validationinterface.h
+++ b/src/validationinterface.h
@@ -99,6 +99,9 @@ class CValidationInterface {
      * replacement. This does not include any transactions which are included
      * in BlockConnectedDisconnected either in block->vtx or in txnConflicted.
      *
+     * The provided block is guaranteed to be the current best block at the
+     * time the callback was generated (not necessarily now).
+     *
      * Called on a background thread.
      */
     virtual void TransactionRemovedFromMempool(const CTransactionRef &ptx) {}
@@ -133,12 +136,9 @@ class CValidationInterface {
      */
     virtual void ChainStateFlushed(const CBlockLocator &locator) {}
     /**
-     * Notifies listeners of a block validation result.
-     * If the provided BlockValidationState IsValid, the provided block
-     * is guaranteed to be the current best block at the time the
-     * callback was generated (not necessarily now)
+     * Validation attempted to connect the block to the best chain, but block connection failed.
      */
-    virtual void BlockChecked(const CBlock&, const BlockValidationState&) {}
+    virtual void BlockFailedConnection(const CBlock&, const BlockValidationState&) {}
     /**
      * Notifies listeners that a block which builds directly on our current tip
      * has been received and connected to the headers tree, though not validated yet */
@@ -180,7 +180,7 @@ class CMainSignals {
     void BlockConnected(const std::shared_ptr<const CBlock> &, const CBlockIndex *pindex, const std::shared_ptr<const std::vector<CTransactionRef>> &);
     void BlockDisconnected(const std::shared_ptr<const CBlock> &, const CBlockIndex* pindex);
     void ChainStateFlushed(const CBlockLocator &);
-    void BlockChecked(const CBlock&, const BlockValidationState&);
+    void BlockFailedConnection(const CBlock&, const BlockValidationState&);
     void NewPoWValidBlock(const CBlockIndex *, const std::shared_ptr<const CBlock>&);
 };
 

From bc9ac68c2f4514f188f10376bd061493d1d4d013 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Fri, 22 Nov 2019 16:14:53 -0500
Subject: [PATCH 13/13] Move BlockFailedConnection to the background thread

---
 src/rpc/mining.cpp          | 8 ++++++++
 src/validationinterface.cpp | 5 ++++-
 src/validationinterface.h   | 2 ++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index aa593aa24e71..c5e7e1ceb77c 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -724,6 +724,11 @@ class submitblock_StateCatcher : public CValidationInterface
         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)
@@ -780,6 +785,9 @@ static UniValue submitblock(const JSONRPCRequest& request)
     RegisterValidationInterface(&sc);
     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";
diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp
index e4fa3582faac..7424f2b71527 100644
--- a/src/validationinterface.cpp
+++ b/src/validationinterface.cpp
@@ -8,6 +8,7 @@
 #include <primitives/block.h>
 #include <scheduler.h>
 #include <txmempool.h>
+#include <consensus/validation.h>
 
 #include <future>
 #include <utility>
@@ -170,7 +171,9 @@ void CMainSignals::ChainStateFlushed(const CBlockLocator &locator) {
 }
 
 void CMainSignals::BlockFailedConnection(const CBlock& block, const BlockValidationState& state) {
-    m_internals->BlockFailedConnection(block, state);
+    m_internals->m_schedulerClient.AddToProcessQueue([block, state, this] {
+        m_internals->BlockFailedConnection(block, state);
+    });
 }
 
 void CMainSignals::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock> &block) {
diff --git a/src/validationinterface.h b/src/validationinterface.h
index ae4f29f2a7d0..7d81944e8541 100644
--- a/src/validationinterface.h
+++ b/src/validationinterface.h
@@ -137,6 +137,8 @@ class CValidationInterface {
     virtual void ChainStateFlushed(const CBlockLocator &locator) {}
     /**
      * Validation attempted to connect the block to the best chain, but block connection failed.
+     *
+     * Called on a background thread.
      */
     virtual void BlockFailedConnection(const CBlock&, const BlockValidationState&) {}
     /**