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

gui: Add Open Wallet menu #15153

Merged
merged 8 commits into from Feb 12, 2019

Conversation

@promag
Copy link
Member

promag commented Jan 12, 2019

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.

screenshot 2019-01-12 at 12 17 02

@fanquake fanquake added the GUI label Jan 12, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jan 12, 2019

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 promag force-pushed the promag:2019-01-openwallet branch 3 times, most recently Jan 12, 2019

@hebasto

This comment has been minimized.

Copy link
Member

hebasto commented Jan 13, 2019

Concept ACK.

@promag promag force-pushed the promag:2019-01-openwallet branch 2 times, most recently Jan 13, 2019

Show resolved Hide resolved src/qt/walletcontroller.cpp Outdated
Show resolved Hide resolved src/qt/walletcontroller.cpp Outdated
Show resolved Hide resolved src/qt/walletcontroller.cpp Outdated
Show resolved Hide resolved src/qt/walletcontroller.cpp Outdated

@promag promag force-pushed the promag:2019-01-openwallet branch Jan 14, 2019

@promag promag referenced this pull request Jan 14, 2019

Merged

gui: Add WalletController #15101

@promag promag force-pushed the promag:2019-01-openwallet branch Jan 14, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jan 14, 2019

@promag promag force-pushed the promag:2019-01-openwallet branch 2 times, most recently Jan 14, 2019

@promag promag force-pushed the promag:2019-01-openwallet branch Jan 15, 2019

@DrahtBot DrahtBot added Needs rebase and removed Needs rebase labels Jan 15, 2019

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Jan 17, 2019

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

@promag promag force-pushed the promag:2019-01-openwallet branch Jan 17, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jan 17, 2019

@promag promag force-pushed the promag:2019-01-openwallet branch 3 times, most recently Jan 17, 2019

@promag promag force-pushed the promag:2019-01-openwallet branch Jan 18, 2019

@jnewbery jnewbery referenced this pull request Jan 18, 2019

Open

Dynamic wallet load / create / unload #13059

8 of 10 tasks complete

@promag promag force-pushed the promag:2019-01-openwallet branch from 5201578 to d0307ef Feb 4, 2019

@promag promag force-pushed the promag:2019-01-openwallet branch from d0307ef to 1951ea4 Feb 4, 2019

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

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Feb 5, 2019

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

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Feb 6, 2019

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

This comment has been minimized.

Copy link
Member Author

promag commented Feb 6, 2019

@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

This comment has been minimized.

Copy link
Member

jonasschnelli commented Feb 7, 2019

Tested ACK 1951ea4

@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

This comment has been minimized.

Copy link
Member

fanquake commented Feb 8, 2019

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:

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:

menu disabled during importing

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:

duplicate wallet loading

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

This comment has been minimized.

Copy link
Member

Sjors commented Feb 8, 2019

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:

  1. Put a warning text in the dialog like "Please wait. Don't close this dialog."
  2. Prevent closing the dialog (quitting QT should still work)
  3. Shut down QT if the user closes the dialog
@promag

This comment has been minimized.

Copy link
Member Author

promag commented Feb 8, 2019

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!


wallet->postInitProcess();
std::string error, warning;
std::shared_ptr<CWallet> const wallet = LoadWallet(*g_rpc_interfaces->chain, location, error, warning);

This comment has been minimized.

@jnewbery

jnewbery Feb 8, 2019

Member

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.

This comment has been minimized.

@promag

promag Feb 11, 2019

Author Member

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:

fs::path wallet_dat_file = location.GetPath() / "wallet.dat";

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?

This comment has been minimized.

@jnewbery

jnewbery Feb 13, 2019

Member

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.

This comment has been minimized.

@promag

promag Feb 13, 2019

Author Member

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

This comment has been minimized.

@jnewbery

jnewbery Feb 13, 2019

Member

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 jnewbery added the Wallet label Feb 8, 2019

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Feb 10, 2019

@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

This comment has been minimized.

Copy link
Member

jonasschnelli commented Feb 10, 2019

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 jonasschnelli merged commit 1951ea4 into bitcoin:master Feb 12, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonasschnelli added a commit that referenced this pull request Feb 12, 2019

Merge #15153: gui: Add Open Wallet menu
1951ea4 gui: Show indeterminate progress dialog while opening walllet (João Barbosa)
8847cda gui: Add OpenWalletActivity (João Barbosa)
4c8982a interfaces: Avoid interface instance if wallet is null (João Barbosa)
be82dea gui: Add thread to run background activity in WalletController (João Barbosa)
6c49a55 gui: Add Open Wallet menu (João Barbosa)
32a8c6a gui: Add openWallet and getWalletsAvailableToOpen to WalletController (João Barbosa)
ab288b4 interfaces: Add loadWallet to Node (João Barbosa)
17abc0f wallet: Factor out LoadWallet (João Barbosa)

Pull request description:

  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">

Tree-SHA512: ebfd75eee0c8264863748899843afab67dadb7dff21313c11e3cb5b6108d954978dd1f1ae786bc07580c5a771ea4ab38d18c1643c9b9b3683ed53f0f6c582e38

@jonasschnelli jonasschnelli removed this from Blockers in High-priority for review Feb 12, 2019

@promag promag deleted the promag:2019-01-openwallet branch Feb 12, 2019

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Feb 12, 2019

I'll follow up improvements right after 0.18 branch.

Q_EMIT message(QMessageBox::Warning, QString::fromStdString(warning));
}
if (wallet) {
Q_EMIT opened(m_wallet_controller->getOrCreateWallet(std::move(wallet)));

This comment has been minimized.

@ryanofsky

ryanofsky Feb 13, 2019

Contributor

In commit "gui: Add OpenWalletActivity" (8847cda)

@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 (comment) (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.

This comment has been minimized.

@promag

promag Feb 13, 2019

Author Member

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.

This comment has been minimized.

@ryanofsky

ryanofsky Feb 13, 2019

Contributor

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.

This comment has been minimized.

@promag

promag Feb 13, 2019

Author Member

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

This comment has been minimized.

@ryanofsky

ryanofsky Feb 13, 2019

Contributor

Yes, I was just thinking wallet name.

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Feb 13, 2019

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

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