[backport#15408] Remove unused TransactionError constants

Summary:
Fixup to #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).

bitcoin/bitcoin@fa9b60c

---

Depends on D6000

This is a backport of Core [[bitcoin/bitcoin#15408 | PR15408]] plus:

A few code cleanups in util/error.h/.cpp
Corrected a comment in validation.cpp (regtest no longer supporting non-standard transactions re: D5764)

Test Plan:
  cmake .. -GNinja -DENABLE_WERROR=ON
  ninja check-all
  cmake .. -GNinja -DENABLE_WERROR=ON -DBUILD_BITCOIN_WALLET=OFF
  ninja check-all

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Subscribers: deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6012

src/node/transaction.cpp

@@ -16,17 +16,13 @@
 
 #include <future>
 
-bool BroadcastTransaction(const Config &config, const CTransactionRef tx,
-                          TxId &txid, TransactionError &error,
-                          std::string &err_string, const bool allowhighfees) {
+TransactionError BroadcastTransaction(const Config &config,
+                                      const CTransactionRef tx, TxId &txid,
+                                      std::string &err_string,
+                                      const Amount highFee) {
     std::promise<void> promise;
     txid = tx->GetId();
 
-    Amount nMaxRawTxFee = maxTxFee;
-    if (allowhighfees) {
-        nMaxRawTxFee = Amount::zero();
-    }
-
     { // cs_main scope
         LOCK(cs_main);
         CCoinsViewCache &view = *pcoinsTip;
@@ -43,21 +39,18 @@ bool BroadcastTransaction(const Config &config, const CTransactionRef tx,
             bool fMissingInputs;
             if (!AcceptToMemoryPool(config, g_mempool, state, std::move(tx),
                                     &fMissingInputs, false /* bypass_limits */,
-                                    nMaxRawTxFee)) {
+                                    highFee)) {
                 if (state.IsInvalid()) {
                     err_string = FormatStateMessage(state);
-                    error = TransactionError::MEMPOOL_REJECTED;
-                    return false;
+                    return TransactionError::MEMPOOL_REJECTED;
                 }
 
                 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 of the new transaction prior to returning. This
@@ -69,8 +62,7 @@ bool BroadcastTransaction(const Config &config, const CTransactionRef tx,
                     [&promise] { promise.set_value(); });
             }
         } 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.
@@ -81,12 +73,11 @@ bool BroadcastTransaction(const Config &config, const CTransactionRef tx,
     promise.get_future().wait();
 
     if (!g_connman) {
-        error = TransactionError::P2P_DISABLED;
-        return false;
+        return TransactionError::P2P_DISABLED;
     }
 
     CInv inv(MSG_TX, txid);
     g_connman->ForEachNode([&inv](CNode *pnode) { pnode->PushInventory(inv); });
 
-    return true;
+    return TransactionError::OK;
 }

src/node/transaction.h

@@ -1,10 +1,11 @@
-// 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 <util/error.h>
 
@@ -16,14 +17,15 @@ struct TxId;
  *
  * @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[out] &err_string reference to std::string to fill with error string
+ * if available
+ * @param[in]  highfee Reject txs with fees higher than this (if 0, accept any
+ * fee)
+ * @return error
  */
-bool BroadcastTransaction(const Config &config, CTransactionRef tx, TxId &txid,
-                          TransactionError &error, std::string &err_string,
-                          bool allowhighfees = false);
+NODISCARD TransactionError BroadcastTransaction(const Config &config,
+                                                CTransactionRef tx, TxId &txid,
+                                                std::string &err_string,
+                                                Amount highfee);
 
 #endif // BITCOIN_NODE_TRANSACTION_H

src/psbt.cpp

@@ -208,22 +208,21 @@ bool FinalizeAndExtractPSBT(PartiallySignedTransaction &psbtx,
     return true;
 }
 
-bool CombinePSBTs(PartiallySignedTransaction &out, TransactionError &error,
-                  const std::vector<PartiallySignedTransaction> &psbtxs) {
+TransactionError
+CombinePSBTs(PartiallySignedTransaction &out,
+             const std::vector<PartiallySignedTransaction> &psbtxs) {
     // Copy the first one
     out = psbtxs[0];
 
     // 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.
 
@@ -507,13 +507,12 @@ bool FinalizeAndExtractPSBT(PartiallySignedTransaction &psbtx,
  * 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

@@ -29,6 +29,7 @@
 #include <script/standard.h>
 #include <txmempool.h>
 #include <uint256.h>
+#include <util/error.h>
 #include <util/strencodings.h>
 #include <validation.h>
 #include <validationinterface.h>
@@ -973,12 +974,12 @@ static UniValue sendrawtransaction(const Config &config,
     if (!request.params[1].isNull()) {
         allowhighfees = request.params[1].get_bool();
     }
-
+    const Amount highfee{allowhighfees ? Amount::zero() : ::maxTxFee};
     TxId txid;
-    TransactionError err;
     std::string err_string;
-    if (!BroadcastTransaction(config, tx, txid, err, err_string,
-                              allowhighfees)) {
+    const TransactionError err =
+        BroadcastTransaction(config, tx, txid, err_string, highfee);
+    if (err != TransactionError::OK) {
         throw JSONRPCTransactionError(err, err_string);
     }
 
@@ -1434,8 +1435,8 @@ static UniValue combinepsbt(const Config &config,
     }
 
     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/util/error.cpp

@@ -6,8 +6,8 @@
 
 #include <util/system.h>
 
-const char *TransactionErrorString(const TransactionError err) {
-    switch (err) {
+std::string TransactionErrorString(const TransactionError error) {
+    switch (error) {
         case TransactionError::OK:
             return "No error";
         case TransactionError::MISSING_INPUTS:
@@ -26,18 +26,15 @@ 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";
-
-        default:
-            break;
-    }
-    return "Unknown error";
+    } // no default case, so the compiler can warn about missing cases
+    assert(false);
 }
 
 std::string AmountHighWarn(const std::string &optname) {
     return strprintf(_("%s is set very high!"), optname);
 }
 
-std::string AmountErrMsg(const char *const optname,
+std::string AmountErrMsg(const std::string &optname,
                          const std::string &strValue) {
     return strprintf(_("Invalid amount for -%s=<amount>: '%s'"), optname,
                      strValue);

src/util/error.h

@@ -18,7 +18,7 @@
 #include <string>
 
 enum class TransactionError {
-    OK = 0,
+    OK, //!< No error
 
     MISSING_INPUTS,
     ALREADY_IN_CHAIN,
@@ -28,17 +28,13 @@ 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(TransactionError error);
 
 std::string AmountHighWarn(const std::string &optname);
 
-std::string AmountErrMsg(const char *const optname,
+std::string AmountErrMsg(const std::string &optname,
                          const std::string &strValue);
 
 #endif // BITCOIN_UTIL_ERROR_H

src/wallet/psbtwallet.cpp

@@ -2,11 +2,13 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <util/error.h>
 #include <wallet/psbtwallet.h>
 
-bool FillPSBT(const CWallet *pwallet, PartiallySignedTransaction &psbtx,
-              TransactionError &error, bool &complete, SigHashType sighash_type,
-              bool sign, bool bip32derivs) {
+TransactionError FillPSBT(const CWallet *pwallet,
+                          PartiallySignedTransaction &psbtx, bool &complete,
+                          SigHashType sighash_type, bool sign,
+                          bool bip32derivs) {
     LOCK(pwallet->cs_wallet);
     // Get all of the previous transactions
     complete = true;
@@ -20,8 +22,7 @@ bool FillPSBT(const CWallet *pwallet, PartiallySignedTransaction &psbtx,
 
         // Verify input looks sane.
         if (!input.IsSane()) {
-            error = TransactionError::INVALID_PSBT;
-            return false;
+            return TransactionError::INVALID_PSBT;
         }
 
         // If we have no utxo, grab it from the wallet.
@@ -39,8 +40,7 @@ bool FillPSBT(const CWallet *pwallet, PartiallySignedTransaction &psbtx,
         // Get the Sighash type
         if (sign && input.sighash_type.getRawSigHashType() > 0 &&
             input.sighash_type != sighash_type) {
-            error = TransactionError::SIGHASH_MISMATCH;
-            return false;
+            return TransactionError::SIGHASH_MISMATCH;
         }
 
         complete &=
@@ -64,5 +64,6 @@ bool FillPSBT(const CWallet *pwallet, PartiallySignedTransaction &psbtx,
                          creator, out.scriptPubKey, sigdata);
         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,17 +18,19 @@
  *
  * @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`)
+ * @param[in] bip32derivs whether to fill in bip32 derivation information if
+ * available
+ * @return error
  */
-bool FillPSBT(const CWallet *pwallet, PartiallySignedTransaction &psbtx,
-              TransactionError &error, bool &complete,
-              SigHashType sighash_type = SigHashType(), bool sign = true,
-              bool bip32derivs = false);
+
+NODISCARD TransactionError FillPSBT(const CWallet *pwallet,
+                                    PartiallySignedTransaction &psbtx,
+                                    bool &complete,
+                                    SigHashType sighash_type = SigHashType(),
+                                    bool sign = true, bool bip32derivs = false);
 
 #endif // BITCOIN_WALLET_PSBTWALLET_H

src/wallet/rpcwallet.cpp

@@ -23,6 +23,7 @@
 #include <rpc/util.h>
 #include <shutdown.h>
 #include <timedata.h>
+#include <util/error.h>
 #include <util/moneystr.h>
 #include <util/system.h>
 #include <util/url.h>
@@ -4613,9 +4614,9 @@ static UniValue walletprocesspsbt(const Config &config,
     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);
     }
 
@@ -4810,9 +4811,10 @@ static UniValue walletcreatefundedpsbt(const Config &config,
     bool bip32derivs =
         request.params[4].isNull() ? false : request.params[4].get_bool();
     bool complete = true;
-    TransactionError err;
-    if (!FillPSBT(pwallet, psbtx, err, complete, SigHashType().withForkId(),
-                  false, bip32derivs)) {
+    const TransactionError err =
+        FillPSBT(pwallet, psbtx, complete, SigHashType().withForkId(), false,
+                 bip32derivs);
+    if (err != TransactionError::OK) {
         throw JSONRPCTransactionError(err);
     }
 

src/wallet/test/psbt_wallet_tests.cpp

@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <key_io.h>
+#include <util/error.h>
 #include <util/strencodings.h>
 #include <wallet/psbtwallet.h>
 #include <wallet/test/wallet_test_fixture.h>
@@ -89,9 +90,10 @@ BOOST_AUTO_TEST_CASE(psbt_updater_test) {
     // The path missing comes from the HD masterkey.
 
     // Fill transaction with our data
-    TransactionError err;
     bool complete = true;
-    FillPSBT(&m_wallet, psbtx, err, complete, SigHashType(), false, true);
+    BOOST_REQUIRE_EQUAL(
+        FillPSBT(&m_wallet, psbtx, complete, SigHashType(), false, true),
+        TransactionError::OK);
 
     // Get the final tx
     CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);