[wallet] Change feebumper from class to functions

Change feebumper from a stateful class into a namespace of stateless
functions.

Having the results of feebumper calls persist in an object makes process
separation between Qt and wallet awkward, because it means the feebumper object
either has to be serialized back and forth between Qt and wallet processes
between fee bump calls, or that the feebumper object needs to stay alive in the
wallet process with an object reference passed back to Qt. It's simpler just to
have fee bumper calls return their results immediately instead of storing them
in an object with an extended lifetime.

In addition to making feebumper stateless, also:

- Move LOCK calls from Qt code to feebumper
- Move TransactionCanBeBumped implementation from Qt code to feebumper

src/qt/walletmodel.cpp

@@ -659,45 +659,39 @@ bool WalletModel::abandonTransaction(uint256 hash) const
 
 bool WalletModel::transactionCanBeBumped(uint256 hash) const
 {
-    LOCK2(cs_main, wallet->cs_wallet);
+    return feebumper::TransactionCanBeBumped(wallet, hash);
-    const CWalletTx *wtx = wallet->GetWalletTx(hash);
-    return wtx && SignalsOptInRBF(*(wtx->tx)) && !wtx->mapValue.count("replaced_by_txid");
 }
 
 bool WalletModel::bumpFee(uint256 hash)
 {
-    std::unique_ptr<feebumper::FeeBumper> feeBump;
-    {
     CCoinControl coin_control;
     coin_control.signalRbf = true;
-        LOCK2(cs_main, wallet->cs_wallet);
+    std::vector<std::string> errors;
-        feeBump.reset(new feebumper::FeeBumper(wallet, hash, coin_control, 0));
+    CAmount old_fee;
-    }
+    CAmount new_fee;
-    if (feeBump->getResult() != feebumper::Result::OK)
+    CMutableTransaction mtx;
-    {
+    if (feebumper::CreateTransaction(wallet, hash, coin_control, 0 /* totalFee */, errors, old_fee, new_fee, mtx) != feebumper::Result::OK) {
         QMessageBox::critical(0, tr("Fee bump error"), tr("Increasing transaction fee failed") + "<br />(" +
-            (feeBump->getErrors().size() ? QString::fromStdString(feeBump->getErrors()[0]) : "") +")");
+            (errors.size() ? QString::fromStdString(errors[0]) : "") +")");
          return false;
     }
 
     // allow a user based fee verification
     QString questionString = tr("Do you want to increase the fee?");
     questionString.append("<br />");
-    CAmount oldFee = feeBump->getOldFee();
-    CAmount newFee = feeBump->getNewFee();
     questionString.append("<table style=\"text-align: left;\">");
     questionString.append("<tr><td>");
     questionString.append(tr("Current fee:"));
     questionString.append("</td><td>");
-    questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), oldFee));
+    questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), old_fee));
     questionString.append("</td></tr><tr><td>");
     questionString.append(tr("Increase:"));
     questionString.append("</td><td>");
-    questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), newFee - oldFee));
+    questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), new_fee - old_fee));
     questionString.append("</td></tr><tr><td>");
     questionString.append(tr("New fee:"));
     questionString.append("</td><td>");
-    questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), newFee));
+    questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), new_fee));
     questionString.append("</td></tr></table>");
     SendConfirmationDialog confirmationDialog(tr("Confirm fee bump"), questionString);
     confirmationDialog.exec();
@@ -715,23 +709,15 @@ bool WalletModel::bumpFee(uint256 hash)
     }
 
     // sign bumped transaction
-    bool res = false;
+    if (!feebumper::SignTransaction(wallet, mtx)) {
-    {
-        LOCK2(cs_main, wallet->cs_wallet);
-        res = feeBump->signTransaction(wallet);
-    }
-    if (!res) {
         QMessageBox::critical(0, tr("Fee bump error"), tr("Can't sign transaction."));
         return false;
     }
     // commit the bumped transaction
-    {
+    uint256 txid;
-        LOCK2(cs_main, wallet->cs_wallet);
+    if (feebumper::CommitTransaction(wallet, hash, std::move(mtx), errors, txid) != feebumper::Result::OK) {
-        res = feeBump->commit(wallet);
-    }
-    if(!res) {
         QMessageBox::critical(0, tr("Fee bump error"), tr("Could not commit transaction") + "<br />(" +
-            QString::fromStdString(feeBump->getErrors()[0])+")");
+            QString::fromStdString(errors[0])+")");
          return false;
     }
     return true;

src/wallet/feebumper.cpp

@@ -43,13 +43,13 @@ static int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWalle
     return GetVirtualTransactionSize(txNew);
 }
 
-namespace feebumper {
+//! Check whether transaction has descendant in wallet or mempool, or has been
-
+//! mined, or conflicts with a mined transaction. Return a feebumper::Result.
-bool FeeBumper::preconditionChecks(const CWallet *wallet, const CWalletTx& wtx) {
+static feebumper::Result PreconditionChecks(const CWallet* wallet, const CWalletTx& wtx, std::vector<std::string>& errors)
+{
     if (wallet->HasWalletSpend(wtx.GetHash())) {
         errors.push_back("Transaction has descendants in the wallet");
-        current_result = Result::INVALID_PARAMETER;
+        return feebumper::Result::INVALID_PARAMETER;
-        return false;
     }
 
     {
@@ -57,58 +57,58 @@ bool FeeBumper::preconditionChecks(const CWallet *wallet, const CWalletTx& wtx)
         auto it_mp = mempool.mapTx.find(wtx.GetHash());
         if (it_mp != mempool.mapTx.end() && it_mp->GetCountWithDescendants() > 1) {
             errors.push_back("Transaction has descendants in the mempool");
-            current_result = Result::INVALID_PARAMETER;
+            return feebumper::Result::INVALID_PARAMETER;
-            return false;
         }
     }
 
     if (wtx.GetDepthInMainChain() != 0) {
         errors.push_back("Transaction has been mined, or is conflicted with a mined transaction");
-        current_result = Result::WALLET_ERROR;
+        return feebumper::Result::WALLET_ERROR;
-        return false;
     }
-    return true;
+    return feebumper::Result::OK;
 }
 
-FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinControl& coin_control, CAmount total_fee)
+namespace feebumper {
-    :
+
-    txid(std::move(txid_in)),
+bool TransactionCanBeBumped(CWallet* wallet, const uint256& txid)
-    old_fee(0),
-    new_fee(0)
 {
+    LOCK2(cs_main, wallet->cs_wallet);
+    const CWalletTx* wtx = wallet->GetWalletTx(txid);
+    return wtx && SignalsOptInRBF(*wtx->tx) && !wtx->mapValue.count("replaced_by_txid");
+}
+
+Result CreateTransaction(const CWallet* wallet, const uint256& txid, const CCoinControl& coin_control, CAmount total_fee, std::vector<std::string>& errors,
+                         CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx)
+{
+    LOCK2(cs_main, wallet->cs_wallet);
     errors.clear();
-    bumped_txid.SetNull();
-    AssertLockHeld(wallet->cs_wallet);
     auto it = wallet->mapWallet.find(txid);
     if (it == wallet->mapWallet.end()) {
         errors.push_back("Invalid or non-wallet transaction id");
-        current_result = Result::INVALID_ADDRESS_OR_KEY;
+        return Result::INVALID_ADDRESS_OR_KEY;
-        return;
     }
     const CWalletTx& wtx = it->second;
 
-    if (!preconditionChecks(wallet, wtx)) {
+    Result result = PreconditionChecks(wallet, wtx, errors);
-        return;
+    if (result != Result::OK) {
+        return result;
     }
 
     if (!SignalsOptInRBF(*wtx.tx)) {
         errors.push_back("Transaction is not BIP 125 replaceable");
-        current_result = Result::WALLET_ERROR;
+        return Result::WALLET_ERROR;
-        return;
     }
 
     if (wtx.mapValue.count("replaced_by_txid")) {
         errors.push_back(strprintf("Cannot bump transaction %s which was already bumped by transaction %s", txid.ToString(), wtx.mapValue.at("replaced_by_txid")));
-        current_result = Result::WALLET_ERROR;
+        return Result::WALLET_ERROR;
-        return;
     }
 
     // check that original tx consists entirely of our inputs
     // if not, we can't bump the fee, because the wallet has no way of knowing the value of the other inputs (thus the fee)
     if (!wallet->IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE)) {
         errors.push_back("Transaction contains inputs that don't belong to this wallet");
-        current_result = Result::WALLET_ERROR;
+        return Result::WALLET_ERROR;
-        return;
     }
 
     // figure out which output was change
@@ -118,16 +118,14 @@ FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinCo
         if (wallet->IsChange(wtx.tx->vout[i])) {
             if (nOutput != -1) {
                 errors.push_back("Transaction has multiple change outputs");
-                current_result = Result::WALLET_ERROR;
+                return Result::WALLET_ERROR;
-                return;
             }
             nOutput = i;
         }
     }
     if (nOutput == -1) {
         errors.push_back("Transaction does not have a change output");
-        current_result = Result::WALLET_ERROR;
+        return Result::WALLET_ERROR;
-        return;
     }
 
     // Calculate the expected size of the new transaction.
@@ -135,8 +133,7 @@ FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinCo
     const int64_t maxNewTxSize = CalculateMaximumSignedTxSize(*wtx.tx, wallet);
     if (maxNewTxSize < 0) {
         errors.push_back("Transaction contains inputs that cannot be signed");
-        current_result = Result::INVALID_ADDRESS_OR_KEY;
+        return Result::INVALID_ADDRESS_OR_KEY;
-        return;
     }
 
     // calculate the old fee and fee-rate
@@ -156,15 +153,13 @@ FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinCo
         if (total_fee < minTotalFee) {
             errors.push_back(strprintf("Insufficient totalFee, must be at least %s (oldFee %s + incrementalFee %s)",
                                                                 FormatMoney(minTotalFee), FormatMoney(nOldFeeRate.GetFee(maxNewTxSize)), FormatMoney(::incrementalRelayFee.GetFee(maxNewTxSize))));
-            current_result = Result::INVALID_PARAMETER;
+            return Result::INVALID_PARAMETER;
-            return;
         }
         CAmount requiredFee = GetRequiredFee(maxNewTxSize);
         if (total_fee < requiredFee) {
             errors.push_back(strprintf("Insufficient totalFee (cannot be less than required fee %s)",
                                                                 FormatMoney(requiredFee)));
-            current_result = Result::INVALID_PARAMETER;
+            return Result::INVALID_PARAMETER;
-            return;
         }
         new_fee = total_fee;
         nNewFeeRate = CFeeRate(total_fee, maxNewTxSize);
@@ -187,8 +182,7 @@ FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinCo
      if (new_fee > maxTxFee) {
          errors.push_back(strprintf("Specified or calculated fee %s is too high (cannot be higher than maxTxFee %s)",
                                FormatMoney(new_fee), FormatMoney(maxTxFee)));
-         current_result = Result::WALLET_ERROR;
+         return Result::WALLET_ERROR;
-         return;
      }
 
     // check that fee rate is higher than mempool's minimum fee
@@ -205,8 +199,7 @@ FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinCo
             FormatMoney(minMempoolFeeRate.GetFeePerK()),
             FormatMoney(minMempoolFeeRate.GetFee(maxNewTxSize)),
             FormatMoney(minMempoolFeeRate.GetFeePerK())));
