wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase #19102

achow101 · 2020-05-28T22:13:33Z

In the unit tests, we use a dummy WalletDatabase which does nothing and always returns true. This is currently implemented by creating a BerkeleyDatabase in dummy mode. This PR instead adds a DummyDatabase class which does nothing and never fails for use in the tests. CreateDummyWalletDatabase is changed to return this DummyDatabase and BerkeleyDatabase is cleaned up to remove all of the checks for IsDummy.

Based on WalletDatabase abstract class introduced in #19334

DrahtBot · 2020-05-29T06:43:58Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp #19619 (Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp by ryanofsky)

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.

practicalswift · 2020-05-30T14:22:51Z

Concept ACK

Inspired by DummyDatabase I'll try to find time to do create a FuzzedDatabase implementing the WalletDatabase interface but with data feeded by FuzzedDataProvider :)

laanwj · 2020-06-04T15:20:59Z

Concept ACK, IsDummy was always a bit of a hack and now that there is a proper wallet database interface, creating a dummy at that level makes sense.

luke-jr · 2020-06-05T16:35:44Z

Do we lose testing of bdb this way?

achow101 · 2020-06-05T16:48:14Z

Do we lose testing of bdb this way?

No. IsDummy was only used right at the beginning of functions so caused them to do nothing anyways. In places that may have been called but didn't check IsDummy, they would have exited early anyways due to no m_db being set.

ryanofsky · 2020-06-23T13:28:37Z

Could consider moving new classes to src/wallet/test/ since they are only used to speed up unit tests and it would be confusing if they were ever used in real code

achow101 · 2020-06-23T16:30:34Z

Could consider moving new classes to src/wallet/test/ since they are only used to speed up unit tests and it would be confusing if they were ever used in real code

SalvageWallet uses DummyDatabase for a dummy wallet.

ryanofsky

Code review ACK 1937a88a76548979da8559fd3e959fda7a569901 last 3 commits. Easy review and nice cleanup

23f633c19c65957f0233ec2b2c1a5555006abe37 Introduce DummyDatabase and use it in the tests (5/7)
53c1f553a13077782a6bdf28d186467d320af74f Remove BDB dummy databases (6/7)
1937a88a76548979da8559fd3e959fda7a569901 walletdb: Ensure that having no database handle is a failure (7/7)

ryanofsky

Code review ACK d93adba3f9d3fde4c0a79a62b13e7759fb892466. No changes since last review other than rebase.

Previously having no database handle could still be considered a success when BerkeleyDatabase and BerkeleyBatch were used for dummy database things. With dedicated DummyDatabase and DummyBatch classes now, these should fail.

instagibbs · 2020-07-30T14:32:53Z

utACK 0fcff54

maflcko · 2020-07-30T15:00:42Z

crACK 0fcff54 🚈

Show signature and timestamp

Signature:

Timestamp of file with hash acb069d85216bbd6e4cfaa47f6c835d6684dd52d7006bbeee8a4fbe3e90dca14 -

