GUI: Change the receive button to respond to keypool state changing

achow101 commented at 11:48 pm on January 21, 2019: member

Currently the Receive button in the GUI is displayed enabled or disabled by the initial state of the wallet when the wallet is first loaded. The button is only enabled or disabled depending on whether the disable private keys flag is set when the wallet is loaded. However, future changes to the wallet means that this initial state and check may no longer be accurate. #14938 introduces empty wallets which do not have private keys. An empty wallet that is loaded should have the Receive button disabled, and then it should become enabled once sethdseed is used so that a keypool can be generated and new keys generated. Likewise, with #14075, a wallet can be loaded with no keypool initially, so the button should be disabled. Later, public keys can be imported into the keypool, at which time the button should become enabled. When the keypool runs out again (no new keys are generated as the keypool only consists of imports), the button should become disabled.

This PR makes it so that the button becomes enabled and disabled as the keypool state changes. The check for whether to enable or disable the receive button has changed to checking whether it is possible to get new keys. It now checks for whether the wallet has an HD seed and, if not, whether the private keys are disabled. When an action happens which would make it possible for a new address to be retrieved or make it possible for a no more addresses to be retrieved, a signal is emitted which has the GUI recheck the conditions for the Receive button. These actions are setting a new HD seed, topping up the keypool, retrieving a key from the keypool, and returning a key to the keypool.

achow101 force-pushed on Jan 21, 2019

Check for more than private keys disabled to show receive button 14bcdbe09c

achow101 force-pushed on Jan 22, 2019

fanquake added the label GUI on Jan 22, 2019

DrahtBot commented at 1:20 am on January 22, 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:

#15226 (Allow creating blank (empty) wallets (alternative) by achow101)
#15006 (Add option to create an encrypted wallet by achow101)
#14075 (Import watch only pubkeys to the keypool if private keys are disabled by achow101)

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.

achow101 force-pushed on Jan 22, 2019

jonasschnelli commented at 6:22 am on January 22, 2019: contributor

Looks good. Concept ACK

A minor not very relevant question would be, if it makes sense to transport the work “keypool” upwards to the GUI… since the GUI don’t need to understand the concept of keypool,… just if addresses can be fetched or not.

in src/qt/receivecoinsdialog.cpp:103 in 2795d95bfc outdated

100@@ -101,10 +101,18 @@ void ReceiveCoinsDialog::setModel(WalletModel *_model)
101         }
102 
103         // eventually disable the main receive button if private key operations are disabled

Sjors commented at 6:03 pm on January 22, 2019:

Nit: comment is outdated

achow101 commented at 10:25 pm on January 22, 2019:

Fixed.

in src/qt/receivecoinsdialog.cpp:106 in 2795d95bfc outdated

100@@ -101,10 +101,18 @@ void ReceiveCoinsDialog::setModel(WalletModel *_model)
101         }
102 
103         // eventually disable the main receive button if private key operations are disabled
104-        ui->receiveButton->setEnabled(!model->privateKeysDisabled());
105+        ui->receiveButton->setEnabled(model->canGetAddresses());
106+
107+        // Show new address button when keypool has new keys

Sjors commented at 6:05 pm on January 22, 2019:

Show -> Enable? And maybe say “when new addresses can be generated by the wallet (i.e. when keypool has new keys)”

achow101 commented at 10:25 pm on January 22, 2019:

Changed.

in src/qt/walletmodel.cpp:588 in 2795d95bfc outdated

582@@ -571,6 +583,11 @@ bool WalletModel::privateKeysDisabled() const
583     return m_wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
584 }
585 
586+bool WalletModel::canGetAddresses() const
587+{
588+    return m_wallet->hdEnabled() || (!m_wallet->hdEnabled() && !m_wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));

Sjors commented at 6:21 pm on January 22, 2019:

Suggested comment, since hdEnabled() changes in related pull requests, which can get confusing fast:

0// The wallet can provide a fresh address if:
1// * hdEnabled(): an HD seed is present; or
2// * it is a legacy wallet, because:
3//     * !hdEnabled(): an HD seed is not present; and
4//     * !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS): private keys have not been disabled (which results in hdEnabled() == true)

achow101 commented at 10:25 pm on January 22, 2019:

Done

Sjors commented at 6:22 pm on January 22, 2019: member

Concept ACK.

I agree with @jonasschnelli that the GUI merely needs to know that a new address can be generated. That’s already done at the wallet level WalletModel::canGetAddresses(), and I’m fine with renaming the other stuff later. E.g. handleKeypoolChanged -> handleFreshAddressAvailable / handleCanGetAddresses.

See inline feedback on 2795d95bfc6810fd91e6f94943c93ec247b25662

achow101 force-pushed on Jan 22, 2019

achow101 commented at 10:25 pm on January 22, 2019: member

I’ve renamed the signal and handlers to be CanGetAddresses instead of Keypool.

