Skip to content
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 Close Wallet action #15195

Merged
merged 3 commits into from Feb 14, 2019

Conversation

@promag
Copy link
Member

commented Jan 18, 2019

This PR adds support to close the current wallet in the GUI.

screenshot 2019-01-18 at 00 44 26

screenshot 2019-01-18 at 00 44 38

@fanquake fanquake added the GUI label Jan 18, 2019

@IlyasRidhuan

This comment has been minimized.

Copy link

commented Jan 18, 2019

Ack 2fef185
Tested open and close wallet behaviour with multiple wallets loaded

@promag promag changed the title gui: Add close wallet action gui: Add Close Wallet action Jan 18, 2019

@hebasto

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

Concept ACK.

@harding

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

Tested works for me on Debian stable. Code not reviewed. I did get some compiler warnings:

  CXX      interfaces/libbitcoin_server_a-node.o
interfaces/node.cpp:42:10: warning: redundant redeclaration of ‘boost::filesystem::path GetWalletDir()’ in same scope [-Wredundant-decls]
 fs::path GetWalletDir();
          ^~~~~~~~~~~~
In file included from interfaces/node.cpp:31:0:
./wallet/walletutil.h:13:10: note: previous declaration of ‘boost::filesystem::path GetWalletDir()’
 fs::path GetWalletDir();
          ^~~~~~~~~~~~
interfaces/node.cpp:43:23: warning: redundant redeclaration of ‘std::vector<boost::filesystem::path> ListWalletDir()’ in same scope [-Wredundant-decls]
 std::vector<fs::path> ListWalletDir();
                       ^~~~~~~~~~~~~
In file included from interfaces/node.cpp:31:0:
./wallet/walletutil.h:16:23: note: previous declaration of ‘std::vector<boost::filesystem::path> ListWalletDir()’
 std::vector<fs::path> ListWalletDir();
                       ^~~~~~~~~~~~~



  CXX      wallet/libbitcoin_wallet_a-rpcwallet.o
wallet/rpcwallet.cpp:2494:26: warning: redundant redeclaration of ‘std::shared_ptr<CWallet> LoadWallet(interfaces::Chain&, const WalletLocation&, std::__cxx11::string&, std::__cxx11::string&)’ in same scope [-Wredundant-decls]
 std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocation& location, std::string& error, std::string& warning);
                          ^~~~~~~~~~
In file included from ./wallet/coincontrol.h:11:0,
                 from wallet/rpcwallet.cpp:31:
./wallet/wallet.h:69:26: note: previous declaration of ‘std::shared_ptr<CWallet> LoadWallet(interfaces::Chain&, const WalletLocation&, std::__cxx11::string&, std::__cxx11::string&)’
 std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocation& location, std::string& error, std::string& warning);
                          ^~~~~~~~~~

@jnewbery jnewbery referenced this pull request Jan 18, 2019

Open

Dynamic wallet load / create / unload #13059

8 of 10 tasks complete

@jnewbery jnewbery added this to Issues in Multiwallet support Jan 18, 2019

@jnewbery jnewbery moved this from Issues to In progress in Multiwallet support Jan 18, 2019

@flack

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

Is it possible to visually separate the wallet name from the question text in the confirmation dialog? For example by putting quotes around it or adding a line break before.

Silly example: A wallet named ??!? would currently render

Are you sure you wish to close wallet ??!??

This would look slightly less confusing:

Are you sure you wish to close wallet "??!?"?
@promag

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2019

@flack good point, I'll see other examples too.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 19, 2019

Concept ACK.

When the node is pruned, can you add a warning that closing the wallet for too long can result in having to resync the entire chain?

Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
Show resolved Hide resolved src/qt/bitcoingui.cpp Outdated
Show resolved Hide resolved src/qt/walletcontroller.cpp Outdated

@promag promag force-pushed the promag:2019-01-closewallet branch to cfd9d77 Jan 19, 2019

@meshcollider meshcollider added this to the 0.18.0 milestone Feb 8, 2019

@meshcollider meshcollider added the Wallet label Feb 10, 2019

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

Needs rebase now that open wallet is merged.

@promag promag force-pushed the promag:2019-01-closewallet branch from cfd9d77 to 94086fb Feb 12, 2019

@DrahtBot DrahtBot removed the Needs rebase label Feb 12, 2019

@promag

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

@flack currently the wallet name is in italic.

@practicalswift you reviewed commits that belong #15153, but I'll take them into account once I touch that code again.

@hebasto changed to property based.

@Sjors rebased and added your suggestion.

@promag

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

Note to reviewers, closing the wallet doesn't explicitly unloads it, just tries to drop all references to the wallet - in other words, it doesn't wait for some RPC worker thread, if any, to finish its job.

Please give feedback if this should unload the wallet, like unloadwallet RPC.

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Eventually it should unload the wallet, because an advanced user may want to be able to move files around. It would also make it consistent with the RPC if they both call UnloadWallet() (which would have to added to an interface).

