test: fix an incorrect feature_fee_estimation.py subtest #32463

1. 
   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 append the higher value used to start the node (i.e., the custom minrelaytxfee) to the list of seen fees in self.fees_per_kb before checking smart fees since there is no certainty that we had generate a value higher than it, which it currently does not do.

   Due to the non-deterministic behavior of the feature_fee_estimation.py test, the randomly generated fees include a value higher than the custom minrelaytxfee, causing the test to pass.

   Issue #32461 discover a seed that generates random fees where none exceed the custom minrelaytxfee value, causing the test to reliably fail:

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

   This PR fixes the test by adding the custom higher minrelaytxfee value to self.fees_per_kb, but only when checking smart fee estimates.

   It also improves the subtest name and adds more robust test along with clearer comments.

2. test: fix incorrect `feature_fee_estimation.py` subtest

   - In `test_estimates_with_highminrelaytxfee` the list
     of seen fees should include the high val when checking smart estimates.
   
   - Also add more test and improve the comments.

   2c5f26bc6a
3. 
   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	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.

4. DrahtBot added the label Tests on May 9, 2025
5. 
   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

6. willcl-ark requested review from willcl-ark on May 14, 2025
7. in test/functional/feature_fee_estimation.py:253 in 2c5f26bc6a

   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,
8. willcl-ark approved
9. 
   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.

10. 
    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)

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

Contributors
ismaelsadeeq DrahtBot l0rinc willcl-ark

Labels
Tests

Milestone
30.0