gui: Add Open Wallet menu

promag commented at 12:24 pm on January 12, 2019: member

The Open Wallet menu has all the available wallets currently not loaded. The list of the available wallets comes from listWalletDir.

In the future the menu can be replaced by a custom dialog.

fanquake added the label GUI on Jan 12, 2019

DrahtBot commented at 2:01 pm on January 12, 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:

#15204 (gui: Add Open External Wallet action by promag)
#15202 (gui: Add Close All Wallets action by promag)
#15195 (gui: Add Close Wallet action by promag)

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.

promag force-pushed on Jan 12, 2019

hebasto commented at 2:48 pm on January 13, 2019: member

Concept ACK.

promag force-pushed on Jan 13, 2019

promag force-pushed on Jan 14, 2019

in src/qt/walletcontroller.cpp:83 in cc906b3aa1 outdated

78+    it = m_wallet_models.emplace(name, wallet_model).first;
79+
80+    connect(wallet_model, &WalletModel::unload, [this, it, wallet_model] {
81+        // Unregister wallet model and update current if necessary.
82+        {
83+            QMutexLocker locker(&m_mutex);

practicalswift commented at 1:28 pm on January 14, 2019:

Avoid shadowing locker in the outer scope :-)

promag commented at 1:36 pm on January 14, 2019:

Not shadowed, the above is not captured.

in src/qt/walletcontroller.cpp:53 in cc906b3aa1 outdated

48+        if (m_wallet_models.count(name) == 0) wallets.push_back(name);
49+    }
50+    return wallets;
51+}
52+
53+WalletModel* WalletController::loadWallet(const std::string& path)

practicalswift commented at 1:30 pm on January 14, 2019:

Make sure declaration parameter name matches definition parameter name :-)

in src/qt/walletcontroller.cpp:59 in cc906b3aa1 outdated

