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

promag · 2019-01-12T01:43:13Z

Now with one wallet loaded the selector is visible:

After unloading the wallet the selector is hidden:

jonasschnelli · 2019-01-12T06:17:33Z

utACK b669ba6

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 · 2019-01-14T14:18:48Z

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 · 2019-01-14T14:23:59Z

Thanks @hebasto, will fix.

Is this worth of backport?

promag · 2019-01-14T15:05:04Z

@hebasto unfortunately that's not an easy fix, all wallet RPC call GetWalletForJSONRPCRequest:

bitcoin/src/wallet/rpcwallet.cpp

Lines 56 to 67 in 7633529

    
           std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request) 
        
           { 
        
               std::string wallet_name; 
        
               if (GetWalletNameFromJSONRPCRequest(request, wallet_name)) { 
        
                   std::shared_ptr<CWallet> pwallet = GetWallet(wallet_name); 
        
                   if (!pwallet) throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded"); 
        
                   return pwallet; 
        
               } 
        
               std::vector<std::shared_ptr<CWallet>> wallets = GetWallets(); 
        
               return wallets.size() == 1 || (request.fHelp && wallets.size() > 0) ? wallets[0] : nullptr; 
        
           }

This means that "None" is not really possible unless we define an endpoint for that — "don't default to single wallet".

hebasto · 2019-01-14T15:08:32Z

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

promag · 2019-01-14T15:19:50Z

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 · 2019-01-14T15:26:20Z

@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 · 2019-01-14T16:16:14Z

@hebasto please see/test last commit.

DrahtBot · 2019-01-14T16:50:24Z

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.

src/wallet/rpcwallet.cpp

hebasto · 2019-01-15T18:15:07Z

src/wallet/rpcwallet.cpp

 }

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


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

hebasto · 2019-01-15T18:28:05Z

src/wallet/rpcwallet.h

 std::string HelpRequiringPassphrase(CWallet *);
 void EnsureWalletIsUnlocked(CWallet *);
-bool EnsureWalletIsAvailable(CWallet *, bool avoidException);
+bool EnsureWalletIsAvailable(CWallet *, const JSONRPCRequest& request);


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

Sjors

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.

Sjors · 2019-01-16T17:30:25Z

src/qt/rpcconsole.cpp

        ui->WalletSelector->setCurrentIndex(1);
    }
-    if (ui->WalletSelector->count() > 2) {
+    if (ui->WalletSelector->count() > 1) {


Can we just directly count the number of wallets?

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

Sjors · 2019-01-16T17:32:05Z

src/qt/rpcconsole.cpp

-        if (wallet_index > 0) {
-            wallet_model = ui->WalletSelector->itemData(wallet_index).value<WalletModel*>();
-        }
+        WalletModel* wallet_model = ui->WalletSelector->currentData().value<WalletModel*>();


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

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.

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

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

Yes. It seems more robust.

promag · 2019-01-20T19:15:59Z

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 · 2019-01-21T18:45:00Z

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 · 2019-01-21T18:50:06Z

@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 · 2019-01-21T19:24:25Z

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 · 2019-03-02T15:22:00Z

Needs rebase

luke-jr · 2019-04-17T06:07:54Z

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

luke-jr · 2019-04-17T06:12:57Z

src/wallet/rpcwallet.cpp

 }

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


Seems like GetWalletForJSONRPCRequest is a better place to do this?

DrahtBot · 2019-08-16T14:02:18Z

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.

c279a81 gui: Remove warning "unused variable 'wallet_model'" (João Barbosa) Pull request description: This was part of the abandoned #15150. ACKs for top commit: theStack: utACK c279a81 fanquake: ACK c279a81 - tested wallet loading/unloading in the qt rpc console. Tree-SHA512: 8fbd55c7e213599c7be843b52e960a16cf965b3e01489f426ac3ed9d579d78bb4b2ac230bcccd8abe0397a8b1166ee10e0d685738441a77a5dcb5135c15790fa

…el'" c279a81 gui: Remove warning "unused variable 'wallet_model'" (João Barbosa) Pull request description: This was part of the abandoned bitcoin#15150. ACKs for top commit: theStack: utACK bitcoin@c279a81 fanquake: ACK c279a81 - tested wallet loading/unloading in the qt rpc console. Tree-SHA512: 8fbd55c7e213599c7be843b52e960a16cf965b3e01489f426ac3ed9d579d78bb4b2ac230bcccd8abe0397a8b1166ee10e0d685738441a77a5dcb5135c15790fa

promag added 2 commits January 12, 2019 01:28

gui: Remove warning "unused variable 'wallet_model'"

b669ba6

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

fca4bcd

fanquake added the GUI label Jan 12, 2019

promag mentioned this pull request Jan 12, 2019

gui: Add WalletController #15101

Merged

hebasto reviewed Jan 14, 2019

View reviewed changes

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

wip: rpc: Add /nowallet endpoint

e7a1ff1

promag force-pushed the 2019-01-consolewalletselector branch from 7db2876 to e7a1ff1 Compare January 14, 2019 23:52

hebasto requested changes Jan 15, 2019

View reviewed changes

Sjors reviewed Jan 16, 2019

View reviewed changes

promag mentioned this pull request Jan 20, 2019

Wallet: Remove "-Wunused-variable" warning while build with "--disable-wallet" option #15217

Closed

DrahtBot added the Needs rebase label Mar 2, 2019

luke-jr reviewed Apr 17, 2019

View reviewed changes

DrahtBot added the Up for grabs label Aug 16, 2019

DrahtBot closed this Aug 16, 2019

laanwj removed the Needs rebase label Oct 24, 2019

Sjors mentioned this pull request Jan 16, 2020

gui: Remove warning "unused variable 'wallet_model'" #17939

Merged

fanquake removed the Up for grabs label Jan 17, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Uh oh!

Conversation

promag commented Jan 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Jan 12, 2019

Uh oh!

hebasto commented Jan 14, 2019

Uh oh!

promag commented Jan 14, 2019

Uh oh!

promag commented Jan 14, 2019

Uh oh!

hebasto commented Jan 14, 2019

Uh oh!

promag commented Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jan 14, 2019

Uh oh!

promag commented Jan 14, 2019

Uh oh!

DrahtBot commented Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Jan 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Jan 21, 2019

Uh oh!

promag commented Jan 21, 2019

Uh oh!

Sjors commented Jan 21, 2019

Uh oh!

DrahtBot commented Mar 2, 2019

Uh oh!

luke-jr commented Apr 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Aug 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

promag commented Jan 12, 2019 •

edited

Loading

promag commented Jan 14, 2019 •

edited

Loading

DrahtBot commented Jan 14, 2019 •

edited

Loading

promag commented Jan 20, 2019 •

edited

Loading