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

(added unit tests for each replacement category to ease review)

Additionally, declarations whose initializer reads a _MiB/_GiB literal are switched to braced initialization so a future oversized value is rejected at compile time through the narrowing check instead of silently truncating.

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	janb84, maflcko, hodlinator, achow101
Concept ACK	sedited
Stale ACK	w0xlt

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:

#35097 (util: Return uint64_t from _MiB operator by maflcko)
#35076 (doc: clarify pruning impact on wallet sync by MemeticMoney)
#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)
#34176 (wallet: crash fix, handle non-writable db directories by furszy)
#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)
#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:

// Read only the first 1 Mbyte -> // Read only the first 1 MiB [The code uses 1_MiB (mebibytes), but the comment says “Mbyte” (megabytes), which is a different unit and could mislead understanding of the cutoff size.]

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 filter_index(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
CoinStatsIndex coin_stats_index{interfaces::MakeChain(m_node), 1_MiB, true} in src/test/coinstatsindex_tests.cpp
TxIndex txindex(interfaces::MakeChain(m_node), 1_MiB, true) in src/test/txindex_tests.cpp
chain.InitCoinsDB(1_MiB, /in_memory=/true, /should_wipe=/false) in src/test/util/chainstate.h
c1.InitCoinsDB(/cache_size_bytes=/8_MiB, /in_memory=/true, /should_wipe=/false) in src/test/validation_chainstate_tests.cpp
c2.InitCoinsDB(/cache_size_bytes=/8_MiB, /in_memory=/true, /should_wipe=/false) in src/test/validation_chainstatemanager_tests.cpp
c2.InitCoinsDB(/cache_size_bytes=/8_MiB, /in_memory=/true, /should_wipe=/false) in src/test/validation_chainstatemanager_tests.cpp

2026-04-20 09:19:46

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);
}

janb84 commented at 12:06 PM on April 20, 2026:

static inline int PruneMiBtoGB(int64_t mib) { 
   Assume(mib >=0)
   return (mib * 1_MiB + GB_BYTES - 1) / GB_BYTES; 
}

NIT (very optional): You could make it a bit more explicit why the arithmetic isn't problematic by adding the Assume precondition assertion.

l0rinc commented at 9:47 AM on April 21, 2026:

Thanks, if I need to touch again I'll consider it

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:

maflcko commented at 11:09 AM on April 17, 2026:

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.

Why would it be more involved? For me, it is just a trivial patch replacing the type, see #35097.

The most involved thing was to play code-golf on the test code, until all compilers accepted it 🫠

Note, I am not asking to change all size_t to uint64_t in the whole codebase in one go, but only allow u64 in the return value for this one operator.

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

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.

maflcko commented at 11:23 AM on April 17, 2026: member

left a comment

l0rinc commented at 1:56 PM on April 17, 2026: contributor

rfm?

maflcko commented at 2:17 PM on April 17, 2026: member

rfm?

I am not against merging it, but I don't see the point in modifying all the lines twice. Let's recall that the goal here is to check the range inside the operator.

Then, changing a line to use the operator like uint32_t bla = 4096_MiB; (without the C++11 narrowing check) seems fragile, because it silently compiles without warning.

It would be better to use the C++11 narrowing check when touching. Otherwise this just looks like a style change, rather than a hardening change.

l0rinc force-pushed on Apr 19, 2026

l0rinc commented at 7:21 PM on April 19, 2026: contributor

Thanks for the review, @maflcko, @w0xlt!

Applied @maflcko's braced-initialization suggestion as a new third commit so the narrowing-check hardening can be reviewed independently from the _MiB refactor (one site in chainstatemanager_args.cpp needed an explicit size_t(...) cast to silence a 32-bit narrowing).

Folded @w0xlt's three suggestions (the missed txospender_index log-line conversion, the <util/byte_units.h> IWYU include in node/blockstorage.h, and uint64_t{4} * 1_GiB for HIGH_DEFAULT_DBCACHE_MIN_TOTAL_RAM) into the second commit.

