btck_chainstate_manager_options_*_in_memory functions and rename them from “update” to “set”. These functions now only set the options to true for enabling in-memory mode. This, combined with explicit default initialization of in-memory options to false in ChainstateManagerOptions constructor, should remove ambiguity about what happens when these functions are not called.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33877.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept NACK | stickies-v |
| Concept ACK | yuvicc |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, 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.
Remove the int parameter from `btck_chainstate_manager_options_*_in_memory` functions and rename them from "update" to "set". These functions now only set the options to `true` for enabling in-memory mode. This, combined with explicit default initialization to `false`, removes ambiguity about what happens when these functions are not called and makes the typical use case (disk-based storage) require no action from the user.
🚧 At least one of the CI tasks failed.
Task Linux->Windows cross, no tests: https://github.com/bitcoin/bitcoin/actions/runs/19390523535/job/55483324598
LLM reason (✨ experimental): Compile error: missing initializer for member coins_error_cb in ChainstateManagerOptions, with warnings treated as errors.
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.
This occurred to me while refactoring the context and chainstate manager options in the Go wrapper, so I opened this PR to get feedback.
460@@ -461,8 +461,12 @@ struct ChainstateManagerOptions {
461 .block_tree_db_params = DBParams{
462 .path = data_dir / "blocks" / "index",
463 .cache_bytes = kernel::CacheSizes{DEFAULT_KERNEL_CACHE}.block_tree_db,
464+ .memory_only = false,
465 }}},
466- m_context{context}, m_chainstate_load_options{node::ChainstateLoadOptions{}}
467+ m_context{context}, m_chainstate_load_options{node::ChainstateLoadOptions{
468+ .coins_db_in_memory = false,
469+ .coins_error_cb = {},
ChainstateManagerOptions ctor) for options we have setters for. Looks more intentional and readable to me this way.
But coins_error_cb = {} is to make the compiler happy.
Concept ACK
Makes sense to refractor update to set and remove the int parameter here.
Hmm, I think I’m ~0 on this, leaning towards Concept NACK. Removing unnecessary parameters is good, but I personally find an interface more ergonomic when you can call object.set_bool(value) instead of if (value) object.set_bool(). It generalizes better, and makes using the object more flexible. For example, if value depends on multiple factors, with the last one taking priority, the value can be updated multiple times with object.set_bool(value) without needing extra state.
In the case of test_kernel.cpp, I personally would have just written it as:
0diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
1index df0c8ca1b5..3ebec5b59b 100644
2--- a/src/test/kernel/test_kernel.cpp
3+++ b/src/test/kernel/test_kernel.cpp
4@@ -653,12 +653,8 @@ std::unique_ptr<ChainMan> create_chainman(TestDirectory& test_directory,
5 if (wipe_chainstate) {
6 chainman_opts.SetWipeDbs(/*wipe_block_tree=*/false, /*wipe_chainstate=*/wipe_chainstate);
7 }
8- if (block_tree_db_in_memory) {
9- chainman_opts.SetBlockTreeDbInMemory();
10- }
11- if (chainstate_db_in_memory) {
12- chainman_opts.SetChainstateDbInMemory();
13- }
14+ chainman_opts.UpdateBlockTreeDbInMemory(block_tree_db_in_memory);
15+ chainman_opts.UpdateChainstateDbInMemory(chainstate_db_in_memory);
16
17 auto chainman{std::make_unique<ChainMan>(context, chainman_opts)};
18 return chainman;
Do you have an example of how this makes things better for you?
update(bool_value) function might indicate that we’re supporting some meaningful toggleable functionality, while in reality once we construct the chainman, changing these options has no effect.
wouldn’t the cases you’re describing be mostly for debugging/testing purposes … we would know whether to enable in-memory mode at compile time
I don’t know? I wouldn’t find it unreasonable to use kernel for ephemeral in-memory operations.
Also, having an update(bool_value) function might indicate that we’re supporting some meaningful toggleable functionality, while in reality once we construct the chainman, changing these options has no effect.
The same issue exist with either approach, I don’t think there’s a meaningful difference there?
I don’t know? I wouldn’t find it unreasonable to use kernel for ephemeral in-memory operations.
Of course. I meant that maybe the case where we would “dynamically” decide to use in-memory mode wouldn’t be that common.
I found the approach in this PR to be a slight improvement for the common use cases, making it a bit more straightforward. But I understand if you find the dynamic use cases more common than I do. Thanks for the feedback!
Fair points! Thank you for the feedback.
Closing as this is not as helpful as I initially thought.