Closes: #11800
In scenarios where data is available for higher targets but not for lower ones, this method may return lower fee rates for higher confirmation targets. This could occur if estimateCombinedFee returns no valid data (-1) for some estimates for a low target, but does return valid data for a higher target.
Users of this function should be aware of this potential, if unlikely, inconsistency in behaviour in data-sparse scenarios.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31080.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ismaelsadeeq, adamandrews1 |
| Approach ACK | tdb3 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
901@@ -902,6 +902,14 @@ CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget, FeeCalculation
902 * horizons so we already have monotonically increasing estimates and
903 * the purpose of conservative estimates is not to let short term
904 * fluctuations lower our estimates by too much.
905+ *
906+ * Note: In scenarios where data is available for higher targets but not
Thanks, I’m happy to take this suggestion. What do you think about:
* Note: Monotonically increasing estimates are not guaranteed in certain
* data-sparse scenarios. In particular, if estimateCombinedFee returns no
* valid data (-1) for some estimates for a lower target, but does return
* valid data for a higher target, the estimate can theoretically return a
* higher value for higher targets.
901@@ -902,6 +902,12 @@ CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget, FeeCalculation
902 * horizons so we already have monotonically increasing estimates and
903 * the purpose of conservative estimates is not to let short term
904 * fluctuations lower our estimates by too much.
905+ *
906+ * Note: Monotonically increasing estimates are not guaranteed in certain
907+ * data-sparse scenarios. In particular, if estimateCombinedFee returns no
908+ * valid data (-1) for some estimates for a lower target, but does return
909+ * valid data for a higher target, the estimate can theoretically return a
910+ * higher value for higher targets.
0 * Note: In certain rare edge cases, monotonically increasing estimates may not be
1 * guaranteed. Specifically, given two targets N and M, where M > N, if a sub-estimate
2 * for target N fail to return a valid fee rate, while target M has valid fee rate for that
3 * sub-estimate, target M may result in a higher fee rate
4 * estimate than target N.
5 *
6 * See: [#11800 (comment)](/bitcoin-bitcoin/11800/#issuecomment-349697807)
I think the suggestion describes scenario better as stated in #11800 (comment).
This will be even better when we specify which sub-estimate failures break the monotonic behavior of the fee rate estimates. The comments mention that we check the maximum confirmations for shorter time horizons to preserve this behavior.
This applies to the first three sub-estimates for economical estimates and the first two for conservative estimates.
Approach ACK
The update suggested in #31080 (review) seems to increase clarity.
There hasn’t been much activity lately. What is the status here?
Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
Closes: #11800
Note: In certain rare edge cases, monotonically increasing estimates may
not be guaranteed. Specifically, given two targets N and M, where M > N,
if a sub-estimate for target N fails to return a valid fee rate, while
target M has valid fee rate for that sub-estimate, target M may result
in a higher fee rate estimate than target N.
See: https://github.com/bitcoin/bitcoin/issues/11800#issuecomment-349697807
@willcl-ark I guess you’ll have to push the requested change, or reply to it, or close this pull?
Sure @maflcko, I took the suggestion and rebased.