Make feebumper class stateless #10600

Open
wants to merge 1 commit into
from
Jump to file or symbol
Failed to load files and symbols.
+111 −119
Split
View
@@ -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<CFeeBumper> 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<std::string> 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") + "<br />(" +
- (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("<br />");
- CAmount oldFee = feeBump->getOldFee();
- CAmount newFee = feeBump->getNewFee();
questionString.append("<table style=\"text-align: left;\">");
questionString.append("<tr><td>");
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") + "<br />(" +
- QString::fromStdString(feeBump->getErrors()[0])+")");
+ QString::fromStdString(errors[0])+")");
return false;
}
return true;
View
@@ -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,70 +43,75 @@ 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<std::string>& 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;
}
{
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;
- 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<std::string>& 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,25 +121,22 @@ 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.
int64_t txSize = GetVirtualTransactionSize(*(wtx.tx));
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<std::string>& 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;
}
View
@@ -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<std::string>& 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<std::string>& 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<std::string> vErrors;
- BumpFeeResult currentResult;
- CAmount nOldFee;
- CAmount nNewFee;
+ static BumpFeeResult CommitTransaction(CWallet* pWallet,
+ const uint256& txid,
+ CMutableTransaction&& mtx,
+ std::vector<std::string>& vErrors,
+ uint256& bumpedTxid);
};
#endif // BITCOIN_WALLET_FEEBUMPER_H
Oops, something went wrong.