[Backport] Wallet database handling abstractions/simplifications #2126

furszy · 2021-01-09T00:07:11Z

Back ported bitcoin#9951 + cleaned all of the zerocoin related, not used, methods inside the walletdb class.

Built on top of #2082.

random-zebra · 2021-01-17T13:23:32Z

Can be rebased (#2082 merged)

Abstract database handle from explicit strFilename into CWalletDBWrapper. Backport and adaptation from btc@71afe3c0995592ff17968816a833a8ed3ce05bf2

Instead, CWalletDB() with a dummy handle will just give you a no-op database in which writes always succeeds and reads always fail. CDB already had functionality for this, so just use that.

CWalletDB now contains a CDB instead of inheriting from it. This makes it easier to replace the internal transaction with a different database, without leaking through internals.

This is only for use in the low-level functions, and CDB is already a friend class.

furszy · 2021-01-18T14:06:46Z

Rebased on top of master. Ready for review.

random-zebra

Looking good. Just few nits.
The CWalletDBWrapper::Backup function, introduced here, is currently never used.
We should refactor BackupWallet (which duplicates most of the code), moving it to CWallet, and making it just a wrapper around this new CWalletDBWrapper::Backup (with proper handling of the custom path).

In the future, we should probably also better encapsulate the methods that deal with the DB, inside CWallet, to avoid using GetDBHandle in so many places (e.g., the RPC, the qt options, or the spkm) which shouldn't have knowledge of lower level db.

src/wallet/walletdb.cpp

random-zebra · 2021-01-20T11:25:28Z

src/wallet/wallet.cpp

+        pwalletdbEncryption = new CWalletDB(*dbw);
+        if (!pwalletdbEncryption->TxnBegin()) {
+            delete pwalletdbEncryption;
+            pwalletdbEncryption = NULL;


nit: nullptr

random-zebra · 2021-01-20T11:25:44Z

src/wallet/wallet.cpp

        }
+
+        delete pwalletdbEncryption;
+        pwalletdbEncryption = NULL;


nit: nullptr

furszy · 2021-01-20T14:36:13Z

The CWalletDBWrapper::Backup function, introduced here, is currently never used.
We should refactor BackupWallet (which duplicates most of the code), moving it to CWallet, and making it just a wrapper around this new CWalletDBWrapper::Backup (with proper handling of the custom path).

Yes, absolutely. Was planning to do it on another PR but can tackle it here as well.

In the future, we should probably also better encapsulate the methods that deal with the DB, inside CWallet, to avoid using GetDBHandle in so many places (e.g., the RPC, the qt options, or the spkm) which shouldn't have knowledge of lower level db.

Yep, had the same thoughts but it was too much to wrap it here as well.

furszy · 2021-01-21T12:37:09Z

PR updated, decoupled the custom backup wallet path and the historical backup code block from the static BackupWallet into their own functions.

This is a previous step before be able to combine and generalize the two backup functions that we currently have into a single one. Will work on it in another PR as the historical backups code is tangled with the GUI notification. Plus.. really needs a focused review.

src/wallet/walletdb.cpp

…hods.

…tion.

furszy · 2021-01-22T01:26:06Z

Updated per @random-zebra's feedback.

random-zebra

utACK 745e6fccbfeb1c2d1e544941366c7f2b73d14ea1

Would be good to add a test for the backupwallet RPC.
We can add it in the follow-up PR that will refactor BackWallet using CWalletDBWrapper::Backup.

Fuzzbawls

Overall good, but introduced a compatibility issue with our current minimum supported version of Boost

src/wallet/walletdb.cpp

random-zebra

re-utACK e5fd215

Fuzzbawls

utACK e5fd215

furszy self-assigned this Jan 9, 2021

random-zebra added this to the 5.1.0 milestone Jan 17, 2021

random-zebra added Upstream Wallet labels Jan 17, 2021

laanwj added 6 commits January 18, 2021 10:42

wallet: Introduce database handle wrapper

3f3a289

Abstract database handle from explicit strFilename into CWalletDBWrapper. Backport and adaptation from btc@71afe3c0995592ff17968816a833a8ed3ce05bf2

wallet: Get rid of fFileBacked

e420d3d

Instead, CWalletDB() with a dummy handle will just give you a no-op database in which writes always succeeds and reads always fail. CDB already had functionality for this, so just use that.

wallet: Reduce references to global bitdb environment

f40006a

wallet: CWalletDB CDB composition not inheritance

2132f42

CWalletDB now contains a CDB instead of inheriting from it. This makes it easier to replace the internal transaction with a different database, without leaking through internals.

wallet: Make IsDummy private in CWalletDBWrapper

08d2310

This is only for use in the low-level functions, and CDB is already a friend class.

wallet: Add comment describing the various classes in walletdb.h

53ad1f6

furszy force-pushed the 2020_wallet_db_wrapper branch from 3efae28 to 57681d3 Compare January 18, 2021 14:05

furszy requested review from Fuzzbawls and random-zebra January 19, 2021 14:45

random-zebra previously approved these changes Jan 20, 2021

View reviewed changes

furszy dismissed random-zebra’s stale review via dc4c426 January 21, 2021 12:26

furszy mentioned this pull request Jan 21, 2021

Revamping block validation interface interaction with wallet #2150

Merged

random-zebra reviewed Jan 22, 2021

View reviewed changes

src/wallet/walletdb.cpp Outdated Show resolved Hide resolved

furszy added 2 commits January 21, 2021 21:15

[Cleanup] walletdb: removing all of the zerocoin related not used met…

78c8679

…hods.

[WalletDb] Decouple custom backup path parsing from BackupWallet func…

1ec4b84

…tion.

furszy force-pushed the 2020_wallet_db_wrapper branch from dc4c426 to 745e6fc Compare January 22, 2021 01:25

random-zebra previously approved these changes Jan 22, 2021

View reviewed changes

Fuzzbawls reviewed Jan 23, 2021

View reviewed changes

src/wallet/walletdb.cpp Outdated Show resolved Hide resolved

[WalletDb] Decouple backups history from the main BackupWallet function.

e5fd215

furszy dismissed random-zebra’s stale review via e5fd215 January 23, 2021 10:07

furszy force-pushed the 2020_wallet_db_wrapper branch from 745e6fc to e5fd215 Compare January 23, 2021 10:07

random-zebra approved these changes Jan 23, 2021

View reviewed changes

furszy requested a review from Fuzzbawls January 25, 2021 02:10

Fuzzbawls approved these changes Jan 25, 2021

View reviewed changes

random-zebra merged commit 8ac7333 into PIVX-Project:master Jan 25, 2021

furszy deleted the 2020_wallet_db_wrapper branch November 29, 2022 14:29

[Backport] Wallet database handling abstractions/simplifications #2126

[Backport] Wallet database handling abstractions/simplifications #2126

Uh oh!

Conversation

furszy commented Jan 9, 2021

Uh oh!

random-zebra commented Jan 17, 2021

Uh oh!

furszy commented Jan 18, 2021

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

random-zebra Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

random-zebra Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

furszy commented Jan 20, 2021

Uh oh!

furszy commented Jan 21, 2021

Uh oh!

Uh oh!

furszy commented Jan 22, 2021

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Fuzzbawls left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Fuzzbawls left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants