Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore Send button when using external signer #555

Merged
merged 3 commits into from
Mar 17, 2022
Merged

Restore Send button when using external signer #555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
026b5b4
move-only: helper function to present PSBT
Sjors Feb 21, 2022
4b5a6cd
refactor: helper function signWithExternalSigner()
Sjors Feb 21, 2022
2efdfb8
gui: restore Send for external signer
Sjors Feb 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
205 changes: 112 additions & 93 deletions src/qt/sendcoinsdialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,80 @@ 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()
}

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())
Expand All @@ -411,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<QMessageBox::StandardButton>(confirmationDialog->exec());
Expand All @@ -424,49 +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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e1c1c61 claims "Does not change behavior.", yet this is no longer done for wallets without an external signer.

Copy link
Member Author

@Sjors Sjors Mar 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I moved this out of the helper function.

It leads to some duplication in 0538614, but it's probably better to preserve the behavior. It might even useful, though I'm a bit rusty on what PSBT filling does on top of what the wallet already does.

Copy link
Member

@jonatack jonatack Mar 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as a potential follow-up it looks like this code block could be potentially de-duped by extracting to a helper method that returns psbtx and mtx.

        CMutableTransaction mtx = CMutableTransaction{*(m_current_transaction->getWtx())};
        PartiallySignedTransaction psbtx(mtx);
        bool complete = false;
        TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete);
        assert(!complete);
        assert(err == TransactionError::OK);

(and TransactionError err could be const)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to avoid touching this now, but I agree it would be good to move into a helper.

// 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()) {
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;
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);
}
}
// fillPSBT does not always properly finalize
complete = FinalizeAndExtractPSBT(psbtx, mtx);
}

// 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);
Expand All @@ -476,64 +553,6 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
} else {
send_failure = true;
}
return;
}

// 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()
} 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) {
Expand Down
13 changes: 13 additions & 0 deletions src/qt/sendcoinsdialog.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,24 @@ 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).
void processSendCoinsReturn(const WalletModel::SendCoinsReturn &sendCoinsReturn, const QString &msgArg = QString());
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();

Expand Down Expand Up @@ -117,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:
Expand Down