Introduce MockableDatabase for wallet unit tests
#26715
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
This was referenced Dec 17, 2022
Merged
4e1a86d to
7bab6f2
Compare
This was referenced Jan 10, 2023
This was referenced Jan 18, 2023
7bab6f2 to
720a114
Compare
720a114 to
9487f7b
Compare
9487f7b to
d881f59
Compare
d881f59 to
82538df
Compare
3942517 to
759ce90
Compare
TheCharlatan
reviewed
Mar 2, 2023
TheCharlatan
reviewed
Mar 2, 2023
It's only used by salvage, so make it local to that only.
This will be needed for the following scripted-diff to work.
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-
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.
abb2a83 to
45e3815
Compare
furszy
reviewed
May 3, 2023
45e3815 to
33e2b82
Compare
furszy
approved these changes
May 3, 2023
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 15, 2023
fanquake
added a commit
that referenced
this pull request
May 16, 2023
0282b21 walletdb: Remove unused CreateMockWalletDatabase (Andrew Chow) Pull request description: This has been superseded by the MockableDatabase. Remove to avoid confusion as to which type of mock database to use for testing. I thought this was included in #26715, maybe it got lost in a rebase. ACKs for top commit: furszy: utACK 0282b21 brunoerg: crACK 0282b21 Tree-SHA512: 18445c4d8a4e2609ef7471c6d75297f43694b79e768f6c993a5addb1dc0e88bd12bac263c9d8e903d828ddf6bf50434f9e2f72048f4fc528e98fed8ee65123ca
|
While reviewing #27666, I observed that this PR changed the performance of the PR branch (33e2b82)
master at the time (5394522)
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. |
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 |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 17, 2023
0282b21 walletdb: Remove unused CreateMockWalletDatabase (Andrew Chow) Pull request description: This has been superseded by the MockableDatabase. Remove to avoid confusion as to which type of mock database to use for testing. I thought this was included in bitcoin#26715, maybe it got lost in a rebase. ACKs for top commit: furszy: utACK 0282b21 brunoerg: crACK 0282b21 Tree-SHA512: 18445c4d8a4e2609ef7471c6d75297f43694b79e768f6c993a5addb1dc0e88bd12bac263c9d8e903d828ddf6bf50434f9e2f72048f4fc528e98fed8ee65123ca
achow101
added a commit
that referenced
this pull request
May 18, 2023
69d4390 test: add coverage for wallet read write db deadlock (furszy) 12daf6f walletdb: scope bdb::EraseRecords under a single db txn (furszy) 043fcb0 wallet: bugfix, GetNewCursor() misses to provide batch ptr to BerkeleyCursor (furszy) Pull request description: Decoupled from #26644 so it can closed in favor of #26715. Basically, with bdb, we can't make a write operation while we are traversing the db with the same db handler. These two operations are performed in different txn contexts and cause a deadlock. Added coverage by using `EraseRecords()` which is the simplest function that executes this process. To replicate it, need bdb support and drop the first commit. The test will run forever. ACKs for top commit: achow101: ACK 69d4390 hebasto: re-ACK 69d4390 Tree-SHA512: b3773be78925f674e962f4a5c54b398a9d0cfe697148c01c3ec0d68281cc5c1444b38165960d219ef3cf1a57c8ce6427f44a876275958d49bbc0808486e19d7d
fanquake
added a commit
to bitcoin-core/gui
that referenced
this pull request
May 19, 2023
b8ed951 msvc: Provide `ObjectFileName` explicitly (Hennadii Stepanov) Pull request description: This PR is a follow-up to bitcoin/bitcoin#26715. Fixes intermittent MSVC link [errors](https://cirrus-ci.com/task/6646912535756800). ACKs for top commit: sipsorcery: ACK b8ed951. Tree-SHA512: 4319ecf61b578ce66d240998d089b9bb0a42f89dcfcb1a73f648cd3915f566c773721dcff1feba27d393a743d121334ccb890b1a519173e35a156d6135821ef4
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 19, 2023
69d4390 test: add coverage for wallet read write db deadlock (furszy) 12daf6f walletdb: scope bdb::EraseRecords under a single db txn (furszy) 043fcb0 wallet: bugfix, GetNewCursor() misses to provide batch ptr to BerkeleyCursor (furszy) Pull request description: Decoupled from bitcoin#26644 so it can closed in favor of bitcoin#26715. Basically, with bdb, we can't make a write operation while we are traversing the db with the same db handler. These two operations are performed in different txn contexts and cause a deadlock. Added coverage by using `EraseRecords()` which is the simplest function that executes this process. To replicate it, need bdb support and drop the first commit. The test will run forever. ACKs for top commit: achow101: ACK 69d4390 hebasto: re-ACK 69d4390 Tree-SHA512: b3773be78925f674e962f4a5c54b398a9d0cfe697148c01c3ec0d68281cc5c1444b38165960d219ef3cf1a57c8ce6427f44a876275958d49bbc0808486e19d7d
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 19, 2023
b8ed951 msvc: Provide `ObjectFileName` explicitly (Hennadii Stepanov) Pull request description: This PR is a follow-up to bitcoin#26715. Fixes intermittent MSVC link [errors](https://cirrus-ci.com/task/6646912535756800). ACKs for top commit: sipsorcery: ACK b8ed951. Tree-SHA512: 4319ecf61b578ce66d240998d089b9bb0a42f89dcfcb1a73f648cd3915f566c773721dcff1feba27d393a743d121334ccb890b1a519173e35a156d6135821ef4
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For the wallet's unit tests, we currently use either
DummyDatabaseor memory-only versions of either BDB or SQLite. The tests that useDummyDatabasejust need aWalletDatabaseso that theCWalletcan be constructed, while the tests using the memory-only databases just need a backing data store. There is also aFailDatabasethat is similar toDummyDatabaseexcept 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. LikeDummyDatabase, most functions do nothing and just return true. LikeFailDatabase, 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 theMockableDatabaseand be retrieved later, but all of this is still held in memory. UsingMockableDatabasecompletely 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