Skip to content

Commit aed1d90

Browse files
ryanofskyjnewbery
authored andcommitted
[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
1 parent 37bdcca commit aed1d90

File tree

4 files changed

+117
-141
lines changed

4 files changed

+117
-141
lines changed

src/qt/walletmodel.cpp

Lines changed: 16 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -659,45 +659,39 @@ bool WalletModel::abandonTransaction(uint256 hash) const
659659

660660
bool WalletModel::transactionCanBeBumped(uint256 hash) const
661661
{
662-
LOCK2(cs_main, wallet->cs_wallet);
663-
const CWalletTx *wtx = wallet->GetWalletTx(hash);
664-
return wtx && SignalsOptInRBF(*(wtx->tx)) && !wtx->mapValue.count("replaced_by_txid");
662+
return feebumper::TransactionCanBeBumped(wallet, hash);
665663
}
666664

667665
bool WalletModel::bumpFee(uint256 hash)
668666
{
669-
std::unique_ptr<feebumper::FeeBumper> feeBump;
670-
{
671-
CCoinControl coin_control;
672-
coin_control.signalRbf = true;
673-
LOCK2(cs_main, wallet->cs_wallet);
674-
feeBump.reset(new feebumper::FeeBumper(wallet, hash, coin_control, 0));
675-
}
676-
if (feeBump->getResult() != feebumper::Result::OK)
677-
{
667+
CCoinControl coin_control;
668+
coin_control.signalRbf = true;
669+
std::vector<std::string> errors;
670+
CAmount old_fee;
671+
CAmount new_fee;
672+
CMutableTransaction mtx;
673+
if (feebumper::CreateTransaction(wallet, hash, coin_control, 0 /* totalFee */, errors, old_fee, new_fee, mtx) != feebumper::Result::OK) {
678674
QMessageBox::critical(0, tr("Fee bump error"), tr("Increasing transaction fee failed") + "<br />(" +
679-
(feeBump->getErrors().size() ? QString::fromStdString(feeBump->getErrors()[0]) : "") +")");
675+
(errors.size() ? QString::fromStdString(errors[0]) : "") +")");
680676
return false;
681677
}
682678

683679
// allow a user based fee verification
684680
QString questionString = tr("Do you want to increase the fee?");
685681
questionString.append("<br />");
686-
CAmount oldFee = feeBump->getOldFee();
687-
CAmount newFee = feeBump->getNewFee();
688682
questionString.append("<table style=\"text-align: left;\">");
689683
questionString.append("<tr><td>");
690684
questionString.append(tr("Current fee:"));
691685
questionString.append("</td><td>");
692-
questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), oldFee));
686+
questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), old_fee));
693687
questionString.append("</td></tr><tr><td>");
694688
questionString.append(tr("Increase:"));
695689
questionString.append("</td><td>");
696-
questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), newFee - oldFee));
690+
questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), new_fee - old_fee));
697691
questionString.append("</td></tr><tr><td>");
698692
questionString.append(tr("New fee:"));
699693
questionString.append("</td><td>");
700-
questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), newFee));
694+
questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), new_fee));
701695
questionString.append("</td></tr></table>");
702696
SendConfirmationDialog confirmationDialog(tr("Confirm fee bump"), questionString);
703697
confirmationDialog.exec();
@@ -715,23 +709,15 @@ bool WalletModel::bumpFee(uint256 hash)
715709
}
716710

