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

Instead of automatically creating and loading a default wallet, users should instead explicitly create their wallet or load it on start.

Builds on #19754 which provides the load_on_startup behavior for the GUI.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #18836 (wallet: upgradewallet fixes and additional tests by achow101)

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.

Partial Concept ACK on:

• not creating a default wallet with bitcoind
• storing opened wallets in bitcoin_rw.conf (QT only)

Concept On The Fence about making this a breaking change for bitcoind. I.e. maybe we should still load an existing <datadir>/wallet.dat and <datadir>/wallet.dat, but not create one.

Concept NACK on:

• having QT start with a blank wallet, where the user is instructed to go the file menu and choose Create or Open Wallet
• not loading the default wallet in QT after upgrading: that's a guarantee for user heart attacks

I think the best solution is here is to pull the create wallet UI into the startup wizard, as an additional section. The user can then just click next and it creates the default wallet.

Since it takes at least a few seconds, this gives the RNG a bit more bit time too (I assume the IBD network traffic should generate some extra entropy). (Although that requires starting IBD while the wizard is open.)

An easier but still acceptable solution is a big Create Wallet button in the Overview tab (when no wallet is loaded), as well as point out that they can load an existing wallet (less likely for first time users).

Some issues:

• opening and closing wallets in QT updates bitcoin_rw.conf, which is good but:
  • why use a new field loadedwallet? adding and removing wallet= entries seems cleaner. It won't work for wallets already in bitcoin.conf, but that's fine (we can warn the user in that case).
  • I suggest doing this as soon as the open / close succeeds, rather than at exit

An easier but still acceptable solution is a big Create Wallet button in the Overview tab (when no wallet is loaded), as well as point out that they can load an existing wallet (less likely for first time users).

I've done this.

not loading the default wallet in QT after upgrading: that's a guarantee for user heart attacks

I've changed that back to the original behavior except that it only loads the default wallet if it is available. It will not create it.

• why use a new field loadedwallet? adding and removing wallet= entries seems cleaner. It won't work for wallets already in bitcoin.conf, but that's fine (we can warn the user in that case).

#11082 doesn't let me set the multiple of the same argument.

Much better, thanks. Both bitcoind and QT now load the default wallet if it exists, and the QT create button is more clear.

Can you add screenshots of the UX to the PR description?

The initial download overlay actually hides the wallet functionality, which has the nice side-effect of collecting some entropy. Although I'd rather work on making the initial experience such that users can get started more quickly, that's a whole different story.

<img width="766" alt="schermafbeelding 2019-02-22 om 09 38 29" src="https://user-images.githubusercontent.com/10217/53229854-a2b79000-3685-11e9-8bc6-281dc639f270.png">

When the user hides the sync they see this: <img width="766" alt="schermafbeelding 2019-02-22 om 09 38 55" src="https://user-images.githubusercontent.com/10217/53229898-bb27aa80-3685-11e9-9cac-ebd496244101.png">

A bit ugly, but I think the button makes it super clear what to do. Nit: reverse the order, i.e. suggest creating a wallet and then point out that the user can also load an existing wallet.

Regarding the wallet creation dialog, that can be discussed in #15450, but it's important that we don't overwhelm a first time user with advanced options. One thing though: what to do with the default name, at the risk of bike shedding... <img width="479" alt="schermafbeelding 2019-02-22 om 09 40 37" src="https://user-images.githubusercontent.com/10217/53229990-ed390c80-3685-11e9-95ab-ee1d6c9496da.png">

I suggest we hide the wallet name field for the first wallet, or make it really clear that it's totally optional and that you can't (easily) change it (yet).

As for rw_config, which has been in limbo for quite a long time, let's start with reviewing #14866 by @AkioNak which makes the evaluation of bitcoin.conf (includes) a bit more sane. We should expand the functional test suite to more thoroughly cover the expected behavior. That in turn should make it easier to merge rw_config.

Found a small bug:

src/bitcoin-cli -testnet listwallets
[
]

src/bitcoin-cli -testnet createwallet ""
error code: -4
error message:
Wallet  already exists

concept ACK

Rebased this on top of #15937

Approach ACK 830351b6531eb532830cc6f6523232f16b8958d1. Simple & good change overall

Updated with the changes from #15937

Could use a fresh rebase in light of discussion in bitcoin-core/gui#26

Could use a fresh rebase in light of discussion in bitcoin-core/gui#26

Waiting on #15937 rebase

Code review ACK efd93dec07906b4f28e0a9b5620c2c426c9e80d2 last 3 commits. No changes since last review other than rebase