in src/qt/walletmodel.cpp:433 in f76822dc07 outdated

427@@ -423,6 +428,11 @@ static void NotifyWatchonlyChanged(WalletModel *walletmodel, bool fHaveWatchonly
428                               Q_ARG(bool, fHaveWatchonly));
429 }
430 
431+static void NotifyCanGetAddressesChanged(WalletModel *walletmodel)
432+{
433+    QMetaObject::invokeMethod(walletmodel, "updateCanGetAddresses", Qt::QueuedConnection);

promag commented at 10:33 pm on January 22, 2019:

I think it’s safe to omit Qt::QueuedConnection - if current thread is not the same as the wallet_model’s thread then the default Qt::AutoConnection will do the same.

I think you can also call the signal invokeMethod(wallet_model, "canGetAddressChanged") and avoid the proxy slot updateCanGetAddress.

achow101 commented at 8:55 pm on January 23, 2019:

Done. I’ve removed the proxy slot.

in src/qt/walletmodel.cpp:431 in f76822dc07 outdated

427@@ -423,6 +428,11 @@ static void NotifyWatchonlyChanged(WalletModel *walletmodel, bool fHaveWatchonly
428                               Q_ARG(bool, fHaveWatchonly));
429 }
430 
431+static void NotifyCanGetAddressesChanged(WalletModel *walletmodel)

promag commented at 10:33 pm on January 22, 2019:

nit, WalletModel* wallet_model.

achow101 commented at 8:55 pm on January 23, 2019:

Done

in src/qt/walletmodel.h:289 in f76822dc07 outdated

284@@ -283,6 +285,9 @@ class WalletModel : public QObject
285     // Signal that wallet is about to be removed
286     void unload();
287 
288+    // Notify that there are now keys in the keypool
289+    void notifyCanGetAddressesChanged();

promag commented at 10:35 pm on January 22, 2019:

nit, just canGetAddressChanged().

achow101 commented at 8:55 pm on January 23, 2019:

Done.

in src/qt/receivecoinsdialog.cpp:107 in f76822dc07 outdated

104-        ui->receiveButton->setEnabled(!model->privateKeysDisabled());
105+        // Set the button to be enabled or disabled based on whether the wallet can give out new addresses.
106+        ui->receiveButton->setEnabled(model->canGetAddresses());
107+
108+        // Enable/disable the receive button if the wallet is now able/unable to give out new addresses.
109+        connect(_model, &WalletModel::notifyCanGetAddressesChanged, this, &ReceiveCoinsDialog::updateReceiveButton);

promag commented at 10:43 pm on January 22, 2019:

I think you could avoid the slot, as it is only used once:

0connect(model, &WalletModel::canGetAddressesChanged, [this] {
1    ui->receiveButton->setEnabled(model->canGetAddresses());
2});

achow101 commented at 8:55 pm on January 23, 2019:

Changed.

promag commented at 10:50 pm on January 22, 2019: member

utACK f76822d, just some comments for your consideration.

Notify the GUI that the keypool has changed to set the receive button

Whenever the keypool changes (new keys generated, new seed set,
keypool runs out, etc.), notify the GUI that the keypool has changed. The
receive button can then be enabled and disabled as necessary.

2bc4c3eaf9

achow101 force-pushed on Jan 23, 2019

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

jonasschnelli commented at 8:20 pm on January 29, 2019: contributor

utACK 2bc4c3eaf96f5f8490fc79280422916c5d14cde3 I think hard to protect against regression as it is (not functional exposed). Ideally test coverage follows in #15226.

Sjors commented at 11:14 am on January 30, 2019: member

Semi tested ACK 2bc4c3e. Based on testing #15226, but they’re currently rebased out of sync.

laanwj merged this on Jan 31, 2019

laanwj closed this on Jan 31, 2019

laanwj referenced this in commit 7c09e209ef on Jan 31, 2019

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

meshcollider referenced this in commit 6f4e0d1542 on Feb 10, 2019

fanquake referenced this in commit c4be50fea3 on Aug 31, 2020

fanquake referenced this in commit 9876ab8c74 on Sep 3, 2020

Munkybooty referenced this in commit 2fa3899964 on Aug 21, 2021

Munkybooty referenced this in commit a1cef14cce on Aug 21, 2021

Munkybooty referenced this in commit 8c6c481d4a on Aug 23, 2021

Munkybooty referenced this in commit 0a3ce36ba6 on Aug 24, 2021

Munkybooty referenced this in commit 9c696c5a14 on Aug 24, 2021

Munkybooty referenced this in commit 81bfd0c463 on Aug 24, 2021

UdjinM6 referenced this in commit 8d38af1289 on Aug 24, 2021

Munkybooty referenced this in commit baa76aa331 on Aug 24, 2021

DrahtBot locked this on Dec 16, 2021

GUI: Change the receive button to respond to keypool state changing #15225

Conflicts