Summary: Note: the `#include <util/memory.h>` is unnecessary because we use `std::make_unique` rather than `MakeUnique`. This is a backport of [[bitcoin/bitcoin#19102 | core#19102]] [1/3] bitcoin/bitcoin@0103d64 Depends on D10038 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10048

Summary: This is a backport of [[bitcoin/bitcoin#19102 | core#19102]] [2/3] bitcoin/bitcoin@da039d2 Depends on D10048 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10049

Summary: Previously having no database handle could still be considered a success when BerkeleyDatabase and BerkeleyBatch were used for dummy database things. With dedicated DummyDatabase and DummyBatch classes now, these should fail. This is a backport of [[bitcoin/bitcoin#19102 | core#19102]] [3/3] bitcoin/bitcoin@0fcff54 Depends on D10049 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10050

achow101 mentioned this pull request May 28, 2020

wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077

Merged

fanquake added the Wallet label May 28, 2020

DrahtBot mentioned this pull request Jun 1, 2020

gui: Drop RecentRequestsTableModel dependency to WalletModel #18618

Closed

achow101 force-pushed the true-dummydb branch from 11b96ab to 57d9b7d Compare June 1, 2020 19:26

DrahtBot mentioned this pull request Jun 2, 2020

wallettool: Add dump and createfromdump commands #19137

Merged

achow101 force-pushed the true-dummydb branch from 57d9b7d to 4f821eb Compare June 2, 2020 03:31

DrahtBot added the Needs rebase label Jun 2, 2020

achow101 force-pushed the true-dummydb branch from 4f821eb to 6ec7c68 Compare June 2, 2020 16:45

DrahtBot removed the Needs rebase label Jun 2, 2020

DrahtBot mentioned this pull request Jun 9, 2020

wallet: Remove first parameter to ScanForWalletTransactions start_hash #19216

Closed

achow101 force-pushed the true-dummydb branch from 6ec7c68 to 8c68b79 Compare June 9, 2020 21:04

This was referenced Jun 16, 2020

wallet: Refactor BerkeleyBatch Read, Write, Erase, and Exists functions into non-template functions #19292

Merged

wallet: move BDB specific classes to bdb.{cpp/h} #19290

Merged

wallet: Refactor the classes in wallet/db.{cpp/h} #18971

Closed

achow101 force-pushed the true-dummydb branch from 2a03ed8 to a8fb121 Compare June 23, 2020 16:34

DrahtBot added the Needs rebase label Jul 1, 2020

achow101 force-pushed the true-dummydb branch 2 times, most recently from b6b5a2f to fa1a8e6 Compare July 1, 2020 16:09

DrahtBot added Needs rebase and removed Needs rebase labels Jul 1, 2020

achow101 force-pushed the true-dummydb branch from fa1a8e6 to a88eace Compare July 6, 2020 18:36

DrahtBot removed the Needs rebase label Jul 6, 2020

DrahtBot mentioned this pull request Jul 7, 2020

wallet: Introduce WalletDatabase abstract class #19334

Merged

DrahtBot added the Needs rebase label Jul 8, 2020

achow101 force-pushed the true-dummydb branch from a88eace to 1f1e9d3 Compare July 9, 2020 16:03

DrahtBot removed the Needs rebase label Jul 9, 2020

achow101 force-pushed the true-dummydb branch from 1f1e9d3 to 1937a88 Compare July 14, 2020 15:22

ryanofsky approved these changes Jul 21, 2020

View reviewed changes

achow101 force-pushed the true-dummydb branch from 1937a88 to d93adba Compare July 23, 2020 03:47

ryanofsky approved these changes Jul 24, 2020

View reviewed changes

DrahtBot mentioned this pull request Jul 29, 2020

Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp #19619

Merged

achow101 added 3 commits July 29, 2020 12:28

Introduce DummyDatabase and use it in the tests

0103d64

Remove BDB dummy databases

da039d2

walletdb: Ensure that having no database handle is a failure

0fcff54

Previously having no database handle could still be considered a success when BerkeleyDatabase and BerkeleyBatch were used for dummy database things. With dedicated DummyDatabase and DummyBatch classes now, these should fail.

achow101 force-pushed the true-dummydb branch from d93adba to 0fcff54 Compare July 29, 2020 16:35

maflcko merged commit 37b765b into bitcoin:master Jul 30, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase #19102

wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase #19102

Uh oh!

achow101 commented May 28, 2020 •

edited

Loading

Uh oh!

DrahtBot commented May 29, 2020 •

edited

Loading

Uh oh!

practicalswift commented May 30, 2020

Uh oh!

laanwj commented Jun 4, 2020

Uh oh!

luke-jr commented Jun 5, 2020

Uh oh!

achow101 commented Jun 5, 2020

Uh oh!

ryanofsky commented Jun 23, 2020

Uh oh!

achow101 commented Jun 23, 2020

Uh oh!

ryanofsky left a comment

Uh oh!

ryanofsky left a comment

Uh oh!

instagibbs commented Jul 30, 2020

Uh oh!

maflcko commented Jul 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase #19102

wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase #19102

Uh oh!

Conversation

achow101 commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift commented May 30, 2020

Uh oh!

laanwj commented Jun 4, 2020

Uh oh!

luke-jr commented Jun 5, 2020

Uh oh!

achow101 commented Jun 5, 2020

Uh oh!

ryanofsky commented Jun 23, 2020

Uh oh!

achow101 commented Jun 23, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Jul 30, 2020

Uh oh!

maflcko commented Jul 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

achow101 commented May 28, 2020 •

edited

Loading

DrahtBot commented May 29, 2020 •

edited

Loading