Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from
Jul 30, 2020
Merged

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

merged 3 commits into from
Jul 30, 2020

Conversation

achow101
Copy link
Member

@achow101 achow101 commented May 28, 2020 •
edited
Loading

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

@achow101 achow101 mentioned this pull request May 28, 2020
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented May 29, 2020 •
edited
Loading

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

Conflicts

Reviewers, 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 May 29, 2020
Merged
Merged
Merged
Merged
Merged
Closed
Merged
Closed
Merged
Closed
Closed
@practicalswift
Copy link
Contributor

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 :)

@DrahtBot DrahtBot mentioned this pull request Jun 1, 2020
Closed
@DrahtBot DrahtBot mentioned this pull request Jun 2, 2020
Merged
@laanwj
Copy link
Member

laanwj commented Jun 4, 2020

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
Copy link
Member

luke-jr commented Jun 5, 2020

Do we lose testing of bdb this way?

@achow101
Copy link
Member Author

achow101 commented Jun 5, 2020

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.

@DrahtBot DrahtBot mentioned this pull request Jun 9, 2020
Closed
This was referenced Jun 16, 2020
Merged
Merged
Closed
@ryanofsky
Copy link
Contributor

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
Copy link
Member Author

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.

@achow101 achow101 force-pushed the true-dummydb branch 2 times, most recently from b6b5a2f to fa1a8e6 Compare July 1, 2020 16:09
@DrahtBot DrahtBot mentioned this pull request Jul 7, 2020
Merged
ryanofsky
ryanofsky approved these changes Jul 21, 2020
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

ryanofsky
ryanofsky approved these changes Jul 24, 2020
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DrahtBot DrahtBot mentioned this pull request Jul 29, 2020
Merged
achow101 added 3 commits July 29, 2020 12:28
@achow101
Introduce DummyDatabase and use it in the tests
0103d64
@achow101
Remove BDB dummy databases
da039d2
@achow101
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.
@instagibbs
Copy link
Member

utACK 0fcff54

@maflcko
Copy link
Member

maflcko commented Jul 30, 2020

crACK 0fcff54 🚈

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

crACK 0fcff547d5b47822c13104978fda0c486e596526 🚈
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgGZgv+JeOhpzNalFjNtnK6xDi2rqXcqrtHiNvIa4TGToO6uY5Xh7cIyWrPq6nP
a93uJ8Sagwa+Wspt+MCC1bLmERIvBhfYzr2/GQUbTOlXzR0u5/B2wTNywN23vyNs
27Rjt6cQdE+0XRMNjHPbuQaZw16CdRdMGoW1Tn8fjceRCRHQg0BZaOH9UzWBbDra
42UVvVdmBQiPuYtRrrFOHy0BMJefkPxiYgRoHM6oLordMYH7TnC7RZf+Rn1Kpyk+
/ZqGWl+eRk+Yz5eCizArmzFobcCvzY2VvUsLeg+xq1k1oIbStuCrV2QsLaYRCp4V
Hm180u0sq65gCljrvjnLCKQO/B3tqcR4jsQA5Vy6/PM7yZEHWLKrw4D2H/QvRG/S
aDZrZSO8ZX80MAc6cxLYmZ8cMiWzzet52GIFD5p+QhLhEPQAFhKLBpbQIObA6/L5
X4ozPsQqQj3PiL8ynUTqjDFFpb4yrCOiMBEP1HwBrrAgfMgQtYMaGPnkMC6qHgC0
l6UNOH28
=32me
-----END PGP SIGNATURE-----

Timestamp of file with hash acb069d85216bbd6e4cfaa47f6c835d6684dd52d7006bbeee8a4fbe3e90dca14 -

@maflcko maflcko merged commit 37b765b into bitcoin:master Jul 30, 2020
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 6, 2021
@achow101 @PiRK
Introduce DummyDatabase and use it in the tests
636082e 
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
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 6, 2021
@achow101 @PiRK
Remove BDB dummy databases
5948655 
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
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 6, 2021
@achow101 @PiRK
walletdb: Ensure that having no database handle is a failure
2e5de08 
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@achow101 @DrahtBot @practicalswift @laanwj @luke-jr @ryanofsky @instagibbs @maflcko @fanquake