wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase #33032
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33032. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
utACK 7889750 |
WalletBatch needs to be in a scope so that it is destroyed before the database is closed during migration.
Instead of directly copying the stored records map when duplicating a MockableDatabase, use a Cursor to read the records, and a Batch to write them into the new database. This prepares for using SQLite as the database backend for MockableDatabase.
7889750 to
f698524
Compare
There was a problem hiding this comment.
Concept ACK f698524
I have looked in depth only the last commit. Almost all of my suggestions merge towards having child mock class(es) extending the base SQLite specific classes, wherever required. They can be deleted in the future if the need arises without touching the base SQLite classes. Also, I believe they would not make it difficult to achieve the following goal stated in the PR desc.
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.
| @@ -3448,7 +3448,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"); | |||
There was a problem hiding this comment.
In f698524 "test, wallet: Replace MockableDatabase with in-memory SQLiteDatabase"
Nit and need not be done in this PR: To avoid hardcoding mocking constants in non-test code, perhaps the following? If the return value of Format had been an enum, the following would come across as a tighter check.
- Assert(m_database->Format() == "bdb_ro" || m_database->Format() == "sqlite-mock");
+ Assert(m_database->Format() != "sqlite"); There was a problem hiding this comment.
I prefer to leave this as-is as it is a more specific check. It checks for the db types that are allowed to be legacy wallets, rather than allowing legacy wallets for all non-sqlite databases.
The mocking functionality of MockableDatabase, MockableBatch, and MockableCursor was not really being used. These are changed to be subclasses of their respective SQLite* classes and will use in-memory SQLite databases so that the tests are more representative of actual database behavior. MockableCursor is removed as there are no overrides needed in SQLiteCursor for the tests.
f698524 to
98bb7ef
Compare
98bb7ef to
1cdb916
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
src/wallet/test/util.cpp
Outdated
| std::unique_ptr<DatabaseBatch> batch_orig = database.MakeBatch(); | ||
| std::unique_ptr<DatabaseCursor> cursor_orig = batch_orig->GetNewCursor(); | ||
|
|
||
| std::unique_ptr<WalletDatabase> new_db = std::make_unique<MockableDatabase>(); | ||
| std::unique_ptr<WalletDatabase> new_db = CreateMockableWalletDatabase(); | ||
| std::unique_ptr<DatabaseBatch> new_db_batch = new_db->MakeBatch(); | ||
| MockableBatch* batch_new = dynamic_cast<MockableBatch*>(new_db_batch.get()); | ||
| MockableSQLiteBatch* batch_new = dynamic_cast<MockableSQLiteBatch*>(new_db_batch.get()); | ||
| Assert(batch_new); | ||
|
|
||
| while (true) { |
There was a problem hiding this comment.
Nit to use tighter function signature that goes along with the function name too. Builds & tests fine on my system.
Use MockableSQLiteDatabase in return type that passes for WalletDatabase
diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp
index 6017582395..58fafeecf7 100644
--- a/src/wallet/test/util.cpp
+++ b/src/wallet/test/util.cpp
@@ -80,12 +80,12 @@ void TestUnloadWallet(std::shared_ptr<CWallet>&& wallet)
WaitForDeleteWallet(std::move(wallet));
}
-std::unique_ptr<WalletDatabase> DuplicateMockDatabase(WalletDatabase& database)
+std::unique_ptr<MockableSQLiteDatabase> DuplicateMockDatabase(MockableSQLiteDatabase& database)
{
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<MockableSQLiteDatabase> 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);
@@ -116,7 +116,7 @@ MockableSQLiteDatabase::MockableSQLiteDatabase()
: SQLiteDatabase(fs::PathFromString("mock/"), fs::PathFromString("mock/wallet.dat"), DatabaseOptions(), SQLITE_OPEN_MEMORY)
{}
-std::unique_ptr<WalletDatabase> CreateMockableWalletDatabase()
+std::unique_ptr<MockableSQLiteDatabase> CreateMockableWalletDatabase()
{
return std::make_unique<MockableSQLiteDatabase>();
}
diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h
index a240a8ae4f..cedd37ee7e 100644
--- a/src/wallet/test/util.h
+++ b/src/wallet/test/util.h
@@ -70,7 +70,7 @@ public:
std::unique_ptr<DatabaseBatch> MakeBatch() override { return std::make_unique<MockableSQLiteBatch>(*this); }
};
-std::unique_ptr<WalletDatabase> CreateMockableWalletDatabase();
+std::unique_ptr<MockableSQLiteDatabase> CreateMockableWalletDatabase();
MockableSQLiteDatabase& GetMockableDatabase(CWallet& wallet);
DescriptorScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string& desc_str, const bool success);
MockableDatabasewas 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 theMockabeDatabasewith 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.