refactor: rename fees.{h,cpp} to fees/block_policy_estimator.{h,cpp} #33218

1. 
   ismaelsadeeq commented at 5:47 pm on August 19, 2025: member

   This PR is a simple refactoring that does four things:

   1. Renames test/policy_fee_tests.cpp to test/feerounder_tests.cpp.
   2. Renames policy/fees.{h,cpp} to policy/fees/block_policy_estimator.{h,cpp}.
   3. Renames policy/fees_args.cpp to policy/fees/block_policy_estimator_args.cpp.
   4. Modifies estimateSmartFee to return the block height at which the estimate was made by adding a best_height unsigned int value to the FeeCalculation struct.

   Motivation

   In preparation for adding a new fee estimator, the fees directory is created so we can organize code into block_policy_estimator and mempool because

   a) It would be clunky to add more code directly under fees. b) Having policy/fees.{h,cpp} and policy/mempool.{h,cpp} would also be undesirable.

   Therefore, it makes sense to structure the it as policy/fees/block_policy_estimator, policy/fees/mempool, etc. Hence test file were also updated accordingly.

   The current block height is also returned because later in #30157 we log the height at which each estimate is made (at the debug log category of fee estimation :) ). This feature is particularly useful for empirical data analysis.

2. 
   DrahtBot commented at 5:47 pm on August 19, 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/33218.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	maflcko, willcl-ark, janb84, polespinasa

   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:

   • #31664 (Fees: add Fee rate Forecaster Manager by ismaelsadeeq)
   • #28690 (build: Introduce internal kernel library by TheCharlatan)

   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.

3. ismaelsadeeq renamed this:
   fees: refactor: Create `policy/fees` dir and rename `fees.{h, cpp}` to `fees/block_policy_estimator{h, cpp}`
   refactor: rename `fees.{h, cpp}` to `fees/block_policy_estimator{h, cpp}`
   on Aug 19, 2025
4. DrahtBot added the label Refactoring on Aug 19, 2025
5. DrahtBot added the label Needs rebase on Aug 21, 2025
6. ismaelsadeeq force-pushed on Sep 1, 2025
7. ismaelsadeeq renamed this:
   refactor: rename `fees.{h, cpp}` to `fees/block_policy_estimator{h, cpp}`
   refactor: rename `fees.{h,cpp}` to `fees/block_policy_estimator{h,cpp}`
   on Sep 1, 2025
8. DrahtBot removed the label Needs rebase on Sep 1, 2025
9. 
   polespinasa commented at 6:03 pm on October 8, 2025: member

   ACK c70781d4241cb8b30fe33e7205868382031b4670

   Code reviewed and tested. These changes are just a refactor and not functional at all, but are needed in order to implement the FeeRate Forecast Manager in a “clean” and “ordered” manner #31664

10. 
    maflcko commented at 10:11 am on October 9, 2025: member

    Re-open to trigger the newly added GHA CI tasks
11. maflcko closed this on Oct 9, 2025

    ---

12. maflcko reopened this on Oct 9, 2025

    ---

13. in src/wallet/feebumper.cpp:8 in 8d06b3006f outdated

    4@@ -5,7 +5,8 @@
    5 #include <common/system.h>
    6 #include <consensus/validation.h>
    7 #include <interfaces/chain.h>
    8-#include <policy/fees.h>
    9+#include <node/types.h>
    

    ---

    ---

    

    maflcko commented at 10:24 am on October 9, 2025:

    nit in 8d06b3006f48ff84f2324c485a8141118849b919: Unrelated unused include added?

    ---

    

    ismaelsadeeq commented at 11:21 am on October 13, 2025:

    will fix nits when retouching.

    ---

    

    ismaelsadeeq commented at 9:44 am on October 27, 2025:

    fixed
14. maflcko approved
15. 
    maflcko commented at 10:53 am on October 9, 2025: member

    lgtm, just one nit, which is harmless and can be ignored.

    review ACK c70781d4241cb8b30fe33e7205868382031b4670 💬

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: review ACK c70781d4241cb8b30fe33e7205868382031b4670 💬
    3shy2Lln1WPp2bMAFQOeY1xXJpNGuyNM6VvzYDnc/BEGWbVSZW7oSocHznaCbPtCYPlRxTvoUtyVjWRdypFUmAw==
    

