Merge #19277: util: Add Assert identity function

MarcoFalke · MarcoFalke · commit 5ec19df687c2 · 2020-07-04T08:44:45.000-04:00
fab80fe refactor: Remove unused EnsureChainman (MarcoFalke) fa34587 scripted-diff: Replace EnsureChainman with Assert in unit tests (MarcoFalke) fa6ef70 util: Add Assert identity function (MarcoFalke) fa457fb move-only: Move NDEBUG compile time check to util/check (MarcoFalke) Pull request description: The utility function 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: https://github.com/bitcoin/bitcoin/pull/18923/files#diff-fdb2a1a1d8bc790fcddeb6cf5a42ac55R503 . Instead of silently relying on that assumption, `Assert` can be used to abort the program and avoid UB should the assumption ever be violated. ACKs for top commit: promag: Tested ACK fab80fe. ryanofsky: Code review ACK fab80fe Tree-SHA512: 830fba10152ba17d47c4dd42809c7e26f9fe6d38e17a2d5b3f054fd644a5c4c9841286ac421ec9bb28cea9f5faeb659740fcf00de6cc589d423fee7694c42d16
diff --git a/src/init.cpp b/src/init.cpp
@@ -50,6 +50,7 @@
 #include <txdb.h>
 #include <txmempool.h>
 #include <util/asmap.h>
+#include <util/check.h>
 #include <util/moneystr.h>
 #include <util/string.h>
 #include <util/system.h>
@@ -1378,9 +1379,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/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/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
@@ -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.
diff --git 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
@@ -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
@@ -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)
@@ -31,17 +32,16 @@ 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};
 }
 
 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.cpp b/src/test/util/setup_common.cpp
@@ -231,7 +231,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/util/setup_common.h b/src/test/util/setup_common.h
@@ -12,6 +12,7 @@
 #include <pubkey.h>
 #include <random.h>
 #include <txmempool.h>
+#include <util/check.h>
 #include <util/string.h>
 
 #include <type_traits>
diff --git 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/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.
  */
@@ -42,4 +42,18 @@ class NonFatalCheckError : public std::runtime_error
         }                                                         \
     } while (false)
 
+#if defined(NDEBUG)
+#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/src/validation.cpp b/src/validation.cpp
@@ -39,6 +39,7 @@
 #include <txmempool.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
 
diff --git 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});
 
diff --git 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

Original file line number	Diff line number	Diff line change
`@@ -75,7 +75,10 @@ CTxMemPool& EnsureMemPool(const util::Ref& context)`
`75`	`75`	`ChainstateManager& EnsureChainman(const util::Ref& context)`
`76`	`76`	`{`
`77`	`77`	`NodeContext& node = EnsureNodeContext(context);`
`78`		`- return EnsureChainman(node);`
	`78`	`+ if (!node.chainman) {`
	`79`	`+ throw JSONRPCError(RPC_INTERNAL_ERROR, "Node chainman not found");`
	`80`	`+ }`
	`81`	`+ return *node.chainman;`
`79`	`82`	`}`
`80`	`83`
`81`	`84`	`/* Calculate the difficulty for a given block index.`
Original file line number	Diff line number	Diff line change
`@@ -253,7 +253,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)`
`253`	`253`	`pblock->nNonce = blockinfo[i].nonce;`
`254`	`254`	`}`
`255`	`255`	`std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(*pblock);`
`256`		`- BOOST_CHECK(EnsureChainman(m_node).ProcessNewBlock(chainparams, shared_pblock, true, nullptr));`
	`256`	`+ BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlock(chainparams, shared_pblock, true, nullptr));`
`257`	`257`	`pblock->hashPrevBlock = pblock->GetHash();`
`258`	`258`	`}`
`259`	`259`