-        current_result = Result::WALLET_ERROR;
+        return Result::WALLET_ERROR;
-        return;
     }
 
     // Now modify the output to increase the fee.
@@ -217,8 +210,7 @@ FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinCo
     CTxOut* poutput = &(mtx.vout[nOutput]);
     if (poutput->nValue < nDelta) {
         errors.push_back("Change output is too small to bump the fee");
-        current_result = Result::WALLET_ERROR;
+        return Result::WALLET_ERROR;
-        return;
     }
 
     // If the output would become dust, discard it (converting the dust to fee)
@@ -236,31 +228,31 @@ FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinCo
         }
     }
 
-    current_result = Result::OK;
+    return Result::OK;
 }
 
-bool FeeBumper::signTransaction(CWallet *wallet)
+bool SignTransaction(CWallet* wallet, CMutableTransaction& mtx) {
-{
+    LOCK2(cs_main, wallet->cs_wallet);
     return wallet->SignTransaction(mtx);
 }
 
-bool FeeBumper::commit(CWallet *wallet)
+Result CommitTransaction(CWallet* wallet, const uint256& txid, CMutableTransaction&& mtx, std::vector<std::string>& errors, uint256& bumped_txid)
 {
-    AssertLockHeld(wallet->cs_wallet);
+    LOCK2(cs_main, wallet->cs_wallet);
-    if (!errors.empty() || current_result != Result::OK) {
+    if (!errors.empty()) {
-        return false;
+        return Result::MISC_ERROR;
     }
     auto it = txid.IsNull() ? wallet->mapWallet.end() : wallet->mapWallet.find(txid);
     if (it == wallet->mapWallet.end()) {
         errors.push_back("Invalid or non-wallet transaction id");
-        current_result = Result::MISC_ERROR;
+        return Result::MISC_ERROR;
-        return false;
     }
     CWalletTx& oldWtx = it->second;
 
     // make sure the transaction still has no descendants and hasn't been mined in the meantime
-    if (!preconditionChecks(wallet, oldWtx)) {
+    Result result = PreconditionChecks(wallet, oldWtx, errors);
-        return false;
+    if (result != Result::OK) {
+        return result;
     }
 
     CWalletTx wtxBumped(wallet, MakeTransactionRef(std::move(mtx)));
@@ -276,14 +268,14 @@ bool FeeBumper::commit(CWallet *wallet)
     if (!wallet->CommitTransaction(wtxBumped, reservekey, g_connman.get(), state)) {
         // NOTE: CommitTransaction never returns false, so this should never happen.
         errors.push_back(strprintf("The transaction was rejected: %s", state.GetRejectReason()));
-        return false;
+        return Result::WALLET_ERROR;
     }
 
     bumped_txid = wtxBumped.GetHash();
     if (state.IsInvalid()) {
         // This can happen if the mempool rejected the transaction.  Report
         // what happened in the "errors" response.
-        errors.push_back(strprintf("The transaction was rejected: %s", FormatStateMessage(state)));
+        errors.push_back(strprintf("Error: The transaction was rejected: %s", FormatStateMessage(state)));
     }
 
     // mark the original tx as bumped
@@ -294,7 +286,7 @@ bool FeeBumper::commit(CWallet *wallet)
         // replaced does not succeed for some reason.
         errors.push_back("Created new bumpfee transaction but could not mark the original transaction as replaced");
     }
-    return true;
+    return Result::OK;
 }
 
 } // namespace feebumper

src/wallet/feebumper.h

@@ -25,40 +25,33 @@ enum class Result
     MISC_ERROR,
 };
 
