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 |
|---|---|
| Concept ACK | ismaelsadeeq |
| Approach ACK | adamandrews1, 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.
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.
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.