Repository: bitcoin/bitcoin
File: src/kernel/caches.h (C/C++)

Code before:
// Copyright (c) 2024-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#ifndef BITCOIN_KERNEL_CACHES_H
#define BITCOIN_KERNEL_CACHES_H

#include <util/check.h>

#include <algorithm>
#include <bit>
#include <cstdint>
#include <limits>

//! Suggested default amount of cache reserved for the kernel (MiB)
static constexpr int64_t DEFAULT_KERNEL_CACHE{450};
//! Max memory allocated to block tree DB specific cache (MiB)
static constexpr int64_t MAX_BLOCK_DB_CACHE{2};
//! Max memory allocated to coin DB specific cache (MiB)
static constexpr int64_t MAX_COINS_DB_CACHE{8};

//! Guard against truncation of values before converting.
constexpr size_t MiBToBytes(int64_t mib)
{
    Assert(std::countl_zero(static_cast<uint64_t>(mib)) >= 21); // Ensure sign bit is unset + enough zeros to shift.
    const int64_t bytes{mib << 20};
    Assert(static_cast<uint64_t>(bytes) <= std::numeric_limits<size_t>::max());
    return static_cast<size_t>(bytes);
}

namespace kernel {
struct CacheSizes {
    size_t block_tree_db;
    size_t coins_db;
    size_t coins;

    CacheSizes(size_t total_cache)
    {
        block_tree_db = std::min(total_cache / 8, MiBToBytes(MAX_BLOCK_DB_CACHE));
        total_cache -= block_tree_db;
        coins_db = std::min(total_cache / 2, MiBToBytes(MAX_COINS_DB_CACHE));
        total_cache -= coins_db;
        coins = total_cache; // the rest goes to the coins cache
    }
};
} // namespace kernel

#endif // BITCOIN_KERNEL_CACHES_H

Review comment (bug):
I'm not sure we need `MiBToBytes()` at all. All but one callsite can be eliminated by just storing the constants as `size_t` byte amounts, allowing compile-time guarantees that we're not overflowing the system's `size_t` size. It adds a few more right shift operations, but those aren't prone to overflowing, and are only required for viewing purposes.

The only place where we're dealing with run-time values (in `CalculateCacheSizes()`, we already partially (and buggily, as per my [other comment](https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907214794)) implemented the checks, so I'd suggest inlining the function there.

<details>
<summary>git diff on 578654c686</summary>

