This PR is in response to the issue #19699.
Based on the discussion in the comments of PR #22673 changes have been made in the estimatesmartfee itself such that it takes into account mempoolMinFee and relayMinFee . Hence it provides a fee estimate that is most likely to be paid by the user in an actual transaction, preventing issues such as #16072.
The test file test/functional/feature_fee_estimation.py has also been updated to check this functionality.
1120+ feeRate = min_mempool_feerate;
1121+ }
1122+ CFeeRate min_relay_feerate = ::minRelayTxFee;
1123+ if (feeRate < min_relay_feerate) {
1124+ feeRate = min_relay_feerate;
1125+ }
Suggestion, you could use braced initialization and std::max with an initializer list.
0- CFeeRate feeRate = fee_estimator.estimateSmartFee(conf_target, &feeCalc, conservative);
1- CFeeRate min_mempool_feerate = mempool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000);
2- if (feeRate < min_mempool_feerate) {
3- feeRate = min_mempool_feerate;
4- }
5- CFeeRate min_relay_feerate = ::minRelayTxFee;
6- if (feeRate < min_relay_feerate) {
7- feeRate = min_relay_feerate;
8- }
9+ CFeeRate feeRate{fee_estimator.estimateSmartFee(conf_target, &feeCalc, conservative)};
10+ CFeeRate min_mempool_feerate{mempool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000)};
11+ CFeeRate min_relay_feerate{::minRelayTxFee};
12+ feeRate = std::max({feeRate, min_mempool_feerate, min_relay_feerate});
281@@ -278,6 +282,11 @@ def run_test(self):
282 self.log.info("Final estimates after emptying mempools")
283 check_estimates(self.nodes[1], self.fees_per_kb)
284
285+ # check that the effective feerate is greater than or equal to the mempoolminfee even for high mempoolminfee
286+ self.log.info("Estimates after restarting node with high MempoolMinFee")
0 self.log.info("Test fee rate estimation after restarting node with high MempoolMinFee")
137+ minRelaytxFee = node.getmempoolinfo()['minrelaytxfee']
138 for i, e in enumerate(all_smart_estimates): # estimate is for i+1
139 feerate = float(e["feerate"])
140 assert_greater_than(feerate, 0)
141+ assert_greater_than_or_equal(feerate,float(mempoolMinFee))
142+ assert_greater_than_or_equal(feerate,float(minRelaytxFee))
nit, missing space
0- assert_greater_than_or_equal(feerate,float(mempoolMinFee))
1- assert_greater_than_or_equal(feerate,float(minRelaytxFee))
2+ assert_greater_than_or_equal(feerate, float(mempoolMinFee))
3+ assert_greater_than_or_equal(feerate, float(minRelaytxFee))
1096@@ -1097,6 +1097,8 @@ static RPCHelpMan estimatesmartfee()
1097 RPCTypeCheckArgument(request.params[0], UniValue::VNUM);
1098
1099 CBlockPolicyEstimator& fee_estimator = EnsureAnyFeeEstimator(request.context);
1100+ NodeContext& node = EnsureAnyNodeContext(request.context);
can be const
0 const NodeContext& node = EnsureAnyNodeContext(request.context);
Concept ACK (Including mempoolMinFee)
estimatesmartfee should become better (smart) after this change although I am not sure about the approach. Will test it today.
Tried this on Pop!_OS with regtest. Initially had issues understanding how GetMinFee() works, Andrew Chow answered it in detail on stackexchange: https://bitcoin.stackexchange.com/questions/108126/getminfee-in-blockchain-cpp
I saved minrelaytxfee 0.0002 similar to /test/functional/feature_fee_estimation.py and did some test transactions with different fee rate.
estimatesmartfee suggests 20 sat/vB conf_target 2
Changed minrelaytxfee to 0.00001 (default value)
estimatesmartfee suggests 1 sat/vB conf_target 2
0$bitcoin-cli estimatesmartfee 1
1
2{
3 "feerate_min": 0.00001000
4 "feerate_max": 0.00020000
5 "blocks: 2
6}
0$bitcoin-cli estimatesmartfee 1
1
2{
3 "feerate_min": 0.00001000
4 "feerate_max": 0.00100000
5 "blocks: 2
6}
feerate_min based on mempool fee rate distribution and feerate_max is the fee_rate returned right now
How to calculate feerate_min when conf_target is 2 ?
2 * average_block_vsize where average_block_vsize is average virtual size of last N blocks
bytes from getmempoolinfo
if ((bytes/ 2 * average_block_vsize) < 1 use minimum fee rate from getmempoolinfo else use same as feerate_max.
Why this will be useful?
feerate_min for RBF transaction and replace with feerate_max if not confirmed in next blockconf_target works for 1 to 1008 blocks right now. This can be reduced to 1-10 if others agree to change it.
https://github.com/TrueLevelSA/btc-congestion-manager
It returns mempool position which can be added in output for estimatesmartfee:
estimatesmartfee RPC. Maybe we could have a wrapper to CBlockPolicyEstimator’s estimateSmartFee in CTxMemPool that would return the max of minerPolicyEstimator->estimateSmartFee() and GetMinFee? This way the node interface would call this wrapper instead of the estimator’s one and it would apply this behaviour to all the current call sites.
estimatesmartfee such that is returns “feerate_min” and “feerate_max” ? If yes, then maybe I could try that later in a different PR.
estimateSmartFee and it was in the GetMinimumFeeRate function. And this function returns the feerate_needed after considering the mempoolMinFee. So maybe we can keep this change limited to the estimatesmartfee RPC.
But as of now, I only found one other call
Sounds good to me, it’s already a fix as is and we can always de-duplicate the min(estimateSmarFee, mempoolMinFee) logic in a folowup.
are you suggesting to change estimatesmartfee such that is returns “feerate_min” and “feerate_max” ? If yes, then maybe I could try that later in a different PR.
Yes. We could do something similar in follow up to improve estimatesmartfee. However, I also wanted to share that results for estimatesmartfee even after this PR are not helpful or misleading in some cases.
For example: In first case mempool has some transactions with fee rate ranging from 1 to 100 sat/vB (less than 2MvB), estimatesmartfee should suggest fee rate close to 1 sat/vB for conf_target 2. It shows 20x what we actually need minimum at that moment.
278@@ -275,6 +279,11 @@ def run_test(self):
279 self.log.info("Final estimates after emptying mempools")
280 check_estimates(self.nodes[1], self.fees_per_kb)
281
282+ # check that the effective feerate is greater than or equal to the mempoolminfee even for high mempoolminfee
283+ self.log.info("Test fee rate estimation after restarting node with high MempoolMinFee")
284+ self.restart_node(1, extra_args=['-minrelaytxfee=0.0003'])
minrelaytxfee to a value which is 3 times the estimatesmartfee(1) called from the same node. So the test would be more robust against future changes than the previous one in which I set it directly to “0.0003”.
I have incorporated most of the suggestions I got, and some of them, I would take up as follow-up in the future.
But for now, do suggest if any other change is required in the current PR.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
1114@@ -1113,7 +1115,10 @@ static RPCHelpMan estimatesmartfee()
1115 UniValue result(UniValue::VOBJ);
1116 UniValue errors(UniValue::VARR);
1117 FeeCalculation feeCalc;
1118- CFeeRate feeRate = fee_estimator.estimateSmartFee(conf_target, &feeCalc, conservative);
1119+ CFeeRate feeRate{fee_estimator.estimateSmartFee(conf_target, &feeCalc, conservative)};
1120+ CFeeRate min_mempool_feerate{mempool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000)};
gArgs.GetIntArg now (as of #22976)
This will provide better estimates which would be closer to fee paid in actual
transactions.
The test has also been changed such that when the node is restarted with a
high mempoolMinFee, the estimatesmartfee still returns a feeRate greater
than or equal to the mempoolMinFee, minRelayTxFee.(just like the feeRate of actual transactions)