Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp #19619

ryanofsky · 2020-07-29T13:21:36Z

Get rid of file path handling in wallet application code and move it down to database layer.

There is no change in behavior except for some changed error messages.

Motivation for this change is to make code more understandable, but also to prepare for adding SQLite support in #19077 so SQLite implementation can be contained at the database layer and wallet loading code does not need to become more complicated.

hebasto · 2020-07-29T14:15:38Z

Concept ACK.

DrahtBot · 2020-07-29T14:28:44Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: Get rid of more redundant chain methods #19425 (refactor: Get rid of more redundant chain methods by ryanofsky)
Replace boost::filesystem with std::filesystem #19245 ([WIP DONOTMERGE] Replace boost::filesystem with std::filesystem (in c++17) by kiminuo)
wallettool: Add dump and createfromdump commands #19137 (wallettool: Add dump and createfromdump commands by achow101)
refactor: remove ::vpwallets and related global variables #19101 (refactor: remove ::vpwallets and related global variables by ryanofsky)
Don't call lsn_reset in periodic flush #18904 (Don't call lsn_reset in periodic flush by bvbfan)
wallet: upgradewallet fixes and additional tests #18836 (wallet: upgradewallet fixes and additional tests by achow101)
refactor: Remove CAddressBookData::destdata #18608 (refactor: Remove CAddressBookData::destdata by ryanofsky)
External signer support - Wallet Box edition #16546 (External signer support - Wallet Box edition by Sjors)

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.

achow101 · 2020-07-29T19:20:13Z

Could the second commit be split up a bit? It seems to be doing a lot and several functions are being renamed and removed.

From what I understand, the second commit adds DatabaseOptions and DatabaseStatus. Then CreateWalletDatabase is renamed to MakeWalletDatabase and all of the various checks (file existence checks and database verify) are pushed into that function with DatabaseOptions informing the function what to actually do. And lastly MakeWalletDatabase becomes a separate distinct step that callers must do instead of letting CWallet::Create (renamed from CreateWalletFromFile) handle internally.

From this, I think you could add a few more commits:

Introduction of DatabaseOptions and DatabaseStatus to consolidate the current options and status
Moving everything into MakeWalletDatabase
Separating MakeWalletDatabase from CWallet::Create

ryanofsky · 2020-07-29T20:15:39Z

Could the second commit be split up a bit? It seems to be doing a lot and several functions are being renamed and removed.

Sure, I think I can split up like you're suggesting. The main change is introducing the MakeBerkeleyDatabase function. Then that function is called various places to get rid of path operations. So I think introducing MakeBerkeleyDatabase could be one commit, and then this can be followed by more commits calling it from RPC code, and then gui code, and then wallet tool code

ryanofsky · 2020-07-29T20:18:29Z

Oh, also I didn't want this PR to depend on #19099, but this would be a little smaller with #19099 merged frist, because then there would be no need to change interfaces/node and walletinitinterface code here

Rebased 8994145 -> 0e4cb50 (pr/path.2 -> pr/path.3, compare) due to conflict with #19102

luke-jr · 2020-07-31T01:44:03Z

I can see why we'd want to move anything about "wallet.dat" or the "wallet is a directory" stuff into the database layer, but IMO it doesn't make sense to hide the path the wallet is located within...

meshcollider · 2020-07-31T11:46:25Z

Concept ACK, as a logical separation I think it makes sense to have database-format-specific stuff like this at a lower layer than the logical use of a generic database in the rest of the code.

achow101 · 2020-09-01T17:24:41Z

ACK 83a8f51

This removes a source of complexity and indirection that makes it harder to understand path checking code. Path checks will be simplified in upcoming commits. There is no change in behavior in this commit other than a slightly more descriptive error message in `loadwallet` if the default "" wallet can't be found. (The error message is improved more in upcoming commit "wallet: Remove path checking code from loadwallet RPC".)

New function is not currently called but will be called in upcoming commits. It moves database path checking, and existence checking, and already-loaded checking, and verification into a single function so this logic does not need to be repeated all over higher level wallet code, and so higher level code does not need to change when SQLite support is added in bitcoin#19077. This also lets higher level wallet code make fewer assumptions about the contents of wallet directories. This commit just adds the new function and does not change behavior in any way.

No changes in behavior. Just replaces arguments and return types

Checks are now consolidated in MakeBerkeleyDatabase function instead of happening in higher level code. This commit does not change behavior except for error messages which now include more complete information.

No changes in behavior

This commit does not change behavior except for error messages which now include more complete information.

hebasto

ACK 83a8f51, tested on Linux Mint 20 (x86_64).

src/wallet/rpcwallet.cpp

src/wallet/wallet.cpp

src/wallet/load.cpp

src/wallet/salvage.cpp

ryanofsky

