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

achow101 · 2019-02-20T22:30:23Z

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.

DrahtBot · 2019-02-20T23:33:15Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: upgradewallet fixes and additional tests #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.

promag · 2019-02-20T23:58:16Z

src/wallet/init.cpp

@@ -184,8 +184,16 @@ void WalletInit::Construct(InitInterfaces& interfaces) const
        LogPrintf("Wallet disabled!\n");
        return;
    }
-    gArgs.SoftSetArg("-wallet", "");


Unless I'm missing something, this is breaking change. Why not change the behavior from "load or create each -wallet" to "just load each -wallet"? Then to create a wallet there is the option in the GUI and createwallet in RPC.

Sjors · 2019-02-21T09:20:14Z

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

achow101 · 2019-02-21T18:45:11Z

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.

Sjors · 2019-02-22T09:04:02Z

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.

When the user hides the sync they see this:

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...

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

jnewbery · 2019-02-22T13:38:19Z

concept ACK

practicalswift · 2019-02-25T23:32:57Z

src/wallet/init.cpp

@@ -228,10 +240,19 @@ void StopWallets()
 void UnloadWallets()
 {
    auto wallets = GetWallets();
+    std::string loaded_wallets = "";


Nit: Redundant initialisation :-)

luke-jr · 2019-10-21T20:26:26Z

src/wallet/load.cpp

+        if (!loaded_wallets.empty()) {
+            loaded_wallets += ",";
+        }
+        loaded_wallets += wallet->GetName();


What if the wallet name has a comma? Would prefer to see rwconf extended to multiple wallet= fields...

So would I. I would prefer that #11082 implements it that way instead of requiring me to change it as I am not familiar with that code.

luke-jr · 2019-10-21T20:26:35Z

src/wallet/load.cpp

        wallets.pop_back();
        RemoveWallet(wallet);
        UnloadWallet(std::move(wallet));
    }
