Merge bitcoin/bitcoin#33032: wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase

037ea2c714 walletdb: Remove m_mock from SQLiteDatabase (Ava Chow)
59484e2fdb wallet: Make Mockable{Database,Batch} subclasses of SQLite classes (Ava Chow)
b69f989dc5 wallet, bench: Use TestingSetup in CoinSelection benchmark (Ava Chow)
e7d67c9fd9 test: Make duplicating MockableDatabases use cursor and batch (Ava Chow)
964eafb71c bench, wallet: Make WalletMigration's setup WalletBatch scoped (Ava Chow)

Pull request description:

  `MockableDatabase` was introduced for the tests to avoid tying non-database tests to a particular database type. However, since the only database type now is sqlite, and because the mockable behavior is no longer used by the tests, we can replace usage of the `MockabeDatabase` with a SQLite database that lives only in memory.

  This is particularly useful for future work that has the wallet make use of SQLite's capabilities more, which are less conducive to having a separate mock database implementation.

ACKs for top commit:
  brunoerg:
    code review ACK 037ea2c714
  sedited:
    Re-ACK 037ea2c714
  furszy:
    Code review ACK 037ea2c714

Tree-SHA512: 0a99c27ef4e590966b3af929bf3acf99666861905aabf150fe5660ea07c881a49935a4e7dcd676dcd5e70616898d89d872b6e156ae9c600de1361c1b2469b64d

src/bench/coin_selection.cpp

@@ -12,8 +12,10 @@
 #include <primitives/transaction.h>
 #include <random.h>
 #include <sync.h>
+#include <test/util/setup_common.h>
 #include <util/result.h>
 #include <wallet/coinselection.h>
+#include <wallet/context.h>
 #include <wallet/spend.h>
 #include <wallet/test/util.h>
 #include <wallet/transaction.h>
@@ -26,19 +28,7 @@
 #include <utility>
 #include <vector>
 
