kernel: Rename in-memory DB option setters and simplify API

[stringintech]

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 of in-memory options to false in ChainstateManagerOptions constructor, should remove ambiguity about what happens when these functions are not called.

[DrahtBot]

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/33877.

Reviews

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #32427 ((RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store by TheCharlatan)

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]

🚧 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.}

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.

[stringintech]

This occurred to me while refactoring the context and chainstate manager options in the Go wrapper, so I opened this PR to get feedback.

cc @TheCharlatan @stickies-v

[yuvicc]

Concept ACK

Makes sense to refractor update to set and remove the int parameter here.

[yuvicc]

I think these options are set by default?

[stringintech]

Yes they are. No strong opinion, but it might be better to also explicitly set the default values here (in the kernel code - 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.

[stickies-v]

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:

git diff on 3ac69a6

diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index df0c8ca1b5..3ebec5b59b 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/kernel/test_kernel.cpp
@@ -653,12 +653,8 @@ std::unique_ptr<ChainMan> create_chainman(TestDirectory& test_directory,
     if (wipe_chainstate) {
         chainman_opts.SetWipeDbs(/*wipe_block_tree=*/false, /*wipe_chainstate=*/wipe_chainstate);
     }
-    if (block_tree_db_in_memory) {
-        chainman_opts.SetBlockTreeDbInMemory();
-    }
-    if (chainstate_db_in_memory) {
-        chainman_opts.SetChainstateDbInMemory();
-    }
+    chainman_opts.UpdateBlockTreeDbInMemory(block_tree_db_in_memory);
+    chainman_opts.UpdateChainstateDbInMemory(chainstate_db_in_memory);
 
     auto chainman{std::make_unique<ChainMan>(context, chainman_opts)};
     return chainman;

Do you have an example of how this makes things better for you?

[stringintech]

Good point! I understand your rationale, but wouldn't the cases you're describing be mostly for debugging/testing purposes?
It seems to me that for typical use cases, we would know whether to enable in-memory mode at compile time, not something that is derived dynamically, so it feels more natural/clear to simply call a function that does that with no input parameter.
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.

[stickies-v]

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?

[stringintech]

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!

[sedited]

I tend to agree with stickies-v here. I think when it comes to setting up these options structs we should support dynamic settings. In my view that is the point of them. You get this "toggle" behaviour through the options structs, but not when passing arguments when creating the actual underlying object.

[stringintech]

Fair points! Thank you for the feedback.

Closing as this is not as helpful as I initially thought.