From 7c4f0091957d305679546fde592ffa2de2d186fe Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 20 Sep 2017 16:04:05 -0400 Subject: [PATCH 1/3] [trivial] Rename feebumper variables according to project code style Future PRs will completely refactor this translation unit and touch all this code so we rename the variables to follow project stlye guidelines in this preparation commit. Don't use m_ prefixes for member variables since we're going to remove the class entirely in the next commits. --- src/qt/walletmodel.cpp | 4 +- src/wallet/feebumper.cpp | 166 +++++++++++++++++++-------------------- src/wallet/feebumper.h | 32 ++++---- src/wallet/rpcwallet.cpp | 2 +- 4 files changed, 102 insertions(+), 102 deletions(-) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index e1d066062..d850c5e55 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -666,12 +666,12 @@ bool WalletModel::transactionCanBeBumped(uint256 hash) const bool WalletModel::bumpFee(uint256 hash) { - std::unique_ptr feeBump; + 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)); + feeBump.reset(new FeeBumper(wallet, hash, coin_control, 0)); } if (feeBump->getResult() != BumpFeeResult::OK) { diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 8b7c50b4e..fa64c2154 100644 --- a/src/wallet/feebumper.cpp +++ b/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) +int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet) { CMutableTransaction txNew(tx); std::vector 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,10 +43,10 @@ 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; +bool FeeBumper::preconditionChecks(const CWallet *wallet, const CWalletTx& wtx) { + if (wallet->HasWalletSpend(wtx.GetHash())) { + errors.push_back("Transaction has descendants in the wallet"); + current_result = BumpFeeResult::INVALID_PARAMETER; return false; } @@ -54,58 +54,58 @@ bool CFeeBumper::preconditionChecks(const CWallet *pWallet, const CWalletTx& wtx 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; + errors.push_back("Transaction has descendants in the mempool"); + current_result = BumpFeeResult::INVALID_PARAMETER; return false; } } if (wtx.GetDepthInMainChain() != 0) { - vErrors.push_back("Transaction has been mined, or is conflicted with a mined transaction"); - currentResult = BumpFeeResult::WALLET_ERROR; + errors.push_back("Transaction has been mined, or is conflicted with a mined transaction"); + current_result = BumpFeeResult::WALLET_ERROR; return false; } return true; } -CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoinControl& coin_control, CAmount totalFee) +FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinControl& coin_control, CAmount total_fee) : - txid(std::move(txidIn)), - nOldFee(0), - nNewFee(0) + txid(std::move(txid_in)), + old_fee(0), + new_fee(0) { - 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; + 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 = BumpFeeResult::INVALID_ADDRESS_OR_KEY; return; } const CWalletTx& wtx = it->second; - if (!preconditionChecks(pWallet, wtx)) { + if (!preconditionChecks(wallet, wtx)) { return; } if (!SignalsOptInRBF(*wtx.tx)) { - vErrors.push_back("Transaction is not BIP 125 replaceable"); - currentResult = BumpFeeResult::WALLET_ERROR; + errors.push_back("Transaction is not BIP 125 replaceable"); + current_result = BumpFeeResult::WALLET_ERROR; return; } 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; + 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 = BumpFeeResult::WALLET_ERROR; return; } // 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; + if (!wallet->IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE)) { + errors.push_back("Transaction contains inputs that don't belong to this wallet"); + current_result = BumpFeeResult::WALLET_ERROR; return; } @@ -113,33 +113,33 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoin // 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; + errors.push_back("Transaction has multiple change outputs"); + current_result = BumpFeeResult::WALLET_ERROR; return; } nOutput = i; } } if (nOutput == -1) { - vErrors.push_back("Transaction does not have a change output"); - currentResult = BumpFeeResult::WALLET_ERROR; + errors.push_back("Transaction does not have a change output"); + current_result = BumpFeeResult::WALLET_ERROR; return; } // 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; + errors.push_back("Transaction contains inputs that cannot be signed"); + current_result = BumpFeeResult::INVALID_ADDRESS_OR_KEY; return; } // 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 +149,26 @@ 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; + current_result = BumpFeeResult::INVALID_PARAMETER; return; } 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; + current_result = BumpFeeResult::INVALID_PARAMETER; return; } - 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,15 +177,15 @@ 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; + 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 = BumpFeeResult::WALLET_ERROR; return; } @@ -193,29 +193,29 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoin // (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; + current_result = BumpFeeResult::WALLET_ERROR; return; } // 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; + errors.push_back("Change output is too small to bump the fee"); + current_result = BumpFeeResult::WALLET_ERROR; return; } @@ -223,7 +223,7 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoin 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 +234,36 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoin } } - currentResult = BumpFeeResult::OK; + current_result = BumpFeeResult::OK; } -bool CFeeBumper::signTransaction(CWallet *pWallet) +bool FeeBumper::signTransaction(CWallet *wallet) { - return pWallet->SignTransaction(mtx); + return wallet->SignTransaction(mtx); } -bool CFeeBumper::commit(CWallet *pWallet) +bool FeeBumper::commit(CWallet *wallet) { - AssertLockHeld(pWallet->cs_wallet); - if (!vErrors.empty() || currentResult != BumpFeeResult::OK) { + AssertLockHeld(wallet->cs_wallet); + if (!errors.empty() || current_result != BumpFeeResult::OK) { return false; } - 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; + 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 = BumpFeeResult::MISC_ERROR; return false; } CWalletTx& oldWtx = it->second; // make sure the transaction still has no descendants and hasn't been mined in the meantime - if (!preconditionChecks(pWallet, oldWtx)) { + if (!preconditionChecks(wallet, oldWtx)) { return false; } - 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,26 +271,26 @@ 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())); + errors.push_back(strprintf("The transaction was rejected: %s", state.GetRejectReason())); return false; } - 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("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; } diff --git a/src/wallet/feebumper.h b/src/wallet/feebumper.h index 3d64e53c1..8daf80367 100644 --- a/src/wallet/feebumper.h +++ b/src/wallet/feebumper.h @@ -23,39 +23,39 @@ 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; } + FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinControl& coin_control, CAmount total_fee); + BumpFeeResult 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 *pWallet); + bool signTransaction(CWallet *wallet); /* 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) + * 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 *pWalletNonConst); + bool commit(CWallet *wallet); private: - bool preconditionChecks(const CWallet *pWallet, const CWalletTx& wtx); + bool preconditionChecks(const CWallet *wallet, const CWalletTx& wtx); const uint256 txid; - uint256 bumpedTxid; + uint256 bumped_txid; CMutableTransaction mtx; - std::vector vErrors; - BumpFeeResult currentResult; - CAmount nOldFee; - CAmount nNewFee; + std::vector errors; + BumpFeeResult current_result; + CAmount old_fee; + CAmount new_fee; }; #endif // BITCOIN_WALLET_FEEBUMPER_H diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index d4015f6c8..e9fd15aa0 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3125,7 +3125,7 @@ UniValue bumpfee(const JSONRPCRequest& request) LOCK2(cs_main, pwallet->cs_wallet); EnsureWalletIsUnlocked(pwallet); - CFeeBumper feeBump(pwallet, hash, coin_control, totalFee); + FeeBumper feeBump(pwallet, hash, coin_control, totalFee); BumpFeeResult res = feeBump.getResult(); if (res != BumpFeeResult::OK) { From 37bdcca3c363cf08ad68e044b493e24e89f2d158 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 20 Sep 2017 16:19:30 -0400 Subject: [PATCH 2/3] [refactor] Make feebumper namespace Future commit will remove the FeeBumper class. This commit simply places everything into a feebumper namespace, and changes the enum class name from BumpeFeeResult to feebumper::Result. --- src/qt/walletmodel.cpp | 6 +++--- src/wallet/feebumper.cpp | 42 ++++++++++++++++++++++------------------ src/wallet/feebumper.h | 10 +++++++--- src/wallet/rpcwallet.cpp | 14 +++++++------- 4 files changed, 40 insertions(+), 32 deletions(-) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index d850c5e55..50d375c8d 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -666,14 +666,14 @@ bool WalletModel::transactionCanBeBumped(uint256 hash) const bool WalletModel::bumpFee(uint256 hash) { - std::unique_ptr feeBump; + std::unique_ptr feeBump; { CCoinControl coin_control; coin_control.signalRbf = true; LOCK2(cs_main, wallet->cs_wallet); - feeBump.reset(new FeeBumper(wallet, hash, coin_control, 0)); + feeBump.reset(new feebumper::FeeBumper(wallet, hash, coin_control, 0)); } - if (feeBump->getResult() != BumpFeeResult::OK) + if (feeBump->getResult() != feebumper::Result::OK) { QMessageBox::critical(0, tr("Fee bump error"), tr("Increasing transaction fee failed") + "
(" + (feeBump->getErrors().size() ? QString::fromStdString(feeBump->getErrors()[0]) : "") +")"); diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index fa64c2154..1e6f8329f 100644 --- a/src/wallet/feebumper.cpp +++ b/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 *wallet) +static int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet) { CMutableTransaction txNew(tx); std::vector vCoins; @@ -43,10 +43,12 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wall return GetVirtualTransactionSize(txNew); } +namespace feebumper { + bool FeeBumper::preconditionChecks(const CWallet *wallet, const CWalletTx& wtx) { if (wallet->HasWalletSpend(wtx.GetHash())) { errors.push_back("Transaction has descendants in the wallet"); - current_result = BumpFeeResult::INVALID_PARAMETER; + current_result = Result::INVALID_PARAMETER; return false; } @@ -55,14 +57,14 @@ 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 = BumpFeeResult::INVALID_PARAMETER; + current_result = Result::INVALID_PARAMETER; return false; } } if (wtx.GetDepthInMainChain() != 0) { errors.push_back("Transaction has been mined, or is conflicted with a mined transaction"); - current_result = BumpFeeResult::WALLET_ERROR; + current_result = Result::WALLET_ERROR; return false; } return true; @@ -80,7 +82,7 @@ FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinCo auto it = wallet->mapWallet.find(txid); if (it == wallet->mapWallet.end()) { errors.push_back("Invalid or non-wallet transaction id"); - current_result = BumpFeeResult::INVALID_ADDRESS_OR_KEY; + current_result = Result::INVALID_ADDRESS_OR_KEY; return; } const CWalletTx& wtx = it->second; @@ -91,13 +93,13 @@ FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinCo if (!SignalsOptInRBF(*wtx.tx)) { errors.push_back("Transaction is not BIP 125 replaceable"); - current_result = BumpFeeResult::WALLET_ERROR; + current_result = Result::WALLET_ERROR; return; } 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 = BumpFeeResult::WALLET_ERROR; + current_result = Result::WALLET_ERROR; return; } @@ -105,7 +107,7 @@ FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinCo // 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 = BumpFeeResult::WALLET_ERROR; + current_result = Result::WALLET_ERROR; return; } @@ -116,7 +118,7 @@ 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 = BumpFeeResult::WALLET_ERROR; + current_result = Result::WALLET_ERROR; return; } nOutput = i; @@ -124,7 +126,7 @@ FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinCo } if (nOutput == -1) { errors.push_back("Transaction does not have a change output"); - current_result = BumpFeeResult::WALLET_ERROR; + current_result = Result::WALLET_ERROR; return; } @@ -133,7 +135,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 = BumpFeeResult::INVALID_ADDRESS_OR_KEY; + current_result = Result::INVALID_ADDRESS_OR_KEY; return; } @@ -154,14 +156,14 @@ 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 = BumpFeeResult::INVALID_PARAMETER; + current_result = Result::INVALID_PARAMETER; return; } 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 = BumpFeeResult::INVALID_PARAMETER; + current_result = Result::INVALID_PARAMETER; return; } new_fee = total_fee; @@ -185,7 +187,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 = BumpFeeResult::WALLET_ERROR; + current_result = Result::WALLET_ERROR; return; } @@ -203,7 +205,7 @@ FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinCo FormatMoney(minMempoolFeeRate.GetFeePerK()), FormatMoney(minMempoolFeeRate.GetFee(maxNewTxSize)), FormatMoney(minMempoolFeeRate.GetFeePerK()))); - current_result = BumpFeeResult::WALLET_ERROR; + current_result = Result::WALLET_ERROR; return; } @@ -215,7 +217,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 = BumpFeeResult::WALLET_ERROR; + current_result = Result::WALLET_ERROR; return; } @@ -234,7 +236,7 @@ FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinCo } } - current_result = BumpFeeResult::OK; + current_result = Result::OK; } bool FeeBumper::signTransaction(CWallet *wallet) @@ -245,13 +247,13 @@ bool FeeBumper::signTransaction(CWallet *wallet) bool FeeBumper::commit(CWallet *wallet) { AssertLockHeld(wallet->cs_wallet); - if (!errors.empty() || current_result != BumpFeeResult::OK) { + if (!errors.empty() || current_result != Result::OK) { 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"); - current_result = BumpFeeResult::MISC_ERROR; + current_result = Result::MISC_ERROR; return false; } CWalletTx& oldWtx = it->second; @@ -295,3 +297,5 @@ bool FeeBumper::commit(CWallet *wallet) return true; } +} // namespace feebumper + diff --git a/src/wallet/feebumper.h b/src/wallet/feebumper.h index 8daf80367..046bd5600 100644 --- a/src/wallet/feebumper.h +++ b/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, @@ -27,7 +29,7 @@ class FeeBumper { public: FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinControl& coin_control, CAmount total_fee); - BumpFeeResult getResult() const { return current_result; } + 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; } @@ -53,9 +55,11 @@ private: uint256 bumped_txid; CMutableTransaction mtx; std::vector errors; - BumpFeeResult current_result; + Result current_result; CAmount old_fee; CAmount new_fee; }; +} // namespace feebumper + #endif // BITCOIN_WALLET_FEEBUMPER_H diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index e9fd15aa0..8738952bf 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3125,21 +3125,21 @@ UniValue bumpfee(const JSONRPCRequest& request) LOCK2(cs_main, pwallet->cs_wallet); EnsureWalletIsUnlocked(pwallet); - FeeBumper feeBump(pwallet, hash, coin_control, totalFee); - BumpFeeResult res = feeBump.getResult(); - if (res != BumpFeeResult::OK) + feebumper::FeeBumper feeBump(pwallet, hash, coin_control, totalFee); + feebumper::Result res = feeBump.getResult(); + if (res != feebumper::Result::OK) { switch(res) { - case BumpFeeResult::INVALID_ADDRESS_OR_KEY: + case feebumper::Result::INVALID_ADDRESS_OR_KEY: throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, feeBump.getErrors()[0]); break; - case BumpFeeResult::INVALID_REQUEST: + case feebumper::Result::INVALID_REQUEST: throw JSONRPCError(RPC_INVALID_REQUEST, feeBump.getErrors()[0]); break; - case BumpFeeResult::INVALID_PARAMETER: + case feebumper::Result::INVALID_PARAMETER: throw JSONRPCError(RPC_INVALID_PARAMETER, feeBump.getErrors()[0]); break; - case BumpFeeResult::WALLET_ERROR: + case feebumper::Result::WALLET_ERROR: throw JSONRPCError(RPC_WALLET_ERROR, feeBump.getErrors()[0]); break; default: From aed1d90aca81c20c6e982ad567291f3812d47c8f Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Thu, 15 Jun 2017 10:34:17 -0400 Subject: [PATCH 3/3] [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 | 55 +++++++++----------- src/wallet/rpcwallet.cpp | 43 +++++++++------- 4 files changed, 114 insertions(+), 138 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; } +//! Return whether transaction can be bumped. +bool TransactionCanBeBumped(CWallet* wallet, const uint256& 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); +//! 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); - /* 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); +//! 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); -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; -}; +//! 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; }