Skip to content

kernel,node: add dbcache setter and clarify defaults#35205

Open
l0rinc wants to merge 9 commits into
bitcoin:masterfrom
l0rinc:l0rinc/share-dbcache-defaults
Open

kernel,node: add dbcache setter and clarify defaults#35205
l0rinc wants to merge 9 commits into
bitcoin:masterfrom
l0rinc:l0rinc/share-dbcache-defaults

Conversation

@l0rinc

@l0rinc l0rinc commented May 4, 2026

Copy link
Copy Markdown
Contributor

Problem: The node automatically chooses between a 450 MiB and 1 GiB database cache based on detected RAM, as introduced in #34692, while bitcoinkernel always uses the former.
The existing helper and constant names blur the node's automatic policy with the fixed kernel fallback, and kernel callers cannot supply their own cache budget.

Fix: Cache the detected RAM value, keep the node's two-tier default unchanged, and make the fixed fallback explicit.
Add a chainstate-manager option setter so kernel callers can provide the total database cache budget; when unset, the kernel still uses 450 MiB.

@DrahtBot

DrahtBot commented May 4, 2026

Copy link
Copy Markdown
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 and AI policy for information on the review process.

Type Reviewers
ACK w0xlt
Stale ACK sedited

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> 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)
  • #34995 (iwyu: Fix warnings in src/common and treat them as errors by hebasto)
  • #31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
  • #29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
  • #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)
  • #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.

@l0rinc l0rinc mentioned this pull request May 4, 2026
13 tasks
@DrahtBot

DrahtBot commented May 4, 2026

Copy link
Copy Markdown
Contributor

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

Hints

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.

Comment thread src/common/system_ram.cpp Outdated

@stickies-v stickies-v left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

edited: my concern seems to be addressed

@l0rinc

l0rinc commented May 18, 2026

Copy link
Copy Markdown
Contributor Author

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 l0rinc marked this pull request as ready for review May 18, 2026 20:38

@w0xlt w0xlt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 6d5a3bb

Some non-blocking nits:

Comment thread src/common/system.cpp Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are those includes stale ?

diff --git a/src/common/system.cpp b/src/common/system.cpp
index 0b1f05e02c..306b17d87e 100644
--- a/src/common/system.cpp
+++ b/src/common/system.cpp
@@ -25,12 +25,8 @@
 #include <malloc.h>
 #endif
 
-#include <algorithm>
-#include <cstddef>
-#include <cstdint>
 #include <cstdlib>
 #include <locale>
-#include <optional>
 #include <stdexcept>
 #include <string>
 #include <thread>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks

Comment thread src/common/system_ram.h
Comment thread src/test/kernel/test_kernel.cpp Outdated
@DrahtBot DrahtBot requested a review from sedited June 19, 2026 01:25
@l0rinc l0rinc force-pushed the l0rinc/share-dbcache-defaults branch from 6d5a3bb to ce12cd6 Compare June 24, 2026 02:40

@l0rinc l0rinc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rebased and applied nits.

@stickies-v, I'd appreciate your reevaluation.

Comment thread src/test/kernel/test_kernel.cpp Outdated
Comment thread src/common/system_ram.h
Comment thread src/common/system.cpp Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks

@w0xlt w0xlt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK ce12cd6

@sedited sedited left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK ce12cd6

@maflcko maflcko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ce12cd6a1c602658d2ef70cf9b26d7f727ef4af0~5

