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

[luke-jr]

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]

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?

[luke-jr]

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

[kristapsk]

Concept ACK, will review / test.

[kristapsk]

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]

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

[kristapsk]

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

[kristapsk]

ACK cc85951

[ryanofsky]

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.

[ryanofsky]

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

Two issues this line:

1. 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).
2. 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;

[DrahtBot]

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.

[ryanofsky]

@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 (comment).

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 bitcoin/bitcoin#23997, pruning, a missing external signer bitcoin/bitcoin#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 could not be loaded because of , 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]

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