kernel: Move kernel-related cache constants to kernel cache #31483

Missing the (MiB) comment.

Thanks, fixed.

nit: will we have separate structs for different node cache sizes? I think just keeping it CacheSizes and adding the node:: namespace in ambiguous places makes more sense for now?

<details> <summary>git diff on 2e6c43f9c3</summary>

diff --git a/src/node/caches.cpp b/src/node/caches.cpp
index 50e41b8d1e..41bd8c95d0 100644
--- a/src/node/caches.cpp
+++ b/src/node/caches.cpp
@@ -13,11 +13,11 @@
 #include <string>
 
 namespace node {
-std::tuple<IndexCacheSizes, kernel::CacheSizes> CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
+std::tuple<node::CacheSizes, kernel::CacheSizes> CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
 {
     int64_t nTotalCache = (args.GetIntArg("-dbcache", nDefaultDbCache) << 20);
     nTotalCache = std::max(nTotalCache, nMinDbCache << 20); // total cache cannot be less than nMinDbCache
-    IndexCacheSizes sizes;
+    CacheSizes sizes;
     sizes.tx_index = std::min(nTotalCache / 8, args.GetBoolArg("-txindex", DEFAULT_TXINDEX) ? nMaxTxIndexCache << 20 : 0);
     nTotalCache -= sizes.tx_index;
     sizes.filter_index = 0;
diff --git a/src/node/caches.h b/src/node/caches.h
index f4cc14bb23..c47134539b 100644
--- a/src/node/caches.h
+++ b/src/node/caches.h
@@ -14,11 +14,11 @@
 class ArgsManager;
 
 namespace node {
-struct IndexCacheSizes {
+struct CacheSizes {
     int64_t tx_index;
     int64_t filter_index;
 };
-std::tuple<IndexCacheSizes, kernel::CacheSizes> CalculateCacheSizes(const ArgsManager& args, size_t n_indexes = 0);
+std::tuple<node::CacheSizes, kernel::CacheSizes> CalculateCacheSizes(const ArgsManager& args, size_t n_indexes = 0);
 } // namespace node
 
 #endif // BITCOIN_NODE_CACHES_H
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index 46035772f5..fbea90a20b 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 {
-    node::IndexCacheSizes m_index_cache_sizes{std::get<0>(node::CalculateCacheSizes(m_args))};
+    node::CacheSizes m_index_cache_sizes{std::get<0>(node::CalculateCacheSizes(m_args))};
     kernel::CacheSizes m_kernel_cache_sizes{std::get<1>(node::CalculateCacheSizes(m_args))};
     bool m_coins_db_in_memory{true};
     bool m_block_tree_db_in_memory{true};

</details>

My expectation would be that if other non-index caches are added later, they would get their own struct too. This also might make it a bit easier in case the indexes ever get their own process?

Works for me, thanks!

nit: since it doesn't meaningfully change the diff, I would much prefer not to use using here to immediately disambiguate with node::CacheSizes (+ in chainstate.cpp)

<details> <summary>git diff on 2e6c43f9c3</summary>

diff --git a/src/init.cpp b/src/init.cpp
index dd273cfe61..f52933b658 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -120,8 +120,6 @@ using common::AmountErrMsg;
 using common::InvalidPortErrMsg;
 using common::ResolveErrMsg;
 
-using kernel::CacheSizes;
-
 using node::ApplyArgsManOptions;
 using node::BlockManager;
 using node::CalculateCacheSizes;
@@ -1179,7 +1177,7 @@ static ChainstateLoadResult InitAndLoadChainstate(
     NodeContext& node,
     bool do_reindex,
     const bool do_reindex_chainstate,
-    CacheSizes& cache_sizes,
+    kernel::CacheSizes& cache_sizes,
     const ArgsManager& args)
 {
     const CChainParams& chainparams = Params();
diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index 660d2c3289..d4d8828f73 100644
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -29,14 +29,12 @@
 #include <memory>
 #include <vector>
 
-using kernel::CacheSizes;
-
 namespace node {
 // Complete initialization of chainstates after the initial call has been made
 // to ChainstateManager::InitializeChainstate().
 static ChainstateLoadResult CompleteChainstateInitialization(
     ChainstateManager& chainman,
-    const CacheSizes& cache_sizes,
+    const kernel::CacheSizes& cache_sizes,
     const ChainstateLoadOptions& options) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
 {
     auto& pblocktree{chainman.m_blockman.m_block_tree_db};
@@ -172,7 +170,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
     return {ChainstateLoadStatus::SUCCESS, {}};
 }
 
-ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSizes& cache_sizes,
+ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const kernel::CacheSizes& cache_sizes,
                                     const ChainstateLoadOptions& options)
 {
     if (!chainman.AssumedValidBlock().IsNull()) {

</details>

edit: oh, that's less true for the current node::IndexCacheSizes name of course. I personally still prefer keeping the namespace for clarity, but it also fits within the init.cpp coding style.

nit: this is in the wrong commit (2e6c43f9c3aed577678a537b5456adaa31a2cb04)

I'm not sure what the best solution is, but aren't MIN_DB_CACHE and DEFAULT_DB_CACHE node attributes, instead of kernel? Then again, we do need a default total cache size in kernel, so maybe the best approach is to just rename to e.g. MIN_KERNEL_CACHE and DEFAULT_KERNEL_CACHE - or redefine DEFAULT_DB_CACHE{DEFAULT_KERNEL_CACHE};?

Mmh, is there a scenario where DEFAULT_DB_CACHE and DEFAULT_KERNEL_CACHE would ever diverge? The current naming does not seem too confusing to me.

Mmh, is there a scenario where DEFAULT_DB_CACHE and DEFAULT_KERNEL_CACHE would ever diverge?

node could decide to increase the default total cache size (e.g. because it enables certain indexes by default), and that shouldn't affect the kernel default cache size.

The current naming does not seem too confusing to me.

It's not necessarily confusing, but -dbcache is a node setting, not a kernel setting. So I think DEFAULT_KERNEL_CACHE is both more accurate (in kernel/caches.h) and - as per the previous paragraph - more futureproof.

Either way, it's easy to fix in the future. My suggestion feels more logical to me but either is fine, so can be resolved if you disagree.

This member is not used, best to remove until it is?

    nTotalCache = std::max(nTotalCache, MIN_DB_CACHE << 20); // total cache cannot be less than MIN_DB_CACHE

edit: actually, the comment is pretty redundant, might as well just remove it too.

thanks for the suggestion, I removed it.

re: In commit "kernel: Move kernel-related cache constants to kernel cache" (8e929e343e14b0228f6f4841ecf2d85a73afda3b)

Precedes this PR, but this code does not seem to make any sense. (1 << 23) is 8MiB which is the same as MAX_COINS_DB_CACHE. So this is effectively min(total / 2, total / 4 + 8MiB, 8Mib) with the middle argument not doing anything. Could be simplified to std::min(total_cache / 2, MAX_COINS_DB_CACHE << 20) with no change in behavior AFAICT.

Heh, I guess I should have actually done the math here. Will remove.

Would be prudent to ensure no negative values here

        assert(total_cache >= 0);
        block_tree_db = std::min(total_cache / 8, MAX_BLOCK_DB_CACHE << 20);

re: #31483 (review)

Or make total_cache size_t, removing the need for an assert, and also helps signal the unit being bytes? (May require adding casts though).

We also have some implicit narrowing going on here, especially on 32-bit platforms, from int64_t to size_t. Adding curlies trigger an error.

        block_tree_db = {std::min(total_cache / 8, MAX_BLOCK_DB_CACHE << 20)};

I think I'm ~0 on this change. I follow @ryanofsky's reasoning, but it also goes against the work done e.g. in #23962. If kernel::CacheSizes becomes part of the public kernel interface, then keeping these members an int64_t might be more prudent, as per e.g. the link in this comment? But of course, size_t is a pretty natural fit for a size type.

re: #31483 (review)

I agree with changes in the issues you linked to, but I don't think the reasoning in those cases carries over to this one. int32_t is a good choice for transaction sizes and weights, because we know the sizes of transactions are bounded and can't get very big, so it makes sense to use a small type, and we want the bounds to be platform independent so it makes to use fixed width types, and because they are numbers we do arithmetic with, and signed types tend to be safer than unsigned types for arithmetic.

I think size_t is a better choice for the cache size variables in this PR, because these values are eventually used to allocate memory so making them size_t from the beginning is clearer and avoids the need for casts and conversions later on. Also size_t is the standard type to use for this purpose so it's more idiomatic c++. Also maximum cache sizes do depend on the platform, so size_t a safe choice because it is guaranteed to be able to represent the maximum size of any allocated object on a platform. I do think a downside of choosing size_t is that it is unsigned and unsigned types have less intuitive overflow behavior, but if the values are just going to get casted to size_t later anyway, potentially bad overflow behavior is there no matter what.

Yeah, thinking about it more I agree with all of your points. I was a bit alarmed by all the comparison operations we were doing with these (now) size_t types, but they almost all happen inside the constructor. Once the sizes are determined, they don't seem likely to be used for arithmetic operations or comparisons, just for allocating memory.

I think the benefits you outline outweigh the potential downsides, so I'm happy with the new approach.

re: #31483 (review)

My intuition was "why not keep using 64-bit types in case the platform is 32-bit", but 32-bit implies it will not be able to make use of larger than size_t, so size_t is preferable.

Added:

static_assert(sizeof(size_t) == 4);

and built on a 32-bit container (i686-centos).

What is worrying is that I also tested adding an extra zero here:

//! Suggested default amount of cache reserved for the kernel (MiB)
static constexpr int64_t DEFAULT_KERNEL_CACHE{4500};

Shifted left 20 bits, that value should have become greater than 4294967295/max of size_t, but didn't trigger any errors. :warning: See my suggestion in another comment about adding curlies to trigger narrowing conversion errors.

This 10x tweak of that constant is caught by asserts I added to my branch.

In commit "init: Simplify coinsdb cache calculation" (c71fc6ac6e62e5eea96e8aa4e0f43260afc81723)

For background on why previous code was written this way, apparently the first line coins_db = min(total / 2, total / 4 + 8MiB) made sense when it was added in b3ed4236beb7f68e1720ceb3da15e0c3682ef629 from #6102. But when the second line coins_db = min(coins_db, 8MiB) was added later in 32cab91278651d07a11132b7636dc3d21144e616 from #8273, the total / 4 term in the first line became redundant.

It seems like the second change was probably intentional (even though the implementation is confusing) because the commit message says "This avoids that the extra cache memory goes to the much less effective leveldb cache instead of our application-level cache."

re: #31483 (review)

Agree with the simplification mostly. Only way the old calculation was useful was if someone was to tweak nMaxCoinsDBCache to be lower than (1 << 23)/8MiB, which is very unlikely.

To be pedantic, old calculation could only be useful if nMaxCoinsDBCache was higher not lower than 8 MiB. As long as it is 8MiB or lower this commit does not change behavior.

In #6102 the coins_db cache size was set to min(50% total cache size, 8MiB + 25% total cache size). But then in #8273 it was capped to nMaxCoinsDBCache, so as long as nMaxCoinsDBCache <= 8 MiB, this is just equivalent to min(50% total cache size, nMaxCoinsDBCache)

nit: Me and the compilers would prefer a contrived name over std::tuple<...>. It still allows for the structured bindings in init.cpp.

struct IndexAndKernelCacheSizes {
    IndexCacheSizes index;
    kernel::CacheSizes kernel;
};
IndexAndKernelCacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes = 0);

<details> <summary>Full diff</summary>

diff --git a/src/node/caches.cpp b/src/node/caches.cpp
index bd0c93d76f..c7197b6ef1 100644
--- a/src/node/caches.cpp
+++ b/src/node/caches.cpp
@@ -18,7 +18,7 @@ static constexpr int64_t MAX_TX_INDEX_CACHE{1024};
 static constexpr int64_t MAX_FILTER_INDEX_CACHE{1024};
 
 namespace node {
-std::tuple<IndexCacheSizes, kernel::CacheSizes> CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
+IndexAndKernelCacheSizes 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);
diff --git a/src/node/caches.h b/src/node/caches.h
index 02b91cee87..afa0946d8b 100644
--- a/src/node/caches.h
+++ b/src/node/caches.h
@@ -9,7 +9,6 @@
 
 #include <cstddef>
 #include <cstdint>
-#include <tuple>
 
 class ArgsManager;
 
@@ -23,7 +22,11 @@ struct IndexCacheSizes {
     size_t tx_index;
     size_t filter_index;
 };
-std::tuple<IndexCacheSizes, kernel::CacheSizes> CalculateCacheSizes(const ArgsManager& args, size_t n_indexes = 0);
+struct IndexAndKernelCacheSizes {
+    IndexCacheSizes index;
+    kernel::CacheSizes kernel;
+};
+IndexAndKernelCacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes = 0);
 } // namespace node
 
 #endif // BITCOIN_NODE_CACHES_H
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index ca9e2111a4..33ad258457 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{std::get<1>(node::CalculateCacheSizes(m_args))};
+    kernel::CacheSizes m_kernel_cache_sizes{node::CalculateCacheSizes(m_args).kernel};
     bool m_coins_db_in_memory{true};
     bool m_block_tree_db_in_memory{true};
     std::function<void()> m_make_chainman{};

</details>

The compilation speed difference is probably not measurable, more a case of death by a thousand eventual cuts.

nit: The message of commit 69159326810822c804ab53a009ef57fb5f28fac9 belonging to this PR states:

It always applied in the same way, no matter how the txindex is setup.

There is some nuance here, the comment was correct in 32cab91278651d07a11132b7636dc3d21144e616 (laanwj 2016 #8273), but was no longer valid after 8181db88f6e0ed96654951e18b1558cd8f78765b (jimpo 2017 #13033). Could add nuance to the commit message.

nit: Arguably clearer:

    IndexCacheSizes index_sizes;

No longer documents unit (MiB) due to commit 69159326810822c804ab53a009ef57fb5f28fac9.

(Sorry to be correcting my own suggestion, ran out of steam a bit by the time I landed on the approach of using this free function).

• The 2 first asserts could be improved through replacing them with this more complex one.
• size_t is always unsigned, it was probably unnecessary to cast the max value in the last assert.

    Assert(std::countl_zero(static_cast<uint64_t>(mib)) >= 21); // Ensure signed 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());

countl_zero requires:

+ #include <bit>

nit: Could deserve a brief comment?

//! Guards against truncation of values before converting.
constexpr size_t MiBToBytes(int64_t mib)

Good to have this for user supplied data instead of triggering asserts!

nit: Might be able to skip one cast:

    if (static_cast<uint64_t>(db_cache << 20) > std::numeric_limits<size_t>::max()) {

Would be good to check against negative -dbcache values as well.

Ugh, must have switched to the old branch by mistake when I was testing this. I think it would be better to clamp the negative values to zero, which preserves the current behaviour.

nit: Could use more modern LogInfo() instead.

nit: Simpler to say 4GiB?

I know 32-bit Windows has issues using more than 2GB unless one sets /LARGEADDRESSAWARE, not sure whether we do that. See https://learn.microsoft.com/en-us/windows/win32/memory/memory-limits-for-windows-releases

Edit: I guess we don't ship 32-bit Windows builds any longer, so the 2GB limit should not be a concern.

nit: If you re-touch, slightly more correct:

    Assert(std::countl_zero(static_cast<uint64_t>(mib)) >= 21); // Ensure sign bit is unset + enough zeros to shift.

Should probably fix this typo in commit it was introduced (a5742b7c09f1b4204abf4a1992018fb7be2fef27), instead of in final commit (224bfeb07f7561470451c95dc0df1db959bc187e).

nit: <limits> should be added in same commit as <bit> and MiBToBytes().

I think the docstring is misleading: we're casting to uint64_t, so the sign bit will always be unset. Relatedly, this function does not deal with negative input well, so I think we should first assert that mib is positive and update the docstring to remove the signedness reference.

Tried compiling a static_assert with mib replaced by -1. Got the expected result from the sign bit:

/home/hodlinator/bitcoin/src/bitcoind.cpp:164:63: error: static assertion failed
  164 |     static_assert(std::countl_zero(static_cast<uint64_t>(-1)) >= 21);
      |                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
/home/hodlinator/bitcoin/src/bitcoind.cpp:164:63: note: the comparison reduces to ‘(0 >= 21)’

Edit: The sign bit is referring to the mib variable, and that bit position does not become unset just from casting.

Did you hard-code the -1 literal? I don't think that holds for a variable mib at runtime?

Yes, I hard-coded the literal into the static_assert above. What part of integer arithmetic and casting changes when done in a runtime context?

Even if the assert is correct, maybe the fact that this thread exists is justification enough for replacing it with something closer to what it was originally. Like #31483 (review)

Sorry, maybe I'm misunderstanding you but I think for a run-time invocation of MiBToBytes() with mib==-1 this assertion Assert(std::countl_zero(static_cast<uint64_t>(mib)) >= 21); would not be hit, which is different to your hard-coded static_assert expression.

But I agree that @ryanofsky's right-shifting the max approach preferable (and also what I suggested in my inlined approach in #31483 (review))

I think a std::optional<size_t> return type is more appropriate for this function, the assertions make this function hard to test. Below diff updates the return type, adds a positiveness check (as per my other comment), and adds a test case:

<details> <summary>git diff on 578654c686</summary>

diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
index 46ca35ea90..623aec5aa0 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{*Assert(MiBToBytes(DEFAULT_KERNEL_CACHE))};
     node::ChainstateLoadOptions options;
     auto [status, error] = node::LoadChainstate(chainman, cache_sizes, options);
     if (status != node::ChainstateLoadStatus::SUCCESS) {
diff --git a/src/kernel/caches.h b/src/kernel/caches.h
index 2b5ba9eefc..298ebf8d43 100644
--- a/src/kernel/caches.h
+++ b/src/kernel/caches.h
@@ -11,6 +11,7 @@
 #include <bit>
 #include <cstdint>
 #include <limits>
+#include <optional>
 
 //! Suggested default amount of cache reserved for the kernel (MiB)
 static constexpr int64_t DEFAULT_KERNEL_CACHE{450};
@@ -20,11 +21,15 @@ static constexpr int64_t MAX_BLOCK_DB_CACHE{2};
 static constexpr int64_t MAX_COINS_DB_CACHE{8};
 
 //! Guard against truncation of values before converting.
-constexpr size_t MiBToBytes(int64_t mib)
+constexpr std::optional<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.
+    if (mib < 0 || std::countl_zero(static_cast<uint64_t>(mib)) < 21) {
+        return std::nullopt;
+    }
     const int64_t bytes{mib << 20};
-    Assert(static_cast<uint64_t>(bytes) <= std::numeric_limits<size_t>::max());
+    if (static_cast<uint64_t>(bytes) > std::numeric_limits<size_t>::max()) {
+        return std::nullopt;
+    }
     return static_cast<size_t>(bytes);
 }
 
@@ -36,9 +41,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, *Assert(MiBToBytes(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, *Assert(MiBToBytes(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..f59748a222 100644
--- a/src/node/caches.cpp
+++ b/src/node/caches.cpp
@@ -8,6 +8,7 @@
 #include <index/txindex.h>
 #include <kernel/caches.h>
 #include <logging.h>
+#include <util/check.h>
 
 #include <algorithm>
 #include <optional>
@@ -31,14 +32,14 @@ std::optional<CacheSizes> CalculateCacheSizes(const ArgsManager& args, size_t n_
 
     // 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(*Assert(MiBToBytes(db_cache)), *Assert(MiBToBytes(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) ? *Assert(MiBToBytes(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, *Assert(MiBToBytes(MAX_FILTER_INDEX_CACHE)));
         index_sizes.filter_index = max_cache / n_indexes;
         total_cache -= index_sizes.filter_index * n_indexes;
     }
diff --git a/src/test/CMakeLists.txt b/src/test/CMakeLists.txt
index ed73e44901..7cf42c2aa9 100644
--- a/src/test/CMakeLists.txt
+++ b/src/test/CMakeLists.txt
@@ -71,6 +71,7 @@ add_executable(test_bitcoin
   httpserver_tests.cpp
   i2p_tests.cpp
   interfaces_tests.cpp
+  kernel_caches_tests.cpp
   key_io_tests.cpp
   key_tests.cpp
   logging_tests.cpp

</details>

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) implemented the checks, so I'd suggest inlining the function there.

<details> <summary>git diff on 578654c686</summary>

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: [#8273 (comment)](/bitcoin-bitcoin/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 [#8273](/bitcoin-bitcoin/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.

allowing compile-time guarantees that we're not overflowing the system's size_t size.

I'm not sure that is actually true. Does this actually guard against any possible overflow?

I'm not sure what the guarantees are, but at least on my machine, with static constexpr size_t MAX_FILTER_INDEX_CACHE{18446744073709551616}; (i.e. max + 1 on 64 bit) compilation fails with:

../../src/node/caches.cpp:22:48: error: integer literal is too large to be represented in any integer type
static constexpr size_t MAX_FILTER_INDEX_CACHE{18446744073709551616};
                                               ^
1 error generated.

Similarly, for static constexpr uint8_t TEST_8{266}; it fails with:

../../src/node/caches.cpp:23:33: error: constant expression evaluates to 266 which cannot be narrowed to type 'uint8_t' (aka 'unsigned char') [-Wc++11-narrowing]
static constexpr uint8_t TEST_8{266};
                                ^~~
../../src/node/caches.cpp:23:33: note: insert an explicit cast to silence this issue
static constexpr uint8_t TEST_8{266};
                                ^~~
                                static_cast<uint8_t>( )
../../src/node/caches.cpp:23:33: warning: implicit conversion from 'int' to 'const uint8_t' (aka 'const unsigned char') changes value from 266 to 10 [-Wconstant-conversion]
static constexpr uint8_t TEST_8{266};
                               ~^~~
1 warning and 1 error generated.

I'm more concerned how the shift will be cast. E.g. a potential future bump to 3000 MiB would error:

error: constant expression evaluates to -1149239296 which cannot be narrowed to type 'size_t' (aka 'unsigned long') [-Wc++11-narrowing]
   16 | static constexpr size_t DEFAULT_KERNEL_CACHE{3000 << 20};

so to make this future proof, I think a function interpreting the values properly, or more ugly casts are required.

re: #31483 (review)

and adds a test case

I don't think I see a new test case added in the diff. Is it missing?

Another way to make this testable without adding interface complexity would just be to throw std::overflow_error

Hmm, you raise a good point. I see two alternatives:

1. The low-code solution: when values get problematic (which is pretty infrequent, and constant), write them as one of follows:

static constexpr size_t DEFAULT_KERNEL_CACHE{3145728000};
static_assert(DEFAULT_KERNEL_CACHE >> 20 == 3000);

or

static constexpr size_t DEFAULT_KERNEL_CACHE{size_t{3000} << 20};
static_assert(DEFAULT_KERNEL_CACHE >> 20 == 3000);

2. instead of adding a MiBToBytes function, add a template <typename T> constexpr T CheckedLeftShift(T value, unsigned shift) function to util/overflow.h which can more generally be re-used?

static constexpr uint32_t DEFAULT_KERNEL_CACHE{CheckedLeftShift<uint32_t>(3000, 20)};

...

template <typename T>
constexpr T CheckedLeftShift(T value, unsigned shift)
{
    static_assert(std::is_unsigned_v<T>, 
                  "CheckedLeftShift requires an unsigned type");

    if (shift >= std::numeric_limits<T>::digits) {
        throw std::overflow_error("shift out of range");
    }

    if (value > (std::numeric_limits<T>::max() >> shift)) {
        throw std::overflow_error("shift would overflow");
    }

    return static_cast<T>(value << shift);
}

I don't think I see a new test case added in the diff. Is it missing?

Snap, they weren't included in my git diff because they were added in a new file, and I didn't notice, sorry. I see @TheCharlatan already added a test case in his latest push.

Are there places where this would be re-used? I'd be happy to add something more generic if there is demand for it.

re: #31483 (review)

Are there places where this would be re-used? I'd be happy to add something more generic if there is demand for it.

FWIW, all of the approaches discussed in this thread seem fine to me. If I were writing this PR, I think I would drop the MiBToBytes function and just write the constants as byte counts:

static constexpr size_t MAX_BLOCK_DB_CACHE{2 << 20};
static constexpr size_t MAX_COINS_DB_CACHE{8 << 20};

Then when converting the -dbcache value from MiB to bytes I would just cap the value if it would overflow. (I might consider adding a SaturatingLeftShift function to overflow.h to do that, but that would probably be overkill).

@hodlinator since you initially proposed the current solution, do you have any fresh opinions here?

Are there places where this would be re-used?

I can't quite find one - it's also a bit difficult to search for given how much << is used for streams too. I agree there's no need to generalize too early, but given how similar the functionality is I still have a preference for the more generic CheckedLeftShift() and/or SaturatingLeftShift() as suggested by @ryanofsky. But, I'm okay with either approach.

I think I would drop the MiBToBytes function and just write the constants as byte counts:

I agree (+ with everything in your comment).

Okay I'm sorry I can't seem to just let it go but here's another approach. It adds SaturatingLeftShift and CheckedLeftShift() util/overflow.h functionality (+tests, pinky promise), a _MiB string literal to allow remaining sizes to be converted to size_t bytes with concise notation and compile-time overflow guarantees (🤞). It also incorporates @ryanofsky's suggestion to just saturate -dbcache values instead of throwing. It's not necessary for my suggestion but I think it's a good approach and aligns well with the SaturatingLeftShift() helper fn.

<details> <summary>git diff on 82706e217f</summary>

diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
index 82bda4b22b..51c482607a 100644
--- a/src/bitcoin-chainstate.cpp
+++ b/src/bitcoin-chainstate.cpp
@@ -26,7 +26,6 @@
 #include <random.h>
 #include <script/sigcache.h>
 #include <util/chaintype.h>
-#include <util/byte_conversion.h>
 #include <util/fs.h>
 #include <util/signalinterrupt.h>
 #include <util/task_runner.h>
@@ -124,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..847facad87 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);
@@ -1179,7 +1179,7 @@ static ChainstateLoadResult InitAndLoadChainstate(
     NodeContext& node,
     bool do_reindex,
     const bool do_reindex_chainstate,
-    CacheSizes& cache_sizes,
+    const CacheSizes& cache_sizes,
     const ArgsManager& args)
 {
     const CChainParams& chainparams = Params();
@@ -1605,11 +1605,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     ReadNotificationArgs(args, kernel_notifications);
 
     // cache size calculations
-    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;
+    const auto [index_cache_sizes, kernel_cache_sizes] = CalculateCacheSizes(args, g_enabled_filter_types.size());
 
     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 9ee5a31e0a..4414d1a123 100644
--- a/src/kernel/caches.h
+++ b/src/kernel/caches.h
@@ -5,16 +5,16 @@
 #ifndef BITCOIN_KERNEL_CACHES_H
 #define BITCOIN_KERNEL_CACHES_H
 
-#include <util/byte_conversion.h>
+#include <util/storage.h>
 
 #include <algorithm>
 
-//! 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{450_MiB};
 //! Max memory allocated to block tree DB specific cache (bytes)
-static constexpr size_t MAX_BLOCK_DB_CACHE{MiBToBytes(2)};
+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{MiBToBytes(8)};
+static constexpr size_t MAX_COINS_DB_CACHE{8_MiB};
 
 namespace kernel {
 struct CacheSizes {
diff --git a/src/node/caches.cpp b/src/node/caches.cpp
index 41a8971665..94ad5bbe61 100644
--- a/src/node/caches.cpp
+++ b/src/node/caches.cpp
@@ -8,34 +8,27 @@
 #include <index/txindex.h>
 #include <kernel/caches.h>
 #include <logging.h>
-#include <util/byte_conversion.h>
+#include <util/overflow.h>
+#include <util/storage.h>
 
 #include <algorithm>
-#include <optional>
 #include <stdexcept>
 #include <string>
 
 // Unlike for the UTXO database, for the txindex scenario the leveldb cache make
 // a meaningful difference: [#8273 (comment)](/bitcoin-bitcoin/8273/#issuecomment-229601991)
 //! Max memory allocated to tx index DB specific cache in bytes.
-static constexpr size_t MAX_TX_INDEX_CACHE{MiBToBytes(1024)};
+static constexpr size_t MAX_TX_INDEX_CACHE{1024_MiB};
 //! Max memory allocated to all block filter index caches combined in bytes.
-static constexpr size_t MAX_FILTER_INDEX_CACHE{MiBToBytes(1024)};
+static constexpr size_t MAX_FILTER_INDEX_CACHE{1024_MiB};
 
 namespace node {
-std::optional<CacheSizes> CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
+CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
 {
-    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), MiBToBytes(MIN_DB_CACHE));
-    } catch (const std::out_of_range&) {
-        LogError("Cannot allocate more than %d MiB in total for db caches.", std::numeric_limits<size_t>::max() >> 20);
-        return std::nullopt;
+    // Total cache size in MiB, floored by MIN_DB_CACHE and capped by max size_t value
+    size_t total_cache{DEFAULT_DB_CACHE};
+    if (auto db_cache = args.GetIntArg("-dbcache")) {
+        total_cache = std::max(MIN_DB_CACHE, SaturatingLeftShift<size_t>(*db_cache, 20));
     }
 
     IndexCacheSizes index_sizes;
@@ -47,6 +40,6 @@ std::optional<CacheSizes> CalculateCacheSizes(const ArgsManager& args, size_t n_
         index_sizes.filter_index = max_cache / n_indexes;
         total_cache -= index_sizes.filter_index * n_indexes;
     }
-    return {{index_sizes, kernel::CacheSizes{total_cache}}};
+    return {index_sizes, kernel::CacheSizes{total_cache}};
 }
 } // namespace node
diff --git a/src/node/caches.h b/src/node/caches.h
index b4f2e3d516..aab7871e93 100644
--- a/src/node/caches.h
+++ b/src/node/caches.h
@@ -6,17 +6,16 @@
 #define BITCOIN_NODE_CACHES_H
 
 #include <kernel/caches.h>
-#include <util/byte_conversion.h>
+#include <util/storage.h>
 
 #include <cstddef>
-#include <optional>
 
 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_MiB};
+//! -dbcache default (bytes)
+static constexpr size_t DEFAULT_DB_CACHE{DEFAULT_KERNEL_CACHE};
 
 namespace node {
 struct IndexCacheSizes {
@@ -27,7 +26,7 @@ struct CacheSizes {
     IndexCacheSizes index;
     kernel::CacheSizes kernel;
 };
-std::optional<CacheSizes> CalculateCacheSizes(const ArgsManager& args, size_t n_indexes = 0);
+CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes = 0);
 } // namespace node
 
 #endif // BITCOIN_NODE_CACHES_H
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 [#8273](/bitcoin-bitcoin/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);
     }
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index 7d7c7b7cd4..33ad258457 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).value().kernel};
+    kernel::CacheSizes m_kernel_cache_sizes{node::CalculateCacheSizes(m_args).kernel};
     bool m_coins_db_in_memory{true};
     bool m_block_tree_db_in_memory{true};
     std::function<void()> m_make_chainman{};
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index e8e0abbda6..dbe1d76ca8 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -13,12 +13,12 @@
 #include <test/util/setup_common.h>
 #include <uint256.h>
 #include <util/bitdeque.h>
-#include <util/byte_conversion.h>
 #include <util/fs.h>
 #include <util/fs_helpers.h>
 #include <util/moneystr.h>
 #include <util/overflow.h>
 #include <util/readwritefile.h>
+#include <util/storage.h>
 #include <util/strencodings.h>
 #include <util/string.h>
 #include <util/time.h>
@@ -141,19 +141,6 @@ BOOST_AUTO_TEST_CASE(util_criticalsection)
     } while(0);
 }
 
-BOOST_AUTO_TEST_CASE(byte_conversion)
-{
-    // maximum allowed value in MiB
-    const int64_t max_cache = std::numeric_limits<size_t>::max() >> 20;
-
-    BOOST_CHECK_EXCEPTION(MiBToBytes(-1), std::out_of_range, HasReason("Value may not be negative."));
-    BOOST_CHECK_EXCEPTION(MiBToBytes(std::numeric_limits<int64_t>::max()), std::out_of_range, HasReason("Conversion to bytes of"));
-    BOOST_CHECK_EXCEPTION(MiBToBytes(max_cache + 1), std::out_of_range, HasReason("Conversion to bytes of"));
-
-    BOOST_CHECK_EQUAL(MiBToBytes(0), 0);
-    BOOST_CHECK_EQUAL(MiBToBytes(max_cache), max_cache << 20);
-}
-
 constexpr char HEX_PARSE_INPUT[] = "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f";
 constexpr uint8_t HEX_PARSE_OUTPUT[] = {
     0x04, 0x67, 0x8a, 0xfd, 0xb0, 0xfe, 0x55, 0x48, 0x27, 0x19, 0x67, 0xf1, 0xa6, 0x71, 0x30, 0xb7,
@@ -1892,4 +1879,63 @@ BOOST_AUTO_TEST_CASE(clearshrink_test)
     }
 }
 
+template <typename T>
+void TestCheckedLeftShift()
+{
+    BOOST_TEST_CONTEXT("Testing CheckedLeftShift with " << typeid(T).name()) {
+        // Basic operations
+        BOOST_CHECK_EQUAL(CheckedLeftShift<T>(0, 1), 0);
+        BOOST_CHECK_EQUAL(CheckedLeftShift<T>(1, 1), 2);
+        BOOST_CHECK_EQUAL(CheckedLeftShift<T>(2, 2), 8);
+
+        // Negative input
+        BOOST_CHECK(!CheckedLeftShift<T>(-1, 1));
+
+        // Overflow cases
+        BOOST_CHECK(!CheckedLeftShift<T>((std::numeric_limits<T>::max() >> 1) + 1, 1));
+        BOOST_CHECK(!CheckedLeftShift<T>(std::numeric_limits<T>::max(), 1));
+    }
+}
+
+template <typename T>
+void TestSaturatingLeftShift()
+{
+    BOOST_TEST_CONTEXT("Testing SaturatingLeftShift with " << typeid(T).name()) {
+        // Basic operations
+        BOOST_CHECK_EQUAL(SaturatingLeftShift<T>(0, 1), 0);
+        BOOST_CHECK_EQUAL(SaturatingLeftShift<T>(1, 1), 2);
+        BOOST_CHECK_EQUAL(SaturatingLeftShift<T>(2, 2), 8);
+
+        // Negative input
+        BOOST_CHECK_EQUAL(SaturatingLeftShift<T>(-1, 1), 0);
+
+        // Saturation cases
+        BOOST_CHECK_EQUAL(SaturatingLeftShift<T>((std::numeric_limits<T>::max() >> 1) + 1, 1), std::numeric_limits<T>::max());
+        BOOST_CHECK_EQUAL(SaturatingLeftShift<T>(std::numeric_limits<T>::max(), 1), std::numeric_limits<T>::max());
+    }
+}
+
+BOOST_AUTO_TEST_CASE(checked_left_shift_test)
+{
+    TestCheckedLeftShift<uint8_t>();
+    TestCheckedLeftShift<size_t>();
+    TestCheckedLeftShift<uint64_t>();
+}
+
+BOOST_AUTO_TEST_CASE(saturating_left_shift_test)
+{
+    TestSaturatingLeftShift<uint8_t>();
+    TestSaturatingLeftShift<size_t>();
+    TestSaturatingLeftShift<uint64_t>();
+}
+
+BOOST_AUTO_TEST_CASE(mib_string_literal_test)
+{
+    BOOST_CHECK_EQUAL(0_MiB, 0);
+    BOOST_CHECK_EQUAL(1_MiB, 1024 * 1024);
+
+    constexpr size_t max_safe_mib = std::numeric_limits<size_t>::max() / (1024 * 1024);
+    BOOST_CHECK_EQUAL(max_safe_mib * (1024 * 1024), max_safe_mib * 1_MiB);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/util/byte_conversion.h b/src/util/byte_conversion.h
deleted file mode 100644
index bbdad04bff..0000000000
--- a/src/util/byte_conversion.h
+++ /dev/null
@@ -1,26 +0,0 @@
-// Copyright (c) 2025-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_UTIL_BYTE_CONVERSION_H
-#define BITCOIN_UTIL_BYTE_CONVERSION_H
-
-#include <tinyformat.h>
-
-#include <cstdint>
-#include <limits>
-#include <stdexcept>
-
-//! Guard against truncation of values before converting.
-constexpr size_t MiBToBytes(int64_t mib)
-{
-    if (mib < 0) {
-        throw std::out_of_range("Value may not be negative.");
-    }
-    if (static_cast<uint64_t>(mib) > std::numeric_limits<size_t>::max() >> 20) {
-        throw std::out_of_range(strprintf("Conversion to bytes of %d does not fit into size_t with maximum value %d.", mib, std::numeric_limits<size_t>::max()));
-    }
-    return static_cast<size_t>(mib) << 20;
-}
-
-#endif // BITCOIN_UTIL_BYTE_CONVERSION_H
diff --git a/src/util/overflow.h b/src/util/overflow.h
index 7e0cce6c27..bb5277db28 100644
--- a/src/util/overflow.h
+++ b/src/util/overflow.h
@@ -47,4 +47,42 @@ template <class T>
     return i + j;
 }
 
+/**
+ * [@brief](/bitcoin-bitcoin/contributor/brief/) Left bit shift with overflow checking.
+ * [@param](/bitcoin-bitcoin/contributor/param/) i The input value to be left shifted.
+ * [@param](/bitcoin-bitcoin/contributor/param/) shift The number of bits to left shift.
+ * [@return](/bitcoin-bitcoin/contributor/return/) The result of the left shift, or std::nullopt in case of
+ *         overflow or negative input value.
+ */
+template <class Output, class Input>
+constexpr std::optional<Output> CheckedLeftShift(Input i, unsigned shift) noexcept
+    requires std::is_unsigned_v<Output>
+{
+    if constexpr (std::is_signed_v<Input>) {
+        if (i < 0) return std::nullopt;
+    }
+    if (std::make_unsigned_t<Input>(i) > (std::numeric_limits<Output>::max() >> shift)) {
+        return std::nullopt;
+    }
+    return i << shift;
+}
+
+/**
+ * [@brief](/bitcoin-bitcoin/contributor/brief/) Left bit shift with safe minimum and maximum values.
+ * [@param](/bitcoin-bitcoin/contributor/param/) i The input value to be left shifted.
+ * [@param](/bitcoin-bitcoin/contributor/param/) shift The number of bits to left shift.
+ * [@return](/bitcoin-bitcoin/contributor/return/) The result of the left shift, with the return value clamped
+ *         between zero and the maximum Output value if overflow occurs.
+ */
+template <class Output, class Input>
+constexpr Output SaturatingLeftShift(Input i, unsigned shift) noexcept
+    requires std::is_unsigned_v<Output>
+{
+    auto default_value{std::numeric_limits<Output>::max()};
+    if constexpr (std::is_signed_v<Input>) {
+        if (i < 0) default_value = 0;
+    }
+    return CheckedLeftShift<Output>(i, shift).value_or(default_value);
+}
+
 #endif // BITCOIN_UTIL_OVERFLOW_H
diff --git a/src/util/storage.h b/src/util/storage.h
new file mode 100644
index 0000000000..042875510b
--- /dev/null
+++ b/src/util/storage.h
@@ -0,0 +1,24 @@
+// Copyright (c) 2025-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_UTIL_STORAGE_H
+#define BITCOIN_UTIL_STORAGE_H
+
+#include <util/overflow.h>
+
+#include <cstdint>
+#include <limits>
+#include <stdexcept>
+
+//! Overflow-safe conversion of MiB to bytes.
+constexpr size_t operator"" _MiB(unsigned long long mebibytes)
+{
+    auto bytes{CheckedLeftShift<size_t>(static_cast<size_t>(mebibytes), 20)};
+    if (!bytes) {
+        throw std::overflow_error("mebibytes cannot be converted to bytes");
+    }
+    return *bytes;
+}
+
+#endif // BITCOIN_UTIL_STORAGE_H

</details>

@hodlinator since you initially proposed the current solution, do you have any fresh opinions here?

When used with size_t constants, I get helpful GCC compiler errors when increasing the value of the constants (I guess this in part comes from C++ integer literals being signed 32-bit by default?):

/home/hodlinator/bitcoin/src/node/caches.cpp:23:53: error: narrowing conversion of ‘-176160768’ from ‘int’ to ‘size_t’ {aka ‘long unsigned int’} [-Wnarrowing]
   23 | static constexpr size_t MAX_FILTER_INDEX_CACHE{8024 << 20};
      |                                                ~~~~~^~~~~

I don't get such errors when shifting an int64_t constant and then truncating it into a size_t function argument. 10x-ed DEFAULT_KERNEL_CACHE:

static constexpr int64_t DEFAULT_KERNEL_CACHE{4500};

Added this into caches.cpp/CalculateCacheSizes(), sending a too large value into size_t function argument:

kernel::CacheSizes cache_sizes{DEFAULT_KERNEL_CACHE << 20};

Got no error/warning when compiling on CentOS (32-bit).

The second block above makes me lean towards recommending keeping some kind of free function with checks for non-size_t constants. @stickies-v's latest diff seems overall like a good direction, building on the pre-existing overflow.h.

This check seems pretty broken to me. Left shifting a negative value is UB, and if db_cache is indeed too large it would already overflow in static_cast<uint64_t>(db_cache << 20).

On my machine, it seems this whole block is compiled away entirely, I'm not getting any warnings for either negative or too large values.

This is only relevant on machines where size_t is not a uint64_t, so I'm not surprised this is optimized away. I tested this by compiling for arm-linux-gnueabihf and running with:

qemu-arm -L /usr/arm-linux-gnueabihf build_arm_32/src/bitcoind -nowallet -dbcache=10000 -signet

+1 for clamping the value range to zero before doing that check though.

This is only relevant on machines where size_t is not a uint64_t, so I'm not surprised this is optimized away.

It seems to me like this check is necessary on all platforms? E.g. on this branch, when I run with -dbcache=10000000000000000 on arm64 I get

../../src/kernel/caches.h:25 MiBToBytes: Assertion `std::countl_zero(static_cast<uint64_t>(mib)) >= 21' failed.

In static_cast<uint64_t>(db_cache << 20) we're left-shifting db_cache without checking that it'll fit the uint64_t. I think my approach suggested in #31483 (review) addresses all of this quite nicely?

re: #31483 (review)

Agree with stickies overflow should be fixed. I think an easy way would just be to check if db_cache > std::numeric_limits<size_t>::max() >> 20 though.

In commit "init: Simplify coinsdb cache calculation" (19316eccaf93776c49b1870f8aa9a5fe57ec5f33)

Didn't notice this first time I reviewed this, but it would be good if commit subject included "refactor" and make it obvious this is not changing behavior. This simplification is only changing the way the calculation is performed, not changing the result of the calculation.

In commit "kernel: Move kernel-specific cache size options to kernel" (41bc7164880ffd0782ff5882211c4fcadd656022)

This seems like it is overthinking things. I think this should just be:

Assert(mib >= 0);
Assert(mib <= std::numeric_limits<size_t>::max() >> 20);
return mib << 20;

In commit "init: Use size_t consistently for cache sizes" (82706e217f34d1f09cbd30dde6b8ae5ac0385f0a)

Code could be simplified to make it clear what is causing the new error:

<details><summary>diff</summary> <p>

--- a/src/node/caches.cpp
+++ b/src/node/caches.cpp
@@ -25,14 +25,10 @@ static constexpr size_t MAX_FILTER_INDEX_CACHE{MiBToBytes(1024)};
 namespace node {
 std::optional<CacheSizes> CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
 {
-    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);
-
+    const int64_t db_cache = std::max(MIN_DB_CACHE, args.GetIntArg("-dbcache", DEFAULT_DB_CACHE));
     size_t total_cache = 0;
     try {
-        total_cache = std::max(MiBToBytes(db_cache), MiBToBytes(MIN_DB_CACHE));
+        total_cache = MiBToBytes(db_cache);
     } catch (const std::out_of_range&) {
         LogError("Cannot allocate more than %d MiB in total for db caches.", std::numeric_limits<size_t>::max() >> 20);
         return std::nullopt;

</p> </details>

Though I think we don't need this error, could do something even simpler like:

// Total cache size in MiB, floored by MIN_DB_CACHE
const int64_t db_cache = std::max(MIN_DB_CACHE, args.GetIntArg("-dbcache", DEFAULT_DB_CACHE));
// Total cache size in bytes, capped by max size_t value
size_t total_cache = std::min<int64_t>(db_cache, std::numeric_limits<size_t>::max() >> 20) << 20;

Current code or whatever you prefer seems fine though.

Thread #31483 (review): Agree it's a bit verbose right now. I especially like the first suggestion in the diff.

nit: this could be const if you update InitAndLoadChainstate()'s cache_sizes parameter to const

nit: Give a hint at why?

        throw std::overflow_error("Too many mebibytes to fit into size_t bytes");

nit: Typo

    // Convert -dbcache from MiB units to bytes. The total cache is floored by MIN_DB_CACHE and capped by max size_t value.

nit: Could have less churn on the line by making InitAndLoadChainstate() take const in the same commit where this line is first touched.

nit: Could modernize cast.

    if (std::make_unsigned_t<Input>(i) > Output{std::numeric_limits<Output>::max() >> shift}) {

Using non-narrowing conversion should give you a compiler error. Allowing type narrowing here should be safe.

In thread #31483 (review): What the.. could have sworn my suggestion compiled without warnings, but now I get them. Sorry.

in e0a541702def3a4c6cce8e44b6ebe8d608edad4e:

Right-shifting by more bits than the width of Output is UB, so we should probably add an extra check for that. I've also added some test cases, but interestingly they don't seem to trigger UBSan (without the util/overflow.h patch) on my machine:

<details> <summary>git diff on e0a541702d</summary>

diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 09ba278d3a..ae292d7761 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -1897,6 +1897,7 @@ void TestCheckedLeftShift()
         // Overflow cases
         BOOST_CHECK(!CheckedLeftShift<T>((MAX >> 1) + 1, 1));
         BOOST_CHECK(!CheckedLeftShift<T>(MAX, 1));
+        BOOST_CHECK(!CheckedLeftShift<T>(1, std::numeric_limits<T>::digits + 1));
     }
 }
 
@@ -1919,6 +1920,8 @@ void TestSaturatingLeftShift()
         // Saturation cases
         BOOST_CHECK_EQUAL(SaturatingLeftShift<T>((MAX >> 1) + 1, 1), MAX);
         BOOST_CHECK_EQUAL(SaturatingLeftShift<T>(MAX, 1), MAX);
+        BOOST_CHECK_EQUAL(SaturatingLeftShift<T>(-1, std::numeric_limits<T>::digits + 1), 0);
+        BOOST_CHECK_EQUAL(SaturatingLeftShift<T>(1, std::numeric_limits<T>::digits + 1), MAX);
     }
 }
 
diff --git a/src/util/overflow.h b/src/util/overflow.h
index a0698f5c69..d85f5b9c9d 100644
--- a/src/util/overflow.h
+++ b/src/util/overflow.h
@@ -61,7 +61,8 @@ constexpr std::optional<Output> CheckedLeftShift(Input i, unsigned shift) noexce
     if constexpr (std::is_signed_v<Input>) {
         if (i < 0) return std::nullopt;
     }
-    if (std::make_unsigned_t<Input>(i) > Output(std::numeric_limits<Output>::max() >> shift)) {
+    if (shift >= std::numeric_limits<Input>::digits ||
+        std::make_unsigned_t<Input>(i) > Output(std::numeric_limits<Output>::max() >> shift)) {
         return std::nullopt;
     }
     return i << shift;

</details>

nit in commit 4eebb638916ecf43cfd9237c7e8bb8da7bc397d7: Seems like we could use std::min without the template parameter here from the start (it's removed later without parameter types changing, resulting in line churn).

in e0a541702def3a4c6cce8e44b6ebe8d608edad4e: nit: would prefer replacing i with input (I'm sorry for not doing this in my initial suggestion)

<details> <summary>git diff on e0a541702d</summary>

diff --git a/src/util/overflow.h b/src/util/overflow.h
index a0698f5c69..c35e44690a 100644
--- a/src/util/overflow.h
+++ b/src/util/overflow.h
@@ -50,38 +50,38 @@ template <class T>
 
 /**
  * [@brief](/bitcoin-bitcoin/contributor/brief/) Left bit shift with overflow checking.
- * [@param](/bitcoin-bitcoin/contributor/param/) i The input value to be left shifted.
+ * [@param](/bitcoin-bitcoin/contributor/param/) input The input value to be left shifted.
  * [@param](/bitcoin-bitcoin/contributor/param/) shift The number of bits to left shift.
  * [@return](/bitcoin-bitcoin/contributor/return/) The result of the left shift, or std::nullopt in case of
  *         overflow or negative input value.
  */
 template <std::unsigned_integral Output, std::integral Input>
-constexpr std::optional<Output> CheckedLeftShift(Input i, unsigned shift) noexcept
+constexpr std::optional<Output> CheckedLeftShift(Input input, unsigned shift) noexcept
 {
     if constexpr (std::is_signed_v<Input>) {
-        if (i < 0) return std::nullopt;
+        if (input < 0) return std::nullopt;
     }
-    if (std::make_unsigned_t<Input>(i) > Output(std::numeric_limits<Output>::max() >> shift)) {
+    if (std::make_unsigned_t<Input>(input) > Output(std::numeric_limits<Output>::max() >> shift)) {
         return std::nullopt;
     }
-    return i << shift;
+    return input << shift;
 }
 
 /**
  * [@brief](/bitcoin-bitcoin/contributor/brief/) Left bit shift with safe minimum and maximum values.
- * [@param](/bitcoin-bitcoin/contributor/param/) i The input value to be left shifted.
+ * [@param](/bitcoin-bitcoin/contributor/param/) input The input value to be left shifted.
  * [@param](/bitcoin-bitcoin/contributor/param/) shift The number of bits to left shift.
  * [@return](/bitcoin-bitcoin/contributor/return/) The result of the left shift, with the return value clamped
  *         between zero and the maximum Output value if overflow occurs.
  */
 template <std::unsigned_integral Output, std::integral Input>
-constexpr Output SaturatingLeftShift(Input i, unsigned shift) noexcept
+constexpr Output SaturatingLeftShift(Input input, unsigned shift) noexcept
 {
     auto default_value{std::numeric_limits<Output>::max()};
     if constexpr (std::is_signed_v<Input>) {
-        if (i < 0) default_value = 0;
+        if (input < 0) default_value = 0;
     }
-    return CheckedLeftShift<Output>(i, shift).value_or(default_value);
+    return CheckedLeftShift<Output>(input, shift).value_or(default_value);
 }
 
 #endif // BITCOIN_UTIL_OVERFLOW_H

</details>

in e0a541702def3a4c6cce8e44b6ebe8d608edad4e:

Saturated left shifting a 0-input should probably always return a 0-output, so suggested update (+ tests, one depending on my other comment re right-shift UB):

<details> <summary>git diff on e0a541702d</summary>

diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 09ba278d3a..07b98be446 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -1897,6 +1897,7 @@ void TestCheckedLeftShift()
         // Overflow cases
         BOOST_CHECK(!CheckedLeftShift<T>((MAX >> 1) + 1, 1));
         BOOST_CHECK(!CheckedLeftShift<T>(MAX, 1));
+        // BOOST_CHECK(!CheckedLeftShift<T>(0, std::numeric_limits<T>::digits + 1));  // TODO: requires right-shift overflow check to pass
     }
 }
 
@@ -1919,6 +1920,7 @@ void TestSaturatingLeftShift()
         // Saturation cases
         BOOST_CHECK_EQUAL(SaturatingLeftShift<T>((MAX >> 1) + 1, 1), MAX);
         BOOST_CHECK_EQUAL(SaturatingLeftShift<T>(MAX, 1), MAX);
+        BOOST_CHECK_EQUAL(SaturatingLeftShift<T>(0, std::numeric_limits<T>::digits + 1), 0);
     }
 }
 
diff --git a/src/util/overflow.h b/src/util/overflow.h
index a0698f5c69..f9e1fe4e82 100644
--- a/src/util/overflow.h
+++ b/src/util/overflow.h
@@ -77,10 +77,7 @@ constexpr std::optional<Output> CheckedLeftShift(Input i, unsigned shift) noexce
 template <std::unsigned_integral Output, std::integral Input>
 constexpr Output SaturatingLeftShift(Input i, unsigned shift) noexcept
 {
-    auto default_value{std::numeric_limits<Output>::max()};
-    if constexpr (std::is_signed_v<Input>) {
-        if (i < 0) default_value = 0;
-    }
+    auto default_value{i <= 0 ? 0 : std::numeric_limits<Output>::max()};
     return CheckedLeftShift<Output>(i, shift).value_or(default_value);
 }
 

</details>

in ccf8caa4430f57b8f148e96ac2a241221977bcab

note/nit: this can overflow on 32 bit machines. It is fixed in later commits, and I think fixing it in this commit would be overly verbose, but I'm mentioning it in case there are further code changes that could make this problematic again.

in ccf8caa4430f57b8f148e96ac2a241221977bcab

nit: I've raised this before, but since we now have a kernel::CacheSizes and node::CacheSizes, I think using a kernel::CacheSizes in a node file is unnecessarily confusing, so I would prefer making this explicit (as in the chainstate.h file):

<details> <summary>git diff on ccf8caa443</summary>

diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index 660d2c3289..d4d8828f73 100644
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -29,14 +29,12 @@
 #include <memory>
 #include <vector>
 
-using kernel::CacheSizes;
-
 namespace node {
 // Complete initialization of chainstates after the initial call has been made
 // to ChainstateManager::InitializeChainstate().
 static ChainstateLoadResult CompleteChainstateInitialization(
     ChainstateManager& chainman,
-    const CacheSizes& cache_sizes,
+    const kernel::CacheSizes& cache_sizes,
     const ChainstateLoadOptions& options) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
 {
     auto& pblocktree{chainman.m_blockman.m_block_tree_db};
@@ -172,7 +170,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
     return {ChainstateLoadStatus::SUCCESS, {}};
 }
 
-ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSizes& cache_sizes,
+ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const kernel::CacheSizes& cache_sizes,
                                     const ChainstateLoadOptions& options)
 {
     if (!chainman.AssumedValidBlock().IsNull()) {

</details>

in 0e2887262e1a3a4a5ab0382f9e8713b135408a77

nit: optional header no longer necessary

in 0e2887262e

nit: I would default-initialize IndexCacheSizes members to 0, avoiding potential issues of accessing uninitialized members (and removing the need for this line):

<details> <summary>git diff on 0e2887262e</summary>

diff --git a/src/node/caches.cpp b/src/node/caches.cpp
index 0e0305cd19..f4fbb7ead9 100644
--- a/src/node/caches.cpp
+++ b/src/node/caches.cpp
@@ -33,7 +33,6 @@ CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
     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(total_cache / 8, MAX_FILTER_INDEX_CACHE);
         index_sizes.filter_index = max_cache / n_indexes;
diff --git a/src/node/caches.h b/src/node/caches.h
index 0f916c7e5b..f24e9cc910 100644
--- a/src/node/caches.h
+++ b/src/node/caches.h
@@ -19,8 +19,8 @@ static constexpr size_t DEFAULT_DB_CACHE{DEFAULT_KERNEL_CACHE};
 
 namespace node {
 struct IndexCacheSizes {
-    size_t tx_index;
-    size_t filter_index;
+    size_t tx_index{0};
+    size_t filter_index{0};
 };
 struct CacheSizes {
     IndexCacheSizes index;

</details>

in 0e2887262e1a3a4a5ab0382f9e8713b135408a77

nit: this should now also be a size_t

        size_t max_cache{std::min(total_cache / 8, MAX_FILTER_INDEX_CACHE)};

In commit "util: Add integer left shift helpers" (e0a541702def3a4c6cce8e44b6ebe8d608edad4e)

It seems wrong that calling SaturatingLeftShift on a negative input can wrap around to 0. The point of saturating operations should be that they don't wrap around, but effectively perform operations with full precision and just clamp the result values. In c++ left shift i << j is defined as <em>i * 2^j</em>, so CheckedLeftShift and SaturatingLeftShift should be able to compute the same thing, but without undefined behavior or wrapping.

It also seems like a footgun that these left shift functions are using different input and output types instead of just using the one type like other functions in this file. Would suggest making new functions more similar to existing functions and writing like:

template <std::integral T>
constexpr std::optional<T> CheckedLeftShift(T i, unsigned shift) noexcept
{
    if (shift == 0 || i == 0) return i;
    // Avoid undefined c++ behavior if shift is >= number of bits in T.
    if (shift >= sizeof(T) * CHAR_BIT) return std::nullopt;
    // If i << shift is too big to fit in T, return nullopt.
    const auto max{std::numeric_limits<T>::max() >> shift};
    if (i > 0 && i > max) return std::nullopt;
    if (i < 0 && i < -max - 1) return std::nullopt;
    return i << shift;
}

template <std::integral T>
constexpr T SaturatingLeftShift(T i, unsigned shift) noexcept
{
    if (auto result{CheckedLeftShift(i, shift)}) return *result;
    // If i << shift is to big to fit in T, return biggest positive or negative
    // number that fits.
    return i < 0 ? std::numeric_limits<T>::min() : std::numeric_limits<T>::max();
}

I didn't test these extensively but did sanity check with:

    static_assert(SaturatingLeftShift<signed char>(0, 100) ==  0);
    static_assert(SaturatingLeftShift<signed char>(1, 100) ==  127);
    static_assert(SaturatingLeftShift<signed char>(-1, 100) ==  -128);

    static_assert(SaturatingLeftShift<signed char>(3, 2) ==  12);
    static_assert(SaturatingLeftShift<signed char>(-3, 2) ==  -12);

    static_assert(SaturatingLeftShift<signed char>(6, 4) ==  96);
    static_assert(SaturatingLeftShift<signed char>(-6, 4) ==  -96);

    static_assert(SaturatingLeftShift<signed char>(63, 1) ==  126);
    static_assert(SaturatingLeftShift<signed char>(64, 1) ==  127);
    static_assert(SaturatingLeftShift<signed char>(-63, 1) ==  -126);
    static_assert(SaturatingLeftShift<signed char>(-64, 1) ==  -128);
    static_assert(SaturatingLeftShift<signed char>(-65, 1) ==  -128);


    static_assert(SaturatingLeftShift<signed char>(31, 2) ==  124);
    static_assert(SaturatingLeftShift<signed char>(32, 2) ==  127);
    static_assert(SaturatingLeftShift<signed char>(-31, 2) ==  -124);
    static_assert(SaturatingLeftShift<signed char>(-32, 2) ==  -128);
    static_assert(SaturatingLeftShift<signed char>(-33, 2) ==  -128);

It seems wrong that calling SaturatingLeftShift on a negative input can wrap around to 0.

My understanding was that left shift a << b with negative a was UB in C++, which is why I thought saturating negative inputs to 0 was the most sensible noexcept approach. Thanks for pointing out (as per your cppreference link) that since C++20 this is indeed defined, which definitely means our implementation can (and should) too, nice!

It also seems like a footgun that these left shift functions are using different input and output types instead of just using the one type like other functions in this file.

That was my initial approach, but I eventually decided on separate types as to be able to safely catch negative input values instead of silently wrapping around (e.g. when calling SaturatingLeftShift<size_t>(*db_cache, 20) with a negative db_cache). For my understanding, if it's not too much off-topic, I'd be curious to hear where the footgun potential is - since the point of my approach was to exactly avoid that?

I agree that since C++20, left shift helper functions with negative input and result values, a single type T is preferable.

Your suggested implementation LGTM but I think keeping std::numeric_limits<T>::digits instead of sizeof(T) * CHAR_BIT is preferable since it takes into account the sign bit?

edit: I still think, to prevent accidental overflow/wrap-around, that having separate Input and Output types is safer, even when allowing for negative inputs and outputs.

Your suggested implementation LGTM but I think keeping std::numeric_limits<T>::digits instead of sizeof(T) * CHAR_BIT is preferable since it takes into account the sign bit?

As far as I understand it is legal to shift e.g. int32_t{1} << 31 to get the minimum int32_t value. But I think in this case we should prohibit this quirk of the underlying bit representation and use std::numeric_limits<T>::digits.

re: #31483 (review)

I didn't know about digits() and agree that it would be better to use than sizeof() so thanks for suggesting that.

For my understanding, if it's not too much off-topic, I'd be curious to hear where the footgun potential is - since the point of my approach was to exactly avoid that?

My concern with using different types for input and output is that code like auto kbps = SaturatingLeftShift(mbps, 10) would be misleading because you could reasonably assume that multiplying a number by 1024 should not change its type from signed to unsigned (and set it to 0 if the input is negative), but that is what the current implementation in e0a541702def3a4c6cce8e44b6ebe8d608edad4e) would do.

but that is what the current implementation in e0a5417) would do.

Are you sure it would? I don't see how the Output return type could be deduced with auto. That's kind of the point of the separate Input and Output types: it force the user to specify the return type, because especially with left shift the chances of the return type being much bigger than the input are high. Using (and potentially wrapping around) the input type, for that reason, seems footgunny for a util/overflow.h function?

When testing with:

    int32_t mbps{20};
    auto kbps = SaturatingLeftShift(mbps, 10);

I get, as expected:

../../src/node/caches.cpp:30:17: error: no matching function for call to 'SaturatingLeftShift'
    auto kbps = SaturatingLeftShift(mbps, 10);
                ^~~~~~~~~~~~~~~~~~~
../../src/util/overflow.h:78:18: note: candidate template ignored: couldn't infer template argument 'Output'
constexpr Output SaturatingLeftShift(Input i, unsigned shift) noexcept
                 ^
1 error generated.

would be misleading

With the user being forced to specify the return type, I don't think there's any room for ambiguity?

Are you sure it would? I don't see how the Output return type could be deduced with auto.

You're right. I didn't realize e0a541702def3a4c6cce8e44b6ebe8d608edad4e required specifying what type to cast the output to. In that case, I take back the comment that it has a footgun.

But I would say the implementation does not seem consistent with other functions in this file, or consistent with what I would expect a function that is doing a saturating multiplication operation to do. I think the fact that it casts output to a different type is not ideal, and the wrapping to 0 doesn't make much sense to me. I think SaturatingLeftShift(i, shift) should just be a saturating implementation of (i x 2^shift) like SaturatingAdd(i, j) is a saturating implementation of (i + j).

But I would say the implementation does not seem consistent with other functions in this file, or consistent with what I would expect a function that is doing a saturating multiplication operation to do.

I tend to agree with this, and rolled with your suggestion for now, but still open for other suggestions.

Alright, I'm on board too. I liked how the dual-type implementation made using the functions without casting and clamping easier (and as such perhaps a bit safer), but I agree that from a single-responsibility principle the current approach makes more sense, and it is consistent.

In commit "util: Add integer left shift helpers" (a85c07a891f76cd9e606aa83c0671506b1173f30)

Could consider adding a fuzz test too. Maybe:

namespace {
//! Test overflow operations for type T using a wider type, W, to verify results.
template <typename T, typename W>
void TestOverflow(FuzzedDataProvider& fuzzed_data_provider)
{
    constexpr auto min{std::numeric_limits<T>::min()};
    constexpr auto max{std::numeric_limits<T>::max()};
    static_assert(min >= std::numeric_limits<W>::min() / 2);
    static_assert(max <= std::numeric_limits<W>::max() / 2);

    auto widen = [](T value) -> W { return value; };
    auto clamp = [](W value) -> W { return std::clamp<W>(value, min, max); };
    auto check = [](W value) -> std::optional<W> { if (value >= min && value <= max) return value; else return std::nullopt; };

    const T i = fuzzed_data_provider.ConsumeIntegral<T>();
    const T j = fuzzed_data_provider.ConsumeIntegral<T>();
    const unsigned shift = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, std::numeric_limits<W>::digits - 1);

    Assert(clamp(widen(i) + widen(j)) == SaturatingAdd(i, j));
    Assert(check(widen(i) + widen(j)) == CheckedAdd(i, j));

    Assert(clamp(widen(i) << shift) == SaturatingLeftShift(i, j));
    Assert(check(widen(i) << shift) == CheckedLeftShift(i, j));
}
} // namespace

FUZZ_TARGET(overflow)
{
    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
    TestOverflow<int8_t, int16_t>(fuzzed_data_provider);
    TestOverflow<int16_t, int32_t>(fuzzed_data_provider);
    TestOverflow<int32_t, int64_t>(fuzzed_data_provider);
    TestOverflow<uint8_t, uint16_t>(fuzzed_data_provider);
    TestOverflow<uint16_t, uint32_t>(fuzzed_data_provider);
    TestOverflow<uint32_t, uint64_t>(fuzzed_data_provider);
}

(This compiles but I didn't actually test it)

Thanks, but I get a crash immediately:

test/fuzz/overflow.cpp:36 TestOverflow: Assertion `check(widen(i) << shift) == CheckedLeftShift(i, j)' failed.

Does this correction make sense to you?

diff --git a/src/test/fuzz/overflow.cpp b/src/test/fuzz/overflow.cpp
index defaf9926d..d29273eb6e 100644
--- a/src/test/fuzz/overflow.cpp
+++ b/src/test/fuzz/overflow.cpp
@@ -27,13 +27,13 @@ void TestOverflow(FuzzedDataProvider& fuzzed_data_provider)
 
     const T i = fuzzed_data_provider.ConsumeIntegral<T>();
     const T j = fuzzed_data_provider.ConsumeIntegral<T>();
-    const unsigned shift = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, std::numeric_limits<W>::digits - 1);
+    const unsigned shift = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, std::numeric_limits<T>::digits - 1);
 
     Assert(clamp(widen(i) + widen(j)) == SaturatingAdd(i, j));
     Assert(check(widen(i) + widen(j)) == CheckedAdd(i, j));
 
-    Assert(clamp(widen(i) << shift) == SaturatingLeftShift(i, j));
-    Assert(check(widen(i) << shift) == CheckedLeftShift(i, j));
+    Assert(clamp(widen(i) << shift) == SaturatingLeftShift(i, shift));
+    Assert(check(widen(i) << shift) == CheckedLeftShift(i, shift));
 }
 } // namespace

Edit: The test seems fine otherwise and I don't find new paths after just a few seconds.

Thanks, the bottom part of that correction (shifting by shift instead of j) makes sense but the top part changing std::numeric_limits<W>::digits to std::numeric_limits<T>::digits reduces the coverage of the fuzz test by reducing the range of shift so I think would not be a good change.

Ah, of course. The actual problem is that we are shifting even out of the widened type's range, so it wraps around. Not sure if there is a nice solution to that without re-implementing the logic we are trying to test. Do you have a better solution besides changing the shift value to get the digits of T?

Ah, of course. The actual problem is that we are shifting even out of the widened type's range, so it wraps around.

I see. So the max amount you can safely shift is without overflowing is not std::numeric_limits<W>::digits - 1 but actually std::numeric_limits<W>::digits - std::numeric_limits<T>::digits since that is how much free space there is to the right of T's bits in W.

So maybe if you switch to that amount and change the test cases to:

TestOverflow<int8_t, int64_t>(fuzzed_data_provider);
TestOverflow<int16_t, int64_t>(fuzzed_data_provider);
TestOverflow<int32_t, int64_t>(fuzzed_data_provider);
TestOverflow<uint8_t, uint64_t>(fuzzed_data_provider);
TestOverflow<uint16_t, uint64_t>(fuzzed_data_provider);
TestOverflow<uint32_t, uint64_t>(fuzzed_data_provider);

It could provide good coverage without overflowing

Yeah, that should work. I'll fuzz with this for some time.

In thread #31483 (review):

I see. So the max amount you can safely shift is without overflowing is not std::numeric_limits<W>::digits - 1 but actually std::numeric_limits<W>::digits - std::numeric_limits<T>::digits since that is how much free space there is to the right of T's bits in W.

Would appreciate if someone were to ELI5 this explanation. I'm thinking it must be clamp(widen(i) << shift) that doesn't tolerate it somehow (but why?), while SaturatingLeftShift(i, shift) works okay? Tried fuzzing with std::numeric_limits<W>::digits - (std::numeric_limits<T>::digits - 1) which fails, and it's not intuitive to me.

(This does not compile (for unsigned types) due to shifting as many bits as are in the type:

static_assert((W{1} << std::numeric_limits<W>::digits) == 0);

Tried to trigger some UB-complaint through the following, but did not succeed:

static_assert((W{1 << 8} << (std::numeric_limits<W>::digits - 1)) == 0);

)

In thread #31483 (comment):

I see. So the max amount you can safely shift is without overflowing is not std::numeric_limits<W>::digits - 1 but actually std::numeric_limits<W>::digits - std::numeric_limits<T>::digits since that is how much free space there is to the right of T's bits in W.

Would appreciate if someone were to ELI5 this explanation. I'm thinking it must be clamp(widen(i) << shift) that doesn't tolerate it somehow (but why?), while SaturatingLeftShift(i, shift) works okay?

That's right, the case I was thinking of is T=uint16_t, W=uint32_t, shift=31, i=0x8000, where 0x8000 << 31 is too big to fit in an uint32_t. The result will be well-defined but it will be 0 before it is seen by clamp(). The biggest shift that is possible without wrapping to 0 is 0x8000 << 16

Ah, so widen(i) << shift => 0 for the left side even before clamp(), and SaturatingLeftShift(i, shift) => std::numeric_limits<uint16_t>::max() for the right side. Thanks!

In commit "util: Add integer left shift helpers" (https://github.com/bitcoin/bitcoin/commit/a85c07a891f76cd9e606aa83c0671506b1173f30)

Could consider dropping this line. Code is correct without it and it seems like a distraction. Or, if it is important to check type of T for some reason, presumably that reason would also apply in the SaturatingLeftShift function below and this same check should be done there before checking for input < 0.

I had a compiler complain about signed to unsigned integer comparison without it.

In commit "util: Add integer left shift helpers" (https://github.com/bitcoin/bitcoin/commit/a85c07a891f76cd9e606aa83c0671506b1173f30)

"overflow or too high shift value" seem like the same thing so it is confusing to refer to them as different cases. Phrasing is also potentially ambiguous. Would maybe suggest @return (input * 2^shift) or nullopt if it would not fit in the return type.

In commit "util: Add integer left shift helpers" (https://github.com/bitcoin/bitcoin/commit/a85c07a891f76cd9e606aa83c0671506b1173f30)

Again would suggest something shorter and less ambiguous about meaning of left shift like @return (input * 2^shift) clamped to fit between the lowest and highest representable values of the type T

In commit "init: Use size_t consistently for cache sizes" (922aa7a25650bdfd8428198f171f88915af1ffa0)

Note: Passing max size_t value to min<uint64_t> could be broken in the theoretical case where size_t type is wider than uint64_t. If we had a saturate_cast function the code be simplified to avoid this type of problem:

    if (std::optional<int64_t> db_cache = args.GetIntArg("-dbcache")) {
        total_cache = std::max(MIN_DB_CACHE, saturate_cast<size_t>(SaturatingLeftShift(*db_cache, 20)));
    }

But I think it is fine to keep current code because for practical purposes size_t should not be wider than uint64_t.

in 7993f26173d911d3773cb2580938ac9aad8ad31b:

nit: I think the runtime signedness checks are unnecessary, and find std::numeric_limits<T>::min() a bit more readable than -max_allowed_input - 1, so putting it all together this can just be simplified to

    if (input > (std::numeric_limits<T>::max() >> shift)) return std::nullopt;
    if (input < (std::numeric_limits<T>::min() >> shift)) return std::nullopt;

re: #31483 (review)

nit: I think the runtime signedness checks are unnecessary, and find std::numeric_limits<T>::min() a bit more readable than -max_allowed_input - 1, so putting it all together this can just be simplified to

Agree with this suggestion. The only reason for writing code that way was that I read "For negative a, the value of a >> b is implementation-defined" in the c++ reference and didn't realize that sentence was in the "until c++20" section. After c++ 20, right shifting negative values is well defined so this should be ok.

Mmh, yeah, I think that should work.

in 7993f26173d911d3773cb2580938ac9aad8ad31b

You could make this test platform independent with:

<details> <summary>git diff on 7993f26173</summary>

diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 9991b1dda5..5bde29d0bb 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -1970,7 +1970,9 @@ BOOST_AUTO_TEST_CASE(mib_string_literal_test)
 {
     BOOST_CHECK_EQUAL(0_MiB, 0);
     BOOST_CHECK_EQUAL(1_MiB, 1024 * 1024);
-    BOOST_CHECK_EXCEPTION(17592186044416_MiB, std::overflow_error, HasReason("MiB value too large for size_t byte conversion"));
+    const auto max_mib{std::numeric_limits<size_t>::max() >> 20};
+    BOOST_CHECK_EXCEPTION(operator"" _MiB(static_cast<unsigned long long>(max_mib) + 1),
+                          std::overflow_error, HasReason("MiB value too large for size_t byte conversion"));
 }
 
 BOOST_AUTO_TEST_SUITE_END()

</details>

I think these should be size_t too?

Must have dropped this by mistake during one of the rebases.

In commit "util: Add integer left shift helpers" (7993f26173d911d3773cb2580938ac9aad8ad31b)

intput (unless you want to coin a phrase)

In commit "fuzz: Add fuzz test for checked and saturating add and left shift" (b7c8d88ec2d299d2b19f5c622ee5080cfdf8b2a0)

Could add comment to explain divide by two: "// Range needs to be at least twice as big to allow two numbers to be added without overflowing." Apparently I have goldfish memory, I wrote this code yesterday and already forgot why it was there.

mega-nit in case you re-touch:

    // Avoid undefined C++ behaviour if shift is >= number of bits in T.

nit: Naked else without braces is contrary to developer-notes.md:

https://github.com/bitcoin/bitcoin/blob/98939ce7b7441cc95b8fda4d502f182aab1d6a32/doc/developer-notes.md#L76-L79

Consider using ternary operator:

    auto check = [](W value) -> std::optional<W> { return (value >= min && value <= max) ? std::optional{value} : std::nullopt; };

nit in commit 8826cae285490439dc1f19b25fa70b2b9e62dfe8: (We're assigning what seems to be an int64_t to size_t filter_index here. I tried adding curlies on the right side of the assignment and compiling for 32-bit but was unable to trigger narrowing errors. Maybe the compiler is clever enough to see that (MAX_FILTER_INDEX_CACHE << 20) < std::numeric_limits<size_t>::max()).

src/util/byte_units.h:13:29: error: identifier '_MiB' preceded by whitespace in a literal operator declaration is deprecated [-Werror,-Wdeprecated-literal-operator]
   13 | constexpr size_t operator"" _MiB(unsigned long long mebibytes)
      |                  ~~~~~~~~~~~^~~~
      |                  operator""_MiB
1 error generated.

clang 20.0.0

Fixed in https://github.com/bitcoin/bitcoin/pull/31691