refactor: Some wallet cleanups

promag commented at 11:04 pm on September 19, 2020: member

promag renamed this:
~~2020 09 wallet cleanups~~
refactor: Some wallet cleanups
on Sep 19, 2020

fanquake added the label Wallet on Sep 19, 2020

promag force-pushed on Sep 19, 2020

in src/wallet/wallet.h:737 in a66278f44f outdated

737-     */
738-    WalletDatabase& GetDBHandle()
739+    WalletDatabase& GetDatabase() const override
740     {
741-        return *database;
742+        assert(static_cast<bool>(m_database));

promag commented at 11:57 pm on September 19, 2020:

@ryanofsky made this change in the context of https://github.com/bitcoin-core/gui/issues/95#issuecomment-694254368, where m_database can be null.

ryanofsky commented at 1:42 pm on October 15, 2020:

@ryanofsky made this change in the context of bitcoin-core/gui#95 (comment), where m_database can be null.

Would be interested to see branch

promag force-pushed on Sep 20, 2020

DrahtBot commented at 1:53 am on September 20, 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:

#20096 (wallet: Remove WalletDatabase refcounting and enforce only one Batch access the database at a time by achow101)
#19876 (wallet: Fix wallet loading race during node start by fjahr)
#18904 (Don’t call lsn_reset in periodic flush by bvbfan)

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 12:19 pm on September 24, 2020: member

@achow101 @meshcollider ty

in src/wallet/load.cpp:74 in c6a5cd7a64 outdated

77+        std::unique_ptr<WalletDatabase> database = MakeWalletDatabase(wallet_file, options, status, error_string);
78+        if (!database) {
79             chain.initError(error_string);
80             return false;
81         }
82+        if (!wallet_paths.insert(database->Filename()).second) {

achow101 commented at 6:17 pm on September 24, 2020:

Since MakeWalletDatabase will have a duplicate open check, this error will never be hit. Instead the user will get a less useful error about Another instance of bitcoin may be using it.

promag commented at 6:22 pm on September 24, 2020:

What PR changes it? SQLite?

ryanofsky commented at 1:39 pm on October 15, 2020:

In commit “refactor: Drop wallet path determination from VerifyWallets” (c6a5cd7a64c78b162f545a3467d0fea7dcaadfcc)

re: #19980 (review)

What PR changes it? SQLite?

Yes, sqlite doesn’t keep a global list of databases. I think best thing would be to drop this commit. Even though current check is awkward and I understand wanting to get rid of it, I think it makes more sense to check for duplicate wallets at wallet level not db level like achow says: to be able to distinguish duplicate loads from locking errors, without the db layer needing to keep its own list of open databases.

Wallet code currently keeps a lists of loaded wallets, a list of wallets being loaded in current thread, a list of wallets being loaded in other threads, a list of wallets being unloaded in other threads. I started changes to combine these into one list in #19619, but dropped the changes to keep that PR more focused. BDB has a list of open databases to deal with shared environments, but sqlite doesn’t need this complication.

DrahtBot added the label Needs rebase on Oct 14, 2020

ryanofsky approved

ryanofsky commented at 2:03 pm on October 15, 2020: member

Code review ACK 3fcb7cd9b629695206a72c75e7ca3f6a210b1d20. Does need rebase. Also, would suggest dropping VerifyWallets commit c6a5cd7a64c78b162f545a3467d0fea7dcaadfcc to keep PR from changing behavior and keep it more focused. PR title could then be changed to something like “wallet refactor: consistently call CWallet::GetDatabase”

refactor: Drop redudant CWallet::GetDBHandle 021feb3187

refactor: Assert before dereference in CWallet::GetDatabase 9b74461fa2

promag force-pushed on Nov 7, 2020

DrahtBot removed the label Needs rebase on Nov 7, 2020

meshcollider commented at 0:35 am on November 18, 2020: contributor

utACK 9b74461fa293453a9eb0b1717b30b3f7fa778d91

ryanofsky approved

ryanofsky commented at 4:49 pm on December 1, 2020: member

Code review ACK 9b74461fa293453a9eb0b1717b30b3f7fa778d91. Changes since last review: rebasing due to conflict, dropping wallet path commit c6a5cd7a64c78b162f545a3467d0fea7dcaadfcc as suggested in discussion, making GetDatabase() const in the earlier commit. Giving more descriptive title like “Consistently use CWallet::GetDatabase” instead of “Some wallet cleanups” might motivate more reviews, but this seems about ready

achow101 commented at 7:24 pm on December 1, 2020: member

Code Review ACK 9b74461fa293453a9eb0b1717b30b3f7fa778d91

fanquake merged this on Dec 2, 2020

fanquake closed this on Dec 2, 2020

promag deleted the branch on Dec 2, 2020

sidhujag referenced this in commit abbce5106f on Dec 2, 2020

DrahtBot locked this on Feb 15, 2022

refactor: Some wallet cleanups #19980

Conflicts