Merge #18727: test: Add CreateWalletFromFile test

7918c1b019 test: Add CreateWalletFromFile test (Russell Yanofsky)

Pull request description:

  Add unit test calling CreateWalletFromFile, which isn't currently called from other unit tests, with some basic checks to make sure it rescans and registers for notifications correctly.

  Motivation for this change was to try to write a test that would fail without the early `handleNotifications` call in ef8c6ca60767cac589d98ca57ee33179608ccda8 from https://github.com/bitcoin/bitcoin/pull/16426, but succeed with it:

  ef8c6ca607/src/wallet/wallet.cpp (L3978-L3986)

  However, writing a full test for the race condition that call prevents isn't possible without the locking changes from #16426. So this PR just adds as much test coverage as is possible now.

  This new test is also useful for https://github.com/bitcoin/bitcoin/pull/15719, since it detects the stale notifications.transactionAddedToMempool notifications that PR eliminates.

ACKs for top commit:
  MarcoFalke:
    ACK 7918c1b019
  jonatack:
    ACK 7918c1b019

Tree-SHA512: 44035aee698ecb722c6039d061d8fac2011e9da0b314e4aff19be1d610b53cacff99016b34d6b84669bb3b61041b2318d9d8e3363658f087802ae4aa36ca17b8

src/test/util/logging.cpp

@@ -11,13 +11,13 @@
 
 #include <stdexcept>
 
-DebugLogHelper::DebugLogHelper(std::string message)
-    : m_message{std::move(message)}
+DebugLogHelper::DebugLogHelper(std::string message, MatchFn match)
+    : m_message{std::move(message)}, m_match(std::move(match))
 {
     m_print_connection = LogInstance().PushBackCallback(
         [this](const std::string& s) {
             if (m_found) return;
-            m_found = s.find(m_message) != std::string::npos;
+            m_found = s.find(m_message) != std::string::npos && m_match(&s);
         });
     noui_test_redirect();
 }
@@ -26,7 +26,7 @@ void DebugLogHelper::check_found()
 {
     noui_reconnect();
     LogInstance().DeleteCallback(m_print_connection);
-    if (!m_found) {
+    if (!m_found && m_match(nullptr)) {
         throw std::runtime_error(strprintf("'%s' not found in debug log\n", m_message));
     }
 }

src/test/util/logging.h

@@ -17,10 +17,22 @@ class DebugLogHelper
     bool m_found{false};
     std::list<std::function<void(const std::string&)>>::iterator m_print_connection;
 
+    //! Custom match checking function.
+    //!
+    //! Invoked with pointers to lines containing matching strings, and with
+    //! null if check_found() is called without any successful match.
+    //!
+    //! Can return true to enable default DebugLogHelper behavior of:
+    //! (1) ending search after first successful match, and
+    //! (2) raising an error in check_found if no match was found
+    //! Can return false to do the opposite in either case.
+    using MatchFn = std::function<bool(const std::string* line)>;
+    MatchFn m_match;
+
     void check_found();
 
 public:
-    DebugLogHelper(std::string message);
+    DebugLogHelper(std::string message, MatchFn match = [](const std::string*){ return true; });
     ~DebugLogHelper() { check_found(); }
 };
 

src/wallet/test/wallet_tests.cpp

@@ -4,6 +4,7 @@
 
 #include <wallet/wallet.h>
 
+#include <future>
 #include <memory>
 #include <stdint.h>
 #include <vector>
@@ -12,6 +13,7 @@
 #include <node/context.h>
 #include <policy/policy.h>
 #include <rpc/server.h>
+#include <test/util/logging.h>
 #include <test/util/setup_common.h>
 #include <validation.h>
 #include <wallet/coincontrol.h>
@@ -26,6 +28,36 @@ extern UniValue importwallet(const JSONRPCRequest& request);
 
 BOOST_FIXTURE_TEST_SUITE(wallet_tests, WalletTestingSetup)
 
