From fa457fbd3387661e1973a8f4e5cc2def79e0c625 Mon Sep 17 00:00:00 2001
From: MarcoFalke <falke.marco@gmail.com>
Date: Mon, 8 Jun 2020 08:54:23 -0400
Subject: [PATCH 1/4] move-only: Move NDEBUG compile time check to util/check

---
 src/net_processing.cpp | 11 ++++-------
 src/util/check.h       |  4 ++++
 src/validation.cpp     |  5 +----
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index d48745aef25..06a6557b3b1 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -13,10 +13,9 @@
 #include <consensus/validation.h>
 #include <hash.h>
 #include <index/blockfilterindex.h>
-#include <validation.h>
 #include <merkleblock.h>
-#include <netmessagemaker.h>
 #include <netbase.h>
+#include <netmessagemaker.h>
 #include <policy/fees.h>
 #include <policy/policy.h>
 #include <primitives/block.h>
@@ -26,16 +25,14 @@
 #include <scheduler.h>
 #include <tinyformat.h>
 #include <txmempool.h>
-#include <util/system.h>
+#include <util/check.h> // For NDEBUG compile time check
 #include <util/strencodings.h>
+#include <util/system.h>
+#include <validation.h>
 
 #include <memory>
 #include <typeinfo>
 
-#if defined(NDEBUG)
-# error "Bitcoin cannot be compiled without assertions."
-#endif
-
 /** Expiration time for orphan transactions in seconds */
 static constexpr int64_t ORPHAN_TX_EXPIRE_TIME = 20 * 60;
 /** Minimum time between orphan transactions expire time checks in seconds */
diff --git a/src/util/check.h b/src/util/check.h
index 5c0f32cf519..5cc078b36b8 100644
--- a/src/util/check.h
+++ b/src/util/check.h
@@ -42,4 +42,8 @@ class NonFatalCheckError : public std::runtime_error
         }                                                         \
     } while (false)
 
+#if defined(NDEBUG)
+#error "Cannot compile without assertions!"
+#endif
+
 #endif // BITCOIN_UTIL_CHECK_H
diff --git a/src/validation.cpp b/src/validation.cpp
index cbe89443b82..9f02db80c85 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -39,6 +39,7 @@
 #include <ui_interface.h>
 #include <uint256.h>
 #include <undo.h>
+#include <util/check.h> // For NDEBUG compile time check
 #include <util/moneystr.h>
 #include <util/rbf.h>
 #include <util/strencodings.h>
@@ -51,10 +52,6 @@
 
 #include <boost/algorithm/string/replace.hpp>
 
-#if defined(NDEBUG)
-# error "Bitcoin cannot be compiled without assertions."
-#endif
-
 #define MICRO 0.000001
 #define MILLI 0.001
 

From fa6ef701adba1cb48535cac25fd43c742a82e40d Mon Sep 17 00:00:00 2001
From: MarcoFalke <falke.marco@gmail.com>
Date: Mon, 8 Jun 2020 08:47:10 -0400
Subject: [PATCH 2/4] util: Add Assert identity function

The utility is primarily useful to dereference pointer types, which are
known to be not null at that time.

For example, the ArgsManager is known to exist when the wallets are
started. Instead of silently relying on that assumption, Assert can be
used to abort the program and avoid UB should the assumption ever be
violated.
---
 src/test/util/mining.cpp     |  4 ++--
 src/test/util/setup_common.h |  1 +
 src/util/check.h             | 12 +++++++++++-
 test/lint/lint-assertions.sh |  2 +-
 4 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/src/test/util/mining.cpp b/src/test/util/mining.cpp
index dac7f1a07b8..b0c74954db7 100644
--- a/src/test/util/mining.cpp
+++ b/src/test/util/mining.cpp
@@ -11,6 +11,7 @@
 #include <node/context.h>
 #include <pow.h>
 #include <script/standard.h>
+#include <util/check.h>
 #include <validation.h>
 
 CTxIn generatetoaddress(const NodeContext& node, const std::string& address)
