From 02e62c6c9af4beabaeea58fb1ea3ad0dc5094678 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 13 Dec 2023 11:43:16 -0500 Subject: [PATCH] common: Add PSBTError enum Add separate PSBTError enum instead of reusing TransactionError enum for PSBT operations, and drop unused error codes. The error codes returned by PSBT operations and transaction broadcast functions mostly do not overlap, so using an unified enum makes it harder to call any of these functions and know which errors actually need to be handled. Define PSBTError in the common library because PSBT functionality is implemented in the common library and used by both the node (for rawtransaction RPCs) and the wallet. --- src/Makefile.am | 1 + src/common/types.h | 26 +++++++++++++++ src/interfaces/wallet.h | 6 ++-- src/node/types.h | 6 ---- src/psbt.cpp | 6 ++-- src/psbt.h | 4 +-- src/qt/psbtoperationsdialog.cpp | 17 +++++----- src/qt/sendcoinsdialog.cpp | 17 +++++----- src/qt/walletmodel.cpp | 4 +-- src/rpc/rawtransaction.cpp | 5 ++- src/rpc/util.cpp | 27 +++++++++++---- src/rpc/util.h | 4 +++ src/test/fuzz/kitchen_sink.cpp | 5 --- src/util/error.cpp | 33 ++++++++++++------- src/util/error.h | 5 +++ .../external_signer_scriptpubkeyman.cpp | 10 +++--- src/wallet/external_signer_scriptpubkeyman.h | 2 +- src/wallet/feebumper.cpp | 4 +-- src/wallet/interfaces.cpp | 3 +- src/wallet/rpc/spend.cpp | 22 ++++++------- src/wallet/scriptpubkeyman.cpp | 18 +++++----- src/wallet/scriptpubkeyman.h | 7 ++-- src/wallet/test/psbt_wallet_tests.cpp | 4 +-- src/wallet/wallet.cpp | 11 ++++--- src/wallet/wallet.h | 6 ++-- 25 files changed, 156 insertions(+), 97 deletions(-) create mode 100644 src/common/types.h diff --git a/src/Makefile.am b/src/Makefile.am index c7b193daafd35..36b9102712787 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -137,6 +137,7 @@ BITCOIN_CORE_H = \ common/bloom.h \ common/init.h \ common/run_command.h \ + common/types.h \ common/url.h \ compat/assumptions.h \ compat/byteswap.h \ diff --git a/src/common/types.h b/src/common/types.h new file mode 100644 index 0000000000000..0d9cb67ce9444 --- /dev/null +++ b/src/common/types.h @@ -0,0 +1,26 @@ +// Copyright (c) 2010-2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +//! @file common/types.h is a home for simple enum and struct type definitions +//! that can be used internally by functions in the libbitcoin_common library, +//! but also used externally by node, wallet, and GUI code. +//! +//! This file is intended to define only simple types that do not have external +//! dependencies. More complicated types should be defined in dedicated header +//! files. + +#ifndef BITCOIN_COMMON_TYPES_H +#define BITCOIN_COMMON_TYPES_H + +namespace common { +enum class PSBTError { + MISSING_INPUTS, + SIGHASH_MISMATCH, + EXTERNAL_SIGNER_NOT_FOUND, + EXTERNAL_SIGNER_FAILED, + UNSUPPORTED, +}; +} // namespace common + +#endif // BITCOIN_COMMON_TYPES_H diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index f1bfffa7d465f..1c916aacb0add 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -30,9 +30,11 @@ class CFeeRate; class CKey; enum class FeeReason; enum class OutputType; -enum class TransactionError; struct PartiallySignedTransaction; struct bilingual_str; +namespace common { +enum class PSBTError; +} // namespace common namespace wallet { class CCoinControl; class CWallet; @@ -202,7 +204,7 @@ class Wallet int& num_blocks) = 0; //! Fill PSBT. - virtual TransactionError fillPSBT(int sighash_type, + virtual std::optional fillPSBT(int sighash_type, bool sign, bool bip32derivs, size_t* n_signed, diff --git a/src/node/types.h b/src/node/types.h index 7febc45a2b4ff..9dd2edebb5340 100644 --- a/src/node/types.h +++ b/src/node/types.h @@ -17,16 +17,10 @@ enum class TransactionError { OK, //!< No error MISSING_INPUTS, ALREADY_IN_CHAIN, - P2P_DISABLED, MEMPOOL_REJECTED, MEMPOOL_ERROR, - INVALID_PSBT, - PSBT_MISMATCH, - SIGHASH_MISMATCH, MAX_FEE_EXCEEDED, MAX_BURN_EXCEEDED, - EXTERNAL_SIGNER_NOT_FOUND, - EXTERNAL_SIGNER_FAILED, INVALID_PACKAGE, }; diff --git a/src/psbt.cpp b/src/psbt.cpp index b2ee3ce7a5ea1..9e59b52b41d01 100644 --- a/src/psbt.cpp +++ b/src/psbt.cpp @@ -508,17 +508,17 @@ bool FinalizeAndExtractPSBT(PartiallySignedTransaction& psbtx, CMutableTransacti return true; } -TransactionError CombinePSBTs(PartiallySignedTransaction& out, const std::vector& psbtxs) +bool CombinePSBTs(PartiallySignedTransaction& out, const std::vector& psbtxs) { out = psbtxs[0]; // Copy the first one // Merge for (auto it = std::next(psbtxs.begin()); it != psbtxs.end(); ++it) { if (!out.Merge(*it)) { - return TransactionError::PSBT_MISMATCH; + return false; } } - return TransactionError::OK; + return true; } std::string PSBTRoleName(PSBTRole role) { diff --git a/src/psbt.h b/src/psbt.h index 3f7408371707c..95f6fdf91a711 100644 --- a/src/psbt.h +++ b/src/psbt.h @@ -1263,9 +1263,9 @@ bool FinalizeAndExtractPSBT(PartiallySignedTransaction& psbtx, CMutableTransacti * * @param[out] out the combined PSBT, if successful * @param[in] psbtxs the PSBTs to combine - * @return error (OK if we successfully combined the transactions, other error if they were not compatible) + * @return True if we successfully combined the transactions, false if they were not compatible */ -[[nodiscard]] TransactionError CombinePSBTs(PartiallySignedTransaction& out, const std::vector& psbtxs); +[[nodiscard]] bool CombinePSBTs(PartiallySignedTransaction& out, const std::vector& psbtxs); //! Decode a base64ed PSBT into a PartiallySignedTransaction [[nodiscard]] bool DecodeBase64PSBT(PartiallySignedTransaction& decoded_psbt, const std::string& base64_psbt, std::string& error); diff --git a/src/qt/psbtoperationsdialog.cpp b/src/qt/psbtoperationsdialog.cpp index 353709c7f5d2c..7bbebec46c76b 100644 --- a/src/qt/psbtoperationsdialog.cpp +++ b/src/qt/psbtoperationsdialog.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -55,10 +56,10 @@ void PSBTOperationsDialog::openWithPSBT(PartiallySignedTransaction psbtx) bool complete = FinalizePSBT(psbtx); // Make sure all existing signatures are fully combined before checking for completeness. if (m_wallet_model) { size_t n_could_sign; - TransactionError err = m_wallet_model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, &n_could_sign, m_transaction_data, complete); - if (err != TransactionError::OK) { + const auto err{m_wallet_model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, &n_could_sign, m_transaction_data, complete)}; + if (err) { showStatus(tr("Failed to load transaction: %1") - .arg(QString::fromStdString(TransactionErrorString(err).translated)), + .arg(QString::fromStdString(PSBTErrorString(*err).translated)), StatusLevel::ERR); return; } @@ -79,11 +80,11 @@ void PSBTOperationsDialog::signTransaction() WalletModel::UnlockContext ctx(m_wallet_model->requestUnlock()); - TransactionError err = m_wallet_model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/true, /*bip32derivs=*/true, &n_signed, m_transaction_data, complete); + const auto err{m_wallet_model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/true, /*bip32derivs=*/true, &n_signed, m_transaction_data, complete)}; - if (err != TransactionError::OK) { + if (err) { showStatus(tr("Failed to sign transaction: %1") - .arg(QString::fromStdString(TransactionErrorString(err).translated)), StatusLevel::ERR); + .arg(QString::fromStdString(PSBTErrorString(*err).translated)), StatusLevel::ERR); return; } @@ -247,9 +248,9 @@ size_t PSBTOperationsDialog::couldSignInputs(const PartiallySignedTransaction &p size_t n_signed; bool complete; - TransactionError err = m_wallet_model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/false, &n_signed, m_transaction_data, complete); + const auto err{m_wallet_model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/false, &n_signed, m_transaction_data, complete)}; - if (err != TransactionError::OK) { + if (err) { return 0; } return n_signed; diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index 0d8c0f7a636b4..401c79f007a7f 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -37,6 +37,7 @@ #include #include +using common::PSBTError; using wallet::CCoinControl; using wallet::DEFAULT_PAY_TX_FEE; @@ -442,26 +443,26 @@ void SendCoinsDialog::presentPSBT(PartiallySignedTransaction& psbtx) } bool SendCoinsDialog::signWithExternalSigner(PartiallySignedTransaction& psbtx, CMutableTransaction& mtx, bool& complete) { - TransactionError err; + std::optional 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) { + if (err == PSBTError::EXTERNAL_SIGNER_NOT_FOUND) { //: "External signer" means using devices such as hardware wallets. const QString msg = tr("External signer not found"); QMessageBox::critical(nullptr, msg, msg); return false; } - if (err == TransactionError::EXTERNAL_SIGNER_FAILED) { + if (err == PSBTError::EXTERNAL_SIGNER_FAILED) { //: "External signer" means using devices such as hardware wallets. const QString msg = tr("External signer failure"); QMessageBox::critical(nullptr, msg, msg); return false; } - if (err != TransactionError::OK) { + if (err) { tfm::format(std::cerr, "Failed to sign PSBT"); processSendCoinsReturn(WalletModel::TransactionCreationFailed); return false; @@ -501,9 +502,9 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked) PartiallySignedTransaction psbtx(mtx); bool complete = false; // Fill without signing - TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete); + const auto err{model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete)}; assert(!complete); - assert(err == TransactionError::OK); + assert(!err); // Copy PSBT to clipboard and offer to save presentPSBT(psbtx); @@ -517,9 +518,9 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked) 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); + const auto err{model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete)}; assert(!complete); - assert(err == TransactionError::OK); + assert(!err); 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; diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 87ad98a4cc33c..f7843706e0767 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -534,8 +534,8 @@ bool WalletModel::bumpFee(uint256 hash, uint256& new_hash) // "Create Unsigned" clicked PartiallySignedTransaction psbtx(mtx); bool complete = false; - const TransactionError err = wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, nullptr, psbtx, complete); - if (err != TransactionError::OK || complete) { + const auto err{wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, nullptr, psbtx, complete)}; + if (err || complete) { QMessageBox::critical(nullptr, tr("Fee bump error"), tr("Can't draft transaction.")); return false; } diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 634be2f7fb9d6..674f9bfe90cfd 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1485,9 +1485,8 @@ static RPCHelpMan combinepsbt() } PartiallySignedTransaction merged_psbt; - const TransactionError error = CombinePSBTs(merged_psbt, psbtxs); - if (error != TransactionError::OK) { - throw JSONRPCTransactionError(error); + if (!CombinePSBTs(merged_psbt, psbtxs)) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "PSBTs not compatible (different transactions)"); } DataStream ssTx{}; diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index f5a2e9eb6392c..9ae6dd98e9723 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include