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

pull ismaelsadeeq wants to merge 4 commits into bitcoin:master from ismaelsadeeq:08-2025-block-policy-refactor changing 26 files +38 −36
  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

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-10-31 12:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me