refactor: CFeeRate encapsulates FeeFrac internally

[polespinasa]

The FeeFrac type represents a fraction, intended to be used for sats/vbyte or sats/WU. It was added to improve accuracy when evaluating fee rates in cluster mempool. [1]
But it can also be used to fix the precision issues that the current CFeeRate class has now.

At the moment, CFeeRate handles the fee rate as satoshis per kilovirtualbyte: CAmount / kvB using an integer.
This PR fix CFeeRate precision issues by encapsulating FeeFrac internally keeping backwards compatibility.

This PR can also be used as a based to use multiple units on RPC calls as detailed in this issue [2].

Some previous discussions:
[1] #30535
[2] #32093

[DrahtBot]

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/32750.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	murchandamus, ismaelsadeeq, theStack, achow101
Concept ACK	Eunovo

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

Conflicts

No conflicts as of last run.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

• param@[in] -> @PARAM[in] [correct Doxygen tag syntax]
• param@[in] -> @PARAM[in] [correct Doxygen tag syntax]

^{drahtbot_id_4_m}

[DrahtBot]

🚧 At least one of the CI tasks failed.
_{Task fuzzer,address,undefined,integer, no depends: https://github.com/bitcoin/bitcoin/runs/44121425336
LLM reason (✨ experimental): The CI failure is caused by an implicit integer conversion that triggered an UndefinedBehaviorSanitizer error during fuzzing.}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
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.

[ismaelsadeeq]

Concept ACK I've taken a look into this previously in #30535 (review)

[ismaelsadeeq]

I think you can fix the fuzz crash with

diff --git a/src/test/fuzz/fee_rate.cpp b/src/test/fuzz/fee_rate.cpp
index 92616b62bea..f69cb59ab7b 100644
--- a/src/test/fuzz/fee_rate.cpp
+++ b/src/test/fuzz/fee_rate.cpp
@@ -20,7 +20,7 @@ FUZZ_TARGET(fee_rate)
     const CFeeRate fee_rate{satoshis_per_k};
 
     (void)fee_rate.GetFeePerK();
-    const auto bytes = fuzzed_data_provider.ConsumeIntegral<uint32_t>();
+    const auto bytes = fuzzed_data_provider.ConsumeIntegral<int32_t>();
     if (!MultiplicationOverflow(int64_t{bytes}, satoshis_per_k)) {
         (void)fee_rate.GetFee(bytes);
     }

Also the // Compare approximately with CFeeRate test in src/test/fuzz/fee_frac.cpp is now not required.

[Eunovo]

Concept ACK

Left some comments

[polespinasa]

13efb13bbe0e904babd6f7646dda1885e80b8226 Rebased on top of Master (c584966)

[theStack]

Concept ACK

[polespinasa]

db63d5bf81895d78d39fe8cf94ded6f63d81d614 removes an unused include and some nit on arg names.
990010f49ca97479f9fe46e288ed0f0fbb0fde94 modifies the code to use uint instead of ints see #32750 (comment) for context

[polespinasa]

I think I'll drop bf63317ae7395b0509f86e2c188b1c6cacf995de and revert to only db63d5bf81 (unless someone sees a blocking reason and the need to use uint32 instead of int32).

Changing to uint32 needs many changes in FeeFrac (which I would like to avoid touching). In FeeFrac size is defined as an int32_t so using uint32 doesn't give us any "advantage".

Friendly ping @davidgumberg @Eunovo @sipa to know your opinion :)

[ismaelsadeeq]

re-ACK d3b8a54 📦

[theStack]

Code-review ACK d3b8a54

[theStack]

losely related nit: seems like CFeeRate serialization hasn't been used for more than a decade [1] (other than in the fuzz test) and we could as well just remove it; was shortly looking into that as I was worried that this line could change some file format

[1] AFAICT, the last use was removed in #5159

[maflcko]

#22962 ;)

[achow101]

ACK d3b8a54

As pointed out in #33177, this broke the Guix build. Was fixed in #33178.

nit: any reason to exclude negative values? They are documented to behave as-if 0 was passed, so it seems fine to keep the fuzz coverage for it? (https://maflcko.github.io/b-c-cov/fuzz.coverage/src/policy/feerate.cpp.gcov.html)

(There is a unit test covering it, so just a nit)

Negatives values would make the code crash because of the Assume introduced in GetFee

bitcoin/src/policy/feerate.cpp

Line 22 in 73220fc

Assume(virtual_bytes >= 0);

$ FUZZ=wallet_fees build_fuzz/bin/fuzz
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 2952456953
INFO: Loaded 1 modules   (614477 inline 8-bit counters): 614477 [0x5832503883e0, 0x58325041e42d), 
INFO: Loaded 1 PC tables (614477 PCs): 614477 [0x58325041e430,0x583250d7e900), 
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
policy/feerate.cpp:22 GetFee: Assertion `virtual_bytes >= 0' failed.
==89225== ERROR: libFuzzer: deadly signal

The addition of that Assertion was recommended here #32750 (comment) and further discussed here #32750 (comment) and here #32750 (review)

nano nit: The c-style cast isn't needed and the narrowing check can be enabled:

    CAmount nFee{m_feerate.EvaluateFeeUp(virtual_bytes)};