gui: Add Open External Wallet action

promag commented at 4:03 pm on January 18, 2019: member

This PR adds the ability to open external wallets on the GUI.

laanwj added the label Wallet on Jan 18, 2019

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

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

DrahtBot commented at 10:38 pm on January 18, 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:

#16963 (wallet: Fix unique_ptr usage in boost::signals2 by promag)
#16432 (qt: Add privacy to the Overview page by hebasto)

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.

Sjors commented at 3:12 pm on January 19, 2019: member

Concept ACK, but let’s put this in the Open Wallet sub menu. You could put a separator underneath the list of known wallets and then call the menu item “Open file…”. I don’t like the word “External” because it could get confusing if we add (e.g.) hardware wallet support.

promag commented at 4:05 pm on January 19, 2019: member

@Sjors yes that was already brought up by @jnewbery and in IRC I suggested “File -> Open Wallet -> Other…” On the end of the menu.

promag commented at 5:36 pm on January 19, 2019: member

@Sjors how about:

jnewbery commented at 5:49 pm on January 19, 2019: member

@Sjors how about:

Yes, this looks ideal to me

promag force-pushed on Jan 19, 2019

promag commented at 9:10 pm on January 19, 2019: member

Updated.

meshcollider added the label GUI on Feb 10, 2019

DrahtBot added the label Needs rebase on Feb 12, 2019

promag force-pushed on Feb 12, 2019

promag commented at 11:09 pm on February 12, 2019: member

Updated, still rebased on #15195.

DrahtBot removed the label Needs rebase on Feb 12, 2019

Sjors commented at 9:50 am on February 13, 2019: member

tACK b537d29, but note that you can only open directory based wallets

promag commented at 10:17 am on February 13, 2019: member

@Sjors right, and it looks like a pain to try to open both with Qt 😕

promag commented at 8:55 pm on February 14, 2019: member

Is this for 0.18?

jonasschnelli added this to the milestone 0.19.0 on Feb 14, 2019

Sjors commented at 2:55 pm on February 15, 2019: member

Needs rebase because Open Wallet got merged. That shouldn’t impact my earlier tACK, ~so we might be able to add this to 0.18 today~ (update: too late).

promag force-pushed on Feb 15, 2019

DrahtBot added the label Needs rebase on Feb 27, 2019

promag force-pushed on Mar 17, 2019

DrahtBot removed the label Needs rebase on Mar 17, 2019

DrahtBot added the label Needs rebase on Mar 22, 2019

promag force-pushed on Mar 24, 2019

DrahtBot removed the label Needs rebase on Mar 24, 2019

jonasschnelli commented at 11:08 am on May 18, 2019: contributor

Needs rebase

DrahtBot added the label Needs rebase on May 18, 2019

fanquake commented at 9:00 am on September 8, 2019: member

@promag Given that this is tagged for 0.19.0, can you rebase?

fanquake added the label Waiting for author on Sep 8, 2019

promag commented at 10:25 am on September 8, 2019: member

Sure, in a couple of hours.

promag force-pushed on Sep 8, 2019

DrahtBot removed the label Needs rebase on Sep 8, 2019

fanquake removed the label Waiting for author on Sep 8, 2019

fanquake commented at 12:52 pm on September 8, 2019: member

@promag I prefer the previous version, with the action in the wallet menu. Will let others weigh in.

promag commented at 1:18 pm on September 8, 2019: member

You mean the open menu with:

no wallets available
separator
other…

fanquake commented at 1:25 pm on September 8, 2019: member

The one currently in the PR description and in this comment.

jnewbery commented at 2:34 pm on September 8, 2019: member

I agree with @fanquake that keeping it in the ‘Open Wallet’ submenu makes more sense.

promag force-pushed on Sep 8, 2019

promag commented at 5:17 pm on September 8, 2019: member

Thanks, also updated OP.

meshcollider commented at 11:36 pm on September 9, 2019: contributor

Concept ACK

jonatack commented at 7:46 pm on September 13, 2019: member

Concept ACK, will review this weekend.

instagibbs commented at 8:09 pm on September 13, 2019: member

got this message on stderr on qt startup with this PR: Gtk-Message: 16:07:25.804: GtkDialog mapped without a transient parent. This is discouraged.

I tried opening up a random wallet filed I had, but it was greyed out in the file selection? Am I missing something?

promag commented at 8:20 pm on September 13, 2019: member

@instaggibbs see #15204 (comment)

instagibbs commented at 8:28 pm on September 13, 2019: member

ok that’s going to have to be communicated in some obvious way…

instagibbs commented at 8:29 pm on September 13, 2019: member

what does directory based wallets even mean i.e., how is this detected?

ryanofsky commented at 10:35 pm on September 13, 2019: member

what does directory based wallets even mean i.e., how is this detected?

The underlying wallet code can only open:

Wallet files named wallet.dat
Wallet files with names other than wallet.dat that are located directly in the top level wallet_dir

It hasn’t been possible since #11687 to even create wallet files named anything other than wallet.dat so the second option just exists for backwards compatibility.

Only creating wallet files named wallet.dat and referring to wallets by directory rather than file name was done to address concerns from luke and others #11687 (comment) that if users only backed up individual wallet files instead of whole wallet directories they could lose data.

ryanofsky commented at 10:57 pm on September 13, 2019: member

Haven’t tested this but looking at the implementation it seems like this might be lacking the error checking from the loadwallet RPC that ensures that the wallet which the user is trying to open actually exists, and that CreateWalletFromFIle code will not go and create at new empty wallet at the specified location:

https://github.com/bitcoin/bitcoin/blob/fb4f5beb6ede4aadeaff779cd67a0f6665419488/src/wallet/rpcwallet.cpp#L2577-L2585

Could be a pretty scary user experience to try to load an existing wallet but choose the wrong path, and see a new completely empty wallet instead of a “location does not contain wallet.dat” error.

If adding the check is necessary, I’d suggest moving the code out of the loadwallet RPC to a common location instead of adding duplicate code for the gui.

promag commented at 9:43 am on September 15, 2019: member

Could be a pretty scary user experience to try to load an existing wallet but choose the wrong path, and see a new completely empty wallet instead of a “location does not contain wallet.dat” error. @ryanofsky indeed, currently it’s like “load or create” :trollface: will change accordingly.

ok that’s going to have to be communicated in some obvious way… @instagibbs you mean in the release notes or do you have something else in mind?

promag force-pushed on Sep 15, 2019

instagibbs commented at 7:20 pm on September 15, 2019: member

oh…. you have to click “open” while in the right directory or after clicking once on that directory holding the wallet file?

that’s not even close to intuitive :grimacing:

instagibbs commented at 7:21 pm on September 15, 2019: member

So this either needs an extensive explanation before the user ventures to use it, or it should allow the user to click and open a wallet.dat in a directory, in addition to the directory itself. Latter seems to make most sense?

promag force-pushed on Sep 15, 2019

promag commented at 10:07 pm on September 15, 2019: member

@instagibbs mind testing again? Now you can select the wallet.dat or the parent directory. @ryanofsky now it doesn’t create the wallet, thanks!

Sjors commented at 8:48 am on September 16, 2019: member

On macOS, when I select a directory ~/temp/A it complains:

When I double click on the directory A (so it shows the wallet.dat file, etc) it still gives the above error.

It only works when I select the wallet.dat file. But then it loses the wallet name and shows the full path instead in the wallet dropdown menu.

MarcoFalke commented at 1:53 pm on September 16, 2019: member

This has missed the deadline in #15940

MarcoFalke removed this from the milestone 0.19.0 on Sep 16, 2019

jnewbery commented at 3:54 pm on September 16, 2019: member

I don’t think the wallet name should display the path to that wallet:

Screenshot from 2019-09-16 11-54-07

ryanofsky commented at 4:42 pm on September 16, 2019: member

So this either needs an extensive explanation before the user ventures to use it, or it should allow the user to click and open a wallet.dat in a directory, in addition to the directory itself. Latter seems to make most sense?

Allowing opening of wallet.dat files seems good. Would just need to strip the last wallet.dat component to make the path a wallet location. Another option would be to use a directory selector instead of file selector. https://stackoverflow.com/questions/9618381/how-to-specify-the-qfiledialoggetexistingdirectory-method

I don’t think the wallet name should display the path to that wallet:

I kind of do, but anything in particular you’d like to replace it with? It should be ok to replace it with something something shorter in the GUI selector. I’d be more wary of changing the output of getwalletinfo RPC, or changing the invariant that whatever string you pass to -wallet or loadwallet or createwallet will uniquely identify the wallet while it is open, and be the wallet endpoint and -rpcwallet argument you pass to bitcoin-cli.

promag commented at 9:26 pm on September 16, 2019: member

Would just need to strip the last wallet.dat component to make the path a wallet location

That’s done here: https://github.com/bitcoin/bitcoin/pull/15204/commits/9e1fdd7f94161ecfbe7a4247fb2a672d5190ff83#diff-dc8c2957962a3032071c55c70e120932R331 but apparently shouldn’t be done for directories.

I don’t think the wallet name should display the path to that wallet: @jnewbery this wasn’t changed here, it’s already the case if you use loadwallet with a path. Let’s move that discussion elsewhere? @Sjors I think I’ve fixed it, mind checking?

promag commented at 9:38 pm on September 16, 2019: member

BTW I’m avoiding QFileDialog static methods, such as QFileDialog::getOpenFileName, which have the following drawback (from the doc):

Warning: Do not delete parent during the execution of the dialog. If you want to do this, you should create the dialog yourself using one of the QFileDialog constructors.

#15310 is related to this warning.

Sjors commented at 1:01 pm on September 17, 2019: member

In 91d6e0b I’m able to to open a wallet by just selecting the directory. Opening the directory and opening the wallet.dat file work as well.

Agree with @jnewbery about using the wallet name instead of the full path. Since the RPC has the same problem, we can deal with that in a followup.

instagibbs commented at 1:13 pm on September 17, 2019: member

ACK on punting the full path issue until a followup

promag force-pushed on Sep 17, 2019

instagibbs commented at 2:16 pm on September 17, 2019: member

Now when I use it, I can only open the wallet by selecting a file within a directory with a wallet.dat present. It lets me “open” any file in that directory, but it opens up the specific wallet.dat that is in the same directory, I think.

promag commented at 2:20 pm on September 17, 2019: member

I’m tempted to allow selecting either wallet.dat or a directory.

ryanofsky commented at 2:35 pm on September 17, 2019: member

I’m tempted to allow selecting either wallet.dat or a directory.