16. ismaelsadeeq renamed this:
    refactor: rename `fees.{h,cpp}` to `fees/block_policy_estimator{h,cpp}`
    refactor: rename `fees.{h,cpp}` to `fees/block_policy_estimator.{h,cpp}`
    on Oct 9, 2025
17. 
    darosior commented at 2:31 pm on October 9, 2025: member

    nit: if it’s already in a fees/ subfolder i think naming them estimator.{h,cpp} would be as clear and thrice as short?
18. 
    janb84 commented at 3:20 pm on October 9, 2025: contributor

    ACK 8a83178ba27ce9bb21a22fd013bf7bc25df8cc3e

    Would drop c70781d4241cb8b30fe33e7205868382031b4670 (or create separate PR). That commit add’s new code, the rest of the PR (except the comment of maflcko) is just a refactor (as per PR title)

    The motivation of the refactor seems logical and an improvement in the light of the new code. Even if the new code does not get merged, the new folder structure does not seems to be bad.

19. DrahtBot requested review from janb84 on Oct 9, 2025
20. 
    ismaelsadeeq commented at 11:21 am on October 13, 2025: member

    nit: if it’s already in a fees/ subfolder i think naming them estimator.{h,cpp} would be as clear and thrice as short?

    Hmm, I wanted it to be specific, thats why I named it block policy estimator and the file being in the fees directory was meant to indicate it is estimating fees.

21. DrahtBot added the label Needs rebase on Oct 24, 2025
22. fees: refactor: rename policy_fee_tests.cpp to feerounder_tests.cpp

    - Also remame the test suite name to match the new name.

    6dfdd7e034
23. fees: refactor: rename fees to block_policy_estimator

    - Also move it to policy/fees and update the includes

    06db08a435
24. fees: rename fees_args to block_policy_estimator_args

    - Also move them to policy/fees/ and update includes
    - Note: the block_policy_estimator_args.h include in block_policy_estimator_args.cpp was done manually.

    ab49480d9b
25. fees: return current block height in estimateSmartFee 1a7fb5eeee
26. ismaelsadeeq force-pushed on Oct 27, 2025
27. 
    ismaelsadeeq commented at 9:45 am on October 27, 2025: member

    Rebased and fixed a nit diff
28. DrahtBot removed the label Needs rebase on Oct 27, 2025
29. 
    maflcko commented at 10:24 am on October 27, 2025: member

    re-ACK 1a7fb5eeeef3575c1e7c27915c9b98695191299d 🐤

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: re-ACK 1a7fb5eeeef3575c1e7c27915c9b98695191299d 🐤
    30NDmvK93j/5tSSnLoQV4ss3q8a7+wHsEqypMWrAD9M2ebpvgjkvEnf0jHN6pqNLLoiuSQRenKpW1kbrXJLXOCg==
    

30. DrahtBot requested review from polespinasa on Oct 27, 2025
31. willcl-ark approved
32. 
    willcl-ark commented at 11:30 am on October 27, 2025: member

    ACK 1a7fb5eeeef3575c1e7c27915c9b98695191299d

    Refactor makes sense in the context of #30157 and is a logical carve-out of the larger PR.

    I was also surprised to find the 4th commit adding new functionality inside a refactor: ... PR, but the commit itself is reasonable enough (and again makes sense in the context of the direction of fee estimation in the project).

33. 
    janb84 commented at 11:36 am on October 27, 2025: contributor

    re ACK 1a7fb5eeeef3575c1e7c27915c9b98695191299d

    Although 4th commit is still part of the PR, I agree with @willcl-ark that it is a minor issue and should not be blocking for the PR.

    changes sinds last ACK:

    • author addressed some small nits.
    • rebase
34. 
    polespinasa commented at 7:05 pm on October 27, 2025: member

    re ACK 1a7fb5eeeef3575c1e7c27915c9b98695191299d

    Just small rebase and small nits

35. glozow merged this on Oct 28, 2025
36. glozow closed this on Oct 28, 2025

    ---

37. ismaelsadeeq deleted the branch on Oct 28, 2025

Contributors
ismaelsadeeq DrahtBot polespinasa maflcko darosior janb84 willcl-ark

Review Requested
polespinasa

Labels
Refactoring