Code review ACK 546bdce4ece60635a1eb2fa9d46d82ae41aeee24 last 3 commits. No changes since last review other than rebase

I've changed the way that the GUI handles the load_on_start stuff and split it into #19754

@achow101 I know you mentioned #11082 a couple times in the writing of this. Does the upcoming closing of that PR, and the merging of the alternative #15935 affect this?

@Rspigler this PR had since been changed to use load_on_start so #11082 is not needed.

Code review ACK a386fcd605509767b20a90719cf966e093bfa5dd. Only change since last review is rebasing and replacing bbda06f9cd1345aa80301bfc98b1cae4ec231f6d with e8bfa09b1c1888c33acfd5dcebbe136bf0d6fe82 from #19754.

re: #15454#issue-254819021

Instead of automatically creating and loading a default wallet, users should instead explicitly create their wallet or load it on start.

This makes use of the load_on_start feature provided by #15937. The GUI is also changed to always do load on start for all loaded and created wallets.

It would probably be good to update PR description to mention #19754 instead of #15937

Having https://github.com/bitcoin-core/gui/issues/77 implemented would go a long way towards making the "Create Wallet" dialog - which all new users have to deal with after this PR - less intimidating.

#19754 isn't really mentioned anywhere here but the first commit in this PR is from #19754 and can be reviewed there

Code review ACK 952bf2c9eef9d6b2f1a01142191c5f41403ed3de. No changes since last review except in first commit which is its own PR #19754

I guess we can leave #19754 (comment) for another PR. But I wonder what the best approach would be. Currently we have:

auto wallet_client = interfaces::MakeWalletClient(*node.chain, args, args.GetArgs("-wallet"));

Which then calls

std::shared_ptr<CWallet> pwallet = CWallet::CreateWalletFromFile(chain, WalletLocation(walletFile), error, warnings);

So, for this code path, we should add some sort of flag to "create if exists"/"just load".

Alternatively we could filter settings -wallet, but I think that's the best approach.

I guess we can leave #19754 (comment) for another PR. But I wonder what the best approach would be. Currently we have:

Ideally, I don't think it should be handled in the wallet/init.cpp Construct method. That method was only meant to construct WalletClient object early on startup, not to access the filesystem or do anything slow or blocking. Also with #10102, that function runs in the node process, not the wallet process.

More ideally, handling not existing wallets would be done in wallet/load.cpp VerifyWallets function. With #19619 all that would be required is setting the DatabaseOptions::require_existing option and handling DatabaseStatus::FAILED_NOT_FOUND return code. It would be easy to implement by permanently removing non-existing wallets from settings.json there, or by just temporarily ignoring them.

More difficult but nicer, I think, would be to have non-existing wallets not removed from settings.json, and shown in the GUI in a disabled state, grayed out with "Wallet path was not accessible" message. That way user could be informed that wallet wasn't accessible and decide whether or not they want to keep trying to load it in the future.

and shown in the GUI in a disabled state

IMO more appropriate is to show a modal dialog on start (for each non existing wallet) asking if it should be removed/forgotten from settings, and if he chooses to keep it then it could be disabled.

Matter of taste, I guess. I don't love modal dialogs, and if I have a multiple wallets and some of them are on an encrypted mount or external drive which is unavailable, I would rather just see the wallets listed normally, disabled and unavailable, not have to manually click through a bunch of pop-up dialogs confirming serially that I still want to load each wallet in the future.

How would you drop disabled wallets (from settings)?

How would you drop disabled wallets (from settings)?

The same way you drop an enabled wallet from settings. Switch to wallet in drop-down, go to File -> Close Wallet...

Oh, so it would be possible to switch in the combo (not actually disabled) but then WalletFrame/WalletView should say that wallet is not available or something like that and probably disable send/receive/transactions views.

Yes, exactly. I guess the term "disabled" is overloaded. But that's just a suggestion. Other approaches (just silently not loading non-existing wallets, silently not loading and also removing from settings, or prompting the user) all seem reasonable and probably easier to implement.

Error building after #19619 was merged:

