From 026b5b4523317fdefc69cf5cec55f76f18ad0c0a Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 21 Feb 2022 12:46:30 +0100 Subject: [PATCH 1/3] move-only: helper function to present PSBT This commit does not change behavior. Review hint: git show --color-moved --color-moved-ws=allow-indentation-change --- src/qt/sendcoinsdialog.cpp | 89 ++++++++++++++++++++------------------ src/qt/sendcoinsdialog.h | 2 + 2 files changed, 49 insertions(+), 42 deletions(-) diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index 579ef0c3fd..b77fbb7b3b 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -401,6 +401,52 @@ bool SendCoinsDialog::PrepareSendText(QString& question_string, QString& informa return true; } +void SendCoinsDialog::presentPSBT(PartiallySignedTransaction& psbtx) +{ + // Serialize the PSBT + CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); + ssTx << psbtx; + GUIUtil::setClipboard(EncodeBase64(ssTx.str()).c_str()); + QMessageBox msgBox; + msgBox.setText("Unsigned Transaction"); + msgBox.setInformativeText("The PSBT has been copied to the clipboard. You can also save it."); + msgBox.setStandardButtons(QMessageBox::Save | QMessageBox::Discard); + msgBox.setDefaultButton(QMessageBox::Discard); + switch (msgBox.exec()) { + case QMessageBox::Save: { + QString selectedFilter; + QString fileNameSuggestion = ""; + bool first = true; + for (const SendCoinsRecipient &rcp : m_current_transaction->getRecipients()) { + if (!first) { + fileNameSuggestion.append(" - "); + } + QString labelOrAddress = rcp.label.isEmpty() ? rcp.address : rcp.label; + QString amount = BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), rcp.amount); + fileNameSuggestion.append(labelOrAddress + "-" + amount); + first = false; + } + fileNameSuggestion.append(".psbt"); + QString filename = GUIUtil::getSaveFileName(this, + tr("Save Transaction Data"), fileNameSuggestion, + //: Expanded name of the binary PSBT file format. See: BIP 174. + tr("Partially Signed Transaction (Binary)") + QLatin1String(" (*.psbt)"), &selectedFilter); + if (filename.isEmpty()) { + return; + } + std::ofstream out{filename.toLocal8Bit().data(), std::ofstream::out | std::ofstream::binary}; + out << ssTx.str(); + out.close(); + Q_EMIT message(tr("PSBT saved"), "PSBT saved to disk", CClientUIInterface::MSG_INFORMATION); + break; + } + case QMessageBox::Discard: + break; + default: + assert(false); + } // msgBox.exec() +} + void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked) { if(!model || !model->getOptionsModel()) @@ -481,48 +527,7 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked) // Copy PSBT to clipboard and offer to save assert(!complete); - // Serialize the PSBT - CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); - ssTx << psbtx; - GUIUtil::setClipboard(EncodeBase64(ssTx.str()).c_str()); - QMessageBox msgBox; - msgBox.setText("Unsigned Transaction"); - msgBox.setInformativeText("The PSBT has been copied to the clipboard. You can also save it."); - msgBox.setStandardButtons(QMessageBox::Save | QMessageBox::Discard); - msgBox.setDefaultButton(QMessageBox::Discard); - switch (msgBox.exec()) { - case QMessageBox::Save: { - QString selectedFilter; - QString fileNameSuggestion = ""; - bool first = true; - for (const SendCoinsRecipient &rcp : m_current_transaction->getRecipients()) { - if (!first) { - fileNameSuggestion.append(" - "); - } - QString labelOrAddress = rcp.label.isEmpty() ? rcp.address : rcp.label; - QString amount = BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), rcp.amount); - fileNameSuggestion.append(labelOrAddress + "-" + amount); - first = false; - } - fileNameSuggestion.append(".psbt"); - QString filename = GUIUtil::getSaveFileName(this, - tr("Save Transaction Data"), fileNameSuggestion, - //: Expanded name of the binary PSBT file format. See: BIP 174. - tr("Partially Signed Transaction (Binary)") + QLatin1String(" (*.psbt)"), &selectedFilter); - if (filename.isEmpty()) { - return; - } - std::ofstream out{filename.toLocal8Bit().data(), std::ofstream::out | std::ofstream::binary}; - out << ssTx.str(); - out.close(); - Q_EMIT message(tr("PSBT saved"), "PSBT saved to disk", CClientUIInterface::MSG_INFORMATION); - break; - } - case QMessageBox::Discard: - break; - default: - assert(false); - } // msgBox.exec() + presentPSBT(psbtx); } else { assert(!model->wallet().privateKeysDisabled()); // now send the prepared transaction diff --git a/src/qt/sendcoinsdialog.h b/src/qt/sendcoinsdialog.h index 4a16702756..1e3284fb08 100644 --- a/src/qt/sendcoinsdialog.h +++ b/src/qt/sendcoinsdialog.h @@ -70,6 +70,8 @@ public Q_SLOTS: bool fFeeMinimized; const PlatformStyle *platformStyle; + // Copy PSBT to clipboard and offer to save it. + void presentPSBT(PartiallySignedTransaction& psbt); // Process WalletModel::SendCoinsReturn and generate a pair consisting // of a message and message flags for use in Q_EMIT message(). // Additional parameter msgArg can be used via .arg(msgArg). From 4b5a6cd14967b8ec3cb525e4cb18628de6c15091 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 21 Feb 2022 13:16:05 +0100 Subject: [PATCH 2/3] refactor: helper function signWithExternalSigner() Does not change behavior. Review hint: git show --color-moved --color-moved-ws=allow-indentation-change --- src/qt/sendcoinsdialog.cpp | 56 ++++++++++++++++++++------------------ src/qt/sendcoinsdialog.h | 9 ++++++ 2 files changed, 38 insertions(+), 27 deletions(-) diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index b77fbb7b3b..b73032cb37 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -447,6 +447,34 @@ void SendCoinsDialog::presentPSBT(PartiallySignedTransaction& psbtx) } // msgBox.exec() } +bool SendCoinsDialog::signWithExternalSigner(PartiallySignedTransaction& psbtx, CMutableTransaction& mtx, bool& complete) { + TransactionError err; + try { + err = model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/true, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete); + } catch (const std::runtime_error& e) { + QMessageBox::critical(nullptr, tr("Sign failed"), e.what()); + return false; + } + if (err == TransactionError::EXTERNAL_SIGNER_NOT_FOUND) { + //: "External signer" means using devices such as hardware wallets. + QMessageBox::critical(nullptr, tr("External signer not found"), "External signer not found"); + return false; + } + if (err == TransactionError::EXTERNAL_SIGNER_FAILED) { + //: "External signer" means using devices such as hardware wallets. + QMessageBox::critical(nullptr, tr("External signer failure"), "External signer failure"); + return false; + } + if (err != TransactionError::OK) { + tfm::format(std::cerr, "Failed to sign PSBT"); + processSendCoinsReturn(WalletModel::TransactionCreationFailed); + return false; + } + // fillPSBT does not always properly finalize + complete = FinalizeAndExtractPSBT(psbtx, mtx); + return true; +} + void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked) { if(!model || !model->getOptionsModel()) @@ -479,33 +507,7 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked) assert(!complete); assert(err == TransactionError::OK); if (model->wallet().hasExternalSigner()) { - try { - err = model->wallet().fillPSBT(SIGHASH_ALL, true /* sign */, true /* bip32derivs */, nullptr, psbtx, complete); - } catch (const std::runtime_error& e) { - QMessageBox::critical(nullptr, tr("Sign failed"), e.what()); - send_failure = true; - return; - } - if (err == TransactionError::EXTERNAL_SIGNER_NOT_FOUND) { - //: "External signer" means using devices such as hardware wallets. - QMessageBox::critical(nullptr, tr("External signer not found"), "External signer not found"); - send_failure = true; - return; - } - if (err == TransactionError::EXTERNAL_SIGNER_FAILED) { - //: "External signer" means using devices such as hardware wallets. - QMessageBox::critical(nullptr, tr("External signer failure"), "External signer failure"); - send_failure = true; - return; - } - if (err != TransactionError::OK) { - tfm::format(std::cerr, "Failed to sign PSBT"); - processSendCoinsReturn(WalletModel::TransactionCreationFailed); - send_failure = true; - return; - } - // fillPSBT does not always properly finalize - complete = FinalizeAndExtractPSBT(psbtx, mtx); + send_failure = !signWithExternalSigner(psbtx, mtx, complete); } // Broadcast transaction if complete (even with an external signer this diff --git a/src/qt/sendcoinsdialog.h b/src/qt/sendcoinsdialog.h index 1e3284fb08..87f3fbc14c 100644 --- a/src/qt/sendcoinsdialog.h +++ b/src/qt/sendcoinsdialog.h @@ -79,6 +79,15 @@ public Q_SLOTS: void minimizeFeeSection(bool fMinimize); // Format confirmation message bool PrepareSendText(QString& question_string, QString& informative_text, QString& detailed_text); + /* Sign PSBT using external signer. + * + * @param[in,out] psbtx the PSBT to sign + * @param[in,out] mtx needed to attempt to finalize + * @param[in,out] complete whether the PSBT is complete (a successfully signed multisig transaction may not be complete) + * + * @returns false if any failure occurred, which may include the user rejection of a transaction on the device. + */ + bool signWithExternalSigner(PartiallySignedTransaction& psbt, CMutableTransaction& mtx, bool& complete); void updateFeeMinimizedLabel(); void updateCoinControlState(); From 2efdfb88aab6496dcf2b98e0de30635bc6bade85 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 21 Feb 2022 13:32:21 +0100 Subject: [PATCH 3/3] gui: restore Send for external signer Before this change the send confirmation dialog would keep the Send option disabled. The Create Unsigned choice would actually send. This is potentially confusing. With this change the Create Unsigned button will not attempt to sign and always produce a PSBT. The Send button will attempt to sign, and only return a PSBT if more signatures are needed. When using an external signer, the Create Unsigned option only appears when PSBT controls are enabled in the wallet settings. This commit maintains the pre-existing behavior of filling the PSBT (without signing) even when not using an external signer. Closes #551 Co-authored-by: Jon Atack --- src/qt/sendcoinsdialog.cpp | 64 ++++++++++++++++++++++---------------- src/qt/sendcoinsdialog.h | 2 ++ 2 files changed, 40 insertions(+), 26 deletions(-) diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index b73032cb37..c924789796 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -485,7 +485,9 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked) assert(m_current_transaction); const QString confirmation = tr("Confirm send coins"); - auto confirmationDialog = new SendConfirmationDialog(confirmation, question_string, informative_text, detailed_text, SEND_CONFIRM_DELAY, !model->wallet().privateKeysDisabled(), model->getOptionsModel()->getEnablePSBTControls(), this); + const bool enable_send{!model->wallet().privateKeysDisabled() || model->wallet().hasExternalSigner()}; + const bool always_show_unsigned{model->getOptionsModel()->getEnablePSBTControls()}; + auto confirmationDialog = new SendConfirmationDialog(confirmation, question_string, informative_text, detailed_text, SEND_CONFIRM_DELAY, enable_send, always_show_unsigned, this); confirmationDialog->setAttribute(Qt::WA_DeleteOnClose); // TODO: Replace QDialog::exec() with safer QDialog::show(). const auto retval = static_cast(confirmationDialog->exec()); @@ -498,23 +500,50 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked) bool send_failure = false; if (retval == QMessageBox::Save) { + // "Create Unsigned" clicked CMutableTransaction mtx = CMutableTransaction{*(m_current_transaction->getWtx())}; PartiallySignedTransaction psbtx(mtx); bool complete = false; - // Always fill without signing first. This prevents an external signer - // from being called prematurely and is not expensive. - TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, nullptr, psbtx, complete); + // Fill without signing + TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete); assert(!complete); assert(err == TransactionError::OK); + + // Copy PSBT to clipboard and offer to save + presentPSBT(psbtx); + } else { + // "Send" clicked + assert(!model->wallet().privateKeysDisabled() || model->wallet().hasExternalSigner()); + bool broadcast = true; if (model->wallet().hasExternalSigner()) { + CMutableTransaction mtx = CMutableTransaction{*(m_current_transaction->getWtx())}; + PartiallySignedTransaction psbtx(mtx); + bool complete = false; + // Always fill without signing first. This prevents an external signer + // from being called prematurely and is not expensive. + TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete); + assert(!complete); + assert(err == TransactionError::OK); send_failure = !signWithExternalSigner(psbtx, mtx, complete); + // Don't broadcast when user rejects it on the device or there's a failure: + broadcast = complete && !send_failure; + if (!send_failure) { + // A transaction signed with an external signer is not always complete, + // e.g. in a multisig wallet. + if (complete) { + // Prepare transaction for broadcast transaction if complete + const CTransactionRef tx = MakeTransactionRef(mtx); + m_current_transaction->setWtx(tx); + } else { + presentPSBT(psbtx); + } + } } - // Broadcast transaction if complete (even with an external signer this - // is not always the case, e.g. in a multisig wallet). - if (complete) { - const CTransactionRef tx = MakeTransactionRef(mtx); - m_current_transaction->setWtx(tx); + // Broadcast the transaction, unless an external signer was used and it + // failed, or more signatures are needed. + if (broadcast) { + // now send the prepared transaction WalletModel::SendCoinsReturn sendStatus = model->sendCoins(*m_current_transaction); // process sendStatus and on error generate message shown to user processSendCoinsReturn(sendStatus); @@ -524,23 +553,6 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked) } else { send_failure = true; } - return; - } - - // Copy PSBT to clipboard and offer to save - assert(!complete); - presentPSBT(psbtx); - } else { - assert(!model->wallet().privateKeysDisabled()); - // now send the prepared transaction - WalletModel::SendCoinsReturn sendStatus = model->sendCoins(*m_current_transaction); - // process sendStatus and on error generate message shown to user - processSendCoinsReturn(sendStatus); - - if (sendStatus.status == WalletModel::OK) { - Q_EMIT coinsSent(m_current_transaction->getWtx()->GetHash()); - } else { - send_failure = true; } } if (!send_failure) { diff --git a/src/qt/sendcoinsdialog.h b/src/qt/sendcoinsdialog.h index 87f3fbc14c..400503d0c0 100644 --- a/src/qt/sendcoinsdialog.h +++ b/src/qt/sendcoinsdialog.h @@ -128,6 +128,8 @@ class SendConfirmationDialog : public QMessageBox public: SendConfirmationDialog(const QString& title, const QString& text, const QString& informative_text = "", const QString& detailed_text = "", int secDelay = SEND_CONFIRM_DELAY, bool enable_send = true, bool always_show_unsigned = true, QWidget* parent = nullptr); + /* Returns QMessageBox::Cancel, QMessageBox::Yes when "Send" is + clicked and QMessageBox::Save when "Create Unsigned" is clicked. */ int exec() override; private Q_SLOTS: