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.
<img width="674" alt="screenshot 2019-01-12 at 12 17 02" src="https://user-images.githubusercontent.com/3534524/51073166-ac041480-1664-11e9-8302-be81702bc146.png">
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
Concept ACK.
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);
Avoid shadowing locker in the outer scope :-)
Not shadowed, the above is not captured.
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)
Make sure declaration parameter name matches definition parameter name :-)
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()) {
Drop this since empty body?
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;
Unused? Please drop :-)
Concept ACK. Code looks good at first glance; will review once upstream is merged.
Rebased.
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).
@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.
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.
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.
Apply the following
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1684,6 +1684,8 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const CBlockIndex* const
}
double progress_current = progress_begin;
while (pindex && !fAbortRescan && !ShutdownRequested()) {
+ fAbortRescan = true;
+ MilliSleep(3000);
if (pindex->nHeight % 100 == 0 && progress_end - progress_begin > 0.0) {
ShowProgress(strprintf("%s " + _("Rescanning..."), GetDisplayName()), std::max(1, std::min(99, (int)((progress_current - progress_begin) / (progress_end - progress_begin) * 100))));
}
Launch the app:
bitcoin-qt -regtest -rescan -nowallet -debug -printtoconsole
Then open a wallet in File->Open Wallet.
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;
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.
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.
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.
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.
Added a small comment regarding the notification.
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));
In commit "interfaces: Add loadWallet to Node" (68fbe0d24c5e003498915939e2b4756f59d3423d)
To work with separate wallet / node processes, this would need to change to something like:
auto client = interfaces::MakeWalletClient(*interfaces.chain, {name});
client->verify();
client->load();
client->start();
m_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.
Care to elaborate? I don't really understand why.
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.
Thanks, I still have to go there (multi process) so I'll defer this.
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);
Shouldn't this cancel the rescan?
It's not possible at the moment.
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).
I think we should keep this simple, maybe remove setWindowModality(Qt::ApplicationModal) and improve later.
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"));
How do I test this condition?
Some relevant IRC chat: <img width="1100" alt="schermafbeelding 2019-01-22 om 18 08 43" src="https://user-images.githubusercontent.com/10217/51552389-c8782c00-1e70-11e9-86c5-403373a496d6.png">
@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.
Rebased on #15280. @Sjors apply
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1683,7 +1683,10 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const CBlockIndex* const
}
}
double progress_current = progress_begin;
+ int n = 5;
while (pindex && !fAbortRescan && !ShutdownRequested()) {
+ MilliSleep(1000);
+ if (n-- == 0) fAbortRescan = true;
if (pindex->nHeight % 100 == 0 && progress_end - progress_begin > 0.0) {
ShowProgress(strprintf("%s " + _("Rescanning..."), GetDisplayName()), std::max(1, std::min(99, (int)((progress_current - progress_begin) / (progress_end - progress_begin) * 100))));
}
then wallet File -> Open Wallet -> [default wallet]. You can also cmd+q right after opening the wallet.
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");
Nit: don't forget to remove (or at least fix typo)
Done.
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:
wallet loaeded!
wallet 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.
This looks pretty good. @promag: can you answer, implement or reject @ryanofsky points and check if the rescan issue @Sjors mentioned?
28 | @@ -29,6 +29,7 @@ 29 | #include <ui_interface.h> 30 | #include <util/system.h> 31 | #include <validation.h> 32 | +#include <wallet/walletutil.h>
This makes these redeclarations: https://github.com/bitcoin/bitcoin/blob/964fe494a68c06490fba2e27ad924a4f67b6ae5f/src/interfaces/node.cpp#L43-L44 redundant.
Removed include.
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);
It seems a redundant redeclaration. Could be removed.
Done.
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; }
I think just returning MakeUnique<WalletImpl>(wallet) here works even if !wallet, no?
I may have to withdraw this suggestion. I was looking at
which may croak on a null value as opposed to an empty one.
Exactly, thanks anyway.
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();
Maybe I'm missing something subtle, but it looks like you also need to requestInterruption before waiting:
#if QT_VERSION >= QT_VERSION_CHECK(5,2,0)
m_activity_thread.requestInterruption();
#endif
m_activity_thread.wait();
(from https://stackoverflow.com/questions/25224575/how-to-safely-destruct-a-qthread)
@kallewoof that's when you reimplement QThread::run in a sub class. It's not necessary when you use the even loop.
utACK
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)".
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] {
Should probably create another method or two, rather than stuffing this all into a lambda?
I'm pretty sure this will go away from this class so I prefer to have it all in one place.
Concept ACK, I agree that we should try hard to get this into 0.18, will review shortly
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).
@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.
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.
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.
src/bitcoin-cli -regtest createwallet example
src/bitcoin-cli -regtest 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:
src/bitcoin-qt -rescan
src/bitcoin-cli createwallet test
src/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:
<img width="522" alt="menu disabled during importing" src="https://user-images.githubusercontent.com/863730/52462636-d804ae00-2bae-11e9-91a1-fd879aa663e2.png">
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:
<img width="446" alt="duplicate wallet loading" src="https://user-images.githubusercontent.com/863730/52463206-071c1f00-2bb1-11e9-96e5-39dad321a575.png">
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.
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:
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!
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);
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.
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?
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.
That's why I think we should just try to open instead of checking before opening - those checks could be removed.
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).
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).
I'll follow up improvements right after 0.18 branch.
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)));
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.
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.
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.
But how would it know if the notified wallet handle belongs to the wallet opened? Wallet path?
Yes, I was just thinking wallet name.
tested ACK 1951ea4342db4122032660139248b79a02c574f3. Glad to see this make it for 0.18!
Milestone
0.18.0