From b9374946405c831560227daa3bbe6a8e6235221f Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Thu, 15 Jun 2017 10:34:17 -0400 Subject: [PATCH] Make feebumper class stateless 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 --- src/qt/walletmodel.cpp | 40 ++++++----------- src/wallet/feebumper.cpp | 112 ++++++++++++++++++++++++----------------------- src/wallet/feebumper.h | 39 ++++++++--------- src/wallet/rpcwallet.cpp | 39 ++++++++++------- 4 files changed, 111 insertions(+), 119 deletions(-) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index ba0e1da0c78..e2c91f288d6 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -659,32 +659,26 @@ 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) && !wtx->mapValue.count("replaced_by_txid"); + return FeeBumper::TransactionCanBeBumped(wallet, hash); } bool WalletModel::bumpFee(uint256 hash) { - std::unique_ptr 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 errors; + CAmount oldFee; + CAmount newFee; + CMutableTransaction mtx; + if (FeeBumper::CreateTransaction(wallet, hash, coin_control, 0 /* totalFee */, errors, oldFee, newFee, mtx) != BumpFeeResult::OK) { QMessageBox::critical(0, tr("Fee bump error"), tr("Increasing transaction fee failed") + "
(" + - (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("
"); - CAmount oldFee = feeBump->getOldFee(); - CAmount newFee = feeBump->getNewFee(); questionString.append(""); questionString.append("
"); questionString.append(tr("Current fee:")); @@ -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) != BumpFeeResult::OK) { QMessageBox::critical(0, tr("Fee bump error"), tr("Could not commit transaction") + "
(" + - QString::fromStdString(feeBump->getErrors()[0])+")"); + QString::fromStdString(errors[0])+")"); return false; } return true; diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 4bfd8726a54..de1b610e822 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -15,6 +15,7 @@ #include "util.h" #include "net.h" +namespace { // Calculate the size of the transaction assuming all signatures are max size // Use DummySignatureCreator, which inserts 72 byte signatures everywhere. // TODO: re-use this in CWallet::CreateTransaction (right now @@ -42,11 +43,11 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *pWal return GetVirtualTransactionSize(txNew); } -bool CFeeBumper::preconditionChecks(const CWallet *pWallet, const CWalletTx& wtx) { +BumpFeeResult PreconditionChecks(const CWallet* pWallet, const CWalletTx& wtx, std::vector& vErrors) +{ if (pWallet->HasWalletSpend(wtx.GetHash())) { vErrors.push_back("Transaction has descendants in the wallet"); - currentResult = BumpFeeResult::INVALID_PARAMETER; - return false; + return BumpFeeResult::INVALID_PARAMETER; } { @@ -54,58 +55,63 @@ bool CFeeBumper::preconditionChecks(const CWallet *pWallet, const CWalletTx& wtx 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; + return BumpFeeResult::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; + return BumpFeeResult::WALLET_ERROR; } - return true; + return BumpFeeResult::OK; } +} // namespace -CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoinControl& coin_control, CAmount totalFee) - : - txid(std::move(txidIn)), - nOldFee(0), - nNewFee(0) +bool FeeBumper::TransactionCanBeBumped(CWallet* pWallet, const uint256& txid) { + LOCK2(cs_main, pWallet->cs_wallet); + const CWalletTx* wtx = pWallet->GetWalletTx(txid); + return wtx && SignalsOptInRBF(*wtx) && !wtx->mapValue.count("replaced_by_txid"); +} + +BumpFeeResult FeeBumper::CreateTransaction(const CWallet* pWallet, + const uint256& txid, + const CCoinControl& coin_control, + CAmount totalFee, + std::vector& vErrors, + CAmount& nOldFee, + CAmount& nNewFee, + CMutableTransaction& mtx) +{ + LOCK2(cs_main, pWallet->cs_wallet); vErrors.clear(); - bumpedTxid.SetNull(); - AssertLockHeld(pWallet->cs_wallet); if (!pWallet->mapWallet.count(txid)) { vErrors.push_back("Invalid or non-wallet transaction id"); - currentResult = BumpFeeResult::INVALID_ADDRESS_OR_KEY; - return; + return BumpFeeResult::INVALID_ADDRESS_OR_KEY; } auto it = pWallet->mapWallet.find(txid); const CWalletTx& wtx = it->second; - if (!preconditionChecks(pWallet, wtx)) { - return; + BumpFeeResult result = PreconditionChecks(pWallet, wtx, vErrors); + if (result != BumpFeeResult::OK) { + return result; } if (!SignalsOptInRBF(wtx)) { vErrors.push_back("Transaction is not BIP 125 replaceable"); - currentResult = BumpFeeResult::WALLET_ERROR; - return; + return BumpFeeResult::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; + return BumpFeeResult::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, ISMINE_SPENDABLE)) { vErrors.push_back("Transaction contains inputs that don't belong to this wallet"); - currentResult = BumpFeeResult::WALLET_ERROR; - return; + return BumpFeeResult::WALLET_ERROR; } // figure out which output was change @@ -115,16 +121,14 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoin if (pWallet->IsChange(wtx.tx->vout[i])) { if (nOutput != -1) { vErrors.push_back("Transaction has multiple change outputs"); - currentResult = BumpFeeResult::WALLET_ERROR; - return; + return BumpFeeResult::WALLET_ERROR; } nOutput = i; } } if (nOutput == -1) { vErrors.push_back("Transaction does not have a change output"); - currentResult = BumpFeeResult::WALLET_ERROR; - return; + return BumpFeeResult::WALLET_ERROR; } // Calculate the expected size of the new transaction. @@ -132,8 +136,7 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoin const int64_t maxNewTxSize = CalculateMaximumSignedTxSize(*wtx.tx, pWallet); if (maxNewTxSize < 0) { vErrors.push_back("Transaction contains inputs that cannot be signed"); - currentResult = BumpFeeResult::INVALID_ADDRESS_OR_KEY; - return; + return BumpFeeResult::INVALID_ADDRESS_OR_KEY; } // calculate the old fee and fee-rate @@ -153,15 +156,13 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoin if (totalFee < minTotalFee) { vErrors.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 BumpFeeResult::INVALID_PARAMETER; } CAmount requiredFee = CWallet::GetRequiredFee(maxNewTxSize); if (totalFee < requiredFee) { vErrors.push_back(strprintf("Insufficient totalFee (cannot be less than required fee %s)", FormatMoney(requiredFee))); - currentResult = BumpFeeResult::INVALID_PARAMETER; - return; + return BumpFeeResult::INVALID_PARAMETER; } nNewFee = totalFee; nNewFeeRate = CFeeRate(totalFee, maxNewTxSize); @@ -184,8 +185,7 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoin 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; + return BumpFeeResult::WALLET_ERROR; } // check that fee rate is higher than mempool's minimum fee @@ -196,8 +196,7 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoin CFeeRate minMempoolFeeRate = mempool.GetMinFee(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000); if (nNewFeeRate.GetFeePerK() < minMempoolFeeRate.GetFeePerK()) { vErrors.push_back(strprintf("New fee rate (%s) is less than the minimum fee rate (%s) to get into the mempool. totalFee value should to be at least %s or 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 BumpFeeResult::WALLET_ERROR; } // Now modify the output to increase the fee. @@ -208,8 +207,7 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoin 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; + return BumpFeeResult::WALLET_ERROR; } // If the output would become dust, discard it (converting the dust to fee) @@ -227,30 +225,34 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoin } } - currentResult = BumpFeeResult::OK; + return BumpFeeResult::OK; } -bool CFeeBumper::signTransaction(CWallet *pWallet) -{ - return pWallet->SignTransaction(mtx); +bool FeeBumper::SignTransaction(CWallet* pWallet, CMutableTransaction& mtx) { + LOCK2(cs_main, pWallet->cs_wallet); + return pWallet->SignTransaction(mtx); } -bool CFeeBumper::commit(CWallet *pWallet) +BumpFeeResult FeeBumper::CommitTransaction(CWallet* pWallet, + const uint256& txid, + CMutableTransaction&& mtx, + std::vector& vErrors, + uint256& bumpedTxid) { - AssertLockHeld(pWallet->cs_wallet); - if (!vErrors.empty() || currentResult != BumpFeeResult::OK) { - return false; + LOCK2(cs_main, pWallet->cs_wallet); + if (!vErrors.empty()) { + return BumpFeeResult::MISC_ERROR; } if (txid.IsNull() || !pWallet->mapWallet.count(txid)) { vErrors.push_back("Invalid or non-wallet transaction id"); - currentResult = BumpFeeResult::MISC_ERROR; - return false; + return BumpFeeResult::MISC_ERROR; } CWalletTx& oldWtx = pWallet->mapWallet[txid]; // make sure the transaction still has no descendants and hasn't been mined in the meantime - if (!preconditionChecks(pWallet, oldWtx)) { - return false; + BumpFeeResult result = PreconditionChecks(pWallet, oldWtx, vErrors); + if (result != BumpFeeResult::OK) { + return result; } CWalletTx wtxBumped(pWallet, MakeTransactionRef(std::move(mtx))); @@ -266,7 +268,7 @@ bool CFeeBumper::commit(CWallet *pWallet) if (!pWallet->CommitTransaction(wtxBumped, reservekey, g_connman.get(), state)) { // NOTE: CommitTransaction never returns false, so this should never happen. vErrors.push_back(strprintf("Error: The transaction was rejected! Reason given: %s", state.GetRejectReason())); - return false; + return BumpFeeResult::WALLET_ERROR; } bumpedTxid = wtxBumped.GetHash(); @@ -284,6 +286,6 @@ bool CFeeBumper::commit(CWallet *pWallet) // replaced does not succeed for some reason. vErrors.push_back("Error: Created new bumpfee transaction but could not mark the original transaction as replaced."); } - return true; + return BumpFeeResult::OK; } diff --git a/src/wallet/feebumper.h b/src/wallet/feebumper.h index 3d64e53c15c..aa5b2dea60d 100644 --- a/src/wallet/feebumper.h +++ b/src/wallet/feebumper.h @@ -23,39 +23,38 @@ enum class BumpFeeResult MISC_ERROR, }; -class CFeeBumper +class FeeBumper { public: - CFeeBumper(const CWallet *pWalletIn, const uint256 txidIn, const CCoinControl& coin_control, CAmount totalFee); - BumpFeeResult getResult() const { return currentResult; } - const std::vector& getErrors() const { return vErrors; } - CAmount getOldFee() const { return nOldFee; } - CAmount getNewFee() const { return nNewFee; } - uint256 getBumpedTxId() const { return bumpedTxid; } + /* return whether transaction can be bumped */ + static bool TransactionCanBeBumped(CWallet* pWallet, const uint256& txid); + + /* create bumpfee transaction */ + static BumpFeeResult CreateTransaction(const CWallet* pWallet, + const uint256& txid, + const CCoinControl& coin_control, + CAmount totalFee, + std::vector& vErrors, + CAmount& nOldFee, + CAmount& nNewFee, + CMutableTransaction& mtx); /* 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); + static bool SignTransaction(CWallet* pWallet, CMutableTransaction& mtx); /* 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 vErrors; - BumpFeeResult currentResult; - CAmount nOldFee; - CAmount nNewFee; + static BumpFeeResult CommitTransaction(CWallet* pWallet, + const uint256& txid, + CMutableTransaction&& mtx, + std::vector& vErrors, + uint256& bumpedTxid); }; #endif // BITCOIN_WALLET_FEEBUMPER_H diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index f63ce1bb48c..37e70994b47 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2946,45 +2946,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(); + std::vector errors; + CAmount oldFee; + CAmount newFee; + CMutableTransaction mtx; + BumpFeeResult res = FeeBumper::CreateTransaction(pwallet, hash, coin_control, totalFee, errors, oldFee, newFee, mtx); if (res != BumpFeeResult::OK) { switch(res) { case BumpFeeResult::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 BumpFeeResult::INVALID_REQUEST: - throw JSONRPCError(RPC_INVALID_REQUEST, feeBump.getErrors()[0]); + throw JSONRPCError(RPC_INVALID_REQUEST, errors[0]); break; case BumpFeeResult::INVALID_PARAMETER: - throw JSONRPCError(RPC_INVALID_PARAMETER, feeBump.getErrors()[0]); + throw JSONRPCError(RPC_INVALID_PARAMETER, errors[0]); break; case BumpFeeResult::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)) { - throw JSONRPCError(RPC_WALLET_ERROR, feeBump.getErrors()[0]); + uint256 txid; + if (FeeBumper::CommitTransaction(pwallet, hash, std::move(mtx), errors, txid) != BumpFeeResult::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(oldFee))); + result.push_back(Pair("fee", ValueFromAmount(newFee))); + 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; }