This simplifies the shutdown code a bit and reduces the amount of extra logic required in kernel/bitcoinkernel.cpp to tear down the chainman.
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, stickies-v, mzumsande |
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.
🚧 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
maybe time to introduce a replace mempool method like Carl suggested there?
Did this now, ready for review.
311+ m_node.mempool.reset();
312+ bilingual_str error;
313+ m_node.mempool = std::make_unique<CTxMemPool>(MemPoolOptionsForTest(m_node), error);
314+ auto& dummy_chainstate{static_cast<DummyChainState&>(m_node.chainman->ActiveChainstate())};
315+ dummy_chainstate.SetMempool(m_node.mempool.get());
316+ Assert(error.empty());
310+ // instead.
311+ m_node.mempool.reset();
312+ bilingual_str error;
313+ m_node.mempool = std::make_unique<CTxMemPool>(MemPoolOptionsForTest(m_node), error);
314+ auto& dummy_chainstate{static_cast<DummyChainState&>(m_node.chainman->ActiveChainstate())};
315+ dummy_chainstate.SetMempool(m_node.mempool.get());
Chainstate::m_mempool member. It feels like a footgunny shortcut (but need to investigate more), conceptually doesn’t make a lot of sense to me, and not having it would avoid having to do e.g. theDummyChainState gymnastics. Is this something you’ve explored already?
352+ // if missing a callback results in an unrecoverable situation, unclean
353+ // shutdown would too. The only reason to do the above flushes is to let
354+ // the wallet and indexes catch up with our current chain to avoid any
355+ // strange pruning edge cases and make next startup faster by avoiding
356+ // rescan.
357+
nit: I would go further and merge it with the above docstring. IIUC correctly, they all talk about L349?
0diff --git a/src/init.cpp b/src/init.cpp
1index 695e365867..b558a1594d 100644
2--- a/src/init.cpp
3+++ b/src/init.cpp
4@@ -344,16 +344,17 @@ void Shutdown(NodeContext& node)
5 }
6 }
7
8- // After there are no more peers/RPC left to give us new data which may generate
9- // CValidationInterface callbacks, flush them...
10+ // After there are no more peers/RPC left to give us new data which
11+ // may generate CValidationInterface callbacks, flush them. Any
12+ // future callbacks will be dropped. This should absolutely be safe
13+ // - if missing a callback results in an unrecoverable situation,
14+ // unclean shutdown would too. The only reason to flush is to let
15+ // the wallet and indexes catch up with our current chain to avoid
16+ // any strange pruning edge cases and make next startup faster by
17+ // avoiding rescan.
18 if (node.validation_signals) node.validation_signals->FlushBackgroundCallbacks();
19
20- // Any future callbacks will be dropped. This should absolutely be safe -
21- // if missing a callback results in an unrecoverable situation, unclean
22- // shutdown would too. The only reason to do the above flushes is to let
23- // the wallet and indexes catch up with our current chain to avoid any
24- // strange pruning edge cases and make next startup faster by avoiding
25- // rescan.
26+
27
28 // Stop and delete all indexes only after flushing background callbacks.
29 for (auto* index : node.indexes) index->Stop();
385@@ -397,7 +386,11 @@ struct SnapshotTestSetup : TestChain100Setup {
386 // For robustness, ensure the old manager is destroyed before creating a
387 // new one.
388 m_node.chainman.reset();
389+ // Process all callbacks referring to the old manager before creating a
390+ // new one.
391+ m_node.validation_signals->SyncWithValidationInterfaceQueue();
m_node.chainman.reset();? I’m not sure if any of the callbacks actually depend on the chainman (it doesn’t look like it, but I’ll need to investigate more), but I think it’s a bit more logical at least?
The chainman reset may trigger validation interface notifications
Ah, right, ChainStateFlushed would be called. Yeah, I think with that knowledge doing it afterwards makes more sense. Also, callbacks are processed regardless of SyncWithValidationInterfaceQueue() being called, so I suppose this doesn’t really change anything wrt callbacks assuming m_node.chainman exists.
378- cs->ForceFlushStateToDisk();
379- }
380- // Process all callbacks referring to the old manager before wiping it.
381- m_node.validation_signals->SyncWithValidationInterfaceQueue();
382- LOCK(::cs_main);
383- chainman.ResetChainstates();
nit: the docstring is now a bit out of date:
0 // Simulate a restart of the node by flushing all state to disk, clearing the
1 // existing ChainstateManager, and unloading the block index.
Sorry, I meant to to remove the flushing reference since that’s now automatically handled by clearing chainman, so I agree with your comment.
Simulate a restart of the node by clearing the existing ChainstateManager, and unloading the block index.
Concept ACK
Simplifies the shutdown logic, and makes ChainstateManager more self contained. I’ll need to think a bit more about how to test or verify the safety of changes such as in 7a37f2008372c021541809d06d9300278fd5a8da.
Thank you for the review @stickies-v,
Updated 6c3bc478ca343557d798e2b18b5f99c08079ba9a -> 8e86e77311a2c76fd34b4481f83205b2432b151a (chainman_flush_destructor_0 -> chainman_flush_destructor_1, compare)
Assert checking that there is no mempool construction error to right after constructing the mempool.6284@@ -6285,6 +6285,12 @@ ChainstateManager::ChainstateManager(const util::SignalInterrupt& interrupt, Opt
6285 ChainstateManager::~ChainstateManager()
6286 {
6287 LOCK(::cs_main);
6288+ for (Chainstate* chainstate : GetAll()) {
6289+ if (chainstate->CanFlushToDisk()) {
6290+ chainstate->ForceFlushStateToDisk();
AFAIU the second flush is the original one, while the first flush was introduced in https://github.com/bitcoin/bitcoin/commit/3192975f1d177aa9f0bbd823c6387cfbfa943610 to help cleanly process all remaining callbacks. Maybe it could have removed the second flush in that commit?
That said I think it is good practice to be defensive here and keep both.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/35129562635
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.
Rebased 8e86e77311a2c76fd34b4481f83205b2432b151a -> fe7d09b5017b748fc71dab2a8bb186ff2c1624bb (chainman_flush_destructor_1 -> chainman_flush_destructor_2, compare)
Rebased fe7d09b5017b748fc71dab2a8bb186ff2c1624bb -> 1c299f70833188654b9cf3936d63d370856a408d (chainman_flush_destructor_2 -> chainman_flush_destructor_3, compare)
Rebased 1c299f70833188654b9cf3936d63d370856a408d -> e219f15976f50e0a703bc696b6445feb9950e65d (chainman_flush_destructor_3 -> chainman_flush_destructor_4, compare)
Rebased e219f15976f50e0a703bc696b6445feb9950e65d -> eec7c49cf777cdcc53aeb20e31fbd5881c2b008b (chainman_flush_destructor_4 -> chainman_flush_destructor_5, compare)
387@@ -388,9 +388,9 @@ void Shutdown(NodeContext& node)
388 if (node.validation_signals) {
389 node.validation_signals->UnregisterAllValidationInterfaces();
390 }
391+ node.chainman.reset();
In commit ef83e9569752b4273e6aed3124642dc057cab540 (shutdown: Tear down mempool after chainman)
Feel free to disregard as out of scope, but while addressing this, it might be nice to make reintroducing this dangling pointer impossible by making the various mempool pointers shared_ptr, e.g. https://github.com/davidgumberg/bitcoin/commit/9972413211be8992016ec039c03c4b32755704f5
Thanks for taking a look :)
I think the problem here is that we are moving the mempool pointer around between different objects, while they don’t really share ownership of it at any one time. It not having a value at some points in time does have a logical meaning in the current code. In general I don’t like shared pointers for a number of reasons. In this instance I agree that it would solve the problem of not knowing when to destruct the mempool. I am a bit apprehensive to commit this change here though, because I feel like the current design with a unique pointer and raw pointers signalling optional pointer values was done on purpose over a shared pointer design.
Rebased 6137cecfd3a448c2befe043849fce0b4ea2cd1fe -> 0adf8761c1f8860eb89f6162d7eec47f15a4c01b (chainman_flush_destructor_7 -> chainman_flush_destructor_8, compare)
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 code in the
destructor.
allows for getting rid of the
gotoinbitcoin-chainstate
I don’t think this is accurate?