Merge #2859: [GUI] CoinControl Change

1ec227f Fix coincontrol change (Alessandro Rezzi) Pull request description: This PR extends the functionality of CoinControl change by adding shield addresses. In particular: - In a transparent transaction user can select either a transparent or a sapling shield change - In a shield transaction user can only use a shield address as change (to preserve privacy) (NB: with transparent transaction I mean a transaction in which inputs are transparent and analogous definition for shield transaction) In addition this PR fixes #2245 ACKs for top commit: Liquid369: re tACK 1ec227f Fuzzbawls: ACK 1ec227f Tree-SHA512: de8a48066181d42f5080599ac0e0aa8f28c6562c42b7da48060a3275817cd0c0d7503ae22195a2b7d52214f05a77ac1294c16ec4f71096245e9338a1300588f8
PIVX-Project · Jul 27, 2023 · fec4cfe · fec4cfe
2 parents 559e4f1 + 1ec227f
commit fec4cfe
Show file tree

Hide file tree

Showing 5 changed files with 71 additions and 33 deletions.
diff --git a/src/coincontrol.h b/src/coincontrol.h
@@ -7,8 +7,10 @@
 #ifndef BITCOIN_COINCONTROL_H
 #define BITCOIN_COINCONTROL_H
 
+#include "optional.h"
 #include "policy/feerate.h"
 #include "primitives/transaction.h"
+#include "sapling/address.h"
 #include "script/standard.h"
 #include <unordered_set>
 
