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; }