Wallet: remove db mode string #20130

S3RK · 2020-10-12T10:52:54Z

This is a follow-up for #19077

This PR simplifies DB interface by removing mode string from WalletDatabase and WalletBatch.

The mode string was used to determine two flags for the instantiation of db connection:

read-only flag. Never used on connection level. And on batch level Is only used within BerkeleyDatabase::Rewrite where it's replaced with bool flag.
create flag. Is not required as we always check require_existing & require_create flags in MakeDatabase() before creating actual database instance. So we can safely default to always creating database if it doesn't exist yet.

achow101 · 2020-10-12T14:59:58Z

src/wallet/bdb.cpp

@@ -305,20 +305,20 @@ BerkeleyDatabase::~BerkeleyDatabase()
    }
 }

-BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr), m_cursor(nullptr), m_database(database)
+BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const bool fReadOnly, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr), m_cursor(nullptr), m_database(database)


nit: for new variables the naming convention is snake_case.

334eade

👍 and drop this-> below.

BTW, why not just drop member fReadOnly since it's always false?

Changed the name and dropped this->

BTW, why not just drop member fReadOnly since it's always false?

I kept it, because it's set to true in BerkeleyDatabase::Rewrite.

https://github.com/bitcoin/bitcoin/pull/20130/files#diff-b06753f6e4621f5bcd6cd27fed4137e70a262596b08f7f3a1416ea21977c53d4R467

achow101 · 2020-10-12T15:00:17Z

Concept ACK

laanwj · 2020-10-12T16:06:14Z

read-only flag. Never used on connection level.

Should we have a way to open databases read-only? It seems this would be useful in some cases, e.g. in the past I remember discussion about the bitcoin-wallet tool for purely informational commands, which should be able to leave a database unmodified otherwise (or be able to act on a read-only file system).

achow101 · 2020-10-12T16:12:05Z

Should we have a way to open databases read-only? It seems this would be useful in some cases, e.g. in the past I remember discussion about the bitcoin-wallet tool for purely informational commands, which should be able to leave a database unmodified otherwise (or be able to act on a read-only file system).

I think that it would be useful to have a read-only mode in the future, but it should be implemented differently from this mode string that we currently have. So this PR is at least a first step towards that, and we can add read-only mode for when we need it.

DrahtBot · 2020-10-12T17:01:23Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpc, wallet: Expose database format in getwalletinfo #20125 (rpc, wallet: Expose database format in getwalletinfo by promag)
wallet: Remove WalletDatabase refcounting and enforce only one Batch access the database at a time #20096 (wallet: Remove WalletDatabase refcounting and enforce only one Batch access the database at a time by achow101)
refactor: Some wallet cleanups #19980 (refactor: Some wallet cleanups by promag)
wallet: Mark replaced tx to not be in the mempool anymore #18842 (wallet: Mark replaced tx to not be in the mempool anymore by MarcoFalke)

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.

promag

Tested ACK 53718b2, small commits that could be squashed but no big deal.

promag · 2020-10-12T21:02:27Z

src/wallet/bdb.cpp

@@ -305,20 +305,20 @@ BerkeleyDatabase::~BerkeleyDatabase()
    }
 }

-BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr), m_cursor(nullptr), m_database(database)
+BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const bool fReadOnly, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr), m_cursor(nullptr), m_database(database)


334eade

👍 and drop this-> below.

BTW, why not just drop member fReadOnly since it's always false?

We never need to open database in read-only mode as it's controlled separately for every batch. Also we can safely create database if it doesn't exist already because require_existing option is verified in MakeDatabase before creating a new WalletDatabase instance.

S3RK · 2020-10-13T12:04:50Z

read-only flag. Never used on connection level.

Should we have a way to open databases read-only? It seems this would be useful in some cases, e.g. in the past I remember discussion about the bitcoin-wallet tool for purely informational commands, which should be able to leave a database unmodified otherwise (or be able to act on a read-only file system).

Though I agree we might want to have it I think it's better not to keep unused functionality in the code. Also, current implementation is pretty limited. We use one connection per wallet and this prevents us from opening a true read-only connection, because it will break some wallet functionality. I guess this is why read-only mode was never used in practice.

If we want to introduce proper read-only mode we need to discuss different design trade-offs. One option would be to look at the possibility to bring it to a wallet level (for example load wallet in read-only mode). Alternatively for a command-level read-only mode we need to: a) either manage it on a batch level b) create separate read-write and read-only connections c) reconnect when needed d) other ideas..

I'll be happy to continue the work if there is strong demand for the feature.

S3RK · 2020-10-13T12:07:45Z

