New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic Multiwallet GUI support #11383

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@luke-jr
Member

luke-jr commented Sep 21, 2017

This adds a combobox to the GUI main window and debug window to select the wallet being viewed/used.

Show outdated Hide outdated src/qt/bitcoin.cpp Outdated
@@ -462,6 +462,15 @@ void BitcoinGUI::createToolBars()
toolbar->addAction(receiveCoinsAction);
toolbar->addAction(historyAction);
overviewAction->setChecked(true);
#ifdef ENABLE_WALLET

This comment has been minimized.

@jonasschnelli

jonasschnelli Sep 22, 2017

Member

this #ifdef could be avoided/removed, right?

@jonasschnelli

jonasschnelli Sep 22, 2017

Member

this #ifdef could be avoided/removed, right?

This comment has been minimized.

@luke-jr

luke-jr Sep 22, 2017

Member

Maybe.

@luke-jr

luke-jr Sep 22, 2017

Member

Maybe.

This comment has been minimized.

@jonasschnelli

jonasschnelli Feb 12, 2018

Member

Has to bee there I guess because of bool setCurrentWallet(const QString& name); being only defined if ENABLE_WALLET.

@jonasschnelli

jonasschnelli Feb 12, 2018

Member

Has to bee there I guess because of bool setCurrentWallet(const QString& name); being only defined if ENABLE_WALLET.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Sep 22, 2017

Member

Nice!
Works pretty good (played around with it a bit).
I think this may be a first step.

Conceptual issues:

  • The Combox box without a direct user feedback may lead to selecting the wrong wallet. Android dual/multi-sim works against this by always displaying an icon for the SIM card the action will be executed with. Not 1:1 applicable here... but users should get A) a feedback after switching wallets (are you sure, etc.), B) better visual distinction.
  • Having the HD / encryption status in the status-bar seems not to be the right place when running multiwallet. It is basically on the same level then the QComboBox.
  • Wallet name tied to the filename may be not the best choice. Would something speak against having a dedicated walletname per wallet, .. or would this confuse?

Ideas:

  • Ideally we would have a wallet-navigation-drawer at the left side that consume a more prominent space and make it move obvious which wallet is used.
  • Critical actions like send-coins should maybe remind the user in the confirmation dialog which wallet it will use.
  • Users eventually should choose an (coloured) icon per wallet (give a pre-selection of 12 icons) which then will be always visible on a top window level and or displayed when executing critical actions (showing addresses, sending coins, etc.)

Code:

  • I don't see why the RPC auth argument would be necessary at this point (but I respect your comment that we should discuss that in #10615)
  • I dislike the #Ifdef ENABLE_WALLET clustering. I would reduce it to a minimum and runtime bypass where possible. Especially the #Ifdefs in RPC / http low level code is ugly.
Member

jonasschnelli commented Sep 22, 2017

Nice!
Works pretty good (played around with it a bit).
I think this may be a first step.

Conceptual issues:

  • The Combox box without a direct user feedback may lead to selecting the wrong wallet. Android dual/multi-sim works against this by always displaying an icon for the SIM card the action will be executed with. Not 1:1 applicable here... but users should get A) a feedback after switching wallets (are you sure, etc.), B) better visual distinction.
  • Having the HD / encryption status in the status-bar seems not to be the right place when running multiwallet. It is basically on the same level then the QComboBox.
  • Wallet name tied to the filename may be not the best choice. Would something speak against having a dedicated walletname per wallet, .. or would this confuse?

Ideas:

  • Ideally we would have a wallet-navigation-drawer at the left side that consume a more prominent space and make it move obvious which wallet is used.
  • Critical actions like send-coins should maybe remind the user in the confirmation dialog which wallet it will use.
  • Users eventually should choose an (coloured) icon per wallet (give a pre-selection of 12 icons) which then will be always visible on a top window level and or displayed when executing critical actions (showing addresses, sending coins, etc.)

Code:

  • I don't see why the RPC auth argument would be necessary at this point (but I respect your comment that we should discuss that in #10615)
  • I dislike the #Ifdef ENABLE_WALLET clustering. I would reduce it to a minimum and runtime bypass where possible. Especially the #Ifdefs in RPC / http low level code is ugly.
@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Oct 5, 2017

Member

I think we can fairly easily pull out the contentious rpcauth parts of this to make it more palatable for review. I have a branch here: https://github.com/jnewbery/bitcoin/tree/pr11383.1 which removes the first three commits and makes the one change to rpcconsole.cpp to make this work without rpcauth. It's also rebased on master since there was a merge conflict.

Member

jnewbery commented Oct 5, 2017

