darosior commented at 1:19 pm on May 16, 2025: member

32-bit architecture is limited to 4GiB of RAM, so it doesn’t make sense to set a too high value. A too high value could cause an OOM unbeknownst to the user a while after startup as mempool / dbcache fills.

DrahtBot commented at 1:19 pm on May 16, 2025: contributor

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

Code Coverage & Benchmarks

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, dergoegge, glozow, instagibbs
Concept ACK	Sjors
Stale ACK	tapcrafter

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24539 (Add a “tx output spender” index by sstone)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

l0rinc changes_requested

l0rinc commented at 2:23 pm on May 16, 2025: contributor

Can you please explain this in more detail? Why would a 64 bit system care about the 32 bit limits?

I’m already often setting the dbcache to values like 45GiB (which is already way over the 32 bit limit), I’m not sure I understand the motivation for capping the mempool size to below 4.5 GiB - see: https://github.com/bitcoin/bitcoin/pull/31645/files#diff-3d0856e8b7f136c588b229e0cbd3b2e2c309cd097ade0879521daba4e1bb2a33R1092-R1097

Same for the surprisingly low default values, could you please expand on the justifying arguments.

sipa commented at 6:54 pm on May 16, 2025: member

Why would a 64 bit system care about the 32 bit limits?

It wouldn’t. This PR only affects 32-bit systems.

l0rinc commented at 7:06 pm on May 16, 2025: contributor

Looking at the code, I see that I misunderstood the description, I though we’re capping the values in every circumstance, but now I see that the architectures are handles independently.

l0rinc approved

in src/node/caches.cpp:23 in e88056ba32 outdated

18@@ -19,6 +19,8 @@
19 static constexpr size_t MAX_TX_INDEX_CACHE{1024_MiB};
20 //! Max memory allocated to all block filter index caches combined in bytes.
21 static constexpr size_t MAX_FILTER_INDEX_CACHE{1024_MiB};
22+//! Maximum dbcache size on 32-bit systems.
23+static constexpr size_t MAX_32BIT_DBCACHE{1024_MiB};

tapcrafter commented at 8:14 am on May 18, 2025:

Do I assume correctly that the reason for this limit not being 4 GiB but only 1 GiB is that the sum of all cache/limit values should be below 4GB to avoid issues on 32-bit systems? The same question would apply to MAX_32BIT_MEMPOOL_MB, what is the rationale for the value of 500MiB?

Not sure if this would fall into the “you chose to shoot yourself in the foot” category, but I wonder: If I turned on all indexes and used the maximum “allowed” values for all of them, would I still be able to reach a total sum that is higher than 4GiB on a 32-bit system?

Mostly just thinking aloud here, feel free to ignore. Though a sentence or two of rationale for the chosen values would be ideal IMHO.

Sjors commented at 12:46 pm on May 19, 2025:

ea561779abfb2298a9f20ceafa9e05fe2c58d841: as with -maxmempool, I don’t think we should be too “helpful” and just limit it to the physical maximum.

l0rinc commented at 3:15 pm on May 19, 2025:

Agree with @Sjors, which would also simplify max_db_cache to just std::numeric_limits<size_t>::max()

darosior commented at 8:16 pm on May 19, 2025:

If it’s set to the maximum RAM capacity on 32-bit system the process will start and then just randomly crash after having received enough blocks. 1 GiB seems like a reasonable maximum for a 32-bit system.

in src/node/caches.cpp:33 in e88056ba32 outdated

29@@ -28,7 +30,8 @@ CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
30     if (std::optional<int64_t> db_cache = args.GetIntArg("-dbcache")) {
31         if (*db_cache < 0) db_cache = 0;
32         uint64_t db_cache_bytes = SaturatingLeftShift<uint64_t>(*db_cache, 20);
33-        total_cache = std::max<size_t>(MIN_DB_CACHE, std::min<uint64_t>(db_cache_bytes, std::numeric_limits<size_t>::max()));
34+        const auto max_db_cache{sizeof(void*) == 4 ? MAX_32BIT_DBCACHE : std::numeric_limits<size_t>::max()};

tapcrafter commented at 8:18 am on May 18, 2025:

I’m a bit surprised there isn’t a macro for the 32-bit detection (sizeof(void*) == 4). But I’m fairly new to the C++ part of the codebase and I see this is used in other places too, so just a random review thought that can probably be ignored.

l0rinc commented at 3:14 pm on May 19, 2025:

Same, can be constexpr and could use more descriptive bitness check:

0        constexpr auto max_db_cache{SIZE_MAX == UINT32_MAX ? MAX_32BIT_DBCACHE : std::numeric_limits<size_t>::max()};

darosior commented at 8:41 pm on May 19, 2025:

I disagree this is a more descriptive bitness check. The size of pointers is what we actually care about here. Shouldn’t matter in practice but i don’t even think it’s strictly guaranteed that SIZE_MAX == sizeof(void*). Finally, this is the exact same check we do elsewhere in this codebase to check for 32-bit systems. For these reasons, i kept it as-is.

Made constexpr as requested, though.

tapcrafter commented at 9:08 am on May 18, 2025: none

utACK e88056ba323da9127775a41db92835378600d9fc

Will test this later once I’ve familiarized myself a bit more with the build system to produce 32-bit ARM binaries outside of the guix build.

in src/node/caches.cpp:34 in e88056ba32 outdated

29@@ -28,7 +30,8 @@ CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
30     if (std::optional<int64_t> db_cache = args.GetIntArg("-dbcache")) {
31         if (*db_cache < 0) db_cache = 0;
32         uint64_t db_cache_bytes = SaturatingLeftShift<uint64_t>(*db_cache, 20);
33-        total_cache = std::max<size_t>(MIN_DB_CACHE, std::min<uint64_t>(db_cache_bytes, std::numeric_limits<size_t>::max()));
34+        const auto max_db_cache{sizeof(void*) == 4 ? MAX_32BIT_DBCACHE : std::numeric_limits<size_t>::max()};
35+        total_cache = std::max<size_t>(MIN_DB_CACHE, std::min<uint64_t>(db_cache_bytes, max_db_cache));

tapcrafter commented at 7:08 pm on May 18, 2025:

Should we unify the behavior with the -maxmempool option and error out instead of silently truncating the value? Or is the rationale that the DB cache only affects performance while the mempool size will have an impact on the operation of the node and therefore is more important to notify the user?

darosior commented at 8:41 pm on May 19, 2025:

I adapted to the surrounding code. For -dbcache we trunk so i did the same. For -maxmempool we return an error so i did the same.

tapcrafter commented at 7:13 pm on May 18, 2025: none

tACK e88056ba323da9127775a41db92835378600d9fc

Restrictions on 32-bit machine

Running on a linux-armv7l machine, the -maxmempool option is correctly validated on startup:

0$ uname -a
1
2Linux debian 6.1.0-35-armmp-lpae [#1](/bitcoin-bitcoin/1/) SMP Debian 6.1.137-1 (2025-05-07) armv7l GNU/Linux
3
4$ ./bitcoind -regtest -maxmempool=4096
5
6Error: -maxmempool is set to 4096 but can't be over 500 MB on 32-bit systems

However, the -dbcache value is silently truncated:

0$ ./bitcoind -regtest -dbcache=16384
1
22025-05-18T19:02:57Z Bitcoin Core version v29.99.0-ge88056ba323da9127775a41db92835378600d9fc (release build)
3...
42025-05-18T19:02:57Z Command-line arg: dbcache="16384"
52025-05-18T19:02:57Z Command-line arg: regtest=""
6...
72025-05-18T19:02:58Z * Using 1014.0 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)

No restrictions on any 64-bit machine

Running e88056ba323da9127775a41db92835378600d9fc on a linux-amd64 machine:

 0$ uname -a
 1
 2Linux ubuntu 6.14.0-15-generic [#15](/bitcoin-bitcoin/15/)-Ubuntu SMP PREEMPT_DYNAMIC Sun Apr  6 15:05:05 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
 3
 4$ ./build/bin/bitcoind -regtest -maxmempool=4096 -dbcache=16384
 5
 62025-05-18T14:40:52Z Bitcoin Core version v29.99.0-e88056ba323d (release build)
 7...
 82025-05-18T14:40:52Z Command-line arg: dbcache="16384"
 92025-05-18T14:40:52Z Command-line arg: maxmempool="4096"
102025-05-18T14:40:52Z Command-line arg: regtest=""
11...
122025-05-18T14:40:52Z Cache configuration:
132025-05-18T14:40:52Z * Using 2.0 MiB for block index database
142025-05-18T14:40:52Z * Using 8.0 MiB for chain state database
152025-05-18T14:40:52Z * Using 16374.0 MiB for in-memory UTXO set (plus up to 3906.2 MiB of unused mempool space)
16...
172025-05-18T14:40:52Z Verification progress: 83%
182025-05-18T14:40:52Z Verification progress: 99%
192025-05-18T14:40:52Z Verification: No coin database inconsistencies in last 6 blocks (7 transactions)
202025-05-18T14:40:52Z Block index and chainstate loaded
212025-05-18T14:40:52Z Setting NODE_NETWORK on non-prune mode

Starts without issue, so no impact on 64-bit machines.

laanwj added the label Settings on May 19, 2025

in src/node/mempool_args.cpp:49 in ea561779ab outdated

44@@ -42,7 +45,12 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& argsman, const CChainP
45 {
46     mempool_opts.check_ratio = argsman.GetIntArg("-checkmempool", mempool_opts.check_ratio);
47 
48-    if (auto mb = argsman.GetIntArg("-maxmempool")) mempool_opts.max_size_bytes = *mb * 1'000'000;
49+    if (auto mb = argsman.GetIntArg("-maxmempool")) {
50+        if (sizeof(void*) == 4 && *mb > MAX_32BIT_MEMPOOL_MB) {

Sjors commented at 12:44 pm on May 19, 2025:

Maybe clarify this with something like:

0constexpr is_32bit{sizeof(void*) == 4};

l0rinc commented at 3:12 pm on May 19, 2025:

we could make it an if constexpr as well and use a more descriptive way of checking for 32 bitness:

0        if constexpr (SIZE_MAX == UINT32_MAX && *mb > MAX_32BIT_MEMPOOL_MB) {

darosior commented at 8:41 pm on May 19, 2025:

Sjors, i took your suggestion. L0rinc, as explained above i don’t think this is an improvement.

in src/node/mempool_args.cpp:29 in ea561779ab outdated

24@@ -25,6 +25,9 @@ using common::AmountErrMsg;
25 using kernel::MemPoolLimits;
26 using kernel::MemPoolOptions;
27 
28+//! Maximum mempool size on 32-bit systems.
29+static constexpr int MAX_32BIT_MEMPOOL_MB{500};

Sjors commented at 12:44 pm on May 19, 2025:

ea561779abfb2298a9f20ceafa9e05fe2c58d841: let’s just use 4096? This setting doesn’t check actual memory capacity on 64-bit systems either, so we don’t need to be this protective.

l0rinc commented at 3:17 pm on May 19, 2025:

… and I think it should be a size_t instead

darosior commented at 8:39 pm on May 19, 2025:

If it’s set to the maximum RAM capacity on 32-bit system the process will start and then just randomly crash once there is high transaction traffic. Really if someone wants to use a large mempool they should use a 64-bit system.

I don’t think this constant should be a size_t because it gets compared to a signed integer.

Sjors commented at 12:47 pm on May 19, 2025: member

Concept ACK

If only to remind ourselves that 32-bit systems exist.

Before #28358 we automatically rounded down the number, which wasn’t great.

l0rinc commented at 3:20 pm on May 19, 2025: contributor

I thought we’re sunsetting 32 bit support slowly (it’s why I was originally confused by the PR description). I agree with others that the limits are arbitrary, we should only warn if the values obviously don’t fit into a size_t.

init: cap -maxmempool to 500 MB on 32-bit systems

32-bit architecture is limited to 4GiB, so it doesn't make sense to set a too high value. 500 MB is
chosen as an arbitrary maximum value that seems reasonable.

2c43b6adeb

node: cap -dbcache to 1GiB on 32-bit architectures

32-bit architecture is limited to 4GiB, so it doesn't make sense to set a too
high value. Since this setting is performance critical, pick an arbitrary value
higher than for -maxmempool but still reasonable.

9f8e7b0b3b

darosior force-pushed on May 19, 2025

darosior commented at 8:57 pm on May 20, 2025: member

Can someone with permissions turn this into draft?

glozow marked this as a draft on May 21, 2025

darosior marked this as ready for review on Jun 18, 2025

darosior commented at 4:31 pm on June 18, 2025: member

Undrafted this and added a bit more motivation to OP. This is not a huge deal, but a too high value could lead to OOM after the node has run for some time, once mempool or dbcache fills up. This mitigates this by failing early instead.

achow101 commented at 0:09 am on June 19, 2025: member

ACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6

DrahtBot requested review from tapcrafter on Jun 19, 2025

DrahtBot requested review from Sjors on Jun 19, 2025

dergoegge approved

dergoegge commented at 8:59 am on June 24, 2025: member

Code review ACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6

The chosen limits seem reasonable to me.

fanquake added the label Needs release note on Jun 25, 2025

fanquake commented at 12:32 pm on June 25, 2025: member

Should get a release note, and likely also going to be backported to 29.x.

glozow commented at 6:58 pm on June 25, 2025: member

utACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6

instagibbs commented at 4:04 pm on June 26, 2025: member

utACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6

fanquake merged this on Jun 26, 2025

fanquake closed this on Jun 26, 2025

fanquake commented at 4:36 pm on June 26, 2025: member

Backported to 29.x in #32810.

fanquake referenced this in commit eafea2393d on Jun 26, 2025

fanquake referenced this in commit 1c0e19b93a on Jun 26, 2025

darosior referenced this in commit 558f0880a8 on Jun 26, 2025

darosior deleted the branch on Jun 26, 2025

fanquake removed the label Needs release note on Jun 26, 2025

fanquake referenced this in commit 4a3475a43e on Jun 27, 2025

node: cap -maxmempool and -dbcache values for 32-bit #32530

Code Coverage & Benchmarks

Reviews

Conflicts

Restrictions on 32-bit machine

No restrictions on any 64-bit machine

node: cap `-maxmempool` and `-dbcache` values for 32-bit #32530