git range-diff 459f813bd4890e8842e8840a42caf943481cc4be..a35f04a76a26da58f4168014518783633115697d c5ef940129f247388e809cb99347519255487edf..c6bb8a132a3def8b6d11a0382152602806d1e241

l0rinc force-pushed on Apr 19, 2026

DrahtBot added the label CI failed on Apr 19, 2026

DrahtBot commented at 7:28 PM on April 19, 2026: contributor

🚧 At least one of the CI tasks failed. Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/24637097583/job/72034398438 LLM reason (✨ experimental): CI failed because IWYU detected and auto-required header include changes (failure “Failure generated from IWYU”) that were not already committed.

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

DrahtBot removed the label CI failed on Apr 19, 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.

b3edd30aa2

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.

Also switch to brace init on every declaration assigned from `_MiB`/`_GiB` literals so a future oversized value (e.g. `unsigned int x{4096_MiB}`) becomes a compile error through the C++11 narrowing check instead of silently truncating.

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

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>

af0ee28eb6

in src/bench/chacha20.cpp:19 in 535431f0e4

  15 | @@ -15,7 +16,7 @@
  16 |  /* Number of bytes to process per iteration */
  17 |  static const uint64_t BUFFER_SIZE_TINY  = 64;
  18 |  static const uint64_t BUFFER_SIZE_SMALL = 256;
  19 | -static const uint64_t BUFFER_SIZE_LARGE = 1024*1024;
  20 | +static const uint64_t BUFFER_SIZE_LARGE = 1_MiB;

maflcko commented at 7:26 AM on April 20, 2026:

i think it is fine to squash the third commit into the earlier ones. Otherwise, there is extra work for reviewers, because they have to review twice and also go back and forth and check that the second time, all places of the first change were indeed touched again.

If you want to split up commits, you can probably split up the src/util/byte_units.h changes (+unit tests) into the first commit, but no strong opinion, up to you.

l0rinc commented at 9:19 AM on April 20, 2026:

Sure, folded the changes back to the original commits

l0rinc force-pushed on Apr 20, 2026

janb84 commented at 12:45 PM on April 20, 2026: contributor

ACK af0ee28eb623a54e7ff8602864f61cab1f23848d

Using GiB next to MiB seems like a logical next step, data usages are increasing. And consistently using MiB/Gib makes the code easier to reason about. (1_MiB in stead of (1024*1024))

Small very optional NIT, for the rest the change looks good to me.

DrahtBot requested review from w0xlt on Apr 20, 2026

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

  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};
  39 | +static constexpr uint64_t HIGH_DEFAULT_DBCACHE_MIN_TOTAL_RAM{uint64_t{4} * 1_GiB};

maflcko commented at 7:02 AM on April 21, 2026:

nit in b3edd30aa24af3a5f63cbcebb7013e60a6d7b8a4: Looks a bit like a shortcoming that should be fixed up after merge, or this should wait for https://github.com/bitcoin/bitcoin/pull/35097

l0rinc commented at 9:41 AM on April 21, 2026:

This was specifically requested - your follow-up PR will allow us to make this less hacky.

hodlinator commented at 8:47 PM on April 22, 2026:

Things like this line makes me prefer that #35097 be merged first even if it was created as a reaction to this one. Improving the underlying primitive before applying it in more places makes more sense. But we can fix it up later.

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

  17 | @@ -18,6 +18,7 @@
  18 |  #include <streams.h>
  19 |  #include <sync.h>
  20 |  #include <uint256.h>
  21 | +#include <util/byte_units.h> // IWYU pragma: keep

maflcko commented at 7:28 AM on April 21, 2026:

nit in af0ee28eb623a54e7ff8602864f61cab1f23848d: Obviously nothing that can be done here, but after merge, it could make sense to open a tracking issue about this. Maybe iwyu is confusing itself with includes that are already present indirectly?

fanquake commented at 9:37 AM on April 21, 2026:

The one thing that should be done, is explain why this is needed, in an inline comment.

l0rinc commented at 9:43 AM on April 21, 2026:

I'm not sure why IWYU is misfiring here, but the file has obvious _MiB usages. Fixing IWYU is outside the scope of this PR - without this line the CI was failing.

fanquake commented at 9:51 AM on April 21, 2026:

There's no need to fix IWYU, just to document the workaround.

l0rinc commented at 10:25 AM on April 22, 2026:

I have opened a fix in include-what-you-use/include-what-you-use#2014.

The issue was that IWYU could miss that a user-defined literal requires the header declaring its literal operator, and would then incorrectly suggest removing it.

hebasto commented at 2:45 PM on April 24, 2026:

I have opened a fix in include-what-you-use/include-what-you-use#2014.

Thank you!

hebasto commented at 7:42 AM on April 27, 2026:

Leaving a note for myself. The IWYU master branch @ ae18593bd2f3d4b16d8e9921aa95af0afb368490 requires the following change:

--- a/src/index/blockfilterindex.cpp
+++ b/src/index/blockfilterindex.cpp
@@ -19,6 +19,7 @@
 #include <sync.h>
 #include <uint256.h>
 #include <util/check.h>
+#include <util/byte_units.h>
 #include <util/fs.h>
 #include <util/hasher.h>
 #include <util/log.h>

l0rinc commented at 10:58 AM on April 27, 2026:

The reproducer I used adjusted more than that: https://github.com/l0rinc/bitcoin/pull/141/changes/63325eacd6aef3dc76736ff0b8526bf680b101af

in src/node/chainstatemanager_args.cpp:68 in af0ee28eb6

  64 | @@ -64,7 +65,7 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
  65 |          //    script execution cache create the minimum possible cache (2
  66 |          //    elements). Therefore, we can use 0 as a floor here.
  67 |          // 2. Multiply first, divide after to avoid integer truncation.
  68 | -        size_t clamped_size_each = std::max<int64_t>(*max_size, 0) * (1 << 20) / 2;
  69 | +        size_t clamped_size_each{size_t(std::max<int64_t>(*max_size, 0) * 1_MiB / 2)};

maflcko commented at 8:36 AM on April 21, 2026:

nit in af0ee28eb623a54e7ff8602864f61cab1f23848d: I know that maxsigcachesize is a debug-only option for experts, but since you are adding a cast here, and given that can overflow, see also commit ab14d1d6a4a8ef5fe5013150e6c5ebcb5f5e4ea9, it seems better to avoid the overflow.

I think it would be fine to call GetArg<uint32_t>("-maxsigcachesize") here, which allows to drop the max call, and the result should fit into u64, which can then be passed to setup_bytes, which will then apply the min call.

l0rinc commented at 9:44 AM on April 21, 2026:

Do you mind if we do that in a follow-up instead?

maflcko commented at 11:50 AM on April 21, 2026:

Well, I am not sure. I just have a really hard time reviewing this line in the diff. To clarify: The MiB operator may return u32, or u64. However, integer promotion depends on the width of the type.

On master (1<<20) is int (i32), multiplied with i64, is i64, which may overflow signed (UB/crash)

With the changes here, MiB is either u32, or u64. In one case the multiply will yield u64, in the other i64. So for one platform, this is a UB/crash, for the other platform, it is a silent and defined unsigned overflow.

I guess it is just me, and the other reviewers were fine with this, but I find it tedious and pointless to review every line of code twice for every architecture. With #35097, there would only be one consistent behavior and only one review needed.

hodlinator commented at 5:32 PM on April 22, 2026:

I agree that the current change is awkward as the multiplication leads to different signedness on 32 and 64 bit due to integer promotion rules for same/different sized integers. (Whereas on master the behavior has a higher chance of being consistent). How about moving the ending parens of the cast?

        size_t clamped_size_each{size_t(std::max<int64_t>(*max_size, 0)) * 1_MiB / 2};

That way we multiply size_t * size_t, which is either u32 * u32 or u64 * u64.

There's still potential for unsigned overflow, but at least it's not UB. (Would almost be nice to have SaturatingMultiply()).

