refactor: Make CFeeRate constructor architecture-independent

MarcoFalke commented at 7:16 am on May 4, 2021: member

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:

0policy/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]
1    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.

fanquake added the label TX fees and policy on May 4, 2021

practicalswift commented at 7:38 am on May 4, 2021: contributor

Strong concept ACK

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

MarcoFalke added the label Refactoring on May 4, 2021

MarcoFalke commented at 7:49 am on May 4, 2021: member

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

practicalswift commented at 8:16 am on May 4, 2021: contributor

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:

0$ cling
1[cling]$ #include <cstdint>
2[cling]$ int64_t j = 2147483648
3(long) 2147483648
4[cling]$ int32_t non_uniform_1 = j
5(int) -2147483648
6[cling]$ int32_t non_uniform_2(j)
7(int) -2147483648
8[cling]$ int32_t uniform{j}
9input_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.

Subawit approved

in src/policy/feerate.h:44 in fa5cfbdef1 outdated

38@@ -39,20 +39,17 @@ class CFeeRate
39         // We've previously had bugs creep in from silent double->int conversion...
40         static_assert(std::is_integral<I>::value, "CFeeRate should be used without floats");
41     }
42-    /** Constructor for a fee rate in satoshis per kvB (sat/kvB). The size in bytes must not exceed (2^63 - 1).
43+    /** Constructor for a fee rate in satoshis per kvB (sat/kvB).
44      *
45-     *  Passing an nBytes value of COIN (1e8) returns a fee rate in satoshis per vB (sat/vB),
46+     *  Passing an num_bytes value of COIN (1e8) returns a fee rate in satoshis per vB (sat/vB),

jonatack commented at 9:09 am on May 4, 2021:

0     *  Passing a num_bytes value of COIN (1e8) returns a fee rate in satoshis per vB (sat/vB),

MarcoFalke commented at 9:23 am on May 4, 2021:

Fixed unrelated typo

jonatack commented at 9:13 am on May 4, 2021: member

Approach ACK

Maybe write “architecture” instead of “arch” (I first thought this was about arch linux :smile:)

in src/policy/feerate.cpp:12 in fa5cfbdef1 outdated

 6@@ -7,29 +7,30 @@
 7 
 8 #include <tinyformat.h>
 9 
10-CFeeRate::CFeeRate(const CAmount& nFeePaid, size_t nBytes_)
11+CFeeRate::CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes)
12 {
13-    assert(nBytes_ <= uint64_t(std::numeric_limits<int64_t>::max()));
14-    int64_t nSize = int64_t(nBytes_);
15+    int64_t nSize = int64_t{num_bytes};

jonatack commented at 9:14 am on May 4, 2021:

use a named cast? https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-casts-named

MarcoFalke commented at 9:23 am on May 4, 2021:

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es23-prefer-the--initializer-syntax

MarcoFalke renamed this:
~~refactor: Make CFeeRate constructor arch-independent~~
refactor: Make CFeeRate constructor architecture-independent
on May 4, 2021

MarcoFalke force-pushed on May 4, 2021

DrahtBot commented at 10:16 am on May 4, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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 commented at 7:00 pm on May 4, 2021: contributor

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 :)

0test/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)

MarcoFalke force-pushed on May 5, 2021

MarcoFalke commented at 4:17 pm on May 5, 2021: member

Thanks, fixed

in src/policy/feerate.cpp:23 in fa776331cf outdated

26-CAmount CFeeRate::GetFee(size_t nBytes_) const
27+CAmount CFeeRate::GetFee(uint32_t num_bytes) const
28 {
29-    assert(nBytes_ <= uint64_t(std::numeric_limits<int64_t>::max()));
30-    int64_t nSize = int64_t(nBytes_);
31+    int64_t nSize = int64_t{num_bytes};

promag commented at 9:45 am on May 12, 2021:

nit, const int64_t nSize{num_bytes};

MarcoFalke commented at 1:17 pm on May 12, 2021:

Thx, fixed.

in src/policy/feerate.cpp:28 in fa776331cf outdated

32 
33     CAmount nFee = nSatoshisPerK * nSize / 1000;
34 
35     if (nFee == 0 && nSize != 0) {
36-        if (nSatoshisPerK > 0)
37+        if (nSatoshisPerK > 0) {

promag commented at 9:46 am on May 12, 2021:

nit, just make one line?

MarcoFalke commented at 1:17 pm on May 12, 2021:

Thx, fixed

promag commented at 10:43 am on May 12, 2021: member

Code review ACK fa776331cf21c554c41292c323740601d1a12307.

MarcoFalke force-pushed on May 12, 2021

promag commented at 1:47 pm on May 13, 2021: member

Code review ACK fa748a6fb20486e1a839151f13284d719c25ad18.

theStack commented at 12:54 pm on May 16, 2021: member

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 (https://github.com/bitcoin/bitcoin/pull/21848#discussion_r630891087)?

MarcoFalke force-pushed on May 16, 2021

theStack approved

theStack commented at 6:28 pm on May 17, 2021: member

Code-review ACK fa12128869b2c96709420ddc862bce299f5d1e97 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).

refactor: Make CFeeRate constructor architecture-independent fafd121026

MarcoFalke force-pushed on May 18, 2021

MarcoFalke commented at 5:14 am on May 18, 2021: member

Added const

theStack approved

theStack commented at 7:23 am on May 18, 2021: member

re-ACK fafd121026c4f1e25d498983e4f88c119516552b

MarcoFalke requested review from promag on May 24, 2021

in src/policy/feerate.cpp:10 in fafd121026

 6@@ -7,29 +7,26 @@
 7 
 8 #include <tinyformat.h>
 9 
10-CFeeRate::CFeeRate(const CAmount& nFeePaid, size_t nBytes_)
11+CFeeRate::CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes)

promag commented at 9:02 am on May 24, 2021:

nit

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

And make const CAmount nSatoshisPerK.

MarcoFalke commented at 9:15 am on May 24, 2021:

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

promag commented at 9:04 am on May 24, 2021: member

Code review ACK fafd121026c4f1e25d498983e4f88c119516552b.

MarcoFalke merged this on May 24, 2021

MarcoFalke closed this on May 24, 2021

MarcoFalke deleted the branch on May 24, 2021

hebasto commented at 10:31 am on May 24, 2021: member

This PR effectively fixes the bug from #19735.

:+1:

sidhujag referenced this in commit 651ed10a8f on May 25, 2021

gwillen referenced this in commit d1bbcf2a0a on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

refactor: Make CFeeRate constructor architecture-independent #21848

Conflicts