Bugfix: Allow the user to start anyway when loading a wallet errors #236

luke-jr commented at 1:08 AM on March 3, 2021: member

Right now, if a GUI user leaves a wallet loaded and something happens to make it error at startup, they're stuck.

This turns the error dialog into a question about starting without the wallet instead.

luke-jr commented at 1:09 AM on March 3, 2021: member

~~Not sure why GitHub thinks this has conflicts - it doesn't.~~

~~Mainly this is a WIP because it uses a hacky global to avoid trying to load the wallets the user has chosen to skip... Suggestions for a good way to deal with that?~~

hebasto added the label UX on May 9, 2021

hebasto added the label Wallet on May 9, 2021

luke-jr force-pushed on Jul 30, 2021

DrahtBot added the label Needs rebase on Dec 13, 2021

luke-jr force-pushed on Jan 10, 2022

luke-jr marked this as ready for review on Jan 10, 2022

luke-jr commented at 1:52 AM on January 10, 2022: member

Rebased, which made it practical to drop the global hack. This seems ready for review now.

DrahtBot removed the label Needs rebase on Jan 10, 2022

kristapsk commented at 6:14 PM on March 6, 2022: contributor

Concept ACK, will review / test.

kristapsk commented at 4:18 PM on March 8, 2022: contributor

Still gives me error message without ability to continue, not question.

$ ./src/qt/bitcoin-qt -version
Bitcoin Core versija v22.99.0-fb3ea0ad3a87
Copyright (C) 2009-2021 The Bitcoin Core developers

Please contribute if you find Bitcoin Core useful. Visit
<https://bitcoincore.org/> for further information about the software.
The source code is available from <https://github.com/bitcoin/bitcoin>.

This is experimental software.
Distributed under the MIT software license, see the accompanying file COPYING
or <https://opensource.org/licenses/MIT>

$ git log | head -n 1
commit fb3ea0ad3a8788e023b9ba7237c26fc8ef0dba79
$ ./src/qt/bitcoin-qt -signet
Error: Error loading /fast_home/neonz/.bitcoin/signet/wallets/jmw/wallet.dat: Wallet requires newer version of Bitcoin Core

luke-jr commented at 10:53 PM on March 8, 2022: member

Ah, it only caught some errors. Pushed a revision that should catch the rest.

luke-jr force-pushed on Mar 8, 2022

util: Add a ForceSetArgV that can handle non-strings bc89fbe723

interfaces/chain: Provide a way to ask the user a question during init 6c12879048

luke-jr force-pushed on Mar 8, 2022