@@ -39,9 +40,8 @@ CTxIn MineBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
 
 std::shared_ptr<CBlock> PrepareBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
 {
-    assert(node.mempool);
     auto block = std::make_shared<CBlock>(
-        BlockAssembler{*node.mempool, Params()}
+        BlockAssembler{*Assert(node.mempool), Params()}
             .CreateNewBlock(coinbase_scriptPubKey)
             ->block);
 
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index d5cda8a95b3..b70039e8489 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -13,6 +13,7 @@
 #include <random.h>
 #include <scheduler.h>
 #include <txmempool.h>
+#include <util/check.h>
 #include <util/string.h>
 
 #include <type_traits>
diff --git a/src/util/check.h b/src/util/check.h
index 5cc078b36b8..3d534fd33e0 100644
--- a/src/util/check.h
+++ b/src/util/check.h
@@ -25,7 +25,7 @@ class NonFatalCheckError : public std::runtime_error
  * - where the condition is assumed to be true, not for error handling or validating user input
  * - where a failure to fulfill the condition is recoverable and does not abort the program
  *
- * For example in RPC code, where it is undersirable to crash the whole program, this can be generally used to replace
+ * For example in RPC code, where it is undesirable to crash the whole program, this can be generally used to replace
  * asserts or recoverable logic errors. A NonFatalCheckError in RPC code is caught and passed as a string to the RPC
  * caller, which can then report the issue to the developers.
  */
@@ -46,4 +46,14 @@ class NonFatalCheckError : public std::runtime_error
 #error "Cannot compile without assertions!"
 #endif
 
+/** Helper for Assert(). TODO remove in C++14 and replace `decltype(get_pure_r_value(val))` with `T` (templated lambda) */
+template <typename T>
+T get_pure_r_value(T&& val)
+{
+    return std::forward<T>(val);
+}
+
+/** Identity function. Abort if the value compares equal to zero */
+#define Assert(val) [&]() -> decltype(get_pure_r_value(val))& { auto& check = (val); assert(#val && check); return check; }()
+
 #endif // BITCOIN_UTIL_CHECK_H
diff --git a/test/lint/lint-assertions.sh b/test/lint/lint-assertions.sh
index 1aacc09bcc6..d30a8ca231a 100755
--- a/test/lint/lint-assertions.sh
+++ b/test/lint/lint-assertions.sh
@@ -23,7 +23,7 @@ fi
 # Macro CHECK_NONFATAL(condition) should be used instead of assert for RPC code, where it
 # is undesirable to crash the whole program. See: src/util/check.h
 # src/rpc/server.cpp is excluded from this check since it's mostly meta-code.
-OUTPUT=$(git grep -nE 'assert *\(.*\);' -- "src/rpc/" "src/wallet/rpc*" ":(exclude)src/rpc/server.cpp")
+OUTPUT=$(git grep -nE '\<(A|a)ssert *\(.*\);' -- "src/rpc/" "src/wallet/rpc*" ":(exclude)src/rpc/server.cpp")
 if [[ ${OUTPUT} != "" ]]; then
     echo "CHECK_NONFATAL(condition) should be used instead of assert for RPC code."
     echo

From fa34587f1c811d99200453b0936219c473f514b0 Mon Sep 17 00:00:00 2001
From: MarcoFalke <falke.marco@gmail.com>
Date: Sun, 14 Jun 2020 14:44:35 -0400
Subject: [PATCH 3/4] scripted-diff: Replace EnsureChainman with Assert in unit
 tests

-BEGIN VERIFY SCRIPT-
sed --regexp-extended -i -e 's/EnsureChainman\((m?_?node)\)\./Assert(\1.chainman)->/g' $(git grep -l EnsureChainman)
-END VERIFY SCRIPT-
---
 src/test/blockfilter_index_tests.cpp |  8 ++++----
 src/test/miner_tests.cpp             |  2 +-
 src/test/util/mining.cpp             |  2 +-
 src/test/util/setup_common.cpp       |  2 +-
 src/test/validation_block_tests.cpp  | 10 +++++-----
 src/wallet/test/wallet_tests.cpp     |  6 +++---
 6 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp
index 7dff2e6e869..00c4bdc14ef 100644
--- a/src/test/blockfilter_index_tests.cpp
+++ b/src/test/blockfilter_index_tests.cpp
@@ -94,7 +94,7 @@ bool BuildChainTestingSetup::BuildChain(const CBlockIndex* pindex,
         CBlockHeader header = block->GetBlockHeader();
 
         BlockValidationState state;
-        if (!EnsureChainman(m_node).ProcessNewBlockHeaders({header}, state, Params(), &pindex)) {
+        if (!Assert(m_node.chainman)->ProcessNewBlockHeaders({header}, state, Params(), &pindex)) {
             return false;
         }
     }
@@ -171,7 +171,7 @@ 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(EnsureChainman(m_node).ProcessNewBlock(Params(), block, true, nullptr));
+        BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(Params(), block, true, nullptr));
     }
     for (size_t i = 0; i < 2; i++) {
         const auto& block = chainA[i];
@@ -189,7 +189,7 @@ 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(EnsureChainman(m_node).ProcessNewBlock(Params(), block, true, nullptr));
+        BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(Params(), block, true, nullptr));
     }
     for (size_t i = 0; i < 3; i++) {
         const auto& block = chainB[i];
@@ -220,7 +220,7 @@ 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(EnsureChainman(m_node).ProcessNewBlock(Params(), block, true, nullptr));
+         BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(Params(), block, true, nullptr));
      }
 
      // 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 11ff7b833bf..62a0dc42413 100644
