refactor: Extract MIB_BYTES constant for init.cpp #25386

pull Empact wants to merge 4 commits into bitcoin:master from Empact:2022-06-mib-bytes changing 12 files +56 −30

Empact commented at 6:09 AM on June 16, 2022: member

This is a common concept which is more communicative and explicit if articulated, IMO.

This could be applied elsewhere if there is a better place to reference it in common, but I don't see it.

Pulled from #25315 to ease review there.
laanwj commented at 6:47 AM on June 16, 2022: member

Concept ACK

if there is a better place to reference it in common, but I don't see it.

Right, not sure init.cpp is the best place for this constant, but the idea of having it as a named constant makes sense to me, it tells anyone encountering the code about intent and the unit.
DrahtBot added the label Refactoring on Jun 16, 2022
DrahtBot commented at 7:32 AM on June 16, 2022: contributor


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



Reviews

See the guideline for information on the review process.

Type Reviewers

Concept ACK laanwj, brunoerg, mzumsande

Approach ACK w0xlt

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:
- #26674 (Add reindex=auto flag to automatically reindex corrupt data by AaronDewes)
- #26288 (Enable -Wstring-concatenation and -Wstring-conversion on clang builds by Empact)
- #25972 (build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set by fanquake)
- #25781 (Remove almost all blockstorage globals by MarcoFalke)
- #25361 (BIP324: Cipher suite by dhruv)
- #23233 (BIP324: Add encrypted p2p transport {de}serializer by dhruv)
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.
brunoerg commented at 2:13 PM on June 17, 2022: contributor

Concept ACK

It's easier to read and maintain the code this way imo.
w0xlt commented at 9:06 PM on June 17, 2022: contributor

Approach ACK

Adding the unit makes the code more readable (in the same way that using std::chrono instead of int makes the code better)
in src/init.cpp:121 in 60236bf35b outdated
```
 117 | @@ -118,6 +118,8 @@ using node::nPruneTarget;
 118 |  static const bool DEFAULT_PROXYRANDOMIZE = true;
 119 |  static const bool DEFAULT_REST_ENABLE = false;
 120 |  
 121 | +static constexpr size_t MIB_BYTES = 1024 * 1024;
```
maflcko commented at 6:34 AM on June 18, 2022:

This will force all places where this is used to uint64_t, even if they are supposed to be signed integers. Not sure, but maybe a #define or int type is better, so that the sign of the surrounding code is chosen?

laanwj commented at 7:49 AM on June 20, 2022:

Ouch, good point. I'm starting to doubt this change now.

Empact commented at 5:59 PM on October 10, 2022:

Thanks @MarcoFalke, this was indeed surprising to me:

Otherwise, if the unsigned operand's conversion rank is greater or equal to the conversion rank of the signed operand, the signed operand is converted to the unsigned operand's type.

https://en.cppreference.com/w/cpp/language/operator_arithmetic#Conversions

Empact commented at 6:35 PM on October 10, 2022:

I updated the PR to use int32_t for these constants, on the theory that int being the smallest value for an unsuffixed literal field, these values would be equivalent to the inline arithmetic. https://en.cppreference.com/w/cpp/language/integer_literal#The_type_of_the_literal

Empact commented at 6:45 PM on October 10, 2022:

Though I maintain uint64_t for GB_BYTES to maintain protections against overflow in PruneGBtoMiB.

Empact commented at 9:21 PM on October 12, 2022:

In an effort to validate these concerns, I ran build with -Wsign-conversion and -Wsign-compare, with the new and old code, and did not see any warnings relative to MIB_BYTES, MIN_PRUNE_TARGET_GB, or _FOR_BLOCK_FILES. I also reviewed each impacted line and did not identify such issues.

Prior, uint64_t code here: https://github.com/Empact/bitcoin/commits/2022-06-mib-bytes-sign-conversion-test Current, int32_t code here: https://github.com/Empact/bitcoin/commits/2022-06-mib-bytes-sign-conversion
DrahtBot added the label Needs rebase on Jun 29, 2022
Empact force-pushed on Aug 8, 2022
DrahtBot removed the label Needs rebase on Aug 8, 2022
DrahtBot added the label Needs rebase on Sep 1, 2022

Type	Reviewers
Concept ACK	laanwj, brunoerg, mzumsande
Approach ACK	w0xlt

refactor: Extract MIB_BYTES constant for init.cpp