@promag thanks for the quick review! I fixed the comment and squashed the commits.

Side node: I don't know what is the accepted practice here, but I usually do small commits as it's easier to squash than split. I also believe it sometimes can be useful as it provides more information on implementation.

achow101 · 2020-10-13T16:05:55Z

ACK 135afa7

laanwj · 2020-10-14T08:45:09Z

I think that it would be useful to have a read-only mode in the future, but it should be implemented differently from this mode string that we currently have. So this PR is at least a first step towards that, and we can add read-only mode for when we need it.

Fair enough, concept ACK then.

Code review ACK 135afa7

135afa7 wallet: remove db mode string (Ivan Metlushko) Pull request description: This is a [follow-up](bitcoin#19077 (comment)) for bitcoin#19077 This PR simplifies DB interface by removing mode string from `WalletDatabase` and `WalletBatch`. The mode string was used to determine two flags for the instantiation of db connection: 1) read-only flag. Never used on connection level. And on batch level Is only used within `BerkeleyDatabase::Rewrite` where it's replaced with bool flag. 2) create flag. Is not required as we always check `require_existing` & `require_create` flags in `MakeDatabase()` before creating actual database instance. So we can safely default to always creating database if it doesn't exist yet. ACKs for top commit: achow101: ACK 135afa7 laanwj: Code review ACK 135afa7 Tree-SHA512: f49c07c7387c02e517a58199620a678a918f8dfc20d1347d29fd6adea0bc89698c26cb8eef42b0977961c11c207c4bbe109bc31059f47c126cc600b01fd987eb

Summary: We never need to open database in read-only mode as it's controlled separately for every batch. Also we can safely create database if it doesn't exist already because require_existing option is verified in MakeDatabase before creating a new WalletDatabase instance. This is a backport of [[bitcoin/bitcoin#20130 | core#20130]] Notes: - walletdb_tests.cpp is an addtional ABC test suite introduced in D745 - ABC does not have a `CWallet::MarkReplaced` method Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10468

DrahtBot added the Wallet label Oct 12, 2020

achow101 reviewed Oct 12, 2020

View reviewed changes

This was referenced Oct 12, 2020

rpc, wallet: Expose database format in getwalletinfo #20125

Merged

wallet: Remove WalletDatabase refcounting and enforce only one Batch access the database at a time #20096

Closed

refactor: Some wallet cleanups #19980

Merged

promag reviewed Oct 12, 2020

View reviewed changes

DrahtBot mentioned this pull request Oct 12, 2020

wallet: Mark replaced tx to not be in the mempool anymore #18842

Merged

S3RK force-pushed the wallet_remove_mode_3 branch from 53718b2 to 135afa7 Compare October 13, 2020 11:44

laanwj merged commit 9efa55c into bitcoin:master Oct 14, 2020

achow101 mentioned this pull request Oct 14, 2020

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

Merged

S3RK deleted the wallet_remove_mode_3 branch October 15, 2020 04:52

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: remove db mode string #20130

Wallet: remove db mode string #20130

Uh oh!

S3RK commented Oct 12, 2020

Uh oh!

achow101 Oct 12, 2020

Uh oh!

promag Oct 12, 2020

Uh oh!

S3RK Oct 13, 2020

Uh oh!

promag Oct 13, 2020

Uh oh!

achow101 Oct 13, 2020

Uh oh!

achow101 commented Oct 12, 2020

Uh oh!

laanwj commented Oct 12, 2020

Uh oh!

achow101 commented Oct 12, 2020

Uh oh!

DrahtBot commented Oct 12, 2020

Uh oh!

promag left a comment

Uh oh!

promag Oct 12, 2020

Uh oh!

S3RK commented Oct 13, 2020

Uh oh!

S3RK commented Oct 13, 2020

Uh oh!

achow101 commented Oct 13, 2020

Uh oh!

laanwj commented Oct 14, 2020 •

edited

Loading

Uh oh!

Uh oh!

Wallet: remove db mode string #20130

Wallet: remove db mode string #20130

Uh oh!

Conversation

S3RK commented Oct 12, 2020

Uh oh!

achow101 Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

promag Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

S3RK Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

promag Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 commented Oct 12, 2020

Uh oh!

laanwj commented Oct 12, 2020

Uh oh!

achow101 commented Oct 12, 2020

Uh oh!

DrahtBot commented Oct 12, 2020

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

S3RK commented Oct 13, 2020

Uh oh!

S3RK commented Oct 13, 2020

Uh oh!

achow101 commented Oct 13, 2020

Uh oh!

laanwj commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

laanwj commented Oct 14, 2020 •

edited

Loading