-using node::NodeContext;
-using wallet::AttemptSelection;
-using wallet::CHANGE_LOWER;
-using wallet::COutput;
-using wallet::CWallet;
-using wallet::CWalletTx;
-using wallet::CoinEligibilityFilter;
-using wallet::CoinSelectionParams;
-using wallet::CreateMockableWalletDatabase;
-using wallet::OutputGroup;
-using wallet::SelectCoinsBnB;
-using wallet::TxStateInactive;
-
+namespace wallet {
 static void addCoin(const CAmount& nValue, const CWallet& wallet, std::vector<std::unique_ptr<CWalletTx>>& wtxs)
 {
     static int nextLockTime = 0;
@@ -58,9 +48,8 @@ static void addCoin(const CAmount& nValue, const CWallet& wallet, std::vector<st
 // (https://github.com/bitcoin/bitcoin/issues/7883#issuecomment-224807484)
 static void CoinSelection(benchmark::Bench& bench)
 {
-    NodeContext node;
-    auto chain = interfaces::MakeChain(node);
-    CWallet wallet(chain.get(), "", CreateMockableWalletDatabase());
+    const auto test_setup = MakeNoLogFileContext<TestingSetup>();
+    CWallet wallet(test_setup->m_node.chain.get(), "", CreateMockableWalletDatabase());
     std::vector<std::unique_ptr<CWalletTx>> wtxs;
     LOCK(wallet.cs_wallet);
 
@@ -136,3 +125,4 @@ static void BnBExhaustion(benchmark::Bench& bench)
 
 BENCHMARK(CoinSelection);
 BENCHMARK(BnBExhaustion);
+}; // namespace wallet

src/bench/wallet_migration.cpp

@@ -29,40 +29,42 @@ static void WalletMigration(benchmark::Bench& bench)
 
     // Setup legacy wallet
     std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(test_setup->m_node.chain.get(), "", CreateMockableWalletDatabase());
-    LegacyDataSPKM* legacy_spkm = wallet->GetOrCreateLegacyDataSPKM();
-    WalletBatch batch{wallet->GetDatabase()};
+    {
+        LegacyDataSPKM* legacy_spkm = wallet->GetOrCreateLegacyDataSPKM();
+        WalletBatch batch{wallet->GetDatabase()};
 
-    // Write a best block record as migration expects one to exist
-    CBlockLocator loc;
-    batch.WriteBestBlock(loc);
+        // Write a best block record as migration expects one to exist
+        CBlockLocator loc;
+        batch.WriteBestBlock(loc);
 
-    // Add watch-only addresses
-    std::vector<CScript> scripts_watch_only;
-    for (int w = 0; w < NUM_WATCH_ONLY_ADDR; ++w) {
-        CKey key = GenerateRandomKey();
-        LOCK(wallet->cs_wallet);
-        const PKHash dest{key.GetPubKey()};
-        const CScript& script = scripts_watch_only.emplace_back(GetScriptForDestination(dest));
-        assert(legacy_spkm->LoadWatchOnly(script));
-        assert(wallet->SetAddressBook(dest, strprintf("watch_%d", w), /*purpose=*/std::nullopt));
-        batch.WriteWatchOnly(script, CKeyMetadata());
-    }
+        // Add watch-only addresses
+        std::vector<CScript> scripts_watch_only;
+        for (int w = 0; w < NUM_WATCH_ONLY_ADDR; ++w) {
+            CKey key = GenerateRandomKey();
+            LOCK(wallet->cs_wallet);
+            const PKHash dest{key.GetPubKey()};
+            const CScript& script = scripts_watch_only.emplace_back(GetScriptForDestination(dest));
+            assert(legacy_spkm->LoadWatchOnly(script));
+            assert(wallet->SetAddressBook(dest, strprintf("watch_%d", w), /*purpose=*/std::nullopt));
+            batch.WriteWatchOnly(script, CKeyMetadata());
+        }
 
-    // Generate transactions and local addresses
-    for (int j = 0; j < 500; ++j) {
-        CKey key = GenerateRandomKey();
-        CPubKey pubkey = key.GetPubKey();
-        // Load key, scripts and create address book record
-        Assert(legacy_spkm->LoadKey(key, pubkey));
-        CTxDestination dest{PKHash(pubkey)};
-        Assert(wallet->SetAddressBook(dest, strprintf("legacy_%d", j), /*purpose=*/std::nullopt));
+        // Generate transactions and local addresses
+        for (int j = 0; j < 500; ++j) {
+            CKey key = GenerateRandomKey();
+            CPubKey pubkey = key.GetPubKey();
+            // Load key, scripts and create address book record
+            Assert(legacy_spkm->LoadKey(key, pubkey));
+            CTxDestination dest{PKHash(pubkey)};
+            Assert(wallet->SetAddressBook(dest, strprintf("legacy_%d", j), /*purpose=*/std::nullopt));
 
-        CMutableTransaction mtx;
-        mtx.vout.emplace_back(COIN, GetScriptForDestination(dest));
-        mtx.vout.emplace_back(COIN, scripts_watch_only.at(j % NUM_WATCH_ONLY_ADDR));
-        mtx.vin.resize(2);
-        wallet->AddToWallet(MakeTransactionRef(mtx), TxStateInactive{}, /*update_wtx=*/nullptr, /*rescanning_old_block=*/true);
-        batch.WriteKey(pubkey, key.GetPrivKey(), CKeyMetadata());
+            CMutableTransaction mtx;
+            mtx.vout.emplace_back(COIN, GetScriptForDestination(dest));
+            mtx.vout.emplace_back(COIN, scripts_watch_only.at(j % NUM_WATCH_ONLY_ADDR));
+            mtx.vin.resize(2);
+            wallet->AddToWallet(MakeTransactionRef(mtx), TxStateInactive{}, /*update_wtx=*/nullptr, /*rescanning_old_block=*/true);
+            batch.WriteKey(pubkey, key.GetPrivKey(), CKeyMetadata());
+        }
     }
 
     bench.epochs(/*numEpochs=*/1).epochIterations(/*numIters=*/1) // run the migration exactly once

src/test/util/CMakeLists.txt

@@ -24,6 +24,7 @@ target_link_libraries(test_util
   PRIVATE
     core_interface
     Boost::headers
+    $<$<BOOL:${ENABLE_WALLET}>:SQLite3::SQLite3>
   PUBLIC
     univalue
 )

src/wallet/sqlite.cpp

@@ -111,8 +111,12 @@ static void SetPragma(sqlite3* db, const std::string& key, const std::string& va
 Mutex SQLiteDatabase::g_sqlite_mutex;
 int SQLiteDatabase::g_sqlite_count = 0;
 
-SQLiteDatabase::SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, bool mock)
-    : WalletDatabase(), m_mock(mock), m_dir_path(dir_path), m_file_path(fs::PathToString(file_path)), m_write_semaphore(1), m_use_unsafe_sync(options.use_unsafe_sync)
+SQLiteDatabase::SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options)
+    : SQLiteDatabase(dir_path, file_path, options, /*additional_flags=*/0)
+{}
+
+SQLiteDatabase::SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, int additional_flags)
+    : WalletDatabase(), m_dir_path(dir_path), m_file_path(fs::PathToString(file_path)), m_write_semaphore(1), m_use_unsafe_sync(options.use_unsafe_sync)
 {
     {
         LOCK(g_sqlite_mutex);
@@ -135,7 +139,7 @@ SQLiteDatabase::SQLiteDatabase(const fs::path& dir_path, const fs::path& file_pa
     }
 
     try {
-        Open();
+        Open(additional_flags);
     } catch (const std::runtime_error&) {
         // If open fails, cleanup this object and rethrow the exception
         Cleanup();
@@ -243,13 +247,15 @@ bool SQLiteDatabase::Verify(bilingual_str& error)
 
 void SQLiteDatabase::Open()
 {
-    int flags = SQLITE_OPEN_FULLMUTEX | SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE;
-    if (m_mock) {
-        flags |= SQLITE_OPEN_MEMORY; // In memory database for mock db
-    }
+    Open(/*additional_flags*/0);
+}
+
+void SQLiteDatabase::Open(int additional_flags)
+{
+    int flags = SQLITE_OPEN_FULLMUTEX | SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE | additional_flags;
 
     if (m_db == nullptr) {
-        if (!m_mock) {
+        if (!(flags & SQLITE_OPEN_MEMORY)) {
             TryCreateDirectories(m_dir_path);
         }
         int ret = sqlite3_open_v2(m_file_path.c_str(), &m_db, flags, nullptr);

src/wallet/sqlite.h

@@ -75,6 +75,7 @@ private:
     void SetupSQLStatements();
     bool ExecStatement(sqlite3_stmt* stmt, std::span<const std::byte> blob);
 
+protected:
     bool ReadKey(DataStream&& key, DataStream& value) override;
     bool WriteKey(DataStream&& key, DataStream&& value, bool overwrite = true) override;
     bool EraseKey(DataStream&& key) override;
@@ -102,8 +103,6 @@ public:
 class SQLiteDatabase : public WalletDatabase
 {
 private:
-    const bool m_mock{false};
-
     const fs::path m_dir_path;
 
     const std::string m_file_path;
@@ -119,11 +118,16 @@ private:
 
     void Cleanup() noexcept EXCLUSIVE_LOCKS_REQUIRED(!g_sqlite_mutex);
 
+    void Open(int additional_flags);
+
+protected:
+    SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, int additional_flags);
+
 public:
     SQLiteDatabase() = delete;
 
     /** Create DB handle to real database */
-    SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, bool mock = false);
+    SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options);
 
     ~SQLiteDatabase();
 

src/wallet/test/util.cpp

@@ -14,6 +14,8 @@
 #include <wallet/wallet.h>
 #include <wallet/walletdb.h>
 
+#include <sqlite3.h>
+
 #include <memory>
 
 namespace wallet {
@@ -105,7 +107,23 @@ void TestUnloadWallet(std::shared_ptr<CWallet>&& wallet)
 
 std::unique_ptr<WalletDatabase> DuplicateMockDatabase(WalletDatabase& database)
 {
-    return std::make_unique<MockableDatabase>(dynamic_cast<MockableDatabase&>(database).m_records);
+    std::unique_ptr<DatabaseBatch> batch_orig = database.MakeBatch();
+    std::unique_ptr<DatabaseCursor> cursor_orig = batch_orig->GetNewCursor();
+
+    std::unique_ptr<WalletDatabase> new_db = CreateMockableWalletDatabase();
+    std::unique_ptr<DatabaseBatch> new_db_batch = new_db->MakeBatch();
+    MockableSQLiteBatch* batch_new = dynamic_cast<MockableSQLiteBatch*>(new_db_batch.get());
+    Assert(batch_new);
+
+    while (true) {
+        DataStream key, value;
+        DatabaseCursor::Status status = cursor_orig->Next(key, value);
+        Assert(status != DatabaseCursor::Status::FAIL);
+        if (status != DatabaseCursor::Status::MORE) break;
+        batch_new->WriteKey(std::move(key), std::move(value));
+    }
+
+    return new_db;
 }
 
 std::string getnewaddress(CWallet& w)
@@ -119,103 +137,13 @@ CTxDestination getNewDestination(CWallet& w, OutputType output_type)
     return *Assert(w.GetNewDestination(output_type, ""));
 }
 
-MockableCursor::MockableCursor(const MockableData& records, bool pass, std::span<const std::byte> prefix)
-{
-    m_pass = pass;
-    std::tie(m_cursor, m_cursor_end) = records.equal_range(BytePrefix{prefix});
-}
+MockableSQLiteDatabase::MockableSQLiteDatabase()
+    : SQLiteDatabase(fs::PathFromString("mock/"), fs::PathFromString("mock/wallet.dat"), DatabaseOptions(), SQLITE_OPEN_MEMORY)
+{}
 
-DatabaseCursor::Status MockableCursor::Next(DataStream& key, DataStream& value)
+std::unique_ptr<WalletDatabase> CreateMockableWalletDatabase()
 {
-    if (!m_pass) {
-        return Status::FAIL;
-    }
-    if (m_cursor == m_cursor_end) {
-        return Status::DONE;
-    }
-    key.clear();
-    value.clear();
-    const auto& [key_data, value_data] = *m_cursor;
-    key.write(key_data);
-    value.write(value_data);
-    m_cursor++;
-    return Status::MORE;
-}
-
-bool MockableBatch::ReadKey(DataStream&& key, DataStream& value)
-{
-    if (!m_pass) {
-        return false;
-    }
-    SerializeData key_data{key.begin(), key.end()};
-    const auto& it = m_records.find(key_data);
-    if (it == m_records.end()) {
-        return false;
-    }
-    value.clear();
-    value.write(it->second);
-    return true;
-}
-
-bool MockableBatch::WriteKey(DataStream&& key, DataStream&& value, bool overwrite)
-{
-    if (!m_pass) {
-        return false;
-    }
-    SerializeData key_data{key.begin(), key.end()};
-    SerializeData value_data{value.begin(), value.end()};
-    auto [it, inserted] = m_records.emplace(key_data, value_data);
-    if (!inserted && overwrite) { // Overwrite if requested
-        it->second = value_data;
-        inserted = true;
-    }
-    return inserted;
-}
-
-bool MockableBatch::EraseKey(DataStream&& key)
-{
-    if (!m_pass) {
-        return false;
-    }
-    SerializeData key_data{key.begin(), key.end()};
-    m_records.erase(key_data);
-    return true;
-}
-
-bool MockableBatch::HasKey(DataStream&& key)
-{
-    if (!m_pass) {
-        return false;
-    }
-    SerializeData key_data{key.begin(), key.end()};
-    return m_records.contains(key_data);
-}
-
-bool MockableBatch::ErasePrefix(std::span<const std::byte> prefix)
-{
-    if (!m_pass) {
-        return false;
-    }
-    auto it = m_records.begin();
-    while (it != m_records.end()) {
-        auto& key = it->first;
-        if (key.size() < prefix.size() || std::search(key.begin(), key.end(), prefix.begin(), prefix.end()) != key.begin()) {
-            it++;
-            continue;
-        }
-        it = m_records.erase(it);
-    }
-    return true;
-}
-
-std::unique_ptr<WalletDatabase> CreateMockableWalletDatabase(MockableData records)
-{
-    return std::make_unique<MockableDatabase>(records);
-}
-
-MockableDatabase& GetMockableDatabase(CWallet& wallet)
-{
-    return dynamic_cast<MockableDatabase&>(wallet.GetDatabase());
+    return std::make_unique<MockableSQLiteDatabase>();
 }
 
 wallet::DescriptorScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string& desc_str, const bool success)

src/wallet/test/util.h

@@ -8,6 +8,7 @@
 #include <addresstype.h>
 #include <wallet/db.h>
 #include <wallet/scriptpubkeyman.h>
+#include <wallet/sqlite.h>
 
 #include <memory>
 
@@ -48,76 +49,31 @@ CTxDestination getNewDestination(CWallet& w, OutputType output_type);
 
 using MockableData = std::map<SerializeData, SerializeData, std::less<>>;
 
-class MockableCursor: public DatabaseCursor
+
+class MockableSQLiteBatch : public SQLiteBatch
 {
 public:
-    MockableData::const_iterator m_cursor;
-    MockableData::const_iterator m_cursor_end;
-    bool m_pass;
-
-    explicit MockableCursor(const MockableData& records, bool pass) : m_cursor(records.begin()), m_cursor_end(records.end()), m_pass(pass) {}
-    MockableCursor(const MockableData& records, bool pass, std::span<const std::byte> prefix);
-    ~MockableCursor() = default;
-
-    Status Next(DataStream& key, DataStream& value) override;
-};
-
-class MockableBatch : public DatabaseBatch
-{
-private:
-    MockableData& m_records;
-    bool m_pass;
-
-    bool ReadKey(DataStream&& key, DataStream& value) override;
-    bool WriteKey(DataStream&& key, DataStream&& value, bool overwrite=true) override;
-    bool EraseKey(DataStream&& key) override;
-    bool HasKey(DataStream&& key) override;
-    bool ErasePrefix(std::span<const std::byte> prefix) override;
-
-public:
-    explicit MockableBatch(MockableData& records, bool pass) : m_records(records), m_pass(pass) {}
-    ~MockableBatch() = default;
-
-    void Close() override {}
-
-    std::unique_ptr<DatabaseCursor> GetNewCursor() override
-    {
-        return std::make_unique<MockableCursor>(m_records, m_pass);
-    }
-    std::unique_ptr<DatabaseCursor> GetNewPrefixCursor(std::span<const std::byte> prefix) override {
-        return std::make_unique<MockableCursor>(m_records, m_pass, prefix);
-    }
-    bool TxnBegin() override { return m_pass; }
-    bool TxnCommit() override { return m_pass; }
-    bool TxnAbort() override { return m_pass; }
-    bool HasActiveTxn() override { return false; }
+    using SQLiteBatch::SQLiteBatch;
+    using SQLiteBatch::WriteKey;
 };
 
 /** A WalletDatabase whose contents and return values can be modified as needed for testing
  **/
-class MockableDatabase : public WalletDatabase
+class MockableSQLiteDatabase : public SQLiteDatabase
 {
 public:
-    MockableData m_records;
-    bool m_pass{true};
+    MockableSQLiteDatabase();
 
-    MockableDatabase(MockableData records = {}) : WalletDatabase(), m_records(records) {}
-    ~MockableDatabase() = default;
-
-    void Open() override {}
-
-    bool Rewrite() override { return m_pass; }
-    bool Backup(const std::string& strDest) const override { return m_pass; }
-    void Close() override {}
+    bool Backup(const std::string& strDest) const override { return true; }
 
     std::string Filename() override { return "mockable"; }
     std::vector<fs::path> Files() override { return {}; }
-    std::string Format() override { return "mock"; }
-    std::unique_ptr<DatabaseBatch> MakeBatch() override { return std::make_unique<MockableBatch>(m_records, m_pass); }
+    std::string Format() override { return "sqlite-mock"; }
+    std::unique_ptr<DatabaseBatch> MakeBatch() override { return std::make_unique<MockableSQLiteBatch>(*this); }
 };
 
-std::unique_ptr<WalletDatabase> CreateMockableWalletDatabase(MockableData records = {});
-MockableDatabase& GetMockableDatabase(CWallet& wallet);
+std::unique_ptr<WalletDatabase> CreateMockableWalletDatabase();
+MockableSQLiteDatabase& GetMockableDatabase(CWallet& wallet);
 
 DescriptorScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string& desc_str, bool success);
 } // namespace wallet

src/wallet/wallet.cpp

@@ -3521,7 +3521,7 @@ void CWallet::SetupLegacyScriptPubKeyMan()
         return;
     }
 
-    Assert(m_database->Format() == "bdb_ro" || m_database->Format() == "mock");
+    Assert(m_database->Format() == "bdb_ro" || m_database->Format() == "sqlite-mock");
     std::unique_ptr<ScriptPubKeyMan> spk_manager = std::make_unique<LegacyDataSPKM>(*this);
 
     for (const auto& type : LEGACY_OUTPUT_TYPES) {