56+    WalletModel* wallet_model = getOrCreateModel(m_node.loadWallet(path, error, warning));
57+    if (!wallet_model) {
58+        //QMessageBox::information(this, tr("Open Wallet"), QString::fromStdString(error));
59+        return nullptr;
60+    }
61+    if (!warning.empty()) {

practicalswift commented at 1:33 pm on January 14, 2019:

Drop this since empty body?

in src/qt/walletcontroller.cpp:46 in cc906b3aa1 outdated

41+
42+std::vector<std::string> WalletController::getWalletsAvailableToLoad() const
43+{
44+    QMutexLocker locker(&m_mutex);
45+    std::vector<std::string> wallets;
46+    std::set<std::string> loaded;

practicalswift commented at 1:33 pm on January 14, 2019:

Unused? Please drop :-)

promag force-pushed on Jan 14, 2019

DrahtBot added the label Needs rebase on Jan 14, 2019

promag force-pushed on Jan 14, 2019

DrahtBot removed the label Needs rebase on Jan 14, 2019

promag force-pushed on Jan 14, 2019

promag force-pushed on Jan 15, 2019

DrahtBot added the label Needs rebase on Jan 15, 2019

promag force-pushed on Jan 15, 2019

DrahtBot removed the label Needs rebase on Jan 15, 2019

DrahtBot added the label Needs rebase on Jan 15, 2019

Sjors commented at 4:07 pm on January 17, 2019: member

Concept ACK. Code looks good at first glance; will review once upstream is merged.

promag force-pushed on Jan 17, 2019

DrahtBot removed the label Needs rebase on Jan 17, 2019

promag force-pushed on Jan 17, 2019

promag force-pushed on Jan 18, 2019

promag commented at 8:37 pm on January 18, 2019: member

Rebased.

jnewbery added this to the "Issues" column in a project

jnewbery moved this from the "Issues" to the "In progress" column in a project

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

fanquake commented at 3:05 am on January 19, 2019: member

Concept ACK.

How should we handle opening a wallet that requires rescanning/is partialling rescanned?

i.e with 67cdd14 if you start bitcoin-qt with -rescan, then open a wallet after the GUI has loaded, that wallet will be rescanned, which will block the UI with no output (expect for in debug.log).

promag force-pushed on Jan 19, 2019

promag commented at 3:54 pm on January 21, 2019: member

@fanquake good point! I wonder if it’s reasonable to tackle that later, to prevent further delays, or if it should be fixed now.

ATM I’m trying to come up with a simple solution.

promag commented at 5:04 pm on January 21, 2019: member

@fanquake updated with a WIP version of what could be temporary solution to loading the wallet asynchronously, making the UI usable. @ryanofsky I’d love your feedback here.

Sjors commented at 6:41 pm on January 21, 2019: member

I opened a wallet that requires a rescan. The UI doesn’t block, which is nice, but there’s no indicator anywhere (except the log file). This should probably open the standard rescan modal window. Ideally the cancel button in that case cancels the wallet load.

Sjors commented at 2:44 pm on January 22, 2019: member

I don’t think the progress bar actually works. Try opening a wallet that’s far behind. I also got a crash during or shortly after the rescan, not sure why (I think I canceled the rescan a while before the crash).

Suggest renaming “Loading” to “Scanning ‘[wallet name]’ for new transactions…”

Then add an explanation text below: “Closing this window may result in recent transactions not appearing in your wallet. To resume scanning for new transactions later, close and then re-open this wallet.”

If not too hard, maybe add a QT test that opens a wallet normally, and one that opens a wallet on a (too far) pruned node.

promag force-pushed on Jan 22, 2019

promag commented at 3:21 pm on January 22, 2019: member

Apply the following

 0--- a/src/wallet/wallet.cpp
 1+++ b/src/wallet/wallet.cpp
 2@@ -1684,6 +1684,8 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const CBlockIndex* const
 3         }
 4         double progress_current = progress_begin;
 5         while (pindex && !fAbortRescan && !ShutdownRequested()) {
 6+            fAbortRescan = true;
 7+            MilliSleep(3000);
 8             if (pindex->nHeight % 100 == 0 && progress_end - progress_begin > 0.0) {
 9                 ShowProgress(strprintf("%s " + _("Rescanning..."), GetDisplayName()), std::max(1, std::min(99, (int)((progress_current - progress_begin) / (progress_end - progress_begin) * 100))));
10             }

Launch the app:

0bitcoin-qt -regtest -rescan -nowallet -debug -printtoconsole

Then open a wallet in File->Open Wallet.

in src/interfaces/node.h:198 in 68fbe0d24c outdated

187@@ -188,6 +188,9 @@ class Node
188     //! Return interfaces for accessing wallets (if any).
189     virtual std::vector<std::unique_ptr<Wallet>> getWallets() = 0;
190 
191+    //! Attempts to load a wallet from file or directory.
192+    virtual std::unique_ptr<Wallet> loadWallet(const std::string& name, std::string& error, std::string& warning) = 0;

ryanofsky commented at 4:15 pm on January 22, 2019:

In commit “interfaces: Add loadWallet to Node” (68fbe0d24c5e003498915939e2b4756f59d3423d)

Comment should specify whether or not this is supposed to trigger the handleLoadWallet notification. It’s a bit strange if calling this returns one wallet pointer but leads to a handleLoadWallet event passing a different pointer. It might actually simplify things to have this method just return void and rely on the existing notification.

promag commented at 5:50 pm on January 22, 2019:

Currently BitcoinGUI::setCurrentWallet is only called when a wallet is opened with the GUI, not after handleLoadWallet notification. For that reason it’s returning the wallet interface. Beside, I was under the impression that it’s valid to have multiple interface instances to the same underlying wallet/node/etc.

ryanofsky commented at 6:14 pm on January 22, 2019:

I was under the impression that it’s valid to have multiple interface instances to the same underlying wallet/node/etc.

It will work, but I assume will cause unnecessary complexity and code duplication, because now there will be two different ways for the GUI to be updated when the wallet is loaded instead of just one. I think ideally this PR would add a new load command but reuse existing mechanisms for attaching new wallets to the GUI.

Whatever approach you take, I think should be a comment describing how this interacts with the load wallet notification.

promag commented at 0:00 am on January 30, 2019:

now there will be two different ways for the GUI to be updated when the wallet is loaded instead of just one

I think the current approach makes sense (as in wallet = loadwallet(...)) in the same way Qt has QFileDialog::getOpenFileName — the implementation is not ideal but some refactors are needed to make this better which I intend to work on.

I’ll add comments in the next push.

promag commented at 3:43 pm on February 4, 2019:

Added a small comment regarding the notification.

in src/interfaces/node.cpp:258 in 68fbe0d24c outdated

245@@ -244,6 +246,10 @@ class NodeImpl : public Node
246         }
247         return wallets;
248     }
249+    std::unique_ptr<Wallet> loadWallet(const std::string& name, std::string& error, std::string& warning) override
250+    {
251+        return MakeWallet(LoadWallet(*m_interfaces.chain, name, error, warning));

ryanofsky commented at 4:50 pm on January 22, 2019:

In commit “interfaces: Add loadWallet to Node” (68fbe0d24c5e003498915939e2b4756f59d3423d)

To work with separate wallet / node processes, this would need to change to something like:

0auto client = interfaces::MakeWalletClient(*interfaces.chain, {name});
1client->verify();
2client->load();
3client->start();
4m_interfaces.chain_clients.emplace_back(std::move(client));

and happen through the wallet init interface to disable & throw when ENABLE_WALLET isn’t defined.

It shouldn’t be hard to get this working, but it could also be done in another PR.

promag commented at 6:03 pm on January 22, 2019:

Care to elaborate? I don’t really understand why.

ryanofsky commented at 6:23 pm on January 22, 2019:

You can try #10102, but that PR creates three new executables: bitcoin-gui, bitcoin-node, and bitcoin-wallet. The current implementation of this method with that PR would try to invoke wallet code inside the bitcoin-node process. My suggested change would start a new bitcoin-wallet process and load the wallet from there.

promag commented at 0:04 am on January 30, 2019:

Thanks, I still have to go there (multi process) so I’ll defer this.

in src/qt/bitcoingui.cpp:379 in ace950b47a outdated

375                     OpenWalletActivity* activity = m_wallet_controller->openWallet(path);
376+
377+                    QProgressDialog* dialog = new QProgressDialog(this);
378+                    dialog->setLabelText(tr("Opening Wallet <b>%1</b>...").arg(name.toHtmlEscaped()));
379+                    dialog->setRange(0, 0);
380+                    dialog->setCancelButton(nullptr);

Sjors commented at 5:06 pm on January 22, 2019:

Shouldn’t this cancel the rescan?

promag commented at 5:28 pm on January 22, 2019:

It’s not possible at the moment.

Sjors commented at 6:25 pm on January 22, 2019:

Ok, but then I probably shouldn’t be able to close the dialog either? At least not without a warning that rescan will continue (and wallet won’t show up until that’s done).

promag commented at 6:30 pm on January 22, 2019:

I think we should keep this simple, maybe remove setWindowModality(Qt::ApplicationModal) and improve later.

in src/qt/bitcoingui.cpp:386 in ace950b47a outdated

382+                    dialog->show();
383+
384+                    connect(activity, &OpenWalletActivity::message, this, [this] (QMessageBox::Icon icon, QString text) {
385+                        QMessageBox box;
386+                        box.setIcon(icon);
387+                        box.setText(tr("Open Wallet Failed"));

Sjors commented at 5:07 pm on January 22, 2019:

How do I test this condition?

Sjors commented at 5:09 pm on January 22, 2019: member

Some relevant IRC chat:

ryanofsky commented at 5:34 pm on January 22, 2019: member

@ryanofsky I’d love your feedback here

Maybe this is feedback for #15101, but the threading in commit “interfaces: Avoid interface instance if wallet is null” (130752bfa494d8a7608acface807000d0d84c02e), seems right to me, but threading in commit “gui: Add WalletController” (8fa271f08969b440cbc4aeb760db41c556ecf9c5) seems unusual.

Would probably be cleaner to avoid adding a mutex and checking currentThread() in WalletController, and instead follow a model where model/view/controller code generally runs the main gui event thread with no locks, and all blocking calls & wallet notifications happen in their own threads and signal the GUI thread asynchronously.

promag force-pushed on Jan 29, 2019

promag commented at 1:28 am on January 29, 2019: member

Rebased on #15280. @Sjors apply

 0--- a/src/wallet/wallet.cpp
 1+++ b/src/wallet/wallet.cpp
 2@@ -1683,7 +1683,10 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const CBlockIndex* const
 3             }
 4         }
 5         double progress_current = progress_begin;
 6+        int n = 5;
 7         while (pindex && !fAbortRescan && !ShutdownRequested()) {
 8+            MilliSleep(1000);
 9+            if (n-- == 0) fAbortRescan = true;
10             if (pindex->nHeight % 100 == 0 && progress_end - progress_begin > 0.0) {
11                 ShowProgress(strprintf("%s " + _("Rescanning..."), GetDisplayName()), std::max(1, std::min(99, (int)((progress_current - progress_begin) / (progress_end - progress_begin) * 100))));
12             }

then wallet File -> Open Wallet -> [default wallet]. You can also cmd+q right after opening the wallet.

promag force-pushed on Jan 29, 2019

in src/qt/walletcontroller.cpp:137 in 58366f6edb outdated

132+    std::unique_ptr<interfaces::Wallet> wallet = m_wallet_controller->m_node.loadWallet(m_name, error, warning);
133+    if (!warning.empty()) {
134+        Q_EMIT message(QMessageBox::Warning, QString::fromStdString(warning));
135+    }
136+    if (wallet) {
137+        printf("wallet loaeded!\n");

Sjors commented at 4:44 pm on January 29, 2019:

Nit: don’t forget to remove (or at least fix typo)

promag commented at 3:42 pm on February 4, 2019:

Done.

Sjors commented at 4:49 pm on January 29, 2019: member

If I open a wallet that’s far behind and then click the (x) button, the rescan progress bar goes away, but afaik it’s still scanning. If I then open it again from the menu, I get a “duplicate file name” error a bit later, and the log says:

0wallet loaeded!
1wallet not loaded :(

If the rescan can’t be aborted, then it shouldn’t be possible to close the modal (and there should a text saying that).

Alternatively, you could grey-out the menu entry for the specific wallet or the entire load wallet menu. At least that gives some hint the user needs to wait (or quit QT).

Other than that the code as of 964fe494a68c06490fba2e27ad924a4f67b6ae5f looks pretty clean.

jonasschnelli commented at 8:39 pm on January 29, 2019: contributor

This looks pretty good. @promag: can you answer, implement or reject @ryanofsky points and check if the rescan issue @Sjors mentioned?

promag commented at 0:06 am on January 30, 2019: member

@Sjors thanks for the review. Sorry those trash messages.. Will improve that case the next push.

in src/interfaces/node.cpp:32 in 964fe494a6 outdated

28@@ -29,6 +29,7 @@
29 #include <ui_interface.h>
30 #include <util/system.h>
31 #include <validation.h>
32+#include <wallet/walletutil.h>

hebasto commented at 8:32 am on January 30, 2019:

This makes these redeclarations: https://github.com/bitcoin/bitcoin/blob/964fe494a68c06490fba2e27ad924a4f67b6ae5f/src/interfaces/node.cpp#L43-L44 redundant.

promag commented at 3:42 pm on February 4, 2019:

Removed include.

hebasto changes_requested

in src/wallet/rpcwallet.cpp:2523 in 964fe494a6 outdated

2490@@ -2491,6 +2491,8 @@ static UniValue listwallets(const JSONRPCRequest& request)
2491     return obj;
2492 }
2493 
2494+std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocation& location, std::string& error, std::string& warning);

hebasto commented at 9:16 am on January 30, 2019:

It seems a redundant redeclaration. Could be removed.

promag commented at 3:42 pm on February 4, 2019:

Done.

in src/interfaces/wallet.cpp:526 in 964fe494a6 outdated

513@@ -514,7 +514,7 @@ class WalletClientImpl : public ChainClient
514 
515 } // namespace
516 
517-std::unique_ptr<Wallet> MakeWallet(const std::shared_ptr<CWallet>& wallet) { return MakeUnique<WalletImpl>(wallet); }
518+std::unique_ptr<Wallet> MakeWallet(const std::shared_ptr<CWallet>& wallet) { return wallet ? MakeUnique<WalletImpl>(wallet) : nullptr; }

kallewoof commented at 6:12 am on January 31, 2019:

I think just returning MakeUnique<WalletImpl>(wallet) here works even if !wallet, no?

kallewoof commented at 8:10 am on January 31, 2019:

I may have to withdraw this suggestion. I was looking at

https://github.com/bitcoin/bitcoin/blob/cb77dc820f1bc1965a9d40759feb201d7869cfa9/src/wallet/db.h#L140-L150

which may croak on a null value as opposed to an empty one.

promag commented at 8:12 am on January 31, 2019:

Exactly, thanks anyway.

in src/qt/walletcontroller.cpp:38 in 964fe494a6 outdated

34 // available in the header, just forward declared.
35-WalletController::~WalletController() {}
36+WalletController::~WalletController()
37+{
38+    m_activity_thread.quit();
39+    m_activity_thread.wait();

kallewoof commented at 6:22 am on January 31, 2019:

Maybe I’m missing something subtle, but it looks like you also need to requestInterruption before waiting:

0#if QT_VERSION >= QT_VERSION_CHECK(5,2,0)
1    m_activity_thread.requestInterruption();
2#endif
3    m_activity_thread.wait();

(from https://stackoverflow.com/questions/25224575/how-to-safely-destruct-a-qthread)

promag commented at 7:06 pm on January 31, 2019:

@kallewoof that’s when you reimplement QThread::run in a sub class. It’s not necessary when you use the even loop.

kallewoof commented at 6:25 am on January 31, 2019: member

utACK

gwillen commented at 6:10 am on February 1, 2019: contributor

Tested a bit on OS X (just the basics.) Looks good. :-)

Nit: When there are no more wallets to open, it’s weird that clicking “Open Wallet >” does nothing. I think it’s typical in this situation to either gray out the menu item, OR (better) open a popup with a single greyed out item reading something like “(No wallets to open)”.

in src/qt/bitcoingui.cpp:368 in 964fe494a6 outdated

364@@ -361,6 +365,37 @@ void BitcoinGUI::createActions()
365         connect(usedSendingAddressesAction, &QAction::triggered, walletFrame, &WalletFrame::usedSendingAddresses);
366         connect(usedReceivingAddressesAction, &QAction::triggered, walletFrame, &WalletFrame::usedReceivingAddresses);
367         connect(openAction, &QAction::triggered, this, &BitcoinGUI::openClicked);
368+        connect(m_open_wallet_action->menu(), &QMenu::aboutToShow, [this] {

gwillen commented at 6:25 am on February 1, 2019:

Should probably create another method or two, rather than stuffing this all into a lambda?

promag commented at 8:14 am on February 1, 2019:

I’m pretty sure this will go away from this class so I prefer to have it all in one place.

promag commented at 8:17 am on February 1, 2019: member

When there are no more wallets to open, it’s weird that clicking “Open Wallet >” does nothing @gwillen another PR adds a fixed action in the menu “Open Other…” so that won’t be an issue.

fanquake added the label Needs rebase on Feb 4, 2019

promag force-pushed on Feb 4, 2019

DrahtBot removed the label Needs rebase on Feb 4, 2019

wallet: Factor out LoadWallet 17abc0fd52

interfaces: Add loadWallet to Node ab288b4e59

gui: Add openWallet and getWalletsAvailableToOpen to WalletController 32a8c6abfe

gui: Add Open Wallet menu 6c49a55b47

gui: Add thread to run background activity in WalletController be82dea23c

interfaces: Avoid interface instance if wallet is null 4c8982a88e

promag force-pushed on Feb 4, 2019

gui: Add OpenWalletActivity 8847cdaaae

gui: Show indeterminate progress dialog while opening walllet 1951ea4342

promag force-pushed on Feb 4, 2019

laanwj added this to the milestone 0.18.0 on Feb 5, 2019

meshcollider commented at 8:56 pm on February 5, 2019: contributor

Concept ACK, I agree that we should try hard to get this into 0.18, will review shortly

Sjors commented at 9:51 am on February 6, 2019: member

Here’s a testnet wallet last synced in December 2017. When you open it it starts a rescan in the background. When you cancel it, that rescan doesn’t stop afaik. Weird things then happen if you close QT or try to open another or even the same wallet.

Maybe just calling abort rescan internally would be the easiest short term fix? Safest would be to make that blocking (so don’t close the modal until the cancel is done).

old.dat.zip

promag commented at 10:02 am on February 6, 2019: member

@Sjors can’t abort rescan because the rescan happens before getting the wallet instance on the GUI. I plan to work on that next.

In more detail, CWallet::CreateWalletFromFile is a huge “atomic” operation — among other things it rescans — and as such it’s not possible to abort the rescan.

jonasschnelli commented at 6:50 am on February 7, 2019: contributor

Tested ACK 1951ea4342db4122032660139248b79a02c574f3 @Sjors and @MeshCollider can you finalise your review (ack/nack/feedback)? It’d like to merge this asap. Non-Blocking opening and other improvements can be added later.

fanquake commented at 6:53 am on February 8, 2019: member

tACK 1951ea4 on top of master (b9b26d9) on macOS 10.14.3

Tested basic loading/unloading opening wallets with the new menu (using regtest):

bitcoin-qt with the default wallet loaded. open-menu only default wallet

src/bitcoin-cli -regtest createwallet example

createwallet example

src/bitcoin-cli -regtest unloadwallet example

after unloadwallet example

When loading the example wallet through the Open menu, there is a flash of the rescan/load dialogue (because the load/rescan is fast). In future maybe this could be cleaned up somehow?, because the flashing modals could end up being confusing.

Also tested using mainnet, with wallets that would have long rescan times:

0src/bitcoin-qt -rescan
1src/bitcoin-cli createwallet test
2src/bitcoin-cli -rpcwallet=test importaddress 18cBEMRxXHqzWWCxZNtU91F5sbUNKhL5PX "lots of tx" false

While a wallet is importing the Open wallet menu, and all other operations, are disabled:

However, you can currently close the importing modal, then open the same wallet again, which results in the wallet opening (from the first open), the second opening will obviously fail:

In future this could be cleaned up, so that if you close the modal, the wallet that is being imported isn’t still available in the Open menu.

Sjors commented at 9:46 am on February 8, 2019: member

I can’t reason about the things that might go wrong when a wallet keeps rescanning after a user cancels the load action. I fully expect users to just keep trying to open the same wallet, while that rescan is still going. Can it lead to data loss? Crashes?

If others think it’s perfectly safe than do say so… But otherwise, fixing it later would block 0.18 release.

Here’s a few ideas:

Put a warning text in the dialog like “Please wait. Don’t close this dialog.”
Prevent closing the dialog (quitting QT should still work)
Shut down QT if the user closes the dialog

promag commented at 2:16 pm on February 8, 2019: member

while that rescan is still going. Can it lead to data loss? Crashes?

The same concerns are valid for loadwallet RPC. This PR doesn’t make it worse IMO. The UX can be improved for sure!

in src/wallet/rpcwallet.cpp:2559 in 1951ea4342

2566-    }
2567-    AddWallet(wallet);
2568-
2569-    wallet->postInitProcess();
2570+    std::string error, warning;
2571+    std::shared_ptr<CWallet> const wallet = LoadWallet(*g_rpc_interfaces->chain, location, error, warning);

jnewbery commented at 3:14 pm on February 8, 2019:

Why isn’t the file existence checking above also moved to LoadWallet()? If I call LoadWallet() with a filename that doesn’t exist or doesn’t contain a wallet.dat file, LoadWallet() will create a new wallet file. I don’t think that’s desired behaviour.

I can trigger this by hovering over the ‘Open Wallet’ menu in qt, then deleting the wallet file in a different terminal, then trying to load the wallet.

promag commented at 5:29 pm on February 11, 2019:

Correct me if I’m wrong but technically a race would still exist even if the check is moved, it would be hard to trigger it.

Anyway, in order to move the check to LoadWallet I have to somehow map the “not found error” to RPC_WALLET_NOT_FOUND - at the moment if LoadWallet fails the raised error is RPC_WALLET_ERROR - so I would have to change the interface.

Doing what you suggest has the advantage of reducing references to wallet.dat: https://github.com/bitcoin/bitcoin/blob/1bc149d05b09d716723d2f091250fab38fd70fc2/src/wallet/rpcwallet.cpp#L2564 And leave it just to src/wallet/db.cpp.

Considering I’d like to refactor interfaces::Node::loadWallet to be asynchronous - to know load progress, to cancel/abort, to know errors/warns - I’ll have to change these calls too.

Considering the above ACK’s I suggest to follow up your and @Sjors improvements.

WDYT?

jnewbery commented at 7:16 pm on February 13, 2019:

technically a race would still exist even if the check is moved

Yes, you’re right. The race is still there, but it’s very hard to trigger if the checks are moved into LoadWallet().

Maybe this is too hacky, but perhaps the error code raised by the RPC could be set based on the error string returned by LoadWallet()? Or we could just change the return code in this case to a generic RPC_WALLET_ERROR.

Whatever way we go, I think it’s fine to defer changing this to a future PR.

promag commented at 7:25 pm on February 13, 2019:

That’s why I think we should just try to open instead of checking before opening - those checks could be removed.

jnewbery commented at 7:28 pm on February 13, 2019:

we should just try to open instead of checking before opening

Sure, as long as ’try to open’ doesn’t create a new wallet if one isn’t there (which I believe is what happens currently).

jnewbery added the label Wallet on Feb 8, 2019

Sjors commented at 3:37 pm on February 10, 2019: member

@promag RPC users are usually far more technically sophisticated than GUI users. In particular, I expect them to be more aware of the risk of data corruption.

jonasschnelli commented at 7:01 pm on February 10, 2019: contributor

I think the rescan/dialog situation can be improved later… let’s try to get this in and make progress. I see both points (RPC/Console loadwallet has the same flaw as well as the GUI should do more hand-holding).

jonasschnelli merged this on Feb 12, 2019

jonasschnelli closed this on Feb 12, 2019

jonasschnelli referenced this in commit 7d3f255316 on Feb 12, 2019

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

promag deleted the branch on Feb 12, 2019

promag commented at 8:05 pm on February 12, 2019: member

I’ll follow up improvements right after 0.18 branch.

in src/qt/walletcontroller.cpp:137 in 8847cdaaae outdated

132+    std::unique_ptr<interfaces::Wallet> wallet = m_wallet_controller->m_node.loadWallet(m_name, error, warning);
133+    if (!warning.empty()) {
134+        Q_EMIT message(QMessageBox::Warning, QString::fromStdString(warning));
135+    }
136+    if (wallet) {
137+        Q_EMIT opened(m_wallet_controller->getOrCreateWallet(std::move(wallet)));

ryanofsky commented at 6:59 pm on February 13, 2019:

In commit “gui: Add OpenWalletActivity” (8847cdaaaeb45c1ddee89f43ac4b8fafb20e5c0d) @promag, would there there any downsides to deleting this line? It seems redundant and as far as I can tell. Everything seems to works fine without it due to the load wallet event.

I’m trying to implement my suggestion from #15153 (review) (so wallets will be loaded in the bitcoin-wallet processes not inside the bitcoin-node process), and I’d prefer just to have one way to attach a Wallet to the gui, not two different ways.

promag commented at 7:08 pm on February 13, 2019:

Note that the signal opened is connected to the setCurrentlyWallet but there is currently a problem there because the wallet view is created after, which needs to be fixed - I was already aware of this.

I don’t think that’s a good idea (only use the notification) - suppose you have 2 windows, you open a wallet on each (because the loading is async and doesn’t block the GUI) you should see each wallet on the expected window.

ryanofsky commented at 7:37 pm on February 13, 2019:

Thanks, a multi window case is helpful to think about. I know you are working on fixing sync stuff in the gui, but what I would expect when loading a slow wallet is for the window to show “Loading wallet X…” right after the user selects the wallet, so the user has some immediate feedback. Then when the loadWallet notification is triggered, the GUI would use the Wallet handle passed with the notification to display actual wallet information. There would be no need for the call which the GUI makes to request loading of the wallet to return a wallet handle.

It sounds like this a concern for the future, though. Thanks for answering my immediate question.

promag commented at 7:55 pm on February 13, 2019:

But how would it know if the notified wallet handle belongs to the wallet opened? Wallet path?

ryanofsky commented at 8:34 pm on February 13, 2019:

Yes, I was just thinking wallet name.

jnewbery commented at 7:17 pm on February 13, 2019: member

tested ACK 1951ea4342db4122032660139248b79a02c574f3. Glad to see this make it for 0.18!

fanquake moved this from the "In progress" to the "Done" column in a project

deadalnix referenced this in commit 65b70e37d6 on May 16, 2020

deadalnix referenced this in commit 9df5db9b96 on May 18, 2020

deadalnix referenced this in commit 39153d2e62 on May 18, 2020

deadalnix referenced this in commit f4a83401a2 on May 19, 2020

deadalnix referenced this in commit 706743973b on May 19, 2020

deadalnix referenced this in commit e616c35ce4 on May 19, 2020

deadalnix referenced this in commit 756d5ab4ae on May 19, 2020

deadalnix referenced this in commit 03ee4b87a7 on May 19, 2020

jonspock referenced this in commit 38a01214ed on Aug 14, 2020

jonspock referenced this in commit f514f240a2 on Aug 27, 2020

jonspock referenced this in commit 2254f05f66 on Aug 28, 2020

jonspock referenced this in commit edc7a2deb4 on Sep 4, 2020

jonspock referenced this in commit ddbd06df00 on Sep 5, 2020

jonspock referenced this in commit 92ae27f8a1 on Sep 6, 2020

jonspock referenced this in commit 53479271d5 on Sep 13, 2020

jonspock referenced this in commit ecbd6c8a52 on Sep 13, 2020

jonspock referenced this in commit 38b58b9bc5 on Sep 15, 2020

jonspock referenced this in commit 44b783bab0 on Sep 16, 2020

jonspock referenced this in commit 5956733588 on Sep 16, 2020

jonspock referenced this in commit bf08c2c0d3 on Sep 17, 2020

jonspock referenced this in commit 407f328068 on Sep 18, 2020

jonspock referenced this in commit 2d1cc6b9ee on Sep 21, 2020

jonspock referenced this in commit 5896933a74 on Sep 21, 2020

jonspock referenced this in commit 7fe4364243 on Sep 23, 2020

jonspock referenced this in commit 3ae1054239 on Sep 24, 2020

proteanx referenced this in commit 23ac88984c on Sep 25, 2020

Munkybooty referenced this in commit 7ff543eac0 on Aug 27, 2021

PastaPastaPasta referenced this in commit 0fd3f230c4 on Sep 10, 2021

PastaPastaPasta referenced this in commit 1aa84f8a3b on Oct 23, 2021

pravblockc referenced this in commit b8b3c8090d on Nov 18, 2021

MarcoFalke locked this on Dec 16, 2021

gui: Add Open Wallet menu #15153

Conflicts