Repository: bitcoin/bitcoin (language: cpp)

Issue:
util/check.h Assert/Assume: namespacing issues
The `Assert`/`Assume` macros are implemented via lambda functions to allow the result of the assertion to both be evaluated (and trigger an abort) and be returned without having the expression be evaluated twice.

This causes some ugly namespacing issues though. Because there's a function call, clang's thread safety annotations don't get passed through (as the lambda is unannotated), possibly causing an unnecessary compiler error because the compiler loses track that a mutex is held when a guarded variable is accessed.

It also seems that gcc (but not clang) gets confused about member functions (but not member variables), eg:

```c++
class TestAssert
{
public:
    int variable = 3;
    int test_1(void) { return variable; }
    int test_2(void) {
        auto x = [&]() {
            Assert(test_1() == 3);
        };
        x();
        return ++variable;
    }
};
```

results in:

```
test/util_tests.cpp:91:26: error: cannot call member function ‘int util_tests::TestAssert::test_1()’ without object
   91 |             Assert(test_1() == 3);
```

requiring you to write `Assert(this->test_1() == 3)` instead.


Discussion / hints:
Changing the `Assert` macro to something like:

```c++
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();
}

template <typename T>
inline T inline_assertion_check(T&& val, const char* file, int line, const char* func, const char* assertion)
{
    if (!val) assertion_fail(file, line, func, assertion);
    return std::forward<T>(val);
}

#define Assert(val) inline_assertion_check(val, __FILE__, __LINE__, __func__, #val)
```

might work to avoid this -- that way `val` is evaluated in the original context where all the locks and this pointers are already known about. I'm not sure if this handles all C++'s various copy/move rules as well as the current Assert does though. It also gives up on re-using `assert()` and just manually reports file/line/function.
@ajtowns I think the return type should be `T&&` for the forwarding to work.
Related: #21596
@sipa I can't convince myself that returning `T&&` wouldn't mess up lifetimes somehow and return a dangling reference... Trying it out, it seems like `T` as the return-type might invoke the move constructor while `T&&` might not, so what you say at least seems plausible. I think the current approach (with a lambda) also invokes the move constructor; though destruction happens in a different order...
@ajtowns I'm happy to reason through a specific case you're concerned about if you have one. But perhaps this helps: `std::forward<T>` has as return type also `T&&` (note that (A&)&& = A&, so if T=A&, the return type is A&, not A&&).
@sipa I think my concern is that `f(1+2) + 4` might be interpreted as "pass 3 to f, then destruct 3, then add 4 to whatever  f returned" which would cause a problem if f were returning the 3 object that was just destructed. But I think "[All temporary objects are destroyed as the last step in evaluating the full-expression that (lexically) contains the point where they were created,](https://en.cppreference.com/w/cpp/language/lifetime)" means that's not allowed?
@ajtowns So I think the only concern in that regard is something like:

```c++
auto&& x = f(1+2);
return x + 4;
```

where the `x` is a dangling reference to the temporary created by `1 + 2`.

However, that's just as dangerous as doing the same with `auto&& x = std::forward<int>(1+2)`, e.g.

Fix patch:
diff --git a/src/Makefile.am b/src/Makefile.am
index af6bcaf4d797f..a9eac7a51ca07 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -621,6 +621,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 \
@@ -839,6 +840,7 @@ bitcoin_chainstate_SOURCES = \
   uint256.cpp \
   util/asmap.cpp \
   util/bytevectorhash.cpp \
+  util/check.cpp \
   util/getuniquepath.cpp \
   util/hasher.cpp \
   util/moneystr.cpp \
diff --git a/src/util/check.cpp b/src/util/check.cpp
new file mode 100644
index 0000000000000..2a9f885560d02
--- /dev/null
+++ b/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();
+}
diff --git a/src/util/check.h b/src/util/check.h
index a443c13cf2386..80e973e7e293d 100644
--- a/src/util/check.h
+++ b/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


Test patch:
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 1881573e7a1db..b5d8411e1d7a7 100644
--- a/src/test/util_tests.cpp
+++ b/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)
diff --git a/src/wallet/test/wallet_test_fixture.cpp b/src/wallet/test/wallet_test_fixture.cpp
index cb006dea3a324..38d23b6f91080 100644
--- a/src/wallet/test/wallet_test_fixture.cpp
+++ b/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();
diff --git a/src/wallet/test/wallet_test_fixture.h b/src/wallet/test/wallet_test_fixture.h
index d4b855b145d64..e0b38a40e7453 100644
--- a/src/wallet/test/wallet_test_fixture.h
+++ b/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;
 };