Add Wallet Restore in the GUI #471

w0xlt · 2021-11-14T06:22:27Z

This PR adds a menu item to restore a wallet from a backup file in the GUI.
Currently this option exists only in RPC interface.

Motivation: It makes easier for non-technical users to restore backups.

Master	PR

shaavan

Concept ACK

I am going to do a full tested review in some time.
It would help to add the screenshots of the difference between the master branch and the PR branch in the description.

In the meantime, here's one nit I caught.

src/wallet/wallet.cpp

shaavan

Tested Successfully on Ubuntu 20.04

I was able to successfully load the wallet from the wallet.dat file using the menu option introduced in this PR.

Master	PR

Tested that error message is working perfectly using the following patch:

--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -357,7 +357,7 @@ std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const std::string
 {
     const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::u8path(wallet_name));

-    if (fs::exists(wallet_path) || !TryCreateDirectories(wallet_path)) {
+    if (!fs::exists(wallet_path) || !TryCreateDirectories(wallet_path)) {
         error = Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", fs::PathToString(wallet_path)));
         status = DatabaseStatus::FAILED_ALREADY_EXISTS;
         return nullptr;

Screenshot of error message:

Tested warning message using the following patch

--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -85,7 +85,7 @@ static void UpdateWalletSetting(interfaces::Chain& chain,
                                 std::vector<bilingual_str>& warnings)
 {
     if (!load_on_startup) return;
-    if (load_on_startup.value() && !AddWalletSetting(chain, wallet_name)) {
+    if (load_on_startup.value() && AddWalletSetting(chain, wallet_name)) {
         warnings.emplace_back(Untranslated("Wallet load on startup setting could not be updated, so wallet may not be loaded next node startup."));
     } else if (!load_on_startup.value() && !RemoveWalletSetting(chain, wallet_name)) {

Screenshot of warning message:

There are some minor corrections and suggestions I found that might help improve this PR.

src/interfaces/wallet.h

src/qt/walletcontroller.cpp

jarolrod

concept ack

src/qt/bitcoingui.cpp

hebasto · 2021-11-21T20:59:27Z

Concept ACK.

@w0xlt
And warm welcome as a new contributor!

shaavan

Changes since my last review:

Addressed following suggestions: 1, 2
Added translator comment for strings displayed on the user’s side.

The translator comment looks good and helps significantly clarify the meaning and purpose of each string. I have some suggestions for the translator comment, though, that you might consider, to improve upon these comments.

src/qt/bitcoingui.cpp

shaavan

Changes since my last review:

Translator comments are updated according to this suggestion.

I think this PR is functionally correct now. However, there are some formatting corrections I would like to suggest. (p.s. Sorry, I forgot to check formatting in my earlier reviews of this PR.)

To automatically correct formatting, you can use the following line of code:

git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v

The differences I observed after running the above statement:

diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
index f21c5048f..363b618d8 100644
--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -442,10 +442,10 @@ void BitcoinGUI::createActions()
         });
         connect(m_restore_wallet_action, &QAction::triggered, [this] {
             QString backupFile = GUIUtil::getOpenFileName(this,
-                //: The title for Restore Wallet File Windows
-                tr("Load Wallet Backup"), QString(),
-                //: The file extension for Restore Wallet File Windows
-                tr("Wallet Data File (*.dat)"), nullptr);
+                                                          //: The title for Restore Wallet File Windows
+                                                          tr("Load Wallet Backup"), QString(),
+                                                          //: The file extension for Restore Wallet File Windows
+                                                          tr("Wallet Data File (*.dat)"), nullptr);
             if (backupFile.isEmpty()) return;
 
             bool walletNameOk;
diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp
index 38095ee74..81822d5e5 100644
--- a/src/qt/walletcontroller.cpp
+++ b/src/qt/walletcontroller.cpp
@@ -381,8 +381,7 @@ void RestoreWalletActivity::restore(const std::string& backup_file, const std::s
         tr("Restore Wallet"),
         /*: Descriptive text of the restore wallets progress window which indicates to
             the user that wallets are currently being restored.*/
-        tr("Restoring Wallet <b>%1</b>…").arg(name.toHtmlEscaped())
-    );
+        tr("Restoring Wallet <b>%1</b>…").arg(name.toHtmlEscaped()));
 
     QTimer::singleShot(0, worker(), [this, backup_file, wallet_name] {
         std::unique_ptr<interfaces::Wallet> wallet = node().walletClient().restoreWallet(backup_file, wallet_name, m_error_message, m_warning_message);
@@ -410,4 +409,3 @@ void RestoreWalletActivity::finish()
 
     Q_EMIT finished();
 }

shaavan

ACK e0b6d19

Changes since my last review:

Formatting has been fixed.

Great work, @w0xlt!

src/wallet/interfaces.cpp

luke-jr

This duplicates new RPC functionality. Please first abstract the RPC code so GUI can share it instead of duplicating it.

Also, I think it would be better to handle error conditions nicer before exposing this to the GUI. For example, we should detect if the wallet is already present and error if so. If an error occurs, we should delete the copy-destination file we made.

w0xlt · 2021-12-09T02:59:59Z

d021bf9 modifies the restoredwallet() RPC to reuse the wallet code and also deletes the wallet folder if the reload fails, as suggested in #471 (review) .

A separate PR (bitcoin/bitcoin#23721) has been opened in the Bitcoin repository to change files that are not GUI related, as suggested in #471 (comment)

Rspigler · 2021-12-14T23:44:35Z

Will test soon. Concept ACK

…ogic to the wallet section 62fa61f refactor: remove the wallet folder if the restore fails (w0xlt) abbb7ec refactor: Move restorewallet() RPC logic to the wallet section (w0xlt) 4807f73 refactor: Implement restorewallet() logic in the wallet section (w0xlt) Pull request description: Currently `restorewallet()` logic is written in the RPC layer and it can´t be reused by GUI. So it moves this to the wallet section and then, GUI can access it. This is necessary to implement the "Restore Wallet" menu item in the GUI (which is already implemented in #471 ). This commit also simplifies error handling and adds a new behavior: if the restore fails, the invalid wallet folder is removed. ACKs for top commit: achow101: ACK 62fa61f shaavan: crACK 62fa61f Tree-SHA512: 7ccfbad5943f38616ba0c2dd443c97a4b5bc1f6612dbf5a9e7a0263100aba36671fae929a2e7688442667be394645f44484af137a4802f204a33c4689eb27c39

…the wallet section 62fa61f refactor: remove the wallet folder if the restore fails (w0xlt) abbb7ec refactor: Move restorewallet() RPC logic to the wallet section (w0xlt) 4807f73 refactor: Implement restorewallet() logic in the wallet section (w0xlt) Pull request description: Currently `restorewallet()` logic is written in the RPC layer and it can´t be reused by GUI. So it moves this to the wallet section and then, GUI can access it. This is necessary to implement the "Restore Wallet" menu item in the GUI (which is already implemented in bitcoin-core/gui#471 ). This commit also simplifies error handling and adds a new behavior: if the restore fails, the invalid wallet folder is removed. ACKs for top commit: achow101: ACK 62fa61f shaavan: crACK 62fa61f Tree-SHA512: 7ccfbad5943f38616ba0c2dd443c97a4b5bc1f6612dbf5a9e7a0263100aba36671fae929a2e7688442667be394645f44484af137a4802f204a33c4689eb27c39

w0xlt · 2022-04-12T17:58:11Z

Rebased. CI error is unrelated.

hebasto · 2022-04-12T21:21:05Z

Rebased. CI error is unrelated.

CI job has been restarted.

hebasto · 2022-04-12T21:57:00Z

Approach ACK e681634.

@promag Mind having a look into this PR?

furszy

Tested and code reviewed e681634

Found the following issue:

The restored signal is being processed prior to the full execution of addWallet (walletAdded signal slot), which causes a mismatch where the GUI shows the frame of the recently loaded wallet without updating the wallet selector (the top right combo box) nor the window title.

It seems to be happening because QT lowers the addWallet execution priority while is creating the new WalletView object.

It can be fixed by making the restored signal connection a Qt::QueuedConnection type.
In other words, patch:

diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
--- a/src/qt/bitcoingui.cpp	(revision 34dff82864965126e35876c02c8158e06d652013)
+++ b/src/qt/bitcoingui.cpp	(date 1654004973424)
@@ -433,7 +433,7 @@
             if (!walletNameOk || walletName.isEmpty()) return;
 
             auto activity = new RestoreWalletActivity(m_wallet_controller, this);
-            connect(activity, &RestoreWalletActivity::restored, this, &BitcoinGUI::setCurrentWallet);
+            connect(activity, &RestoreWalletActivity::restored, this, &BitcoinGUI::setCurrentWallet, Qt::QueuedConnection);
 
             auto backup_file_path = fs::PathFromString(backup_file.toStdString());
             activity->restore(backup_file_path, walletName.toStdString());

src/qt/walletcontroller.h

w0xlt · 2022-06-02T18:36:46Z

@furszy Thanks for the sugestion. After applying it, the GUI now switches to the wallet as soon as it finishes restoring it.

w0xlt · 2022-06-02T21:35:51Z

CI error seems unrelated.

furszy

Code review + tested ACK c99f58a

DrahtBot · 2022-06-12T21:48:13Z

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

Conflicts

No conflicts as of last run.

hebasto

Approach ACK c99f58a.

Suggesting to apply clang-format-diff.py to your commit(s).

Is it better to combine actions in a menu group:

--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -473,12 +473,13 @@ void BitcoinGUI::createMenuBar()
     {
         file->addAction(m_create_wallet_action);
         file->addAction(m_open_wallet_action);
-        file->addAction(m_restore_wallet_action);
         file->addAction(m_close_wallet_action);
         file->addAction(m_close_all_wallets_action);
         file->addSeparator();
-        file->addAction(openAction);
         file->addAction(backupWalletAction);
+        file->addAction(m_restore_wallet_action);
+        file->addSeparator();
+        file->addAction(openAction);
         file->addAction(signMessageAction);
         file->addAction(verifyMessageAction);
         file->addAction(m_load_psbt_action);

?

hebasto · 2022-06-26T17:28:50Z

src/qt/bitcoingui.cpp

+                                                          //: The title for Restore Wallet File Windows
+                                                          tr("Load Wallet Backup"), QString(),
+                                                          //: The file extension for Restore Wallet File Windows
+                                                          tr("Wallet Data File (*.dat)"), nullptr);


This line and this one

gui/src/qt/walletview.cpp

Line 216 in c99f58a

tr("Wallet Data") + QLatin1String(" (*.dat)"), nullptr);

must be consistent. Although, have no preference which one is better.

And please separate untranslatable file extension from the translatable string.

Done in e7a3f69.
The file extension is now separated from the file format name.

hebasto · 2022-06-26T17:31:11Z

src/qt/bitcoingui.cpp

+                                                          tr("Wallet Data File (*.dat)"), nullptr);
+            if (backup_file.isEmpty()) return;
+
+            bool walletNameOk;


Please follow our naming convention:

Suggested change

bool walletNameOk;

bool wallet_name_ok;

Done in e7a3f69.

hebasto · 2022-06-26T17:32:11Z

src/qt/bitcoingui.cpp

+
+            bool walletNameOk;
+            //: Title of the Restore Wallet input dialog (where the wallet name is entered)
+            QString walletName = QInputDialog::getText(this, tr("Restore Name"), tr("Wallet Name:"), QLineEdit::Normal, "", &walletNameOk);


Please follow our naming convention:

Suggested change

QString walletName = QInputDialog::getText(this, tr("Restore Name"), tr("Wallet Name:"), QLineEdit::Normal, "", &walletNameOk);

QString wallet_name = QInputDialog::getText(this, tr("Restore Name"), tr("Wallet Name:"), QLineEdit::Normal, "", &walletNameOk);

Done in e7a3f69.

hebasto · 2022-06-26T17:33:45Z

src/qt/walletcontroller.cpp

+    if (m_wallet_model) Q_EMIT restored(m_wallet_model);
+
+    Q_EMIT finished();
+}


Add a newline?

Done in e7a3f69.

Co-authored-by: Shashwat Vangani <85434418+shaavan@users.noreply.github.com> Co-authored-by: furszy <matiasfurszyfer@protonmail.com>

w0xlt · 2022-06-27T13:41:05Z

@hebasto Thanks for the review. Your suggestions were addressed in e7a3f69.

jarolrod

Concept ACK, could use a release note

shaavan

Changes since my last review:

renamed walletName → wallet_name
renamed walletNameOk → wallet_name_ok
The connection between restored and set current wallet has been changed to Qt::QueuedConnection, which is explained in this comment Add Wallet Restore in the GUI #471 (review)). On a side note, a very nice catch of bug indeed, @furszy!
The backup wallet and restore wallet options are grouped in a menu group.
Declared class path.h at the beginning of walletcontroller.h to facilitate using fs::path.

I successfully compiled this PR on Ubuntu 22.04 with Qt version 5.15.3. I was able to test that the backup wallet and restore wallet are visually grouped in the GUI.

Screenshot:

I agree with @jarolrod. Adding release notes would be a good idea!

w0xlt · 2022-07-01T13:21:14Z

Added a release note in a new commit (bc13ec8) to not invalidate the ACKs for the previous one.

furszy

utACK bc13ec8

shaavan

ACK bc13ec8

In conjunction with my previous review, I find the changes in this PR to be complete and ready for merge.

Great work, @w0xlt!

hebasto

ACK bc13ec8

hebasto · 2022-07-10T19:36:33Z

src/qt/bitcoingui.cpp

+            //: Title of the Restore Wallet input dialog (where the wallet name is entered)
+            QString wallet_name = QInputDialog::getText(this, tr("Restore Name"), tr("Wallet Name:"), QLineEdit::Normal, "", &wallet_name_ok);


A translator comment followed by a line with more than one tr() could not work as expected.

9d9a098 gui: Fix translator comment for Restore Wallet QInputDialog (w0xlt) Pull request description: Fix translator comment for Restore Wallet `QInputDialog`, as suggested in #471 (comment). This also changes the window title name from `Restore Name` to `Restore Wallet` as it seems clearer. ACKs for top commit: shaavan: reACK 9d9a098 Tree-SHA512: 02aec661839215ab1183e4e92fa131671daa986339373a87c0a0e2c5e79a46f362a8846f4a5f6d630a99884a7949031982d13352336bd3f0573625826406dde8

… `QInputDialog` 9d9a098 gui: Fix translator comment for Restore Wallet QInputDialog (w0xlt) Pull request description: Fix translator comment for Restore Wallet `QInputDialog`, as suggested in bitcoin-core/gui#471 (comment). This also changes the window title name from `Restore Name` to `Restore Wallet` as it seems clearer. ACKs for top commit: shaavan: reACK 9d9a098 Tree-SHA512: 02aec661839215ab1183e4e92fa131671daa986339373a87c0a0e2c5e79a46f362a8846f4a5f6d630a99884a7949031982d13352336bd3f0573625826406dde8

shaavan reviewed Nov 14, 2021

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

w0xlt force-pushed the restore_wallet_gui branch from 985c64e to f202ddb Compare November 14, 2021 08:12

shaavan reviewed Nov 14, 2021

View reviewed changes

src/interfaces/wallet.h Outdated Show resolved Hide resolved

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

jarolrod reviewed Nov 21, 2021

View reviewed changes

src/qt/bitcoingui.cpp Show resolved Hide resolved

hebasto added Feature Wallet labels Nov 21, 2021

hebasto changed the title ~~gui: Add Wallet Restore in the GUI~~ Add Wallet Restore in the GUI Nov 21, 2021

hebasto requested a review from promag November 21, 2021 20:59

w0xlt force-pushed the restore_wallet_gui branch 2 times, most recently from 32b6e3e to a047c1a Compare November 22, 2021 06:38

shaavan reviewed Nov 22, 2021

View reviewed changes

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

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

w0xlt force-pushed the restore_wallet_gui branch from a047c1a to b5ad1bd Compare November 22, 2021 18:33

shaavan reviewed Nov 23, 2021

View reviewed changes

w0xlt force-pushed the restore_wallet_gui branch from b5ad1bd to e0b6d19 Compare November 24, 2021 00:34

shaavan approved these changes Nov 26, 2021

View reviewed changes

jarolrod reviewed Nov 28, 2021

View reviewed changes

src/wallet/interfaces.cpp Outdated Show resolved Hide resolved

luke-jr suggested changes Nov 30, 2021

View reviewed changes

DrahtBot added the Needs rebase label Dec 8, 2021

w0xlt force-pushed the restore_wallet_gui branch 2 times, most recently from 32b9eba to d021bf9 Compare December 8, 2021 19:02

DrahtBot removed the Needs rebase label Dec 8, 2021

This was referenced Dec 8, 2021

ci, error: bitcoind should have exited within 2400s with expected error Error opening block database. bitcoin/bitcoin#23717

Closed

wallet: Move restorewallet() logic to the wallet section bitcoin/bitcoin#23721

Merged

DrahtBot added the Needs rebase label Dec 16, 2021

w0xlt force-pushed the restore_wallet_gui branch from d021bf9 to e82a654 Compare December 16, 2021 21:22

hebasto removed the Waiting for author label Apr 12, 2022

furszy suggested changes May 31, 2022

View reviewed changes

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

w0xlt force-pushed the restore_wallet_gui branch from e681634 to c99f58a Compare June 2, 2022 18:31

furszy approved these changes Jun 3, 2022

View reviewed changes

hebasto reviewed Jun 26, 2022

View reviewed changes

gui: Add Wallet Restore in the GUI

e7a3f69

Co-authored-by: Shashwat Vangani <85434418+shaavan@users.noreply.github.com> Co-authored-by: furszy <matiasfurszyfer@protonmail.com>

w0xlt force-pushed the restore_wallet_gui branch from c99f58a to e7a3f69 Compare June 27, 2022 13:32

jarolrod reviewed Jun 28, 2022

View reviewed changes

shaavan reviewed Jun 29, 2022

View reviewed changes

doc: Add a release note about the "restore wallet" menu item

bc13ec8

furszy approved these changes Jul 1, 2022

View reviewed changes

shaavan approved these changes Jul 5, 2022

View reviewed changes

hebasto added this to the 24.0 milestone Jul 10, 2022

hebasto approved these changes Jul 10, 2022

View reviewed changes

hebasto merged commit f9783b0 into bitcoin-core:master Jul 10, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 11, 2022

Merge bitcoin-core/gui#471: Add Wallet Restore in the GUI

ae76e37

w0xlt deleted the restore_wallet_gui branch July 11, 2022 12:36

w0xlt mentioned this pull request Jul 11, 2022

Fix translator comment for Restore Wallet QInputDialog #629

Merged

w0xlt mentioned this pull request Sep 9, 2022

Switch to the selected wallet after loading #665

Merged

bitcoin-core locked and limited conversation to collaborators Nov 16, 2023

	QString walletName = QInputDialog::getText(this, tr("Restore Name"), tr("Wallet Name:"), QLineEdit::Normal, "", &walletNameOk);
	QString wallet_name = QInputDialog::getText(this, tr("Restore Name"), tr("Wallet Name:"), QLineEdit::Normal, "", &walletNameOk);

		//: Title of the Restore Wallet input dialog (where the wallet name is entered)
		QString wallet_name = QInputDialog::getText(this, tr("Restore Name"), tr("Wallet Name:"), QLineEdit::Normal, "", &wallet_name_ok);

Add Wallet Restore in the GUI #471

Add Wallet Restore in the GUI #471

Conversation

w0xlt commented Nov 14, 2021 • edited

shaavan left a comment

Choose a reason for hiding this comment

shaavan left a comment

Choose a reason for hiding this comment

jarolrod left a comment

Choose a reason for hiding this comment

hebasto commented Nov 21, 2021

shaavan left a comment

Choose a reason for hiding this comment

shaavan left a comment

Choose a reason for hiding this comment

shaavan left a comment

Choose a reason for hiding this comment

luke-jr left a comment

Choose a reason for hiding this comment

w0xlt commented Dec 9, 2021

Rspigler commented Dec 14, 2021

w0xlt commented Apr 12, 2022

hebasto commented Apr 12, 2022

hebasto commented Apr 12, 2022

furszy left a comment • edited

Choose a reason for hiding this comment

w0xlt commented Jun 2, 2022

w0xlt commented Jun 2, 2022

furszy left a comment

Choose a reason for hiding this comment

DrahtBot commented Jun 12, 2022 • edited

Conflicts

hebasto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w0xlt commented Jun 27, 2022 • edited

jarolrod left a comment

Choose a reason for hiding this comment

shaavan left a comment

Choose a reason for hiding this comment

w0xlt commented Jul 1, 2022

furszy left a comment

Choose a reason for hiding this comment

shaavan left a comment

Choose a reason for hiding this comment

hebasto left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w0xlt commented Nov 14, 2021 •

edited

furszy left a comment •

edited

DrahtBot commented Jun 12, 2022 •

edited

w0xlt commented Jun 27, 2022 •

edited

hebasto left a comment •

edited