Don't abort launch when failing to load a future wallet

[Sjors]

The new settings.json introduced in bitcoin/bitcoin#15935 creates a problem when downgrading a node. E.g. try creating a hardware wallet enabled wallet with #4 and then downgrade back to master. It will fail at launch with "Wallet requires newer version of Bitcoin Core". You then have to manually delete the settings.json file.

It would be better if QT continues to load, but without the wallet, when this happens.

[Sjors]

cc @ryanofsky @achow101

[jonasschnelli]

This could be potentially harmful for pruned peers where syncing the chain may cause the wallet to fall behind the prune depth and thus require a fresh sync of the whole chain.

So I’m unsure about this. I think the current behavior (stop on wallet load failure) is preferable and the saner option. One option would be to threat pruned peers different... but sounds meh.

[promag]

@jonasschnelli that's already a problem with multiwallet? When a wallet is needed and loaded it can already require a sync.

[Sjors]

If could be a Yes / No prompt.

[jonasschnelli]

@promag with multiwallet, it is an issue if you manually load or unload a wallet. The GUI should probably warn about that (it gets less critical once we automatically load manual loaded wallets after a restart).

But wallets set to load via the startup arguments should probably never silently unload.

[promag]

Agreed, unnecessary source of panic. A warning or so would do the trick as @Sjors suggest.

[ryanofsky]

Good catch. A similar problem was discussed in bitcoin/bitcoin#19754 (comment) and bitcoin/bitcoin#15454 (comment)
about what should happen if a previously loaded wallet in settings.json could no longer be loaded because it was deleted/moved/unmounted. Different options mentioned there were:

1. Quietly not load the wallet, just logging a warning
2. Quietly not load the wallet, logging a warning, and removing it from settings.json so it doesn't get automatically loaded again in the future.
3. Show a modal prompt like "Wallet <wallet name> could not be loaded because of <reason>, do you want to try to load it next time Bitcoin Core is started?"
4. Load the wallet, but show indications it is having problems, and limit interactions disabling the transaction list or controls that won't work. Allow switching to the wallet in the GUI dropdown and unloading it with the "File -> Close Wallet...", like other wallets to avoid loading it again in the future.

[promag]

What would be the corresponding RPC approach for 4.?

[promag]

I like option 4. but that would require a new wallet state to be checked in lots of places. Maybe it's an easy change like, for instance, add the new check in GetWalletForJSONRPCRequest. I could give it a try if everyone agrees with it.

[ryanofsky]

What would be the corresponding RPC approach for 4.?

I think ideally GUI & RPC behavior match and share the same implementation. So wallet would be listed in listwallets, and could be unloaded with unloadwallet taking a load_on_startup option. Trying to list transactions, or get the balance, or generate addresses would result in RPC errors. (Though maybe showing transactions or balances could theoretically work in external signer case? Unsure.)

I like option 4. but that would require a new wallet state to be checked in lots of places. Maybe it's an easy change like, for instance, add the new check in GetWalletForJSONRPCRequest. I could give it a try if everyone agrees with it.

I think that'd be great, and I'd be interested in seeing this. I think a representation for a wallet that couldn't be loaded might be a CWallet instance with a null database pointer, or a dummy database pointer if null is too big of a change. I'm hazy on what error indication might look like in the GUI. Maybe some kind of error marker next to wallet names in the list, and error details shown when the wallet is selected

[maflcko]

Is this something that should be fixed before the next major release?

[promag]

I have a branch with 4. but it's not pretty, it's based on bitcoin/bitcoin#19980.

I think this is not a blocker, especially because the "fix" is what @Sjors had to do.