kernel,node: add dbcache setter and clarify defaults#35205
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35205. ReviewsSee the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
eb35f8b to
892d6cb
Compare
There was a problem hiding this comment.
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
dbcache default sizingdbcache helpers and add kernel API
892d6cb to
78a6d4b
Compare
|
Thanks @stickies-v, I rewrote the kernel part based on your feedback. The latest push no longer makes |
There was a problem hiding this comment.
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>6d5a3bb to
ce12cd6
Compare
l0rinc
left a comment
There was a problem hiding this comment.
rebased and applied nits.
@stickies-v, I'd appreciate your reevaluation.
maflcko
left a comment
There was a problem hiding this comment.
ce12cd6a1c602658d2ef70cf9b26d7f727ef4af0~5
| size_t filter_index{0}; | ||
| size_t txospender_index{0}; | ||
| }; | ||
| struct CacheSizes { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/dbcacheholds the-dbcachepolicy,node/cachesapplies 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.
There was a problem hiding this comment.
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!

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.
ce12cd6 to
6c5d8e0
Compare
|
Rebased on current master, adapted the stack to the upstream |
dbcache helpers and add kernel APIdbcache setter and clarify defaults
6c5d8e0 to
dc2bc25
Compare
|
|
||
| ChainstateManagerOptions chainman_opts{context, PathToString(test_directory.m_directory), PathToString(test_directory.m_directory / "blocks")}; | ||
| chainman_opts.SetWorkerThreads(4); | ||
| chainman_opts.SetDatabaseCacheBytes(1_GiB); |
There was a problem hiding this comment.
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));There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
}There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
1_GiB is 24 MiB higher than the original test with 1000_MiB. Is this change intentional ?
There was a problem hiding this comment.
Yes, I don't think we can have total memory of 1000_MiB (hence BOOST_CHECK_GE and not BOOST_CHECK_GT)
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.
dc2bc25 to
88dd778
Compare
Problem: The node automatically chooses between a
450 MiBand1 GiBdatabase cache based on detected RAM, as introduced in #34692, whilebitcoinkernelalways 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.