Introduce `MockableDatabase` for wallet unit tests

achow101 commented at 6:28 pm on December 16, 2022: member

For the wallet’s unit tests, we currently use either DummyDatabase or memory-only versions of either BDB or SQLite. The tests that use DummyDatabase just need a WalletDatabase so that the CWallet can be constructed, while the tests using the memory-only databases just need a backing data store. There is also a FailDatabase that is similar to DummyDatabase except it fails be default or can have a configured return value. Having all of these different database types can make it difficult to write tests, particularly tests that work when either BDB or SQLite is disabled.

This PR unifies all of these different unit test database classes into a single MockableDatabase. Like DummyDatabase, most functions do nothing and just return true. Like FailDatabase, the return value of some functions can be configured on the fly to test various failure cases. Like the memory-only databases, records can actually be written to the MockableDatabase and be retrieved later, but all of this is still held in memory. Using MockableDatabase completely removes the need for having BDB or SQLite backed wallets in the unit tests for the tests that are not actually testing specific database behaviors.

Because MockableDatabases can be created by each unit test, we can also control what records are stored in the database. Records can be added and removed externally from the typical database modification functions. This will give us greater ability to test failure conditions, particularly those involving corrupted records.

Possible alternative to #26644

DrahtBot commented at 6:28 pm on December 16, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/723 (Add warnings for non-active addresses in receive tab and address book by pinheadmz)
#27556 (wallet: fix deadlock in bdb read write operation by furszy)
#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)
#26627 (wallet: Migrate non-HD keys with single combo containing a list of keys by achow101)
#26008 (wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets by achow101)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25297 (wallet: group independent db writes on single batched db transaction by furszy)
#24914 (wallet: Load database records in a particular order by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

achow101 force-pushed on Jan 7, 2023

achow101 force-pushed on Jan 23, 2023

achow101 marked this as ready for review on Jan 23, 2023

achow101 force-pushed on Jan 25, 2023

DrahtBot added the label Needs rebase on Jan 26, 2023

achow101 force-pushed on Jan 26, 2023

DrahtBot removed the label Needs rebase on Jan 26, 2023

achow101 force-pushed on Jan 26, 2023

DrahtBot added the label Needs rebase on Feb 17, 2023

achow101 force-pushed on Feb 17, 2023

DrahtBot removed the label Needs rebase on Feb 17, 2023

in src/wallet/salvage.h:45 in 21a916f09e outdated

40+    bool TxnAbort() override { return true; }
41+};
42+
43+/** A dummy WalletDatabase that does nothing and never fails. Only used by salvage.
44+ **/
45+class DummyDatabase : public WalletDatabase

TheCharlatan commented at 11:22 am on March 2, 2023:

Couldn’t this class definition be moved out of the header?

achow101 commented at 6:17 pm on March 15, 2023:

Done

TheCharlatan commented at 6:50 pm on March 15, 2023:

ACK, this is resolved.

in src/wallet/salvage.cpp:174 in 21a916f09e outdated