+static std::shared_ptr<CWallet> TestLoadWallet(interfaces::Chain& chain)
+{
+    std::string error;
+    std::vector<std::string> warnings;
+    auto wallet = CWallet::CreateWalletFromFile(chain, WalletLocation(""), error, warnings);
+    wallet->postInitProcess();
+    return wallet;
+}
+
+static void TestUnloadWallet(std::shared_ptr<CWallet>&& wallet)
+{
+    SyncWithValidationInterfaceQueue();
+    wallet->m_chain_notifications_handler.reset();
+    UnloadWallet(std::move(wallet));
+}
+
+static CMutableTransaction TestSimpleSpend(const CTransaction& from, uint32_t index, const CKey& key, const CScript& pubkey)
+{
+    CMutableTransaction mtx;
+    mtx.vout.push_back({from.vout[index].nValue - DEFAULT_TRANSACTION_MAXFEE, pubkey});
+    mtx.vin.push_back({CTxIn{from.GetHash(), index}});
+    FillableSigningProvider keystore;
+    keystore.AddKey(key);
+    std::map<COutPoint, Coin> coins;
+    coins[mtx.vin[0].prevout].out = from.vout[index];
+    std::map<int, std::string> input_errors;
+    BOOST_CHECK(SignTransaction(mtx, &keystore, coins, SIGHASH_ALL, input_errors));
+    return mtx;
+}
+
 static void AddKey(CWallet& wallet, const CKey& key)
 {
     auto spk_man = wallet.GetOrCreateLegacyScriptPubKeyMan();
@@ -658,4 +690,86 @@ BOOST_FIXTURE_TEST_CASE(wallet_descriptor_test, BasicTestingSetup)
     BOOST_CHECK_EXCEPTION(vr >> w_desc, std::ios_base::failure, malformed_descriptor);
 }
 
+//! Test CreateWalletFromFile function and its behavior handling potential race
+//! conditions if it's called the same time an incoming transaction shows up in
+//! the mempool or a new block.
+//!
+//! It isn't possible for a unit test to totally verify there aren't race
+//! conditions without hooking into the implementation more, so this test just
+//! verifies that new transactions are detected during loading without any
+//! notifications at all, to infer that timing of notifications shouldn't
+//! matter. The test could be extended to cover other scenarios in the future.
+BOOST_FIXTURE_TEST_CASE(CreateWalletFromFile, TestChain100Setup)
+{
+    // Create new wallet with known key and unload it.
+    auto chain = interfaces::MakeChain(m_node);
+    auto wallet = TestLoadWallet(*chain);
+    CKey key;
+    key.MakeNewKey(true);
+    AddKey(*wallet, key);
+    TestUnloadWallet(std::move(wallet));
+
+    // Add log hook to detect AddToWallet events from rescans, blockConnected,
+    // and transactionAddedToMempool notifications
+    int addtx_count = 0;
+    DebugLogHelper addtx_counter("[default wallet] AddToWallet", [&](const std::string* s) {
+        if (s) ++addtx_count;
+        return false;
+    });
+
+    bool rescan_completed = false;
+    DebugLogHelper rescan_check("[default wallet] Rescan completed", [&](const std::string* s) {
+        if (s) {
+            // For now, just assert that cs_main is being held during the
+            // rescan, ensuring that a new block couldn't be connected
+            // that the wallet would miss. After
+            // https://github.com/bitcoin/bitcoin/pull/16426 when cs_main is no
+            // longer held, the test can be extended to append a new block here
+            // and check it's handled correctly.
+            AssertLockHeld(::cs_main);
+            rescan_completed = true;
+        }
+        return false;
+    });
+
+    // Block the queue to prevent the wallet receiving blockConnected and
+    // transactionAddedToMempool notifications, and create block and mempool
+    // transactions paying to the wallet
+    std::promise<void> promise;
+    CallFunctionInValidationInterfaceQueue([&promise] {
+        promise.get_future().wait();
+    });
+    std::string error;
+    m_coinbase_txns.push_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
+    auto block_tx = TestSimpleSpend(*m_coinbase_txns[0], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
+    m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
+    auto mempool_tx = TestSimpleSpend(*m_coinbase_txns[1], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
+    BOOST_CHECK(chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error));
+
+    // Reload wallet and make sure new transactions are detected despite events
+    // being blocked
+    wallet = TestLoadWallet(*chain);
+    BOOST_CHECK(rescan_completed);
+    BOOST_CHECK_EQUAL(addtx_count, 2);
+    unsigned int block_tx_time, mempool_tx_time;
+    {
+        LOCK(wallet->cs_wallet);
+        block_tx_time = wallet->mapWallet.at(block_tx.GetHash()).nTimeReceived;
+        mempool_tx_time = wallet->mapWallet.at(mempool_tx.GetHash()).nTimeReceived;
+    }
+
+    // Unblock notification queue and make sure stale blockConnected and
+    // transactionAddedToMempool events are processed
+    promise.set_value();
+    SyncWithValidationInterfaceQueue();
+    BOOST_CHECK_EQUAL(addtx_count, 4);
+    {
+        LOCK(wallet->cs_wallet);
+        BOOST_CHECK_EQUAL(block_tx_time, wallet->mapWallet.at(block_tx.GetHash()).nTimeReceived);
+        BOOST_CHECK_EQUAL(mempool_tx_time, wallet->mapWallet.at(mempool_tx.GetHash()).nTimeReceived);
+    }
+
+    TestUnloadWallet(std::move(wallet));
+}
+
 BOOST_AUTO_TEST_SUITE_END()