gui: Show wallet selector on console window if there are wallets loaded #15150

promag commented at 1:43 AM on January 12, 2019: member

Now with one wallet loaded the selector is visible: <img width="563" alt="screenshot 2019-01-12 at 01 38 34" src="https://user-images.githubusercontent.com/3534524/51067539-1b4d1a80-160b-11e9-931c-4240f5851172.png">

After unloading the wallet the selector is hidden: <img width="478" alt="screenshot 2019-01-12 at 01 39 06" src="https://user-images.githubusercontent.com/3534524/51067568-4c2d4f80-160b-11e9-932d-ef8c2ae8c8ce.png">

gui: Remove warning "unused variable 'wallet_model'" b669ba6bd8

gui: Show wallet selector on console window if there are wallets loaded fca4bcdc32

fanquake added the label GUI on Jan 12, 2019

jonasschnelli commented at 6:17 AM on January 12, 2019: contributor

utACK b669ba6bd86a7808e4692bb0cf692b1c95d8d32d

Clarification: in master, if you start with no wallet, then load a wallet, you'll miss the selector between "none" and the "loaded wallet".

This seems to be a bugfix.

hebasto commented at 2:18 PM on January 14, 2019: member

Tested on Linux Mint 19.1. Detected behavior change:

In master having "none" in selector implies an error output of "getwalletinfo":

Wallet file not specified (must request wallet RPC through /wallet/<filename> uri-path). (code -19)

With this PR:

bitcoin-qt -nowallet

The selector value is "none".

Then in Console:

loadwallet "test_alpha"

Note the selector remains "none". But "getwalletinfo" returns data about "test_alpha" wallet.

promag commented at 2:23 PM on January 14, 2019: member

Thanks @hebasto, will fix.

Is this worth of backport?

promag commented at 3:05 PM on January 14, 2019: member

@hebasto unfortunately that's not an easy fix, all wallet RPC call GetWalletForJSONRPCRequest: https://github.com/bitcoin/bitcoin/blob/76335298f427de7d7ce58ced177518a7905706cc/src/wallet/rpcwallet.cpp#L56-L67 This means that "None" is not really possible unless we define an endpoint for that — "don't default to single wallet".

hebasto commented at 3:08 PM on January 14, 2019: member

@promag maybe update the wallet selector when the first wallet has been loaded?

promag commented at 3:19 PM on January 14, 2019: member

I don't think we should mimic the RPC server behaviour. It was done like that to avoid breaking changes and so I think the GUI should be explicit. I also think it should be possible to have a no-wallet RPC endpoint — one that no wallet is automatically used. That said I think your test case should be fixed in a separate PR.

hebasto commented at 3:26 PM on January 14, 2019: member

@promag

the GUI should be explicit.

Right.

That said I think your test case should be fixed in a separate PR.

Could this test case be mentioned in this PR description?

promag commented at 4:16 PM on January 14, 2019: member

@hebasto please see/test last commit.

DrahtBot commented at 4:50 PM on January 14, 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:

#15492 ([rpc] remove deprecated generate method by Sjors)
#15157 (rpc: Bumpfee units change, satoshis to BTC by benthecarman)
#10615 (RPC: Allow rpcauth configs to specify a 4th parameter naming a specific wallet by luke-jr)

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.

wip: rpc: Add /nowallet endpoint e7a1ff13ef

in src/wallet/rpcwallet.cpp:87 in 7db2876cdd outdated

  86 | -    if (!HasWallets()) {
  87 | +    if (request.fHelp) return false;
  88 | +    if (!HasWallets() || request.URI == "/nowallet") {
  89 |          throw JSONRPCError(
  90 | -            RPC_METHOD_NOT_FOUND, "Method not found (wallet method is disabled because no wallet is loaded)");
  91 | +            RPC_METHOD_NOT_FOUND, "Method not found (wallet method is disabled because no wallet loaded or selected)");

hebasto commented at 7:04 PM on January 14, 2019:

Missed "is" causes test failures.

promag commented at 7:06 PM on January 14, 2019:

Ops

promag force-pushed on Jan 14, 2019

in src/wallet/rpcwallet.cpp:81 in e7a1ff13ef

  77 | @@ -73,13 +78,13 @@ std::string HelpRequiringPassphrase(CWallet * const pwallet)
  78 |          : "";
  79 |  }
  80 |  
  81 | -bool EnsureWalletIsAvailable(CWallet * const pwallet, bool avoidException)
  82 | +bool EnsureWalletIsAvailable(CWallet * const pwallet, const JSONRPCRequest& request)

hebasto commented at 6:15 PM on January 15, 2019:

bool EnsureWalletIsAvailable(const CWallet* const pwallet, const JSONRPCRequest& request)

luke-jr commented at 6:12 AM on April 17, 2019:

Seems like GetWalletForJSONRPCRequest is a better place to do this?