166@@ -167,4 +167,10 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil
167 
168     return fSuccess;
169 }
170+
171+/** Return object for accessing dummy database with no read/write capabilities. */
172+std::unique_ptr<WalletDatabase> CreateDummyWalletDatabase()
173+{
174+    return std::make_unique<DummyDatabase>();

TheCharlatan commented at 11:23 am on March 2, 2023:

Nit: Couldn’t this just be used directly at its single call site?

achow101 commented at 6:17 pm on March 15, 2023:

Done

TheCharlatan commented at 6:50 pm on March 15, 2023:

ACK, this is resolved.

TheCharlatan commented at 11:26 am on March 2, 2023: contributor

Approach ACK

Just some minor comments; I like this change. I did not check yet if there are other tests that might be simpler with the new mockable class.

achow101 force-pushed on Mar 15, 2023

furszy commented at 10:46 pm on March 17, 2023: member

Concept ACK, great idea to make db independent tests.

in src/wallet/test/util.cpp:124 in 7152972a9d outdated

119+    if (overwrite) {
120+        m_records[key_data] = value_data;
121+        return true;
122+    }
123+    auto res = m_records.emplace(key_data, value_data);
124+    return res.second;

furszy commented at 11:23 pm on March 17, 2023:

tiny nit, Could:

0auto [it, inserted] = m_records.emplace(key_data, value_data);
1if (!inserted && overwrite) { // overwrite if requested
2    it->second = value_data;
3    inserted = true;
4}
5return inserted;

achow101 commented at 3:55 pm on April 10, 2023:

Done as suggested

in src/wallet/test/wallet_tests.cpp:963 in 6d2fa15f53 outdated

959@@ -1008,7 +960,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_sync_tx_invalid_state_test, TestingSetup)
960     // 1) Make db always fail
961     // 2) Try to add a transaction that spends the previously created transaction and
962     //    verify that we are not moving forward if the wallet cannot store it
963-    static_cast<FailDatabase&>(wallet.GetDatabase()).m_pass = false;
964+    static_cast<MockableDatabase&>(wallet.GetDatabase()).m_pass = false;

furszy commented at 11:33 pm on March 17, 2023:

nit: could use the previously introduced function.

0GetMockableDatabase(wallet).m_pass = false;

achow101 commented at 3:55 pm on April 10, 2023:

Done as suggested

in src/wallet/salvage.h:11 in b3be4ccbe0 outdated

 7@@ -8,6 +8,7 @@
 8 
 9 #include <fs.h>
10 #include <streams.h>
11+#include <wallet/db.h>

furszy commented at 11:49 pm on March 17, 2023:

this include doesn’t seems to be needed here.

achow101 commented at 3:55 pm on April 10, 2023:

Removed

in src/wallet/test/util.cpp:51 in bdec0057a5 outdated

66-        if (status == DatabaseCursor::Status::DONE) break;
67-        new_batch->Write(key, value);
68-    }
69-
70+    auto new_database = std::make_unique<MockableDatabase>();
71+    new_database->m_records = dynamic_cast<MockableDatabase&>(database).m_records;

furszy commented at 11:57 pm on March 17, 2023:

tiny nit:

0return std::make_unique<MockableDatabase>(dynamic_cast<MockableDatabase&>(database).m_records);

achow101 commented at 3:55 pm on April 10, 2023:

Done as suggested

furszy commented at 0:27 am on March 18, 2023: member

Code review ACK a9c58a93 with something that we should correct here and improve in a follow-up after #24914:

Need to restore m_records to previous state after each scenario in wallet_load_ckey.

The reason is that we are currently not checking for specific error messages, we only check that LoadWallet returns a DBErrors::CORRUPT error. So, follow-up test cases could just be carrying previous corruption errors and not the one that we are trying to test.

Note: This is currently fine because errors are tested in-order (checksum error is tested prior to pubkey corruption error) but could not be the case in the future.

After #24914, we could improve it to check against the specific error message.

in src/wallet/test/util.cpp:10 in 7152972a9d outdated

 6@@ -7,6 +7,7 @@
 7 #include <chain.h>
 8 #include <key.h>
 9 #include <key_io.h>
10+#include <streams.h>

TheCharlatan commented at 8:54 pm on March 20, 2023:

nit: I don’t think this include is required.

furszy commented at 3:17 am on March 22, 2023:

why not? DataStream is defined there.

TheCharlatan commented at 6:31 am on March 22, 2023:

Yeah totally, not sure what I was thinking here. Please resolve this.

TheCharlatan approved

TheCharlatan commented at 9:01 pm on March 20, 2023: contributor

Code review ACK a9c58a9

DrahtBot added the label Needs rebase on Apr 3, 2023

achow101 force-pushed on Apr 10, 2023

DrahtBot removed the label Needs rebase on Apr 10, 2023