Rebased 83a8f51 -> 7bf6dfb (pr/path.8 -> pr/path.9, compare) due to conflict with #19754

Thanks for reviews! Reminder to anyone interested that sqlite PR #19077 currently depends on this. And so far achow and hebasto have acked, and meshcollider, promag, and sjors have concept acked this.

src/wallet/rpcwallet.cpp

ryanofsky · 2020-09-03T18:34:01Z

src/wallet/rpcwallet.cpp

-    if (!location.Exists()) {
-        throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet " + location.GetName() + " not found.");
-    } else if (fs::is_directory(location.GetPath())) {
+    if (fs::symlink_status(path).type() == fs::file_not_found) {


I sort of liked that the functionality was wrapped in one function. This avoids issues exactly like this

Replied #19619 (review)

src/wallet/load.cpp

src/wallet/wallet.cpp

src/wallet/salvage.cpp

achow101 · 2020-09-04T00:25:20Z

ACK 7bf6dfb

hebasto

re-ACk 7bf6dfb

meshcollider

Code re-review and functional test run ACK 7bf6dfb

… rpcwallet.cpp 7bf6dfb wallet: Remove path checking code from bitcoin-wallet tool (Russell Yanofsky) 77d5bb7 wallet: Remove path checking code from createwallet RPC (Russell Yanofsky) a987438 wallet: Remove path checking code from loadwallet RPC (Russell Yanofsky) 8b5e729 refactor: Pass wallet database into CWallet::Create (Russell Yanofsky) 3c815cf wallet: Remove Verify and IsLoaded methods (Russell Yanofsky) 0d94e60 refactor: Use DatabaseStatus and DatabaseOptions types (Russell Yanofsky) b5b4141 wallet: Add MakeDatabase function (Russell Yanofsky) 288b4ff Remove WalletLocation class (Russell Yanofsky) Pull request description: Get rid of file path handling in wallet application code and move it down to database layer. There is no change in behavior except for some changed error messages. Motivation for this change is to make code more understandable, but also to prepare for adding SQLite support in bitcoin#19077 so SQLite implementation can be contained at the database layer and wallet loading code does not need to become more complicated. ACKs for top commit: achow101: ACK 7bf6dfb meshcollider: Code re-review and functional test run ACK 7bf6dfb Tree-SHA512: 23ad18324c9e8947f0cf88a3734c2e9fb25536b2cb4d552cf5d1a4ade320fbffb73bb2d1b3a99585c11630aa7092e0fcfc2dd4fe65b91e3a54161433a5cd13cb

Summary: PR description: > Get rid of file path handling in wallet application code and move it down to database layer. > > There is no change in behavior except for some changed error messages. > > Motivation for this change is to make code more understandable, but also to prepare for adding SQLite support in #19077 so SQLite implementation can be contained at the database layer and wallet loading code does not need to become more complicated. This removes a source of complexity and indirection that makes it harder to understand path checking code. Path checks will be simplified in upcoming commits. There is no change in behavior in this commit other than a slightly more descriptive error message in `loadwallet` if the default "" wallet can't be found. (The error message is improved more in upcoming commit "wallet: Remove path checking code from loadwallet RPC".) This is a backport of [[bitcoin/bitcoin#19619 | core#19619]] [1/8] bitcoin/bitcoin@288b4ff Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10224

Summary: New function is not currently called but will be called in upcoming commits. It moves database path checking, and existence checking, and already-loaded checking, and verification into a single function so this logic does not need to be repeated all over higher level wallet code, and so higher level code does not need to change when SQLite support is added in bitcoin/bitcoin#19077. This also lets higher level wallet code make fewer assumptions about the contents of wallet directories. This commit just adds the new function and does not change behavior in any way. This is a backport of [[bitcoin/bitcoin#19619 | core#19619]] [2/8] bitcoin/bitcoin@b5b4141 Depends on D10224 Test Plan: `ninja` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10225

Summary: No changes in behavior. Just replaces arguments and return types This is a backport of [[bitcoin/bitcoin#19619 | core#19619]] [3/8] bitcoin/bitcoin@0d94e60 Depends on D10225 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10226

Summary: Checks are now consolidated in MakeBerkeleyDatabase function instead of happening in higher level code. This commit does not change behavior except for error messages which now include more complete information. This is a backport of [[bitcoin/bitcoin#19619 | core#19619]] [4/8] bitcoin/bitcoin@3c815cf Depends on D10226 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10227

Summary: No changes in behavior This is a backport of [[bitcoin/bitcoin#19619 | core#19619]] [5/8] bitcoin/bitcoin@8b5e729 Depends on D10227 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10228

Summary: This commit does not change behavior except for error messages which now include more complete information. This is a backport of [[bitcoin/bitcoin#19619 | core#19619]] [6/8] bitcoin/bitcoin@a987438 Depends on D10228 Depends on D10213 (for rpc_createmultisig.py) Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10229

Summary: This commit does not change behavior except for error messages which now include more complete information. This is a backport of [[bitcoin/bitcoin#19619 | core#19619]] [7/8] bitcoin/bitcoin@77d5bb7 Depends on D10229 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10230

Summary: This commit does not change behavior except for error messages which now include more complete information. This concludes backport of [[bitcoin/bitcoin#19619 | core#19619]] [8/8] bitcoin/bitcoin@7bf6dfb Depends on D10230 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10231

fanquake added the Wallet label Jul 29, 2020

fanquake requested a review from achow101 July 29, 2020 13:22

DrahtBot mentioned this pull request Jul 29, 2020

Remove the automatic creation and loading of the default wallet #15454

Merged

DrahtBot mentioned this pull request Jul 30, 2020

Disable fee estimation in blocksonly mode (by removing the fee estimates global) #18766

Merged

DrahtBot added the Needs rebase label Jul 30, 2020

ryanofsky force-pushed the pr/path branch from 8994145 to 0e4cb50 Compare July 30, 2020 23:19

DrahtBot removed the Needs rebase label Jul 31, 2020

This was referenced Jul 31, 2020

Remove mempool global #19556

Merged

refactor: remove ::vpwallets and related global variables #19101

Merged

DrahtBot mentioned this pull request Jul 31, 2020

External signer support - Wallet Box edition #16546

Merged

2 tasks

ryanofsky added 8 commits September 3, 2020 12:24

refactor: Use DatabaseStatus and DatabaseOptions types

0d94e60

No changes in behavior. Just replaces arguments and return types

wallet: Remove Verify and IsLoaded methods

3c815cf

Checks are now consolidated in MakeBerkeleyDatabase function instead of happening in higher level code. This commit does not change behavior except for error messages which now include more complete information.

refactor: Pass wallet database into CWallet::Create

8b5e729

No changes in behavior

wallet: Remove path checking code from loadwallet RPC

a987438

This commit does not change behavior except for error messages which now include more complete information.

wallet: Remove path checking code from createwallet RPC

77d5bb7

This commit does not change behavior except for error messages which now include more complete information.

wallet: Remove path checking code from bitcoin-wallet tool

7bf6dfb

This commit does not change behavior except for error messages which now include more complete information.

hebasto approved these changes Sep 3, 2020

View reviewed changes

DrahtBot added the Needs rebase label Sep 3, 2020

ryanofsky force-pushed the pr/path branch from 83a8f51 to 7bf6dfb Compare September 3, 2020 19:39

ryanofsky commented Sep 3, 2020

View reviewed changes

DrahtBot removed the Needs rebase label Sep 3, 2020

hebasto approved these changes Sep 4, 2020

View reviewed changes

meshcollider reviewed Sep 6, 2020

View reviewed changes

meshcollider merged commit 56d47e1 into bitcoin:master Sep 6, 2020

ryanofsky mentioned this pull request Sep 8, 2020

bugfix: make LoadWallet assigns status always #19919

Merged

ryanofsky mentioned this pull request Oct 15, 2020

refactor: Some wallet cleanups #19980

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp #19619

Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp #19619

ryanofsky commented Jul 29, 2020

hebasto commented Jul 29, 2020

DrahtBot commented Jul 29, 2020 •

edited

Loading

achow101 commented Jul 29, 2020 •

edited

Loading

ryanofsky commented Jul 29, 2020

ryanofsky commented Jul 29, 2020 •

edited

Loading

luke-jr commented Jul 31, 2020

meshcollider commented Jul 31, 2020

achow101 commented Sep 1, 2020

hebasto left a comment

ryanofsky left a comment

ryanofsky Sep 3, 2020

achow101 commented Sep 4, 2020

hebasto left a comment

meshcollider left a comment

Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp #19619

Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp #19619

Conversation

ryanofsky commented Jul 29, 2020

hebasto commented Jul 29, 2020

DrahtBot commented Jul 29, 2020 • edited Loading

Conflicts

achow101 commented Jul 29, 2020 • edited Loading

ryanofsky commented Jul 29, 2020

ryanofsky commented Jul 29, 2020 • edited Loading

luke-jr commented Jul 31, 2020

meshcollider commented Jul 31, 2020

achow101 commented Sep 1, 2020

hebasto left a comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Sep 3, 2020

Choose a reason for hiding this comment

achow101 commented Sep 4, 2020

hebasto left a comment

Choose a reason for hiding this comment

meshcollider left a comment

Choose a reason for hiding this comment

DrahtBot commented Jul 29, 2020 •

edited

Loading

achow101 commented Jul 29, 2020 •

edited

Loading

ryanofsky commented Jul 29, 2020 •

edited

Loading