From 9dcbcfc8bbd021a7c9650f9a6b95259adbeab5b1 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Tue, 17 Dec 2024 21:58:47 +0100 Subject: [PATCH] init: Use size_t consistently for cache sizes This avoids having to rely on implicit casts when passing them to the various functions allocating the caches. This also fixes a bug where an incorrect amount of memory was allocated on 32bit platforms. If the db cache size exceeded the maximum size of a 32 bit unsigned integer, it would overflow and allocate much less memory than instructed to. Fix this by returning an error to the user if the passed in dbcache size exceeeds the size of a size_t on that platform. Add a release note for the change in behaviour on 32-bit systems. Also take this opportunity to make the total amounts of cache in the chainstate manager a size_t too. --- doc/release-notes-31483.md | 6 +++++ src/bitcoin-chainstate.cpp | 2 +- src/init.cpp | 6 ++++- src/kernel/caches.h | 4 +-- src/node/caches.cpp | 45 ++++++++++++++++++++++------------ src/node/caches.h | 17 +++++++------ src/node/chainstate.cpp | 2 +- src/test/util/setup_common.cpp | 2 +- src/test/util/setup_common.h | 2 +- src/validation.h | 4 +-- 10 files changed, 58 insertions(+), 32 deletions(-) create mode 100644 doc/release-notes-31483.md diff --git a/doc/release-notes-31483.md b/doc/release-notes-31483.md new file mode 100644 index 00000000000000..9076aca3eff4f4 --- /dev/null +++ b/doc/release-notes-31483.md @@ -0,0 +1,6 @@ +Updated settings +------ + +- On 32-bit systems an error is returned on startup if the user passes a + `-dbcache` value exceeding 4GiB. + diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index 46ca35ea90ad71..51c482607aa8f4 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 a3edeb55241b8e..1ae968760d3211 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1605,7 +1605,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) ReadNotificationArgs(args, kernel_notifications); // cache size calculations - auto [index_cache_sizes, kernel_cache_sizes] = CalculateCacheSizes(args, g_enabled_filter_types.size()); + const auto cache_sizes = CalculateCacheSizes(args, g_enabled_filter_types.size()); + if (!cache_sizes) { + return InitError(_("Failed to calculate cache sizes. Try lowering the -dbcache value.")); + } + auto [index_cache_sizes, kernel_cache_sizes] = *cache_sizes; LogInfo("Cache configuration:"); LogInfo("* Using %.1f MiB for block index database", kernel_cache_sizes.block_tree_db * (1.0 / 1024 / 1024)); diff --git a/src/kernel/caches.h b/src/kernel/caches.h index 9ee5a31e0ae207..e2b687906ac28d 100644 --- a/src/kernel/caches.h +++ b/src/kernel/caches.h @@ -9,8 +9,8 @@ #include -//! Suggested default amount of cache reserved for the kernel (MiB) -static constexpr int64_t DEFAULT_KERNEL_CACHE{450}; +//! Suggested default amount of cache reserved for the kernel (bytes) +static constexpr size_t DEFAULT_KERNEL_CACHE{MiBToBytes(450)}; //! Max memory allocated to block tree DB specific cache (bytes) static constexpr size_t MAX_BLOCK_DB_CACHE{MiBToBytes(2)}; //! Max memory allocated to coin DB specific cache (bytes) diff --git a/src/node/caches.cpp b/src/node/caches.cpp index 21e2654b4e4f7a..9654c8ec6de99d 100644 --- a/src/node/caches.cpp +++ b/src/node/caches.cpp @@ -7,31 +7,46 @@ #include #include #include +#include +#include #include +#include +#include #include // 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{MiBToBytes(1024)}; +//! Max memory allocated to all block filter index caches combined in bytes. +static constexpr size_t MAX_FILTER_INDEX_CACHE{MiBToBytes(1024)}; namespace node { -CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes) +std::optional CalculateCacheSizes(const ArgsManager& args, size_t n_indexes) { - int64_t nTotalCache = (args.GetIntArg("-dbcache", DEFAULT_DB_CACHE) << 20); - nTotalCache = std::max(nTotalCache, MIN_DB_CACHE << 20); - IndexCacheSizes sizes; - sizes.tx_index = std::min(nTotalCache / 8, args.GetBoolArg("-txindex", DEFAULT_TXINDEX) ? MAX_TX_INDEX_CACHE << 20 : 0); - nTotalCache -= sizes.tx_index; - sizes.filter_index = 0; + int64_t db_cache = args.GetIntArg("-dbcache", DEFAULT_DB_CACHE); + + // negative values are permitted, but interpreted as zero. + db_cache = std::max(int64_t{0}, db_cache); + + size_t total_cache = 0; + try { + total_cache = std::max(MiBToBytes(db_cache), MIN_DB_CACHE); + } catch(const std::out_of_range& _) { + LogError("Cannot allocate more than %d MiB in total for db caches.", std::numeric_limits::max() >> 20); + return std::nullopt; + } + + IndexCacheSizes index_sizes; + 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(nTotalCache / 8, MAX_FILTER_INDEX_CACHE << 20); - sizes.filter_index = max_cache / n_indexes; - nTotalCache -= sizes.filter_index * n_indexes; + 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; } - return {sizes, kernel::CacheSizes{static_cast(nTotalCache)}}; + return {{index_sizes, kernel::CacheSizes{total_cache}}}; } } // namespace node diff --git a/src/node/caches.h b/src/node/caches.h index 45a1623e735e95..258266af7c1c1f 100644 --- a/src/node/caches.h +++ b/src/node/caches.h @@ -6,27 +6,28 @@ #define BITCOIN_NODE_CACHES_H #include +#include #include -#include +#include 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{MiBToBytes(4)}; +//! -dbcache default (bytes) +static constexpr size_t DEFAULT_DB_CACHE{DEFAULT_KERNEL_CACHE}; namespace node { struct IndexCacheSizes { - int64_t tx_index; - int64_t filter_index; + size_t tx_index; + size_t filter_index; }; struct CacheSizes { IndexCacheSizes index; kernel::CacheSizes kernel; }; -CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes = 0); +std::optional CalculateCacheSizes(const ArgsManager& args, size_t n_indexes = 0); } // namespace node #endif // BITCOIN_NODE_CACHES_H diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index 660d2c3289c659..3ee557ae62222f 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -46,7 +46,7 @@ static ChainstateLoadResult CompleteChainstateInitialization( try { pblocktree = std::make_unique(DBParams{ .path = chainman.m_options.datadir / "blocks" / "index", - .cache_bytes = static_cast(cache_sizes.block_tree_db), + .cache_bytes = cache_sizes.block_tree_db, .memory_only = options.block_tree_db_in_memory, .wipe_data = options.wipe_block_tree_db, .options = chainman.m_options.block_tree_db}); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 5ede59b82a1807..1c7bfa85d5111e 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -252,7 +252,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts) LOCK(m_node.chainman->GetMutex()); m_node.chainman->m_blockman.m_block_tree_db = std::make_unique(DBParams{ .path = m_args.GetDataDirNet() / "blocks" / "index", - .cache_bytes = static_cast(m_kernel_cache_sizes.block_tree_db), + .cache_bytes = m_kernel_cache_sizes.block_tree_db, .memory_only = true, }); }; diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h index 33ad258457353e..7d7c7b7cd43461 100644 --- a/src/test/util/setup_common.h +++ b/src/test/util/setup_common.h @@ -104,7 +104,7 @@ struct BasicTestingSetup { * initialization behaviour. */ struct ChainTestingSetup : public BasicTestingSetup { - kernel::CacheSizes m_kernel_cache_sizes{node::CalculateCacheSizes(m_args).kernel}; + kernel::CacheSizes m_kernel_cache_sizes{node::CalculateCacheSizes(m_args).value().kernel}; bool m_coins_db_in_memory{true}; bool m_block_tree_db_in_memory{true}; std::function m_make_chainman{}; diff --git a/src/validation.h b/src/validation.h index aea7a4621bbd36..f75337604ad52e 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1067,11 +1067,11 @@ class ChainstateManager //! The total number of bytes available for us to use across all in-memory //! coins caches. This will be split somehow across chainstates. - int64_t m_total_coinstip_cache{0}; + size_t m_total_coinstip_cache{0}; // //! The total number of bytes available for us to use across all leveldb //! coins databases. This will be split somehow across chainstates. - int64_t m_total_coinsdb_cache{0}; + size_t m_total_coinsdb_cache{0}; //! Instantiate a new chainstate. //!