It can be done later as long as it doesn't break stuff. I was able to open and close the same wallet multiple times without a crash, so that's good.

Much more useful, once #11082 is merged, is for this menu item to remove the wallet from rw_config, so that it remains closed on the next launch.

tACK 94086fb

@gwillen

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

Tested on OS X, seems to work great. tACK 94086fb, notwithstanding the kibbitz below about unloading.

As to unloading, am I correctly understanding the process: the unloadwallet RPC will block until the wallet is unloaded, which could take time if someone has taken a reference to the shared_ptr, because UnloadWallet will block until they drop it. The approach taken in this PR is not to block, but to remove the wallet from the UI's list immediately; if someone still has a reference to it, it will not actually be unloaded until they drop the reference?

It seems like the worst case here is: a user closes a wallet on which some long-running operation is holding the CWallet open. (Is this possible? I don't know how long a wallet can take to unload.) Then the wallet will appear in the Open Wallet... menu, since getWalletsAvailableToOpen will not find it already open (since it's searching the WalletController's WalletModel list, not the CWallet list.) But if it's clicked, opening it will fail, since it's still open. And if I'm following the logic right, a critical messagebox will pop up displaying the message: "Wallet file verification failed: Error loading wallet [blah].dat. Duplicate -wallet filename specified. (code -4)"

This is certainly a little weird but doesn't seem terrible.

(But now I see Sjors's comment, and it's an interesting point that a power user may want to touch the wallet file, and may assume that when it disappears from the list in the GUI, that means it's safe to move it, which would be disastrous if it's not unloaded. If this really is an issue, I don't see any way around this other than putting up a modal "unloading..." dialog, ultimately.)

@promag

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

@Sjors right, I thought the same. In order to not block UI with UnloadWallet I have to create a CloseWalletActivity, so IMO definitely something to improve later - I mean, can be improved even after feature freeze (I think?).

@promag

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

If the question is what do I think should happen when a wallet is slow to unload, it's the same as what I wrote in #15153 (comment) about what should I think should happen when a wallet is slow to load. Wallet display should show "Unloading wallet X..." while the wallet is unloading, and when it's done unloading, the wallet should be removed from the dropdown list, and if it was still selected, the next item in the list should be selected instead.

While the wallet is unloading, the user should be able to switch to other wallets, load and unload other wallets, shut down the program, and do everything else normally except interact with the wallet that's being unloaded. There shouldn't be any popups or modal unloading dialogs or anything like that.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

I agree with @ryanofsky. As a long term goal. Unsure if this can be broken down to simpler steps. A wallet can – currently –  consume a decent amount of memory (and eventually CPU) and closing a wallet should IMO unload the wallet (graceful).

I haven't looked at the code yet,... is there a reason to not unload the wallet synchronously as a first step (blocking the GUI as an initial close seems acceptable)?

@promag

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

@jonasschnelli IMO currently it's not so easily done, it's not the same as the RPC unloadwallet where there is only a std::shared_ptr<CWallet>. Here we have interfaces::Wallet which has a member CWallet&.

void WalletImpl::unload() override
{
    RemoveWallet(m_shared_wallet);
    UnloadWallet(std::move(m_shred_wallet));
    // now m_wallet is invalid, and this interface is also "null"
    // also, this interface would be deleted by the unload notification (?)
}

@ryanofsky what do you suggest to fix the above? I thought not calling UnloadWallet and then wait for the unload notification..

Anyway, I think this is ready to merge and then I can follow up with improvements.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Tested ACK 94086fb
This is a good first step.

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

@jonasschnelli jonasschnelli merged commit 94086fb into bitcoin:master Feb 14, 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 14, 2019

Merge #15195: gui: Add Close Wallet action
94086fb gui: Add close wallet action (João Barbosa)
f77ba34 gui: Add closeWallet to WalletController (João Barbosa)
f6122ab interfaces: Add remove to Wallet (João Barbosa)

Pull request description:

  This PR adds support to close the current wallet in the GUI.

  <img width="543" alt="screenshot 2019-01-18 at 00 44 26" src="https://user-images.githubusercontent.com/3534524/51358241-424b9680-1aba-11e9-88f2-b85869507737.png">
  <img width="532" alt="screenshot 2019-01-18 at 00 44 38" src="https://user-images.githubusercontent.com/3534524/51358242-424b9680-1aba-11e9-83e2-fa275a9017b3.png">

Tree-SHA512: fd7da4d0f73dc240864cc57a1ff1526daf2376904ce3872e52eeca5d40cc21c6dd29eb2ef25f85ffa63697362c150221a2369d80ad36ae445cc99989d337b688
@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

@ryanofsky what do you suggest to fix the above?

What you suggested looks good, but WalletImpl class should just have a single std::shared_ptr<CWallet> m_wallet member, not two wallet members.

@promag promag deleted the promag:2019-01-closewallet branch Feb 17, 2019

@fanquake fanquake moved this from In progress to Done in Multiwallet support Feb 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.