I think we can fairly easily pull out the contentious rpcauth parts of this to make it more palatable for review. I have a branch here: https://github.com/jnewbery/bitcoin/tree/pr11383.1 which removes the first three commits and makes the one change to rpcconsole.cpp to make this work without rpcauth. It's also rebased on master since there was a merge conflict.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Oct 6, 2017

Member

Rebased, eliminated ENABLE_WALLET stuff added to non-wallet/GUI code, and removed the dependency on rpcauth default wallet stuff.

I did not address @jonasschnelli's GUI comments, because I feel some is best explored separately, as improvements on top of this, while others (confirming wallet changes with a prompt) I think would make the feature annoying to use.

Member

luke-jr commented Oct 6, 2017

Rebased, eliminated ENABLE_WALLET stuff added to non-wallet/GUI code, and removed the dependency on rpcauth default wallet stuff.

I did not address @jonasschnelli's GUI comments, because I feel some is best explored separately, as improvements on top of this, while others (confirming wallet changes with a prompt) I think would make the feature annoying to use.

static const std::string WALLET_ENDPOINT_BASE = "/wallet/";
CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
void JSONRPCRequestWalletResolver(JSONRPCRequest& jreq, const HTTPRequest& httpreq)

This comment has been minimized.

@ryanofsky

ryanofsky Oct 9, 2017

Contributor

In commit "RPC: Pass wallet through JSONRPCRequest"

Multiwallet rpc calls are broken in this commit (and the whole PR) because JSONRPCRequestWalletResolver is not actually registered or called anywhere. Maybe a commit was lost in the rebase? The problem causes multiwallet.py test to fail with "Wallet file not specified"

@ryanofsky

ryanofsky Oct 9, 2017

Contributor

In commit "RPC: Pass wallet through JSONRPCRequest"

Multiwallet rpc calls are broken in this commit (and the whole PR) because JSONRPCRequestWalletResolver is not actually registered or called anywhere. Maybe a commit was lost in the rebase? The problem causes multiwallet.py test to fail with "Wallet file not specified"

This comment has been minimized.

@luke-jr

luke-jr Oct 12, 2017

Member

Fixed

@luke-jr

luke-jr Oct 12, 2017

Member

Fixed

Show outdated Hide outdated src/qt/bitcoin.cpp Outdated
@@ -95,13 +95,13 @@ void WalletView::setBitcoinGUI(BitcoinGUI *gui)
connect(this, SIGNAL(message(QString,QString,unsigned int)), gui, SLOT(message(QString,QString,unsigned int)));
// Pass through encryption status changed signals
connect(this, SIGNAL(encryptionStatusChanged(int)), gui, SLOT(setEncryptionStatus(int)));
connect(this, SIGNAL(encryptionStatusChanged(int)), gui, SLOT(updateWalletStatus()));

This comment has been minimized.

@ryanofsky

ryanofsky Oct 9, 2017

Contributor

In commit "Qt: Ensure UI updates only come from the currently selected walletView"

This is silently dropping the int status value passed to encryptionStatusChanged, and the int hdEnabled value passed to hdEnabledStatusChanged. Assuming this is intended, you should clean up after this change by deleting the unused parameters from encryptionStatusChanged and hdEnabledStatusChanged declarations and calls.

@ryanofsky

ryanofsky Oct 9, 2017

Contributor

In commit "Qt: Ensure UI updates only come from the currently selected walletView"

This is silently dropping the int status value passed to encryptionStatusChanged, and the int hdEnabled value passed to hdEnabledStatusChanged. Assuming this is intended, you should clean up after this change by deleting the unused parameters from encryptionStatusChanged and hdEnabledStatusChanged declarations and calls.

Show outdated Hide outdated src/qt/rpcconsole.cpp Outdated
@@ -65,6 +65,15 @@ WalletModel::~WalletModel()
unsubscribeFromCoreSignals();
}
QString WalletModel::getWalletName() const

This comment has been minimized.

@ryanofsky

ryanofsky Oct 9, 2017

Contributor

In commit "Qt: When multiple wallets are used, include in notifications the name"

Would be nice to follow this up by removing the name parameter passed to BitcoinGUI::addWallet to avoid duplicating .dat stripping logic.

@ryanofsky

ryanofsky Oct 9, 2017

Contributor

In commit "Qt: When multiple wallets are used, include in notifications the name"

Would be nice to follow this up by removing the name parameter passed to BitcoinGUI::addWallet to avoid duplicating .dat stripping logic.

This comment has been minimized.

@luke-jr

luke-jr Oct 12, 2017

Member

Done.

@luke-jr

luke-jr Oct 12, 2017

Member

Done.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Oct 12, 2017

Member

(Note Travis failure is due to the batch request bug fixed in #11277)

Member

luke-jr commented Oct 12, 2017

(Note Travis failure is due to the batch request bug fixed in #11277)

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Oct 12, 2017

Member

#11277 is merged. Can you rebase?

Member

jnewbery commented Oct 12, 2017

#11277 is merged. Can you rebase?

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Oct 12, 2017

Member

Rebased

Member

luke-jr commented Oct 12, 2017

Rebased

@fanquake fanquake added this to In progress in Multiwallet support Nov 4, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 6, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 6, 2017

@laanwj laanwj added this to Blockers in High-priority for review Nov 16, 2017

@jonasschnelli

Lightly tested ACK: 555eec1

Works as intended.
Visually not very elegant, but an acceptable first step.

PrepareJSONRPCRequestCallbacks.connect(preparer);
}
void UnregisterJSONRPCRequestPreparer(const JSONRPCRequestPreparer& preparer)

This comment has been minimized.

@jonasschnelli

jonasschnelli Nov 16, 2017

Member

This gets never called, right?

@jonasschnelli

jonasschnelli Nov 16, 2017

Member

This gets never called, right?

This comment has been minimized.

@luke-jr

luke-jr Dec 14, 2017

Member

Right, nor does /wallet/ get unregistered... Fixing

@luke-jr

luke-jr Dec 14, 2017

Member

Right, nor does /wallet/ get unregistered... Fixing

Show outdated Hide outdated src/httprpc.cpp Outdated
/** WWW-Authenticate to present with 401 Unauthorized response */
static const char* WWW_AUTH_HEADER_DATA = "Basic realm=\"jsonrpc\"";
boost::signals2::signal<void (JSONRPCRequest&, const HTTPRequest&)> PrepareJSONRPCRequestCallbacks;

This comment has been minimized.

@jonasschnelli

jonasschnelli Nov 16, 2017

Member

Isn't the HTTPRequest object unused? Why pack it into the signal, future extensions?

@jonasschnelli

jonasschnelli Nov 16, 2017

Member

Isn't the HTTPRequest object unused? Why pack it into the signal, future extensions?

This comment has been minimized.

@luke-jr

luke-jr Dec 14, 2017

Member

Right

@luke-jr

luke-jr Dec 14, 2017

Member

Right

// in Qt, use always the wallet with index 0 when running with multiple wallets
QByteArray encodedName = QUrl::toPercentEncoding(QString::fromStdString(vpwallets[0]->GetName()));
req.URI = "/wallet/"+std::string(encodedName.constData(), encodedName.length());
CWalletRef * const ppwallet = (CWalletRef*)ppwallet_v;

This comment has been minimized.

@jonasschnelli

jonasschnelli Nov 16, 2017

Member

looks like we should use a C++11 (shared?) pointer here to avoid the manual memory management.

@jonasschnelli

jonasschnelli Nov 16, 2017

Member

looks like we should use a C++11 (shared?) pointer here to avoid the manual memory management.

This comment has been minimized.

@jonasschnelli

jonasschnelli Mar 2, 2018

Member

Instead of the whole signal & gluing together the wallet and the RPC server, why not just refactor out GetWalletForJSONRPCRequest and use the same logic here? Seems much more understandable and less code.

@jonasschnelli

jonasschnelli Mar 2, 2018

Member

Instead of the whole signal & gluing together the wallet and the RPC server, why not just refactor out GetWalletForJSONRPCRequest and use the same logic here? Seems much more understandable and less code.

@promag

Tested, but will look the code. Works as expected.

There are some style issues that could be addressed here or in follow ups:

  • combo and label in the toolbar use different style (font for instance) than the buttons;
  • not sure if the right side is the best place in terms of UX;
  • maybe it should be possible to select wallet from the menu bar (for instance, File -> Wallets -> w1);
  • window titles could have the wallet name too;
  • the combo in the console is a bit lost (this time is not in a toolbar);
  • could show a message in the console when the wallet is changed.
@@ -45,8 +47,9 @@ class JSONRPCRequest
bool fHelp;
std::string URI;
std::string authUser;
CWallet *wallet;

This comment has been minimized.

@promag

promag Nov 19, 2017

Member

Do we want wallet here even an opaque pointer?

@promag

promag Nov 19, 2017

Member

Do we want wallet here even an opaque pointer?

This comment has been minimized.

@luke-jr

luke-jr Dec 14, 2017

Member

? This is where it belongs..

@luke-jr

luke-jr Dec 14, 2017

Member

? This is where it belongs..

@Sjors

This comment has been minimized.

Show comment
Hide comment
@Sjors

Sjors Nov 20, 2017

Member

I tested the debug console by switching wallets (including no wallet) and using dumpwallet. This seems to work as expected. I also checked that wallet backups from the application menu use the correct wallet.

I agree with @promag's suggestions and added a few below. I don't think any should block this PR, although some might simplify it.

Maybe this is what you were saying, but I have a light preference for reducing UI clutter by removing the dropdowns and instead adding Wallet to the primary menu (between File and Settings). The application title would contain the name of the currently selected wallet, as you suggested. In the Wallet menu, the selected wallet would have a check box, and not be selectable. This has a couple of advantages:

  • it's more future proof for when there's UI for adding wallets, creating new wallets, etc
  • you can move Backup Wallet there as well (and rename it to Backup [name of wallet])
  • you can move Encrypt and change passphrase there (and get rid of Settings menu)
  • you could open each wallet in its own window (probably not desirable though)
    • it would make it easier for users to send funds between wallets, and be more like opening multiple word documents
    • I don't know if that's possible (maybe it creates a threading nightmare)
    • it makes it more difficult to use the application menu for actions, because you have to clarify which wallet the action refers to (or they need to work nicely with multiple wallets)

I assume #10740 is needed before wallets can be added through the UI?

I like @jonasschnelli's suggestion of allowing each wallet to have a name, so the file name can be inconspicuous. Same for clearly showing the wallet name on the send confirmation screen. I also like the idea of using color, although that can get ugly very quickly.

In case anyone else was confused like me, this how you launch with multiple wallets:
Bitcoin-Qt -wallet=wallet1.dat -wallet=wallet2.dat (and it won't complain if you open a non-existing wallet, e.g. because you left out .dat; it will just create it for you)

Member

Sjors commented Nov 20, 2017

I tested the debug console by switching wallets (including no wallet) and using dumpwallet. This seems to work as expected. I also checked that wallet backups from the application menu use the correct wallet.

I agree with @promag's suggestions and added a few below. I don't think any should block this PR, although some might simplify it.

Maybe this is what you were saying, but I have a light preference for reducing UI clutter by removing the dropdowns and instead adding Wallet to the primary menu (between File and Settings). The application title would contain the name of the currently selected wallet, as you suggested. In the Wallet menu, the selected wallet would have a check box, and not be selectable. This has a couple of advantages:

  • it's more future proof for when there's UI for adding wallets, creating new wallets, etc
  • you can move Backup Wallet there as well (and rename it to Backup [name of wallet])
  • you can move Encrypt and change passphrase there (and get rid of Settings menu)
  • you could open each wallet in its own window (probably not desirable though)
    • it would make it easier for users to send funds between wallets, and be more like opening multiple word documents
    • I don't know if that's possible (maybe it creates a threading nightmare)
    • it makes it more difficult to use the application menu for actions, because you have to clarify which wallet the action refers to (or they need to work nicely with multiple wallets)

I assume #10740 is needed before wallets can be added through the UI?

I like @jonasschnelli's suggestion of allowing each wallet to have a name, so the file name can be inconspicuous. Same for clearly showing the wallet name on the send confirmation screen. I also like the idea of using color, although that can get ugly very quickly.

In case anyone else was confused like me, this how you launch with multiple wallets:
Bitcoin-Qt -wallet=wallet1.dat -wallet=wallet2.dat (and it won't complain if you open a non-existing wallet, e.g. because you left out .dat; it will just create it for you)

bool BitcoinGUI::multiWallet() const
{
return (WalletSelector->count() > 1);

This comment has been minimized.

@Sjors

Sjors Nov 22, 2017

Member

Using a UI element to count the number of wallets doesn't seem ideal.

@Sjors

Sjors Nov 22, 2017

Member

Using a UI element to count the number of wallets doesn't seem ideal.

This comment has been minimized.

@Sjors

Sjors Jan 2, 2018

Member

I think a better approach is to give QT access to std::vector<CWalletRef> vpwallets or m_wallet_models, and then when the dropdown box (re-)renders itself, it should query that.

That would also avoid passing walletModel instances around like in rpcConsole->addWallet(walletModel);.

@Sjors

Sjors Jan 2, 2018

Member

I think a better approach is to give QT access to std::vector<CWalletRef> vpwallets or m_wallet_models, and then when the dropdown box (re-)renders itself, it should query that.

That would also avoid passing walletModel instances around like in rpcConsole->addWallet(walletModel);.

This comment has been minimized.

@jonasschnelli

jonasschnelli Feb 12, 2018

Member

Agree with @Sjors

@jonasschnelli

jonasschnelli Feb 12, 2018

Member

Agree with @Sjors

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Nov 29, 2017

Member

@luke-jr friendly ping, rebase needed.

Member

promag commented Nov 29, 2017

@luke-jr friendly ping, rebase needed.

@laanwj laanwj removed this from Blockers in High-priority for review Dec 7, 2017

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 7, 2017

Member

As per IRC discussion removing this from high priority for review until review comments get addressed.

Member

laanwj commented Dec 7, 2017

As per IRC discussion removing this from high priority for review until review comments get addressed.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 1, 2018

Member

The unused signal (remove it and add it when needed)

What unused signal?

Fix the delete ppwallet; (move to shared pointers or similar?)

I don't know how to do this with Qt. Note that the original design here had the CWalletRef as a shared pointer type, but that still needed a new/delete for the ref itself. If someone feels like improving it, that can be done in a later PR.

Member

luke-jr commented Mar 1, 2018

The unused signal (remove it and add it when needed)

What unused signal?

Fix the delete ppwallet; (move to shared pointers or similar?)

I don't know how to do this with Qt. Note that the original design here had the CWalletRef as a shared pointer type, but that still needed a new/delete for the ref itself. If someone feels like improving it, that can be done in a later PR.

@randolf

This comment has been minimized.

Show comment
Hide comment
@randolf

randolf Mar 1, 2018

Contributor

utACK.

Contributor

randolf commented Mar 1, 2018

utACK.

@@ -226,6 +241,8 @@ static bool InitRPCAuthentication()
return true;
}
void JSONRPCRequestWalletResolver(JSONRPCRequest&, const HTTPRequest&);

This comment has been minimized.

@jonasschnelli

jonasschnelli Mar 2, 2018

Member

Looks after a strange design to forward decelerate this here, which is implemented in rpcwallet.cpp with not even a #ifdef ENABLE_WALLET

@jonasschnelli

jonasschnelli Mar 2, 2018

Member

Looks after a strange design to forward decelerate this here, which is implemented in rpcwallet.cpp with not even a #ifdef ENABLE_WALLET

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 2, 2018

Member

I would recommend to remove the signal and the JSONRequest wallet member (the JSON RPC layer should not know what a wallet is) and add the URI parsing logic in rpcconsole.cpp (though WalletModel and a little refactoring of GetWalletForJSONRPCRequest).

Member

jonasschnelli commented Mar 2, 2018

I would recommend to remove the signal and the JSONRequest wallet member (the JSON RPC layer should not know what a wallet is) and add the URI parsing logic in rpcconsole.cpp (though WalletModel and a little refactoring of GetWalletForJSONRPCRequest).

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 2, 2018

Member

That would be ugly. :/

Member

luke-jr commented Mar 2, 2018

That would be ugly. :/

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 2, 2018

Member

That would be ugly. :/

We should just avoid overlapping layers.
Your current design adds wallet knowhow to JSONRPCRequest (server.h) and adds a boost signal (which we are trying to get rid of) including a forward declaration in the httpserver of a wallet function.
It looks like you have added this only to allow getting the called wallet from within rpcconsole.cpp.

IMO it would be much more straight forward to parse the URI there instead of the current invasive approach.

Member

jonasschnelli commented Mar 2, 2018

That would be ugly. :/

We should just avoid overlapping layers.
Your current design adds wallet knowhow to JSONRPCRequest (server.h) and adds a boost signal (which we are trying to get rid of) including a forward declaration in the httpserver of a wallet function.
It looks like you have added this only to allow getting the called wallet from within rpcconsole.cpp.

IMO it would be much more straight forward to parse the URI there instead of the current invasive approach.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 2, 2018

Member

I assume by parse, you mean fabricate...?

The JSONRPCRequest only holds a pointer. This is a good design, unlike making up and then parsing strings internally.

Member

luke-jr commented Mar 2, 2018

I assume by parse, you mean fabricate...?

The JSONRPCRequest only holds a pointer. This is a good design, unlike making up and then parsing strings internally.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 2, 2018

Member

I assume by parse, you mean fabricate...?

Yes. Since the rpcconsole does already fabricate artificial JSONRPCRequests. There is nothing wrong with that. If you want to avoid the artificial JSONRPCRquest creation, you would need to get rid of it completely.

See my commit that...

  • ...remove the unnecessary coupling of server.h and wallet
  • ...removes the signal
  • ...removes "knowhow" of "JSONRPC" from httpserver
  • ...simply continues the JSONRPCRequest fabrication

It also makes this PR more compact (17 additions and 61 deletions.)

jonasschnelli@f8eab48

The other thing that needs fixing is the passing around of pure pointers for CWallet*.
I think, for now, passing around the wallet name could be sufficient (I don't like that,... but seems better then passing around pure pointers). Wallet names are unique for now.

Member

jonasschnelli commented Mar 2, 2018

I assume by parse, you mean fabricate...?

Yes. Since the rpcconsole does already fabricate artificial JSONRPCRequests. There is nothing wrong with that. If you want to avoid the artificial JSONRPCRquest creation, you would need to get rid of it completely.

See my commit that...

  • ...remove the unnecessary coupling of server.h and wallet
  • ...removes the signal
  • ...removes "knowhow" of "JSONRPC" from httpserver
  • ...simply continues the JSONRPCRequest fabrication

It also makes this PR more compact (17 additions and 61 deletions.)

jonasschnelli@f8eab48

The other thing that needs fixing is the passing around of pure pointers for CWallet*.
I think, for now, passing around the wallet name could be sufficient (I don't like that,... but seems better then passing around pure pointers). Wallet names are unique for now.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 2, 2018

Member

Another tiny conceptual problem is that if one switches the wallet in the main window, it does not automatically switch in the console window (Not sure if it should, but if so, it can be address in a follow up PR).

Member

jonasschnelli commented Mar 2, 2018

Another tiny conceptual problem is that if one switches the wallet in the main window, it does not automatically switch in the console window (Not sure if it should, but if so, it can be address in a follow up PR).

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 2, 2018

Member

I fundamentally disagree with that approach. It is a poor design, and assumes the RPC layer and GUI share some common wallet name table. Perhaps they do for now, but it doesn't excuse the bad design.

Member

luke-jr commented Mar 2, 2018

I fundamentally disagree with that approach. It is a poor design, and assumes the RPC layer and GUI share some common wallet name table. Perhaps they do for now, but it doesn't excuse the bad design.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 3, 2018

Member

I rewrote this PR to avoid the pointer passing and the signal & wallet-server coupling:
https://github.com/bitcoin/bitcoin/compare/master...jonasschnelli:2018/03/multiwallet?expand=1

If I'm alone with my design concerns, then I'm fine dropping it. :)
Would be good the hear other opinions: @ryanofsky @Sjors @promag

Member

jonasschnelli commented Mar 3, 2018

I rewrote this PR to avoid the pointer passing and the signal & wallet-server coupling:
https://github.com/bitcoin/bitcoin/compare/master...jonasschnelli:2018/03/multiwallet?expand=1

If I'm alone with my design concerns, then I'm fine dropping it. :)
Would be good the hear other opinions: @ryanofsky @Sjors @promag

@Sjors

This comment has been minimized.

Show comment
Hide comment
@Sjors

Sjors Mar 5, 2018

Member

@jonasschnelli can you turn that into a PR so it's easier to provide line-by-line feedback? It seems bool BitcoinGUI::multiWallet() still counts UI elements (QComboBox *m_wallet_selector instances). Can that be avoided?

Member

Sjors commented Mar 5, 2018

@jonasschnelli can you turn that into a PR so it's easier to provide line-by-line feedback? It seems bool BitcoinGUI::multiWallet() still counts UI elements (QComboBox *m_wallet_selector instances). Can that be avoided?

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Mar 5, 2018

Member

In the "Sending addresses" and "Receiving addresses" dialogs, it's missing the wallet name. Not sure if the dialog title is the best place.

Member

promag commented Mar 5, 2018

In the "Sending addresses" and "Receiving addresses" dialogs, it's missing the wallet name. Not sure if the dialog title is the best place.

wallet = walletModel->getWalletName();
}
Q_EMIT incomingTransaction(date, walletModel->getOptionsModel()->getDisplayUnit(), amount, type, address, label, wallet);

This comment has been minimized.

@jonasschnelli

jonasschnelli Mar 6, 2018

Member

Why not remove the bitcoinGui member and always send the wallet name?

@jonasschnelli

jonasschnelli Mar 6, 2018

Member

Why not remove the bitcoinGui member and always send the wallet name?

This comment has been minimized.

@luke-jr

luke-jr Mar 6, 2018

Member

When not in multiwallet mode, this causes the "Wallet" line to be omitted from the notification popup.

@luke-jr

luke-jr Mar 6, 2018

Member

When not in multiwallet mode, this causes the "Wallet" line to be omitted from the notification popup.

This comment has been minimized.

@jonasschnelli

jonasschnelli Mar 6, 2018

Member

Since only bitcoingui.cpp listens to the incomingTransaction signal, it could be added or not added there. IMO the signal emitter should not decide wether to show the wallet name or not.

@jonasschnelli

jonasschnelli Mar 6, 2018

Member

Since only bitcoingui.cpp listens to the incomingTransaction signal, it could be added or not added there. IMO the signal emitter should not decide wether to show the wallet name or not.

This comment has been minimized.

@luke-jr

luke-jr Mar 6, 2018

Member

But there is no wallet name in single-wallet mode...

@luke-jr

luke-jr Mar 6, 2018

Member

But there is no wallet name in single-wallet mode...

@laanwj laanwj removed this from Blockers in High-priority for review Mar 6, 2018

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 7, 2018

Member

I really like to merge this,... but I would feel pretty uncomfortable to merge code where we unnecessary glue together the RPC/HTTP layer with the wallet as well as manual pointer memory management... this is something that should be fixed in the initial PR IMO rather the count for follow up PR.

Member

jonasschnelli commented Mar 7, 2018

I really like to merge this,... but I would feel pretty uncomfortable to merge code where we unnecessary glue together the RPC/HTTP layer with the wallet as well as manual pointer memory management... this is something that should be fixed in the initial PR IMO rather the count for follow up PR.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 7, 2018

Member

@jonasschnelli This only reduces/removes existing glue between the RPC/HTTP and wallet. It doesn't add any.

The manual pointer management is unavoidable with Qt AFAIK, without resorting to worse, ugly hacks like passing it as a string (which will break as soon as we support wallet unloading).

Member

luke-jr commented Mar 7, 2018

@jonasschnelli This only reduces/removes existing glue between the RPC/HTTP and wallet. It doesn't add any.

The manual pointer management is unavoidable with Qt AFAIK, without resorting to worse, ugly hacks like passing it as a string (which will break as soon as we support wallet unloading).

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 7, 2018

Member

@jonasschnelli This only reduces/removes existing glue between the RPC/HTTP and wallet. It doesn't add any.

Are you referring to this PR? In this PR you are trying to add a CWallet *wallet; member to JSONRPCRequest which IMO is the wrong direction.

The manual pointer management is unavoidable with Qt AFAIK, without resorting to worse, ugly hacks like passing it as a string (which will break as soon as we support wallet unloading).

Right now, we do identify wallets in multiwallet by it's filename. IMO identifying wallets by it's name is currently superior to passing around CWallet pointer with manual deallocation (delete) (which will break worse in case of dynamic loading/unloading IMO).

Member

jonasschnelli commented Mar 7, 2018

@jonasschnelli This only reduces/removes existing glue between the RPC/HTTP and wallet. It doesn't add any.

Are you referring to this PR? In this PR you are trying to add a CWallet *wallet; member to JSONRPCRequest which IMO is the wrong direction.

The manual pointer management is unavoidable with Qt AFAIK, without resorting to worse, ugly hacks like passing it as a string (which will break as soon as we support wallet unloading).

Right now, we do identify wallets in multiwallet by it's filename. IMO identifying wallets by it's name is currently superior to passing around CWallet pointer with manual deallocation (delete) (which will break worse in case of dynamic loading/unloading IMO).

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 7, 2018

Member

The manual pointer management is unavoidable with Qt AFAIK, without resorting to worse, ugly hacks like passing it as a string (which will break as soon as we support wallet unloading).

Right now, we do identify wallets in multiwallet by it's filename. IMO identifying wallets by it's name is currently superior to passing around CWallet pointer with manual deallocation (delete) (which will break worse in case of dynamic loading/unloading IMO).

Clarification: I'm only referring to the RPCConsole here where we simulate a JSONRPCRequest (and therefore need to derive the wallet name anyways to use the the correct URI endpoint).

Member

jonasschnelli commented Mar 7, 2018

The manual pointer management is unavoidable with Qt AFAIK, without resorting to worse, ugly hacks like passing it as a string (which will break as soon as we support wallet unloading).

Right now, we do identify wallets in multiwallet by it's filename. IMO identifying wallets by it's name is currently superior to passing around CWallet pointer with manual deallocation (delete) (which will break worse in case of dynamic loading/unloading IMO).

Clarification: I'm only referring to the RPCConsole here where we simulate a JSONRPCRequest (and therefore need to derive the wallet name anyways to use the the correct URI endpoint).

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 7, 2018

Member

Are you referring to this PR? In this PR you are trying to add a CWallet *wallet; member to JSONRPCRequest which IMO is the wrong direction.

Indeed I am. This PR improves the situation by moving the wallet-handling logic out of the RPC/HTTP code, into the wallet-specific module. Having a reference to the wallet for the request, on the request is only reasonable. It is a shortcoming of C++ that it needs to be declared together with the rest of the request class. That could be avoided with a map<string, void*> I suppose, but then we need to do manual allocation of any future refcounted pointer - and the only gain in this is extremely minor.

Right now, we do identify wallets in multiwallet by it's filename.

Only for interfacing to HTTP, an inherently text-based protocol. It makes no sense to do so otherwise, and there are serious shortcomings to this approach. (You could very well end up with a multi-step RPC beginning with one wallet, and finishing in another loaded with the same name after the first was unloaded!)

IMO identifying wallets by it's name is currently superior to passing around CWallet pointer with manual deallocation (delete) (which will break worse in case of dynamic loading/unloading IMO).

This code doesn't do that. The only manual allocation is of a CWalletRef (that is, it is currently a pointer to a pointer). We could store the CWalletRef directly in place of it, but that would break when CWalletRef becomes a refcounting type. The only way to avoid this would be to find a way to teach QVariant to hold a CWalletRef directly.

Member

luke-jr commented Mar 7, 2018

Are you referring to this PR? In this PR you are trying to add a CWallet *wallet; member to JSONRPCRequest which IMO is the wrong direction.

Indeed I am. This PR improves the situation by moving the wallet-handling logic out of the RPC/HTTP code, into the wallet-specific module. Having a reference to the wallet for the request, on the request is only reasonable. It is a shortcoming of C++ that it needs to be declared together with the rest of the request class. That could be avoided with a map<string, void*> I suppose, but then we need to do manual allocation of any future refcounted pointer - and the only gain in this is extremely minor.

Right now, we do identify wallets in multiwallet by it's filename.

Only for interfacing to HTTP, an inherently text-based protocol. It makes no sense to do so otherwise, and there are serious shortcomings to this approach. (You could very well end up with a multi-step RPC beginning with one wallet, and finishing in another loaded with the same name after the first was unloaded!)

IMO identifying wallets by it's name is currently superior to passing around CWallet pointer with manual deallocation (delete) (which will break worse in case of dynamic loading/unloading IMO).

This code doesn't do that. The only manual allocation is of a CWalletRef (that is, it is currently a pointer to a pointer). We could store the CWalletRef directly in place of it, but that would break when CWalletRef becomes a refcounting type. The only way to avoid this would be to find a way to teach QVariant to hold a CWalletRef directly.

jonasschnelli added a commit that referenced this pull request Mar 26, 2018

Merge #12610: Multiwallet for the GUI
779c5f9 Qt: hide RPCConsole wallet selector when no wallets are present (Jonas Schnelli)
dc6f150 Qt: show wallet name in request dlg in case of multiwallet (Jonas Schnelli)
4826ca4 Qt: show wallet name in send confirmation dlg in case of multiwallet (Jonas Schnelli)
cfa4133 GUI: RPCConsole: Log wallet changes (Luke Dashjr)
b6d04fc Qt: Get wallet name from WalletModel rather than passing it around (Luke Dashjr)
12d8d26 Qt: When multiple wallets are used, include in notifications the name (Jonas Schnelli)
d1ec34a Qt: QComboBox::setVisible doesn't work in toolbars, so defer adding it at all until needed (Luke Dashjr)
d49cc70 Qt: Add wallet selector to debug console (Jonas Schnelli)
d558f44 Bugfix: RPC: Add missing UnregisterHTTPHandler for /wallet/ (Luke Dashjr)
85d5319 Qt: Ensure UI updates only come from the currently selected walletView (Luke Dashjr)
e449f9a Qt: Add a combobox to toolbar to select from multiple wallets (Luke Dashjr)
3dba3c3 Qt: Load all wallets into WalletModels (Luke Dashjr)

Pull request description:

  This is an overhaul of #11383 (plus some additions).
  It avoids unnecessary coupling of httpserver/jsonrpc and the wallet as well as it avoids pointer pure passing (and pointer deletion) of `CWallet` (plus other minor design changes).

  Additionally it adds the wallet name to the sendconfirmation and request dialog (in case multiwallet is active)

Tree-SHA512: 3d06e18badbc5d1821e488bf1dae463bb0be544cf11b2b618e025812bfdd13c5f39604bb93b4c705313930e7dc4e66f4848b9469ba14871bade58e7a027246a1
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 26, 2018

Member

Closing in favour of now merged #12610

Member

jonasschnelli commented Mar 26, 2018

Closing in favour of now merged #12610

@jonasschnelli jonasschnelli removed this from In progress in Multiwallet support Apr 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment