gui: Add dynamic wallets support

promag commented at 11:01 pm on April 26, 2018: member

This PR adds a menu entry Open Wallet to support loading existing wallets in the UI.

fanquake added the label GUI on Apr 27, 2018

ken2812221 commented at 1:33 am on April 27, 2018: contributor

~~If the wallet folder name contains Chinese, it will create a new folder that has same wallet file with a weird name on Windows, maybe the encoding issue~~ (Not related)

Also, the seperator is incorrect on Windows

promag force-pushed on Apr 27, 2018

promag commented at 8:36 am on April 27, 2018: member

@ken2812221 does the problem exists if you use the RPC command?

ken2812221 commented at 8:49 am on April 27, 2018: contributor

~~It shows wallet not found on RPC.~~ (Not related)

promag commented at 8:55 am on April 27, 2018: member

@ken2812221 but it does exists right? What if you launch the application with -wallet=d:\...?

ken2812221 commented at 9:14 am on April 27, 2018: contributor

-wallet=d:\錢包 does works, but the name displayed in Qt is incorrect

And it crash when I quit Qt

Checked that this isn’t an issue on Linux.

ryanofsky commented at 12:47 pm on April 27, 2018: member

Maybe windows or boost is using big5 encoding. which ends in a right square bracket ]

0>>> '錢包'.encode('big5')
1b'\xbf\xfa\xa5]'
2>>> '錢包'.encode('big5').decode('utf8', 'replace')
3'���]'

promag commented at 2:08 pm on April 27, 2018: member

@ken2812221 thanks for the review and bug report. Let’s move the bug discussion to #13103.

in src/qt/bitcoingui.cpp:750 in f61aff5796 outdated

706@@ -701,6 +707,20 @@ void BitcoinGUI::openClicked()
707     }
708 }
709 
710+void BitcoinGUI::openWalletClicked()
711+{
712+    QString fileName = QFileDialog::getExistingDirectory(this, tr("Open Wallet"),

ken2812221 commented at 10:56 am on April 29, 2018:

Use QDir::toNativeSeparators to convert / to \ on Windows

promag commented at 4:33 pm on May 2, 2018:

Done.

unknown commented at 5:01 am on June 12, 2018:

^^

in src/interfaces/node.cpp:238 in f61aff5796 outdated

233+    }
234+    std::pair<std::unique_ptr<Wallet>, std::string> loadWallet(const std::string& wallet_file) override
235+    {
236+#ifdef ENABLE_WALLET
237+        try {
238+            std::string dummy_warning;

jnewbery commented at 6:12 pm on April 30, 2018:

This seems almost exactly duplicated from loadwallet() in rpcwallet.cpp. Does it make sense to factor it out to avoid repetition?

promag commented at 6:20 pm on April 30, 2018:

Yeah, I would like to move to WalletManager

promag commented at 4:32 pm on May 2, 2018:

@jnewbery done in #9888dc2.

in src/interfaces/node.cpp:254 in f61aff5796 outdated

249+            AddWallet(wallet);
250+
251+            wallet->postInitProcess();
252+            return { MakeWallet(*wallet), std::string() };
253+        } catch (...) {
254+            return { std::unique_ptr<Wallet>(), "ops" };

jnewbery commented at 6:14 pm on April 30, 2018:

I don’t understand why you’re passing back an error string if it’s always the string ops

promag commented at 6:21 pm on April 30, 2018:

Ops, it’s unfinished. Not sure if it should return the pair.

promag commented at 4:32 pm on May 2, 2018:

Removed, changed the return type.

in src/qt/bitcoingui.cpp:717 in f61aff5796 outdated

712+    QString fileName = QFileDialog::getExistingDirectory(this, tr("Open Wallet"),
713+                                                QString(),
714+                                                QFileDialog::ShowDirsOnly
715+                                                | QFileDialog::DontResolveSymlinks);
716+    if (fileName.isEmpty()) return;
717+    const auto& result = m_node.loadWallet(fileName.toStdString());

jnewbery commented at 6:15 pm on April 30, 2018:

The use of auto here makes it difficult to understand the result.second below.

promag commented at 4:33 pm on May 2, 2018:

No an issue now, return value is not necessary.

meshcollider commented at 9:12 am on May 2, 2018: contributor

Concept ACK

promag force-pushed on May 2, 2018

in src/qt/bitcoingui.cpp:720 in 462bf7fd69 outdated

715+                                                QFileDialog::ShowDirsOnly
716+                                                | QFileDialog::DontResolveSymlinks);
717+    if (fileName.isEmpty()) return;
718+
719+    std::vector<std::string> errors;
720+    m_node.loadWallet(QDir::toNativeSeparators(fileName).toStdString(), errors);

promag commented at 9:37 am on May 3, 2018:

@jnewbery @laanwj ATM the wallet is loaded in the UI thread, should I refactor to load it in the background?

jnewbery commented at 8:04 pm on May 3, 2018:

I think that’s a good future enhancement. Not necessary in the initial version IMO.

jnewbery commented at 8:03 pm on May 3, 2018: member

Please update description to say that this fixes #7675

jnewbery added this to the "In progress" column in a project

promag force-pushed on May 4, 2018

promag commented at 10:06 pm on May 4, 2018: member

Please update description to say that this fixes #7675

Done. Also rebased.

promag force-pushed on May 23, 2018

promag commented at 11:57 am on May 23, 2018: member

Rebased on master.

jnewbery commented at 1:29 pm on May 23, 2018: member

wallet_multiwallet.py fails

promag force-pushed on May 23, 2018

MarcoFalke renamed this:
~~Add menu entry to open wallet~~
gui: Add menu entry to open wallet
on May 23, 2018

promag force-pushed on May 24, 2018

promag commented at 1:21 pm on May 24, 2018: member

Rebased.

in src/qt/bitcoingui.cpp:751 in a27b30050f outdated

713@@ -707,6 +714,24 @@ void BitcoinGUI::openClicked()
714     }
715 }
716 
717+void BitcoinGUI::openWalletClicked()
718+{
719+    QString fileName = QFileDialog::getExistingDirectory(this, tr("Open Wallet"),
720+                                                QString(),

jnewbery commented at 2:51 pm on May 24, 2018:

Suggestion: start in the data directory for the current network (mainnet/testnet/regtest) instead of the home directory, since that’s where the wallet files and folders will usually be (and because it may be difficult to traverse down into .bitcoin).

in src/qt/bitcoingui.cpp:721 in a27b30050f outdated

713@@ -707,6 +714,24 @@ void BitcoinGUI::openClicked()
714     }
715 }
716 
717+void BitcoinGUI::openWalletClicked()
718+{
719+    QString fileName = QFileDialog::getExistingDirectory(this, tr("Open Wallet"),
720+                                                QString(),
721+                                                QFileDialog::ShowDirsOnly

jnewbery commented at 2:51 pm on May 24, 2018:

Why only show directories and not symlinks? Wallets can be .dat files.

promag commented at 3:48 pm on May 24, 2018:

Right.

unknown approved

promag force-pushed on Jun 23, 2018

DrahtBot added the label Needs rebase on Jun 25, 2018

promag renamed this:
~~gui: Add menu entry to open wallet~~
gui: Add dynamic wallets support
on Jul 1, 2018

jnewbery commented at 6:48 pm on July 9, 2018: member

@promag - what’s the status of this? It’d be great to have this in v0.17, but feature freeze is next week.

promag force-pushed on Jul 10, 2018

promag commented at 2:20 am on July 10, 2018: member

@jnewbery agree, please test, I’ll address your comments as soon as I can.

promag force-pushed on Jul 10, 2018

DrahtBot removed the label Needs rebase on Jul 10, 2018

in src/qt/bitcoingui.cpp:327 in 6741c10ce4 outdated

323@@ -322,6 +324,15 @@ void BitcoinGUI::createActions()
324     openAction = new QAction(platformStyle->TextColorIcon(":/icons/open"), tr("Open &URI..."), this);
325     openAction->setStatusTip(tr("Open a bitcoin: URI or payment request"));
326 
327+    m_new_wallet_action = new QAction(platformStyle->TextColorIcon(":/icons/open"), tr("New &Wallet..."), this);

Sjors commented at 1:11 pm on July 10, 2018:

IIUC the & sets the keyboard shortcut, so these should be unique (or absent).

promag commented at 2:08 pm on July 10, 2018:

Then I think it’s best to omit shortcuts for now, WDYT?

Sjors commented at 2:13 pm on July 10, 2018:

I’m fine with either no shortcuts or unique ones.

Sjors changes_requested

Sjors commented at 1:30 pm on July 10, 2018: member

On macOS when opening a wallet or creating a new wallet it suggests the directory from which opened QT by default. This also leads to strangeness in the dropdown list:

The default should be the datadir, if that can be passed into the dialog.

For creating new wallet, I think a better approach is to ask for a wallet name, and not offer the user the option to put it in a different directory for the UI.

For opening an existing wallet I have a light preference for just scanning the default directory for wallets and showing a list of wallets to choose from. This also prevents confusion around wallets that are files vs. wallets that are directories. Offering multiple wallets and offering the ability to put wallets anywhere in the filesystem are two different features.

Code looks good to me, but I’m not qualified to tell if the memory / pointer management stuff is correct.

macOS threw a warning:

0objc[17648]: Class FIFinderSyncExtensionHost is implemented in both /System/Library/PrivateFrameworks/FinderKit.framework/Versions/A/FinderKit (0x7fff86ae1c90) and /System/Library/PrivateFrameworks/FileProvider.framework/OverrideBundles/FinderSyncCollaborationFileProviderOverride.bundle/Contents/MacOS/FinderSyncCollaborationFileProviderOverride (0x113143cd8). One of the two will be used. Which one is undefined.

promag commented at 2:09 pm on July 10, 2018: member

@Sjors thanks for testing!

The default should be the datadir,

Or walletdir?

Sjors commented at 2:14 pm on July 10, 2018: member

Or walletdir?

If present, probably yes. Afaik it’s only created if there was no datadir at launch time, so at least in normal circumstances if /wallets exists, it should be non-confusing to use that as a default.

fanquake added this to the "Blockers" column in a project

in src/wallet/wallet.cpp:124 in 6741c10ce4 outdated

120+
121+    wallet->postInitProcess();
122+    return wallet;
123+}
124+
125+std::shared_ptr<CWallet> LoadWallet(const std::string& wallet_file, std::string& error, std::string& warning)

Empact commented at 3:56 pm on July 15, 2018:

nit: there’s a lot of common code between LoadWallet and CreateWallet

DrahtBot added the label Needs rebase on Jul 20, 2018

promag commented at 1:25 am on July 22, 2018: member

With #9662 merged, do we want the same feature in the UI?

promag force-pushed on Jul 22, 2018

promag commented at 1:28 am on July 22, 2018: member

Rebased.

Sjors commented at 10:46 am on July 22, 2018: member

A check box for watch-only in the GUI can probably wait. Not sure if it’s useful, but not against it either.

DrahtBot removed the label Needs rebase on Jul 22, 2018

in src/qt/bitcoingui.cpp:363 in cb89573eba outdated

357@@ -347,6 +358,9 @@ void BitcoinGUI::createActions()
358         connect(usedSendingAddressesAction, SIGNAL(triggered()), walletFrame, SLOT(usedSendingAddresses()));
359         connect(usedReceivingAddressesAction, SIGNAL(triggered()), walletFrame, SLOT(usedReceivingAddresses()));
360         connect(openAction, SIGNAL(triggered()), this, SLOT(openClicked()));
361+        connect(m_new_wallet_action, SIGNAL(triggered()), this, SLOT(newWallet()));
362+        connect(m_open_wallet_action, SIGNAL(triggered()), this, SLOT(openWallet()));
363+        connect(m_close_wallet_action, SIGNAL(triggered()), this, SLOT(closeWallet()));

MarcoFalke commented at 3:29 pm on July 22, 2018:

Could use the new syntax here?

jnewbery commented at 8:56 pm on July 23, 2018: member

Fails wallet_disableprivatekeys.py. The wallet_creation_flags are not being passed through to CreateWalletFromFile(), and so this regresses being able to create a watch-only wallet.

With #9662 merged, do we want the same feature in the UI?

Later. Let’s just try to get the basic functionality into the GUI first!

promag force-pushed on Jul 24, 2018

promag commented at 8:49 pm on July 24, 2018: member

@jnewbery thanks for the hint, fixed.

Sjors commented at 8:53 am on July 25, 2018: member

I’m a bit confused by git here: Github shows 737ecac as the most recent commit, but when I fetch your branch it’s not there. Travis seems to ignore it as well.

promag commented at 9:23 am on July 25, 2018: member

@Sjors the last commit in the branch is 940e445, github orders by author date IIUC.

in src/qt/bitcoingui.cpp:709 in 940e44556f outdated

703@@ -681,6 +704,54 @@ void BitcoinGUI::openClicked()
704     }
705 }
706 
707+void BitcoinGUI::newWallet()
708+{
709+    QString file_name = QFileDialog::getSaveFileName(this, tr("New Wallet"));

jnewbery commented at 9:37 pm on July 26, 2018:

This still defaults to the directory that qt is running in. Can we change that to the wallet directory?

in src/qt/bitcoingui.cpp:724 in 940e44556f outdated

719+    }
720+}
721+
722+void BitcoinGUI::openWallet()
723+{
724+    QString file_name = QFileDialog::getExistingDirectory(this, tr("Open Wallet"), QString(), QFileDialog::DontResolveSymlinks);

jnewbery commented at 9:38 pm on July 26, 2018:

Why is this getExistingDirectory()? A wallet can also be a file I think.

Also defaults to the directory that qt is running in.

in src/wallet/wallet.cpp:84 in 940e44556f outdated

78@@ -79,6 +79,74 @@ std::shared_ptr<CWallet> GetWallet(const std::string& name)
79     return nullptr;
80 }
81 
82+static void GetWalletNameAndPath(const std::string& wallet_file, std::string& wallet_name, fs::path& wallet_path) {

jnewbery commented at 9:38 pm on July 26, 2018:

I find this function quite inscrutable. Can you add some comments?

promag commented at 8:52 pm on September 7, 2018:

Sure!

laanwj removed this from the "Blockers" column in a project

DrahtBot added the label Needs rebase on Aug 21, 2018

laanwj added this to the "Blockers" column in a project

PierreRochard commented at 11:13 pm on August 25, 2018: contributor

Concept ACK, I read through the changes and manually tested the GUI. Ditto to @jnewbery’s comments

MarcoFalke removed this from the "Blockers" column in a project

MarcoFalke commented at 7:35 pm on August 29, 2018: member

Removing from High-priority due to conflicts

Sjors commented at 8:14 pm on September 7, 2018: member

I suggest adding a warning, once a user has selected a wallet file to open, that you should never ever use wallet files downloaded from strangers.

jnewbery commented at 8:31 pm on September 7, 2018: member

@promag - are you still actively working on this PR?

promag commented at 8:54 pm on September 7, 2018: member

I suggest adding a warning, once a user has selected a wallet file to open, that you should never ever use wallet files downloaded from strangers. @Sjors do you think it should be done in this PR?

@promag - are you still actively working on this PR? @jnewbery yes, I have some changes to submit.

jnewbery commented at 9:23 pm on September 7, 2018: member

yes, I have some changes to submit.

Great! Looking forward to seeing them :grin:

Sjors commented at 9:29 am on September 8, 2018: member

in this PR?

No, but worth creating an up for grabs issue for.

jnewbery commented at 5:37 pm on September 11, 2018: member

@promag - it’d be great to get this in for v0.18. It’s the last piece in https://github.com/bitcoin/bitcoin/projects/2.

Longer term, one of my motivations for #13059 was to have bitcoind/qt not create a default wallet at startup if one didn’t exist. This PR is the final blocker for that to happen.

gui: Factor out WalletModel::getDisplayName() b70b3452a1

gui: Move private slot BitcoinApplication::removeWallet to lambda 295f9797cf

wallet: Add CreateWallet and LoadWallet functions 919c5e22b0

interfaces: Add dynamic wallets support 2577ab48f4

gui: Add dynamic wallets support 1bb5612db8

promag commented at 10:03 pm on September 21, 2018: member

Related #11485 (comment)

wip: Some fixups a7a8be8d1f

promag force-pushed on Sep 21, 2018

DrahtBot removed the label Needs rebase on Sep 21, 2018

DrahtBot commented at 7:21 am on September 22, 2018: member

#14451 (Add BIP70 deprecation warning and allow building GUI without BIP70 support by jameshilliard)
#14437 (Refactor: Start to separate wallet from node by ryanofsky)
#14350 (Add WalletInfo class by promag)
#14291 (wallet: Add ListWalletDir utility function by promag)
#13926 ([Tools] bitcoin-wallet - a tool for creating and managing wallets offline by jnewbery)
#13756 (wallet: -avoidreuse feature for improved privacy by kallewoof)
#11625 (Add BitcoinApplication & RPCConsole tests by ryanofsky)
#10973 (Refactor: separate wallet from node by ryanofsky)

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.

practicalswift commented at 6:07 pm on September 22, 2018: contributor

This PR does not compile after rebasing on master :-)

Sjors commented at 11:51 am on September 25, 2018: member

@jnewbery wrote:

Longer term […] to have bitcoind/qt not create a default wallet at startup if one didn’t exist. This PR is the final blocker for that to happen.

That would be nice indeed. I find it quite annoying that you can’t start bitcoind without any wallet loaded. And I have too many wallet.dat files floating around that I need to carefully check are actually safe to delete :-)

I’ll test again after rebase.

Re #11485 listing wallets, this could be added later. It’s nice to just show a list of wallet names that can be loaded, avoiding the file dialog.

For this PR it should be enough if the file dialog opens in walletdir, or datadir if that doesn’t exist. (the latter is impossible now, so maybe a source code comment warning would suffice).

Creating a new wallet should probably create a data and/or wallet dir, though it’s easier if #13761 is merged first.

jnewbery commented at 12:26 pm on September 25, 2018: member

@promag - what’s the status here? This PR seems to have stalled.

promag commented at 12:58 pm on September 25, 2018: member

@jnewbery I’m rebasing on #14291. Current implementation is more suitable to external wallets.

promag commented at 1:27 pm on September 25, 2018: member

@Sjors @jnewbery this is what it looks like:

For this PR it should be enough if the file dialog opens in walletdir, or datadir if that doesn’t exist.

If we handle wallets in the -walletdir differently, then, for external wallets, I think we should point the file dialog to QDesktopServices::DocumentsLocation or whatever.

Sjors commented at 1:37 pm on September 25, 2018: member

That looks pretty nice. I’d move Open External Wallet to the bottom and just call it Other.... Indeed it makes sense that doesn’t use walletdir.

jonasschnelli commented at 6:46 pm on September 25, 2018: contributor

Doesn’t build on linux/win gitian:

 0  CXXLD    test/test_bitcoin.exe
 1  AR       qt/libbitcoinqt.a
 2  OBJCXXLD qt/bitcoin-qt.exe
 3libbitcoin_server.a(libbitcoin_server_a-node.o): In function `getWalletDir':
 4/home/ubuntu/build/bitcoin/distsrc-i686-w64-mingw32/src/interfaces/node.cpp:226: undefined reference to `GetWalletDir[abi:cxx11]()'
 5collect2: error: ld returned 1 exit status
 6Makefile:3904: recipe for target 'qt/bitcoin-qt.exe' failed
 7make[2]: *** [qt/bitcoin-qt.exe] Error 1
 8make[2]: Leaving directory '/home/ubuntu/build/bitcoin/distsrc-i686-w64-mingw32/src'
 9Makefile:10188: recipe for target 'all-recursive' failed
10make[1]: *** [all-recursive] Error 1
11make[1]: Leaving directory '/home/ubuntu/build/bitcoin/distsrc-i686-w64-mingw32/src'
12Makefile:774: recipe for target 'all-recursive' failed
13make: *** [all-recursive] Error 1
14Failed run an application inside container

 0  AR       qt/libbitcoinqt.a
 1  OBJCXXLD qt/bitcoin-qt
 2libbitcoin_server.a(libbitcoin_server_a-node.o): In function `interfaces::(anonymous namespace)::NodeImpl::getWalletDir()':
 3/home/ubuntu/build/bitcoin/distsrc-i686-pc-linux-gnu/src/interfaces/node.cpp:226: undefined reference to `GetWalletDir[abi:cxx11]()'
 4collect2: error: ld returned 1 exit status
 5Makefile:3904: recipe for target 'qt/bitcoin-qt' failed
 6make[2]: *** [qt/bitcoin-qt] Error 1
 7make[2]: Leaving directory '/home/ubuntu/build/bitcoin/distsrc-i686-pc-linux-gnu/src'
 8Makefile:10188: recipe for target 'all-recursive' failed
 9make[1]: *** [all-recursive] Error 1
10make[1]: Leaving directory '/home/ubuntu/build/bitcoin/distsrc-i686-pc-linux-gnu/src'
11Makefile:774: recipe for target 'all-recursive' failed
12make: *** [all-recursive] Error 1
13Failed run an application inside container

DrahtBot commented at 9:52 am on October 20, 2018: member

DrahtBot added the label Needs rebase on Oct 20, 2018

meshcollider commented at 6:21 am on November 3, 2018: contributor

It’d be nice to get this in to 0.18, is that feasible? Needs a rebase

Overall this is looking good, concept ACK with some code review, I’ll review more once rebased/the comments above have been addressed and the WIP fixup commit is done

meshcollider added the label Wallet on Nov 9, 2018

fanquake added this to the milestone 0.18.0 on Nov 10, 2018

Sjors commented at 5:42 pm on December 10, 2018: member

For a followup, once #11082 is merged, we should add a wallet= entry to it once a wallet is created.

Noteable changes that may cause a headache during rebase:

#14350 (by yourself, WalletLocation needed in various places)
#14437 (you need to pass interfaces::Chain& chain to CreateWallet, which needs it for Verify)

in src/qt/bitcoingui.cpp:335 in a7a8be8d1f

331@@ -330,6 +332,15 @@ void BitcoinGUI::createActions()
332     openAction = new QAction(platformStyle->TextColorIcon(":/icons/open"), tr("Open &URI..."), this);
333     openAction->setStatusTip(tr("Open a bitcoin: URI or payment request"));
334 
335+    m_new_wallet_action = new QAction(platformStyle->TextColorIcon(":/icons/open"), tr("New &Wallet"), this);

luke-jr commented at 11:10 am on January 2, 2019:

If we’re going to use icons, I think they ought to be different…

luke-jr commented at 11:13 am on January 2, 2019: member

Why is there a separate File and Wallet menu in the screenshot above? They should be the same menu…

jonasschnelli commented at 6:33 am on January 4, 2019: contributor

@promag: care to rebase/pick this up again?

Sjors commented at 4:10 pm on January 17, 2019: member

A lot of this has been moved to smaller pull requests. E.g. there’s now a separate PR for just the open wallet feature #15153. I suppose “unloading” a wallet would be fairly trivial on top of that.

For my own work on #14912 I’d love to have a (proof of concept) “create wallet” PR to build on top of. Ideally it would a dialog, with a check box for the read-only feature (and later for the empty wallet feature in #14938).

jnewbery commented at 4:20 pm on January 17, 2019: member

Feature freeze is on Feb 15th. I’d really love to have this in v0.18. @promag - any chance of a rebase so we can start testing & reviewing?

promag commented at 4:55 pm on January 17, 2019: member

@jnewbery please see #15101 and then #15153.

jonasschnelli referenced this in commit 63144335be on Jan 18, 2019

jnewbery commented at 8:37 pm on January 18, 2019: member

Can this PR be closed? I think all changes are covered by other PRs, and issue #13059 tracks the total progress.

promag closed this on Jan 18, 2019

promag deleted the branch on Jan 18, 2019

promag commented at 8:40 pm on January 18, 2019: member

Thanks for updating that @jnewbery.

jonasschnelli removed this from the "In progress" column in a project

laanwj removed the label Needs rebase on Oct 24, 2019

PastaPastaPasta referenced this in commit bc3d461c44 on Sep 8, 2021

PastaPastaPasta referenced this in commit 85503c821b on Sep 10, 2021

PastaPastaPasta referenced this in commit 90e8487af9 on Sep 10, 2021

PastaPastaPasta referenced this in commit 43744dc130 on Sep 18, 2021

PastaPastaPasta referenced this in commit 68ad472996 on Sep 18, 2021

PastaPastaPasta referenced this in commit 4197db6a7e on Sep 18, 2021

thelazier referenced this in commit 76c77bc265 on Sep 25, 2021

MarcoFalke locked this on Dec 16, 2021

gui: Add dynamic wallets support #13100