refactor: Make CFeeRate constructor architecture-independent

[maflcko]

Currently the constructor is architecture dependent. This is confusing for several reasons:

• It is impossible to create a transaction larger than the max value of uint32_t, so a 64-bit size_t is not needed
• Policy (and consensus) code should be arch-independent
• The current code will print spurious compile errors when compiled on 32-bit systems:

policy/feerate.cpp:23:22: warning: result of comparison of constant 9223372036854775807 with expression of type 'size_t' (aka 'unsigned int') is always true [-Wtautological-constant-out-of-range-compare]
    assert(nBytes_ <= uint64_t(std::numeric_limits<int64_t>::max()));

Fix all issues by making it arch-independent. Also, fix {} style according to dev notes.

[practicalswift]

Strong concept ACK

I've always found the now cleaned up code confusing. Thanks for making the code easier to reason about.

[maflcko]

I wrote the assert in #7796, so it might be partially my fault

[practicalswift]

Also, fix {} style according to dev notes.

Good!

Important note which may not be immediately clear for all reviewers:

int32_t i{j}; is not necessarily equivalent to int32_t i = j; or int32_t i(j);.

Thus the choice of {}-initialization is not a cosmetic style choice.

int32_t i{j}; is the preferred choice not because it looks nicer, but because it is safer.

Safer how?

Live demo:

$ cling
[cling]$ #include <cstdint>
[cling]$ int64_t j = 2147483648
(long) 2147483648
[cling]$ int32_t non_uniform_1 = j
(int) -2147483648
[cling]$ int32_t non_uniform_2(j)
(int) -2147483648
[cling]$ int32_t uniform{j}
input_line_9:2:18: error: non-constant-expression cannot be narrowed from type 'int64_t' (aka 'long') to 'int32_t' (aka 'int') in initializer list

Note the silent narrowing from 2147483648 to -2147483648 for = j and (j), but not for {j}.

That is why the {}-initializer syntax is said to be safer than the other forms of initializations: by using {} we're guaranteed not to be hit by an unexpected narrowing conversion.

See also C++ Core Guidelines: ES.23: Prefer the {}-initializer syntax.

[jonatack]

Approach ACK

Maybe write "architecture" instead of "arch" (I first thought this was about arch linux 😄)

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• Fix -Wtautological-constant-out-of-range-compare warnings on 32-bit systems #19735 (Fix -Wtautological-constant-out-of-range-compare warnings on 32-bit systems by hebasto)

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.

[practicalswift]

Our automated reviewer friend -fsanitize=integer found the need to also update the fee rate fuzzing harness to make it stay in sync with the function signature changes suggested in this PR :)

test/fuzz/fee_rate.cpp:25:31: runtime error: implicit conversion from type 'size_t' (aka 'unsigned long') of value 55834574847 (64-bit, unsigned) to type 'uint32_t' (aka 'unsigned int') changed the value to 4294967295 (32-bit, unsigned)

[maflcko]

Thanks, fixed

[promag]

Code review ACK fa77633.

[promag]

Code review ACK fa748a6.

[theStack]

Concept ACK

Right now the initialization of nSize from num_bytes is slightly different in the ctor CFeeRate::CFeeRate(...) and the method CFeeRate::GetFee(...). Both could use const int64_t nSize{num_bytes}, as also suggested at one place by promag (#21848 (comment))?

[theStack]

Code-review ACK fa12128
nit: Doesn't matter much in those short methods, but if for any reason you need to retouch, you could add a const in front of int64_t nSize{num_bytes}; (both instances).

[maflcko]

Added const

[theStack]

re-ACK fafd121

[promag]

Code review ACK fafd121.

[promag]

nit

CFeeRate::CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes)
    : nSatoshisPerK(num_bytes > 0 ? nFeePaid * 1000 / int64_t{num_bytes} : 0)
{
}

And make const CAmount nSatoshisPerK.

[maflcko]

Nice, but adding const can be done in a follow-up

[hebasto]

This PR effectively fixes the bug from #19735.

👍