Comment thread src/node/caches.h
size_t filter_index{0};
size_t txospender_index{0};
};
struct CacheSizes {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this pull request? What is the point of splitting this header up further? The size is only 29 lines before this pull request, and after this pull request there are several dbcache modules (caches and dbcache, both concerning the cache sizes of the dbs, where it is not clear why they are different).

Not sure what the goal of this pull is, but I presume it is to remove the confusing DEFAULT_DB_CACHE? If yes, I presume it would be trivial to remove in a single simple commit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the point of splitting this header up further?

@sipa explicitly asked for the dbcache declaration and definition to be colocated and the RAM declaration to move with its implementation.

it is not clear why they are different

node/dbcache holds the -dbcache policy, node/caches applies and splits the selected budget.
I don't mind folding them back together if this separation is not useful.

Not sure what the goal of this pull is, but I presume it is to remove the confusing DEFAULT_DB_CACHE

Partially - the kernel setter is the functional addition, the remaining commits clean up units, types, and tests around the existing two-tier default.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the point of splitting this header up further?

sipa explicitly asked for the dbcache declaration and definition to be colocated and the RAM declaration to move with its implementation.

Right. Though, this is just a rule from the dev notes:

doc/developer-notes.md:807:  - *Rationale*: Include files define the interface for the code in implementation files.  

I don't think sipa asked for the split itself?

Also, the commit is co-authored by an email that doesn't exist?

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

I wonder where that email is from? I couldn't find it in the git logs or the pgp keys, or any other public source.

it is not clear why they are different

node/dbcache holds the -dbcache policy, node/caches applies and splits the selected budget. I don't mind folding them back together if this separation is not useful.

Ok, I see. No strong opinion. I guess it is just me not seeing the point. If other reviewers are happy, then it should be fine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think sipa asked for the split itself?

Hah, looks like I misunderstood what he meant originally, thanks for persisting!
I dropped the commit changing node/dbcache files + CMake entry.

Also, the commit is co-authored by an email that doesn't exist

Yeah, that was bullshit - I couldn't find a source for that, it's not visible in my IDE either - thanks for noticing!
image

I guess it is just me not seeing the point

I didn't mean to imply that - I went over the changes again, dropped the node/dbcache split, split one commit to two smaller ones, and simplified the commit messages and PR description based on the feedback.
I also removed duplicated derived cache state from the kernel options.

@l0rinc l0rinc force-pushed the l0rinc/share-dbcache-defaults branch from ce12cd6 to 6c5d8e0 Compare July 9, 2026 18:23
@l0rinc

l0rinc commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Rebased on current master, adapted the stack to the upstream size_t -> uint64_t changes, folded the final type-cleanup commit into the earlier ones.

@l0rinc l0rinc changed the title kernel,node: clean up dbcache helpers and add kernel API kernel,node: add dbcache setter and clarify defaults Jul 14, 2026
@l0rinc l0rinc force-pushed the l0rinc/share-dbcache-defaults branch from 6c5d8e0 to dc2bc25 Compare July 14, 2026 18:12
Comment thread src/test/kernel/test_kernel.cpp Outdated

ChainstateManagerOptions chainman_opts{context, PathToString(test_directory.m_directory), PathToString(test_directory.m_directory / "blocks")};
chainman_opts.SetWorkerThreads(4);
chainman_opts.SetDatabaseCacheBytes(1_GiB);

@w0xlt w0xlt Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btck_chainstate_manager_options_set_database_cache_bytes() accepts 0 and 1, but these produce an empty coins or coins-DB cache and later abort in node/chainstate.cpp. Should the setter reject these values before mutating the options?

Suggestion:

diff
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index a5c163502c..da6f33fed0 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/kernel/bitcoinkernel.cpp
@@ -1032,12 +1032,19 @@ void btck_chainstate_manager_options_set_worker_threads_num(btck_ChainstateManag
     btck_ChainstateManagerOptions::get(opts).m_chainman_options.worker_threads_num = worker_threads;
 }
 
-void btck_chainstate_manager_options_set_database_cache_bytes(btck_ChainstateManagerOptions* chainman_opts, size_t database_cache_bytes)
+int btck_chainstate_manager_options_set_database_cache_bytes(btck_ChainstateManagerOptions* chainman_opts, size_t database_cache_bytes)
 {
+    const kernel::CacheSizes cache_sizes{database_cache_bytes};
+    if (cache_sizes.coins_db == 0 || cache_sizes.coins == 0) {
+        LogError("Failed to set database cache: size must be at least 2 bytes.");
+        return -1;
+    }
+
     auto& opts{btck_ChainstateManagerOptions::get(chainman_opts)};
     LOCK(opts.m_mutex);
     opts.m_db_cache_bytes = database_cache_bytes;
-    opts.m_blockman_options.block_tree_db_params.cache_bytes = kernel::CacheSizes{database_cache_bytes}.block_tree_db;
+    opts.m_blockman_options.block_tree_db_params.cache_bytes = cache_sizes.block_tree_db;
+    return 0;
 }
 
