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.
Sjors · May 16, 2024 · 02e62c6 · 02e62c6
1 parent 0d44c44
commit 02e62c6
Show file tree

Hide file tree

Showing 25 changed files with 156 additions and 97 deletions.
diff --git 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
@@ -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
@@ -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<common::PSBTError> fillPSBT(int sighash_type,
         bool sign,
         bool bip32derivs,
         size_t* n_signed,

diff --git 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
@@ -508,17 +508,17 @@ bool FinalizeAndExtractPSBT(PartiallySignedTransaction& psbtx, CMutableTransacti
     return true;
 }
 
-TransactionError CombinePSBTs(PartiallySignedTransaction& out, const std::vector<PartiallySignedTransaction>& psbtxs)
+bool CombinePSBTs(PartiallySignedTransaction& out, const std::vector<PartiallySignedTransaction>& 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
@@ -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<PartiallySignedTransaction>& psbtxs);
+[[nodiscard]] bool CombinePSBTs(PartiallySignedTransaction& out, const std::vector<PartiallySignedTransaction>& 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
@@ -13,6 +13,7 @@
 #include <qt/forms/ui_psbtoperationsdialog.h>
 #include <qt/guiutil.h>
 #include <qt/optionsmodel.h>
+#include <util/error.h>
 #include <util/fs.h>
 #include <util/strencodings.h>
 
@@ -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
@@ -37,6 +37,7 @@
 #include <QSettings>
 #include <QTextDocument>
 
+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<PSBTError> 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
@@ -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
@@ -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
@@ -7,6 +7,7 @@
 #include <clientversion.h>
 #include <core_io.h>
 #include <common/args.h>
+#include <common/types.h>
 #include <consensus/amount.h>
 #include <script/interpreter.h>
 #include <key_io.h>
@@ -30,6 +31,8 @@
 #include <tuple>
 #include <utility>
 
+using common::PSBTError;
+
 const std::string UNIX_EPOCH_TIME = "UNIX epoch time";
 const std::string EXAMPLE_ADDRESS[2] = {"bc1q09vm5lfy0j5reeulh4x5752q25uqqvz34hufdl", "bc1q02ad21edsxd23d32dfgqqsz4vv4nmtfzuklhy3"};
 
@@ -364,25 +367,35 @@ unsigned int ParseConfirmTarget(const UniValue& value, unsigned int max_target)
     return unsigned_target;
 }
 
+RPCErrorCode RPCErrorFromPSBTError(PSBTError err)
+{
+    switch (err) {
+        case PSBTError::UNSUPPORTED:
+            return RPC_INVALID_PARAMETER;
+        case PSBTError::SIGHASH_MISMATCH:
+            return RPC_DESERIALIZATION_ERROR;
+        default: break;
+    }
+    return RPC_TRANSACTION_ERROR;
+}
+
 RPCErrorCode RPCErrorFromTransactionError(TransactionError terr)
 {
     switch (terr) {
         case TransactionError::MEMPOOL_REJECTED:
             return RPC_TRANSACTION_REJECTED;
         case TransactionError::ALREADY_IN_CHAIN:
             return RPC_TRANSACTION_ALREADY_IN_CHAIN;
-        case TransactionError::P2P_DISABLED:
-            return RPC_CLIENT_P2P_DISABLED;
-        case TransactionError::INVALID_PSBT:
-        case TransactionError::PSBT_MISMATCH:
-            return RPC_INVALID_PARAMETER;
-        case TransactionError::SIGHASH_MISMATCH:
-            return RPC_DESERIALIZATION_ERROR;
         default: break;
     }
     return RPC_TRANSACTION_ERROR;
 }
 
+UniValue JSONRPCPSBTError(PSBTError err)
+{
+    return JSONRPCError(RPCErrorFromPSBTError(err), PSBTErrorString(err).original);
+}
+
 UniValue JSONRPCTransactionError(TransactionError terr, const std::string& err_string)
 {
     if (err_string.length() > 0) {

diff --git a/src/rpc/util.h b/src/rpc/util.h
@@ -37,6 +37,9 @@ enum class OutputType;
 enum class TransactionError;
 struct FlatSigningProvider;
 struct bilingual_str;
+namespace common {
+enum class PSBTError;
+} // namespace common
 
 static constexpr bool DEFAULT_RPC_DOC_CHECK{
 #ifdef RPC_DOC_CHECK
@@ -128,6 +131,7 @@ int ParseSighashString(const UniValue& sighash);
 unsigned int ParseConfirmTarget(const UniValue& value, unsigned int max_target);
 
 RPCErrorCode RPCErrorFromTransactionError(TransactionError terr);
+UniValue JSONRPCPSBTError(common::PSBTError err);
 UniValue JSONRPCTransactionError(TransactionError terr, const std::string& err_string = "");
 
 //! Parse a JSON range specified as int64, or [int64, int64]

diff --git a/src/test/fuzz/kitchen_sink.cpp b/src/test/fuzz/kitchen_sink.cpp
@@ -18,15 +18,10 @@
 
 namespace {
 constexpr TransactionError ALL_TRANSACTION_ERROR[] = {
-    TransactionError::OK,
     TransactionError::MISSING_INPUTS,
     TransactionError::ALREADY_IN_CHAIN,
-    TransactionError::P2P_DISABLED,
     TransactionError::MEMPOOL_REJECTED,
     TransactionError::MEMPOOL_ERROR,
-    TransactionError::INVALID_PSBT,
-    TransactionError::PSBT_MISMATCH,
-    TransactionError::SIGHASH_MISMATCH,
     TransactionError::MAX_FEE_EXCEEDED,
 };
 }; // namespace

diff --git a/src/util/error.cpp b/src/util/error.cpp
@@ -4,12 +4,33 @@
 
 #include <util/error.h>
 
+#include <common/types.h>
 #include <tinyformat.h>
 #include <util/translation.h>
 
 #include <cassert>
 #include <string>
 
+using common::PSBTError;
+
+bilingual_str PSBTErrorString(PSBTError err)
+{
+    switch (err) {
+        case PSBTError::MISSING_INPUTS:
+            return Untranslated("Inputs missing or spent");
+        case PSBTError::SIGHASH_MISMATCH:
+            return Untranslated("Specified sighash value does not match value stored in PSBT");
+        case PSBTError::EXTERNAL_SIGNER_NOT_FOUND:
+            return Untranslated("External signer not found");
+        case PSBTError::EXTERNAL_SIGNER_FAILED:
+            return Untranslated("External signer failed to sign");
+        case PSBTError::UNSUPPORTED:
+            return Untranslated("Signer does not support PSBT");
+        // no default case, so the compiler can warn about missing cases
+    }
+    assert(false);
+}
+
 bilingual_str TransactionErrorString(const TransactionError err)
 {
     switch (err) {
@@ -19,26 +40,14 @@ bilingual_str TransactionErrorString(const TransactionError err)
             return Untranslated("Inputs missing or spent");
         case TransactionError::ALREADY_IN_CHAIN:
             return Untranslated("Transaction already in block chain");
-        case TransactionError::P2P_DISABLED:
-            return Untranslated("Peer-to-peer functionality missing or disabled");
         case TransactionError::MEMPOOL_REJECTED:
             return Untranslated("Transaction rejected by mempool");
         case TransactionError::MEMPOOL_ERROR:
             return Untranslated("Mempool internal error");
-        case TransactionError::INVALID_PSBT:
-            return Untranslated("PSBT is not well-formed");
-        case TransactionError::PSBT_MISMATCH:
-            return Untranslated("PSBTs not compatible (different transactions)");
-        case TransactionError::SIGHASH_MISMATCH:
-            return Untranslated("Specified sighash value does not match value stored in PSBT");
         case TransactionError::MAX_FEE_EXCEEDED:
             return Untranslated("Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)");
         case TransactionError::MAX_BURN_EXCEEDED:
             return Untranslated("Unspendable output exceeds maximum configured by user (maxburnamount)");
-        case TransactionError::EXTERNAL_SIGNER_NOT_FOUND:
-            return Untranslated("External signer not found");
-        case TransactionError::EXTERNAL_SIGNER_FAILED:
-            return Untranslated("External signer failed to sign");
         case TransactionError::INVALID_PACKAGE:
             return Untranslated("Transaction rejected due to invalid package");
         // no default case, so the compiler can warn about missing cases