This simplifies the shutdown code a bit and allows for getting rid of the goto in bitcoin-chainstate as well as making it a bit easier to use for future users of the kernel library.
Moving the second flush out of the Shutdown function into the ChainstateManager destructor, should be safe since there should be no new events created after the first flush. Tweak the chainman tests to actually exercise this new tear down behaviour.
This PR is part of the libbitcoinkernel project
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31382.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | l0rinc |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
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.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33601554977
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.
68- // things instantiated so far requires running the epilogue to be torn down
69- // properly
70- assert(kernel::SanityChecks(kernel_context));
71+ if (!kernel::SanityChecks(kernel_context)) {
72+ std::cerr << "Failed sanity check.";
73+ return 0;
cerr is used
Next to being more consistent, since tearing down the mempool first
leaves a dangling pointer in the chainman, this also enables the changes
in the next commits, since flushing calls GetCoinsCacheSizeState, which
would access this dangling pointer otherwise.
This is done in preparation for the next commit to also change the
mempool pointer within the chainstate once the old mempool is reset.
Without this the old, dangling pointer will be reused when destructing
the chainstate manager.
Moving the second flush out of the Shutdown function into the
ChainstateManager destructor, should be safe since there should be no
new events created after the first flush.
This simplifies the shutdown code a bit and allows for getting rid of
the `goto` in bitcoin-chainstate.
Also remove the reference to the destructed chainman, which could cause
UB once the chainman is reset.
Now that its only callsite has been moved to the destructor, let the
smart pointer do its thing.
There is no need to flush the background callbacks, since the immediate
task runner is used for the validation signals, which does not have any
pending callbacks. This allows for removal of the epilogue goto.
maybe time to introduce a replace mempool method like Carl suggested there?
Did this now, ready for review.
Labels
Validation