Wallet: remove db mode string

S3RK commented at 10:52 am on October 12, 2020: member

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.

DrahtBot added the label Wallet on Oct 12, 2020

in src/wallet/bdb.cpp:308 in 334eadec38 outdated

304@@ -305,20 +305,20 @@ BerkeleyDatabase::~BerkeleyDatabase()
305     }
306 }
307 
308-BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr), m_cursor(nullptr), m_database(database)
309+BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const bool fReadOnly, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr), m_cursor(nullptr), m_database(database)

achow101 commented at 2:59 pm on October 12, 2020:

nit: for new variables the naming convention is snake_case.

promag commented at 9:02 pm on October 12, 2020:

334eadec38afd0a34edb990322fda0825118ec76

👍 and drop this-> below.

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

S3RK commented at 11:46 am on October 13, 2020:

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.

promag commented at 11:49 am on October 13, 2020:

Where?

achow101 commented at 3:51 pm on October 13, 2020:

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

achow101 commented at 3:00 pm on October 12, 2020: member

Concept ACK

laanwj commented at 4:06 pm on October 12, 2020: member

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 commented at 4:12 pm on October 12, 2020: member

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 commented at 5:01 pm on October 12, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20125 (rpc, wallet: Expose database format in getwalletinfo by promag)
#20096 (wallet: Remove WalletDatabase refcounting and enforce only one Batch access the database at a time by achow101)
#19980 (refactor: Some wallet cleanups by promag)
#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 commented at 9:33 pm on October 12, 2020: member

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

wallet: remove db mode string

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.

135afa749c

S3RK force-pushed on Oct 13, 2020

S3RK commented at 12:04 pm on October 13, 2020: member

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 commented at 12:07 pm on October 13, 2020: member

@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 commented at 4:05 pm on October 13, 2020: member

ACK 135afa749c6e835ea33b8678cdb35da9640eede8

laanwj commented at 8:45 am on October 14, 2020: member

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 135afa749c6e835ea33b8678cdb35da9640eede8

laanwj merged this on Oct 14, 2020

laanwj closed this on Oct 14, 2020

S3RK deleted the branch on Oct 15, 2020

sidhujag referenced this in commit ac1143b11d on Oct 16, 2020

Fabcien referenced this in commit 74290d4c5e on Nov 17, 2021

DrahtBot locked this on Feb 15, 2022

Wallet: remove db mode string #20130

Conflicts