From d824ad030e70bc72e0c63e1b0d00b08413024b55 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Tue, 29 Nov 2016 12:18:44 -0500 Subject: [PATCH 1/2] Disable fee estimates for a confirm target of 1 block --- src/policy/fees.cpp | 7 ++++++- src/rpc/mining.cpp | 2 ++ src/test/policyestimator_tests.cpp | 22 +++++++++++++++------- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 7113390cd..9eb831bc1 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -404,7 +404,8 @@ void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight, CFeeRate CBlockPolicyEstimator::estimateFee(int confTarget) { // Return failure if trying to analyze a target we're not tracking - if (confTarget <= 0 || (unsigned int)confTarget > feeStats.GetMaxConfirms()) + // It's not possible to get reasonable estimates for confTarget of 1 + if (confTarget <= 1 || (unsigned int)confTarget > feeStats.GetMaxConfirms()) return CFeeRate(0); double median = feeStats.EstimateMedianVal(confTarget, SUFFICIENT_FEETXS, MIN_SUCCESS_PCT, true, nBestSeenHeight); @@ -423,6 +424,10 @@ CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget, int *answerFoun if (confTarget <= 0 || (unsigned int)confTarget > feeStats.GetMaxConfirms()) return CFeeRate(0); + // It's not possible to get reasonable estimates for confTarget of 1 + if (confTarget == 1) + confTarget = 2; + double median = -1; while (median < 0 && (unsigned int)confTarget <= feeStats.GetMaxConfirms()) { median = feeStats.EstimateMedianVal(confTarget++, SUFFICIENT_FEETXS, MIN_SUCCESS_PCT, true, nBestSeenHeight); diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index f3cd1fbf0..c2e579157 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -785,6 +785,8 @@ UniValue estimatefee(const JSONRPCRequest& request) "\n" "A negative value is returned if not enough transactions and blocks\n" "have been observed to make an estimate.\n" + "-1 is always returned for nblocks == 1 as it is impossible to calculate\n" + "a fee that is high enough to get reliably included in the next block.\n" "\nExample:\n" + HelpExampleCli("estimatefee", "6") ); diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp index 7dc8f226c..c57feaec9 100644 --- a/src/test/policyestimator_tests.cpp +++ b/src/test/policyestimator_tests.cpp @@ -95,17 +95,22 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) // Highest feerate is 10*baseRate and gets in all blocks, // second highest feerate is 9*baseRate and gets in 9/10 blocks = 90%, // third highest feerate is 8*base rate, and gets in 8/10 blocks = 80%, - // so estimateFee(1) should return 10*baseRate. + // so estimateFee(1) would return 10*baseRate but is hardcoded to return failure // Second highest feerate has 100% chance of being included by 2 blocks, // so estimateFee(2) should return 9*baseRate etc... for (int i = 1; i < 10;i++) { origFeeEst.push_back(mpool.estimateFee(i).GetFeePerK()); - if (i > 1) { // Fee estimates should be monotonically decreasing + if (i > 2) { // Fee estimates should be monotonically decreasing BOOST_CHECK(origFeeEst[i-1] <= origFeeEst[i-2]); } int mult = 11-i; - BOOST_CHECK(origFeeEst[i-1] < mult*baseRate.GetFeePerK() + deltaFee); - BOOST_CHECK(origFeeEst[i-1] > mult*baseRate.GetFeePerK() - deltaFee); + if (i > 1) { + BOOST_CHECK(origFeeEst[i-1] < mult*baseRate.GetFeePerK() + deltaFee); + BOOST_CHECK(origFeeEst[i-1] > mult*baseRate.GetFeePerK() - deltaFee); + } + else { + BOOST_CHECK(origFeeEst[i-1] == CFeeRate(0).GetFeePerK()); + } } // Mine 50 more blocks with no transactions happening, estimates shouldn't change @@ -113,7 +118,8 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) while (blocknum < 250) mpool.removeForBlock(block, ++blocknum); - for (int i = 1; i < 10;i++) { + BOOST_CHECK(mpool.estimateFee(1) == CFeeRate(0)); + for (int i = 2; i < 10;i++) { BOOST_CHECK(mpool.estimateFee(i).GetFeePerK() < origFeeEst[i-1] + deltaFee); BOOST_CHECK(mpool.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee); } @@ -151,7 +157,8 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) } mpool.removeForBlock(block, 265); block.clear(); - for (int i = 1; i < 10;i++) { + BOOST_CHECK(mpool.estimateFee(1) == CFeeRate(0)); + for (int i = 2; i < 10;i++) { BOOST_CHECK(mpool.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee); } @@ -172,7 +179,8 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) mpool.removeForBlock(block, ++blocknum); block.clear(); } - for (int i = 1; i < 10; i++) { + BOOST_CHECK(mpool.estimateFee(1) == CFeeRate(0)); + for (int i = 2; i < 10; i++) { BOOST_CHECK(mpool.estimateFee(i).GetFeePerK() < origFeeEst[i-1] - deltaFee); } From e878689e5539d8de30283d1461d6466eac65f894 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Tue, 29 Nov 2016 12:49:03 -0500 Subject: [PATCH 2/2] Make GUI incapable of setting tx confirm target of 1 --- src/qt/forms/sendcoinsdialog.ui | 2 +- src/qt/sendcoinsdialog.cpp | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qt/forms/sendcoinsdialog.ui b/src/qt/forms/sendcoinsdialog.ui index 33db9f893..0a8afa2e7 100644 --- a/src/qt/forms/sendcoinsdialog.ui +++ b/src/qt/forms/sendcoinsdialog.ui @@ -1064,7 +1064,7 @@ 0 - 24 + 23 1 diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index 57b217943..bd3f20e9d 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -175,7 +175,7 @@ void SendCoinsDialog::setModel(WalletModel *_model) // set the smartfee-sliders default value (wallets default conf.target or last stored value) QSettings settings; if (settings.value("nSmartFeeSliderPosition").toInt() == 0) - ui->sliderSmartFee->setValue(ui->sliderSmartFee->maximum() - model->getDefaultConfirmTarget() + 1); + ui->sliderSmartFee->setValue(ui->sliderSmartFee->maximum() - model->getDefaultConfirmTarget() + 2); else ui->sliderSmartFee->setValue(settings.value("nSmartFeeSliderPosition").toInt()); } @@ -241,7 +241,7 @@ void SendCoinsDialog::on_sendButton_clicked() if (model->getOptionsModel()->getCoinControlFeatures()) ctrl = *CoinControlDialog::coinControl; if (ui->radioSmartFee->isChecked()) - ctrl.nConfirmTarget = ui->sliderSmartFee->maximum() - ui->sliderSmartFee->value() + 1; + ctrl.nConfirmTarget = ui->sliderSmartFee->maximum() - ui->sliderSmartFee->value() + 2; else ctrl.nConfirmTarget = 0; @@ -601,7 +601,7 @@ void SendCoinsDialog::updateGlobalFeeVariables() { if (ui->radioSmartFee->isChecked()) { - int nConfirmTarget = ui->sliderSmartFee->maximum() - ui->sliderSmartFee->value() + 1; + int nConfirmTarget = ui->sliderSmartFee->maximum() - ui->sliderSmartFee->value() + 2; payTxFee = CFeeRate(0); // set nMinimumTotalFee to 0 to not accidentally pay a custom fee @@ -646,7 +646,7 @@ void SendCoinsDialog::updateSmartFeeLabel() if(!model || !model->getOptionsModel()) return; - int nBlocksToConfirm = ui->sliderSmartFee->maximum() - ui->sliderSmartFee->value() + 1; + int nBlocksToConfirm = ui->sliderSmartFee->maximum() - ui->sliderSmartFee->value() + 2; int estimateFoundAtBlocks = nBlocksToConfirm; CFeeRate feeRate = mempool.estimateSmartFee(nBlocksToConfirm, &estimateFoundAtBlocks); if (feeRate <= CFeeRate(0)) // not enough data => minfee