This PR adds the ability to open external wallets on the GUI.
<img width="440" alt="Screenshot 2019-09-08 at 18 14 36" src="https://user-images.githubusercontent.com/3534524/64491830-a34ff680-d264-11e9-9b91-180909564d18.png"> <img width="469" alt="Screenshot 2019-09-08 at 18 13 17" src="https://user-images.githubusercontent.com/3534524/64491831-a34ff680-d264-11e9-8d37-b25a30cb9180.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, 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.
Updated.
tACK b537d29, but note that you can only open directory based wallets
Is this for 0.18?
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).
Needs rebase
Sure, in a couple of hours.
You mean the open menu with:
Thanks, also updated OP.
Concept ACK
Concept ACK, will review this weekend.
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?
ok that's going to have to be communicated in some obvious way...
what does directory based wallets even mean i.e., how is this detected?
what does
directory based walletseven mean i.e., how is this detected?
The underlying wallet code can only open:
wallet.dat that are located directly in the top level wallet_dirIt 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.
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:
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.
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?
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:
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?
@instagibbs mind testing again? Now you can select the wallet.dat or the parent directory. @ryanofsky now it doesn't create the wallet, thanks!
On macOS, when I select a directory ~/temp/A it complains:
<img width="427" alt="Schermafbeelding 2019-09-16 om 10 45 08" src="https://user-images.githubusercontent.com/10217/64944730-4a3d1f80-d86f-11e9-9d7e-0a1c007d5273.png">
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.
This has missed the deadline in #15940
I don't think the wallet name should display the path to that wallet:
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.
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
loadwalletwith a path. Let's move that discussion elsewhere? @Sjors I think I've fixed it, mind checking?
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.
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.
ACK on punting the full path issue until a followup
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.
I'm tempted to allow selecting either wallet.dat or a directory.
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.
I'd use a folder selector
concept ACK on this suggestion. I don't think I would have been overly confused with this originally.
Done, now can only select folders.
ok, this is much more natural. quick tACK https://github.com/bitcoin/bitcoin/pull/15204/commits/4e8cb3fd5efe2d108d8cf4017d197fb3e7c8eb40
Agree the behavior in 4e8cb3f is nicer. I haven't reviewed the code.
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"));
Suggestions: s/on/in an/... or "Open an externally-located wallet"... or "Open a wallet in an external directory", etc.
Picked "Open a wallet in an external directory".
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));
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"?
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());
s/External/external/ as per the other changes with lowercased "external"
Changed to "Open Wallet", I think it's enough.
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()));
toHtmlEscaped(): I manually verified that the wallet name is properly escaped :heavy_check_mark:
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;
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.
Can't reproduce, please let me know if you hit it again.
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.
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?
How about "wallet in external directory" and variations thereof
How about "wallet in external directory" and variations thereof
I like the "Open a wallet in an external directory" Jon suggested
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.
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.
Maybe "Open a wallet located in a different directory"
Will test more and re-review. Some edge case test ideas from today's PR review meeting:
Will test more and re-review. Some edge case test ideas from today's PR review meeting:
- no wallet in directory
it already fails:
<img width="532" alt="Screenshot 2019-09-18 at 22 50 05" src="https://user-images.githubusercontent.com/3534524/65188604-b3d34e80-da66-11e9-8c64-3188da5a7a47.png">
- multiple wallets in directory
not supported - it's already the case with RPC loadwallet:
bitcoin-cli -regtest loadwallet /Users/joao/Desktop/test.dat
Wallet 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.
re-Concept ACK. Here's the dialog on macOS:
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:
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).
libc++abi.dylib: terminating with uncaught exception of type std::runtime_error: BerkeleyBatch: Can't open database wallet.dat (duplicates fileid ccd6a006040000010ccf2f5cc535010000000000 from wallet.dat)
2019-09-19T03:02:31Z GUI: Qt has caught an exception thrown from an event handler. Throwing
exceptions from an event handler is not supported in Qt.
You must not let any exception whatsoever propagate through Qt code.
If that is not possible, in Qt 5 you must at least reimplement
QCoreApplication::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:
<img width="972" alt="path_confusion" src="https://user-images.githubusercontent.com/863730/65215478-3e0bc900-dae0-11e9-955a-a0ed38a13877.png">
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.
@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.
Same GUI hang issue when opening a wallet dir moved to ~/ and wallet.dat file inside touched to be empty:
In both cases, quitting the GUI unhangs the wallet but the wallet hangs until I quit for at least several minutes.
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):
$ qt/bitcoin-qt -testnet
terminate called after throwing an instance of 'std::runtime_error'
what(): BerkeleyBatch: Can't open database wallet.dat (duplicates fileid c4003e0100fe0000fd9fb0c2a56b000000000000 from wallet.dat)
Aborted
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:
(FWIW a GUI wallet dark mode would be great).
@jonatack regarding the "duplicates fileid " crash see #16776 (comment)
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").
select a wallet folder
I like that.
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.
<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase
Needs addressing comments or should otherwise be closed.
Will rebase and open in gui repo.
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.
Milestone
0.21.0