kernel,node: clean up `dbcache` helpers and add kernel API #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 node-side policy is still split across generic cache and system helpers, which makes it harder to see which callers use the current dbcache default. Separately, kernel still has only a fixed 450 MiB fallback and no C API for callers to provide a cache budget from outside.

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

Keep kernel independent from the node/common RAM policy by preserving DEFAULT_KERNEL_CACHE as its fallback and adding a chainstate manager option setter for explicit database cache bytes.

This is a simplified follow-up to #34641, keeping the #34692 two-tier node 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 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 ACK	sedited
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:

#35322 (logging: streamline Logger state and drop redundant methods by ryanofsky)
#35200 (node: smooth oversized dbcache warnings by l0rinc)
#32427 (kernel: Replace leveldb-based BlockTreeDB with flat-file based store by sedited)
#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
#28690 (build: Introduce internal kernel library by sedited)
#26022 (Add util::ResultPtr class by ryanofsky)
#25722 (refactor: Use util::Result class for wallet loading 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

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

l0rinc renamed this:
~~node,kernel: share `dbcache` default sizing~~
kernel,node: clean up `dbcache` helpers and add kernel API
on May 18, 2026

l0rinc force-pushed on May 18, 2026

l0rinc commented at 8:36 PM on May 18, 2026: contributor

Thanks @stickies-v, I rewrote the kernel part based on your feedback.

The latest push no longer makes bitcoinkernel depend on common/system_ram or node/dbcache, but keeps DEFAULT_KERNEL_CACHE as the kernel fallback and adds an explicit database-cache setter to the C API, so callers can pass the cache budget from outside.

l0rinc marked this as ready for review on May 18, 2026

DrahtBot added the label CI failed on May 18, 2026

DrahtBot removed the label CI failed on May 19, 2026

sedited commented at 8:44 PM on May 20, 2026: contributor

Concept ACK

DrahtBot added the label Needs rebase on May 22, 2026

node, qt: use `1_MiB` for dbcache conversions

Convert dbcache display values with `/ 1_MiB` instead of `>> 20`, so the unit is explicit before the helper split.
Use the local Qt functional-cast style for the touched settings migration.

05d7b67be7

common: move `GetTotalRAM()` to system RAM files

Move the OS-specific total RAM query out of `common/system.{cpp,h}` and into `common/system_ram.{cpp,h}`.
This lets dbcache policy include the RAM helper without pulling in the broader system header.

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

14cfb0ad80

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

Use the `Try` prefix to make failed RAM detection visible at call sites.

-BEGIN VERIFY SCRIPT-
git grep -q 'TryGetTotalRam' -- src && echo "Error: TryGetTotalRam already exists in src" && exit 1
git grep -l 'GetTotalRAM' -- src | xargs perl -pi -e 's/\bGetTotalRAM\b/TryGetTotalRam/g'
-END VERIFY SCRIPT-

fd680ff228

common: cache `TryGetTotalRam()` result

Store the detected total RAM in a function-local static so startup paths do not repeat the underlying system query.

236030460d

node: move dbcache helpers to `node/dbcache`

Move dbcache defaults and warning helpers out of `node/caches.{h,cpp}` so cache splitting can include only the policy it needs.
Update node, Qt, and test includes for the new header, and have the Qt migration use `node::GetDefaultDBCache()`.

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

d80ca7920d

kernel: allow setting chainstate `dbcache`

Expose `btck_chainstate_manager_options_set_database_cache_bytes()` so callers can supply the database cache budget instead of making `bitcoinkernel` compute node-side RAM policy.
Keep `DEFAULT_KERNEL_CACHE` as the fallback when no override is set.
Store the selected `kernel::CacheSizes` on `ChainstateManagerOptions`, keep the block-tree DB cache in sync, and pass the same sizes to `LoadChainstate()`.
Add the C++ wrapper method and cover it in `test_kernel`.

Co-authored-by: stickies-v <stickies-v@protonmail.com>

de015172ef

node: add `MAX_DBCACHE_BYTES`

Move the architecture-dependent `-dbcache` cap next to the other dbcache constants.
The effective cap is unchanged: `1 GiB` on 32-bit builds and `size_t` max elsewhere.
Leave the storage type unchanged so the later `uint64_t` cleanup is isolated.

7e08deea5b

scripted-diff: rename dbcache byte constants

Add a `_BYTES` suffix to the min and default dbcache constants so they read as a group with `MAX_DBCACHE_BYTES`.

-BEGIN VERIFY SCRIPT-
git grep -qE '\b(MIN_DBCACHE_BYTES|DEFAULT_DBCACHE_BYTES)\b' -- src && echo "Error: renamed dbcache byte constants already exist in src" && exit 1
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>

7eaf55473b

test: require `TryGetTotalRam()` detection

`TryGetTotalRam()` now feeds the automatic `-dbcache` default, so a detection failure should fail `system_ram_tests` instead of being skipped.
Use `1_GiB` for the lower bound and remove the obsolete skip regex.

71bc8e674b

node: use `uint64_t` for dbcache byte constants

Keep dbcache byte constants in the same type as the byte-unit literals and parsed `-dbcache` byte count.
`GetDefaultDBCache()` and cache-splitting APIs still return `size_t` at their allocation boundaries, but `CalculateDbCacheBytes()` no longer needs an explicit `std::min<uint64_t>`.

c24ed5892a

l0rinc force-pushed on May 22, 2026

l0rinc commented at 7:05 AM on May 22, 2026: contributor

Rebased, ready for review again.

DrahtBot removed the label Needs rebase on May 22, 2026

in src/node/dbcache.h:17 in c24ed5892a

  12 | +#include <limits>
  13 | +
  14 | +//! min. -dbcache (bytes)
  15 | +static constexpr uint64_t MIN_DBCACHE_BYTES{4_MiB};
  16 | +//! -dbcache default (bytes)
  17 | +static constexpr uint64_t DEFAULT_DBCACHE_BYTES{450_MiB};

sedited commented at 9:48 AM on June 8, 2026:

Why is this decoupled from the DEFAULT_KERNEL_CACHE?

in src/qt/optionsmodel.cpp:735 in c24ed5892a

 731 | @@ -732,7 +732,7 @@ void OptionsModel::checkAndMigrate()
 732 |          // see https://github.com/bitcoin/bitcoin/pull/8273
 733 |          // force people to upgrade to the new value if they are using 100MB
 734 |          if (settingsVersion < 130000 && settings.contains("nDatabaseCache") && settings.value("nDatabaseCache").toLongLong() == 100)
 735 | -            settings.setValue("nDatabaseCache", (qint64)(DEFAULT_DB_CACHE >> 20));
 736 | +            settings.setValue("nDatabaseCache", qint64(node::GetDefaultDBCache() / 1_MiB));

sedited commented at 10:02 AM on June 8, 2026:

Is changing this to GetDefaultDBCache a bug fix?

sedited commented at 10:03 AM on June 8, 2026: contributor

Approach ACK

Took me a moment to follow along, but I do think now that what you call a "policy" split is probably a good thing and the changes seem fine.