Co-authored-by: benthecarman <benthecarman@live.com>
Co-authored-by: Justin Litchfield <litch@me.com>
Co-authored-by: Liran Cohen <c.liran.c@gmail.com>
Co-authored-by: Ryan Loomba <ryan.loomba@gmail.com>
Co-authored-by: Buck Perley <bucko.perley@gmail.com>
Co-authored-by: bajjer <bajjer@bajjer.xyz>
Co-authored-by: Suhail Saqan <suhail.saqan@gmail.com>
Co-authored-by: Christopher Sweeney <sweeney.chris@gmail.com>
Co-authored-by: Alyssa <orbitalturtle@protonmail.com>
Co-authored-by: Ben Schroth <ben@styng.social>
Co-authored-by: Jason Hester <mail@jason-hester.me>
Co-authored-by: Matt Clough <Matt.clough@pm.me>
Co-authored-by: Elise Schedler <eliseschedler@gmail.com>
Co-authored-by: ghander <cen254@gmail.com>
Co-authored-by: PopeLaz <btclz@fastmail.com>

3e4259332b

refactor: Expose MIN_MIB_FOR_BLOCK_FILES, MIN_BYTES_FOR_BLOCK_FILES constant 28c7d4c8f2
Empact force-pushed on Oct 10, 2022
Empact force-pushed on Oct 10, 2022
Empact force-pushed on Oct 10, 2022
DrahtBot removed the label Needs rebase on Oct 10, 2022
achow101 commented at 7:36 PM on October 12, 2022: member

cc @mzumsande

Expose MIN_PRUNE_TARGET_GB in guiconstants.h

This requires creating a non-gui analog to guiconstants.h, and
lifting GB/MIB_BYTES and MIN_*_FOR_BLOCK_FILES into it.

86e1d59eb2

Empact force-pushed on Oct 12, 2022
build: Enable -Wsign-compare and -Wsign-conversion 5b5b73cc97

in configure.ac:467 in 5b5b73cc97

 462 | @@ -463,6 +463,8 @@ if test "$CXXFLAGS_overridden" = "no"; then
 463 |                          [AC_LANG_SOURCE([[struct A { virtual void f(); }; struct B : A { void f() final; };]])])
 464 |    AX_CHECK_COMPILE_FLAG([-Wunreachable-code-loop-increment], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunreachable-code-loop-increment"], [], [$CXXFLAG_WERROR])
 465 |    AX_CHECK_COMPILE_FLAG([-Wimplicit-fallthrough], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wimplicit-fallthrough"], [], [$CXXFLAG_WERROR])
 466 | +  AX_CHECK_COMPILE_FLAG([-Wsign-compare], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wsign-compare"], [], [$CXXFLAG_WERROR])
 467 | +  AX_CHECK_COMPILE_FLAG([-Wsign-conversion], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wsign-conversion"], [], [$CXXFLAG_WERROR])

mzumsande commented at 4:53 PM on December 9, 2022:

This leads to lots of warnings all over the place for me (unrelated to MIB_BYTES). I don't think it should be part of this PR:

in src/constants.h:14 in 86e1d59eb2 outdated
```
   9 | +
  10 | +/* One gigabyte (GB) in bytes */
  11 | +static constexpr uint64_t GB_BYTES{1000000000};
  12 | +
  13 | +/* One mebibyte (MiB) in bytes */
  14 | +static constexpr int32_t MIB_BYTES{1024 * 1024};
```
mzumsande commented at 5:28 PM on December 9, 2022:

How about introducing this right from the beginning? It's a bit weird that MIB_BYTES jumps files with each commit, from init.cpp to validation.h to its final destination constants.h.
mzumsande commented at 5:34 PM on December 9, 2022: contributor

Concept ACK.

git grep "1024 / 1024" shows a few more spots that could make use of the constant.
DrahtBot added the label Needs rebase on Mar 15, 2023
DrahtBot commented at 11:53 PM on March 15, 2023: contributor



🐙 This pull request conflicts with the target branch and needs rebase.
achow101 commented at 3:41 PM on April 25, 2023: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.
achow101 closed this on Apr 25, 2023
bitcoin locked this on Apr 24, 2024

Contributors

Labels

Refactoring Needs rebase

Linked (view graph)

#25315 Add warning on first startup if free disk space is less than necessary

refactor: Extract MIB_BYTES constant for init.cpp #25386

Reviews

Conflicts