Merge bitcoin#15408: Remove unused TransactionError constants

fa9b60c Remove unused TransactionError constants (MarcoFalke)

Pull request description:

  Fixup to bitcoin#14978, which introduced a bunch of unused enum values, such as `UNKNOWN_ERROR`, `ERROR_COUNT` and `TRANSACTION_ERR_LAST`. None of those have a meaning in the context of an `enum class`, where the compiler can infer if all cases have been covered in a switch-case.

  Also, move the global `::maxTxFee` back to the rpc caller, so it can be set on a per call basis (in the future).

Tree-SHA512: 7f1e2d795f1c1278ecd54ddab2b92c2a862f3c637b482d1d008208925befa1c9dd4b3c4bb1bfcbc5ca4b66a41004aaf01ea96ea95236f944250b8a6cf99ff173

src/node/transaction.cpp

@@ -13,7 +13,7 @@
 
 #include <future>
 
-const char* TransactionErrorString(const TransactionError err)
+std::string TransactionErrorString(const TransactionError err)
 {
     switch (err) {
         case TransactionError::OK:
@@ -34,22 +34,16 @@ const char* TransactionErrorString(const TransactionError err)
             return "PSBTs not compatible (different transactions)";
         case TransactionError::SIGHASH_MISMATCH:
             return "Specified sighash value does not match existing value";
-
-        case TransactionError::UNKNOWN_ERROR:
-        default: break;
+        // no default case, so the compiler can warn about missing cases
     }
-    return "Unknown error";
+    assert(false);
 }
 
-bool BroadcastTransaction(const CTransactionRef tx, uint256& hashTx, TransactionError& error, std::string& err_string, const bool allowhighfees, const bool bypass_limits)
+TransactionError BroadcastTransaction(const CTransactionRef tx, uint256& hashTx, std::string& err_string, const CAmount& highfee)
 {
     std::promise<void> promise;
     hashTx = tx->GetHash();
 
-    CAmount nMaxRawTxFee = maxTxFee;
-    if (allowhighfees)
-        nMaxRawTxFee = 0;
-
     { // cs_main scope
     LOCK(cs_main);
     CCoinsViewCache &view = *pcoinsTip;
@@ -64,19 +58,16 @@ bool BroadcastTransaction(const CTransactionRef tx, uint256& hashTx, Transaction
         CValidationState state;
         bool fMissingInputs;
         if (!AcceptToMemoryPool(mempool, state, std::move(tx), &fMissingInputs,
-                                bypass_limits, nMaxRawTxFee)) {
+                                bypass_limits, highfee)) {
             if (state.IsInvalid()) {
                 err_string = FormatStateMessage(state);
-                error = TransactionError::MEMPOOL_REJECTED;
-                return false;
+                return TransactionError::MEMPOOL_REJECTED;
             } else {
                 if (fMissingInputs) {
-                    error = TransactionError::MISSING_INPUTS;
-                    return false;
+                    return TransactionError::MISSING_INPUTS;
                 }
                 err_string = FormatStateMessage(state);
-                error = TransactionError::MEMPOOL_ERROR;
-                return false;
+                return TransactionError::MEMPOOL_ERROR;
             }
         } else {
             // If wallet is enabled, ensure that the wallet has been made aware
@@ -89,8 +80,7 @@ bool BroadcastTransaction(const CTransactionRef tx, uint256& hashTx, Transaction
             });
         }
     } else if (fHaveChain) {
-        error = TransactionError::ALREADY_IN_CHAIN;
-        return false;
+        return TransactionError::ALREADY_IN_CHAIN;
     } else {
         // Make sure we don't block forever if re-sending
         // a transaction already in mempool.
@@ -101,12 +91,11 @@ bool BroadcastTransaction(const CTransactionRef tx, uint256& hashTx, Transaction
 
     promise.get_future().wait();
 
-    if(!g_connman) {
-        error = TransactionError::P2P_DISABLED;
-        return false;
+    if (!g_connman) {
+        return TransactionError::P2P_DISABLED;
     }
 
     g_connman->RelayTransaction(*tx);
 
-    return true;
-    }
+    return TransactionError::OK;
+}

src/node/transaction.h

@@ -1,17 +1,16 @@
-// Copyright (c) 2017-2018 The Bitcoin Core developers
+// Copyright (c) 2017-2019 The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #ifndef BITCOIN_NODE_TRANSACTION_H
 #define BITCOIN_NODE_TRANSACTION_H
 
+#include <attributes.h>
 #include <primitives/transaction.h>
 #include <uint256.h>
 
 enum class TransactionError {
-    OK = 0,
-    UNKNOWN_ERROR,
-
+    OK, //!< No error
     MISSING_INPUTS,
     ALREADY_IN_CHAIN,
     P2P_DISABLED,
@@ -20,24 +19,19 @@ enum class TransactionError {
     INVALID_PSBT,
     PSBT_MISMATCH,
     SIGHASH_MISMATCH,
-
-    ERROR_COUNT
 };
 
-#define TRANSACTION_ERR_LAST TransactionError::ERROR_COUNT
-
-const char* TransactionErrorString(const TransactionError error);
+std::string TransactionErrorString(const TransactionError error);
 
 /**
  * Broadcast a transaction
  *
  * @param[in]  tx the transaction to broadcast
  * @param[out] &txid the txid of the transaction, if successfully broadcast
- * @param[out] &error reference to UniValue to fill with error info on failure
  * @param[out] &err_string reference to std::string to fill with error string if available
- * @param[in]  allowhighfees whether to allow fees exceeding maxTxFee
- * return true on success, false on error (and fills in `error`)
+ * @param[in]  highfee Reject txs with fees higher than this (if 0, accept any fee)
+ * return error
  */
-bool BroadcastTransaction(const CTransactionRef tx, uint256& txid, TransactionError& error, std::string& err_string, const bool allowhighfees = false, const bool bypass_limits = false);
+NODISCARD TransactionError BroadcastTransaction(CTransactionRef tx, uint256& txid, std::string& err_string, const CAmount& highfee);
 
 #endif // BITCOIN_NODE_TRANSACTION_H

src/psbt.cpp

@@ -220,21 +220,19 @@ bool FinalizeAndExtractPSBT(PartiallySignedTransaction& psbtx, CMutableTransacti
     return true;
 }
 
-bool CombinePSBTs(PartiallySignedTransaction& out, TransactionError& error, const std::vector<PartiallySignedTransaction>& psbtxs)
+TransactionError 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)) {
-            error = TransactionError::PSBT_MISMATCH;
-            return false;
+            return TransactionError::PSBT_MISMATCH;
         }
     }
     if (!out.IsSane()) {
-        error = TransactionError::INVALID_PSBT;
-        return false;
+        return TransactionError::INVALID_PSBT;
     }
 
-    return true;
+    return TransactionError::OK;
 }

src/psbt.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2009-2018 The Bitcoin Core developers
+// Copyright (c) 2009-2019 The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -497,10 +497,9 @@ bool FinalizeAndExtractPSBT(PartiallySignedTransaction& psbtx, CMutableTransacti
  * Combines PSBTs with the same underlying transaction, resulting in a single PSBT with all partial signatures from each input.
  *
  * @param[out] &out   the combined PSBT, if successful
- * @param[out] &error reference to TransactionError to fill with error info on failure
  * @param[in]  psbtxs the PSBTs to combine
- * @return True if we successfully combined the transactions, false if they were not compatible
+ * @return error (OK if we successfully combined the transactions, other error if they were not compatible)
  */
-bool CombinePSBTs(PartiallySignedTransaction& out, TransactionError& error, const std::vector<PartiallySignedTransaction>& psbtxs);
+NODISCARD TransactionError CombinePSBTs(PartiallySignedTransaction& out, const std::vector<PartiallySignedTransaction>& psbtxs);
 
 #endif // BITCOIN_PSBT_H

src/rpc/rawtransaction.cpp

@@ -1047,10 +1047,11 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
     if (!request.params[1].isNull()) allowhighfees = request.params[1].get_bool();
     bool bypass_limits = false;
     if (!request.params[3].isNull()) bypass_limits = request.params[3].get_bool();
+    const CAmount highfee{allowhighfees ? 0 : ::maxTxFee};
     uint256 txid;
-    TransactionError err;
     std::string err_string;
-    if (!BroadcastTransaction(tx, txid, err, err_string, allowhighfees, bypass_limits)) {
+    const TransactionError err = BroadcastTransaction(tx, txid, err_string, highfee);
+    if (TransactionError::OK != err) {
         throw JSONRPCTransactionError(err, err_string);
     }
 
@@ -1427,8 +1428,8 @@ UniValue combinepsbt(const JSONRPCRequest& request)
     }
 
     PartiallySignedTransaction merged_psbt;
