Don’t abort launch when failing to load a future wallet #95

issue Sjors openend this issue on September 17, 2020
  1. Sjors commented at 11:37 am on September 17, 2020: member

    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.

  2. Sjors added the label Feature on Sep 17, 2020
  3. Sjors commented at 11:38 am on September 17, 2020: member
    cc @ryanofsky @achow101
  4. Sjors removed the label Feature on Sep 17, 2020
  5. jonasschnelli commented at 11:44 am on September 17, 2020: contributor

    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.

  6. promag commented at 11:47 am on September 17, 2020: contributor
    @jonasschnelli that’s already a problem with multiwallet? When a wallet is needed and loaded it can already require a sync.
  7. Sjors commented at 11:58 am on September 17, 2020: member
    If could be a Yes / No prompt.
  8. jonasschnelli commented at 12:19 pm on September 17, 2020: contributor

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

  9. promag commented at 12:22 pm on September 17, 2020: contributor
    Agreed, unnecessary source of panic. A warning or so would do the trick as @Sjors suggest.
  10. ryanofsky commented at 1:32 pm on September 17, 2020: contributor

    Good catch. A similar problem was discussed in https://github.com/bitcoin/bitcoin/pull/19754#issuecomment-685858578 and https://github.com/bitcoin/bitcoin/pull/15454#issuecomment-686168009 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.
  11. promag commented at 1:35 pm on September 17, 2020: contributor
    What would be the corresponding RPC approach for 4.?
  12. promag commented at 1:39 pm on September 17, 2020: contributor
    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.
  13. ryanofsky commented at 1:58 pm on September 17, 2020: contributor

    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

  14. MarcoFalke commented at 3:45 pm on October 1, 2020: contributor
    Is this something that should be fixed before the next major release?
  15. promag commented at 4:04 pm on October 1, 2020: contributor

    I have a branch with 4. but it’s not pretty, it’s based on https://github.com/bitcoin/bitcoin/pull/19980.

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

  16. MarcoFalke commented at 4:08 pm on October 1, 2020: contributor
    Oh right, a verbose abort is better than a silent ignore. Can be fixed later.
  17. ryanofsky referenced this in commit 1044521921 on Oct 19, 2020
  18. ryanofsky referenced this in commit bc2b0d99a5 on Oct 19, 2020
  19. ryanofsky referenced this in commit e368d2bb3b on Oct 21, 2020
  20. ryanofsky referenced this in commit 23bc6999c2 on Oct 21, 2020
  21. ryanofsky referenced this in commit 01476a88a6 on Oct 21, 2020
  22. MarcoFalke referenced this in commit 42b66a6b81 on Oct 29, 2020
  23. ryanofsky commented at 2:50 pm on October 29, 2020: contributor

    I was going to close this issue since I thought https://github.com/bitcoin/bitcoin/pull/20186 resolved it, but actually https://github.com/bitcoin/bitcoin/pull/20186 would need to be broadened a little to change the “the wallet requires a newer version” error into a warning that skips loading the wallet.

    https://github.com/bitcoin/bitcoin/pull/20186 only shows the warning for wallets which don’t exist, not wallets which fail to load for other reasons. It implements the first of four possible solutions described #95 (comment). We could go with one of the other solutions, or a different fix for this issue. @promag, I’d still be curious to see what you came up with in #95 (comment)

  24. promag commented at 2:59 pm on October 29, 2020: contributor

    I have a branch with 4. but it’s not pretty, it’s based on bitcoin/bitcoin#19980. @ryanofsky hope to push today.

  25. sidhujag referenced this in commit 48063676f9 on Oct 29, 2020
  26. janus referenced this in commit d1ed63a9c2 on Dec 13, 2020
  27. hebasto added the label Feature on Mar 6, 2021
  28. hebasto added the label UX on May 1, 2021
  29. hebasto removed the label Feature on May 1, 2021


Sjors jonasschnelli promag ryanofsky MarcoFalke

Labels
UX

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-23 00:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me