```diff
diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
index 46ca35ea90..51c482607a 100644
--- a/src/bitcoin-chainstate.cpp
+++ b/src/bitcoin-chainstate.cpp
@@ -123,7 +123,7 @@ int main(int argc, char* argv[])
     util::SignalInterrupt interrupt;
     ChainstateManager chainman{interrupt, chainman_opts, blockman_opts};
 
-    kernel::CacheSizes cache_sizes{MiBToBytes(DEFAULT_KERNEL_CACHE)};
+    kernel::CacheSizes cache_sizes{DEFAULT_KERNEL_CACHE};
     node::ChainstateLoadOptions options;
     auto [status, error] = node::LoadChainstate(chainman, cache_sizes, options);
     if (status != node::ChainstateLoadStatus::SUCCESS) {
diff --git a/src/init.cpp b/src/init.cpp
index 1ae968760d..300b46a3d2 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -489,7 +489,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
     argsman.AddArg("-conf=<file>", strprintf("Specify path to read-only configuration file. Relative paths will be prefixed by datadir location (only useable from command line, not configuration file) (default: %s)", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS);
     argsman.AddArg("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS);
-    argsman.AddArg("-dbcache=<n>", strprintf("Maximum database cache size <n> MiB (minimum %d, default: %d). Make sure you have enough RAM. In addition, unused memory allocated to the mempool is shared with this cache (see -maxmempool).", MIN_DB_CACHE, DEFAULT_DB_CACHE), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    argsman.AddArg("-dbcache=<n>", strprintf("Maximum database cache size <n> MiB (minimum %d, default: %d). Make sure you have enough RAM. In addition, unused memory allocated to the mempool is shared with this cache (see -maxmempool).", MIN_DB_CACHE >> 20, DEFAULT_DB_CACHE >> 20), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-includeconf=<file>", "Specify additional configuration file, relative to the -datadir path (only useable from configuration file, not command line)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-allowignoredconf", strprintf("For backwards compatibility, treat an unused %s file in the datadir as a warning, not an error.", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-loadblock=<file>", "Imports blocks from external file on startup", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
diff --git a/src/kernel/caches.h b/src/kernel/caches.h
index 2b5ba9eefc..d30813d284 100644
--- a/src/kernel/caches.h
+++ b/src/kernel/caches.h
@@ -8,25 +8,14 @@
 #include <util/check.h>
 
 #include <algorithm>
-#include <bit>
 #include <cstdint>
-#include <limits>
 
-//! Suggested default amount of cache reserved for the kernel (MiB)
-static constexpr int64_t DEFAULT_KERNEL_CACHE{450};
-//! Max memory allocated to block tree DB specific cache (MiB)
-static constexpr int64_t MAX_BLOCK_DB_CACHE{2};
-//! Max memory allocated to coin DB specific cache (MiB)
-static constexpr int64_t MAX_COINS_DB_CACHE{8};
-
-//! Guard against truncation of values before converting.
-constexpr size_t MiBToBytes(int64_t mib)
-{
-    Assert(std::countl_zero(static_cast<uint64_t>(mib)) >= 21); // Ensure sign bit is unset + enough zeros to shift.
-    const int64_t bytes{mib << 20};
-    Assert(static_cast<uint64_t>(bytes) <= std::numeric_limits<size_t>::max());
-    return static_cast<size_t>(bytes);
-}
+//! Suggested default amount of cache reserved for the kernel (bytes)
+static constexpr size_t DEFAULT_KERNEL_CACHE{450 << 20};
+//! Max memory allocated to block tree DB specific cache (bytes)
+static constexpr size_t MAX_BLOCK_DB_CACHE{2 << 20};
+//! Max memory allocated to coin DB specific cache (bytes)
+static constexpr size_t MAX_COINS_DB_CACHE{8 << 20};
 
 namespace kernel {
 struct CacheSizes {
@@ -36,9 +25,9 @@ struct CacheSizes {
 
     CacheSizes(size_t total_cache)
     {
-        block_tree_db = std::min(total_cache / 8, MiBToBytes(MAX_BLOCK_DB_CACHE));
+        block_tree_db = std::min(total_cache / 8, MAX_BLOCK_DB_CACHE);
         total_cache -= block_tree_db;
-        coins_db = std::min(total_cache / 2, MiBToBytes(MAX_COINS_DB_CACHE));
+        coins_db = std::min(total_cache / 2, MAX_COINS_DB_CACHE);
         total_cache -= coins_db;
         coins = total_cache; // the rest goes to the coins cache
     }
diff --git a/src/node/caches.cpp b/src/node/caches.cpp
index 9dbbccd304..af27d9af0f 100644
--- a/src/node/caches.cpp
+++ b/src/node/caches.cpp
@@ -10,35 +10,35 @@
 #include <logging.h>
 
 #include <algorithm>
+#include <bit>
 #include <optional>
 #include <string>
 
 // Unlike for the UTXO database, for the txindex scenario the leveldb cache make
 // a meaningful difference: https://github.com/bitcoin/bitcoin/pull/8273#issuecomment-229601991
-//! Max memory allocated to tx index DB specific cache in MiB.
-static constexpr int64_t MAX_TX_INDEX_CACHE{1024};
-//! Max memory allocated to all block filter index caches combined in MiB.
-static constexpr int64_t MAX_FILTER_INDEX_CACHE{1024};
+//! Max memory allocated to tx index DB specific cache in bytes.
+static constexpr size_t MAX_TX_INDEX_CACHE{1024 << 20};
+//! Max memory allocated to all block filter index caches combined in bytes.
+static constexpr size_t MAX_FILTER_INDEX_CACHE{1024 << 20};
 
 namespace node {
 std::optional<CacheSizes> CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
 {
-    int64_t db_cache = args.GetIntArg("-dbcache", DEFAULT_DB_CACHE);
-    if (static_cast<uint64_t>(db_cache << 20) > std::numeric_limits<size_t>::max()) {
-        LogWarning("Cannot allocate more than %d MiB in total for db caches.", static_cast<double>(std::numeric_limits<size_t>::max()) * (1.0 / 1024 / 1024));
+    // negative values are permitted, but interpreted as zero.
+    uint64_t db_cache_mib = std::max<int64_t>(0, args.GetIntArg("-dbcache", DEFAULT_DB_CACHE >> 20));
+    constexpr size_t max_mib = std::numeric_limits<size_t>::max() >> 20;
+    if (db_cache_mib > max_mib) {
+        LogWarning("Cannot allocate more than %d MiB for db caches.", max_mib);
         return std::nullopt;
     }
-
-    // negative values are permitted, but interpreted as zero.
-    db_cache = std::max(int64_t{0}, db_cache);
-    size_t total_cache = std::max(MiBToBytes(db_cache), MiBToBytes(MIN_DB_CACHE));
+    size_t total_cache{std::max(static_cast<size_t>(db_cache_mib << 20), MIN_DB_CACHE)};
 
     IndexCacheSizes index_sizes;
-    index_sizes.tx_index = std::min(total_cache / 8, args.GetBoolArg("-txindex", DEFAULT_TXINDEX) ? MiBToBytes(MAX_TX_INDEX_CACHE) : 0);
+    index_sizes.tx_index = std::min(total_cache / 8, args.GetBoolArg("-txindex", DEFAULT_TXINDEX) ? MAX_TX_INDEX_CACHE : 0);
     total_cache -= index_sizes.tx_index;
     index_sizes.filter_index = 0;
     if (n_indexes > 0) {
-        int64_t max_cache = std::min(total_cache / 8, MiBToBytes(MAX_FILTER_INDEX_CACHE));
+        int64_t max_cache = std::min(total_cache / 8, MAX_FILTER_INDEX_CACHE);
         index_sizes.filter_index = max_cache / n_indexes;
         total_cache -= index_sizes.filter_index * n_indexes;
     }
diff --git a/src/node/caches.h b/src/node/caches.h
index fd6b8b4ab8..f52fefd766 100644
--- a/src/node/caches.h
+++ b/src/node/caches.h
@@ -13,10 +13,10 @@
 
 class ArgsManager;
 
-//! min. -dbcache (MiB)
-static constexpr int64_t MIN_DB_CACHE{4};
-//! -dbcache default (MiB)
-static constexpr int64_t DEFAULT_DB_CACHE{DEFAULT_KERNEL_CACHE};
+//! min. -dbcache (bytes)
+static constexpr size_t MIN_DB_CACHE{4 << 20};
+//! -dbcache default (bytes)
+static constexpr size_t DEFAULT_DB_CACHE{DEFAULT_KERNEL_CACHE};
 
 namespace node {
 struct IndexCacheSizes {
diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp
index 76b7852109..2cea1bf6ab 100644
--- a/src/qt/optionsdialog.cpp
+++ b/src/qt/optionsdialog.cpp
@@ -95,7 +95,7 @@ OptionsDialog::OptionsDialog(QWidget* parent, bool enableWallet)
     ui->verticalLayout->setStretchFactor(ui->tabWidget, 1);
 
     /* Main elements init */
-    ui->databaseCache->setRange(MIN_DB_CACHE, std::numeric_limits<int>::max());
+    ui->databaseCache->setRange(MIN_DB_CACHE >> 20, std::numeric_limits<int>::max());
     ui->threadsScriptVerif->setMinimum(-GetNumCores());
     ui->threadsScriptVerif->setMaximum(MAX_SCRIPTCHECK_THREADS);
     ui->pruneWarning->setVisible(false);
diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp
index 2c6f13dc6d..5ff22b0f4c 100644
--- a/src/qt/optionsmodel.cpp
+++ b/src/qt/optionsmodel.cpp
@@ -470,7 +470,7 @@ QVariant OptionsModel::getOption(OptionID option, const std::string& suffix) con
                suffix.empty()          ? getOption(option, "-prev") :
                                          DEFAULT_PRUNE_TARGET_GB;
     case DatabaseCache:
-        return qlonglong(SettingToInt(setting(), DEFAULT_DB_CACHE));
+        return qlonglong(SettingToInt(setting(), DEFAULT_DB_CACHE >> 20));
     case ThreadsScriptVerif:
         return qlonglong(SettingToInt(setting(), DEFAULT_SCRIPTCHECK_THREADS));
     case Listen:
@@ -733,7 +733,7 @@ void OptionsModel::checkAndMigrate()
         // see https://github.com/bitcoin/bitcoin/pull/8273
         // force people to upgrade to the new value if they are using 100MB
         if (settingsVersion < 130000 && settings.contains("nDatabaseCache") && settings.value("nDatabaseCache").toLongLong() == 100)
-            settings.setValue("nDatabaseCache", (qint64)DEFAULT_DB_CACHE);
+            settings.setValue("nDatabaseCache", (qint64)DEFAULT_DB_CACHE >> 20);
 
         settings.setValue(strSettingsVersionKey, CLIENT_VERSION);
     }

```
</details>