in src/wallet/load.cpp:29 in 243dc118e1 outdated

  22 | @@ -22,6 +23,17 @@
  23 |  #include <system_error>
  24 |  
  25 |  namespace wallet {
  26 | +
  27 | +bool HandleWalletLoadError(interfaces::Chain& chain, const std::string& wallet_file, const bilingual_str& error_string)
  28 | +{
  29 | +    if (!chain.initQuestion(error_string + Untranslated("\n\n") + _("Continue without wallet?"), error_string, _("Error"), CClientUIInterface::MSG_ERROR | CClientUIInterface::MODAL | CClientUIInterface::BTN_OK | CClientUIInterface::BTN_ABORT)) {

kristapsk commented at 2:35 AM on March 9, 2022:

Maybe better question message would be "Continue without this wallet?", so that is clear that only wallet having error is not being loaded, not all other wallets or wallet functionality disabled altogether?

luke-jr commented at 3:35 AM on March 9, 2022:

Sounds good, done

kristapsk commented at 2:36 AM on March 9, 2022: contributor

Apart from review comment above, ACK, working for me (tested scenario with trying to load external signer wallet without external signer support compiled into Bitcoin Core).

luke-jr force-pushed on Mar 9, 2022

kristapsk approved

kristapsk commented at 8:21 AM on March 9, 2022: contributor

ACK cc85951352a4ebf8464f415b2a2ec66b7773fbd5

hebasto requested review from ryanofsky on Apr 4, 2022

in src/wallet/load.cpp:118 in cc85951352 outdated

 115 |          }
 116 |      }
 117 |  
 118 | +    if (modified_wallet_list) {
 119 | +        // Ensure new wallet list overrides commandline options
 120 | +        args.ForceSetArgV("wallet", chain.getRwSetting("wallet"));

ryanofsky commented at 7:22 PM on April 4, 2022:

In commit "Bugfix: GUI: Allow the user to start anyway when loading a wallet errors" (cc85951352a4ebf8464f415b2a2ec66b7773fbd5)

Two issues this line:

This prevents all bitcoin.conf or command line wallets from being loaded if modified_wallet_list is true, because getRwSetting() doesn't return these wallets (it only returns settings.json wallets).
This line has no effect if wallet code is running in a separate process, because ForceSet call is changing wallet process args, but code that runs later in LoadWallet is calling chain.getSettingsList which reads the node process args instead of wallet process args. The code was confused even before this PR, but this change would make it actually not work correctly.

Would suggest a change like the following as a fix:

diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
index 6f900a64375..5c7c66a62ba 100644
--- a/src/interfaces/chain.h
+++ b/src/interfaces/chain.h
@@ -270,6 +270,10 @@ public:
     //! Get list of settings values.
     virtual std::vector<util::SettingsValue> getSettingsList(const std::string& arg) = 0;
 
+    //! Override setting in memory, so future getSetting / GetArg calls return
+    //! specified value. If value is null, will unset any previously forced value.
+    virtual void forceSetting(const std::string& name, const util::SettingsValue& value) = 0;
+
     //! Return <datadir>/settings.json setting value.
     virtual util::SettingsValue getRwSetting(const std::string& name) = 0;
 
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index 364a8406af7..0f2f90c11e5 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -692,6 +692,16 @@ public:
     {
         return gArgs.GetSettingsList(name);
     }
+    void forceSetting(const std::string& name, const util::SettingsValue& value) override
+    {
+        gArgs.LockSettings([&](util::Settings& settings) {
+            if (value.isNull()) {
+                settings.forced_settings.erase(name);
+            } else {
+                settings.forced_settings[name] = value;
+            }
+        });
+    }
     util::SettingsValue getRwSetting(const std::string& name) override
     {
         util::SettingsValue result;
diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp
index ecd832d6224..8a1164ae71d 100644
--- a/src/wallet/load.cpp
+++ b/src/wallet/load.cpp
@@ -66,7 +66,7 @@ bool VerifyWallets(WalletContext& context)
 
     // For backwards compatibility if an unnamed top level wallet exists in the
     // wallets directory, include it in the default list of wallets to load.
-    if (!args.IsArgSet("wallet")) {
+    if (chain.getSetting("wallet").isNull()) {
         DatabaseOptions options;
         DatabaseStatus status;
         bilingual_str error_string;
@@ -86,6 +86,7 @@ bool VerifyWallets(WalletContext& context)
     std::set<fs::path> wallet_paths;
 
     bool modified_wallet_list = false;
+    util::SettingsValue verified_wallets{UniValue::VARR};
     for (const auto& wallet : chain.getSettingsList("wallet")) {
         const auto& wallet_file = wallet.get_str();
         const fs::path path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(wallet_file));
@@ -110,12 +111,14 @@ bool VerifyWallets(WalletContext& context)
                     return false;
                 }
             }
+        } else {
+            verified_wallets.push_back(wallet_file);
         }
     }
 
     if (modified_wallet_list) {
-        // Ensure new wallet list overrides commandline options
-        args.ForceSetArgV("wallet", chain.getRwSetting("wallet"));
+        // Prevent loading any command-line or bitcoin.conf wallets that failed to verify.
+        chain.forceSetting("wallet", verified_wallets);
     }
 
     return true;

ryanofsky commented at 8:09 PM on April 4, 2022: contributor

Nice fix! I think there is a minor problem in the implementation, where if a user chooses not to load any wallet, none of the bitcoin.conf or command line wallets will be loaded even if they were present and could be verified. I suggested a fix for this below.

hebasto renamed this:
~~Bugfix: GUI: Allow the user to start anyway when loading a wallet errors~~
Bugfix: Allow the user to start anyway when loading a wallet errors
on Apr 15, 2022

hebasto added the label Waiting for author on Jun 1, 2022

DrahtBot commented at 9:49 PM on June 12, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin/bitcoin/17783 (util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
#bitcoin/bitcoin/17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
#bitcoin/bitcoin/17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)
#bitcoin/bitcoin/16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

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.

Bugfix: GUI: Allow the user to start anyway when loading a wallet errors 6cbea59a35

luke-jr force-pushed on Sep 5, 2022

ryanofsky commented at 4:47 PM on October 6, 2022: contributor

@luke-jr do you want to respond to suggestions here? Or would you be open to a someone making a new PR?

Status of the PR seems to be that it is up to date, but has a bug that could cause command line and bitcoin.conf wallets to be ignored. I suggested a fix for the bug and some cleanups in #236 (review).

I think there is also a usability problem with this PR in the case of temporary errors where a wallet can't be loaded due to a temporary issue like: background assumeutxo download https://github.com/bitcoin/bitcoin/pull/23997, pruning, a missing external signer https://github.com/bitcoin/bitcoin/pull/22173, an encrypted drive not being mounted, a removable drive not being plugged in, or a version incompatibility that will be resolved by upgrading or downgrading. In these cases it would be useful to have the option to continue starting the GUI and letting the node sync, while temporarily not loading individual wallets that are unavailable, and the current dialog doesn't provide an option to temporarily not load a wallet. The only options are quit or continue without loading the wallet next time. I think it would be a improvement to change the dialog to "Wallet <wallet name> could not be loaded because of <reason>, do you want to try to load it next time Bitcoin Core is started?" with "Yes" "No" buttons (and maybe "Details" and "Abort" buttons off to the side like #379). This was one of four alternatives suggested in #95, and there is more discussion about this issue there.

hebasto removed the label Waiting for author on Dec 6, 2022

hebasto commented at 7:09 PM on December 6, 2022: member

@luke-jr do you want to respond to suggestions here? Or would you be open to a someone making a new PR?

I'm going to close this, and mark "Up for grabs".

hebasto closed this on Dec 6, 2022

hebasto added the label Up for grabs on Dec 6, 2022

bitcoin-core locked this on Dec 6, 2023