Merge #10600: Make feebumper class stateless

aed1d90ac [wallet] Change feebumper from class to functions (Russell Yanofsky)
37bdcca3c [refactor] Make feebumper namespace (Russell Yanofsky)
7c4f00919 [trivial] Rename feebumper variables according to project code style (Russell Yanofsky)

Pull request description:

  Make feebumper methods static and remove stored state in the class.

  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 methods static, also:

  - Move LOCK calls from Qt code to feebumper
  - Move TransactionCanBeBumped implementation from Qt code to feebumper
  - Rename CFeeBumper class to FeeBumper (every CFeeBumper reference had to be
    updated in this PR anyway so this doesn't increase the size of the diff)

  This change was originally part of https://github.com/bitcoin/bitcoin/pull/10244

Tree-SHA512: bf75e0c741b4e9c8912e66cc1dedf0ff715f77ea65fc33f7020d97d9099b0f6448f5852236dac63eea649de7d6fc03b0b21492e2c5140fb7560a39cf085506fd

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);
-    const CWalletTx *wtx = wallet->GetWalletTx(hash);
-    return wtx && SignalsOptInRBF(*(wtx->tx)) && !wtx->mapValue.count("replaced_by_txid");
+    return feebumper::TransactionCanBeBumped(wallet, hash);
 }
 
 bool WalletModel::bumpFee(uint256 hash)
 {
-    std::unique_ptr<CFeeBumper> feeBump;
-    {
-        CCoinControl coin_control;
-        coin_control.signalRbf = true;
-        LOCK2(cs_main, wallet->cs_wallet);
-        feeBump.reset(new CFeeBumper(wallet, hash, coin_control, 0));
-    }
-    if (feeBump->getResult() != BumpFeeResult::OK)
-    {
+    CCoinControl coin_control;
+    coin_control.signalRbf = true;
+    std::vector<std::string> errors;
+    CAmount old_fee;
+    CAmount new_fee;
+    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;
-    {
-        LOCK2(cs_main, wallet->cs_wallet);
-        res = feeBump->signTransaction(wallet);
-    }
-    if (!res) {
+    if (!feebumper::SignTransaction(wallet, mtx)) {
         QMessageBox::critical(0, tr("Fee bump error"), tr("Can't sign transaction."));
         return false;
     }
     // commit the bumped transaction
-    {
-        LOCK2(cs_main, wallet->cs_wallet);
-        res = feeBump->commit(wallet);
-    }
-    if(!res) {
+    uint256 txid;
+    if (feebumper::CommitTransaction(wallet, hash, std::move(mtx), errors, txid) != feebumper::Result::OK) {
         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

@@ -23,7 +23,7 @@
 // calculation, but we should be able to refactor after priority is removed).
 // NOTE: this requires that all inputs must be in mapWallet (eg the tx should
 // be IsAllFromMe).
-int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *pWallet)
+static int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet)
 {
     CMutableTransaction txNew(tx);
     std::vector<CInputCoin> vCoins;
@@ -31,11 +31,11 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *pWal
     // IsAllFromMe(ISMINE_SPENDABLE), so every input should already be in our
     // wallet, with a valid index into the vout array.
     for (auto& input : tx.vin) {
-        const auto mi = pWallet->mapWallet.find(input.prevout.hash);
-        assert(mi != pWallet->mapWallet.end() && input.prevout.n < mi->second.tx->vout.size());
+        const auto mi = wallet->mapWallet.find(input.prevout.hash);
+        assert(mi != wallet->mapWallet.end() && input.prevout.n < mi->second.tx->vout.size());
         vCoins.emplace_back(CInputCoin(&(mi->second), input.prevout.n));
     }
-    if (!pWallet->DummySignTx(txNew, vCoins)) {
+    if (!wallet->DummySignTx(txNew, vCoins)) {
         // This should never happen, because IsAllFromMe(ISMINE_SPENDABLE)
         // implies that we can sign for every input.
         return -1;
@@ -43,103 +43,102 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *pWal
     return GetVirtualTransactionSize(txNew);
 }
 
-bool CFeeBumper::preconditionChecks(const CWallet *pWallet, const CWalletTx& wtx) {
-    if (pWallet->HasWalletSpend(wtx.GetHash())) {
-        vErrors.push_back("Transaction has descendants in the wallet");
-        currentResult = BumpFeeResult::INVALID_PARAMETER;
-        return false;
+//! Check whether transaction has descendant in wallet or mempool, or has been
+//! mined, or conflicts with a mined transaction. Return a feebumper::Result.
+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");
+        return feebumper::Result::INVALID_PARAMETER;
     }
 
     {
         LOCK(mempool.cs);
         auto it_mp = mempool.mapTx.find(wtx.GetHash());
         if (it_mp != mempool.mapTx.end() && it_mp->GetCountWithDescendants() > 1) {
-            vErrors.push_back("Transaction has descendants in the mempool");
-            currentResult = BumpFeeResult::INVALID_PARAMETER;
-            return false;
+            errors.push_back("Transaction has descendants in the mempool");
+            return feebumper::Result::INVALID_PARAMETER;
         }
     }
 
     if (wtx.GetDepthInMainChain() != 0) {
-        vErrors.push_back("Transaction has been mined, or is conflicted with a mined transaction");
-        currentResult = BumpFeeResult::WALLET_ERROR;
-        return false;
+        errors.push_back("Transaction has been mined, or is conflicted with a mined transaction");
+        return feebumper::Result::WALLET_ERROR;
     }
-    return true;
+    return feebumper::Result::OK;
 }
 
-CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoinControl& coin_control, CAmount totalFee)
-    :
-    txid(std::move(txidIn)),
-    nOldFee(0),
-    nNewFee(0)
+namespace feebumper {
+
+bool TransactionCanBeBumped(CWallet* wallet, const uint256& txid)
 {
-    vErrors.clear();
-    bumpedTxid.SetNull();
-    AssertLockHeld(pWallet->cs_wallet);
-    auto it = pWallet->mapWallet.find(txid);
-    if (it == pWallet->mapWallet.end()) {
-        vErrors.push_back("Invalid or non-wallet transaction id");
-        currentResult = BumpFeeResult::INVALID_ADDRESS_OR_KEY;
-        return;
+    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();
+    auto it = wallet->mapWallet.find(txid);
+    if (it == wallet->mapWallet.end()) {
+        errors.push_back("Invalid or non-wallet transaction id");
+        return Result::INVALID_ADDRESS_OR_KEY;
     }
     const CWalletTx& wtx = it->second;
 
-    if (!preconditionChecks(pWallet, wtx)) {
-        return;
+    Result result = PreconditionChecks(wallet, wtx, errors);
+    if (result != Result::OK) {
+        return result;
     }
 
     if (!SignalsOptInRBF(*wtx.tx)) {
-        vErrors.push_back("Transaction is not BIP 125 replaceable");
-        currentResult = BumpFeeResult::WALLET_ERROR;
-        return;
+        errors.push_back("Transaction is not BIP 125 replaceable");
+        return Result::WALLET_ERROR;
     }
 
     if (wtx.mapValue.count("replaced_by_txid")) {
-        vErrors.push_back(strprintf("Cannot bump transaction %s which was already bumped by transaction %s", txid.ToString(), wtx.mapValue.at("replaced_by_txid")));
-        currentResult = BumpFeeResult::WALLET_ERROR;
-        return;
+        errors.push_back(strprintf("Cannot bump transaction %s which was already bumped by transaction %s", txid.ToString(), wtx.mapValue.at("replaced_by_txid")));
+        return Result::WALLET_ERROR;
     }
 
     // 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 (!pWallet->IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE)) {
-        vErrors.push_back("Transaction contains inputs that don't belong to this wallet");
-        currentResult = BumpFeeResult::WALLET_ERROR;
-        return;
+    if (!wallet->IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE)) {
+        errors.push_back("Transaction contains inputs that don't belong to this wallet");
+        return Result::WALLET_ERROR;
     }
 
     // figure out which output was change
     // if there was no change output or multiple change outputs, fail
     int nOutput = -1;
     for (size_t i = 0; i < wtx.tx->vout.size(); ++i) {
-        if (pWallet->IsChange(wtx.tx->vout[i])) {
+        if (wallet->IsChange(wtx.tx->vout[i])) {
             if (nOutput != -1) {
-                vErrors.push_back("Transaction has multiple change outputs");
-                currentResult = BumpFeeResult::WALLET_ERROR;
-                return;
+                errors.push_back("Transaction has multiple change outputs");
+                return Result::WALLET_ERROR;
             }
             nOutput = i;
         }
     }
     if (nOutput == -1) {
-        vErrors.push_back("Transaction does not have a change output");
-        currentResult = BumpFeeResult::WALLET_ERROR;
-        return;
+        errors.push_back("Transaction does not have a change output");
+        return Result::WALLET_ERROR;
     }
 
     // Calculate the expected size of the new transaction.
     int64_t txSize = GetVirtualTransactionSize(*(wtx.tx));
-    const int64_t maxNewTxSize = CalculateMaximumSignedTxSize(*wtx.tx, pWallet);
+    const int64_t maxNewTxSize = CalculateMaximumSignedTxSize(*wtx.tx, wallet);
     if (maxNewTxSize < 0) {
-        vErrors.push_back("Transaction contains inputs that cannot be signed");
-        currentResult = BumpFeeResult::INVALID_ADDRESS_OR_KEY;
-        return;
+        errors.push_back("Transaction contains inputs that cannot be signed");
+        return Result::INVALID_ADDRESS_OR_KEY;
     }
 
     // calculate the old fee and fee-rate
-    nOldFee = wtx.GetDebit(ISMINE_SPENDABLE) - wtx.tx->GetValueOut();
-    CFeeRate nOldFeeRate(nOldFee, txSize);
+    old_fee = wtx.GetDebit(ISMINE_SPENDABLE) - wtx.tx->GetValueOut();
+    CFeeRate nOldFeeRate(old_fee, txSize);
     CFeeRate nNewFeeRate;
     // The wallet uses a conservative WALLET_INCREMENTAL_RELAY_FEE value to
     // future proof against changes to network wide policy for incremental relay
@@ -149,26 +148,24 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoin
         walletIncrementalRelayFee = ::incrementalRelayFee;
     }
 
-    if (totalFee > 0) {
+    if (total_fee > 0) {
         CAmount minTotalFee = nOldFeeRate.GetFee(maxNewTxSize) + ::incrementalRelayFee.GetFee(maxNewTxSize);
-        if (totalFee < minTotalFee) {
-            vErrors.push_back(strprintf("Insufficient totalFee, must be at least %s (oldFee %s + incrementalFee %s)",
+        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))));
-            currentResult = BumpFeeResult::INVALID_PARAMETER;
-            return;
+            return Result::INVALID_PARAMETER;
         }
         CAmount requiredFee = GetRequiredFee(maxNewTxSize);
-        if (totalFee < requiredFee) {
-            vErrors.push_back(strprintf("Insufficient totalFee (cannot be less than required fee %s)",
+        if (total_fee < requiredFee) {
+            errors.push_back(strprintf("Insufficient totalFee (cannot be less than required fee %s)",
                                                                 FormatMoney(requiredFee)));
-            currentResult = BumpFeeResult::INVALID_PARAMETER;
-            return;
+            return Result::INVALID_PARAMETER;
         }
-        nNewFee = totalFee;
-        nNewFeeRate = CFeeRate(totalFee, maxNewTxSize);
+        new_fee = total_fee;
+        nNewFeeRate = CFeeRate(total_fee, maxNewTxSize);
     } else {
-        nNewFee = GetMinimumFee(maxNewTxSize, coin_control, mempool, ::feeEstimator, nullptr /* FeeCalculation */);
-        nNewFeeRate = CFeeRate(nNewFee, maxNewTxSize);
+        new_fee = GetMinimumFee(maxNewTxSize, coin_control, mempool, ::feeEstimator, nullptr /* FeeCalculation */);
+        nNewFeeRate = CFeeRate(new_fee, maxNewTxSize);
 
         // New fee rate must be at least old rate + minimum incremental relay rate
         // walletIncrementalRelayFee.GetFeePerK() should be exact, because it's initialized
@@ -177,53 +174,50 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoin
         // add 1 satoshi to the result, because it may have been rounded down.
         if (nNewFeeRate.GetFeePerK() < nOldFeeRate.GetFeePerK() + 1 + walletIncrementalRelayFee.GetFeePerK()) {
             nNewFeeRate = CFeeRate(nOldFeeRate.GetFeePerK() + 1 + walletIncrementalRelayFee.GetFeePerK());
-            nNewFee = nNewFeeRate.GetFee(maxNewTxSize);
+            new_fee = nNewFeeRate.GetFee(maxNewTxSize);
         }
     }
 
     // Check that in all cases the new fee doesn't violate maxTxFee
-     if (nNewFee > maxTxFee) {
-         vErrors.push_back(strprintf("Specified or calculated fee %s is too high (cannot be higher than maxTxFee %s)",
-                               FormatMoney(nNewFee), FormatMoney(maxTxFee)));
-         currentResult = BumpFeeResult::WALLET_ERROR;
-         return;
+     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)));
+         return Result::WALLET_ERROR;
      }
 
     // check that fee rate is higher than mempool's minimum fee
     // (no point in bumping fee if we know that the new tx won't be accepted to the mempool)
     // This may occur if the user set TotalFee or paytxfee too low, if fallbackfee is too low, or, perhaps,
     // in a rare situation where the mempool minimum fee increased significantly since the fee estimation just a
-    // moment earlier. In this case, we report an error to the user, who may use totalFee to make an adjustment.
+    // moment earlier. In this case, we report an error to the user, who may use total_fee to make an adjustment.
     CFeeRate minMempoolFeeRate = mempool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000);
     if (nNewFeeRate.GetFeePerK() < minMempoolFeeRate.GetFeePerK()) {
-        vErrors.push_back(strprintf(
+        errors.push_back(strprintf(
             "New fee rate (%s) is lower than the minimum fee rate (%s) to get into the mempool -- "
             "the totalFee value should be at least %s or the settxfee value should be at least %s to add transaction",
             FormatMoney(nNewFeeRate.GetFeePerK()),
             FormatMoney(minMempoolFeeRate.GetFeePerK()),
             FormatMoney(minMempoolFeeRate.GetFee(maxNewTxSize)),
             FormatMoney(minMempoolFeeRate.GetFeePerK())));
-        currentResult = BumpFeeResult::WALLET_ERROR;
-        return;
+        return Result::WALLET_ERROR;
     }
 
     // Now modify the output to increase the fee.
     // If the output is not large enough to pay the fee, fail.
-    CAmount nDelta = nNewFee - nOldFee;
+    CAmount nDelta = new_fee - old_fee;
     assert(nDelta > 0);
     mtx =  *wtx.tx;
     CTxOut* poutput = &(mtx.vout[nOutput]);
     if (poutput->nValue < nDelta) {
-        vErrors.push_back("Change output is too small to bump the fee");
-        currentResult = BumpFeeResult::WALLET_ERROR;
-        return;
+        errors.push_back("Change output is too small to bump the fee");
+        return Result::WALLET_ERROR;
     }
 
     // If the output would become dust, discard it (converting the dust to fee)
     poutput->nValue -= nDelta;
     if (poutput->nValue <= GetDustThreshold(*poutput, ::dustRelayFee)) {
         LogPrint(BCLog::RPC, "Bumping fee and discarding dust output\n");
-        nNewFee += poutput->nValue;
+        new_fee += poutput->nValue;
         mtx.vout.erase(mtx.vout.begin() + nOutput);
     }
 
@@ -234,36 +228,36 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoin
         }
     }
 
-    currentResult = BumpFeeResult::OK;
+    return Result::OK;
 }
 
-bool CFeeBumper::signTransaction(CWallet *pWallet)
-{
-     return pWallet->SignTransaction(mtx);
+bool SignTransaction(CWallet* wallet, CMutableTransaction& mtx) {
+    LOCK2(cs_main, wallet->cs_wallet);
+    return wallet->SignTransaction(mtx);
 }
 
-bool CFeeBumper::commit(CWallet *pWallet)
+Result CommitTransaction(CWallet* wallet, const uint256& txid, CMutableTransaction&& mtx, std::vector<std::string>& errors, uint256& bumped_txid)
 {
-    AssertLockHeld(pWallet->cs_wallet);
-    if (!vErrors.empty() || currentResult != BumpFeeResult::OK) {
-        return false;
+    LOCK2(cs_main, wallet->cs_wallet);
+    if (!errors.empty()) {
+        return Result::MISC_ERROR;
     }
-    auto it = txid.IsNull() ? pWallet->mapWallet.end() : pWallet->mapWallet.find(txid);
-    if (it == pWallet->mapWallet.end()) {
-        vErrors.push_back("Invalid or non-wallet transaction id");
-        currentResult = BumpFeeResult::MISC_ERROR;
-        return false;
+    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");
+        return Result::MISC_ERROR;
     }
     CWalletTx& oldWtx = it->second;
 
     // make sure the transaction still has no descendants and hasn't been mined in the meantime
-    if (!preconditionChecks(pWallet, oldWtx)) {
-        return false;
+    Result result = PreconditionChecks(wallet, oldWtx, errors);
+    if (result != Result::OK) {
+        return result;
     }
 
-    CWalletTx wtxBumped(pWallet, MakeTransactionRef(std::move(mtx)));
+    CWalletTx wtxBumped(wallet, MakeTransactionRef(std::move(mtx)));
     // commit/broadcast the tx
-    CReserveKey reservekey(pWallet);
+    CReserveKey reservekey(wallet);
     wtxBumped.mapValue = oldWtx.mapValue;
     wtxBumped.mapValue["replaces_txid"] = oldWtx.GetHash().ToString();
     wtxBumped.vOrderForm = oldWtx.vOrderForm;
@@ -271,27 +265,29 @@ bool CFeeBumper::commit(CWallet *pWallet)
     wtxBumped.fTimeReceivedIsTxTime = true;
     wtxBumped.fFromMe = true;
     CValidationState state;
-    if (!pWallet->CommitTransaction(wtxBumped, reservekey, g_connman.get(), state)) {
+    if (!wallet->CommitTransaction(wtxBumped, reservekey, g_connman.get(), state)) {
         // NOTE: CommitTransaction never returns false, so this should never happen.
-        vErrors.push_back(strprintf("The transaction was rejected: %s", state.GetRejectReason()));
-        return false;
+        errors.push_back(strprintf("The transaction was rejected: %s", state.GetRejectReason()));
+        return Result::WALLET_ERROR;
     }
 
-    bumpedTxid = wtxBumped.GetHash();
+    bumped_txid = wtxBumped.GetHash();
     if (state.IsInvalid()) {
         // This can happen if the mempool rejected the transaction.  Report
         // what happened in the "errors" response.
-        vErrors.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
-    if (!pWallet->MarkReplaced(oldWtx.GetHash(), wtxBumped.GetHash())) {
+    if (!wallet->MarkReplaced(oldWtx.GetHash(), wtxBumped.GetHash())) {
         // TODO: see if JSON-RPC has a standard way of returning a response
         // along with an exception. It would be good to return information about
         // wtxBumped to the caller even if marking the original transaction
         // replaced does not succeed for some reason.
-        vErrors.push_back("Created new bumpfee transaction but could not mark the original transaction as replaced");
+        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

@@ -13,7 +13,9 @@ class uint256;
 class CCoinControl;
 enum class FeeEstimateMode;
 
-enum class BumpFeeResult
+namespace feebumper {
+
+enum class Result
 {
     OK,
     INVALID_ADDRESS_OR_KEY,
@@ -23,39 +25,34 @@ enum class BumpFeeResult
     MISC_ERROR,
 };
 
-class CFeeBumper
-{
-public:
-    CFeeBumper(const CWallet *pWalletIn, const uint256 txidIn, const CCoinControl& coin_control, CAmount totalFee);
-    BumpFeeResult getResult() const { return currentResult; }
-    const std::vector<std::string>& getErrors() const { return vErrors; }
-    CAmount getOldFee() const { return nOldFee; }
-    CAmount getNewFee() const { return nNewFee; }
-    uint256 getBumpedTxId() const { return bumpedTxid; }
-
-    /* signs the new transaction,
-     * returns false if the tx couldn't be found or if it was
-     * impossible to create the signature(s)
-     */
-    bool signTransaction(CWallet *pWallet);
-
-    /* commits the fee bump,
-     * returns true, in case of CWallet::CommitTransaction was successful
-     * but, eventually sets vErrors if the tx could not be added to the mempool (will try later)
-     * or if the old transaction could not be marked as replaced
-     */
-    bool commit(CWallet *pWalletNonConst);
-
-private:
-    bool preconditionChecks(const CWallet *pWallet, const CWalletTx& wtx);
-
-    const uint256 txid;
-    uint256 bumpedTxid;
-    CMutableTransaction mtx;
-    std::vector<std::string> vErrors;
-    BumpFeeResult currentResult;
-    CAmount nOldFee;
-    CAmount nNewFee;
-};
+//! Return whether transaction can be bumped.
+bool TransactionCanBeBumped(CWallet* wallet, const uint256& txid);
+
+//! Create bumpfee transaction.
+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);
+
+//! Sign the new transaction,
+//! @return false if the tx couldn't be found or if it was
+//! impossible to create the signature(s)
+bool SignTransaction(CWallet* wallet, CMutableTransaction& mtx);
+
+//! Commit the bumpfee transaction.
+//! @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)
+//! or if the old transaction could not be marked as replaced.
+Result CommitTransaction(CWallet* wallet,
+                         const uint256& txid,
+                         CMutableTransaction&& mtx,
+                         std::vector<std::string>& errors,
+                         uint256& bumped_txid);
+
+} // namespace feebumper
 
 #endif // BITCOIN_WALLET_FEEBUMPER_H

src/wallet/rpcwallet.cpp

@@ -3224,45 +3224,50 @@ UniValue bumpfee(const JSONRPCRequest& request)
     LOCK2(cs_main, pwallet->cs_wallet);
     EnsureWalletIsUnlocked(pwallet);
 
-    CFeeBumper feeBump(pwallet, hash, coin_control, totalFee);
-    BumpFeeResult res = feeBump.getResult();
-    if (res != BumpFeeResult::OK)
-    {
+
+    std::vector<std::string> errors;
+    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 BumpFeeResult::INVALID_ADDRESS_OR_KEY:
-                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, feeBump.getErrors()[0]);
+            case feebumper::Result::INVALID_ADDRESS_OR_KEY:
+                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, errors[0]);
                 break;
-            case BumpFeeResult::INVALID_REQUEST:
-                throw JSONRPCError(RPC_INVALID_REQUEST, feeBump.getErrors()[0]);
+            case feebumper::Result::INVALID_REQUEST:
+                throw JSONRPCError(RPC_INVALID_REQUEST, errors[0]);
                 break;
-            case BumpFeeResult::INVALID_PARAMETER:
-                throw JSONRPCError(RPC_INVALID_PARAMETER, feeBump.getErrors()[0]);
+            case feebumper::Result::INVALID_PARAMETER:
+                throw JSONRPCError(RPC_INVALID_PARAMETER, errors[0]);
                 break;
-            case BumpFeeResult::WALLET_ERROR:
-                throw JSONRPCError(RPC_WALLET_ERROR, feeBump.getErrors()[0]);
+            case feebumper::Result::WALLET_ERROR:
+                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)) {
-        throw JSONRPCError(RPC_WALLET_ERROR, feeBump.getErrors()[0]);
+    uint256 txid;
+    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("origfee", ValueFromAmount(feeBump.getOldFee())));
-    result.push_back(Pair("fee", ValueFromAmount(feeBump.getNewFee())));
-    UniValue errors(UniValue::VARR);
-    for (const std::string& err: feeBump.getErrors())
-        errors.push_back(err);
-    result.push_back(Pair("errors", errors));
+    result.push_back(Pair("txid", txid.GetHex()));
+    result.push_back(Pair("origfee", ValueFromAmount(old_fee)));
+    result.push_back(Pair("fee", ValueFromAmount(new_fee)));
+    UniValue result_errors(UniValue::VARR);
+    for (const std::string& error : errors) {
+        result_errors.push_back(error);
+    }
+    result.push_back(Pair("errors", result_errors));
 
     return result;
 }