--- a/src/test/miner_tests.cpp
+++ b/src/test/miner_tests.cpp
@@ -253,7 +253,7 @@ 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(EnsureChainman(m_node).ProcessNewBlock(chainparams, shared_pblock, true, nullptr));
+        BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlock(chainparams, shared_pblock, true, nullptr));
         pblock->hashPrevBlock = pblock->GetHash();
     }
 
diff --git a/src/test/util/mining.cpp b/src/test/util/mining.cpp
index b0c74954db7..74536ae74ce 100644
--- a/src/test/util/mining.cpp
+++ b/src/test/util/mining.cpp
@@ -32,7 +32,7 @@ CTxIn MineBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
         assert(block->nNonce);
     }
 
-    bool processed{EnsureChainman(node).ProcessNewBlock(Params(), block, true, nullptr)};
+    bool processed{Assert(node.chainman)->ProcessNewBlock(Params(), block, 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 3b7a7c8d121..b8a64c32317 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -228,7 +228,7 @@ 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);
-    EnsureChainman(m_node).ProcessNewBlock(chainparams, shared_pblock, true, nullptr);
+    Assert(m_node.chainman)->ProcessNewBlock(chainparams, shared_pblock, true, nullptr);
 
     CBlock result = block;
     return result;
diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp
index 45e0c5484e3..8e85b7df3ee 100644
--- a/src/test/validation_block_tests.cpp
+++ b/src/test/validation_block_tests.cpp
@@ -163,10 +163,10 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
     std::transform(blocks.begin(), blocks.end(), std::back_inserter(headers), [](std::shared_ptr<const CBlock> b) { return b->GetBlockHeader(); });
 
     // Process all the headers so we understand the toplogy of the chain
-    BOOST_CHECK(EnsureChainman(m_node).ProcessNewBlockHeaders(headers, state, Params()));
+    BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlockHeaders(headers, state, Params()));
 
     // Connect the genesis block and drain any outstanding events
-    BOOST_CHECK(EnsureChainman(m_node).ProcessNewBlock(Params(), std::make_shared<CBlock>(Params().GenesisBlock()), true, &ignored));
+    BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlock(Params(), std::make_shared<CBlock>(Params().GenesisBlock()), true, &ignored));
     SyncWithValidationInterfaceQueue();
 
     // subscribe to events (this subscriber will validate event ordering)
@@ -188,13 +188,13 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
             FastRandomContext insecure;
             for (int i = 0; i < 1000; i++) {
                 auto block = blocks[insecure.randrange(blocks.size() - 1)];
-                EnsureChainman(m_node).ProcessNewBlock(Params(), block, true, &ignored);
+                Assert(m_node.chainman)->ProcessNewBlock(Params(), block, 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 = EnsureChainman(m_node).ProcessNewBlock(Params(), block, true, &ignored);
+                    bool processed = Assert(m_node.chainman)->ProcessNewBlock(Params(), block, true, &ignored);
                     assert(processed);
                 }
             }
