From aed1d90aca81c20c6e982ad567291f3812d47c8f Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Thu, 15 Jun 2017 10:34:17 -0400 Subject: [PATCH] [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 | 46 ++++++----------- src/wallet/feebumper.cpp | 108 ++++++++++++++++++--------------------- src/wallet/feebumper.h | 61 ++++++++++------------ src/wallet/rpcwallet.cpp | 43 +++++++++------- 4 files changed, 117 insertions(+), 141 deletions(-) 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; }