node,kernel: share `dbcache` default sizing #35205

l0rinc commented at 12:09 PM on May 4, 2026: contributor

Problem

PR #34692 kept the simplified two-tier -dbcache default from #34641: use 1024 MiB on 64-bit systems with at least 4 GiB of detected RAM, and otherwise keep 450 MiB.

The policy is still split across generic cache and system helpers, while kernel still initializes from a fixed 450 MiB value instead of the shared dbcache default. This makes it harder to see which callers use the current dbcache policy and leaves node, Qt, and kernel paths less obviously aligned.

Fix

Move RAM detection to common/system_ram and move dbcache constants/helpers to node/dbcache, then route the remaining dbcache default users through node::GetDefaultDBCache().

This is a simplified follow-up to #34641, keeping the #34692 two-tier default unchanged. It does not reintroduce continuous RAM-aware default sizing, and keeps the remaining cleanup focused on naming, typing, and test coverage around the shared dbcache policy.

DrahtBot commented at 12:09 PM on May 4, 2026: 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/35205.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Approach NACK	stickies-v

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35200 (node: smooth oversized dbcache warnings by l0rinc)
#34995 (ci, iwyu: Fix warnings in src/common and treat them as errors by hebasto)
#34806 (refactor: logging: Various API improvements by ajtowns)
#32427 (kernel: Replace leveldb-based BlockTreeDB with flat-file based store by sedited)
#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
#28690 (build: Introduce internal kernel library by sedited)
#26022 (Add util::ResultPtr class by ryanofsky)
#25665 (refactor: Add util::Result failure types and ability to merge result values by ryanofsky)
#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
#17783 (common: Disallow calling IsArgSet() on ALLOW_LIST options by ryanofsky)
#17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
#17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
#17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)

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.

DrahtBot added the label CI failed on May 4, 2026

DrahtBot commented at 1:00 PM on May 4, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task i686, no IPC: https://github.com/bitcoin/bitcoin/actions/runs/25318144859/job/74220316972</sub> <sub>LLM reason (✨ experimental): CI failed because the Bitcoin Core Test Suite had failing CTest results—sock_tests failed.</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