717711
// sign bumped transaction
718-
bool res = false;
719-
{
720-
LOCK2(cs_main, wallet->cs_wallet);
721-
res = feeBump->signTransaction(wallet);
722-
}
723-
if (!res) {
712+
if (!feebumper::SignTransaction(wallet, mtx)) {
724713
QMessageBox::critical(0, tr("Fee bump error"), tr("Can't sign transaction."));
725714
return false;
726715
}
727716
// commit the bumped transaction
728-
{
729-
LOCK2(cs_main, wallet->cs_wallet);
730-
res = feeBump->commit(wallet);
731-
}
732-
if(!res) {
717+
uint256 txid;
718+
if (feebumper::CommitTransaction(wallet, hash, std::move(mtx), errors, txid) != feebumper::Result::OK) {
733719
QMessageBox::critical(0, tr("Fee bump error"), tr("Could not commit transaction") + "<br />(" +
734-
QString::fromStdString(feeBump->getErrors()[0])+")");
720+
QString::fromStdString(errors[0])+")");
735721
return false;
736722
}
737723
return true;

src/wallet/feebumper.cpp

Lines changed: 50 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -43,72 +43,72 @@ static int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWalle
4343
return GetVirtualTransactionSize(txNew);
4444
}
4545

46-
namespace feebumper {
47-
48-
bool FeeBumper::preconditionChecks(const CWallet *wallet, const CWalletTx& wtx) {
46+
//! Check whether transaction has descendant in wallet or mempool, or has been
47+
//! mined, or conflicts with a mined transaction. Return a feebumper::Result.
48+
static feebumper::Result PreconditionChecks(const CWallet* wallet, const CWalletTx& wtx, std::vector<std::string>& errors)
49+
{
4950
if (wallet->HasWalletSpend(wtx.GetHash())) {
5051
errors.push_back("Transaction has descendants in the wallet");
51-
current_result = Result::INVALID_PARAMETER;
52-
return false;
52+
return feebumper::Result::INVALID_PARAMETER;
5353
}
5454

5555
{
5656
LOCK(mempool.cs);
5757
auto it_mp = mempool.mapTx.find(wtx.GetHash());
5858
if (it_mp != mempool.mapTx.end() && it_mp->GetCountWithDescendants() > 1) {
5959
errors.push_back("Transaction has descendants in the mempool");
60-
current_result = Result::INVALID_PARAMETER;
61-
return false;
60+
return feebumper::Result::INVALID_PARAMETER;
6261
}
6362
}
6463

6564
if (wtx.GetDepthInMainChain() != 0) {
6665
errors.push_back("Transaction has been mined, or is conflicted with a mined transaction");
67-
current_result = Result::WALLET_ERROR;
68-
return false;
66+
return feebumper::Result::WALLET_ERROR;
6967
}
70-
return true;
68+
return feebumper::Result::OK;
7169
}
7270

73-
FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinControl& coin_control, CAmount total_fee)
74-
:
75-
txid(std::move(txid_in)),
76-
old_fee(0),
77-
new_fee(0)
71+
namespace feebumper {
72+
73+
bool TransactionCanBeBumped(CWallet* wallet, const uint256& txid)
7874
{
75+
LOCK2(cs_main, wallet->cs_wallet);
76+
const CWalletTx* wtx = wallet->GetWalletTx(txid);
77+
return wtx && SignalsOptInRBF(*wtx->tx) && !wtx->mapValue.count("replaced_by_txid");
78+
}
79+
80+
Result CreateTransaction(const CWallet* wallet, const uint256& txid, const CCoinControl& coin_control, CAmount total_fee, std::vector<std::string>& errors,
81+
CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx)
82+
{
83+
LOCK2(cs_main, wallet->cs_wallet);
7984
errors.clear();
80-
bumped_txid.SetNull();
81-
AssertLockHeld(wallet->cs_wallet);
8285
auto it = wallet->mapWallet.find(txid);
8386
if (it == wallet->mapWallet.end()) {
8487
errors.push_back("Invalid or non-wallet transaction id");
85-
current_result = Result::INVALID_ADDRESS_OR_KEY;
86-
return;
88+
return Result::INVALID_ADDRESS_OR_KEY;
8789
}
8890
const CWalletTx& wtx = it->second;
8991

90-
if (!preconditionChecks(wallet, wtx)) {
91-
return;
92+
Result result = PreconditionChecks(wallet, wtx, errors);
93+
if (result != Result::OK) {
94+
return result;
9295
}
9396

9497
if (!SignalsOptInRBF(*wtx.tx)) {
9598
errors.push_back("Transaction is not BIP 125 replaceable");
96-
current_result = Result::WALLET_ERROR;
97-
return;
99+
return Result::WALLET_ERROR;
98100
}
99101

100102
if (wtx.mapValue.count("replaced_by_txid")) {
101103
errors.push_back(strprintf("Cannot bump transaction %s which was already bumped by transaction %s", txid.ToString(), wtx.mapValue.at("replaced_by_txid")));
102-
current_result = Result::WALLET_ERROR;
103-
return;
104+
return Result::WALLET_ERROR;
104105
}
105106

106107
// check that original tx consists entirely of our inputs
107108
// 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)
108109
if (!wallet->IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE)) {
109110
errors.push_back("Transaction contains inputs that don't belong to this wallet");
110-
current_result = Result::WALLET_ERROR;
111-
return;
111+
return Result::WALLET_ERROR;
112112
}
113113

114114
// figure out which output was change
@@ -118,25 +118,22 @@ FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinCo
118118
if (wallet->IsChange(wtx.tx->vout[i])) {
119119
if (nOutput != -1) {
120120
errors.push_back("Transaction has multiple change outputs");
121-
current_result = Result::WALLET_ERROR;
122-
return;
121+
return Result::WALLET_ERROR;
123122
}
124123
nOutput = i;
125124
}
126125
}
127126
if (nOutput == -1) {
128127
errors.push_back("Transaction does not have a change output");
129-
current_result = Result::WALLET_ERROR;
130-
return;
128+
return Result::WALLET_ERROR;
131129
}
132130

133131
// Calculate the expected size of the new transaction.
134132
int64_t txSize = GetVirtualTransactionSize(*(wtx.tx));
135133
const int64_t maxNewTxSize = CalculateMaximumSignedTxSize(*wtx.tx, wallet);
136134
if (maxNewTxSize < 0) {
137135
errors.push_back("Transaction contains inputs that cannot be signed");
138-
current_result = Result::INVALID_ADDRESS_OR_KEY;
139-
return;
136+
return Result::INVALID_ADDRESS_OR_KEY;
140137
}
141138

142139
// calculate the old fee and fee-rate
@@ -156,15 +153,13 @@ FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinCo
156153
if (total_fee < minTotalFee) {
157154
errors.push_back(strprintf("Insufficient totalFee, must be at least %s (oldFee %s + incrementalFee %s)",
158155
FormatMoney(minTotalFee), FormatMoney(nOldFeeRate.GetFee(maxNewTxSize)), FormatMoney(::incrementalRelayFee.GetFee(maxNewTxSize))));
159-
current_result = Result::INVALID_PARAMETER;
160-
return;
156+
return Result::INVALID_PARAMETER;
161157
}
162158
CAmount requiredFee = GetRequiredFee(maxNewTxSize);
163159
if (total_fee < requiredFee) {
164160
errors.push_back(strprintf("Insufficient totalFee (cannot be less than required fee %s)",
165161
FormatMoney(requiredFee)));
166-
current_result = Result::INVALID_PARAMETER;
167-
return;
162+
return Result::INVALID_PARAMETER;
168163
}
169164
new_fee = total_fee;
170165
nNewFeeRate = CFeeRate(total_fee, maxNewTxSize);
@@ -187,8 +182,7 @@ FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinCo
187182
if (new_fee > maxTxFee) {
188183
errors.push_back(strprintf("Specified or calculated fee %s is too high (cannot be higher than maxTxFee %s)",
189184
FormatMoney(new_fee), FormatMoney(maxTxFee)));
190-
current_result = Result::WALLET_ERROR;
191-
return;
185+
return Result::WALLET_ERROR;
192186
}
193187

194188
// 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
205199
FormatMoney(minMempoolFeeRate.GetFeePerK()),
206200
FormatMoney(minMempoolFeeRate.GetFee(maxNewTxSize)),
207201
FormatMoney(minMempoolFeeRate.GetFeePerK())));
208-
current_result = Result::WALLET_ERROR;
209-
return;
202+
return Result::WALLET_ERROR;
210203
}
211204