+    if (gArgs.HaveRWConfigFile()) {
+        gArgs.ModifyRWConfigFile("loadedwallet", loaded_wallets);


Should just use wallet=?

That will cause some problems as is since it would be one string that represents multiple wallets. If/when rwconf allows repeated fields, then it can be wallet=.

Per bitcoin/bitcoin#15454

This changes -wallet setting to only load existing wallets, not create new ones. - Fixes settings.json corner cases reported by sjors & promag: bitcoin-core/gui#95, bitcoin#19754 (comment), bitcoin#19754 (comment) - Prevents accidental creation of wallets reported most recently by jb55 http://www.erisian.com.au/bitcoin-core-dev/log-2020-09-14.html#l-355 - Simplifies behavior after bitcoin#15454. bitcoin#15454 took the big step of disabling creation of the default wallet. This PR extends it to avoid creating other wallets as well. With this change, new wallets just aren't created on startup, instead of sometimes being created, sometimes not. bitcoin#15454 release notes are updated here and are simpler. This change should be targetted for 0.21.0. It's a bug fix and simplifies behavior of the bitcoin#15937 / bitcoin#19754 / bitcoin#15454 features added in 0.21.0.

This changes -wallet setting to only load existing wallets, not create new ones. - Fixes settings.json corner cases reported by sjors & promag: bitcoin-core/gui#95, bitcoin#19754 (comment), bitcoin#19754 (comment) - Prevents accidental creation of wallets reported most recently by jb55 http://www.erisian.com.au/bitcoin-core-dev/log-2020-09-14.html#l-355 - Simplifies behavior after bitcoin#15454. bitcoin#15454 took the big step of disabling creation of the default wallet. This PR extends it to avoid creating other wallets as well. With this change, new wallets just aren't created on startup, instead of sometimes being created, sometimes not. bitcoin#15454 release notes are updated here and are simpler. This change should be targeted for 0.21.0. It's a bug fix and simplifies behavior of the bitcoin#15937 / bitcoin#19754 / bitcoin#15454 features added in 0.21.0.

Since Bitcoin Core will no longer do so; see bitcoin/bitcoin#15454 Tested with bitcoind 0.18.0, 0.20.0, and git dda18e7310a202a8aa46c95279446131659f91c5 (a pre-release 0.21).

01476a8 wallet: Make -wallet setting not create wallets (Russell Yanofsky) Pull request description: This changes `-wallet` setting to only load existing wallets, not create new ones. - Fixes settings.json corner cases reported by sjors & promag: bitcoin-core/gui#95, #19754 (comment), #19754 (comment) - Prevents accidental creation of wallets reported most recently by jb55 http://www.erisian.com.au/bitcoin-core-dev/log-2020-09-14.html#l-355 - Simplifies behavior after #15454. #15454 took the big step of disabling creation of the default wallet. This PR extends it to avoid creating other wallets as well. With this change, new wallets just aren't created on startup, instead of sometimes being created, sometimes not. #15454 release notes are updated here and are simpler. This change should be targeted for 0.21.0. It's a bug fix and simplifies behavior of the #15937 / #19754 / #15454 features added in 0.21.0. --- This PR is implementing the simplest, most basic alternative listed in bitcoin-core/gui#95 (comment). Other improvements mentioned there can build on top of this. ACKs for top commit: achow101: ACK 01476a8 hebasto: re-ACK 01476a8 MarcoFalke: review ACK 01476a8 🏂 Tree-SHA512: 0d50f4e5dfbd04a2efd9fd66c02085a0ed705807bdec1cf5770d0ae8cb6af07080fb81306349937bf66acdb713d03fb35636f6442b650d0820e66cbae09c2f87

01476a8 wallet: Make -wallet setting not create wallets (Russell Yanofsky) Pull request description: This changes `-wallet` setting to only load existing wallets, not create new ones. - Fixes settings.json corner cases reported by sjors & promag: bitcoin-core/gui#95, bitcoin#19754 (comment), bitcoin#19754 (comment) - Prevents accidental creation of wallets reported most recently by jb55 http://www.erisian.com.au/bitcoin-core-dev/log-2020-09-14.html#l-355 - Simplifies behavior after bitcoin#15454. bitcoin#15454 took the big step of disabling creation of the default wallet. This PR extends it to avoid creating other wallets as well. With this change, new wallets just aren't created on startup, instead of sometimes being created, sometimes not. bitcoin#15454 release notes are updated here and are simpler. This change should be targeted for 0.21.0. It's a bug fix and simplifies behavior of the bitcoin#15937 / bitcoin#19754 / bitcoin#15454 features added in 0.21.0. --- This PR is implementing the simplest, most basic alternative listed in bitcoin-core/gui#95 (comment). Other improvements mentioned there can build on top of this. ACKs for top commit: achow101: ACK 01476a8 hebasto: re-ACK 01476a8 MarcoFalke: review ACK 01476a8 🏂 Tree-SHA512: 0d50f4e5dfbd04a2efd9fd66c02085a0ed705807bdec1cf5770d0ae8cb6af07080fb81306349937bf66acdb713d03fb35636f6442b650d0820e66cbae09c2f87

ac64cec gui: create wallet: add advanced section (Sjors Provoost) c99d6f6 gui: create wallet: name placeholder (Sjors Provoost) 5bff825 [gui] create wallet: smarter checkbox toggling (Sjors Provoost) Pull request description: Previously only users who needed a second wallet had to use to the create wallet dialog. With the merge of bitcoin/bitcoin#15454 now all new users have to. I don't think it was user-friendly enough for that. <img width="403" alt="Schermafbeelding 2020-09-18 om 09 41 44" src="https://user-images.githubusercontent.com/10217/93574129-52ef9680-f998-11ea-9a6f-31144f66d3bf.png"> This PR makes a few simple improvements so that new users don't have to think too much: <img width="369" alt="Schermafbeelding 2020-10-15 om 16 45 22" src="https://user-images.githubusercontent.com/10217/96145959-0c914700-0f06-11eb-9526-cf447d841d7a.png"> It's lightly inspired by #77. It would be better if those changes made it into the upcoming release, but this PR is a good start imo. * wallet encryption is no longer checked by default, because such a change in the default needs a separate discussion (fwiw, I suspect it increases the number of users losing access to coins) * watch-only and descriptor wallet stuff is moved to advanced, so new users know they can safely ignore these check boxes * bonus: when you click on "disable private keys" it disables encrypt wallet and checks blank wallet * label changes: see screenshot * tooltip changes: see code diff Note that a blank wallet name isn't allowed in the dialog; I haven't addressed that. _Update 2020-10-30_, dropped the new strings for now: <img width="450" alt="Schermafbeelding 2020-10-30 om 11 26 55" src="https://user-images.githubusercontent.com/10217/97694591-1b99fc80-1aa3-11eb-8b85-e19f1ad5add4.png"> ACKs for top commit: fjahr: Tested ACK ac64cec jonatack: re-ACK ac64cec, per `git diff d393708 ac64cec` only change since my last review is improving the placeholder from "MyWallet" to "Wallet" and dropping the last commit. Tested creating a dozen wallets in signet with different combinations of options and then verifying/comparing their characteristics in the console with getwalletinfo. My remaining caveats are (1) the need for less user surprise by either (a) improving the user info or (b) with less auto-(un)selecting as mentioned in #96 (comment) and (2) I prefer the "Encrypt private keys" and "Watch-only" wording and descriptions below over the current ones; hopefully these can be addressed in a follow-up. hebasto: re-ACK ac64cec ryanofsky: Code review ACK ac64cec. Only changes since last review are tweaking placeholder text and dropping "allow nameless" commit Tree-SHA512: a25f84eb66ee4f99af441d73e33928df9d9cf592177398ef48f0037f5913699e47a162cf1301c83b34501546d43ff4ae12607fd078c5c03b92f573bf7604a9f2

ac64cec gui: create wallet: add advanced section (Sjors Provoost) c99d6f6 gui: create wallet: name placeholder (Sjors Provoost) 5bff825 [gui] create wallet: smarter checkbox toggling (Sjors Provoost) Pull request description: Previously only users who needed a second wallet had to use to the create wallet dialog. With the merge of bitcoin#15454 now all new users have to. I don't think it was user-friendly enough for that. <img width="403" alt="Schermafbeelding 2020-09-18 om 09 41 44" src="https://user-images.githubusercontent.com/10217/93574129-52ef9680-f998-11ea-9a6f-31144f66d3bf.png"> This PR makes a few simple improvements so that new users don't have to think too much: <img width="369" alt="Schermafbeelding 2020-10-15 om 16 45 22" src="https://user-images.githubusercontent.com/10217/96145959-0c914700-0f06-11eb-9526-cf447d841d7a.png"> It's lightly inspired by #77. It would be better if those changes made it into the upcoming release, but this PR is a good start imo. * wallet encryption is no longer checked by default, because such a change in the default needs a separate discussion (fwiw, I suspect it increases the number of users losing access to coins) * watch-only and descriptor wallet stuff is moved to advanced, so new users know they can safely ignore these check boxes * bonus: when you click on "disable private keys" it disables encrypt wallet and checks blank wallet * label changes: see screenshot * tooltip changes: see code diff Note that a blank wallet name isn't allowed in the dialog; I haven't addressed that. _Update 2020-10-30_, dropped the new strings for now: <img width="450" alt="Schermafbeelding 2020-10-30 om 11 26 55" src="https://user-images.githubusercontent.com/10217/97694591-1b99fc80-1aa3-11eb-8b85-e19f1ad5add4.png"> ACKs for top commit: fjahr: Tested ACK ac64cec jonatack: re-ACK ac64cec, per `git diff d393708 ac64cec` only change since my last review is improving the placeholder from "MyWallet" to "Wallet" and dropping the last commit. Tested creating a dozen wallets in signet with different combinations of options and then verifying/comparing their characteristics in the console with getwalletinfo. My remaining caveats are (1) the need for less user surprise by either (a) improving the user info or (b) with less auto-(un)selecting as mentioned in bitcoin-core/gui#96 (comment) and (2) I prefer the "Encrypt private keys" and "Watch-only" wording and descriptions below over the current ones; hopefully these can be addressed in a follow-up. hebasto: re-ACK ac64cec ryanofsky: Code review ACK ac64cec. Only changes since last review are tweaking placeholder text and dropping "allow nameless" commit Tree-SHA512: a25f84eb66ee4f99af441d73e33928df9d9cf592177398ef48f0037f5913699e47a162cf1301c83b34501546d43ff4ae12607fd078c5c03b92f573bf7604a9f2

Since Bitcoin Core will no longer do so; see bitcoin/bitcoin#15454 Tested with all of the following bitcoind versions: 0.17.1 0.19.0.1 0.20.1 0.18.0 0.20.0 v0.21.0rc1 (from git)

Per bitcoin/bitcoin#15454

Includes - coincurve: 13.0.0 -> 15.0.0 - Darkscience Tor onion address update - bitcoin-cli jm_wallet generation - don't copy bitcoin-rpcpassword-privileged as privileged user, instead add the joinmarket user to bitcoin group. Same effect, less complexity. Note, PoLP still obeyed for joinmarket-ob-watcher. Starting with 0.21.0, [bitcoin no longer automatically creates and loads a default wallet](bitcoin/bitcoin#15454). This was being ignored because of [a JoinMarket issue]( JoinMarket-Org/joinmarket-clientserver#812) in CI builds prior to this version. Now a watch-only Bitcoin Core wallet is created in the ExecStartPost.

Summary: No longer create a default wallet. The default wallet will still be loaded if it exists and not other wallets were specified (anywhere, including settings.json, bitcoin.conf, and command line). Tests are updated to be started with `-wallet=` if they need the default wallet. Added test to wallet_startup.py testing that no default wallet is created and that it is loaded if it exists and no other wallets were specified. Tell users how to load or create a wallet when no wallet is loaded This is a backport of [[bitcoin/bitcoin#15454 | core#15454]] and [[bitcoin/bitcoin#19971 | core#19971]] (bug fix) Depends on D10240 Test Plan: `ninja all check check-functional-extended` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10243

Since Bitcoin Core will no longer do so; see bitcoin/bitcoin#15454 Tested with all of the following bitcoind versions: 0.17.1 0.19.0.1 0.20.1 0.18.0 0.20.0 v0.21.0rc1 (from git)

fanquake added the Wallet label Feb 20, 2019

promag reviewed Feb 20, 2019

View reviewed changes

DrahtBot added the Needs rebase label Feb 21, 2019

Sjors mentioned this pull request Feb 21, 2019

Add new bitcoin_rw.conf file that is used for settings modified by this software itself #11082

Closed

achow101 force-pushed the no-default-wallet branch from 5a9ccf2 to 0c61dd9 Compare February 21, 2019 18:41

scravy mentioned this pull request Feb 22, 2019

detect empty wallet or create new wallet dtr-org/unit-e#40

Open

practicalswift reviewed Feb 25, 2019

View reviewed changes

jnewbery added the Revise OP label Jun 7, 2019

achow101 force-pushed the no-default-wallet branch from 0c61dd9 to 342164a Compare October 16, 2019 21:09

DrahtBot added Needs rebase and removed Needs rebase labels Oct 16, 2019

achow101 force-pushed the no-default-wallet branch 2 times, most recently from 112d653 to 4f404be Compare October 21, 2019 18:50

DrahtBot removed the Needs rebase label Oct 21, 2019

luke-jr reviewed Oct 21, 2019

View reviewed changes

jnewbery removed the Revise OP label Oct 21, 2019

achow101 force-pushed the no-default-wallet branch 3 times, most recently from 5bbbcd3 to b7fd6c0 Compare October 23, 2019 20:34

DrahtBot added the Needs rebase label Oct 30, 2019

achow101 force-pushed the no-default-wallet branch from b7fd6c0 to 2e44fcf Compare October 31, 2019 19:21

DrahtBot removed the Needs rebase label Oct 31, 2019

DrahtBot added the Needs rebase label Nov 8, 2019

ryanofsky mentioned this pull request May 21, 2020

Close wallet in prunign mode #19039

Closed

achow101 mentioned this pull request Jun 11, 2020

Add loadwallet and createwallet load_on_startup options #15937

Merged

ryanofsky mentioned this pull request Oct 1, 2020

test: Get rid of default wallet hacks #20034

Merged

jsarenik mentioned this pull request Oct 1, 2020

pyln: Add a fixture for createwallet ElementsProject/lightning#4088

Merged

jsarenik added a commit to jsarenik/lightning that referenced this pull request Oct 1, 2020

pyln: utils:BitcoinD: add -wallet="" option

ca69ec3

Per bitcoin/bitcoin#15454

jsarenik added a commit to jsarenik/lightning that referenced this pull request Oct 1, 2020

pyln: utils:BitcoinD: add -wallet="test" option

cd5508e

Per bitcoin/bitcoin#15454

cdecker pushed a commit to jsarenik/lightning that referenced this pull request Oct 13, 2020

pyln: utils:BitcoinD: add -wallet="test" option

3df395f

Per bitcoin/bitcoin#15454

cdecker pushed a commit to ElementsProject/lightning that referenced this pull request Oct 13, 2020

pyln: utils:BitcoinD: add -wallet="test" option

90acf50

Per bitcoin/bitcoin#15454

ryanofsky mentioned this pull request Oct 19, 2020

wallet: Make -wallet setting not create wallets #20186

Merged

hebasto mentioned this pull request Oct 25, 2020

Fix main window geometry save/restore bitcoin-core/gui#107

Closed

pinheadmz mentioned this pull request Oct 26, 2020

wid primary should be password-protected kyokan/bob-wallet#236

Closed

This was referenced Nov 22, 2020

Bitcoin Core v0.21 compatibility GlacierProtocol/GlacierProtocol#80

Open

Release notes: remove mention of default wallet creation #20513

Closed

vibhaa pushed a commit to spider-pcn/lightning that referenced this pull request Mar 24, 2021

pyln: utils:BitcoinD: add -wallet="test" option

98923c7

Per bitcoin/bitcoin#15454

pavitthrap mentioned this pull request Mar 31, 2021

Bitcoind: Need to explicitly create wallet for tests stacks-network/stacks-core#2559

Closed

achow101 mentioned this pull request Oct 6, 2021

Proposed Timeline for Legacy Wallet and BDB removal #20160

Open

46 tasks

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 the automatic creation and loading of the default wallet #15454

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

achow101 commented Feb 20, 2019 •

edited

Loading

DrahtBot commented Feb 20, 2019 •

edited

Loading

promag Feb 20, 2019

Sjors commented Feb 21, 2019 •

edited

Loading

achow101 commented Feb 21, 2019 •

edited

Loading

Sjors commented Feb 22, 2019

jnewbery commented Feb 22, 2019

practicalswift Feb 25, 2019

luke-jr Oct 21, 2019

achow101 Oct 21, 2019

luke-jr Oct 21, 2019

achow101 Oct 21, 2019

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

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

Conversation

achow101 commented Feb 20, 2019 • edited Loading

DrahtBot commented Feb 20, 2019 • edited Loading

Conflicts

promag Feb 20, 2019

Choose a reason for hiding this comment

Sjors commented Feb 21, 2019 • edited Loading

achow101 commented Feb 21, 2019 • edited Loading

Sjors commented Feb 22, 2019

jnewbery commented Feb 22, 2019

practicalswift Feb 25, 2019

Choose a reason for hiding this comment

luke-jr Oct 21, 2019

Choose a reason for hiding this comment

achow101 Oct 21, 2019

Choose a reason for hiding this comment

luke-jr Oct 21, 2019

Choose a reason for hiding this comment

achow101 Oct 21, 2019

Choose a reason for hiding this comment

achow101 commented Feb 20, 2019 •

edited

Loading

DrahtBot commented Feb 20, 2019 •

edited

Loading

Sjors commented Feb 21, 2019 •

edited

Loading

achow101 commented Feb 21, 2019 •

edited

Loading