 void btck_chainstate_manager_options_destroy(btck_ChainstateManagerOptions* options)
diff --git a/src/kernel/bitcoinkernel.h b/src/kernel/bitcoinkernel.h
index 73475e47e4..d0219ece6f 100644
--- a/src/kernel/bitcoinkernel.h
+++ b/src/kernel/bitcoinkernel.h
@@ -1203,9 +1203,10 @@ BITCOINKERNEL_API void btck_chainstate_manager_options_set_worker_threads_num(
  * called, the total cache defaults to 450 MiB.
  *
  * @param[in] chainstate_manager_options Non-null, options to be set.
- * @param[in] database_cache_bytes       The total database cache size in bytes.
+ * @param[in] database_cache_bytes       The total database cache size in bytes. Values below 2 are rejected.
+ * @return                               0 if the set was successful, non-zero if the set failed.
  */
-BITCOINKERNEL_API void btck_chainstate_manager_options_set_database_cache_bytes(
+BITCOINKERNEL_API int BITCOINKERNEL_WARN_UNUSED_RESULT btck_chainstate_manager_options_set_database_cache_bytes(
     btck_ChainstateManagerOptions* chainstate_manager_options,
     size_t database_cache_bytes) BITCOINKERNEL_ARG_NONNULL(1);
 
diff --git a/src/kernel/bitcoinkernel_wrapper.h b/src/kernel/bitcoinkernel_wrapper.h
index 42d19e4a22..9783c3aba1 100644
--- a/src/kernel/bitcoinkernel_wrapper.h
+++ b/src/kernel/bitcoinkernel_wrapper.h
@@ -1200,9 +1200,9 @@ public:
         btck_chainstate_manager_options_set_worker_threads_num(get(), worker_threads);
     }
 
-    void SetDatabaseCacheBytes(size_t database_cache_bytes)
+    bool SetDatabaseCacheBytes(size_t database_cache_bytes)
     {
-        btck_chainstate_manager_options_set_database_cache_bytes(get(), database_cache_bytes);
+        return btck_chainstate_manager_options_set_database_cache_bytes(get(), database_cache_bytes) == 0;
     }
 
     bool SetWipeDbs(bool wipe_block_tree, bool wipe_chainstate)
diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index 2ac48b6746..c72c8a995a 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/kernel/test_kernel.cpp
@@ -801,7 +801,10 @@ BOOST_AUTO_TEST_CASE(btck_chainman_tests)
 
     ChainstateManagerOptions chainman_opts{context, PathToString(test_directory.m_directory), PathToString(test_directory.m_directory / "blocks")};
     chainman_opts.SetWorkerThreads(4);
-    chainman_opts.SetDatabaseCacheBytes(1_GiB);
+    BOOST_CHECK(chainman_opts.SetDatabaseCacheBytes(2));
+    BOOST_CHECK(chainman_opts.SetDatabaseCacheBytes(1_GiB));
+    BOOST_CHECK(!chainman_opts.SetDatabaseCacheBytes(0));
+    BOOST_CHECK(!chainman_opts.SetDatabaseCacheBytes(1));
     BOOST_CHECK(!chainman_opts.SetWipeDbs(/*wipe_block_tree=*/true, /*wipe_chainstate=*/false));
     BOOST_CHECK(chainman_opts.SetWipeDbs(/*wipe_block_tree=*/true, /*wipe_chainstate=*/true));
     BOOST_CHECK(chainman_opts.SetWipeDbs(/*wipe_block_tree=*/false, /*wipe_chainstate=*/true));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, and move MIN_DBCACHE_BYTES to src/kernel/caches.h? Sure, pushed, let me know what you think.


BOOST_CHECK_GE(*total, 1000_MiB);
const auto total{TryGetTotalRam()};
BOOST_REQUIRE(total);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the Try name and std::optional return type, I assume RAM detection may legitimately fail on some supported environments. Or do we expect successful detection on every test platform?

Suggestion (for the former case):

diff
diff --git a/src/test/CMakeLists.txt b/src/test/CMakeLists.txt
index 0e4e4fd1fb..b5a12f7277 100644
--- a/src/test/CMakeLists.txt
+++ b/src/test/CMakeLists.txt
@@ -199,6 +199,7 @@ function(add_boost_test source_file)
       SKIP_REGULAR_EXPRESSION
         "no test cases matching filter"
         "skipping script_assets_test"
+        "skipping total_ram"
     )
   endforeach()
 endfunction()
diff --git a/src/test/system_ram_tests.cpp b/src/test/system_ram_tests.cpp
index 16da6d1065..60f6361472 100644
--- a/src/test/system_ram_tests.cpp
+++ b/src/test/system_ram_tests.cpp
@@ -12,7 +12,11 @@ BOOST_AUTO_TEST_SUITE(system_ram_tests)
 BOOST_AUTO_TEST_CASE(total_ram)
 {
     const auto total{TryGetTotalRam()};
-    BOOST_REQUIRE(total);
+    if (!total) {
+        BOOST_WARN_MESSAGE(false, "skipping total_ram: total RAM unknown");
+        return;
+    }
+
     BOOST_CHECK_GE(*total, 1_GiB);
     BOOST_CHECK_LT(*total, 10'000_GiB); // ~10 TiB memory is unlikely
 }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean to drop dc2bc25? Since we depend on this for more than just the warning I think we should support every CI agent that we run this on.

BOOST_CHECK_GE(*total, 1000_MiB);
const auto total{TryGetTotalRam()};
BOOST_REQUIRE(total);
BOOST_CHECK_GE(*total, 1_GiB);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1_GiB is 24 MiB higher than the original test with 1000_MiB. Is this change intentional ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I don't think we can have total memory of 1000_MiB (hence BOOST_CHECK_GE and not BOOST_CHECK_GT)

l0rinc and others added 9 commits July 14, 2026 19:06
This lets `node/caches` use the RAM helper without including the broader system header.
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-
-BEGIN VERIFY SCRIPT-
git grep -q '\bMIN_DBCACHE_BYTES\b' -- src && echo "Error: renamed dbcache byte constant already exists in src" && exit 1
git grep -l 'MIN_DB_CACHE' -- src | xargs perl -pi -e 's/\bMIN_DB_CACHE\b/MIN_DBCACHE_BYTES/g'
-END VERIFY SCRIPT-

Co-authored-by: optout <13562139+optout21@users.noreply.github.com>
Keep the total-cache bounds with `kernel::CacheSizes` so callers share the same range.
Add `btck_chainstate_manager_options_set_database_cache_bytes()` so kernel callers can provide the total database cache budget, while `DEFAULT_KERNEL_CACHE` remains the fallback.
Use the selected split for the block tree database and `LoadChainstate()`.

Co-authored-by: stickies-v <stickies-v@protonmail.com>
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
The alias is misleading because automatic selection can also return `HIGH_DEFAULT_DBCACHE`.
RAM detection controls automatic `-dbcache` selection, so fail the test when it is unavailable.
@l0rinc l0rinc force-pushed the l0rinc/share-dbcache-defaults branch from dc2bc25 to 88dd778 Compare July 15, 2026 02:35

@w0xlt w0xlt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK 88dd778

@DrahtBot DrahtBot requested a review from sedited July 15, 2026 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants