refactor: Use u64 over size_t for all cache sizes to fix a 32-bit overflow #35616

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2606-fix-u32 changing 5 files +37 −28
  1. maflcko commented at 9:02 AM on June 28, 2026: member

    This is a refactor on 64-bit systems, because size_t is equal to u64.

    However, on 32-bit systems, it fixes an integer overflow while calculating the cache sizes:

    src/node/caches.cpp:71:49: runtime error: unsigned integer overflow: 471859200 * 10 cannot be represented in type size_t (aka "unsigned int")
    

    This happens while multiplying the default cache size (450MiB) by 10:

    index_sizes.tx_index = std::min(total_cache * 10 / 100, ...)
                                    ^^^^^^^^^^^^^^^^
    

    The issue was introduced in commit d06dabf26bea7d9ca8d635e8338f64aec74c56a8.


    This change follows similar changed one in the past, like 3789215f73466606eb111714f596a2a5e9bb1933, ac76d94117be70d2dcc23ba34b120b44aeb3b0c1, or 28a523fb94d333fd8a28ca101cce746157a90fb6.

    Generally, using fixed sized integer types for calculations is beneficial, because all platforms behave exactly the same way. With platform-dependent types there is a risk that the same calculation yields different results. This has several resulting benefits:

    • Easier review, because there is no need to review the same code several times for each supported platform.
    • Easier quality assurance, because there is less need to run the same code several times in sanitizers for each supported platform, which is tedious.

    There are also no downsides, because there is no measurable overhead on 32-bit for u64 calculations that are done only once in the lifetime of the program. Also, there is no measurable memory overhead when a few fields on 32-bit store some extra zero bytes.


    As said, testing is only possible by picking one of the tedious options:

    • Apply a diff on 64-bit arch and compile with -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DSANITIZERS=integer
    diff --git a/src/node/caches.cpp b/src/node/caches.cpp
    index c98b8ce604..cfd49b60d3 100644
    --- a/src/node/caches.cpp
    +++ b/src/node/caches.cpp
    @@ -58,3 +58,3 @@ CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
     {
    -    size_t total_cache{CalculateDbCacheBytes(args)};
    +    uint32_t total_cache(CalculateDbCacheBytes(args));
     
    @@ -72,6 +72,6 @@ CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
         IndexCacheSizes index_sizes;
    -    index_sizes.tx_index = std::min(total_cache * 10 / 100, args.GetBoolArg("-txindex", DEFAULT_TXINDEX) ? MAX_TX_INDEX_CACHE : 0);
    -    index_sizes.txospender_index = std::min(total_cache * 5 / 100, args.GetBoolArg("-txospenderindex", DEFAULT_TXOSPENDERINDEX) ? MAX_TXOSPENDER_INDEX_CACHE : 0);
    +    index_sizes.tx_index = std::min<uint32_t>(total_cache * 10 / 100, args.GetBoolArg("-txindex", DEFAULT_TXINDEX) ? MAX_TX_INDEX_CACHE : 0);
    +    index_sizes.txospender_index = std::min<uint32_t>(total_cache * 5 / 100, args.GetBoolArg("-txospenderindex", DEFAULT_TXOSPENDERINDEX) ? MAX_TXOSPENDER_INDEX_CACHE : 0);
         if (n_indexes > 0) {
    -        size_t max_cache = std::min(total_cache * 5 / 100, MAX_FILTER_INDEX_CACHE);
    +        size_t max_cache = std::min<uint32_t>(total_cache * 5 / 100, MAX_FILTER_INDEX_CACHE);
             index_sizes.filter_index = max_cache / n_indexes;
    

    This will give a roughly similar error:

    sh-5.3$ echo 'Bw==' | base64 -d > /tmp/blob
    sh-5.3$ UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=block_index_tree ./bld-cmake/bin/fuzz /tmp/blob
    ./src/node/caches.cpp:73:59: runtime error: unsigned integer overflow: 1073741824 * 10 cannot be represented in type 'uint32_t' (aka 'unsigned int')
        [#0](/github-metadata-backup-bitcoin-bitcoin/0/) 0x55ca78da5c47 in node::CalculateCacheSizes(ArgsManager const&, unsigned long) ./src/node/caches.cpp:73:59
    
    • Alternatively, to reproduce in a fresh podman run -it --rm --platform linux/i386 debian:unstable:
    export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git   ./b-c && cd b-c && apt install  build-essential cmake pkg-config  python3-zmq libzmq3-dev libevent-dev libboost-dev libsqlite3-dev  systemtap-sdt-dev  libcapnp-dev capnproto  libqrencode-dev qt6-tools-dev qt6-l10n-tools qt6-base-dev  clang llvm libc++-dev libc++abi-dev  mold -y   &&  cmake -B ./bld-cmake -DAPPEND_CXXFLAGS='-O3 -g2' -DAPPEND_CFLAGS='-O3 -g2' -DCMAKE_BUILD_TYPE=Debug -DCMAKE_EXE_LINKER_FLAGS=-fuse-ld=mold -DCMAKE_C_COMPILER='clang;-ftrivial-auto-var-init=pattern' -DCMAKE_CXX_COMPILER='clang++;-ftrivial-auto-var-init=pattern' -DSANITIZERS=address,float-divide-by-zero,integer,undefined --preset=dev-mode                              && cmake --build ./bld-cmake --parallel  $(nproc)
    
    echo 'Bw==' | base64 -d > /tmp/blob
    
    UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=block_index_tree ./bld-cmake/bin/fuzz /tmp/blob
    
    # or:
    
    UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ASAN_OPTIONS="detect_leaks=0"  ./bld-cmake/bin/test_bitcoin-qt
    
    
  2. DrahtBot added the label Refactoring on Jun 28, 2026
  3. DrahtBot commented at 9:02 AM on June 28, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35616.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline and AI policy for information on the review process.

    Type Reviewers
    ACK sedited, theStack, l0rinc
    Concept ACK fjahr

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #35205 (kernel,node: clean up dbcache helpers and add kernel API by l0rinc)
    • #35200 (node: smooth oversized dbcache warnings by l0rinc)
    • #34844 (util: Add util::NotNull<SmartPtrType> by maflcko)
    • #32427 (kernel: Replace leveldb-based BlockTreeDB with WAL and .dat file based store by sedited)

    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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. maflcko force-pushed on Jun 28, 2026
  5. DrahtBot added the label CI failed on Jun 28, 2026
  6. DrahtBot commented at 9:59 AM on June 28, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task i686, no IPC: https://github.com/bitcoin/bitcoin/actions/runs/28317284514/job/83892802764</sub> <sub>LLM reason (✨ experimental): CI failed due to a C++ build error: -Werror=narrowing treated as fatal when assigning uint64_t (block_tree_db) to size_t in src/init.cpp.</sub>

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

  7. maflcko force-pushed on Jun 28, 2026
  8. DrahtBot removed the label CI failed on Jun 28, 2026
  9. fjahr commented at 7:20 PM on June 28, 2026: contributor

    Concept ACK

  10. sedited commented at 9:01 PM on June 28, 2026: contributor

    Since we recently discussed removing the index caches, might we just do that instead?

  11. maflcko marked this as a draft on Jun 29, 2026
  12. maflcko added this to the milestone 32.0 on Jun 29, 2026
  13. maflcko commented at 7:07 AM on June 29, 2026: member

    Since we recently discussed removing the index caches, might we just do that instead?

    Hmm, I may have missed the discussion. Maybe someone can create a separate pull, and then this one can be closed/rebased? (I've moved to draft for now)

    Personally, I'd still think even without the indexes, the change here should be done for the other caches as a belt-and-suspenders refactor.

  14. sedited commented at 4:50 PM on June 29, 2026: contributor

    Hmm, I may have missed the discussion. Maybe someone can create a separate pull, and then this one can be closed/rebased?

    It was brought up on a recent IRC meeting: https://www.erisian.com.au/bitcoin-core-dev/log-2026-06-18.html#l-317

    (I've moved to draft for now) the change here should be done for the other caches as a belt-and-suspenders refactor.

    Yes, mind moving it out of draft again?

  15. l0rinc commented at 5:52 PM on June 29, 2026: contributor

    Since we recently discussed removing the index caches, might we just do that instead?

    Pushed it to https://github.com/bitcoin/bitcoin/pull/35620

  16. in src/node/caches.h:17 in fabd75f41d outdated
      13 |  
      14 |  class ArgsManager;
      15 |  
      16 |  //! min. -dbcache (bytes)
      17 | -static constexpr size_t MIN_DB_CACHE{4_MiB};
      18 | +static constexpr uint64_t MIN_DB_CACHE{4_MiB};
    


    l0rinc commented at 5:58 PM on June 29, 2026:

    A few of these have already been migrated in #35205 - I personally would prefer merging that before this one as it has the side-effect of already cleaning up a few of the remaining types.


    maflcko commented at 5:53 AM on June 30, 2026:

    A few of these have already been migrated in #35205

    Looks like the pull went through a few iterations. What is the latest status of it. Are all commits of it refactor commits (that do not change behavior), except for the new kernel API? Or are the some other behavior changes?


    l0rinc commented at 6:47 PM on June 30, 2026:

    Are all commits of it refactor commits (that do not change behavior)

    Besides the total-memory queries being cached and the system RAM tests not skipping on detection failure now, the rest of #35205 is behavior-preserving.


    maflcko commented at 2:43 PM on July 8, 2026:

    Looks like there is outstanding review in that pull, so I'll close this thread for now.

  17. in src/node/caches.cpp:54 in fabd75f41d outdated
      52 |          if (*db_cache < 0) db_cache = 0;
      53 |          const uint64_t db_cache_bytes{SaturatingLeftShift<uint64_t>(*db_cache, 20)};
      54 | -        constexpr auto max_db_cache{sizeof(void*) == 4 ? MAX_32BIT_DBCACHE : std::numeric_limits<size_t>::max()};
      55 | -        return std::max<size_t>(MIN_DB_CACHE, std::min<uint64_t>(db_cache_bytes, max_db_cache));
      56 | +        constexpr uint64_t max_db_cache{sizeof(void*) == 4 ? MAX_32BIT_DBCACHE : std::numeric_limits<uint64_t>::max()};
      57 | +        return std::max<uint64_t>(MIN_DB_CACHE, std::min<uint64_t>(db_cache_bytes, max_db_cache));
    


    l0rinc commented at 6:02 PM on June 29, 2026:
            return std::max(MIN_DB_CACHE, std::min(db_cache_bytes, max_db_cache));
    

    maflcko commented at 5:50 AM on June 30, 2026:

    Could probably use std::clamp here instead


    l0rinc commented at 6:39 AM on June 30, 2026:

    max/min was chosen exactly the avoid the potential ambiguity of clamp for cases when the min isn't smaller than the max...


    maflcko commented at 7:43 AM on June 30, 2026:

    Right, but I wonder if it matters. I also wondered if it makes sense to introduce a safe Clamp<MIN,MAX>(val), but not sure if it is worth it.


    l0rinc commented at 6:12 PM on June 30, 2026:

    I'm fine with both, already acked.

  18. in src/node/caches.cpp:90 in fabd75f41d outdated
      86 | @@ -85,7 +87,7 @@ CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
      87 |  void LogOversizedDbCache(const ArgsManager& args) noexcept
      88 |  {
      89 |      if (const auto total_ram{GetTotalRAM()}) {
      90 | -        const size_t db_cache{CalculateDbCacheBytes(args)};
      91 | +        const uint64_t db_cache{CalculateDbCacheBytes(args)};
    


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

    nit: many of these can be auto now:

            const auto db_cache{CalculateDbCacheBytes(args)};
    

    maflcko commented at 5:50 AM on June 30, 2026:

    Yes, they can, but I don't think they should. I don't think https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-auto.html flags them either.


    l0rinc commented at 6:12 PM on June 30, 2026:

    We don't have mixed types in this area anymore - but it's just a nit regardless.

  19. l0rinc approved
  20. l0rinc commented at 7:42 PM on June 29, 2026: contributor

    ACK fabd75f41dd948930f4989db2c34831683d92ef9

    Personally I would prefer the other cleanups be merged before this, but I don't mind if this gets in first - it's a good change.

    Rebased locally, all tests are passing, changes make sense. Left some nits.

  21. DrahtBot requested review from fjahr on Jun 29, 2026
  22. maflcko marked this as ready for review on Jul 6, 2026
  23. maflcko commented at 7:38 AM on July 6, 2026: member

    Yes, mind moving it out of draft again?

    Sure, done

  24. in src/dbwrapper.h:42 in fabd75f41d outdated
      38 | @@ -39,7 +39,7 @@ struct DBParams {
      39 |      //! Location in the filesystem where leveldb data will be stored.
      40 |      fs::path path;
      41 |      //! Configures various leveldb cache settings.
      42 | -    size_t cache_bytes;
      43 | +    uint64_t cache_bytes;
    


    theStack commented at 1:21 PM on July 8, 2026:

    IWYU-nit: should include <cstdint> in this header as well


    maflcko commented at 2:41 PM on July 8, 2026:

    Thx, added all missing includes, according to iwyu in touched files. Should be trivial to re-reviw.

  25. theStack approved
  26. theStack commented at 1:21 PM on July 8, 2026: contributor

    Code-review ACK fabd75f41dd948930f4989db2c34831683d92ef9

  27. sedited approved
  28. sedited commented at 2:09 PM on July 8, 2026: contributor

    ACK fabd75f41dd948930f4989db2c34831683d92ef9

  29. refactor: Use u64 over size_t for all cache sizes to fix a 32-bit overflow
    This is a refactor on 64-bit systems, because size_t is equal to u64.
    
    However, on 32-bit systems, it fixes an integer overflow while calculating the cache sizes:
    
    src/node/caches.cpp:71:49: runtime error: unsigned integer overflow: 471859200 * 10 cannot be represented in type size_t (aka "unsigned int")
    
    This happens while multiplying the default cache size (450MiB) by 10:
    
    index_sizes.tx_index = std::min(total_cache * 10 / 100, ...)
                                    ^^^^^^^^^^^^^^^^
    
    The issue was introduced in commit d06dabf26bea7d9ca8d635e8338f64aec74c56a8.
    
    ====
    
    Also, add missing includes in touched files, according to IWYU.
    fabafd91f1
  30. maflcko force-pushed on Jul 8, 2026
  31. sedited approved
  32. sedited commented at 3:19 PM on July 8, 2026: contributor

    Re-ACK fabafd91f173ea0d870816a49b81d074d5b8dbe2

  33. DrahtBot requested review from theStack on Jul 8, 2026
  34. DrahtBot requested review from l0rinc on Jul 8, 2026
  35. theStack approved
  36. theStack commented at 4:24 PM on July 8, 2026: contributor

    re-ACK fabafd91f173ea0d870816a49b81d074d5b8dbe2

  37. l0rinc commented at 12:23 AM on July 9, 2026: contributor

    ACK fabafd91f173ea0d870816a49b81d074d5b8dbe2

    I would prefer merging #35200 first, but this is also a good change.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-07-09 06:47 UTC