refactor: use `_MiB`/`_GiB` consistently for byte conversions #34435

l0rinc commented at 9:26 PM on January 28, 2026: contributor

Problem

Byte-size conversions in the codebase currently show up in many equivalent formats (multiplication/division chains, shifts, hex/binary literals), which creates a maintenance burden and makes review error-prone - especially considering the architectural differences of size_t. Inspired by #34305 (review), it seemed appropriate to unify Mebibyte usage across the codebase and add Gibibyte support with 32/64 bit size_t validation.

Fix

This PR refactors those call sites to use ""_MiB (existing) and ""_GiB (new), and adds the encountered value/pattern replacements to unit tests to make review straightforward, and to ensure the conversions remain valid. The literals are overflow-checked when converting to size_t, and unit tests cover the 32-bit boundary cases.

Concretely, it replaces patterns such as:

1024*1024, 1<<20, 0x100000, 1048576, / 1024 / 1024, * (1.0 / 1024 / 1024) → 1_MiB or double(1_MiB)
1024*1024*1024, 1<<30, 0x40000000, 1024_MiB, >> 30 → 1_GiB

Note

In the few places where arithmetic involves signed values, the result is identical to the previous code assuming those quantities never become negative.

DrahtBot added the label Refactoring on Jan 28, 2026

DrahtBot commented at 9:26 PM on January 28, 2026: 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
ACK	hodlinator, w0xlt
Concept ACK	sedited

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35043 (refactor: Properly return from ThreadSafeQuestion signal + btcsignals follow-ups by maflcko)
#34937 (Fix startup failure with RLIM_INFINITY fd limits by Sjors)
#34806 (refactor: logging: Various API improvements by ajtowns)
#34641 (node: scale default -dbcache with system RAM by l0rinc)
#34628 (p2p: Replace per-peer transaction rate-limiting with global rate limits by ajtowns)
#34534 (rpc: Manual prune lock management (Take 2) by fjahr)
#34132 (coins: drop error catcher, centralize fatal read handling by l0rinc)
#32427 ((RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store by sedited)
#32317 (kernel: Separate UTXO set access from validation functions by sedited)
#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

randomenv.cpp → // Read only the first 1 Mbyte [Uses “Mbyte” (decimal MB wording) while the code reads 1_MiB (mebibytes); the unit may be misread without guessing intent.]
blockfilterindex.cpp → /** The pre-allocation chunk size for fltr?????.dat files */ [The filename pattern fltr?????.dat is unclear/looks like a broken placeholder, which harms comprehension of what files the constant refers to.]
blockstorage.h → /** The pre-allocation chunk size for blk?????.dat files (since 0.8) */ [Unclear filename pattern blk?????.dat makes it difficult to understand which files the comment is describing.]
blockstorage.h → /** The pre-allocation chunk size for rev?????.dat files (since 0.8) */ [Unclear filename pattern rev?????.dat makes it difficult to understand which files the comment is describing.]

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

BlockFilterIndex(interfaces::MakeChain(m_node), BlockFilterType::BASIC, 1_MiB, true) in src/test/blockfilter_index_tests.cpp
InitBlockFilterIndex([&]{ return interfaces::MakeChain(m_node); }, BlockFilterType::BASIC, 1_MiB, true, false) in src/test/blockfilter_index_tests.cpp
InitBlockFilterIndex([&]{ return interfaces::MakeChain(m_node); }, BlockFilterType::BASIC, 1_MiB, true, false) in src/test/blockfilter_index_tests.cpp
InitBlockFilterIndex([&]{ return interfaces::MakeChain(m_node); }, BlockFilterType::BASIC, 1_MiB, true, false) in src/test/blockfilter_index_tests.cpp
CoinStatsIndex{interfaces::MakeChain(m_node), 1_MiB, true} in src/test/coinstatsindex_tests.cpp
CoinStatsIndex{interfaces::MakeChain(m_node), 1_MiB} in src/test/coinstatsindex_tests.cpp
CoinStatsIndex{interfaces::MakeChain(m_node), 1_MiB} in src/test/coinstatsindex_tests.cpp
TxIndex(interfaces::MakeChain(m_node), 1_MiB, true) in src/test/txindex_tests.cpp