in src/common/system_ram.cpp:23 in 56a8728519

  18 | +std::optional<size_t> GetTotalRAM()
  19 | +{
  20 | +    [[maybe_unused]] auto clamp{[](uint64_t v) { return size_t(std::min(v, uint64_t{std::numeric_limits<size_t>::max()})); }};
  21 | +#ifdef WIN32
  22 | +    if (MEMORYSTATUSEX m{}; (m.dwLength = sizeof(m), GlobalMemoryStatusEx(&m))) return clamp(m.ullTotalPhys);
  23 | +#elif defined(__APPLE__) || \

kevkevinpal commented at 2:56 PM on May 4, 2026:

One thing I noticed here is that in Apple's man page for sysconf, _SC_PHYS_PAGES and _SC_PAGESIZE are not listed at all. There is a chance that on OSX systems they may not be defined as seen in this post.

The intended api for RAM on __APPLE__ looks to be sysctlbyname, which is used in crc32c and src/crypto/sha256.cpp

./src/crc32c/src/crc32c_arm64_check.h:57:  return sysctlbyname("hw.optional.armv8_crc32", &val, &len, nullptr, 0) == 0
./src/crypto/sha256.cpp:673:        if (sysctlbyname("hw.optional.arm.FEAT_SHA256", &val, &len, nullptr, 0) == 0) {

l0rinc commented at 6:19 PM on May 4, 2026:

This was introduced in https://github.com/bitcoin/bitcoin/pull/33333/changes/6c720459beead5c825b354a1d5c11969b6e3a170, it's not strictly related to the change, the code is just moved here.

It's a valid observation though: for some reason the Apple man page doesn't list these two, but they are defined in their own open-source Libc tree and are working in reality:

_SC_PHYS_PAGES: https://github.com/apple-oss-distributions/Libc/blob/71bbe350ab79eef58113991d817ccc6165061a64/include/unistd.h#L383
_SC_PAGESIZE: https://github.com/apple-oss-distributions/Libc/blob/main/include/unistd.h#L259

If it ever becomes a problem we can apply the alternative your suggesting.

kevkevinpal commented at 6:46 PM on May 4, 2026:

Awesome, they must have forgotten to update their docs.

Sounds like a plan, hopefully it is never an issue!

l0rinc closed this on May 5, 2026

l0rinc reopened this on May 5, 2026

DrahtBot removed the label CI failed on May 5, 2026

refactor: use `1_MiB` in `dbcache` size conversions

Replace bare `>> 20` shifts in dbcache-related MiB conversions with `/ 1_MiB`, so the unit is visible at the call site before the dbcache helpers are split out.
Also change an incidentally-touched C-cast in the Qt settings migration to the local functional-cast style.

1663fac2fb

refactor: split `GetTotalRAM()` into `common/system_ram.{cpp,h}`

Move the OS-dependent total RAM query out of `common/system.cpp` into a dedicated `common/system_ram.cpp` translation unit and declare it in `common/system_ram.h`.

Co-authored-by: sipa <sipa@bitcoincore.org>

a4075b5ae3

scripted-diff: rename `GetTotalRAM` to `TryGetTotalRam`

Use the `Try*` prefix and lowercase `Ram` to make the optional/failable nature of the detection explicit at call sites.

-BEGIN VERIFY SCRIPT-
git grep -l 'GetTotalRAM' -- src | xargs perl -pi -e 's/\bGetTotalRAM\b/TryGetTotalRam/g'
-END VERIFY SCRIPT-

66d636daaf

common: cache `TryGetTotalRam()` result

The dbcache default and oversized-warning paths can both query total RAM during startup.
Cache the OS query result in a function-local static, so callers can use `TryGetTotalRam()` without repeating the underlying `sysconf` or `GlobalMemoryStatusEx` call.

79979dc24e

refactor: extract dbcache helpers into `node/dbcache.{cpp,h}`

Move the `dbcache`-specific declarations and definitions out of `node/caches.{h,cpp}` into a dedicated `node/dbcache` translation unit.

Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>

84af915de7

kernel: use the shared `dbcache` default

PR #34692 made node startup and Qt settings use `node::GetDefaultDBCache()`, but `bitcoinkernel` still used the fixed `DEFAULT_KERNEL_CACHE` value.

Use the shared helper in the two kernel initialization paths, so kernel users get the same `450 MiB` or `1024 MiB` default as the node path.
Also align the Qt settings migration default with the helper, matching what `getOption(DatabaseCache)` already returns post-#34692.
Drop `DEFAULT_KERNEL_CACHE` now that the node helper is the single default source.

509bf2c3fd

node: introduce `MAX_DBCACHE_BYTES`

Express the manual `-dbcache` cap once as `MAX_DBCACHE_BYTES` instead of hard-wiring the 32-bit branch at the clamp site.

The effective cap and storage type are unchanged: `1 GiB` on 32-bit builds and `size_t` max on other builds.
This leaves the later byte-constant cleanup as the only commit that changes these dbcache policy constants to `uint64_t`.

17fa230efa

scripted-diff: rename dbcache byte constants

Use the `_BYTES` suffix for `MIN_DBCACHE_BYTES` and `DEFAULT_DBCACHE_BYTES` so the units are obvious and the dbcache bounds/default read as one group with `MAX_DBCACHE_BYTES`.

-BEGIN VERIFY SCRIPT-
git grep -l \
    -e 'MIN_DB_CACHE' -e 'DEFAULT_DB_CACHE' -- src | \
    xargs perl -pi \
        -e 's/\bMIN_DB_CACHE\b/MIN_DBCACHE_BYTES/g;' \
        -e 's/\bDEFAULT_DB_CACHE\b/DEFAULT_DBCACHE_BYTES/g;'
-END VERIFY SCRIPT-

Co-authored-by: optout <13562139+optout21@users.noreply.github.com>

a0c5e6e7fd

test: require `TryGetTotalRam()` in unit test

Now that `TryGetTotalRam()` feeds the auto `-dbcache` default, a detection failure would hide a default-policy problem.
Make `system_ram_tests` fail instead of warning and skipping.

Use `1_GiB` as the lower bound for unit consistency and drop the `skipping total_ram` regex because the test no longer emits it.

33898daa0b

node: use `uint64_t` for dbcache byte constants

The byte-unit literals used by the dbcache policy are `uint64_t`, but several dbcache-local constants still store those values as `size_t`.

Move the min, default, max, and high-default byte constants to `uint64_t` while keeping `GetDefaultDBCache()` and the cache-splitting API at their existing `size_t` boundaries.
With `MAX_DBCACHE_BYTES` now matching the parsed byte count type, `CalculateDbCacheBytes()` no longer needs an explicit `std::min<uint64_t>`.

892d6cb994

l0rinc force-pushed on May 5, 2026

stickies-v commented at 4:13 PM on May 6, 2026: contributor

Kernel shouldn't have any dependencies on common, so I think this approach is a regression. Generally, I think we should move towards kernel having less system dependencies instead of more, so Approach NACK for me

l0rinc marked this as a draft on May 7, 2026