test: fix an incorrect `feature_fee

ismaelsadeeq commented at 6:20 pm on May 9, 2025: member

Attempt to fix #32461

In the estimatesmartfee RPC, we return the maximum of the following: the feerate estimate for the target, minrelaytxfee, and mempoolminfee.

https://github.com/bitcoin/bitcoin/blob/9a05b45da60d214cb1e5a50c3d2293b1defc9bb0/src/rpc/fees.cpp#L85

The test test_feerate_mempoolminfee, originally introduced in https://github.com/bitcoin/bitcoin/commit/ea31caf6b4c182c6f10f136548f84e603800511c, is incorrect.

It should calculate the fee rate ceiling by taking the maximum of the custom minrelaytxfee, mempoolminfee, and the highest fee rate observed during the test (check_smart_estimates). This is necessary because:

There is no guarantee that the generated fee rates will exceed both minrelaytxfee and mempoolminfee.
Users can start a node with custom fee settings.

Due to the non-deterministic nature of the feature_fee_estimation.py test, it often passes by chance. The randomly generated fees typically include a value higher than the custom minrelaytxfee, inadvertently hiding the issue.

Issue #32461 identified a random seeds that consistently fails the test because the generated fees never exceed the custom minrelaytxfee:

e.g

0build/test/functional/feature_fee_estimation.py --random=3450808900320758527

This PR has two commits which :

Correctly fixes the test by calculating the fee rate ceiling as the maximum of the node minrelaytxfee, mempoolminfee, and the highest seen fee rate, when verifying smart fee estimates.
Improves the subtest name and comment for clarity.
Restores the original test behavior by appending 4000 WU to the custom blockmaxweight.

DrahtBot commented at 6:20 pm on May 9, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32463.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	glozow, theStack, achow101
Stale ACK	l0rinc, willcl-ark

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)

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.

DrahtBot added the label Tests on May 9, 2025

l0rinc commented at 7:09 pm on May 9, 2025: contributor

Thanks for fixing it so quickly, I can confirm that this fixes the test for the failing --random=3450808900320758527. Someone more knowledgeable in this area of the code should also look at at, but from my part it’s a lightweight ACK.

ACK 2c5f26bc6ac47e3a6a8555e9a6c60832322a36e8

willcl-ark requested review from willcl-ark on May 14, 2025

in test/functional/feature_fee_estimation.py:253 in 2c5f26bc6a outdated

250+
251+        # Append the high minrelaytxfee to the list of seen fees.
252+        # Because estimatesmartfee returns the max of (fee rate estimate, minrelaytxfee and mempoolminfee)
253+        self.fees_per_kb.append(high_val)
254+        # Test for confirmation target of 2 and then all targets.
255+        assert_greater_than_or_equal(self.nodes[1].estimatesmartfee(1)["feerate"], high_val)

willcl-ark commented at 1:11 pm on June 2, 2025:

Would it be marginally clearer here to use 2 directly here?

0        assert_greater_than_or_equal(self.nodes[1].estimatesmartfee(2)["feerate"], high_val)

ismaelsadeeq commented at 1:20 pm on June 2, 2025:

Would update when retouching,

willcl-ark approved

willcl-ark commented at 1:14 pm on June 2, 2025: member

ACK 2c5f26bc6ac47e3a6a8555e9a6c60832322a36e8

If you re-touch for any reason it would seem a little more logical to me to slightly restructure so we test without new minrelaytxfee first (check_estimates() on L260), then add the high minrelaytxfee before doing the rest of the tests with it added. But zero practical difference and not a blocker for me.

ismaelsadeeq commented at 1:24 pm on June 2, 2025: member

If you re-touch for any reason it would seem a little more logical to me to slightly restructure so we test without new minrelaytxfee first (check_estimates() on L260), then add the high minrelaytxfee before doing the rest of the tests with it added. But zero practical difference and not a blocker for me.

Makes sense, I will add it in the follow-up to this which is retaining the original feature_fee_estimations.py tests behaviour by incrementing the customblockmaxweight by 4000. see #32461 (comment)

maflcko added this to the milestone 30.0 on Jun 3, 2025

ismaelsadeeq commented at 6:44 am on June 25, 2025: member

cc @glozow Is this rfm? I’d like to make a quick follow-up on this, as mentioned in #32463 (comment)

Also, this can be backported to 29.1 cc @fanquake.

in test/functional/feature_fee_estimation.py:460 in 2c5f26bc6a outdated

473         self.log.info(
474-            "Test fee rate estimation after restarting node with high MempoolMinFee"
475+            "Test fee rate estimation after restarting node with high minrelaytxfee"
476         )
477-        self.test_feerate_mempoolminfee()
478+        self.test_estimates_with_highminrelaytxfee()

glozow commented at 9:52 pm on June 25, 2025:

Agree this naming is better 👍

glozow commented at 10:10 pm on June 25, 2025: member

Nice work finding the issue. For some reason, I couldn’t reproduce using --random=3450808900320758527. But --random=4130785039185616282 from https://cirrus-ci.com/task/5487636053229568 and --random=6565824249277094228 from https://github.com/bitcoin/bitcoin/runs/43526123006 worked for me.

Did you consider baking this logic into check_smart_estimates instead? It feels a bit hacky to just append it to the fees seen, given that it isn’t actually a seen fee.

 0diff --git a/test/functional/feature_fee_estimation.py b/test/functional/feature_fee_estimation.py
 1index 61db186de17..07dabc6e40f 100755
 2--- a/test/functional/feature_fee_estimation.py
 3+++ b/test/functional/feature_fee_estimation.py
 4@@ -91,19 +91,21 @@ def check_smart_estimates(node, fees_seen):
 5     """Call estimatesmartfee and verify that the estimates meet certain invariants."""
 6 
 7     delta = 1.0e-6  # account for rounding error
 8-    last_feerate = float(max(fees_seen))
 9     all_smart_estimates = [node.estimatesmartfee(i) for i in range(1, 26)]
10     mempoolMinFee = node.getmempoolinfo()["mempoolminfee"]
11     minRelaytxFee = node.getmempoolinfo()["minrelaytxfee"]
12+    feerate_ceiling = max(max(fees_seen), float(mempoolMinFee), float(minRelaytxFee))
13+
14+    last_feerate = feerate_ceiling
15     for i, e in enumerate(all_smart_estimates):  # estimate is for i+1
16         feerate = float(e["feerate"])
17         assert_greater_than(feerate, 0)
18         assert_greater_than_or_equal(feerate, float(mempoolMinFee))
19         assert_greater_than_or_equal(feerate, float(minRelaytxFee))
20 
21-        if feerate + delta < min(fees_seen) or feerate - delta > max(fees_seen):
22+        if feerate + delta < min(fees_seen) or feerate - delta > feerate_ceiling:
23             raise AssertionError(
24-                f"Estimated fee ({feerate}) out of range ({min(fees_seen)},{max(fees_seen)})"
25+                f"Estimated fee ({feerate}) out of range ({min(fees_seen)},{feerate_ceiling})"
26             )
27         if feerate - delta > last_feerate:
28             raise AssertionError(
29@@ -238,12 +240,13 @@ class EstimateFeeTest(BitcoinTestFramework):
30         self.log.info("Final estimates after emptying mempools")
31         check_estimates(self.nodes[1], self.fees_per_kb)
32 
33-    def test_feerate_mempoolminfee(self):
34+    def test_estimates_with_highminrelaytxfee(self):
35         high_val = 3 * self.nodes[1].estimatesmartfee(1)["feerate"]
36         self.restart_node(1, extra_args=[f"-minrelaytxfee={high_val}"])
37         check_estimates(self.nodes[1], self.fees_per_kb)
38         self.restart_node(1)
39 
40+
41     def sanity_check_rbf_estimates(self, utxos):
42         """During 5 blocks, broadcast low fee transactions. Only 10% of them get
43         confirmed and the remaining ones get RBF'd with a high fee transaction at
44@@ -452,11 +455,11 @@ class EstimateFeeTest(BitcoinTestFramework):
45         self.log.info("Test fee_estimates.dat is flushed periodically")
46         self.test_estimate_dat_is_flushed_periodically()
47 
48-        # check that the effective feerate is greater than or equal to the mempoolminfee even for high mempoolminfee
49+        # check that estimatesmartfee feerate is greater than or equal to maximum of mempoolminfee and minrelaytxfee
50         self.log.info(
51-            "Test fee rate estimation after restarting node with high MempoolMinFee"
52+            "Test fee rate estimation after restarting node with high minrelaytxfee"
53         )
54-        self.test_feerate_mempoolminfee()
55+        self.test_estimates_with_highminrelaytxfee()
56 
57         self.log.info("Test acceptstalefeeestimates option")
58         self.test_acceptstalefeeestimates_option()

test: fix incorrect subtest in `feature_fee_estimation.py`

- Update `check_smart_estimates` to calculate the fee rate ceiling
   by taking the maximum of fees seen, minrelaytxfee, and mempoolminfee.
- Improve the subtest name and comments.

5c1236f04a

test: retain the intended behavior of `feature_fee_estimation.py` nodes

- Increase block weight by 4000 for all nodes with custom -blockmaxweight.
  Prior to this commit, we generated blocks with 4000 weight units less worth of transactions.
  See https://github.com/bitcoin/bitcoin/issues/32461#issuecomment-2925282272 for details.
  This commit fixes it by increasing the block weight by 4000.

9b75cfda4d

ismaelsadeeq force-pushed on Jun 26, 2025

ismaelsadeeq commented at 6:28 am on June 26, 2025: member

Thanks for your review @glozow

Did you consider baking this logic into check_smart_estimates instead? It feels a bit hacky to just append it to the fees seen, given that it isn’t actually a seen fee.

I hadn’t considered that before, your suggestion is definitely more elegant and cleaner.

I’ve updated the PR to implement your approach, and I also added a follow-up improvement I had in mind as a separate commit.

22a36e8..9b75cfd

fanquake added the label Needs backport (29.x) on Jun 26, 2025

in test/functional/feature_fee_estimation.py:150 in 9b75cfda4d

145@@ -146,8 +146,8 @@ def set_test_params(self):
146         self.noban_tx_relay = True
147         self.extra_args = [
148             [],
149-            ["-blockmaxweight=68000"],
150-            ["-blockmaxweight=32000"],
151+            ["-blockmaxweight=72000"],
152+            ["-blockmaxweight=36000"],

glozow commented at 8:42 pm on June 26, 2025:

nit: linking #31384 in the commit message for 9b75cfda4d62a0a3bde402503244dd57e1621a12 makes it easier to see why an extra 4000 is needed

ismaelsadeeq commented at 11:42 am on June 27, 2025:

I think just linking the PR without some explanation will not be that enough, so I instead link the issue comment with the comprehensive explanation, it will be easy to dig in up to that PR after clicking the current link in https://github.com/bitcoin/bitcoin/commit/9b75cfda4d62a0a3bde402503244dd57e1621a12.

glozow commented at 4:31 pm on July 3, 2025:

I’m suggesting linking both, or explaining it directly. Currently it requires clicking on the issue comment, then on the commit linked in that comment, then the PR linked in that commit to understand where the 4000 comes from. But it’s just a nit, feel free to ignore.

ismaelsadeeq commented at 4:48 pm on July 3, 2025:

Yeah, I think that will be clearer. I will fix when retouching.

glozow commented at 10:33 pm on June 26, 2025: member

ACK 9b75cfda4d62a0a3bde402503244dd57e1621a12

DrahtBot requested review from l0rinc on Jun 26, 2025

DrahtBot requested review from willcl-ark on Jun 26, 2025

fanquake requested review from theStack on Jul 2, 2025

theStack approved

theStack commented at 2:43 pm on July 2, 2025: contributor

Light ACK 9b75cfda4d62a0a3bde402503244dd57e1621a12

I’m not familiar with the inner workings of our fee estimation code, but only considering how the “feerate” result is constructed in the estimatesmartfee RPC, the changes look good to me. Increasing the max block weight params by 4000 to compensate for #31384 also makes sense.

Nice work finding the issue. For some reason, I couldn’t reproduce using --random=3450808900320758527. But --random=4130785039185616282 from https://cirrus-ci.com/task/5487636053229568 and --random=6565824249277094228 from https://github.com/bitcoin/bitcoin/runs/43526123006 worked for me.

Same here, fwiw, running with --random=3450808900320758527 didn’t fail for me on master (at commit bf75c9964fb28a5e9b8a07097248ffa960738478).

achow101 commented at 11:39 pm on July 3, 2025: member

ACK 9b75cfda4d62a0a3bde402503244dd57e1621a12

achow101 merged this on Jul 3, 2025

achow101 closed this on Jul 3, 2025

ismaelsadeeq deleted the branch on Jul 4, 2025

fanquake commented at 8:37 am on July 4, 2025: member

Same here, fwiw, running with –random=3450808900320758527 didn’t fail for me on master (at commit https://github.com/bitcoin/bitcoin/commit/bf75c9964fb28a5e9b8a07097248ffa960738478).

This is the one that fails for me on 29.x. Backported to 29.x in #32863.

fanquake referenced this in commit 84c0c0e64b on Jul 4, 2025

fanquake referenced this in commit f85d41c224 on Jul 4, 2025

fanquake removed the label Needs backport (29.x) on Jul 4, 2025

test: fix an incorrect feature_fee_estimation.py subtest #32463

Code Coverage & Benchmarks

Reviews

Conflicts

test: fix an incorrect `feature_fee_estimation.py` subtest #32463