2026-04-06 15:42:36

l0rinc force-pushed on Jan 28, 2026

DrahtBot added the label CI failed on Jan 28, 2026

DrahtBot commented at 9:34 PM on January 28, 2026: contributor

🚧 At least one of the CI tasks failed. Task ASan + LSan + UBSan + integer: https://github.com/bitcoin/bitcoin/actions/runs/21456037564/job/61797017546 LLM reason (✨ experimental): Compilation failed due to invalid user-defined literal operators in util/byte_units.h (invalid parameter type for operator""_MiB/_GiB leading to no matching literal operator).

<details><summary>Hints</summary>

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.

</details>

l0rinc closed this on Jan 28, 2026

l0rinc force-pushed on Jan 28, 2026

l0rinc reopened this on Jan 28, 2026

l0rinc force-pushed on Jan 28, 2026

DrahtBot removed the label CI failed on Jan 28, 2026

in src/util/byte_units.h:16 in 0768e6bc84 outdated

  13 |  
  14 | -//! Overflow-safe conversion of MiB to bytes.
  15 | -constexpr size_t operator""_MiB(unsigned long long mebibytes)
  16 | +namespace util::detail {
  17 | +template<unsigned SHIFT>
  18 | +constexpr size_t ByteUnitsToBytes(unsigned long long units, const std::string& exception_msg)

hodlinator commented at 3:52 PM on January 29, 2026:

Should use string_view or const char* to avoid heap allocations for the happy path.

constexpr size_t ByteUnitsToBytes(unsigned long long units, std::string_view exception_msg)

(Edit: I realize this is only run in a constexpr context so it's probably negligible in practice, but would still be nice to fix).

l0rinc commented at 6:02 PM on January 29, 2026:

I had that originally but had some problems, don't remember what. Will try again on next push. Edit: it's used in std::overflow_error which expects a String:

  _LIBCPP_HIDE_FROM_ABI explicit overflow_error(const string& __s) : runtime_error(__s) {}
  _LIBCPP_HIDE_FROM_ABI explicit overflow_error(const char* __s) : runtime_error(__s) {}

hodlinator commented at 7:32 PM on January 29, 2026:

Yes, so why not use const char* to avoid the "heap" allocation (usually in constexpr/consteval) in cases where the function succeeds and the exception isn't even thrown?

l0rinc commented at 7:53 PM on January 29, 2026:

That would be true without constexpr, as shown in https://godbolt.org/z/TGb57r7zz. But with constexpr it has zero runtime impact since the exception string is completely optimized away e.g. 8_MiB and friends are always compile-time constants, see https://godbolt.org/z/7rhfqc39f. But it would eliminate a string import, so I don't mind, pushed.

in src/qt/optionsmodel.h:30 in 0768e6bc84 outdated

  26 | @@ -26,12 +27,12 @@ static constexpr uint16_t DEFAULT_GUI_PROXY_PORT = 9050;
  27 |  /**
  28 |   * Convert configured prune target MiB to displayed GB. Round up to avoid underestimating max disk usage.
  29 |   */
  30 | -static inline int PruneMiBtoGB(int64_t mib) { return (mib * 1024 * 1024 + GB_BYTES - 1) / GB_BYTES; }
  31 | +static inline int PruneMiBtoGB(int64_t mib) { return (mib * 1_MiB + GB_BYTES - 1) / GB_BYTES; }

hodlinator commented at 4:06 PM on January 29, 2026:

I wonder if multiplying with size_t instead of unspecified integer expression risks flipping negative function inputs into positive output values, but it's probably only used with positive inputs regardless.

l0rinc commented at 6:47 PM on January 29, 2026:

https://github.com/bitcoin/bitcoin/blob/25884bd89684262b94e09c9904c1560d5ba87d86/src/qt/intro.cpp#L35 indicates this cannot be negative

hodlinator commented at 9:41 PM on January 29, 2026:

Experimented a bit and it seems behavior hasn't changed. Passing test:

constexpr uint64_t GB_BYTES{1000000000};
static inline int PruneMiBtoGB_old(int64_t mib) { return (mib * 1024 * 1024 + GB_BYTES - 1) / GB_BYTES; }
static inline int PruneMiBtoGB_new(int64_t mib) { return (mib * 1_MiB + GB_BYTES - 1) / GB_BYTES; }
static inline int64_t PruneGBtoMiB_old(int gb) { return gb * GB_BYTES / 1024 / 1024; }
static inline int64_t PruneGBtoMiB_new(int gb) { return gb * GB_BYTES / 1_MiB; }

BOOST_AUTO_TEST_CASE(prune_tests)
{
    BOOST_CHECK_EQUAL(PruneMiBtoGB_old(1500), 2);
    BOOST_CHECK_EQUAL(PruneMiBtoGB_new(1500), 2);
    BOOST_CHECK_EQUAL(PruneMiBtoGB_old(-1500), 1266874889);
    BOOST_CHECK_EQUAL(PruneMiBtoGB_new(-1500), 1266874889);

    BOOST_CHECK_EQUAL(PruneGBtoMiB_old(1), 953);
    BOOST_CHECK_EQUAL(PruneGBtoMiB_new(1), 953);
    BOOST_CHECK_EQUAL(PruneGBtoMiB_old(-1), 17592186043462);
    BOOST_CHECK_EQUAL(PruneGBtoMiB_new(-1), 17592186043462);
}

in src/util/byte_units.h:15 in 0768e6bc84

  12 |  #include <stdexcept>
  13 |  
  14 | -//! Overflow-safe conversion of MiB to bytes.
  15 | -constexpr size_t operator""_MiB(unsigned long long mebibytes)
  16 | +namespace util::detail {
  17 | +template<unsigned SHIFT>

hodlinator commented at 4:20 PM on January 29, 2026:

nit: I suspect clang-format prefers:

template <unsigned SHIFT>

$ git grep "template <" src/ | wc -l
807
$ git grep "template<" src/ | wc -l
424

l0rinc commented at 6:04 PM on January 29, 2026:

Thanks, will do it on next push

Edit: pushed this nit + rebased

hodlinator changes_requested

l0rinc force-pushed on Jan 29, 2026

DrahtBot added the label CI failed on Jan 29, 2026

in src/init.cpp:1923 in 112b991463 outdated

1919 | @@ -1919,7 +1920,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1920 |  
1921 |      // On first startup, warn on low block storage space
1922 |      if (!do_reindex && !do_reindex_chainstate && chain_active_height <= 1) {
1923 | -        uint64_t assumed_chain_bytes{chainparams.AssumedBlockchainSize() * 1024 * 1024 * 1024};
1924 | +        uint64_t assumed_chain_bytes{chainparams.AssumedBlockchainSize() * 1_MiB * 1024};

hodlinator commented at 8:49 PM on January 29, 2026:

In 112b9914639aab450f0aadc187f6985f56cb7e70: How about putting the GiB commit first and only touch this line once instead of twice?

l0rinc commented at 9:50 PM on January 29, 2026:

Good idea, haven't thought of that! Pushed - also simplified the gib_string_literal_test to remove some duplication.

DrahtBot removed the label CI failed on Jan 29, 2026

hodlinator commented at 9:42 PM on January 29, 2026: contributor

Concept ACK e5b573f18ac2e13508330ada6e92c8155f86a0e5

Makes the code more readible.

Examples:

-constexpr unsigned int FLTR_FILE_CHUNK_SIZE = 0x100000; // 1 MiB
+constexpr unsigned int FLTR_FILE_CHUNK_SIZE = 1_MiB;

-if (m_total_allocated > 0x1000000) return;
+if (m_total_allocated > 16_MiB) return;

-cuckoo_cache.setup_bytes(megabytes << 20);
+cuckoo_cache.setup_bytes(megabytes * 1_MiB);

-LogDebug(BCLog::COINDB, "Writing final batch of %.2f MiB\n", batch.ApproximateSize() * (1.0 / 1048576.0));
+LogDebug(BCLog::COINDB, "Writing final batch of %.2f MiB\n", batch.ApproximateSize() / double(1_MiB));

-constexpr uint64_t min_disk_space = 52428800; // 50 MiB
+constexpr uint64_t min_disk_space = 50_MiB;

l0rinc force-pushed on Jan 29, 2026

l0rinc requested review from hodlinator on Jan 29, 2026

in src/test/util_tests.cpp:1882 in 792d679a54 outdated

1879 | +    BOOST_CHECK_EQUAL(3_GiB, 3U << 30);
1880 | +
1881 | +    // Overflow handling and specific codebase values
1882 | +    constexpr auto max_gib{std::numeric_limits<size_t>::max() >> 30};
1883 | +    if constexpr (SIZE_MAX == UINT32_MAX) {
1884 | +        BOOST_CHECK_EQUAL(max_gib, 3U);

hodlinator commented at 10:27 PM on January 29, 2026:

remark:

Initial thought: "(2^32) >> 30 should be 2^2 == 4, not 3".
Then I realized uint32_t-max is (2^32)-1 and rounds down.

hodlinator approved

hodlinator commented at 10:36 PM on January 29, 2026: contributor

ACK 792d679a54dcaf359628554fa2681c03d05f1676

Concpt. review: #34435#pullrequestreview-3724982952

Checked trickier hex literals actually match _MiB/_GiB-versions.

Diffed against previously reviewed push and aside from commit re-ordering, only saw minimal deduplication in gib_string_literal_test.

DrahtBot added the label Needs rebase on Feb 7, 2026

l0rinc force-pushed on Feb 8, 2026

DrahtBot removed the label Needs rebase on Feb 8, 2026

hodlinator approved

hodlinator commented at 9:42 AM on February 9, 2026: contributor

re-ACK 6091d9105d88ca4a43385dcb1936cf9a0b6c6b03

Change since previous review: Adjusts the second commit to merge cleanly after surrounding code changed, mainly #includes.

DrahtBot added the label Needs rebase on Feb 19, 2026

l0rinc force-pushed on Feb 19, 2026

DrahtBot added the label CI failed on Feb 20, 2026

DrahtBot removed the label Needs rebase on Feb 20, 2026

DrahtBot added the label Needs rebase on Feb 20, 2026

l0rinc force-pushed on Feb 20, 2026

DrahtBot removed the label Needs rebase on Feb 20, 2026

hodlinator approved

hodlinator commented at 12:22 PM on February 20, 2026: contributor

re-ACK 6069bc9de1ff21db304096b6b5fdfb84d8506ad0

Just changes to surrounding diff context and WARN_FLUSH_COINS_SIZE being replaced by constant no longer measured in bytes.

l0rinc closed this on Feb 20, 2026

l0rinc reopened this on Feb 20, 2026

l0rinc force-pushed on Feb 20, 2026

l0rinc force-pushed on Feb 21, 2026

DrahtBot removed the label CI failed on Feb 21, 2026

hodlinator approved

hodlinator commented at 9:45 AM on February 25, 2026: contributor

re-ACK c8e955ca83750eb85873c4823ad1d2128cf9f28b

(Was just rebased to kick CI).

in src/test/util_tests.cpp:1883 in c8e955ca83 outdated

1880 | +
1881 | +    // Overflow handling and specific codebase values
1882 | +    constexpr auto max_gib{std::numeric_limits<size_t>::max() >> 30};
1883 | +    if constexpr (SIZE_MAX == UINT32_MAX) {
1884 | +        BOOST_CHECK_EQUAL(max_gib, 3U);
1885 | +        BOOST_CHECK_EXCEPTION(4_GiB, std::overflow_error, HasReason("GiB value too large for size_t byte conversion"));

maflcko commented at 2:13 PM on February 26, 2026:

would be nice to add:

        BOOST_CHECK_EQUAL(4*uint64_t{1_GiB}, uint64_t{4} << 30);

An alternative would be to just make GiB return u64, and possibly MiB as well.

There shouldn't be any call-site that really needs size_t. size_t seems like a mental overhead, providing bugs such as #34109 and https://github.com/bitcoin/bitcoin/pull/33724

l0rinc commented at 9:52 AM on February 27, 2026:

Thanks for the suggestion, 32-bit architectures should also support this widening-first approach, so I added it to the test suite.

I dislike size_t too (for the exact reasons you mentioned), and I’d gladly review a PR that addresses that, but it seems more involved than this one.

maflcko commented at 3:51 PM on February 27, 2026:

About size_t being annoying, see also #34692 (comment) :sweat_smile:

l0rinc force-pushed on Feb 27, 2026

l0rinc closed this on Feb 27, 2026

l0rinc reopened this on Feb 27, 2026

DrahtBot added the label CI failed on Feb 27, 2026

DrahtBot removed the label CI failed on Feb 27, 2026

DrahtBot added the label Needs rebase on Mar 6, 2026

l0rinc force-pushed on Mar 6, 2026

DrahtBot removed the label Needs rebase on Mar 6, 2026

in src/test/util_tests.cpp:1850 in 85b9d0b72f outdated

1847 | +    BOOST_CHECK_EQUAL(32_MiB, 0x2000000U);
1848 | +    BOOST_CHECK_EQUAL(32_MiB, 32U << 20);
1849 | +    BOOST_CHECK_EQUAL(50_MiB / 1_MiB, 50U);
1850 | +    BOOST_CHECK_EQUAL(50_MiB, 52428800U);
1851 | +    BOOST_CHECK_EQUAL(128_MiB, 0x8000000U);
1852 | +    BOOST_CHECK_EQUAL(550_MiB, 550ULL * 1024 * 1024);

sedited commented at 12:05 PM on March 9, 2026:

Are these really useful? Feels a bit like testing BOOST_CHECK_EQUAL(1+1,2).

l0rinc commented at 4:27 PM on March 9, 2026:

The reviewers can decide. I covered every change category that I modified in this commit as proof that it doesn't change behavior. I often err on the side of over-testing.

sedited commented at 12:11 PM on March 9, 2026: contributor

Concept ACK

I'm not sure about the first commit, seems a bit noisy to do a conversion from 1024 MiB to 1 GiB for a developer who is probably aware of that fact since shortly after using a computer for the first time. For me at least it is also a good reminder that the unit of -dbcache is MiBs.

The second commit seems like a decent readability improvement though.

DrahtBot added the label Needs rebase on Mar 23, 2026

l0rinc force-pushed on Mar 25, 2026

DrahtBot removed the label Needs rebase on Mar 25, 2026

DrahtBot added the label CI failed on Mar 25, 2026

l0rinc force-pushed on Mar 26, 2026

DrahtBot removed the label CI failed on Mar 26, 2026

DrahtBot added the label Needs rebase on Apr 3, 2026

l0rinc force-pushed on Apr 6, 2026

l0rinc commented at 9:12 AM on April 6, 2026: contributor

Rebased after https://github.com/bitcoin/bitcoin/pull/34448/changes#diff-21abb6b14af1e9330a6f0c89a87231035a439248c556ef5e110eb0617b88a1f4R11, ready for review again.

Edit: had some problems with the new IWYU rules

DrahtBot added the label CI failed on Apr 6, 2026

DrahtBot removed the label Needs rebase on Apr 6, 2026

l0rinc force-pushed on Apr 6, 2026

util: add _GiB for Gibibyte conversions

Introduce `operator""_GiB`, sharing the overflow-checked conversion logic with the existing `operator""_MiB`.

Use `1_GiB` in a few existing places where it is a drop-in replacement (e.g. `1024_MiB`, `1<<30`) and extend unit tests to cover boundary behavior.

459f813bd4

l0rinc force-pushed on Apr 6, 2026

refactor: use _MiB consistently for Mebibyte conversions

Replace hard-coded MiB byte conversions (e.g. `1024*1024`, `1<<20`, `1048576`) with the existing `_MiB` literal to improve readability and avoid repeating constants.

In the few spots where arithmetic involves signed values, the result is identical to the previous code assuming those quantities never turn negative.

Extend unit tests to cover the 32-bit `size_t` overflow boundary and to assert equivalence for integer and floating-point conversions.

a35f04a76a

l0rinc force-pushed on Apr 6, 2026

l0rinc requested review from hodlinator on Apr 6, 2026

l0rinc requested review from maflcko on Apr 6, 2026

l0rinc requested review from sedited on Apr 6, 2026

DrahtBot removed the label CI failed on Apr 6, 2026

hodlinator commented at 11:26 AM on April 7, 2026: contributor

re-ACK a35f04a76a26da58f4168014518783633115697d

Since last review:

Added 4 GiB uint64_t test in gib_string_literal_test as suggested in #34435 (review)
Otherwise just #include and diff context updates.

in src/init.cpp:1840 in a35f04a76a

1837 |      if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
1838 | -        LogInfo("* Using %.1f MiB for transaction index database", index_cache_sizes.tx_index * (1.0 / 1024 / 1024));
1839 | +        LogInfo("* Using %.1f MiB for transaction index database", index_cache_sizes.tx_index / double(1_MiB));
1840 |      }
1841 |      if (args.GetBoolArg("-txospenderindex", DEFAULT_TXOSPENDERINDEX)) {
1842 |          LogInfo("* Using %.1f MiB for transaction output spender index database", index_cache_sizes.txospender_index * (1.0 / 1024 / 1024));

w0xlt commented at 2:36 AM on April 12, 2026:

Likely txospenderindex was merged between rebases and the rebase resolution kept the old pattern.

     if (args.GetBoolArg("-txospenderindex", DEFAULT_TXOSPENDERINDEX)) {
-        LogInfo("* Using %.1f MiB for transaction output spender index database", index_cache_sizes.txospender_index * (1.0 / 1024 / 1024));
+        LogInfo("* Using %.1f MiB for transaction output spender index database", index_cache_sizes.txospender_index / double(1_MiB));
     }

in src/node/blockstorage.h:21 in a35f04a76a

w0xlt commented at 2:38 AM on April 12, 2026:

For IWYU reason (src/node/blockstorage.h gets it transitively through <dbwrapper.h>).

 #include <util/expected.h>
+#include <util/byte_units.h>
 #include <util/fs.h>

in src/node/caches.cpp:33 in a35f04a76a

  33 | +static constexpr size_t MAX_32BIT_DBCACHE{1_GiB};
  34 |  //! Larger default dbcache on 64-bit systems with enough RAM.
  35 | -static constexpr size_t HIGH_DEFAULT_DBCACHE{1024_MiB};
  36 | +static constexpr size_t HIGH_DEFAULT_DBCACHE{1_GiB};
  37 |  //! Minimum detected RAM required for HIGH_DEFAULT_DBCACHE.
  38 |  static constexpr uint64_t HIGH_DEFAULT_DBCACHE_MIN_TOTAL_RAM{4096ULL << 20};

w0xlt commented at 2:40 AM on April 12, 2026:

For consistency,

 //! Minimum detected RAM required for HIGH_DEFAULT_DBCACHE.
-static constexpr uint64_t HIGH_DEFAULT_DBCACHE_MIN_TOTAL_RAM{4096ULL << 20};
+static constexpr uint64_t HIGH_DEFAULT_DBCACHE_MIN_TOTAL_RAM{uint64_t{4} * 1_GiB};

w0xlt commented at 2:42 AM on April 12, 2026: contributor

ACK a35f04a76a26da58f4168014518783633115697d mod below nits.