refactor: Some wallet cleanups #19980
Conversation
promag
force-pushed
the
2020-09-wallet-cleanups
branch
3 times, most recently
from
84041df to
a66278f
Compare
src/wallet/wallet.h
Outdated
There was a problem hiding this comment.
@ryanofsky made this change in the context of bitcoin-core/gui#95 (comment), where m_database can be null.
There was a problem hiding this comment.
@ryanofsky made this change in the context of bitcoin-core/gui#95 (comment), where
m_databasecan be null.
Would be interested to see branch
promag
force-pushed
the
2020-09-wallet-cleanups
branch
from
a66278f to
3fcb7cd
Compare
|
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. |
src/wallet/load.cpp
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What PR changes it? SQLite?
There was a problem hiding this comment.
In commit "refactor: Drop wallet path determination from VerifyWallets" (c6a5cd7a64c78b162f545a3467d0fea7dcaadfcc)
re: #19980 (comment)
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.
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
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"
src/wallet/load.cpp
Outdated
There was a problem hiding this comment.
In commit "refactor: Drop wallet path determination from VerifyWallets" (c6a5cd7a64c78b162f545a3467d0fea7dcaadfcc)
re: #19980 (comment)
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.
src/wallet/wallet.h
Outdated
There was a problem hiding this comment.
@ryanofsky made this change in the context of bitcoin-core/gui#95 (comment), where
m_databasecan be null.
Would be interested to see branch
promag
force-pushed
the
2020-09-wallet-cleanups
branch
from
3fcb7cd to
9b74461
Compare
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK 9b74461. 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
|
Code Review ACK 9b74461 |
9b74461 refactor: Assert before dereference in CWallet::GetDatabase (João Barbosa) 021feb3 refactor: Drop redudant CWallet::GetDBHandle (João Barbosa) Pull request description: ACKs for top commit: achow101: Code Review ACK 9b74461 meshcollider: utACK 9b74461 ryanofsky: Code review ACK 9b74461. 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 Tree-SHA512: 68cf3b5e9fe0acb3a5cd081086629989f213f1904cc344e5775767b56759a7d905b1e1c303afbe40f172ff81bf07f3719b59d8f6ec2de3fdd53cd0e2d220fb25
6 participants
No description provided.