Conversation
src/wallet/bdb.cpp
Outdated
There was a problem hiding this comment.
nit: for new variables the naming convention is snake_case.
There was a problem hiding this comment.
334eadec38afd0a34edb990322fda0825118ec76
👍 and drop this-> below.
BTW, why not just drop member fReadOnly since it's always false?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
|
Concept ACK |
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. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
promag
left a comment
promag
left a comment
There was a problem hiding this comment.
Tested ACK 53718b268a37d6501fbb65841e90a30a9c73e85f, small commits that could be squashed but no big deal.
src/wallet/bdb.cpp
Outdated
There was a problem hiding this comment.
334eadec38afd0a34edb990322fda0825118ec76
👍 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
force-pushed
the
wallet_remove_mode_3
branch
from
53718b2 to
135afa7
Compare
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. |
|
@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. |
|
ACK 135afa7 |
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
5 participants
This is a follow-up for #19077
This PR simplifies DB interface by removing mode string from
WalletDatabaseandWalletBatch.The mode string was used to determine two flags for the instantiation of db connection:
BerkeleyDatabase::Rewritewhere it's replaced with bool flag.require_existing&require_createflags inMakeDatabase()before creating actual database instance. So we can safely default to always creating database if it doesn't exist yet.