test: Refactor fee/feerate calculations in feature_fee_estimation.py to use Decimal instead of float

[naiyoma]

This PR addresses the suggestions made here #23225 to use decimals or integers. Decimals and integers are more predictable than floats, which are prone to rounding errors.

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	AngusP, maflcko, achow101
Concept ACK	BrandonOdiwuor, kristapsk
Stale ACK	rkrux, ismaelsadeeq

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

[glozow]

cc @maflcko? (https://github.com/bitcoin/bitcoin/pull/23225#pullrequestreview-774698314)

[naiyoma]

lgtm. Seems fine to force the call site to specify the rounding when turning a random float into a satoshi amout.

Note that you are not really "removing" satoshi_round, as it is still used, just inlined.

An alternative could be to make rounding a required argument.

I agree with the alternative suggestion, I'll restore the satoshi_round function and make rounding a requirement.

[naiyoma]

If this is the scope of this PR, there are still places where we use float to represent fee rate and output values, you can refactor them to use Decimal or Integer but I am not sure how to maintain this and prevent adding fee/feerate representation or calculation in floats.

I'll look through the tests and enforce the use of Decimal or Integer types. I am not sure if we can explicitly prevent floats in fee calculations within tests, but a good place to start would be type hinting

[naiyoma]

tACK b7a4a81

Make successful, all functional tests successful.

To address this, the satoshi_round function is removed, and Decimal values are utilized in its place.

1. As per the second commit, the satoshi_round is back, let's update the PR description to reflect that because it will be picked up by the bot on merge?
2. Should we consider squashing the first 2 commits into one because after the review the function was put back?

Thanks for the review I have updated the PR Description and will squash commits as suggested.

[DrahtBot]

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/23898692372}

[ismaelsadeeq]

The C.I failure here is due to unused import @naiyoma

{'-zmqpubhashblock', '-zmqpubsequencehwm', '-zmqpubsequence', '-zmqpubrawblock', '-zmqpubhashtxhwm', '-zmqpubhashblockhwm', '-includeconf', '-zmqpubhashtx', '-zmqpubrawtx', '-testdatadir', '-zmqpubrawblockhwm', '-zmqpubrawtxhwm'}
test/functional/test_framework/util.py:8:1: F401 'decimal.ROUND_DOWN' imported but unused
^---- failure generated from lint-python.py

^---- ⚠️ Failure generated from lint-*.py scripts!

[naiyoma]

The C.I failure here is due to unused import @naiyoma

{'-zmqpubhashblock', '-zmqpubsequencehwm', '-zmqpubsequence', '-zmqpubrawblock', '-zmqpubhashtxhwm', '-zmqpubhashblockhwm', '-includeconf', '-zmqpubhashtx', '-zmqpubrawtx', '-testdatadir', '-zmqpubrawblockhwm', '-zmqpubrawtxhwm'}
test/functional/test_framework/util.py:8:1: F401 'decimal.ROUND_DOWN' imported but unused
^---- failure generated from lint-python.py

^---- ⚠️ Failure generated from lint-*.py scripts!

Deleted the Unused import, thanks

[kristapsk]

Concept ACK

[AngusP]

ACK aa46f0ec82c65fc8bb67c544c6b16296940d96dd

[DrahtBot]

🚧 At least one of the CI tasks failed. _{Debug: https://github.com/bitcoin/bitcoin/runs/28457526653}

Hints

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

[maflcko]

review ACK 7d7671ad5c83c3d1220d84110259c63b5ed7405d

The pull request description will need to be updated to reflect the current state.

[naiyoma]

The pull request description will need to be updated to reflect the current state.

@maflcko Description updated. Also, reverted fee_increment to a float and then applied satoshi_round with ROUND_DOWN at the call site. This is much cleaner and more readable.

[maflcko]

review ACK ec317bc44b0cb089419d809b5fef38ecb9423051

[achow101]

ACK ec317bc44b0cb089419d809b5fef38ecb9423051