refactor: Some wallet cleanups #19980
Conversation
promag
force-pushed
the
2020-09-wallet-cleanups
branch
3 times, most recently
from
84041df
to
a66278f
Compare
| { | ||
| return *database; | ||
| assert(static_cast<bool>(m_database)); |
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
| chain.initError(error_string); | ||
| return false; | ||
| } | ||
| if (!wallet_paths.insert(database->Filename()).second) { |
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" (c6a5cd7)
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/load.cpp
Outdated
| chain.initError(error_string); | ||
| return false; | ||
| } | ||
| if (!wallet_paths.insert(database->Filename()).second) { |
There was a problem hiding this comment.
In commit "refactor: Drop wallet path determination from VerifyWallets" (c6a5cd7)
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.
| { | ||
| return *database; | ||
| assert(static_cast<bool>(m_database)); |
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
There was a problem hiding this comment.
Code review ACK 9b74461. Changes since last review: rebasing due to conflict, dropping wallet path commit c6a5cd7 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 c6a5cd7 as suggested in discussion, making GetDatabase() const in the earlier commit. Giving more descriptive title like Tree-SHA512: 68cf3b5e9fe0acb3a5cd081086629989f213f1904cc344e5775767b56759a7d905b1e1c303afbe40f172ff81bf07f3719b59d8f6ec2de3fdd53cd0e2d220fb25
No description provided.