rpc: update estimatesmartfee to return max of CBlockPolicyEstimator::estimateSmartFee, mempoolMinFee and minRelayTxFee

[pranabp-bit]

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.

[jonatack]

Concept ACK

[ghost]

Concept ACK (Including mempoolMinFee)

estimatesmartfee should become better (smart) after this change although I am not sure about the approach. Will test it today.

[kristapsk]

Concept ACK

[ghost]

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

Expected results:

$bitcoin-cli estimatesmartfee 1

{
  "feerate_min": 0.00001000
  "feerate_max": 0.00020000
  "blocks: 2
}

$bitcoin-cli estimatesmartfee 1

{
  "feerate_min": 0.00001000
  "feerate_max": 0.00100000
  "blocks: 2
}

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 ?

1. 2 * average_block_vsize where average_block_vsize is average virtual size of last N blocks

2. bytes from getmempoolinfo

3. if ((bytes/ 2 * average_block_vsize) < 1 use minimum fee rate from getmempoolinfo else use same as feerate_max.

Why this will be useful?

1. Users get a range instead of one fee rate
2. Better estimates based on fee rates in mempool
3. Can use feerate_min for RBF transaction and replace with feerate_max if not confirmed in next block

conf_target works for 1 to 1008 blocks right now. This can be reduced to 1-10 if others agree to change it.

💡 One project which can be helpful in improving `estimatesmartfee`

https://github.com/TrueLevelSA/btc-congestion-manager

It returns mempool position which can be added in output for estimatesmartfee:

[darosior]

Concept ACK, but i think this should not be restricted to the 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.

[pranabp-bit]

@prayank23 Thank you for the detailed review. Just wanted to confirm, 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.

[pranabp-bit]

@darosior Thank you for the suggestion. But as of now, I only found one other call for the 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.

[darosior]

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.

[ghost]

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.

[instagibbs]

concept ACK, can you please squash?

[instagibbs]

ACK aside from test nit

[pranabp-bit]

I updated the test such that in the end, it sets the 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.

[laanwj]

Can you please change to a more descriptive PR title and commit message? "update estimatesmartfee" doesn't say much 😄

[instagibbs]

ACK pranabp-bit@b2152f3

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #23074 (Package-aware fee estimation by darosior)
• #22539 (Re-include RBF replacement txs in fee estimation by darosior)

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.

[meshcollider]

utACK b2152f3

[darosior]

utACK b2152f3

[meshcollider]

re-utACK ea31caf