-class FeeBumper
+//! Return whether transaction can be bumped.
-{
+bool TransactionCanBeBumped(CWallet* wallet, const uint256& txid);
-public:
+
-    FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinControl& coin_control, CAmount total_fee);
+//! Create bumpfee transaction.
-    Result getResult() const { return current_result; }
+Result CreateTransaction(const CWallet* wallet,
-    const std::vector<std::string>& getErrors() const { return errors; }
+                         const uint256& txid,
-    CAmount getOldFee() const { return old_fee; }
+                         const CCoinControl& coin_control,
-    CAmount getNewFee() const { return new_fee; }
+                         CAmount total_fee,
-    uint256 getBumpedTxId() const { return bumped_txid; }
+                         std::vector<std::string>& errors,
-
+                         CAmount& old_fee,
-    /* signs the new transaction,
+                         CAmount& new_fee,
-     * returns false if the tx couldn't be found or if it was
+                         CMutableTransaction& mtx);
-     * impossible to create the signature(s)
+
-     */
+//! Sign the new transaction,
-    bool signTransaction(CWallet *wallet);
+//! @return false if the tx couldn't be found or if it was
-
+//! impossible to create the signature(s)
-    /* commits the fee bump,
+bool SignTransaction(CWallet* wallet, CMutableTransaction& mtx);
-     * returns true, in case of CWallet::CommitTransaction was successful
+
-     * but, eventually sets errors if the tx could not be added to the mempool (will try later)
+//! Commit the bumpfee transaction.
-     * or if the old transaction could not be marked as replaced
+//! @return success in case of CWallet::CommitTransaction was successful,
-     */
+//! but sets errors if the tx could not be added to the mempool (will try later)
-    bool commit(CWallet *wallet);
+//! or if the old transaction could not be marked as replaced.
-
+Result CommitTransaction(CWallet* wallet,
-private:
+                         const uint256& txid,
-    bool preconditionChecks(const CWallet *wallet, const CWalletTx& wtx);
+                         CMutableTransaction&& mtx,
-
+                         std::vector<std::string>& errors,
-    const uint256 txid;
+                         uint256& bumped_txid);
-    uint256 bumped_txid;
-    CMutableTransaction mtx;
-    std::vector<std::string> errors;
-    Result current_result;
-    CAmount old_fee;
-    CAmount new_fee;
-};
 
 } // namespace feebumper
 

src/wallet/rpcwallet.cpp

@@ -3125,45 +3125,50 @@ UniValue bumpfee(const JSONRPCRequest& request)
     LOCK2(cs_main, pwallet->cs_wallet);
     EnsureWalletIsUnlocked(pwallet);
 
-    feebumper::FeeBumper feeBump(pwallet, hash, coin_control, totalFee);
+
-    feebumper::Result res = feeBump.getResult();
+    std::vector<std::string> errors;
-    if (res != feebumper::Result::OK)
+    CAmount old_fee;
-    {
+    CAmount new_fee;
+    CMutableTransaction mtx;
+    feebumper::Result res = feebumper::CreateTransaction(pwallet, hash, coin_control, totalFee, errors, old_fee, new_fee, mtx);
+    if (res != feebumper::Result::OK) {
         switch(res) {
             case feebumper::Result::INVALID_ADDRESS_OR_KEY:
-                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, feeBump.getErrors()[0]);
+                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, errors[0]);
                 break;
             case feebumper::Result::INVALID_REQUEST:
-                throw JSONRPCError(RPC_INVALID_REQUEST, feeBump.getErrors()[0]);
+                throw JSONRPCError(RPC_INVALID_REQUEST, errors[0]);
                 break;
             case feebumper::Result::INVALID_PARAMETER:
-                throw JSONRPCError(RPC_INVALID_PARAMETER, feeBump.getErrors()[0]);
+                throw JSONRPCError(RPC_INVALID_PARAMETER, errors[0]);
                 break;
             case feebumper::Result::WALLET_ERROR:
-                throw JSONRPCError(RPC_WALLET_ERROR, feeBump.getErrors()[0]);
+                throw JSONRPCError(RPC_WALLET_ERROR, errors[0]);
                 break;
             default:
-                throw JSONRPCError(RPC_MISC_ERROR, feeBump.getErrors()[0]);
+                throw JSONRPCError(RPC_MISC_ERROR, errors[0]);
                 break;
         }
     }
 
     // sign bumped transaction
-    if (!feeBump.signTransaction(pwallet)) {
+    if (!feebumper::SignTransaction(pwallet, mtx)) {
         throw JSONRPCError(RPC_WALLET_ERROR, "Can't sign transaction.");
     }
     // commit the bumped transaction
-    if(!feeBump.commit(pwallet)) {
+    uint256 txid;
-        throw JSONRPCError(RPC_WALLET_ERROR, feeBump.getErrors()[0]);
+    if (feebumper::CommitTransaction(pwallet, hash, std::move(mtx), errors, txid) != feebumper::Result::OK) {
+        throw JSONRPCError(RPC_WALLET_ERROR, errors[0]);
     }
     UniValue result(UniValue::VOBJ);
-    result.push_back(Pair("txid", feeBump.getBumpedTxId().GetHex()));
+    result.push_back(Pair("txid", txid.GetHex()));
-    result.push_back(Pair("origfee", ValueFromAmount(feeBump.getOldFee())));
+    result.push_back(Pair("origfee", ValueFromAmount(old_fee)));
-    result.push_back(Pair("fee", ValueFromAmount(feeBump.getNewFee())));
+    result.push_back(Pair("fee", ValueFromAmount(new_fee)));
-    UniValue errors(UniValue::VARR);
+    UniValue result_errors(UniValue::VARR);
-    for (const std::string& err: feeBump.getErrors())
+    for (const std::string& error : errors) {
-        errors.push_back(err);
+        result_errors.push_back(error);
-    result.push_back(Pair("errors", errors));
+    }
+    result.push_back(Pair("errors", result_errors));
 
     return result;
 }