212205
// Now modify the output to increase the fee.
@@ -217,8 +210,7 @@ FeeBumper::FeeBumper(const CWallet *wallet, const uint256 txid_in, const CCoinCo
217210
CTxOut* poutput = &(mtx.vout[nOutput]);
218211
if (poutput->nValue < nDelta) {
219212
errors.push_back("Change output is too small to bump the fee");
220-
current_result = Result::WALLET_ERROR;
221-
return;
213+
return Result::WALLET_ERROR;
222214
}
223215

224216
// 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
236228
}
237229
}
238230

239-
current_result = Result::OK;
231+
return Result::OK;
240232
}
241233

242-
bool FeeBumper::signTransaction(CWallet *wallet)
243-
{
244-
return wallet->SignTransaction(mtx);
234+
bool SignTransaction(CWallet* wallet, CMutableTransaction& mtx) {
235+
LOCK2(cs_main, wallet->cs_wallet);
236+
return wallet->SignTransaction(mtx);
245237
}
246238

247-
bool FeeBumper::commit(CWallet *wallet)
239+
Result CommitTransaction(CWallet* wallet, const uint256& txid, CMutableTransaction&& mtx, std::vector<std::string>& errors, uint256& bumped_txid)
248240
{
249-
AssertLockHeld(wallet->cs_wallet);
250-
if (!errors.empty() || current_result != Result::OK) {
251-
return false;
241+
LOCK2(cs_main, wallet->cs_wallet);
242+
if (!errors.empty()) {
243+
return Result::MISC_ERROR;
252244
}
253245
auto it = txid.IsNull() ? wallet->mapWallet.end() : wallet->mapWallet.find(txid);
254246
if (it == wallet->mapWallet.end()) {
255247
errors.push_back("Invalid or non-wallet transaction id");
256-
current_result = Result::MISC_ERROR;
257-
return false;
248+
return Result::MISC_ERROR;
258249
}
259250
CWalletTx& oldWtx = it->second;
260251

261252
// make sure the transaction still has no descendants and hasn't been mined in the meantime
262-
if (!preconditionChecks(wallet, oldWtx)) {
263-
return false;
253+
Result result = PreconditionChecks(wallet, oldWtx, errors);
254+
if (result != Result::OK) {
255+
return result;
264256
}
265257

266258
CWalletTx wtxBumped(wallet, MakeTransactionRef(std::move(mtx)));
@@ -276,14 +268,14 @@ bool FeeBumper::commit(CWallet *wallet)
276268
if (!wallet->CommitTransaction(wtxBumped, reservekey, g_connman.get(), state)) {
277269
// NOTE: CommitTransaction never returns false, so this should never happen.
278270
errors.push_back(strprintf("The transaction was rejected: %s", state.GetRejectReason()));
279-
return false;
271+
return Result::WALLET_ERROR;
280272
}
281273

282274
bumped_txid = wtxBumped.GetHash();
283275
if (state.IsInvalid()) {
284276
// This can happen if the mempool rejected the transaction. Report
285277
// what happened in the "errors" response.
286-
errors.push_back(strprintf("The transaction was rejected: %s", FormatStateMessage(state)));
278+
errors.push_back(strprintf("Error: The transaction was rejected: %s", FormatStateMessage(state)));
287279
}
288280

289281
// mark the original tx as bumped
@@ -294,7 +286,7 @@ bool FeeBumper::commit(CWallet *wallet)
294286
// replaced does not succeed for some reason.
295287
errors.push_back("Created new bumpfee transaction but could not mark the original transaction as replaced");
296288
}
297-
return true;
289+
return Result::OK;
298290
}
299291

300292
} // namespace feebumper

0 commit comments

Comments
 (0)