refactor: Extract MIB_BYTES constant for init.cpp by Empact · Pull Request #25386 · bitcoin/bitcoin

Empact · 2022-06-16T06:09:20Z

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 · 2022-06-16T06:47:59Z

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 · 2022-06-16T07:32:04Z

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

Concept ACK

It's easier to read and maintain the code this way imo.

w0xlt

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)

maflcko · 2022-06-18T06:34:50Z

src/init.cpp

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?

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

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

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

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

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

Co-authored-by: benthecarman <[email protected]> Co-authored-by: Justin Litchfield <[email protected]> Co-authored-by: Liran Cohen <[email protected]> Co-authored-by: Ryan Loomba <[email protected]> Co-authored-by: Buck Perley <[email protected]> Co-authored-by: bajjer <[email protected]> Co-authored-by: Suhail Saqan <[email protected]> Co-authored-by: Christopher Sweeney <[email protected]> Co-authored-by: Alyssa <[email protected]> Co-authored-by: Ben Schroth <[email protected]> Co-authored-by: Jason Hester <[email protected]> Co-authored-by: Matt Clough <[email protected]> Co-authored-by: Elise Schedler <[email protected]> Co-authored-by: ghander <[email protected]> Co-authored-by: PopeLaz <[email protected]>

…onstant

achow101 · 2022-10-12T19:36:34Z

cc @mzumsande

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

mzumsande

Concept ACK.

git grep "1024 / 1024" shows a few more spots that could make use of the constant.

mzumsande · 2022-12-09T16:53:09Z

configure.ac

  AX_CHECK_COMPILE_FLAG([-Wunreachable-code-loop-increment], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunreachable-code-loop-increment"], [], [$CXXFLAG_WERROR])
  AX_CHECK_COMPILE_FLAG([-Wimplicit-fallthrough], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wimplicit-fallthrough"], [], [$CXXFLAG_WERROR])
+  AX_CHECK_COMPILE_FLAG([-Wsign-compare], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wsign-compare"], [], [$CXXFLAG_WERROR])
+  AX_CHECK_COMPILE_FLAG([-Wsign-conversion], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wsign-conversion"], [], [$CXXFLAG_WERROR])


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:

mzumsande · 2022-12-09T17:28:58Z

src/constants.h

+static constexpr uint64_t GB_BYTES{1000000000};
+
+/* One mebibyte (MiB) in bytes */
+static constexpr int32_t MIB_BYTES{1024 * 1024};


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.

DrahtBot · 2023-03-15T23:53:02Z

🐙 This pull request conflicts with the target branch and needs rebase.

achow101 · 2023-04-25T15:41:05Z

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.

Empact mentioned this pull request Jun 16, 2022

doc: Correct comments re. units of constants #25299

Merged

DrahtBot added the Refactoring label Jun 16, 2022

DrahtBot mentioned this pull request Jun 16, 2022

[kernel 3a/n] Decouple CTxMemPool from ArgsManager #25290

Merged

2 tasks

brunoerg reviewed Jun 17, 2022

View reviewed changes

w0xlt reviewed Jun 17, 2022

View reviewed changes

maflcko reviewed Jun 18, 2022

View reviewed changes

DrahtBot mentioned this pull request Jun 21, 2022

Add a -mempoolfullrbf node setting #25353

Merged

DrahtBot added the Needs rebase label Jun 29, 2022

Empact force-pushed the 2022-06-mib-bytes branch from 60236bf to d58422b Compare August 8, 2022 05:35

DrahtBot removed the Needs rebase label Aug 8, 2022

This was referenced Aug 8, 2022

Remove almost all blockstorage globals #25781

Merged

BIP324: Cipher suite #25361

Closed

BIP324: Handshake prerequisites #23561

Closed

BIP324: Add encrypted p2p transport {de}serializer #23233

Closed

DrahtBot added the Needs rebase label Sep 1, 2022

Empact and others added 2 commits October 10, 2022 14:32

refactor: Expose MIN_MIB_FOR_BLOCK_FILES, MIN_BYTES_FOR_BLOCK_FILES c…

28c7d4c

…onstant

Empact force-pushed the 2022-06-mib-bytes branch 3 times, most recently from 98b8176 to 6b64215 Compare October 10, 2022 18:59

DrahtBot removed the Needs rebase label Oct 10, 2022

This was referenced Oct 11, 2022

refactor: add kernel/cs_main.h #26251

Merged

BIP324: Enable v2 P2P encrypted transport #24545

Closed

Expose MIN_PRUNE_TARGET_GB in guiconstants.h

86e1d59

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

Empact force-pushed the 2022-06-mib-bytes branch from 6b64215 to 86e1d59 Compare October 12, 2022 20:07

build: Enable -Wsign-compare and -Wsign-conversion

5b5b73c

DrahtBot mentioned this pull request Oct 21, 2022

Intro: Never change the prune checkbox after the user has touched it bitcoin-core/gui#658

Merged

This was referenced Nov 19, 2022

Enable -Wstring-concatenation and -Wstring-conversion on clang builds #26288

Closed

build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set #25972

Merged

DrahtBot mentioned this pull request Dec 8, 2022

Enable users to configure their monospace font specifically bitcoin-core/gui#497

Merged

mzumsande reviewed Dec 9, 2022

View reviewed changes

DrahtBot mentioned this pull request Dec 10, 2022

Add reindex=auto flag to automatically reindex corrupt data #26674

Closed

DrahtBot mentioned this pull request Dec 17, 2022

test/BIP324: functional tests for v2 P2P encryption #24748

Merged

DrahtBot added the Needs rebase label Mar 15, 2023

achow101 closed this Apr 25, 2023

bitcoin locked and limited conversation to collaborators Apr 24, 2024

Conversation

Empact commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jun 16, 2022

Uh oh!

DrahtBot commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 18, 2022

Choose a reason for hiding this comment

Uh oh!

laanwj Jun 20, 2022

Choose a reason for hiding this comment

Uh oh!

Empact Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

Empact Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

Empact Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

achow101 commented Oct 12, 2022

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

mzumsande Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Mar 15, 2023

Uh oh!

achow101 commented Apr 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Empact commented Jun 16, 2022 •

edited

Loading

DrahtBot commented Jun 16, 2022 •

edited

Loading

Empact Oct 10, 2022 •

edited

Loading