Note: implementing something like this diff should also address all the other comments in this review.


Code after:
// Copyright (c) 2024-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#ifndef BITCOIN_KERNEL_CACHES_H
#define BITCOIN_KERNEL_CACHES_H

#include <util/byte_units.h>

#include <algorithm>

//! Suggested default amount of cache reserved for the kernel (bytes)
static constexpr size_t DEFAULT_KERNEL_CACHE{450_MiB};
//! Max memory allocated to block tree DB specific cache (bytes)
static constexpr size_t MAX_BLOCK_DB_CACHE{2_MiB};
//! Max memory allocated to coin DB specific cache (bytes)
static constexpr size_t MAX_COINS_DB_CACHE{8_MiB};

namespace kernel {
struct CacheSizes {
    size_t block_tree_db;
    size_t coins_db;
    size_t coins;

    CacheSizes(size_t total_cache)
    {
        block_tree_db = std::min(total_cache / 8, MAX_BLOCK_DB_CACHE);
        total_cache -= block_tree_db;
        coins_db = std::min(total_cache / 2, MAX_COINS_DB_CACHE);
        total_cache -= coins_db;
        coins = total_cache; // the rest goes to the coins cache
    }
};
} // namespace kernel

#endif // BITCOIN_KERNEL_CACHES_H