wallet/init.cpp: In member function ‘virtual void WalletInit::Construct(NodeContext&) const’:
wallet/init.cpp:112:37: error: ‘WalletLocation’ was not declared in this scope
     if (!args.IsArgSet("wallet") && WalletLocation("").Exists()) {
                                     ^~~~~~~~~~~~~~
wallet/init.cpp:112:37: note: suggested alternative: ‘WalletBatch’
     if (!args.IsArgSet("wallet") && WalletLocation("").Exists()) {
                                     ^~~~~~~~~~~~~~
                                     WalletBatch
Makefile:11482: recipe for target 'wallet/libbitcoin_server_a-init.o' failed

Rebased

Lightly tested ACK c24bb1a09cc608b499fefba2402a497cf504217d

Code review ACK c24bb1a09cc608b499fefba2402a497cf504217d, but I think it would be good to fix up the first commit to not do file I/O in WalletInit::Construct and not hardcode "wallet.dat" berkeley wallet name outside of bdb.cpp, and to call existing MakeWalletDatabase function instead of adding a new HasDefaultWallet function. Suggested changes that I think would be to squash are: 421a5502ec71dff3e2d26c7f63cbd3d8da10a706 (branch) +32/-33 lines.

Also would also be good if the change had release notes. Off the top of my head something like "Bitcoin Core will no longer create an unnamed "" wallet by default when no -wallet=<name> command line or wallet=<name> configuration options are used, but for backwards compatibility if a "" wallet already exists and would have been loaded previously, it will still be loaded. Instead of automatically creating an unnamed unencrypted wallet, the GUI now prompts to create a named, encrypted wallet. Instead of only loading the unnamed "" wallet on startup the GUI loads previously loaded wallets."

Squashed in @ryanofsky's commit. Also added release notes.

Code review ACK d26f0648f1c0d1115dcb8d76e57195032b88f400. Just suggested changes to first commit (reusing MakeWalletDatabase and adding release notes), no changes to second commit

I would really like to see this merged, because I think it's a significant interface change (or at least a notable one) that a lot of other changes can build on:

• GUI improvements like https://github.com/bitcoin-core/gui/issues/77
• Fixing bad behavior of settings.json wallet list recreating removed wallets #19754 (comment)
• Maybe turning off non-explicit wallet create entirely, and making wallet creation require a createwallet call or gui interaction. jb55 reported problems caused by this yesterday in IRC: http://www.erisian.com.au/bitcoin-core-dev/log-2020-09-14.html#l-355
• Test improvements: getting rid of -wallet/createwallet RPC override hacks and "" wallet reliance in in test framework, and switching to explicit named creates. 5d2e85c5d33c3b65c79673cad5d7bc0e5875b107 starts down this road

Will review soon.

also publicly committing to reviewing soon

Manual test and very light code review ACK d26f0648f1c0d1115dcb8d76e57195032b88f400

I manually tested:

• default wallet is loaded on startup, even if "load_on_startup" is not set for the default wallet.
• if no default wallet exists, it's not created on startup
• this doesn't effect whether other wallets are loaded on startup or not.

ACK d26f0648f1c0d1115dcb8d76e57195032b88f400 light code review, debug build, ran tests, did manual testing with testnet, rebased on master, on linux debian.

One edge case seen: If I load a wallet (default or no), shutdown without unloading it, then rm wallet.dat or rm -rf the wallet folder, then restart, a new wallet is created in its place.

It seems possible to remove the additional "-wallet=" extra_arg from a number of the changed tests before finding a couple that fail without it. Passing -wallet without a value also seems fine:

-            ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet="],
-            ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet="],
-            ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet="],
-            ["-whitelist=noban@127.0.0.1", "-wallet="],
+            ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet"],
+            ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet"],
+            ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet"],
+            ["-whitelist=noban@127.0.0.1", "-wallet"],

re: #15454 (comment)

Passing -wallet without a value also seems fine?

Would not suggest this because that syntax more typically is used to set a boolean setting to true than to set a string setting to empty. #16545 would actually make it an error to use -string syntax for future string-only settings and would require -string= or -string=""

I think both versions of:

-            ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet="],
+            ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet"],

are less clear than they could be, which is why the the followup https://github.com/ryanofsky/bitcoin/commit/5d2e85c5d33c3b65c79673cad5d7bc0e5875b107 I mentioned in #15454 (comment) would change these to:

             ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet=" + self.default_wallet_name],

This is fine to go in as-is, as mentioned, we can easily make changes to the release notes when they are collated.

I made some changes to the wallet creation dialog, because I don't think the current iteration is suitable for new users: https://github.com/bitcoin-core/gui/pull/96

Forgot the extended tests... fixed in #19971

re: #15454 (comment) & #15454#pullrequestreview-490569662

It seems possible to remove the additional "-wallet=" extra_arg from a number of the changed tests

#20034 follows this PR up and removes all of these. It also goes further and removes the test framework -wallet extra_args mangling, which is no longer necessary thanks to this PR and #15937