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

pull achow101 wants to merge 2 commits into bitcoin:master from achow101:no-default-wallet changing 21 files +96 −45
  1. achow101 commented at 10:30 pm on February 20, 2019: member

    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.

  2. fanquake added the label Wallet on Feb 20, 2019
  3. DrahtBot commented at 11:33 pm on February 20, 2019: member

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

    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.

  4. in src/wallet/init.cpp:119 in 5a9ccf2910 outdated
    183@@ -184,8 +184,16 @@ void WalletInit::Construct(InitInterfaces& interfaces) const
    184         LogPrintf("Wallet disabled!\n");
    185         return;
    186     }
    187-    gArgs.SoftSetArg("-wallet", "");
    


    promag commented at 11:58 pm on February 20, 2019:
    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.
  5. DrahtBot added the label Needs rebase on Feb 21, 2019
  6. Sjors commented at 9:20 am on February 21, 2019: member

    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
  7. achow101 force-pushed on Feb 21, 2019
  8. achow101 commented at 6:45 pm on February 21, 2019: member

    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.

  9. Sjors commented at 9:04 am on February 22, 2019: member

    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:

    0src/bitcoin-cli -testnet listwallets
    1[
    2]
    3
    4src/bitcoin-cli -testnet createwallet ""
    5error code: -4
    6error message:
    7Wallet  already exists
    
  10. jnewbery commented at 1:38 pm on February 22, 2019: member
    concept ACK
  11. in src/wallet/init.cpp:243 in 0c61dd9e71 outdated
    239@@ -228,10 +240,19 @@ void StopWallets()
    240 void UnloadWallets()
    241 {
    242     auto wallets = GetWallets();
    243+    std::string loaded_wallets = "";
    


    practicalswift commented at 11:32 pm on February 25, 2019:
    Nit: Redundant initialisation :-)
  12. jnewbery added the label Revise OP on Jun 7, 2019
  13. achow101 force-pushed on Oct 16, 2019
  14. DrahtBot removed the label Needs rebase on Oct 16, 2019
  15. DrahtBot added the label Needs rebase on Oct 18, 2019
  16. achow101 force-pushed on Oct 21, 2019
  17. achow101 force-pushed on Oct 21, 2019
  18. DrahtBot removed the label Needs rebase on Oct 21, 2019
  19. in src/wallet/load.cpp:120 in 4f404be069 outdated
    115+
    116+        // Write the wallet name to rw conf
    117+        if (!loaded_wallets.empty()) {
    118+            loaded_wallets += ",";
    119+        }
    120+        loaded_wallets += wallet->GetName();
    


    luke-jr commented at 8:26 pm on October 21, 2019:
    What if the wallet name has a comma? Would prefer to see rwconf extended to multiple wallet= fields…

    achow101 commented at 9:10 pm on October 21, 2019:
    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.
  20. in src/wallet/load.cpp:127 in 4f404be069 outdated
    122         wallets.pop_back();
    123         RemoveWallet(wallet);
    124         UnloadWallet(std::move(wallet));
    125     }
    126+    if (gArgs.HaveRWConfigFile()) {
    127+        gArgs.ModifyRWConfigFile("loadedwallet", loaded_wallets);
    


    luke-jr commented at 8:26 pm on October 21, 2019:
    Should just use wallet=?

    achow101 commented at 9:11 pm on October 21, 2019:
    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=.
  21. jnewbery removed the label Revise OP on Oct 21, 2019
  22. achow101 force-pushed on Oct 21, 2019
  23. achow101 force-pushed on Oct 23, 2019
  24. achow101 force-pushed on Oct 23, 2019
  25. DrahtBot added the label Needs rebase on Oct 30, 2019
  26. achow101 force-pushed on Oct 31, 2019
  27. DrahtBot removed the label Needs rebase on Oct 31, 2019
  28. DrahtBot added the label Needs rebase on Nov 8, 2019
  29. achow101 force-pushed on Jun 22, 2020
  30. achow101 commented at 10:46 pm on June 22, 2020: member
    Rebased this on top of #15937
  31. DrahtBot removed the label Needs rebase on Jun 22, 2020
  32. in src/wallet/init.cpp:121 in 6feed9508a outdated
    115@@ -116,6 +116,9 @@ void WalletInit::Construct(NodeContext& node) const
    116         LogPrintf("Wallet disabled!\n");
    117         return;
    118     }
    119-    gArgs.SoftSetArg("-wallet", "");
    120+    WalletLocation default_wallet("wallet.dat");
    121+    if (default_wallet.Exists()) {
    122+        gArgs.SoftSetArg("-wallet", "");
    


    ryanofsky commented at 1:31 pm on June 25, 2020:

    In commit “Do not create default wallet” (6feed9508a225276cc2264b7d19b151a62deecea)

    Was experimenting with this PR and would suggest the following fixup: 973bb7e652c4ce10ee536e9967a88c80f6dbe2e0 (tag). SoftSet doesn’t really work here for a persistent setting. I’ll backport this fixup to #15937 so the diff here will be smaller, and the only code change this should have to make is adding && WalletLocation("").Exists()


    ryanofsky commented at 9:54 pm on June 25, 2020:

    re: #15454 (review)

    Was experimenting with this PR and would suggest the following fixup: 973bb7e (tag)

    Feel free to steal updated version of this PR with fix above squashed and newer changes from 15937: https://github.com/ryanofsky/bitcoin/commits/review.15454.2-edit

  33. ryanofsky approved
  34. ryanofsky commented at 1:38 pm on June 25, 2020: member
    Approach ACK 830351b6531eb532830cc6f6523232f16b8958d1. Simple & good change overall
  35. achow101 force-pushed on Jun 25, 2020
  36. achow101 commented at 10:30 pm on June 25, 2020: member
    Updated with the changes from #15937
  37. DrahtBot added the label Needs rebase on Jul 1, 2020
  38. Sjors commented at 7:24 pm on July 2, 2020: member
    Could use a fresh rebase in light of discussion in bitcoin-core/gui#26
  39. achow101 commented at 7:30 pm on July 2, 2020: member

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

    Waiting on #15937 rebase

  40. achow101 force-pushed on Jul 6, 2020
  41. DrahtBot removed the label Needs rebase on Jul 6, 2020
  42. DrahtBot added the label Needs rebase on Jul 11, 2020
  43. achow101 force-pushed on Jul 11, 2020
  44. ryanofsky approved
  45. ryanofsky commented at 8:51 pm on July 21, 2020: member
    Code review ACK efd93dec07906b4f28e0a9b5620c2c426c9e80d2 last 3 commits. No changes since last review other than rebase
  46. achow101 force-pushed on Jul 27, 2020
  47. DrahtBot removed the label Needs rebase on Jul 27, 2020
  48. DrahtBot added the label Needs rebase on Aug 7, 2020
  49. achow101 force-pushed on Aug 10, 2020
  50. DrahtBot removed the label Needs rebase on Aug 10, 2020
  51. achow101 force-pushed on Aug 11, 2020
  52. ryanofsky approved
  53. ryanofsky commented at 9:42 pm on August 11, 2020: member
    Code review ACK 546bdce4ece60635a1eb2fa9d46d82ae41aeee24 last 3 commits. No changes since last review other than rebase
  54. DrahtBot added the label Needs rebase on Aug 13, 2020
  55. in src/qt/walletframe.cpp:37 in 546bdce4ec outdated
    29@@ -25,9 +30,25 @@ WalletFrame::WalletFrame(const PlatformStyle *_platformStyle, BitcoinGUI *_gui)
    30     walletFrameLayout->setContentsMargins(0,0,0,0);
    31     walletFrameLayout->addWidget(walletStack);
    32 
    33-    QLabel *noWallet = new QLabel(tr("No wallet has been loaded."));
    34+    // hbox for no wallet
    35+    QGroupBox* no_wallet_group = new QGroupBox(walletStack);
    36+    QVBoxLayout* no_wallet_layout = new QVBoxLayout(no_wallet_group);
    37+
    38+    QLabel *noWallet = new QLabel(tr("No wallet has been loaded.\nGo to File > Open Wallet to load a wallet.\n- OR -"));
    


    flack commented at 7:12 pm on August 13, 2020:
    wouldn’t it be nicer to just make a open_wallet_button here like it is done below for the create button? That would save the user a bit of mousing around. Also, it would remove the headache of making sure that File > Open Wallet stays consistent with how the menu items are actually called

    achow101 commented at 7:59 pm on August 13, 2020:

    It would, but the way that opening wallets works now would require more extensive changes with code that I’m not too familiar with.

    Someone who is more familiar with the GUI can make this nicer in the future.


    flack commented at 10:07 pm on August 13, 2020:
    if this were Javascript, I would simply make a button that clicks the menu item. But I guess that won’t fly in Qt :-)

    achow101 commented at 10:31 pm on August 13, 2020:
    The menu item doesn’t open a selection dialog, it opens a submenu listing all of the wallets. That doesn’t really work here.

    promag commented at 2:11 pm on August 14, 2020:
    Do you think we should replace the submenu with a dialog?

    achow101 commented at 3:33 pm on August 14, 2020:
    I suppose this could just be a dropdown listing all of the available wallets. But, as I said, I leave this up to someone more familiar with the GUI to do.

    flack commented at 3:45 pm on August 14, 2020:
    if it were up to me I’d say a simple filepicker would totally suffice. It’s self-explanatory and flexible, and the more fancy stuff is still available from the menu for those who want it

    achow101 commented at 3:52 pm on August 14, 2020:
    We would need #15204 to do a file picker type thing.

    flack commented at 3:55 pm on August 14, 2020:
    ah, sorry, I somehow thought that was already merged

    promag commented at 11:47 pm on August 14, 2020:
  56. achow101 force-pushed on Aug 13, 2020
  57. DrahtBot removed the label Needs rebase on Aug 13, 2020
  58. achow101 force-pushed on Aug 15, 2020
  59. achow101 force-pushed on Aug 17, 2020
  60. achow101 commented at 8:36 pm on August 17, 2020: member
    I’ve changed the way that the GUI handles the load_on_start stuff and split it into #19754
  61. achow101 force-pushed on Aug 17, 2020
  62. achow101 force-pushed on Aug 17, 2020
  63. Rspigler commented at 5:50 pm on August 22, 2020: contributor
    @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?
  64. achow101 commented at 8:42 pm on August 22, 2020: member
    @Rspigler this PR had since been changed to use load_on_start so #11082 is not needed.
  65. ryanofsky approved
  66. ryanofsky commented at 10:22 pm on August 24, 2020: member

    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

  67. DrahtBot added the label Needs rebase on Aug 31, 2020
  68. achow101 force-pushed on Aug 31, 2020
  69. Sjors commented at 6:29 pm on August 31, 2020: member
    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.
  70. DrahtBot removed the label Needs rebase on Aug 31, 2020
  71. ryanofsky commented at 0:13 am on September 2, 2020: member
    #19754 isn’t really mentioned anywhere here but the first commit in this PR is from #19754 and can be reviewed there
  72. ryanofsky approved
  73. ryanofsky commented at 0:24 am on September 2, 2020: member
    Code review ACK 952bf2c9eef9d6b2f1a01142191c5f41403ed3de. No changes since last review except in first commit which is its own PR #19754
  74. promag commented at 0:56 am on September 3, 2020: member

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

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

    Which then calls

    0std::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.

  75. ryanofsky commented at 2:26 am on September 3, 2020: member

    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.

  76. promag commented at 2:32 am on September 3, 2020: member

    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.

  77. ryanofsky commented at 2:41 am on September 3, 2020: member
    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.
  78. promag commented at 2:43 am on September 3, 2020: member
    How would you drop disabled wallets (from settings)?
  79. ryanofsky commented at 2:47 am on September 3, 2020: member

    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…

  80. promag commented at 2:51 am on September 3, 2020: member
    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.
  81. ryanofsky commented at 2:59 am on September 3, 2020: member
    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.
  82. DrahtBot added the label Needs rebase on Sep 3, 2020
  83. achow101 force-pushed on Sep 3, 2020
  84. DrahtBot removed the label Needs rebase on Sep 3, 2020
  85. meshcollider commented at 3:34 am on September 7, 2020: contributor

    Error building after #19619 was merged:

    0wallet/init.cpp: In member function virtual void WalletInit::Construct(NodeContext&) const:
    1wallet/init.cpp:112:37: error: WalletLocation was not declared in this scope
    2     if (!args.IsArgSet("wallet") && WalletLocation("").Exists()) {
    3                                     ^~~~~~~~~~~~~~
    4wallet/init.cpp:112:37: note: suggested alternative: WalletBatch
    5     if (!args.IsArgSet("wallet") && WalletLocation("").Exists()) {
    6                                     ^~~~~~~~~~~~~~
    7                                     WalletBatch
    8Makefile:11482: recipe for target 'wallet/libbitcoin_server_a-init.o' failed
    
  86. achow101 commented at 5:34 am on September 7, 2020: member
    Rebased
  87. achow101 force-pushed on Sep 7, 2020
  88. meshcollider commented at 1:33 am on September 8, 2020: contributor
    Lightly tested ACK c24bb1a09cc608b499fefba2402a497cf504217d
  89. ryanofsky approved
  90. ryanofsky commented at 11:20 pm on September 8, 2020: member

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

  91. Do not create default wallet
    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.
    1bee1e6269
  92. Tell users how to load or create a wallet when no wallet is loaded d26f0648f1
  93. achow101 force-pushed on Sep 9, 2020
  94. achow101 commented at 1:03 am on September 9, 2020: member
    Squashed in @ryanofsky’s commit. Also added release notes.
  95. ryanofsky approved
  96. ryanofsky commented at 1:45 am on September 9, 2020: member
    Code review ACK d26f0648f1c0d1115dcb8d76e57195032b88f400. Just suggested changes to first commit (reusing MakeWalletDatabase and adding release notes), no changes to second commit
  97. ryanofsky commented at 2:21 pm on September 15, 2020: member

    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:

  98. jonatack commented at 2:25 pm on September 15, 2020: member
    Will review soon.
  99. instagibbs commented at 5:00 pm on September 16, 2020: member
    also publicly committing to reviewing soon
  100. jnewbery commented at 10:49 am on September 17, 2020: member

    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.
  101. jonatack commented at 12:33 pm on September 17, 2020: member

    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.

    Screenshot from 2020-09-17 14-51-15

  102. jonatack commented at 12:52 pm on September 17, 2020: member

    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:

    0-            ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet="],
    1-            ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet="],
    2-            ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet="],
    3-            ["-whitelist=noban@127.0.0.1", "-wallet="],
    4+            ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet"],
    5+            ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet"],
    6+            ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet"],
    7+            ["-whitelist=noban@127.0.0.1", "-wallet"],
    
  103. in src/wallet/load.cpp:46 in d26f0648f1
    40@@ -41,10 +41,27 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wal
    41 
    42     chain.initMessage(_("Verifying wallet(s)...").translated);
    43 
    44+    // For backwards compatibility if an unnamed top level wallet exists in the
    45+    // wallets directory, include it in the default list of wallets to load.
    46+    if (!gArgs.IsArgSet("wallet")) {
    


    jonatack commented at 1:00 pm on September 17, 2020:

    Would it better to do:

    0-    if (!gArgs.IsArgSet("wallet")) {
    1+    if (!(gArgs.GetBoolArg("wallet", false)) {
    

    ryanofsky commented at 1:04 pm on September 17, 2020:

    re: #15454 (review)

    Would it better to do:

    0-    if (!gArgs.IsArgSet("wallet")) {
    1+    if (!(gArgs.GetBoolArg("wallet", false)) {
    

    This would be bad because it would cause a -nowallet setting to load the default wallet.

  104. ryanofsky commented at 1:15 pm on September 17, 2020: member

    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:

    0-            ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet="],
    1+            ["-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:

    0             ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet=" + self.default_wallet_name],
    
  105. in doc/release-notes-15454.md:4 in 1bee1e6269 outdated
    0@@ -0,0 +1,6 @@
    1+Wallet
    2+------
    3+
    4+Bitcoin Core will no longer create an unnamed `""` wallet by default when no wallet is specified on the command line or in the configuration files.
    


    instagibbs commented at 5:08 pm on September 17, 2020:

    Bitcoin Core will no longer create an unnamed "" wallet by default.

    Just stop here. It won’t make a default wallet of any kind, whether or not you specify one specifically.


    achow101 commented at 5:09 pm on September 17, 2020:
    I think you can make an unnamed wallet with createwallet

    instagibbs commented at 5:12 pm on September 17, 2020:

    by default

    is the key phrase here. It makes nothing by default.


    achow101 commented at 5:15 pm on September 17, 2020:
    Meh. Release notes can always be edited when they go up on the dev wiki.

    instagibbs commented at 5:19 pm on September 17, 2020:
    Ok we’ll just hope our readers are deficient in English ;P

    flack commented at 5:23 pm on September 17, 2020:
    maybe just leave out “by default”?
  106. meshcollider commented at 11:42 pm on September 17, 2020: contributor
    This is fine to go in as-is, as mentioned, we can easily make changes to the release notes when they are collated.
  107. meshcollider merged this on Sep 18, 2020
  108. meshcollider closed this on Sep 18, 2020

  109. Sjors commented at 8:25 am on September 18, 2020: member
    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
  110. Sjors referenced this in commit 9df44e0b97 on Sep 18, 2020
  111. Sjors commented at 10:41 am on September 18, 2020: member
    Forgot the extended tests… fixed in #19971
  112. Sjors referenced this in commit 165b8fd561 on Sep 18, 2020
  113. Sjors referenced this in commit a5f5374b43 on Sep 18, 2020
  114. fanquake referenced this in commit 967be53aee on Sep 19, 2020
  115. sidhujag referenced this in commit 202a5e5958 on Sep 20, 2020
  116. sidhujag referenced this in commit ecb0d694fd on Sep 20, 2020
  117. domob1812 referenced this in commit 16219bfc2e on Sep 22, 2020
  118. ryanofsky commented at 7:46 pm on September 29, 2020: member

    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

  119. jsarenik referenced this in commit ca69ec385e on Oct 1, 2020
  120. jsarenik referenced this in commit cd5508ebce on Oct 1, 2020
  121. cdecker referenced this in commit 3df395ff9b on Oct 13, 2020
  122. cdecker referenced this in commit 90acf5072d on Oct 13, 2020
  123. ryanofsky referenced this in commit 1044521921 on Oct 19, 2020
  124. ryanofsky referenced this in commit bc2b0d99a5 on Oct 19, 2020
  125. ryanofsky referenced this in commit e368d2bb3b on Oct 21, 2020
  126. ryanofsky referenced this in commit 23bc6999c2 on Oct 21, 2020
  127. ryanofsky referenced this in commit 01476a88a6 on Oct 21, 2020
  128. bitcoinhodler referenced this in commit 66bd08dd72 on Oct 22, 2020
  129. MarcoFalke referenced this in commit 42b66a6b81 on Oct 29, 2020
  130. sidhujag referenced this in commit 48063676f9 on Oct 29, 2020
  131. MarcoFalke referenced this in commit e7986c51bc on Nov 17, 2020
  132. sidhujag referenced this in commit 66e8029eba on Nov 17, 2020
  133. bitcoinhodler referenced this in commit 6c0748add7 on Nov 22, 2020
  134. vibhaa referenced this in commit 98923c7f0d on Mar 24, 2021
  135. nixbitcoin referenced this in commit 82b3c88008 on Jul 8, 2021
  136. nixbitcoin referenced this in commit c5884be654 on Jul 12, 2021
  137. Fabcien referenced this in commit 407828747f on Oct 7, 2021
  138. bitcoinfacts referenced this in commit d4f61a03b2 on Dec 27, 2021
  139. DrahtBot locked this on Feb 15, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me