Edit: I guess even with my change on 32-bit, max_size could be larger than max u32 and therefore still lead to UB when casting to size_t. :\

l0rinc commented at 6:47 PM on April 22, 2026:

Should I just drop this line and we'll fix it in another PR?

hodlinator commented at 9:10 PM on April 22, 2026:

It would feel good with a more complete fix, clamping the input somehow to avoid overflow. Or dropping it from the PR. But if you don't re-touch the PR for other reasons, it can be done in a follow-up IMO.

in src/randomenv.cpp:117 in af0ee28eb6

 113 | @@ -113,7 +114,7 @@ void AddFile(CSHA512& hasher, const char *path)
 114 |              if (n > 0) hasher.Write(fbuf, n);
 115 |              total += n;
 116 |              /* not bothering with EINTR handling. */
 117 | -        } while (n == sizeof(fbuf) && total < 1048576); // Read only the first 1 Mbyte
 118 | +        } while (n == sizeof(fbuf) && total < 1_MiB); // Read only the first 1 Mbyte

maflcko commented at 8:40 AM on April 21, 2026:

nit from the llm in af0ee28eb623a54e7ff8602864f61cab1f23848d: // Read only the first 1 Mbyte -> // Read only the first 1 MiB [The code uses 1_MiB (mebibytes), but the comment says “Mbyte” (megabytes), which is a different unit and could mislead understanding of the cutoff size.]

l0rinc commented at 9:45 AM on April 21, 2026:

I thought about removing the comment completely now - but decided to keep it, if I need to touch again I'll just remove it

in src/test/blockfilter_index_tests.cpp:281 in af0ee28eb6

 277 | @@ -277,14 +278,14 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_init_destroy, BasicTestingSetup)
 278 |      filter_index = GetBlockFilterIndex(BlockFilterType::BASIC);
 279 |      BOOST_CHECK(filter_index == nullptr);
 280 |  
 281 | -    BOOST_CHECK(InitBlockFilterIndex([&]{ return interfaces::MakeChain(m_node); }, BlockFilterType::BASIC, 1 << 20, true, false));
 282 | +    BOOST_CHECK(InitBlockFilterIndex([&]{ return interfaces::MakeChain(m_node); }, BlockFilterType::BASIC, 1_MiB, true, false));

maflcko commented at 8:46 AM on April 21, 2026:

llm nit in the second commit: I know this is unrelated, but it would be nice to use named args for those bools, whose meaning and order is otherwise unclear. Either inside the second commit, or a new commit.

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 filter_index(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
    CoinStatsIndex coin_stats_index{interfaces::MakeChain(m_node), 1_MiB, true} in src/test/coinstatsindex_tests.cpp
    TxIndex txindex(interfaces::MakeChain(m_node), 1_MiB, true) in src/test/txindex_tests.cpp

l0rinc commented at 9:46 AM on April 21, 2026:

Agree, I will either do it in a follow-up or here, if I need to touch the code again.

maflcko commented at 9:35 AM on April 21, 2026: member

lgtm, left some comments/nits

review ACK af0ee28eb623a54e7ff8602864f61cab1f23848d 🖍

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK af0ee28eb623a54e7ff8602864f61cab1f23848d 🖍
7kLrlcqOwueHKhUdFEup7YgZ/HgwqO9GMJqW5/5/Pio6L/rV0Eb9p6jbyR8N0+9w72gj8yKIy/R/WHtPYpl+Dg==

</details>

hodlinator approved

hodlinator commented at 9:20 PM on April 22, 2026: contributor

re-ACK af0ee28eb623a54e7ff8602864f61cab1f23848d

Good to see the anti-narrowing braces.

achow101 commented at 10:23 PM on April 22, 2026: member

ACK af0ee28eb623a54e7ff8602864f61cab1f23848d

achow101 merged this on Apr 22, 2026

achow101 closed this on Apr 22, 2026

l0rinc deleted the branch on Apr 23, 2026

sedited referenced this in commit 75f2bf1011 on Apr 25, 2026