If I were implementing this, I’d use a folder selector (https://stackoverflow.com/questions/13299283/folder-browser-dialog-in-qt ), not a file selector, and only allow choosing directories containing wallet.dat files (disable the choose / select button for other directories).

But if there’s a reason to list files as well as directories, I’d think there should be a way to filter the file list to only show wallet.dat files.

instagibbs commented at 2:41 pm on September 17, 2019: member

I’d use a folder selector

concept ACK on this suggestion. I don’t think I would have been overly confused with this originally.

promag force-pushed on Sep 17, 2019

promag commented at 3:45 pm on September 17, 2019: member

Done, now can only select folders.

instagibbs commented at 3:54 pm on September 17, 2019: member

ok, this is much more natural. quick tACK https://github.com/bitcoin/bitcoin/pull/15204/commits/4e8cb3fd5efe2d108d8cf4017d197fb3e7c8eb40

Sjors commented at 5:08 pm on September 17, 2019: member

Agree the behavior in 4e8cb3f is nicer. I haven’t reviewed the code.

in src/qt/bitcoingui.cpp:341 in 4e8cb3fd5e outdated

336@@ -337,6 +337,9 @@ void BitcoinGUI::createActions()
337     m_open_wallet_action->setStatusTip(tr("Open a wallet"));
338     m_open_wallet_menu = new QMenu(this);
339 
340+    m_open_external_wallet_action = new QAction(tr("Other..."), this);
341+    m_open_external_wallet_action->setStatusTip(tr("Open a wallet on external location"));

jonatack commented at 3:41 pm on September 18, 2019:

Suggestions: s/on/in an/… or “Open an externally-located wallet”… or “Open a wallet in an external directory”, etc.

promag commented at 9:16 pm on September 18, 2019:

Picked “Open a wallet in an external directory”.

in src/qt/walletcontroller.cpp:312 in 4e8cb3fd5e outdated

308+    m_progress_dialog->hide();
309+
310+    if (!m_exists) {
311+        QMessageBox::critical(m_parent_widget, tr("Open external wallet failed"), QString::fromStdString(m_error_message));
312+    } else if (!m_error_message.empty()) {
313+        QMessageBox::critical(m_parent_widget, tr("Open external wallet failed"), QString::fromStdString(m_error_message));

jonatack commented at 3:46 pm on September 18, 2019:

Combine the first two conditionals to one since they return the same result? Or, should the first result return a different message along the lines of “No wallet file found”?

in src/qt/walletcontroller.cpp:325 in 4e8cb3fd5e outdated

321+}
322+
323+void OpenExternalWalletActivity::open()
324+{
325+    Q_ASSERT(!m_file_dialog);
326+    m_file_dialog = new QFileDialog(m_parent_widget, tr("Open External wallet"), QDir::homePath());

jonatack commented at 3:47 pm on September 18, 2019:

s/External/external/ as per the other changes with lowercased “external”

promag commented at 9:17 pm on September 18, 2019:

Changed to “Open Wallet”, I think it’s enough.

in src/qt/walletcontroller.cpp:332 in 4e8cb3fd5e outdated

328+    m_file_dialog->setOption(QFileDialog::ShowDirsOnly);
329+
330+    connect(m_file_dialog, &QFileDialog::fileSelected, [this](const QString& path) {
331+        if (path.isEmpty()) return;
332+
333+        showProgressDialog(tr("Opening external wallet <b>%1</b>...").arg(path.toHtmlEscaped()));

jonatack commented at 3:49 pm on September 18, 2019:

toHtmlEscaped(): I manually verified that the wallet name is properly escaped :heavy_check_mark:

in src/wallet/wallet.cpp:172 in 4e8cb3fd5e outdated

171+        // The given filename is a directory. Check that there's a wallet.dat file.
172+        fs::path wallet_dat_file = location.GetPath() / "wallet.dat";
173+        if (fs::symlink_status(wallet_dat_file).type() == fs::file_not_found) {
174+            exists = false;
175+            error = "Directory " + location.GetName() + " does not contain a wallet.dat file.";
176+            return nullptr;

jonatack commented at 3:51 pm on September 18, 2019:

While manually testing selecting a directory with no wallet file, after clicking OK at this dialog, I seemed to encounter a strange behavior where the GUI appeared to be trying to load a wallet. Will test further to re-reproduce/confirm.

promag commented at 9:28 pm on September 18, 2019:

Can’t reproduce, please let me know if you hit it again.

jonatack commented at 4:02 pm on September 18, 2019: member

Light code review and quick manual test ACK 4e8cb3fd5efe2d108d8cf4017d197fb3e7c8eb40 apart from my comments below and a bit more manual testing – I may have encountered weird behavior as described in my comments below. I agree with @jnewbery on the full path behavior and understand @ryanofsky’s points, and don’t have a proposal yet that is more concrete than having a way to see the full path that is discreet and user-friendly. Punting for a follow-up duly noted.

jonatack commented at 4:10 pm on September 18, 2019: member

I don’t like the word “External” because it could get confusing if we add (e.g.) hardware wallet support.

I’m hesitant as well with the ambiguity of the word “external” – should we be more precise?

instagibbs commented at 4:15 pm on September 18, 2019: member

How about “wallet in external directory” and variations thereof

ryanofsky commented at 4:23 pm on September 18, 2019: member

How about “wallet in external directory” and variations thereof

I like the “Open a wallet in an external directory” Jon suggested

michaelfolkson commented at 4:37 pm on September 18, 2019: contributor

The multiple wallets should be in separate directories by default? In which case why not just “Other wallet” or “Additional wallet”? I don’t think attention should be brought to it being in an external directory if that is the default.

ryanofsky commented at 4:52 pm on September 18, 2019: member

The multiple wallets should be in separate directories by default? In which case why not just “Other wallet” or “Additional wallet”? I don’t think attention should be brought to it being in an external directory if that is the default.

External means outside the bitcoin wallet dir (~/.bitcoin/wallets on linux) where the list of wallets comes from. I like your “Open other wallet…” suggestion, though. Maybe saying “external” just adds confusion.

jonatack commented at 4:57 pm on September 18, 2019: member

Maybe “Open a wallet located in a different directory”

promag force-pushed on Sep 18, 2019

jonatack commented at 9:43 pm on September 18, 2019: member

Will test more and re-review. Some edge case test ideas from today’s PR review meeting:

no wallet in directory
multiple wallets in directory
a non-wallet file named wallet.dat
deleting the wallet file while trying to load it

promag commented at 9:57 pm on September 18, 2019: member

Will test more and re-review. Some edge case test ideas from today’s PR review meeting:

no wallet in directory

it already fails:

multiple wallets in directory

not supported - it’s already the case with RPC loadwallet:

0bitcoin-cli -regtest loadwallet /Users/joao/Desktop/test.dat
1
2Wallet file verification failed: Invalid -wallet path '/Users/joao/Desktop/test.dat'. -wallet path should point to a directory where wallet.dat and database/log.?????????? files can be stored, a location where such a directory could be created, or (for backwards compatibility) the name of an existing data file in -walletdir ("/Users/joao/Library/Application Support/Bitcoin/regtest/wallets") (code -4)

a non-wallet file named wallet.dat

not supported ☝️

deleting the wallet file while trying to load it

Didn’t test this here, and not sure how to reliable test it.

fanquake added this to the milestone 0.20.0 on Sep 19, 2019

fanquake commented at 5:24 am on September 19, 2019: member

re-Concept ACK. Here’s the dialog on macOS:

open_dialog

A few thoughts:

It’d be good if the GUI realised when we opened a wallet that’s actually inside our wallet directory via the Other... menu, otherwise you end up with the wallet loaded, but not greyed out: open

This seems to have created at least one path to a segfault, where if you load duplicate (copy of a) wallets we don’t catch the BerkeleyBatch exception. I don’t think it was possible to do this via the GUI before (it’s also currently caught when loading wallets via the RPC etc).

0libc++abi.dylib: terminating with uncaught exception of type std::runtime_error: BerkeleyBatch: Can't open database wallet.dat (duplicates fileid ccd6a006040000010ccf2f5cc535010000000000 from wallet.dat)
1
22019-09-19T03:02:31Z GUI: Qt has caught an exception thrown from an event handler. Throwing
3exceptions from an event handler is not supported in Qt.
4You must not let any exception whatsoever propagate through Qt code.
5If that is not possible, in Qt 5 you must at least reimplement
6QCoreApplication::notify() and catch all exceptions there.

Maybe saying “external” just adds confusion.

Agree.

Noticed at least one quirk where Qt fails to display file paths correctly:

promag commented at 7:06 am on September 19, 2019: member

It’d be good if the GUI realised when we opened a wallet that’s actually inside our wallet directory via the Other... menu, otherwise you end up with the wallet loaded, but not greyed out:

Yes, different PR I guess.

jonatack commented at 10:36 am on September 19, 2019: member

@promag Thanks for the update. While testing f60f80e599284b4f098b54766e2221ebcd11f587, I reproduced the issue mentioned above on Debian 4.19.37-5+deb10u2 (2019-08-08) x86_64 GNU/Linux. Opening a wallet-less folder causes the GUI to display the error (all good) but then a second later the GUI attempts to open the wallet anyway and hangs in this state. Reproduced several times with different locations and directories.

Screenshot from 2019-09-19 13-11-45

Same GUI hang issue when opening a wallet dir moved to ~/ and wallet.dat file inside touched to be empty:

Screenshot from 2019-09-19 12-55-58

In both cases, quitting the GUI unhangs the wallet but the wallet hangs until I quit for at least several minutes.

jonatack commented at 10:51 am on September 19, 2019: member

Copying a wallet dir elsewhere with a different name but same wallet files inside, and attempting to load it in the GUI (EDIT: this is a separate issue that has been reported and a PR has been proposed):

0$ qt/bitcoin-qt -testnet
1terminate called after throwing an instance of 'std::runtime_error'
2  what():  BerkeleyBatch: Can't open database wallet.dat (duplicates fileid c4003e0100fe0000fd9fb0c2a56b000000000000 from wallet.dat)
3Aborted

Screenshot of a wallet successfully loaded after I moved it from ~/.bitcoin to ~/ with an ampersand (&) displayed correctly in the title bar and the open wallet list:

Screenshot from 2019-09-19 12-41-10

(FWIW a GUI wallet dark mode would be great).

promag commented at 1:07 pm on September 19, 2019: member

@jonatack regarding the “duplicates fileid " crash see #16776 (comment)

promag commented at 9:14 pm on September 27, 2019: member

@jonatack just to be sure, if you repeat the above tests with loadwallet in the console you would get the same problems?

jonasschnelli commented at 10:03 am on October 9, 2019: contributor

Tested a bit. What I think is hard to understand is that one needs to select a folder rather than a wallet.dat file.

I think either we allow to select wallet.dat files or inform the user that only wallet-directories are allowed (“select a wallet folder”).

promag commented at 10:24 am on October 9, 2019: member

select a wallet folder

I like that.

DrahtBot added the label Needs rebase on Oct 21, 2019

MarkLTZ referenced this in commit ec72a3b055 on Nov 17, 2019

promag force-pushed on Dec 15, 2019

DrahtBot removed the label Needs rebase on Dec 15, 2019

refactor: Add LoadExistingWallet 59ac7a5b2a

gui: Add OpenExternalWalletActivity 580c6ab87e

gui: Add Open External Wallet action 60b3947b03

promag force-pushed on Dec 15, 2019

Sjors commented at 12:14 pm on December 16, 2019: member

If you make OpenExternalWalletActivity a subclass of OpenWalletActivity it would save duplicate code, which is especially nice in Open[External]WalletActivity::open() with its QTimer::singleShot.

DrahtBot added the label Needs rebase on Jan 8, 2020

DrahtBot commented at 4:22 pm on January 8, 2020: member

laanwj removed this from the milestone 0.20.0 on Mar 26, 2020

laanwj added this to the milestone 0.21.0 on Mar 26, 2020

laanwj added the label Feature on Mar 26, 2020

jonasschnelli commented at 7:23 am on May 29, 2020: contributor

Needs addressing comments or should otherwise be closed.

promag commented at 0:06 am on August 17, 2020: member

Will rebase and open in gui repo.

promag closed this on Aug 17, 2020

promag deleted the branch on Aug 17, 2020

promag commented at 11:52 pm on August 30, 2020: member

Tested a bit. What I think is hard to understand is that one needs to select a folder rather than a wallet.dat file.

I think either we allow to select wallet.dat files or inform the user that only wallet-directories are allowed (“select a wallet folder”).

I’ve tried to support selecting files and folders but this kind of sucks because in order be intuitive it needs non native file dialog - it allows to set a proxy model.

Anyway, I think the best approach, and considering sqlite wallets, selecting wallet.dat is indeed the best option for the moment.

DrahtBot locked this on Feb 15, 2022

gui: Add Open External Wallet action #15204

Conflicts