-    TransactionError error;
-    if (!CombinePSBTs(merged_psbt, error, psbtxs)) {
+    const TransactionError error = CombinePSBTs(merged_psbt, psbtxs);
+    if (error != TransactionError::OK) {
         throw JSONRPCTransactionError(error);
     }
 

src/wallet/psbtwallet.cpp

@@ -4,7 +4,7 @@
 
 #include <wallet/psbtwallet.h>
 
-bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, TransactionError& error, bool& complete, int sighash_type, bool sign, bool bip32derivs)
+TransactionError FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, bool& complete, int sighash_type, bool sign, bool bip32derivs)
 {
     LOCK(pwallet->cs_wallet);
     // Get all of the previous transactions
@@ -13,19 +13,28 @@ bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, Transac
         const CTxIn& txin = psbtx.tx->vin[i];
         PSBTInput& input = psbtx.inputs.at(i);
 
-        const uint256& txhash = txin.prevout.hash;
-        const auto it = pwallet->mapWallet.find(txhash);
-        if (it != pwallet->mapWallet.end()) {
-            const CWalletTx& wtx = it->second;
-            // We only need the non_witness_utxo, which is a superset of the witness_utxo.
-            //   The signing code will switch to the smaller witness_utxo if this is ok.
-            input.non_witness_utxo = wtx.tx;
+        if (PSBTInputSigned(input)) {
+            continue;
+        }
+
+        // Verify input looks sane. This will check that we have at most one uxto, witness or non-witness.
+        if (!input.IsSane()) {
+            return TransactionError::INVALID_PSBT;
+        }
+
+            const uint256& txhash = txin.prevout.hash;
+            const auto it = pwallet->mapWallet.find(txhash);
+            if (it != pwallet->mapWallet.end()) {
+                const CWalletTx& wtx = it->second;
+                // We only need the non_witness_utxo, which is a superset of the witness_utxo.
+                //   The signing code will switch to the smaller witness_utxo if this is ok.
+                input.non_witness_utxo = wtx.tx;
+            }
         }
 
         // Get the Sighash type
         if (sign && input.sighash_type > 0 && input.sighash_type != sighash_type) {
-            error = TransactionError::SIGHASH_MISMATCH;
-            return false;
+            return TransactionError::SIGHASH_MISMATCH;
         }
 
         complete &= SignPSBTInput(HidingSigningProvider(pwallet, !sign, !bip32derivs), *psbtx.tx, input, i, sighash_type);
@@ -45,6 +54,6 @@ bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, Transac
         psbt_out.FromSignatureData(sigdata);
     }
 
-    return true;
+    return TransactionError::OK;
 }
 

src/wallet/psbtwallet.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2009-2018 The Bitcoin Core developers
+// Copyright (c) 2009-2019 The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -18,16 +18,14 @@
  *
  * @param[in]  pwallet pointer to a wallet
  * @param[in]  &psbtx reference to PartiallySignedTransaction to fill in
- * @param[out] &error reference to UniValue to fill with error info on failure
  * @param[out] &complete indicates whether the PSBT is now complete
  * @param[in]  sighash_type the sighash type to use when signing (if PSBT does not specify)
  * @param[in]  sign whether to sign or not
  * @param[in]  bip32derivs whether to fill in bip32 derivation information if available
- * return true on success, false on error (and fills in `error`)
+ * return error
  */
-bool FillPSBT(const CWallet* pwallet,
+NODISCARD TransactionError FillPSBT(const CWallet* pwallet,
               PartiallySignedTransaction& psbtx,
-              TransactionError& error,
               bool& complete,
               int sighash_type = 1 /* SIGHASH_ALL */,
               bool sign = true,

src/wallet/rpcwallet.cpp

@@ -4542,8 +4542,8 @@ UniValue walletprocesspsbt(const JSONRPCRequest& request)
     bool sign = request.params[1].isNull() ? true : request.params[1].get_bool();
     bool bip32derivs = request.params[3].isNull() ? false : request.params[3].get_bool();
     bool complete = true;
-    TransactionError err;
-    if (!FillPSBT(pwallet, psbtx, err, complete, nHashType, sign, bip32derivs)) {
+    const TransactionError err = FillPSBT(pwallet, psbtx, complete, nHashType, sign, bip32derivs);
+    if (err != TransactionError::OK) {
         throw JSONRPCTransactionError(err);
     }
 
@@ -4701,8 +4701,8 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
     // Fill transaction with out data but don't sign
     bool bip32derivs = request.params[4].isNull() ? false : request.params[4].get_bool();
     bool complete = true;
-    TransactionError err;
-    if (!FillPSBT(pwallet, psbtx, err, complete, 1, false, bip32derivs)) {
+    const TransactionError err = FillPSBT(pwallet, psbtx, complete, 1, false, bip32derivs);
+    if (err != TransactionError::OK) {
         throw JSONRPCTransactionError(err);
     }
 

src/wallet/test/psbt_wallet_tests.cpp

@@ -59,9 +59,8 @@ BOOST_AUTO_TEST_CASE(psbt_updater_test)
     const CTransaction txConst(*psbtx.tx);
 
     // Fill transaction with our data
-    TransactionError err;
     bool complete = true;
-    FillPSBT(&m_wallet, psbtx, err, complete, false, true);
+    BOOST_REQUIRE_EQUAL(TransactionError::OK, FillPSBT(&m_wallet, psbtx, complete, false, true));
 
     // Get the final tx
     CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);