DrahtBot added the label Needs rebase on Apr 11, 2023

achow101 force-pushed on Apr 11, 2023

DrahtBot removed the label Needs rebase on Apr 11, 2023

DrahtBot added the label Needs rebase on Apr 20, 2023

achow101 force-pushed on May 1, 2023

DrahtBot removed the label Needs rebase on May 1, 2023

DrahtBot added the label CI failed on May 1, 2023

in src/wallet/test/util.cpp:142 in fd9c2ba605 outdated

136+            it++;
137+            continue;
138+        }
139+        it = m_records.erase(it);
140+    }
141+}

furszy commented at 2:27 pm on May 2, 2023:

Missing return statement.

achow101 commented at 3:27 pm on May 2, 2023:

Oops, fixed

achow101 force-pushed on May 2, 2023

DrahtBot removed the label CI failed on May 2, 2023

in src/wallet/test/util.cpp:138 in 03547a0fa3 outdated

152+    while (it != m_records.end()) {
153+        auto& key = it->first;
154+        if (key.size() < prefix.size() || std::search(key.begin(), key.end(), prefix.begin(), prefix.end()) == key.begin()) {
155+            it++;
156+            continue;
157+        }

furszy commented at 12:34 pm on May 3, 2023:

if found, std::search returns the Iterator to the beginning of the first occurrence of the sequence, so this should be != key.begin or == key.end().

Crafted a quick test to check the error https://github.com/furszy/bitcoin-core/commit/f8a7c0670f95b317c46b01855c4c0f0db7ba7714 (no need to add it, I was just double checking it)

achow101 commented at 2:48 pm on May 3, 2023:

Fixed to !=

in src/bench/wallet_loading.cpp:68 in 5c205cbe66 outdated

70@@ -71,7 +71,7 @@ static void WalletLoading(benchmark::Bench& bench, bool legacy_wallet)
71         options.create_flags = WALLET_FLAG_DESCRIPTORS;
72         options.require_format = DatabaseFormat::SQLITE;
73     }
74-    auto database = CreateMockWalletDatabase(options);
75+    auto database = CreateMockableWalletDatabase();

furszy commented at 12:51 pm on May 3, 2023:

What about the db require_format that is above? Is that used anywhere?

achow101 commented at 2:49 pm on May 3, 2023:

Added a commit dropping those.

Introduce MockableDatabase for wallet unit tests

MockableDatabase is a WalletDatabase that allows us to interact with the
records to change them independently from the wallet, as well as
changing the return values from within the tests. This will give us
greater flexibility in testing the wallet.

33c6245ac1

wallet, tests: Replace usage of dummy db with mockable db f67a385556

wallet: Move DummyDatabase to salvage

It's only used by salvage, so make it local to that only.

14aa4cb1e4

wallet, tests: Include wallet/test/util.h

This will be needed for the following scripted-diff to work.

075962bc25

scripted-diff: Replace CreateMockWalletDB with CreateMockableWalletDB

Since we have a mockable wallet database, we don't really need to be
using BDB or SQLite's in-memory database capabilities. It doesn't really
help us to be using those as we aren't doing anything that requires one
type of db over the other, and will just prefer SQLite if it's
available.

MockableDatabase is suitable for these uses, so use
CreateMockableWalletDatabase to use that.

-BEGIN VERIFY SCRIPT-
sed -i "s/CreateMockWalletDatabase(options)/CreateMockableWalletDatabase()/" $(git grep -l "CreateMockWalletDatabase(options)" -- ":(exclude)src/wallet/walletdb.*")
sed -i "s/CreateMockWalletDatabase/CreateMockableWalletDatabase/" $(git grep -l "CreateMockWalletDatabase" -- ":(exclude)src/wallet/walletdb.*")
-END VERIFY SCRIPT-

f0eecf5e40

tests: Update DuplicateMockDatabase for MockableDatabase b3bb17d5d0

tests: Modify records directly in wallet ckey loading test

In the wallet ckey loading test, we modify various ckey records to test
corruption handling. As the database is now a mockable database, we can
modify the records that the database will be initialized with. This
avoids having to use the verbose database reading and writing functions.

80ace042d8

achow101 force-pushed on May 3, 2023

in src/bench/wallet_loading.cpp:71 in 45e3815800 outdated

70+    if (!legacy_wallet) {
71         options.create_flags = WALLET_FLAG_DESCRIPTORS;
72-        options.require_format = DatabaseFormat::SQLITE;
73     }
74     auto database = CreateMockableWalletDatabase();
75     auto wallet = BenchLoadWallet(std::move(database), context, options);

furszy commented at 2:57 pm on May 3, 2023:

In 45e38158:

Could also remove the DatabaseOptions var by providing the create_flags uint64_t directly to BenchLoadWallet.

And remove the using wallet::DatabaseFormat at the top to make the CI happy.

achow101 commented at 3:06 pm on May 3, 2023:

Done

wallet, bench: Remove unused database options from WalletBenchLoading 33e2b82a4f

achow101 force-pushed on May 3, 2023

DrahtBot added the label CI failed on May 3, 2023

DrahtBot removed the label CI failed on May 3, 2023

furszy approved

furszy commented at 7:53 pm on May 3, 2023: member

ACK 33e2b82

DrahtBot requested review from TheCharlatan on May 3, 2023

TheCharlatan approved

TheCharlatan commented at 4:11 pm on May 14, 2023: contributor

ACK 33e2b82a4fc990253ff77655f437c7aed336bc55

fanquake merged this on May 15, 2023

fanquake closed this on May 15, 2023

sidhujag referenced this in commit 9e20d97fd3 on May 15, 2023

fanquake referenced this in commit b34e19a2bf on May 16, 2023

kouloumos commented at 10:24 am on May 16, 2023: contributor

While reviewing #27666, I observed that this PR changed the performance of the WalletLoadingLegacy benchmark:

PR branch (33e2b82a4fc990253ff77655f437c7aed336bc55)

ns/op	op/s	err%	ins/op	cyc/op	IPC	bra/op	miss%	total	benchmark
63,589,392.00	15.73	0.1%	265,707,729.00	125,674,284.00	2.114	32,837,198.00	1.0%	0.32	`WalletLoadingLegacy`

master at the time (539452242e895f2dcd719d41f447a48896d0e4b2)

ns/op	op/s	err%	ins/op	cyc/op	IPC	bra/op	miss%	total	benchmark
272,970,312.00	3.66	0.1%	617,971,997.00	417,477,301.00	1.480	103,288,956.00	0.4%	1.39	`WalletLoadingLegacy``

This is probably not an issue, but it seems worth noting that a change of the underlying mock implementation had such an impact on the performance of legacy wallet loading. I am thinking that this might be the case of a not so effective benchmark, but I’ll need to investigate further as I haven’t look into the actual changes of this PR yet.

achow101 commented at 3:56 pm on May 16, 2023: member

This is probably not an issue, but it seems worth noting that a change of the underlying mock implementation had such an impact on the performance of legacy wallet loading.

It changed the underlying db used for this benchmark from the BDB in-memory database to the MockableDatabase. However I think the goal of this benchmark is to measure LoadWallet specifically and not necessarily the database engine, so I don’t think it particularly matters.

sidhujag referenced this in commit 0536b32890 on May 17, 2023

achow101 referenced this in commit 6cc136bbd3 on May 18, 2023

fanquake referenced this in commit fc4bee3f19 on May 19, 2023

sidhujag referenced this in commit 33242acf61 on May 19, 2023

sidhujag referenced this in commit f0308535cc on May 19, 2023

bitcoin locked this on May 15, 2024

Introduce MockableDatabase for wallet unit tests #26715

Reviews

Conflicts

Introduce `MockableDatabase` for wallet unit tests #26715