diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 50d375c8d..c5cca0bff 100644 --- a/src/qt/walletmodel.cpp +++ b/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 feeBump; - { - CCoinControl coin_control; - coin_control.signalRbf = true; - LOCK2(cs_main, wallet->cs_wallet); - feeBump.reset(new feebumper::FeeBumper(wallet, hash, coin_control, 0)); - } - if (feeBump->getResult() != feebumper::Result::OK) - { + CCoinControl coin_control; + coin_control.signalRbf = true; + std::vector 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") + "
(" + - (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:")); questionString.append(""); - questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), oldFee)); + questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), old_fee)); questionString.append("
"); questionString.append(tr("Increase:")); questionString.append(""); - questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), newFee - oldFee)); + questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), new_fee - old_fee)); questionString.append("
"); questionString.append(tr("New fee:")); questionString.append(""); - questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), newFee)); + questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), new_fee)); questionString.append("
"); 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") + "
(" + - 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 1e6f8329f..aaddd47de 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -43,13 +43,13 @@ static int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWalle return GetVirtualTransactionSize(txNew); } -namespace feebumper { - -bool FeeBumper::preconditionChecks(const CWallet *wallet, const CWalletTx& wtx) { +//! 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& errors) +{ if (wallet->HasWalletSpend(wtx.GetHash())) { errors.push_back("Transaction has descendants in the wallet"); - current_result = Result::INVALID_PARAMETER; - return false; + return feebumper::Result::INVALID_PARAMETER; } { @@ -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 false; + return feebumper::Result::INVALID_PARAMETER; } } if (wtx.GetDepthInMainChain() != 0) { errors.push_back("Transaction has been mined, or is conflicted with a mined transaction"); - current_result = Result::WALLET_ERROR; - return false; + return feebumper::Result::WALLET_ERROR; } - return true; + return feebumper::Result::OK; } -FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinControl& coin_control, CAmount total_fee) - : - txid(std::move(txid_in)), - old_fee(0), - new_fee(0) +namespace feebumper { + +bool TransactionCanBeBumped(CWallet* wallet, const uint256& txid) { + 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& 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; + return Result::INVALID_ADDRESS_OR_KEY; } const CWalletTx& wtx = it->second; - if (!preconditionChecks(wallet, wtx)) { - return; + Result result = PreconditionChecks(wallet, wtx, errors); + 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; + return Result::WALLET_ERROR; } 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; + 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 (!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; + return Result::WALLET_ERROR; } // 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; + return Result::WALLET_ERROR; } nOutput = i; } } if (nOutput == -1) { errors.push_back("Transaction does not have a change output"); - current_result = Result::WALLET_ERROR; - return; + return Result::WALLET_ERROR; } // 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; + return Result::INVALID_ADDRESS_OR_KEY; } // 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; + return Result::INVALID_PARAMETER; } 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; + return Result::INVALID_PARAMETER; } 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; + return Result::WALLET_ERROR; } // 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; + return Result::WALLET_ERROR; } // 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; + return Result::WALLET_ERROR; } // 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) -{ - return wallet->SignTransaction(mtx); +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& errors, uint256& bumped_txid) { - AssertLockHeld(wallet->cs_wallet); - if (!errors.empty() || current_result != Result::OK) { - return false; + LOCK2(cs_main, wallet->cs_wallet); + if (!errors.empty()) { + 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 false; + 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(wallet, oldWtx)) { - return false; + Result result = PreconditionChecks(wallet, oldWtx, errors); + 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 diff --git a/src/wallet/feebumper.h b/src/wallet/feebumper.h index 046bd5600..8eec30440 100644 --- a/src/wallet/feebumper.h +++ b/src/wallet/feebumper.h @@ -25,40 +25,33 @@ enum class Result MISC_ERROR, }; -class FeeBumper -{ -public: - FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinControl& coin_control, CAmount total_fee); - Result getResult() const { return current_result; } - const std::vector& getErrors() const { return errors; } - CAmount getOldFee() const { return old_fee; } - CAmount getNewFee() const { return new_fee; } - uint256 getBumpedTxId() const { return bumped_txid; } - - /* 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 *wallet); - - /* commits the fee bump, - * 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) - * or if the old transaction could not be marked as replaced - */ - bool commit(CWallet *wallet); - -private: - bool preconditionChecks(const CWallet *wallet, const CWalletTx& wtx); - - const uint256 txid; - uint256 bumped_txid; - CMutableTransaction mtx; - std::vector errors; - Result current_result; - CAmount old_fee; - CAmount new_fee; -}; +//! 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& 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& errors, + uint256& bumped_txid); } // namespace feebumper diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 8738952bf..2d2eb4b6d 100644 --- a/src/wallet/rpcwallet.cpp +++ b/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(); - if (res != feebumper::Result::OK) - { + + std::vector 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 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)) { - 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; }