Merge bitcoin/bitcoin#24714: util/check: Don't use a lambda for Assert/Assume

2ef47ba util/check: stop using lambda for Assert/Assume (Anthony Towns)
7c9fe25 wallet: move Assert() check into constructor (Anthony Towns)

Pull request description:

  Using a lambda creates a couple of odd namespacing issues, in particular making clang's thread safety analysis less helpful, and confusing gcc when calling member functions. Fix this by not using a lambda.

  Fixes #21596
  Fixes #24654

ACKs for top commit:
  MarcoFalke:
    ACK 2ef47ba 🚢
  jonatack:
    Tested re-ACK 2ef47ba

Tree-SHA512: 4bdbf3215f3d14472df0552362c5eebe8b7eea2d0928a8a41109edd4e0c5f95de6f8220eb2fee8506874e352c003907faf5ef344174795939306a618157b1bae

Author: MarcoFalke

src/Makefile.am

@@ -620,6 +620,7 @@ libbitcoin_util_a_SOURCES = \
   util/asmap.cpp \
   util/bip32.cpp \
   util/bytevectorhash.cpp \
+  util/check.cpp \
   util/error.cpp \
   util/fees.cpp \
   util/getuniquepath.cpp \
@@ -838,6 +839,7 @@ bitcoin_chainstate_SOURCES = \
   uint256.cpp \
   util/asmap.cpp \
   util/bytevectorhash.cpp \
+  util/check.cpp \
   util/getuniquepath.cpp \
   util/hasher.cpp \
   util/moneystr.cpp \

src/test/util_tests.cpp

@@ -78,6 +78,31 @@ BOOST_AUTO_TEST_CASE(util_datadir)
     BOOST_CHECK_EQUAL(dd_norm, args.GetDataDirBase());
 }
 
+namespace {
+class NoCopyOrMove
+{
+public:
+    int i;
+    explicit NoCopyOrMove(int i) : i{i} { }
+
+    NoCopyOrMove() = delete;
+    NoCopyOrMove(const NoCopyOrMove&) = delete;
+    NoCopyOrMove(NoCopyOrMove&&) = delete;
+    NoCopyOrMove& operator=(const NoCopyOrMove&) = delete;
+    NoCopyOrMove& operator=(NoCopyOrMove&&) = delete;
+
+    operator bool() const { return i != 0; }
+
+    int get_ip1() { return i + 1; }
+    bool test()
+    {
+        // Check that Assume can be used within a lambda and still call methods
+        [&]() { Assume(get_ip1()); }();
+        return Assume(get_ip1() != 5);
+    }
+};
+} // namespace
+
 BOOST_AUTO_TEST_CASE(util_check)
 {
     // Check that Assert can forward
@@ -89,6 +114,14 @@ BOOST_AUTO_TEST_CASE(util_check)
     // Check that Assume can be used as unary expression
     const bool result{Assume(two == 2)};
     Assert(result);
+
+    // Check that Assert doesn't require copy/move
+    NoCopyOrMove x{9};
+    Assert(x).i += 3;
+    Assert(x).test();
+
+    // Check nested Asserts
+    BOOST_CHECK_EQUAL(Assert((Assert(x).test() ? 3 : 0)), 3);
 }
 
 BOOST_AUTO_TEST_CASE(util_criticalsection)

src/util/check.cpp

@@ -0,0 +1,14 @@
+// Copyright (c) 2022 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <util/check.h>
+
+#include <tinyformat.h>
+
+void assertion_fail(const char* file, int line, const char* func, const char* assertion)
+{
+    auto str = strprintf("%s:%s %s: Assertion `%s' failed.\n", file, line, func, assertion);
+    fwrite(str.data(), 1, str.size(), stderr);
+    std::abort();
+}

src/util/check.h

@@ -47,14 +47,26 @@ class NonFatalCheckError : public std::runtime_error
 #endif
 
 /** Helper for Assert() */
-template <typename T>
-T get_pure_r_value(T&& val)
+void assertion_fail(const char* file, int line, const char* func, const char* assertion);
+
+/** Helper for Assert()/Assume() */
+template <bool IS_ASSERT, typename T>
+T&& inline_assertion_check(T&& val, const char* file, int line, const char* func, const char* assertion)
 {
+    if constexpr (IS_ASSERT
+#ifdef ABORT_ON_FAILED_ASSUME
+                  || true
+#endif
+    ) {
+        if (!val) {
+            assertion_fail(file, line, func, assertion);
+        }
+    }
     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 std::forward<decltype(get_pure_r_value(val))>(check); }())
+#define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
 
 /**
  * Assume is the identity function.
@@ -66,10 +78,6 @@ T get_pure_r_value(T&& val)
  * - For non-fatal errors in interactive sessions (e.g. RPC or command line
  *   interfaces), CHECK_NONFATAL() might be more appropriate.
  */
-#ifdef ABORT_ON_FAILED_ASSUME
-#define Assume(val) Assert(val)
-#else
-#define Assume(val) ([&]() -> decltype(get_pure_r_value(val)) { auto&& check = (val); return std::forward<decltype(get_pure_r_value(val))>(check); }())
-#endif
+#define Assume(val) inline_assertion_check<false>(val, __FILE__, __LINE__, __func__, #val)
 
 #endif // BITCOIN_UTIL_CHECK_H

src/wallet/test/wallet_test_fixture.cpp

@@ -9,6 +9,7 @@
 namespace wallet {
 WalletTestingSetup::WalletTestingSetup(const std::string& chainName)
     : TestingSetup(chainName),
+      m_wallet_loader{interfaces::MakeWalletLoader(*m_node.chain, *Assert(m_node.args))},
       m_wallet(m_node.chain.get(), "", m_args, CreateMockWalletDatabase())
 {
     m_wallet.LoadWallet();

src/wallet/test/wallet_test_fixture.h

@@ -22,7 +22,7 @@ struct WalletTestingSetup : public TestingSetup {
     explicit WalletTestingSetup(const std::string& chainName = CBaseChainParams::MAIN);
     ~WalletTestingSetup();
 
-    std::unique_ptr<interfaces::WalletLoader> m_wallet_loader = interfaces::MakeWalletLoader(*m_node.chain, *Assert(m_node.args));
+    std::unique_ptr<interfaces::WalletLoader> m_wallet_loader;
     CWallet m_wallet;
     std::unique_ptr<interfaces::Handler> m_chain_notifications_handler;
 };