kernel: Flush in ChainstateManager destructor #31382

nit: this being a console app, it might make sense to return a non-zero error code when cerr is used

Yeah, done.

Shouldn't this be checked right after making the mempool?

Yeah, that makes more sense.

I wonder if it would make sense to (probably in a separate PR, I suspect it'll be quite a big change) remove the 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?

I could not come up with a satisfactory alternative yet, suggestions welcome!

nit: I would go further and merge it with the above docstring. IIUC correctly, they all talk about L349?

<details> <summary>git diff on 7a37f20083</summary>

diff --git a/src/init.cpp b/src/init.cpp
index 695e365867..b558a1594d 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -344,16 +344,17 @@ void Shutdown(NodeContext& node)
         }
     }
 
-    // After there are no more peers/RPC left to give us new data which may generate
-    // CValidationInterface callbacks, flush them...
+    // After there are no more peers/RPC left to give us new data which
+    // may generate CValidationInterface callbacks, flush them. Any
+    // future callbacks will be dropped. This should absolutely be safe
+    // - if missing a callback results in an unrecoverable situation,
+    // unclean shutdown would too. The only reason to flush is to let
+    // the wallet and indexes catch up with our current chain to avoid
+    // any strange pruning edge cases and make next startup faster by
+    // avoiding rescan.
     if (node.validation_signals) node.validation_signals->FlushBackgroundCallbacks();
 
-    // Any future callbacks will be dropped. This should absolutely be safe -
-    // if missing a callback results in an unrecoverable situation, unclean
-    // shutdown would too. The only reason to do the above flushes is to let
-    // the wallet and indexes catch up with our current chain to avoid any
-    // strange pruning edge cases and make next startup faster by avoiding
-    // rescan.
+
 
     // Stop and delete all indexes only after flushing background callbacks.
     for (auto* index : node.indexes) index->Stop();

</details>

Should we do this before calling 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, so I think it makes sense to sync with the queue afterwards? Unlike the shutdown procedure, we keep the validation interface around beyond the restarts there.

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.

nit: the docstring is now a bit out of date:

    // Simulate a restart of the node by flushing all state to disk, clearing the
    // existing ChainstateManager, and unloading the block index.

This seems fine as is? I'd prefer not to document these deeper behaviours, that the caller can expect to "just work".

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.

I'll do that next time I have to push.

Not too familiar with the shutdown sequence - I don't fully understand why we need to flush the chainstates twice. Do you know any examples of events that result in the second flush on destruction doing any actual work in bitcoind?

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.

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.