@@ -31,6 +33,8 @@ class OutPointWrapper {
 class CCoinControl
 {
 public:
+    // TODO: upgrade those two fields to a single CWDestination?
+    Optional<libzcash::SaplingPaymentAddress> destShieldChange = boost::none;
     CTxDestination destChange = CNoDestination();
     //! If false, allows unselected inputs, but requires all selected inputs be used
     bool fAllowOtherInputs;

diff --git a/src/qt/pivx/send.cpp b/src/qt/pivx/send.cpp
@@ -6,6 +6,8 @@
 #include "addresstablemodel.h"
 #include "clientmodel.h"
 #include "coincontrol.h"
+#include "destination_io.h"
+#include "key_io.h"
 #include "openuridialog.h"
 #include "operationresult.h"
 #include "optionsmodel.h"
@@ -18,6 +20,8 @@
 #include "qt/pivx/sendchangeaddressdialog.h"
 #include "qt/pivx/sendconfirmdialog.h"
 #include "qt/walletmodel.h"
+#include "sapling/address.h"
+#include "sapling/key_io_sapling.h"
 #include "script/standard.h"
 
 #define REQUEST_PREPARE_TX 1
@@ -273,9 +277,11 @@ void SendWidget::resetCoinControl()
 
 void SendWidget::resetChangeAddress()
 {
-    if (coinControlDialog) coinControlDialog->coinControl->destChange = CNoDestination();
+    if (coinControlDialog) {
+        coinControlDialog->coinControl->destShieldChange = boost::none;
+        coinControlDialog->coinControl->destChange = CNoDestination();
+    }
     ui->btnChangeAddress->setActive(false);
-    ui->btnChangeAddress->setVisible(isTransparent);
 }
 
 void SendWidget::clearEntries()
@@ -414,7 +420,7 @@ void SendWidget::ProcessSend(QList<SendCoinsRecipient>& recipients, bool hasShie
                              const std::function<bool(QList<SendCoinsRecipient>&)>& func)
 {
     // First check SPORK_20 (before unlock)
-    bool isShieldedTx = hasShieldedOutput || !isTransparent;
+    bool isShieldedTx = hasShieldedOutput || !isTransparent || coinControlDialog->coinControl->destShieldChange;
     if (isShieldedTx) {
         if (walletModel->isSaplingInMaintenance()) {
             inform(tr("Sapling Protocol temporarily in maintenance. Shielded transactions disabled (SPORK 20)"));
@@ -468,7 +474,7 @@ void SendWidget::ProcessSend(QList<SendCoinsRecipient>& recipients, bool hasShie
 
 OperationResult SendWidget::prepareShielded(WalletModelTransaction* currentTransaction, bool fromTransparent)
 {
-    bool hasCoinsOrNotesSelected = coinControlDialog && coinControlDialog->coinControl && coinControlDialog->coinControl->HasSelected();
+    bool hasCoinsOrNotesSelected = coinControlDialog && coinControlDialog->coinControl;
     return walletModel->PrepareShieldedTransaction(currentTransaction,
                                                    fromTransparent,
                                                    hasCoinsOrNotesSelected ? coinControlDialog->coinControl : nullptr);
@@ -605,15 +611,18 @@ void SendWidget::updateEntryLabels(const QList<SendCoinsRecipient>& recipients)
 void SendWidget::onChangeAddressClicked()
 {
     showHideOp(true);
-    SendChangeAddressDialog* dialog = new SendChangeAddressDialog(window, walletModel);
+    SendChangeAddressDialog* dialog = new SendChangeAddressDialog(window, walletModel, isTransparent);
     if (IsValidDestination(coinControlDialog->coinControl->destChange)) {
         dialog->setAddress(QString::fromStdString(EncodeDestination(coinControlDialog->coinControl->destChange)));
+    } else if (coinControlDialog->coinControl->destShieldChange) {
+        dialog->setAddress(QString::fromStdString(KeyIO::EncodePaymentAddress(*(coinControlDialog->coinControl->destShieldChange))));
     }
 
-    CTxDestination destChange = (openDialogWithOpaqueBackgroundY(dialog, window, 3, 5) ?
-                                 dialog->getDestination() : CNoDestination());
+    CWDestination destChange = (openDialogWithOpaqueBackgroundY(dialog, window, 3, 5) ?
+                                    dialog->getDestination() :
+                                    CNoDestination());
 
-    if (!IsValidDestination(destChange)) {
+    if (!Standard::IsValidDestination(destChange)) {
         // no change address set
         ui->btnChangeAddress->setActive(false);
     } else {
@@ -627,7 +636,16 @@ void SendWidget::onChangeAddressClicked()
     }
 
     // save change address in coin control
-    coinControlDialog->coinControl->destChange = destChange;
+    const CTxDestination* transparentDest = Standard::GetTransparentDestination(destChange);
+    if (transparentDest) {
+        coinControlDialog->coinControl->destChange = *transparentDest;
+        coinControlDialog->coinControl->destShieldChange = boost::none;
+    }
+    const libzcash::SaplingPaymentAddress* shieldDest = Standard::GetShieldedDestination(destChange);
+    if (shieldDest) {
+        coinControlDialog->coinControl->destShieldChange = *shieldDest;
+        coinControlDialog->coinControl->destChange = CNoDestination();
+    }
     dialog->deleteLater();
 }
 

diff --git a/src/qt/pivx/sendchangeaddressdialog.cpp b/src/qt/pivx/sendchangeaddressdialog.cpp
@@ -6,14 +6,13 @@
 #include "qt/pivx/forms/ui_sendchangeaddressdialog.h"
 #include "qt/pivx/qtutils.h"
 
-SendChangeAddressDialog::SendChangeAddressDialog(QWidget* parent, WalletModel* model) :
-    FocusedDialog(parent),
-    walletModel(model),
-    ui(new Ui::SendChangeAddressDialog)
+SendChangeAddressDialog::SendChangeAddressDialog(QWidget* parent, WalletModel* model, bool isTransparent) : FocusedDialog(parent),
+                                                                                                            walletModel(model),
+                                                                                                            ui(new Ui::SendChangeAddressDialog)
 {
     // Change address
     dest = CNoDestination();
-
+    this->isTransparent = isTransparent;
     if (!walletModel) {
         throw std::runtime_error(strprintf("%s: No wallet model set", __func__));
     }
@@ -47,7 +46,7 @@ void SendChangeAddressDialog::setAddress(QString address)
     ui->btnCancel->setText(tr("RESET"));
 }
 
-CTxDestination SendChangeAddressDialog::getDestination() const
+CWDestination SendChangeAddressDialog::getDestination() const
 {
     return dest;
 }
@@ -74,12 +73,16 @@ void SendChangeAddressDialog::accept()
         QDialog::accept();
     } else {
         // validate address
-        bool isStakingAddr;
-        dest = DecodeDestination(ui->lineEditAddress->text().toStdString(), isStakingAddr);
-        if (!IsValidDestination(dest)) {
+        bool isStakingAddr = false;
+        bool isShield = false;
+        dest = Standard::DecodeDestination(ui->lineEditAddress->text().toStdString(), isStakingAddr, isShield);
+
+        if (!Standard::IsValidDestination(dest)) {
             inform(tr("Invalid address"));
         } else if (isStakingAddr) {
             inform(tr("Cannot use cold staking addresses for change"));
+        } else if (!isShield && !isTransparent) {
+            inform(tr("Cannot use a transparent change for a shield transaction"));
         } else {
             QDialog::accept();
         }

diff --git a/src/qt/pivx/sendchangeaddressdialog.h b/src/qt/pivx/sendchangeaddressdialog.h
@@ -5,9 +5,10 @@
 #ifndef SENDCHANGEADDRESSDIALOG_H
 #define SENDCHANGEADDRESSDIALOG_H
 
-#include "script/standard.h"
+#include "destination_io.h"
 #include "qt/pivx/focuseddialog.h"
 #include "qt/pivx/snackbar.h"
+#include "script/standard.h"
 
 class WalletModel;
 
@@ -20,19 +21,20 @@ class SendChangeAddressDialog : public FocusedDialog
     Q_OBJECT
 
 public:
-    explicit SendChangeAddressDialog(QWidget* parent, WalletModel* model);
+    explicit SendChangeAddressDialog(QWidget* parent, WalletModel* model, bool isTransparent);
     ~SendChangeAddressDialog();
 
     void setAddress(QString address);
-    CTxDestination getDestination() const;
+    CWDestination getDestination() const;
 
     void showEvent(QShowEvent* event) override;
 
 private:
+    bool isTransparent;
     WalletModel* walletModel;
     Ui::SendChangeAddressDialog *ui;
     SnackBar *snackBar = nullptr;
-    CTxDestination dest;
+    CWDestination dest;
 
     void inform(const QString& text);
 

diff --git a/src/sapling/sapling_operation.cpp b/src/sapling/sapling_operation.cpp
@@ -8,6 +8,7 @@
 #include "net.h" // for g_connman
 #include "policy/policy.h" // for GetDustThreshold
 #include "sapling/key_io_sapling.h"
+#include "script/standard.h"
 #include "utilmoneystr.h"        // for FormatMoney
 
 struct TxValues
@@ -82,7 +83,7 @@ OperationResult SaplingOperation::build()
     bool isFromtAddress = false;
     bool isFromShielded = false;
 
-    if (coinControl) {
+    if (coinControl && coinControl->HasSelected()) {
         // if coin control was selected it overrides any other defined configuration
         std::vector<OutPointWrapper> coins;
         coinControl->ListSelected(coins);
@@ -186,17 +187,27 @@ OperationResult SaplingOperation::build()
         const auto& retCalc = checkTxValues(txValues, isFromtAddress, isFromShielded);
         if (!retCalc) return retCalc;
 
-        // Set change address if we are using transparent funds
-        if (isFromtAddress) {
-            if (!tkeyChange) {
-                tkeyChange = new CReserveKey(wallet);
-            }
-            CPubKey vchPubKey;
-            if (!tkeyChange->GetReservedKey(vchPubKey, true)) {
-                return errorOut("Could not generate a taddr to use as a change address");
+        // By default look for a shield change address
+        if (coinControl && coinControl->destShieldChange) {
+            txBuilder.SendChangeTo(*coinControl->destShieldChange, ovk);
+
+            // If not found, and the transaction is transparent, set transparent change address
+        } else if (isFromtAddress) {
+            // Try to use coin control first
+            if (coinControl && IsValidDestination(coinControl->destChange)) {
+                txBuilder.SendChangeTo(coinControl->destChange);
+                // No Coin control! Then we can just use a random key from the keypool
+            } else {
+                if (!tkeyChange) {
+                    tkeyChange = new CReserveKey(wallet);
+                }
+                CPubKey vchPubKey;
+                if (!tkeyChange->GetReservedKey(vchPubKey, true)) {
+                    return errorOut("Could not generate a taddr to use as a change address");
+                }
+                CTxDestination changeAddr = vchPubKey.GetID();
+                txBuilder.SendChangeTo(changeAddr);
             }
-            CTxDestination changeAddr = vchPubKey.GetID();
-            txBuilder.SendChangeTo(changeAddr);
         }
 
         // Build the transaction