in src/wallet/rpcwallet.h:29 in e7a1ff13ef

  25 | @@ -26,7 +26,7 @@ std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& reques
  26 |  
  27 |  std::string HelpRequiringPassphrase(CWallet *);
  28 |  void EnsureWalletIsUnlocked(CWallet *);
  29 | -bool EnsureWalletIsAvailable(CWallet *, bool avoidException);
  30 | +bool EnsureWalletIsAvailable(CWallet *, const JSONRPCRequest& request);

hebasto commented at 6:28 PM on January 15, 2019:

bool EnsureWalletIsAvailable(const CWallet* const pwallet, const JSONRPCRequest& request);

hebasto changes_requested

in src/qt/rpcconsole.cpp:706 in fca4bcdc32 outdated

 702 | @@ -703,7 +703,7 @@ void RPCConsole::addWallet(WalletModel * const walletModel)
 703 |          // First wallet added, set to default so long as the window isn't presently visible (and potentially in use)
 704 |          ui->WalletSelector->setCurrentIndex(1);
 705 |      }
 706 | -    if (ui->WalletSelector->count() > 2) {
 707 | +    if (ui->WalletSelector->count() > 1) {

Sjors commented at 5:30 PM on January 16, 2019:

Can we just directly count the number of wallets?

promag commented at 7:14 PM on January 20, 2019:

This change doesn't make the code worse. I think this can be improved later by refactoring to take advantage of the new WalletController.

in src/qt/rpcconsole.cpp:910 in b669ba6bd8 outdated

 910 |  #ifdef ENABLE_WALLET
 911 | -        const int wallet_index = ui->WalletSelector->currentIndex();
 912 | -        if (wallet_index > 0) {
 913 | -            wallet_model = ui->WalletSelector->itemData(wallet_index).value<WalletModel*>();
 914 | -        }
 915 | +        WalletModel* wallet_model = ui->WalletSelector->currentData().value<WalletModel*>();

Sjors commented at 5:32 PM on January 16, 2019:

Why it safe to remove if (wallet_index > 0) {?

promag commented at 7:12 PM on January 20, 2019:

It's safe because what we want is the current Qt::UserRole data as WalletModel*. If there is none then currentData().value<WalletModel*>() returns nullptr, which is the case for the fist item.

hebasto commented at 7:17 PM on January 20, 2019:

Why was {} syntax for wallet_model initialization dropped? It could prevent an implicit data conversion.

promag commented at 8:39 PM on January 20, 2019:

You mean this should be WalletModel* wallet_model{ui->WalletSelector->currentData().value<WalletModel*>()};?

hebasto commented at 9:47 PM on January 20, 2019:

Yes. It seems more robust.

Sjors commented at 5:35 PM on January 16, 2019: member

Concept ACK for the dropdown change, but I don't understand the need for RPC "/nowallet". Isn't the absence of a wallet path (so no -rpcwallet=...) enough for the RPC server to understand there's no wallet context?

Suggest renaming PR and commit to: gui: only show wallet selector in console window if any wallets are loaded, which it more clear this is a bug fix.

promag commented at 7:15 PM on January 20, 2019: member

but I don't understand the need for RPC "/nowallet" @Sjors it's just an endpoint where there is no automagic wallet selection.

The idea is that you should explicit select the wallet and if you select "(none)" then wallet commands should fail, IMO.

Sjors commented at 6:45 PM on January 21, 2019: member

I agree that wallet commands should fail if you select (none). But I don't understand why we need this /nowallet path, that feels like a hack.

When using bitcoin-cli wallet commands without specifying the wallet, the RPC throws Wallet file not specified (must request wallet RPC through /wallet/<filename> uri-path).. That seems like a better approach; not sure how to translate that to the console UI.

promag commented at 6:50 PM on January 21, 2019: member

@Sjors so /wallet/ explicitly selects the wallet but / automatically selects a wallet if there's only one loaded, so we need to somehow avoid that without breaking the current behavior.

Sjors commented at 7:24 PM on January 21, 2019: member

Mmm, I kind of like that behavior though. Running a node with one wallet shouldn't require -rpcwallet as that's tedious.

Is there a scenario where this leads to the wrong result?

It might be better to hide the "no wallet" option when that is the case.

DrahtBot commented at 3:22 PM on March 2, 2019: member

Needs rebase

DrahtBot added the label Needs rebase on Mar 2, 2019

luke-jr commented at 6:07 AM on April 17, 2019: member

@Sjors Users might select "(none)" wallet with the intent of protecting their wallet from arbitrary commands they are told to run...

Agree /nowallet seems hacky. The original implementation just passed the CWallet pointer... but it got hackily turned into a URI encoding instead. :/

DrahtBot commented at 2:02 PM on August 16, 2019: member

There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.

DrahtBot added the label Up for grabs on Aug 16, 2019

DrahtBot closed this on Aug 16, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

fanquake removed the label Up for grabs on Jan 17, 2020

fanquake referenced this in commit c20fbb7be8 on Jan 17, 2020

PastaPastaPasta referenced this in commit 81f118f3d3 on Sep 17, 2021

thelazier referenced this in commit 092bb8a134 on Sep 25, 2021

DrahtBot locked this on Feb 15, 2022