@@ -233,7 +233,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
 {
     bool ignored;
     auto ProcessBlock = [&](std::shared_ptr<const CBlock> block) -> bool {
-        return EnsureChainman(m_node).ProcessNewBlock(Params(), block, /* fForceProcessing */ true, /* fNewBlock */ &ignored);
+        return Assert(m_node.chainman)->ProcessNewBlock(Params(), block, /* fForceProcessing */ true, /* fNewBlock */ &ignored);
     };
 
     // Process all mined blocks
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 6f8c973f25c..e27264e8d24 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -118,7 +118,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
     // Prune the older block file.
     {
         LOCK(cs_main);
-        EnsureChainman(m_node).PruneOneBlockFile(oldTip->GetBlockPos().nFile);
+        Assert(m_node.chainman)->PruneOneBlockFile(oldTip->GetBlockPos().nFile);
     }
     UnlinkPrunedFiles({oldTip->GetBlockPos().nFile});
 
@@ -144,7 +144,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
     // Prune the remaining block file.
     {
         LOCK(cs_main);
-        EnsureChainman(m_node).PruneOneBlockFile(newTip->GetBlockPos().nFile);
+        Assert(m_node.chainman)->PruneOneBlockFile(newTip->GetBlockPos().nFile);
     }
     UnlinkPrunedFiles({newTip->GetBlockPos().nFile});
 
@@ -181,7 +181,7 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup)
     // Prune the older block file.
     {
         LOCK(cs_main);
-        EnsureChainman(m_node).PruneOneBlockFile(oldTip->GetBlockPos().nFile);
+        Assert(m_node.chainman)->PruneOneBlockFile(oldTip->GetBlockPos().nFile);
     }
     UnlinkPrunedFiles({oldTip->GetBlockPos().nFile});
 

From fab80fef61ddd4afeff6e497c7e76bffcd05e8a4 Mon Sep 17 00:00:00 2001
From: MarcoFalke <falke.marco@gmail.com>
Date: Sun, 14 Jun 2020 14:17:24 -0400
Subject: [PATCH 4/4] refactor: Remove unused EnsureChainman

---
 src/init.cpp           | 5 +++--
 src/node/context.h     | 6 ------
 src/rpc/blockchain.cpp | 5 ++++-
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/src/init.cpp b/src/init.cpp
index fd7c8d0f801..41b4700dde5 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -50,6 +50,7 @@
 #include <txmempool.h>
 #include <ui_interface.h>
 #include <util/asmap.h>
+#include <util/check.h>
 #include <util/moneystr.h>
 #include <util/string.h>
 #include <util/system.h>
@@ -1377,9 +1378,9 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
     node.mempool = &::mempool;
     assert(!node.chainman);
     node.chainman = &g_chainman;
-    ChainstateManager& chainman = EnsureChainman(node);
+    ChainstateManager& chainman = *Assert(node.chainman);
 
-    node.peer_logic.reset(new PeerLogicValidation(node.connman.get(), node.banman.get(), *node.scheduler, *node.chainman, *node.mempool));
+    node.peer_logic.reset(new PeerLogicValidation(node.connman.get(), node.banman.get(), *node.scheduler, chainman, *node.mempool));
     RegisterValidationInterface(node.peer_logic.get());
 
     // sanitize comments per BIP-0014, format user agent and check total size
diff --git a/src/node/context.h b/src/node/context.h
index c45d9e66893..c783c39cd64 100644
--- a/src/node/context.h
+++ b/src/node/context.h
@@ -49,10 +49,4 @@ struct NodeContext {
     ~NodeContext();
 };
 
-inline ChainstateManager& EnsureChainman(const NodeContext& node)
-{
-    assert(node.chainman);
-    return *node.chainman;
-}
-
 #endif // BITCOIN_NODE_CONTEXT_H
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 8252af3ee60..cd1b27490dd 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -75,7 +75,10 @@ CTxMemPool& EnsureMemPool(const util::Ref& context)
 ChainstateManager& EnsureChainman(const util::Ref& context)
 {
     NodeContext& node = EnsureNodeContext(context);
-    return EnsureChainman(node);
+    if (!node.chainman) {
+        throw JSONRPCError(RPC_INTERNAL_ERROR, "Node chainman not found");
+    }
+    return *node.chainman;
 }
 
 /* Calculate the difficulty for a given block index.