diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index dbdc5013f..0f186fa84 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -826,8 +826,10 @@ double CBlockPolicyEstimator::estimateConservativeFee(unsigned int doubleTarget, * estimates, however, required the 95% threshold at 2 * target be met for any * longer time horizons also. */ -CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget, FeeCalculation *feeCalc, const CTxMemPool& pool, bool conservative) const +CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget, FeeCalculation *feeCalc, bool conservative) const { + LOCK(cs_feeEstimator); + if (feeCalc) { feeCalc->desiredTarget = confTarget; feeCalc->returnedTarget = confTarget; @@ -835,80 +837,70 @@ CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget, FeeCalculation double median = -1; EstimationResult tempResult; - { - LOCK(cs_feeEstimator); - // Return failure if trying to analyze a target we're not tracking - if (confTarget <= 0 || (unsigned int)confTarget > longStats->GetMaxConfirms()) - return CFeeRate(0); + // Return failure if trying to analyze a target we're not tracking + if (confTarget <= 0 || (unsigned int)confTarget > longStats->GetMaxConfirms()) + return CFeeRate(0); - // It's not possible to get reasonable estimates for confTarget of 1 - if (confTarget == 1) - confTarget = 2; + // It's not possible to get reasonable estimates for confTarget of 1 + if (confTarget == 1) + confTarget = 2; - unsigned int maxUsableEstimate = MaxUsableEstimate(); - if (maxUsableEstimate <= 1) - return CFeeRate(0); + unsigned int maxUsableEstimate = MaxUsableEstimate(); + if (maxUsableEstimate <= 1) + return CFeeRate(0); - if ((unsigned int)confTarget > maxUsableEstimate) { - confTarget = maxUsableEstimate; - } + if ((unsigned int)confTarget > maxUsableEstimate) { + confTarget = maxUsableEstimate; + } - assert(confTarget > 0); //estimateCombinedFee and estimateConservativeFee take unsigned ints - /** true is passed to estimateCombined fee for target/2 and target so - * that we check the max confirms for shorter time horizons as well. - * This is necessary to preserve monotonically increasing estimates. - * For non-conservative estimates we do the same thing for 2*target, but - * for conservative estimates we want to skip these shorter horizons - * checks for 2*target because we are taking the max over all time - * horizons so we already have monotonically increasing estimates and - * the purpose of conservative estimates is not to let short term - * fluctuations lower our estimates by too much. - */ - double halfEst = estimateCombinedFee(confTarget/2, HALF_SUCCESS_PCT, true, &tempResult); + assert(confTarget > 0); //estimateCombinedFee and estimateConservativeFee take unsigned ints + /** true is passed to estimateCombined fee for target/2 and target so + * that we check the max confirms for shorter time horizons as well. + * This is necessary to preserve monotonically increasing estimates. + * For non-conservative estimates we do the same thing for 2*target, but + * for conservative estimates we want to skip these shorter horizons + * checks for 2*target because we are taking the max over all time + * horizons so we already have monotonically increasing estimates and + * the purpose of conservative estimates is not to let short term + * fluctuations lower our estimates by too much. + */ + double halfEst = estimateCombinedFee(confTarget/2, HALF_SUCCESS_PCT, true, &tempResult); + if (feeCalc) { + feeCalc->est = tempResult; + feeCalc->reason = FeeReason::HALF_ESTIMATE; + } + median = halfEst; + double actualEst = estimateCombinedFee(confTarget, SUCCESS_PCT, true, &tempResult); + if (actualEst > median) { + median = actualEst; if (feeCalc) { feeCalc->est = tempResult; - feeCalc->reason = FeeReason::HALF_ESTIMATE; + feeCalc->reason = FeeReason::FULL_ESTIMATE; } - median = halfEst; - double actualEst = estimateCombinedFee(confTarget, SUCCESS_PCT, true, &tempResult); - if (actualEst > median) { - median = actualEst; - if (feeCalc) { - feeCalc->est = tempResult; - feeCalc->reason = FeeReason::FULL_ESTIMATE; - } + } + double doubleEst = estimateCombinedFee(2 * confTarget, DOUBLE_SUCCESS_PCT, !conservative, &tempResult); + if (doubleEst > median) { + median = doubleEst; + if (feeCalc) { + feeCalc->est = tempResult; + feeCalc->reason = FeeReason::DOUBLE_ESTIMATE; } - double doubleEst = estimateCombinedFee(2 * confTarget, DOUBLE_SUCCESS_PCT, !conservative, &tempResult); - if (doubleEst > median) { - median = doubleEst; + } + + if (conservative || median == -1) { + double consEst = estimateConservativeFee(2 * confTarget, &tempResult); + if (consEst > median) { + median = consEst; if (feeCalc) { feeCalc->est = tempResult; - feeCalc->reason = FeeReason::DOUBLE_ESTIMATE; + feeCalc->reason = FeeReason::CONSERVATIVE; } } - - if (conservative || median == -1) { - double consEst = estimateConservativeFee(2 * confTarget, &tempResult); - if (consEst > median) { - median = consEst; - if (feeCalc) { - feeCalc->est = tempResult; - feeCalc->reason = FeeReason::CONSERVATIVE; - } - } - } - } // Must unlock cs_feeEstimator before taking mempool locks + } if (feeCalc) feeCalc->returnedTarget = confTarget; - // If mempool is limiting txs , return at least the min feerate from the mempool - CAmount minPoolFee = pool.GetMinFee(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFeePerK(); - if (minPoolFee > 0 && minPoolFee > median) { - if (feeCalc) feeCalc->reason = FeeReason::MEMPOOL_MIN; - return CFeeRate(minPoolFee); - } - if (median < 0) return CFeeRate(0); diff --git a/src/policy/fees.h b/src/policy/fees.h index 4c80371c5..f4ef79364 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -208,7 +208,7 @@ public: * the closest target where one can be given. 'conservative' estimates are * valid over longer time horizons also. */ - CFeeRate estimateSmartFee(int confTarget, FeeCalculation *feeCalc, const CTxMemPool& pool, bool conservative) const; + CFeeRate estimateSmartFee(int confTarget, FeeCalculation *feeCalc, bool conservative) const; /** Return a specific fee estimate calculation with a given success * threshold and time horizon, and optionally return detailed data about diff --git a/src/qt/coincontroldialog.cpp b/src/qt/coincontroldialog.cpp index c19420beb..f3ee0fbe3 100644 --- a/src/qt/coincontroldialog.cpp +++ b/src/qt/coincontroldialog.cpp @@ -490,8 +490,6 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog) else nBytesInputs += 148; } - bool conservative_estimate = CalculateEstimateType(FeeEstimateMode::UNSET, coinControl->signalRbf); - // calculation if (nQuantity > 0) { @@ -512,7 +510,7 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog) nBytes -= 34; // Fee - nPayFee = CWallet::GetMinimumFee(nBytes, coinControl->nConfirmTarget, ::mempool, ::feeEstimator, nullptr /* FeeCalculation */, false /* ignoreGlobalPayTxFee */, conservative_estimate); + nPayFee = CWallet::GetMinimumFee(nBytes, *coinControl, ::mempool, ::feeEstimator, nullptr /* FeeCalculation */); if (nPayAmount > 0) { @@ -583,12 +581,8 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog) QString toolTipDust = tr("This label turns red if any recipient receives an amount smaller than the current dust threshold."); // how many satoshis the estimated fee can vary per byte we guess wrong - double dFeeVary; - if (payTxFee.GetFeePerK() > 0) - dFeeVary = (double)std::max(CWallet::GetRequiredFee(1000), payTxFee.GetFeePerK()) / 1000; - else { - dFeeVary = (double)std::max(CWallet::GetRequiredFee(1000), ::feeEstimator.estimateSmartFee(coinControl->nConfirmTarget, NULL, ::mempool, conservative_estimate).GetFeePerK()) / 1000; - } + double dFeeVary = (double)nPayFee / nBytes; + QString toolTip4 = tr("Can vary +/- %1 satoshi(s) per input.").arg(dFeeVary); l3->setToolTip(toolTip4); diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index 86401d3bb..a01886c3e 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -175,18 +175,13 @@ void SendCoinsDialog::setModel(WalletModel *_model) ui->confTargetSelector->addItem(tr("%1 (%2 blocks)").arg(GUIUtil::formatNiceTimeOffset(n*Params().GetConsensus().nPowTargetSpacing)).arg(n)); } connect(ui->confTargetSelector, SIGNAL(currentIndexChanged(int)), this, SLOT(updateSmartFeeLabel())); - connect(ui->confTargetSelector, SIGNAL(currentIndexChanged(int)), this, SLOT(updateGlobalFeeVariables())); connect(ui->confTargetSelector, SIGNAL(currentIndexChanged(int)), this, SLOT(coinControlUpdateLabels())); connect(ui->groupFee, SIGNAL(buttonClicked(int)), this, SLOT(updateFeeSectionControls())); - connect(ui->groupFee, SIGNAL(buttonClicked(int)), this, SLOT(updateGlobalFeeVariables())); connect(ui->groupFee, SIGNAL(buttonClicked(int)), this, SLOT(coinControlUpdateLabels())); - connect(ui->groupCustomFee, SIGNAL(buttonClicked(int)), this, SLOT(updateGlobalFeeVariables())); connect(ui->groupCustomFee, SIGNAL(buttonClicked(int)), this, SLOT(coinControlUpdateLabels())); - connect(ui->customFee, SIGNAL(valueChanged()), this, SLOT(updateGlobalFeeVariables())); connect(ui->customFee, SIGNAL(valueChanged()), this, SLOT(coinControlUpdateLabels())); connect(ui->checkBoxMinimumFee, SIGNAL(stateChanged(int)), this, SLOT(setMinimumFee())); connect(ui->checkBoxMinimumFee, SIGNAL(stateChanged(int)), this, SLOT(updateFeeSectionControls())); - connect(ui->checkBoxMinimumFee, SIGNAL(stateChanged(int)), this, SLOT(updateGlobalFeeVariables())); connect(ui->checkBoxMinimumFee, SIGNAL(stateChanged(int)), this, SLOT(coinControlUpdateLabels())); connect(ui->optInRBF, SIGNAL(stateChanged(int)), this, SLOT(updateSmartFeeLabel())); connect(ui->optInRBF, SIGNAL(stateChanged(int)), this, SLOT(coinControlUpdateLabels())); @@ -194,7 +189,6 @@ void SendCoinsDialog::setModel(WalletModel *_model) updateFeeSectionControls(); updateMinFeeLabel(); updateSmartFeeLabel(); - updateGlobalFeeVariables(); // set default rbf checkbox state ui->optInRBF->setCheckState(model->getDefaultWalletRbf() ? Qt::Checked : Qt::Unchecked); @@ -274,14 +268,10 @@ void SendCoinsDialog::on_sendButton_clicked() CCoinControl ctrl; if (model->getOptionsModel()->getCoinControlFeatures()) ctrl = *CoinControlDialog::coinControl; - if (ui->radioSmartFee->isChecked()) - ctrl.nConfirmTarget = getConfTargetForIndex(ui->confTargetSelector->currentIndex()); - else - ctrl.nConfirmTarget = 0; - ctrl.signalRbf = ui->optInRBF->isChecked(); + updateCoinControlState(ctrl); - prepareStatus = model->prepareTransaction(currentTransaction, &ctrl); + prepareStatus = model->prepareTransaction(currentTransaction, ctrl); // process prepareStatus and on error generate message shown to user processSendCoinsReturn(prepareStatus, @@ -636,18 +626,6 @@ void SendCoinsDialog::updateFeeSectionControls() ui->customFee ->setEnabled(ui->radioCustomFee->isChecked() && !ui->checkBoxMinimumFee->isChecked()); } -void SendCoinsDialog::updateGlobalFeeVariables() -{ - if (ui->radioSmartFee->isChecked()) - { - payTxFee = CFeeRate(0); - } - else - { - payTxFee = CFeeRate(ui->customFee->value()); - } -} - void SendCoinsDialog::updateFeeMinimizedLabel() { if(!model || !model->getOptionsModel()) @@ -669,19 +647,32 @@ void SendCoinsDialog::updateMinFeeLabel() ); } +void SendCoinsDialog::updateCoinControlState(CCoinControl& ctrl) +{ + if (ui->radioCustomFee->isChecked()) { + ctrl.m_feerate = CFeeRate(ui->customFee->value()); + } else { + ctrl.m_feerate.reset(); + } + // Avoid using global defaults when sending money from the GUI + // Either custom fee will be used or if not selected, the confirmation target from dropdown box + ctrl.m_confirm_target = getConfTargetForIndex(ui->confTargetSelector->currentIndex()); + ctrl.signalRbf = ui->optInRBF->isChecked(); +} + void SendCoinsDialog::updateSmartFeeLabel() { if(!model || !model->getOptionsModel()) return; - - int nBlocksToConfirm = getConfTargetForIndex(ui->confTargetSelector->currentIndex()); + CCoinControl coin_control; + updateCoinControlState(coin_control); + coin_control.m_feerate.reset(); // Explicitly use only fee estimation rate for smart fee labels FeeCalculation feeCalc; - bool conservative_estimate = CalculateEstimateType(FeeEstimateMode::UNSET, ui->optInRBF->isChecked()); - CFeeRate feeRate = ::feeEstimator.estimateSmartFee(nBlocksToConfirm, &feeCalc, ::mempool, conservative_estimate); - if (feeRate <= CFeeRate(0)) // not enough data => minfee - { - ui->labelSmartFee->setText(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), - std::max(CWallet::fallbackFee.GetFeePerK(), CWallet::GetRequiredFee(1000))) + "/kB"); + CFeeRate feeRate = CFeeRate(CWallet::GetMinimumFee(1000, coin_control, ::mempool, ::feeEstimator, &feeCalc)); + + ui->labelSmartFee->setText(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), feeRate.GetFeePerK()) + "/kB"); + + if (feeCalc.reason == FeeReason::FALLBACK) { ui->labelSmartFee2->show(); // (Smart fee not initialized yet. This usually takes a few blocks...) ui->labelFeeEstimation->setText(""); ui->fallbackFeeWarningLabel->setVisible(true); @@ -692,8 +683,6 @@ void SendCoinsDialog::updateSmartFeeLabel() } else { - ui->labelSmartFee->setText(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), - std::max(feeRate.GetFeePerK(), CWallet::GetRequiredFee(1000))) + "/kB"); ui->labelSmartFee2->hide(); ui->labelFeeEstimation->setText(tr("Estimated to begin confirmation within %n block(s).", "", feeCalc.returnedTarget)); ui->fallbackFeeWarningLabel->setVisible(false); @@ -752,8 +741,6 @@ void SendCoinsDialog::coinControlFeatureChanged(bool checked) if (!checked && model) // coin control features disabled CoinControlDialog::coinControl->SetNull(); - // make sure we set back the confirmation target - updateGlobalFeeVariables(); coinControlUpdateLabels(); } @@ -844,15 +831,11 @@ void SendCoinsDialog::coinControlUpdateLabels() if (!model || !model->getOptionsModel()) return; + updateCoinControlState(*CoinControlDialog::coinControl); + // set pay amounts CoinControlDialog::payAmounts.clear(); CoinControlDialog::fSubtractFeeFromAmount = false; - if (ui->radioSmartFee->isChecked()) { - CoinControlDialog::coinControl->nConfirmTarget = getConfTargetForIndex(ui->confTargetSelector->currentIndex()); - } else { - CoinControlDialog::coinControl->nConfirmTarget = model->getDefaultConfirmTarget(); - } - CoinControlDialog::coinControl->signalRbf = ui->optInRBF->isChecked(); for(int i = 0; i < ui->entries->count(); ++i) { diff --git a/src/qt/sendcoinsdialog.h b/src/qt/sendcoinsdialog.h index ff7040ac5..70b4aa5a0 100644 --- a/src/qt/sendcoinsdialog.h +++ b/src/qt/sendcoinsdialog.h @@ -68,6 +68,8 @@ private: void processSendCoinsReturn(const WalletModel::SendCoinsReturn &sendCoinsReturn, const QString &msgArg = QString()); void minimizeFeeSection(bool fMinimize); void updateFeeMinimizedLabel(); + // Update the passed in CCoinControl with state from the GUI + void updateCoinControlState(CCoinControl& ctrl); private Q_SLOTS: void on_sendButton_clicked(); @@ -91,7 +93,6 @@ private Q_SLOTS: void updateFeeSectionControls(); void updateMinFeeLabel(); void updateSmartFeeLabel(); - void updateGlobalFeeVariables(); Q_SIGNALS: // Fired when a message should be reported to the user diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 60b55da3e..ba0e1da0c 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -24,6 +24,7 @@ #include "sync.h" #include "ui_interface.h" #include "util.h" // for GetBoolArg +#include "wallet/coincontrol.h" #include "wallet/feebumper.h" #include "wallet/wallet.h" #include "wallet/walletdb.h" // for BackupWallet @@ -191,7 +192,7 @@ bool WalletModel::validateAddress(const QString &address) return addressParsed.IsValid(); } -WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransaction &transaction, const CCoinControl *coinControl) +WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransaction &transaction, const CCoinControl& coinControl) { CAmount total = 0; bool fSubtractFeeFromAmount = false; @@ -258,7 +259,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact return DuplicateAddress; } - CAmount nBalance = getBalance(coinControl); + CAmount nBalance = getBalance(&coinControl); if(total > nBalance) { @@ -667,8 +668,10 @@ 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 CFeeBumper(wallet, hash, nTxConfirmTarget, false, 0, true, FeeEstimateMode::UNSET)); + feeBump.reset(new CFeeBumper(wallet, hash, coin_control, 0)); } if (feeBump->getResult() != BumpFeeResult::OK) { diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 16b0caed4..5258dc669 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -154,7 +154,7 @@ public: }; // prepare transaction for getting txfee before sending coins - SendCoinsReturn prepareTransaction(WalletModelTransaction &transaction, const CCoinControl *coinControl = NULL); + SendCoinsReturn prepareTransaction(WalletModelTransaction &transaction, const CCoinControl& coinControl); // Send coins to a list of recipients SendCoinsReturn sendCoins(WalletModelTransaction &transaction); diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 10bb341e5..b8c94d32e 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -30,6 +30,16 @@ #include +unsigned int ParseConfirmTarget(const UniValue& value) +{ + int target = value.get_int(); + unsigned int max_target = ::feeEstimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE); + if (target < 1 || (unsigned int)target > max_target) { + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid conf_target, must be between %u - %u", 1, max_target)); + } + return (unsigned int)target; +} + /** * Return average network hashes per second based on the last 'lookup' blocks, * or from the last difficulty change if 'lookup' is nonpositive. @@ -815,7 +825,6 @@ UniValue estimatesmartfee(const JSONRPCRequest& request) "\n" "A negative value is returned if not enough transactions and blocks\n" "have been observed to make an estimate for any number of blocks.\n" - "However it will not return a value below the mempool reject fee.\n" "\nExample:\n" + HelpExampleCli("estimatesmartfee", "6") ); @@ -831,7 +840,7 @@ UniValue estimatesmartfee(const JSONRPCRequest& request) UniValue result(UniValue::VOBJ); FeeCalculation feeCalc; - CFeeRate feeRate = ::feeEstimator.estimateSmartFee(nBlocks, &feeCalc, ::mempool, conservative); + CFeeRate feeRate = ::feeEstimator.estimateSmartFee(nBlocks, &feeCalc, conservative); result.push_back(Pair("feerate", feeRate == CFeeRate(0) ? -1.0 : ValueFromAmount(feeRate.GetFeePerK()))); result.push_back(Pair("blocks", feeCalc.returnedTarget)); return result; diff --git a/src/rpc/mining.h b/src/rpc/mining.h index a148d851d..868d7002b 100644 --- a/src/rpc/mining.h +++ b/src/rpc/mining.h @@ -12,4 +12,7 @@ /** Generate blocks (mine) */ UniValue generateBlocks(std::shared_ptr coinbaseScript, int nGenerate, uint64_t nMaxTries, bool keepScript); +/** Check bounds on a command line confirm target */ +unsigned int ParseConfirmTarget(const UniValue& value); + #endif diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp index 8cdd39210..fd8f7191f 100644 --- a/src/test/policyestimator_tests.cpp +++ b/src/test/policyestimator_tests.cpp @@ -177,16 +177,6 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) for (int i = 2; i < 9; i++) { // At 9, the original estimate was already at the bottom (b/c scale = 2) BOOST_CHECK(feeEst.estimateFee(i).GetFeePerK() < origFeeEst[i-1] - deltaFee); } - - // Test that if the mempool is limited, estimateSmartFee won't return a value below the mempool min fee - mpool.addUnchecked(tx.GetHash(), entry.Fee(feeV[5]).Time(GetTime()).Height(blocknum).FromTx(tx)); - // evict that transaction which should set a mempool min fee of minRelayTxFee + feeV[5] - mpool.TrimToSize(1); - BOOST_CHECK(mpool.GetMinFee(1).GetFeePerK() > feeV[5]); - for (int i = 1; i < 10; i++) { - BOOST_CHECK(feeEst.estimateSmartFee(i, NULL, mpool, true).GetFeePerK() >= feeEst.estimateRawFee(i, 0.85, FeeEstimateHorizon::MED_HALFLIFE).GetFeePerK()); - BOOST_CHECK(feeEst.estimateSmartFee(i, NULL, mpool, true).GetFeePerK() >= mpool.GetMinFee(1).GetFeePerK()); - } } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h index bdd01bec1..fc0e7c519 100644 --- a/src/wallet/coincontrol.h +++ b/src/wallet/coincontrol.h @@ -10,6 +10,8 @@ #include "primitives/transaction.h" #include "wallet/wallet.h" +#include + /** Coin Control Features. */ class CCoinControl { @@ -19,12 +21,12 @@ public: bool fAllowOtherInputs; //! Includes watch only addresses which match the ISMINE_WATCH_SOLVABLE criteria bool fAllowWatchOnly; - //! Override estimated feerate + //! Override automatic min/max checks on fee, m_feerate must be set if true bool fOverrideFeeRate; - //! Feerate to use if overrideFeeRate is true - CFeeRate nFeeRate; - //! Override the default confirmation target, 0 = use default - int nConfirmTarget; + //! Override the default payTxFee if set + boost::optional m_feerate; + //! Override the default confirmation target if set + boost::optional m_confirm_target; //! Signal BIP-125 replace by fee. bool signalRbf; //! Fee estimation mode to control arguments to estimateSmartFee @@ -41,9 +43,9 @@ public: fAllowOtherInputs = false; fAllowWatchOnly = false; setSelected.clear(); - nFeeRate = CFeeRate(0); + m_feerate.reset(); fOverrideFeeRate = false; - nConfirmTarget = 0; + m_confirm_target.reset(); signalRbf = fWalletRbf; m_fee_mode = FeeEstimateMode::UNSET; } diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 607ecf418..4bfd8726a 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -3,6 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include "consensus/validation.h" +#include "wallet/coincontrol.h" #include "wallet/feebumper.h" #include "wallet/wallet.h" #include "policy/fees.h" @@ -66,7 +67,7 @@ bool CFeeBumper::preconditionChecks(const CWallet *pWallet, const CWalletTx& wtx return true; } -CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, int newConfirmTarget, bool ignoreGlobalPayTxFee, CAmount totalFee, bool newTxReplaceable, FeeEstimateMode fee_mode) +CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoinControl& coin_control, CAmount totalFee) : txid(std::move(txidIn)), nOldFee(0), @@ -165,8 +166,7 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, int newConf nNewFee = totalFee; nNewFeeRate = CFeeRate(totalFee, maxNewTxSize); } else { - bool conservative_estimate = CalculateEstimateType(fee_mode, newTxReplaceable); - nNewFee = CWallet::GetMinimumFee(maxNewTxSize, newConfirmTarget, mempool, ::feeEstimator, nullptr /* FeeCalculation */, ignoreGlobalPayTxFee, conservative_estimate); + nNewFee = CWallet::GetMinimumFee(maxNewTxSize, coin_control, mempool, ::feeEstimator, nullptr /* FeeCalculation */); nNewFeeRate = CFeeRate(nNewFee, maxNewTxSize); // New fee rate must be at least old rate + minimum incremental relay rate @@ -221,7 +221,7 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, int newConf } // Mark new tx not replaceable, if requested. - if (!newTxReplaceable) { + if (!coin_control.signalRbf) { for (auto& input : mtx.vin) { if (input.nSequence < 0xfffffffe) input.nSequence = 0xfffffffe; } diff --git a/src/wallet/feebumper.h b/src/wallet/feebumper.h index 11e2f5f95..3d64e53c1 100644 --- a/src/wallet/feebumper.h +++ b/src/wallet/feebumper.h @@ -10,6 +10,7 @@ class CWallet; class CWalletTx; class uint256; +class CCoinControl; enum class FeeEstimateMode; enum class BumpFeeResult @@ -25,7 +26,7 @@ enum class BumpFeeResult class CFeeBumper { public: - CFeeBumper(const CWallet *pWalletIn, const uint256 txidIn, int newConfirmTarget, bool ignoreGlobalPayTxFee, CAmount totalFee, bool newTxReplaceable, FeeEstimateMode fee_mode); + 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; } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 873542a96..ee8c7548f 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -356,7 +356,7 @@ UniValue getaddressesbyaccount(const JSONRPCRequest& request) return ret; } -static void SendMoney(CWallet * const pwallet, const CTxDestination &address, CAmount nValue, bool fSubtractFeeFromAmount, CWalletTx& wtxNew, CCoinControl *coin_control = nullptr) +static void SendMoney(CWallet * const pwallet, const CTxDestination &address, CAmount nValue, bool fSubtractFeeFromAmount, CWalletTx& wtxNew, const CCoinControl& coin_control) { CAmount curBalance = pwallet->GetBalance(); @@ -460,7 +460,7 @@ UniValue sendtoaddress(const JSONRPCRequest& request) } if (request.params.size() > 6 && !request.params[6].isNull()) { - coin_control.nConfirmTarget = request.params[6].get_int(); + coin_control.m_confirm_target = ParseConfirmTarget(request.params[6]); } if (request.params.size() > 7 && !request.params[7].isNull()) { @@ -472,7 +472,7 @@ UniValue sendtoaddress(const JSONRPCRequest& request) EnsureWalletIsUnlocked(pwallet); - SendMoney(pwallet, address.Get(), nAmount, fSubtractFeeFromAmount, wtx, &coin_control); + SendMoney(pwallet, address.Get(), nAmount, fSubtractFeeFromAmount, wtx, coin_control); return wtx.GetHash().GetHex(); } @@ -898,7 +898,8 @@ UniValue sendfrom(const JSONRPCRequest& request) if (nAmount > nBalance) throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Account has insufficient funds"); - SendMoney(pwallet, address.Get(), nAmount, false, wtx); + CCoinControl no_coin_control; // This is a deprecated API + SendMoney(pwallet, address.Get(), nAmount, false, wtx, no_coin_control); return wtx.GetHash().GetHex(); } @@ -980,7 +981,7 @@ UniValue sendmany(const JSONRPCRequest& request) } if (request.params.size() > 6 && !request.params[6].isNull()) { - coin_control.nConfirmTarget = request.params[6].get_int(); + coin_control.m_confirm_target = ParseConfirmTarget(request.params[6]); } if (request.params.size() > 7 && !request.params[7].isNull()) { @@ -1033,7 +1034,7 @@ UniValue sendmany(const JSONRPCRequest& request) CAmount nFeeRequired = 0; int nChangePosRet = -1; std::string strFailReason; - bool fCreated = pwallet->CreateTransaction(vecSend, wtx, keyChange, nFeeRequired, nChangePosRet, strFailReason, &coin_control); + bool fCreated = pwallet->CreateTransaction(vecSend, wtx, keyChange, nFeeRequired, nChangePosRet, strFailReason, coin_control); if (!fCreated) throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, strFailReason); CValidationState state; @@ -2729,13 +2730,9 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) RPCTypeCheck(request.params, {UniValue::VSTR}); CCoinControl coinControl; - coinControl.destChange = CNoDestination(); int changePosition = -1; - coinControl.fAllowWatchOnly = false; // include watching bool lockUnspents = false; bool reserveChangeKey = true; - coinControl.nFeeRate = CFeeRate(0); - coinControl.fOverrideFeeRate = false; UniValue subtractFeeFromOutputs; std::set setSubtractFeeFromOutputs; @@ -2787,7 +2784,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) if (options.exists("feeRate")) { - coinControl.nFeeRate = CFeeRate(AmountFromValue(options["feeRate"])); + coinControl.m_feerate = CFeeRate(AmountFromValue(options["feeRate"])); coinControl.fOverrideFeeRate = true; } @@ -2798,7 +2795,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) coinControl.signalRbf = options["replaceable"].get_bool(); } if (options.exists("conf_target")) { - coinControl.nConfirmTarget = options["conf_target"].get_int(); + coinControl.m_confirm_target = ParseConfirmTarget(options["conf_target"]); } if (options.exists("estimate_mode")) { if (!FeeModeFromString(options["estimate_mode"].get_str(), coinControl.m_fee_mode)) { @@ -2904,11 +2901,9 @@ UniValue bumpfee(const JSONRPCRequest& request) hash.SetHex(request.params[0].get_str()); // optional parameters - bool ignoreGlobalPayTxFee = false; - int newConfirmTarget = nTxConfirmTarget; CAmount totalFee = 0; - bool replaceable = true; - FeeEstimateMode fee_mode = FeeEstimateMode::UNSET; + CCoinControl coin_control; + coin_control.signalRbf = true; if (request.params.size() > 1) { UniValue options = request.params[1]; RPCTypeCheckObj(options, @@ -2922,15 +2917,8 @@ UniValue bumpfee(const JSONRPCRequest& request) if (options.exists("confTarget") && options.exists("totalFee")) { throw JSONRPCError(RPC_INVALID_PARAMETER, "confTarget and totalFee options should not both be set. Please provide either a confirmation target for fee estimation or an explicit total fee for the transaction."); - } else if (options.exists("confTarget")) { - // If the user has explicitly set a confTarget in this rpc call, - // then override the default logic that uses the global payTxFee - // instead of the confirmation target. - ignoreGlobalPayTxFee = true; - newConfirmTarget = options["confTarget"].get_int(); - if (newConfirmTarget <= 0) { // upper-bound will be checked by estimatefee/smartfee - throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid confTarget (cannot be <= 0)"); - } + } else if (options.exists("confTarget")) { // TODO: alias this to conf_target + coin_control.m_confirm_target = ParseConfirmTarget(options["confTarget"]); } else if (options.exists("totalFee")) { totalFee = options["totalFee"].get_int64(); if (totalFee <= 0) { @@ -2939,10 +2927,10 @@ UniValue bumpfee(const JSONRPCRequest& request) } if (options.exists("replaceable")) { - replaceable = options["replaceable"].get_bool(); + coin_control.signalRbf = options["replaceable"].get_bool(); } if (options.exists("estimate_mode")) { - if (!FeeModeFromString(options["estimate_mode"].get_str(), fee_mode)) { + if (!FeeModeFromString(options["estimate_mode"].get_str(), coin_control.m_fee_mode)) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid estimate_mode parameter"); } } @@ -2951,7 +2939,7 @@ UniValue bumpfee(const JSONRPCRequest& request) LOCK2(cs_main, pwallet->cs_wallet); EnsureWalletIsUnlocked(pwallet); - CFeeBumper feeBump(pwallet, hash, newConfirmTarget, ignoreGlobalPayTxFee, totalFee, replaceable, fee_mode); + CFeeBumper feeBump(pwallet, hash, coin_control, totalFee); BumpFeeResult res = feeBump.getResult(); if (res != BumpFeeResult::OK) { diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 96a1b14b6..8176a0017 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -13,6 +13,7 @@ #include "rpc/server.h" #include "test/test_bitcoin.h" #include "validation.h" +#include "wallet/coincontrol.h" #include "wallet/test/wallet_test_fixture.h" #include @@ -617,7 +618,8 @@ public: CAmount fee; int changePos = -1; std::string error; - BOOST_CHECK(wallet->CreateTransaction({recipient}, wtx, reservekey, fee, changePos, error)); + CCoinControl dummy; + BOOST_CHECK(wallet->CreateTransaction({recipient}, wtx, reservekey, fee, changePos, error, dummy)); CValidationState state; BOOST_CHECK(wallet->CommitTransaction(wtx, reservekey, nullptr, state)); auto it = wallet->mapWallet.find(wtx.GetHash()); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6cf4c53da..13da7bb2c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2469,9 +2469,9 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC CReserveKey reservekey(this); CWalletTx wtx; - if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, &coinControl, false)) + if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, coinControl, false)) { return false; - + } if (nChangePosInOut != -1) tx.vout.insert(tx.vout.begin() + nChangePosInOut, wtx.tx->vout[nChangePosInOut]); @@ -2502,7 +2502,7 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC } bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet, - int& nChangePosInOut, std::string& strFailReason, const CCoinControl* coinControl, bool sign) + int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign) { CAmount nValue = 0; int nChangePosRequest = nChangePosInOut; @@ -2567,7 +2567,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT LOCK2(cs_main, cs_wallet); { std::vector vAvailableCoins; - AvailableCoins(vAvailableCoins, true, coinControl); + AvailableCoins(vAvailableCoins, true, &coin_control); // Create change script that will be used if we need change // TODO: pass in scriptChange instead of reservekey so @@ -2575,12 +2575,9 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT CScript scriptChange; // coin control: send change to custom address - if (coinControl && !boost::get(&coinControl->destChange)) - scriptChange = GetScriptForDestination(coinControl->destChange); - - // no coin control: send change to newly generated address - else - { + if (!boost::get(&coin_control.destChange)) { + scriptChange = GetScriptForDestination(coin_control.destChange); + } else { // no coin control: send change to newly generated address // Note: We use a new key here to keep it from being obvious which side is the change. // The drawback is that by not reusing a previous key, the change may be lost if a // backup is restored, if the backup doesn't have the new private key for the change. @@ -2654,7 +2651,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT if (pick_new_inputs) { nValueIn = 0; setCoins.clear(); - if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coinControl)) + if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, &coin_control)) { strFailReason = _("Insufficient funds"); return false; @@ -2705,8 +2702,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT // to avoid conflicting with other possible uses of nSequence, // and in the spirit of "smallest possible change from prior // behavior." - bool rbf = coinControl ? coinControl->signalRbf : fWalletRbf; - const uint32_t nSequence = rbf ? MAX_BIP125_RBF_SEQUENCE : (std::numeric_limits::max() - 1); + const uint32_t nSequence = coin_control.signalRbf ? MAX_BIP125_RBF_SEQUENCE : (std::numeric_limits::max() - 1); for (const auto& coin : setCoins) txNew.vin.push_back(CTxIn(coin.outpoint,CScript(), nSequence)); @@ -2725,17 +2721,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT vin.scriptWitness.SetNull(); } - // Allow to override the default confirmation target over the CoinControl instance - int currentConfirmationTarget = nTxConfirmTarget; - if (coinControl && coinControl->nConfirmTarget > 0) - currentConfirmationTarget = coinControl->nConfirmTarget; - - // Allow to override the default fee estimate mode over the CoinControl instance - bool conservative_estimate = CalculateEstimateType(coinControl ? coinControl->m_fee_mode : FeeEstimateMode::UNSET, rbf); - - CAmount nFeeNeeded = GetMinimumFee(nBytes, currentConfirmationTarget, ::mempool, ::feeEstimator, &feeCalc, false /* ignoreGlobalPayTxFee */, conservative_estimate); - if (coinControl && coinControl->fOverrideFeeRate) - nFeeNeeded = coinControl->nFeeRate.GetFee(nBytes); + CAmount nFeeNeeded = GetMinimumFee(nBytes, coin_control, ::mempool, ::feeEstimator, &feeCalc); // If we made it here and we aren't even able to meet the relay fee on the next pass, give up // because we must be at the maximum allowed fee. @@ -2760,7 +2746,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT // new inputs. We now know we only need the smaller fee // (because of reduced tx size) and so we should add a // change output. Only try this once. - CAmount fee_needed_for_change = GetMinimumFee(change_prototype_size, currentConfirmationTarget, ::mempool, ::feeEstimator, nullptr, false /* ignoreGlobalPayTxFee */, conservative_estimate); + CAmount fee_needed_for_change = GetMinimumFee(change_prototype_size, coin_control, ::mempool, ::feeEstimator, nullptr); CAmount minimum_value_for_change = GetDustThreshold(change_prototype_txout, ::dustRelayFee); CAmount max_excess_fee = fee_needed_for_change + minimum_value_for_change; if (nFeeRet > nFeeNeeded + max_excess_fee && nChangePosInOut == -1 && nSubtractFeeFromAmount == 0 && pick_new_inputs) { @@ -2936,33 +2922,61 @@ CAmount CWallet::GetRequiredFee(unsigned int nTxBytes) return std::max(minTxFee.GetFee(nTxBytes), ::minRelayTxFee.GetFee(nTxBytes)); } -CAmount CWallet::GetMinimumFee(unsigned int nTxBytes, unsigned int nConfirmTarget, const CTxMemPool& pool, const CBlockPolicyEstimator& estimator, FeeCalculation *feeCalc, bool ignoreGlobalPayTxFee, bool conservative_estimate) +CAmount CWallet::GetMinimumFee(unsigned int nTxBytes, const CCoinControl& coin_control, const CTxMemPool& pool, const CBlockPolicyEstimator& estimator, FeeCalculation *feeCalc) { - // payTxFee is the user-set global for desired feerate - CAmount nFeeNeeded = payTxFee.GetFee(nTxBytes); - // User didn't set: use -txconfirmtarget to estimate... - if (nFeeNeeded == 0 || ignoreGlobalPayTxFee) { - nFeeNeeded = estimator.estimateSmartFee(nConfirmTarget, feeCalc, pool, conservative_estimate).GetFee(nTxBytes); - // ... unless we don't have enough mempool data for estimatefee, then use fallbackFee - if (nFeeNeeded == 0) { - nFeeNeeded = fallbackFee.GetFee(nTxBytes); + /* User control of how to calculate fee uses the following parameter precedence: + 1. coin_control.m_feerate + 2. coin_control.m_confirm_target + 3. payTxFee (user-set global variable) + 4. nTxConfirmTarget (user-set global variable) + The first parameter that is set is used. + */ + CAmount fee_needed; + if (coin_control.m_feerate) { // 1. + fee_needed = coin_control.m_feerate->GetFee(nTxBytes); + if (feeCalc) feeCalc->reason = FeeReason::PAYTXFEE; + // Allow to override automatic min/max check over coin control instance + if (coin_control.fOverrideFeeRate) return fee_needed; + } + else if (!coin_control.m_confirm_target && ::payTxFee != CFeeRate(0)) { // 3. TODO: remove magic value of 0 for global payTxFee + fee_needed = ::payTxFee.GetFee(nTxBytes); + if (feeCalc) feeCalc->reason = FeeReason::PAYTXFEE; + } + else { // 2. or 4. + // We will use smart fee estimation + unsigned int target = coin_control.m_confirm_target ? *coin_control.m_confirm_target : ::nTxConfirmTarget; + // By default estimates are economical iff we are signaling opt-in-RBF + bool conservative_estimate = !coin_control.signalRbf; + // Allow to override the default fee estimate mode over the CoinControl instance + if (coin_control.m_fee_mode == FeeEstimateMode::CONSERVATIVE) conservative_estimate = true; + else if (coin_control.m_fee_mode == FeeEstimateMode::ECONOMICAL) conservative_estimate = false; + + fee_needed = estimator.estimateSmartFee(target, feeCalc, conservative_estimate).GetFee(nTxBytes); + if (fee_needed == 0) { + // if we don't have enough data for estimateSmartFee, then use fallbackFee + fee_needed = fallbackFee.GetFee(nTxBytes); if (feeCalc) feeCalc->reason = FeeReason::FALLBACK; } - } else { - if (feeCalc) feeCalc->reason = FeeReason::PAYTXFEE; + // Obey mempool min fee when using smart fee estimation + CAmount min_mempool_fee = pool.GetMinFee(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFee(nTxBytes); + if (fee_needed < min_mempool_fee) { + fee_needed = min_mempool_fee; + if (feeCalc) feeCalc->reason = FeeReason::MEMPOOL_MIN; + } } + // prevent user from paying a fee below minRelayTxFee or minTxFee - CAmount requiredFee = GetRequiredFee(nTxBytes); - if (requiredFee > nFeeNeeded) { - nFeeNeeded = requiredFee; + CAmount required_fee = GetRequiredFee(nTxBytes); + if (required_fee > fee_needed) { + fee_needed = required_fee; if (feeCalc) feeCalc->reason = FeeReason::REQUIRED; } // But always obey the maximum - if (nFeeNeeded > maxTxFee) { - nFeeNeeded = maxTxFee; + if (fee_needed > maxTxFee) { + fee_needed = maxTxFee; if (feeCalc) feeCalc->reason = FeeReason::MAXTXFEE; } - return nFeeNeeded; + return fee_needed; } @@ -4200,15 +4214,3 @@ bool CMerkleTx::AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState& { return ::AcceptToMemoryPool(mempool, state, tx, true, NULL, NULL, false, nAbsurdFee); } - -bool CalculateEstimateType(FeeEstimateMode mode, bool opt_in_rbf) { - switch (mode) { - case FeeEstimateMode::UNSET: - return !opt_in_rbf; // Allow for lower fees if RBF is an option - case FeeEstimateMode::CONSERVATIVE: - return true; - case FeeEstimateMode::ECONOMICAL: - return false; - } - return true; -} diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 884844868..b716e46a7 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -954,7 +954,7 @@ public: * @note passing nChangePosInOut as -1 will result in setting a random position */ bool CreateTransaction(const std::vector& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet, int& nChangePosInOut, - std::string& strFailReason, const CCoinControl *coinControl = NULL, bool sign = true); + std::string& strFailReason, const CCoinControl& coin_control, bool sign = true); bool CommitTransaction(CWalletTx& wtxNew, CReserveKey& reservekey, CConnman* connman, CValidationState& state); void ListAccountCreditDebit(const std::string& strAccount, std::list& entries); @@ -969,7 +969,7 @@ public: * Estimate the minimum fee considering user set parameters * and the required fee */ - static CAmount GetMinimumFee(unsigned int nTxBytes, unsigned int nConfirmTarget, const CTxMemPool& pool, const CBlockPolicyEstimator& estimator, FeeCalculation *feeCalc, bool ignoreGlobalPayTxFee, bool conservative_estimate); + static CAmount GetMinimumFee(unsigned int nTxBytes, const CCoinControl& coin_control, const CTxMemPool& pool, const CBlockPolicyEstimator& estimator, FeeCalculation *feeCalc); /** * Return the minimum required fee taking into account the * floating relay fee and user set minimum transaction fee @@ -1220,6 +1220,4 @@ bool CWallet::DummySignTx(CMutableTransaction &txNew, const ContainerType &coins return true; } -bool CalculateEstimateType(FeeEstimateMode mode, bool opt_in_rbf); - #endif // BITCOIN_WALLET_WALLET_H