Basic Multiwallet GUI support

luke-jr commented at 10:09 pm on September 21, 2017: member

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

fanquake added the label GUI on Sep 21, 2017

fanquake added the label Wallet on Sep 21, 2017

in src/qt/bitcoin.cpp:494 in 4a1ef7065d outdated

495+            WalletModel * const walletModel = new WalletModel(platformStyle, pwallet, optionsModel);
496 
497-            window->addWallet(BitcoinGUI::DEFAULT_WALLET, walletModel);
498-            window->setCurrentWallet(BitcoinGUI::DEFAULT_WALLET);
499+            QString WalletName = QString::fromStdString(pwallet->GetName());
500+            if (WalletName.endsWith(".dat")) {

jonasschnelli commented at 4:32 am on September 22, 2017:

Hmm.. not sure if we should hardcode .dat at this point.

luke-jr commented at 4:48 am on September 22, 2017:

It’s strictly for stripping.

ryanofsky commented at 10:28 pm on January 2, 2018:

#11383 (review)

Hmm.. not sure if we should hardcode .dat at this point.

Note: this is now cleaned up in a later commit (“Get wallet name from WalletModel rather than passing it around”)

in src/qt/bitcoingui.cpp:467 in 4a1ef7065d outdated

461@@ -462,6 +462,15 @@ void BitcoinGUI::createToolBars()
462         toolbar->addAction(receiveCoinsAction);
463         toolbar->addAction(historyAction);
464         overviewAction->setChecked(true);
465+
466+#ifdef ENABLE_WALLET

jonasschnelli commented at 4:39 am on September 22, 2017:

~~this #ifdef could be avoided/removed, right?~~

luke-jr commented at 4:49 am on September 22, 2017:

Maybe.

jonasschnelli commented at 11:15 am on February 12, 2018:

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

jonasschnelli commented at 4:55 am on September 22, 2017: contributor

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 commented at 10:01 pm on October 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.

luke-jr force-pushed on Oct 6, 2017

luke-jr commented at 2:51 am on October 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.

in src/wallet/rpcwallet.cpp:37 in ca11c309cf outdated

32@@ -33,21 +33,34 @@
33 
34 #include <univalue.h>
35 
36-static const std::string WALLET_ENDPOINT_BASE = "/wallet/";
37-
38-CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
39+void JSONRPCRequestWalletResolver(JSONRPCRequest& jreq, const HTTPRequest& httpreq)

ryanofsky commented at 2:19 pm on October 9, 2017:

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”

luke-jr commented at 6:59 am on October 12, 2017:

Fixed

in src/qt/bitcoin.cpp:255 in eececdbbbb outdated

251@@ -252,7 +252,7 @@ public Q_SLOTS:
252     QTimer *pollShutdownTimer;
253 #ifdef ENABLE_WALLET
254     PaymentServer* paymentServer;
255-    WalletModel *walletModel;
256+    std::vector<WalletModel*> walletModels;

ryanofsky commented at 2:21 pm on October 9, 2017:

In commit “Qt: Load all wallets into WalletModels”

Might be simpler to make this a vector of unique_ptr, to avoid need for manual deletions. Also since renaming this member anyway, could follow current naming convenetion (m_wallet_models).

luke-jr commented at 7:23 am on October 12, 2017:

Not sure unique_ptr is the right tool for the job here. Renamed.

in src/qt/walletview.cpp:99 in 120ef7ea31 outdated

94@@ -95,13 +95,13 @@ void WalletView::setBitcoinGUI(BitcoinGUI *gui)
95         connect(this, SIGNAL(message(QString,QString,unsigned int)), gui, SLOT(message(QString,QString,unsigned int)));
96 
97         // Pass through encryption status changed signals
98-        connect(this, SIGNAL(encryptionStatusChanged(int)), gui, SLOT(setEncryptionStatus(int)));
99+        connect(this, SIGNAL(encryptionStatusChanged(int)), gui, SLOT(updateWalletStatus()));

ryanofsky commented at 2:31 pm on October 9, 2017:

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.

in src/qt/rpcconsole.cpp:873 in 8739e5cacf outdated

864@@ -843,8 +865,18 @@ void RPCConsole::on_lineEdit_returnPressed()
865 
866         cmdBeforeBrowsing = QString();
867 
868+        void *ppwallet_v = nullptr;
869+#ifdef ENABLE_WALLET
870+        const int wallet_index = ui->WalletSelector->currentIndex();
871+        if (wallet_index > 0) {
872+            CWalletRef *ppwallet = (CWalletRef*)ui->WalletSelector->itemData(wallet_index).value<void*>();
873+            ppwallet = new CWalletRef(*ppwallet);  // Refcount

ryanofsky commented at 2:36 pm on October 9, 2017:

In commit “Qt: Add wallet selector to debug console”

What does refcount comment mean? Is this a todo suggesting that you want to add refcounting in the future? Should clarify comment or maybe remove it.

luke-jr commented at 7:24 am on October 12, 2017:

Clarified.

in src/qt/walletmodel.cpp:68 in 24ca9cded5 outdated

64@@ -65,6 +65,15 @@ WalletModel::~WalletModel()
65     unsubscribeFromCoreSignals();
66 }
67 
68+QString WalletModel::getWalletName() const

ryanofsky commented at 2:47 pm on October 9, 2017:

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.

luke-jr commented at 7:24 am on October 12, 2017:

Done.

luke-jr commented at 7:34 am on October 12, 2017: member

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

jnewbery commented at 6:02 pm on October 12, 2017: member

#11277 is merged. Can you rebase?

luke-jr force-pushed on Oct 12, 2017

luke-jr commented at 7:58 pm on October 12, 2017: member

Rebased

fanquake added this to the "In progress" column in a project

luke-jr referenced this in commit 51b44325f6 on Nov 6, 2017

luke-jr referenced this in commit 42ca0f5ac7 on Nov 6, 2017

laanwj added this to the "Blockers" column in a project

luke-jr force-pushed on Nov 16, 2017

in src/httprpc.cpp:33 in 555eec163b outdated

28+void RegisterJSONRPCRequestPreparer(const JSONRPCRequestPreparer& preparer)
29+{
30+    PrepareJSONRPCRequestCallbacks.connect(preparer);
31+}
32+
33+void UnregisterJSONRPCRequestPreparer(const JSONRPCRequestPreparer& preparer)

jonasschnelli commented at 8:33 pm on November 16, 2017:

This gets never called, right?

luke-jr commented at 3:16 am on December 14, 2017:

Right, nor does /wallet/ get unregistered… Fixing

in src/httprpc.cpp:255 in 555eec163b outdated

250@@ -236,6 +251,9 @@ bool StartHTTPRPC()
251 #ifdef ENABLE_WALLET
252     // ifdef can be removed once we switch to better endpoint support and API versioning
253     RegisterHTTPHandler("/wallet/", false, HTTPReq_JSONRPC);
254+
255+    void JSONRPCRequestWalletResolver(JSONRPCRequest& jreq, const HTTPRequest& httpreq);

jonasschnelli commented at 8:49 pm on November 16, 2017:

use externoutside function scope?

luke-jr commented at 3:17 am on December 14, 2017:

Isn’t extern not needed (nor desirable in our codebase) for function references?

in src/httprpc.cpp:26 in 555eec163b outdated

17@@ -18,10 +18,23 @@
18 #include <stdio.h>
19 
20 #include <boost/algorithm/string.hpp> // boost::trim
21+#include <boost/signals2/signal.hpp>
22 
23 /** WWW-Authenticate to present with 401 Unauthorized response */
24 static const char* WWW_AUTH_HEADER_DATA = "Basic realm=\"jsonrpc\"";
25 
26+boost::signals2::signal<void (JSONRPCRequest&, const HTTPRequest&)> PrepareJSONRPCRequestCallbacks;

jonasschnelli commented at 8:57 pm on November 16, 2017:

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

luke-jr commented at 3:15 am on December 14, 2017:

Right

in src/qt/rpcconsole.cpp:306 in 555eec163b outdated

304@@ -304,11 +305,10 @@ bool RPCConsole::RPCParseCommandLine(std::string &strResult, const std::string &
305                             req.params = RPCConvertValues(stack.back()[0], std::vector<std::string>(stack.back().begin() + 1, stack.back().end()));
306                             req.strMethod = stack.back()[0];
307 #ifdef ENABLE_WALLET
308-                            // TODO: Move this logic to WalletModel
309-                            if (!vpwallets.empty()) {
310-                                // in Qt, use always the wallet with index 0 when running with multiple wallets
311-                                QByteArray encodedName = QUrl::toPercentEncoding(QString::fromStdString(vpwallets[0]->GetName()));
312-                                req.URI = "/wallet/"+std::string(encodedName.constData(), encodedName.length());
313+                            CWalletRef * const ppwallet = (CWalletRef*)ppwallet_v;

jonasschnelli commented at 9:00 pm on November 16, 2017:

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

jonasschnelli commented at 4:19 am on March 2, 2018:

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 approved

jonasschnelli commented at 9:02 pm on November 16, 2017: contributor

Lightly tested ACK: 555eec163b1b62615bf0b49469058eb0be894268

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

in src/rpc/server.h:50 in 555eec163b outdated

46@@ -45,8 +47,9 @@ class JSONRPCRequest
47     bool fHelp;
48     std::string URI;
49     std::string authUser;
50+    CWallet *wallet;

promag commented at 10:44 pm on November 19, 2017:

Do we want wallet here even an opaque pointer?

luke-jr commented at 3:30 am on December 14, 2017:

? This is where it belongs..

promag commented at 10:45 pm on November 19, 2017: member

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.

Sjors commented at 3:07 pm on November 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)

in src/qt/bitcoingui.cpp:576 in 555eec163b outdated

573@@ -555,6 +574,11 @@ void BitcoinGUI::removeAllWallets()
574     setWalletActionsEnabled(false);
575     walletFrame->removeAllWallets();
576 }
577+
578+bool BitcoinGUI::multiWallet() const
579+{
580+    return (WalletSelector->count() > 1);

Sjors commented at 1:04 pm on November 22, 2017:

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

Sjors commented at 12:25 pm on January 2, 2018:

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);.

jonasschnelli commented at 11:15 am on February 12, 2018:

Agree with @Sjors

promag commented at 0:05 am on November 29, 2017: member

@luke-jr friendly ping, rebase needed.

laanwj removed this from the "Blockers" column in a project

laanwj commented at 7:05 pm on December 7, 2017: member

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

Bugfix: RPC: Add missing UnregisterHTTPHandler for /wallet/ cf1eba3896

RPC: Add JSONRPCRequest preparation callbacks bbbc2349da

RPC: Pass wallet through JSONRPCRequest 76df77f10a

Qt: Load all wallets into WalletModels 4401eee537

Qt: Ensure UI updates only come from the currently selected walletView da943d317b

Qt: Add a combobox to toolbar to select from multiple wallets 469239d263

Qt: Add wallet selector to debug console 91b7f95cc3

Qt: QComboBox::setVisible doesn't work in toolbars, so defer adding it at all until needed 703d0f6174

Qt: When multiple wallets are used, include in notifications the name of the wallet involved 66507329ee

Qt: Get wallet name from WalletModel rather than passing it around 69c019c003

GUI: RPCConsole: Log wallet changes f5aa574c37

luke-jr force-pushed on Dec 14, 2017

luke-jr commented at 3:34 am on December 14, 2017: member

Comments addressed and rebased. Also added a fix to unregister the "/wallet/" handler.

jonasschnelli added this to the "Blockers" column in a project

Sjors commented at 12:27 pm on January 2, 2018: member

I was able to combine this with #11403 without difficulty, so we can probably safely merge either first.

Sjors commented at 2:23 pm on January 2, 2018: member

I tried making m_wallet_models of BitcoinApplication public in hopes of being able to use it from bitcoingui.cpp, bitcoin.cpp and rpcconsole.cpp, but that turned into a mess.

I’m still confused as to what’s supposed to be the difference between qt/bitcoin.cpp and qt/bitcoin-gui.cpp. Why are they both importing qt/bitcoin-gui.h and why is there no qt/bitcoin.h?

Why is the contents of vpwallets copied into m_wallet_models ?

luke-jr commented at 8:28 pm on January 2, 2018: member

It’s not copied… m_wallet_models has models, not the wallets themselves.

ryanofsky commented at 10:58 pm on January 2, 2018: member

utACK f5aa574c372eba83add1e8d9336621e0e475c793.

This looks great, and is very cleanly implemented and nicely broken up into simple commits. (I think I’m going to look into breaking up my commits this way, just based on how ridiculously easy this was to review.)

I agree with various comments from Jonas, John, and Sjors above and think there’s room for a lot of UI improvements in the future, but this PR seems like a nice and simple first start.

I’m still confused as to what’s supposed to be the difference between qt/bitcoin.cpp and qt/bitcoin-gui.cpp. Why are they both importing qt/bitcoin-gui.h and why is there no qt/bitcoin.h?

I replied in IRC, but bitcoin-gui.cpp holds just the BitcoinGUI class, while bitcoin.cpp holds init classes that use BitcoinGUI. The naming and organization could probably be improved, though it seems pretty reasonable. #11625 adds a bitcoin.h file, which isn’t necessary right now because bitcoin.cpp contains the main() entry point.

in src/qt/bitcoingui.h:181 in f5aa574c37

174@@ -171,6 +175,13 @@ public Q_SLOTS:
175     void message(const QString &title, const QString &message, unsigned int style, bool *ret = nullptr);
176 
177 #ifdef ENABLE_WALLET
178+    bool setCurrentWallet(const QString& name);
179+
180+    /** Set the UI status indicators based on the currently selected wallet.
181+    */

jonasschnelli commented at 11:16 am on February 12, 2018:

nit: same line comment?

jonasschnelli commented at 11:18 am on February 12, 2018: contributor

Tested again, works. I think it’s worth to fix…

The unused signal (remove it and add it when needed)
Fix the delete ppwallet; (move to shared pointers or similar?)

luke-jr commented at 7:12 pm on March 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.

randolf commented at 7:19 pm on March 1, 2018: contributor

utACK.

in src/httprpc.cpp:244 in f5aa574c37

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

jonasschnelli commented at 4:13 am on March 2, 2018:

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

jonasschnelli commented at 4:24 am on March 2, 2018: contributor

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 commented at 4:41 am on March 2, 2018: member

That would be ugly. :/

jonasschnelli commented at 5:01 am on March 2, 2018: contributor

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 commented at 5:07 am on March 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.

jonasschnelli commented at 5:47 am on March 2, 2018: contributor

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

https://github.com/jonasschnelli/bitcoin/commit/f8eab48a81653a0488f81ad57116515571b82882

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 commented at 5:49 am on March 2, 2018: contributor

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 commented at 5:53 am on March 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.

jonasschnelli commented at 2:32 am on March 3, 2018: contributor

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 commented at 2:49 pm on March 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?

promag commented at 5:49 pm on March 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.

in src/qt/walletview.cpp:177 in f5aa574c37

173+    QString wallet;
174+    if (bitcoinGui->multiWallet()) {
175+        wallet = walletModel->getWalletName();
176+    }
177+
178+    Q_EMIT incomingTransaction(date, walletModel->getOptionsModel()->getDisplayUnit(), amount, type, address, label, wallet);

jonasschnelli commented at 4:08 am on March 6, 2018:

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

luke-jr commented at 4:10 am on March 6, 2018:

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

jonasschnelli commented at 4:13 am on March 6, 2018:

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.

luke-jr commented at 4:22 am on March 6, 2018:

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

laanwj removed this from the "Blockers" column in a project

jonasschnelli commented at 2:08 am on March 7, 2018: contributor

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 commented at 3:02 am on March 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).

jonasschnelli commented at 3:39 am on March 7, 2018: contributor

@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 commented at 3:47 am on March 7, 2018: contributor

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 commented at 4:36 am on March 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.

jonasschnelli referenced this in commit 25cf18f239 on Mar 26, 2018

jonasschnelli commented at 11:59 am on March 26, 2018: contributor

Closing in favour of now merged #12610

jonasschnelli closed this on Mar 26, 2018

jonasschnelli removed this from the "In progress" column in a project

PastaPastaPasta referenced this in commit 8a62412558 on May 14, 2020

PastaPastaPasta referenced this in commit 10b0549bfd on Jul 12, 2020

PastaPastaPasta referenced this in commit 8d4f66cd09 on Jul 12, 2020

PastaPastaPasta referenced this in commit 147101bc5c on Jul 16, 2020

PastaPastaPasta referenced this in commit 2af7ce84fb on Jul 20, 2020

MarcoFalke locked this on Sep 8, 2021

Basic Multiwallet GUI support #11383