Assertion `m_node.chainman' failed errors on early startup
#34661
This fixes Assertion `m_node.chainman' failed errors first reported #33994 (comment) when IPC mining methods are called before ChainstateManager is loaded.
The fix works by making the Init.makeMining method wait until chainstate data is loaded. It’s probably the simplest possible fix but other alternatives like moving the wait to Mining.createNewBlock were discussed in the thread #34661 (review) and could be implemented later without changes to clients.
There's no change in behavior. This is just a refactoring to avoid a minor
layer violation in init code. The node.init object is intended to return
interface pointers for code outside the node (like wallet and gui code), not
used by node itself to initialize its internal state.
(Motivation for this change is to introduce a MakeMining wait_loaded option in
an upcoming commit that can only be used internally and not set by external
clients.)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | Sjors, ismaelsadeeq, achow101 |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
1205@@ -1206,8 +1206,8 @@ bool AppInitLockDirectories()
1206
1207 bool AppInitInterfaces(NodeContext& node)
1208 {
1209- node.chain = node.init->makeChain();
1210- node.mining = node.init->makeMining();
1211+ node.chain = interfaces::MakeChain(node);
1212+ node.mining = interfaces::MakeMining(node, /*wait_loaded=*/false);
In 0564f1e449e83ea875d64aff8f227fa1353a928d ipc mining: Prevent Assertion `m_node.chainman' failed errors on early startup: would be good to document why this needs to opt out.
Or perhaps better would be to expand the documentation of MakeMining, and also mention there that it always applies to IPC clients, since this isn’t exposed in init.capnp.
re: #34661 (review)
would be good to document why this needs to opt out.
Thanks, added comments both places
The comment in bbc8f1e0a7e5739f15b2e646a4ace338083309a3 ipc mining: Prevent Assertion `m_node.chainman' failed errors on early startup is better. But it doesn’t explain why it’s safe for internal consumers.
For RPC IIRC all methods are blocked until we’ve made some (enough?) progress in initialization. The new BlockTemplateCache in #33421 should be fine initially, because it doesn’t introduce new accessors. Once it’s expanded to fee estimation, that code needs to be careful though?
re: #34661 (review)
The comment in bbc8f1e ipc mining: Prevent
Assertion `m_node.chainman' failederrors on early startup is better. But it doesn’t explain why it’s safe for internal consumers.For RPC IIRC all methods are blocked until we’ve made some (enough?) progress in initialization. The new
BlockTemplateCachein #33421 should be fine initially, because it doesn’t introduce new accessors. Once it’s expanded to fee estimation, that code needs to be careful though?
I guess I don’t really see it as a goal to for internal interface pointers to be safe to use at all time. I think basically all node code including RPC methods need to be aware of initialization and shutdown and check for null objects, check for interrupts appropriately, and only external clients shouldn’t need to do this.
From my perspective even making the makeMining method blocking for external clients is potentially too restrictive and was not my first choice. My initial attempt at fixing this crash was to leave makeMining alone and update all the mining interface methods individually to handle the chainstate not being loaded yet, and return false, or null, or wait inside as appropriate inside each method. However, implementing this turned into a bigger change (especially trying to integrate the chainstate wait with mining methods that were already waiting), so I decided to take the simpler approach of leaving all the methods alone except Init.makeMining.
I still think we may want to go back a version of the other approach, if for example we want to give mining clients the ability to show progress messages on node startup or disconnect cleanly while chain data is being loaded.
Maybe add an assert m_state.chainstate_loaded inside or right before SetRPCWarmupFinished?
(missed your earlier reply)
Init or some other interface?
I think it’s fine to make mining interface clients wait. If we want to give a way for IPC clients to get node warmup info, before we have a chainstate, maybe expose that on the
Initor some other interface?
To be clear, I also think it’s ok to make mining interface clients wait, but I don’t think it’s ideal that there’s no timeout for the wait, no way to cancel it, no way to get current status. This could be addressed by making makeMining nonblocking again in the future or adding new Init methods as you suggest.
I also don’t think it’s ideal to have to wait before calling methods like Mining.isInitialBlockDownload which could return immediately, or to need to call a waiting method like makeMining just to call another waiting method like createNewBlock. It’s all fine, just not as transparent or flexible as I’d want ideally, so I think beyond fixing the crash, other improvements could be made here later, without requiring changes to clients.
Another approach could be to have a blocking ready() method on the mining interface, which internally just does what wait_loaded does now, but also toggles an m_ready. All the other methods then throw an error if they’re called before m_ready is true.
But forcing clients to check ready() is a bit annoying.
But forcing clients to check
ready()is a bit annoying.
Yep, adding a separate method instead of building this in to createNewBlock would probably something to avoid.
I think it’s nicest if most methods just return whatever information they have quickly, not blocking unnecessarily (or crashing, obviously). I think there should only be a few dedicated methods which do block (createNewBlock, waitTipChanged, waitNext) and these should support timeouts and cancellations.
So IMO having the makeMining method block is not ideal. But it is probably the simplest way of preventing current crashes, and it’s backwards & forwards compatible so it should be possible to change this behavior in the future without clients needing to do anything.
1205@@ -1206,8 +1206,8 @@ bool AppInitLockDirectories()
1206
1207 bool AppInitInterfaces(NodeContext& node)
1208 {
1209- node.chain = node.init->makeChain();
1210- node.mining = node.init->makeMining();
1211+ node.chain = interfaces::MakeChain(node);
re: #34661 (review)
since this touches a different interface, it might be worth its own commit.
Makes sense, added separate commit
1482@@ -1486,6 +1483,10 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1483 node.validation_signals = std::make_unique<ValidationSignals>(std::make_unique<SerialTaskRunner>(scheduler));
1484 auto& validation_signals = *node.validation_signals;
1485
1486+ assert(!node.notifications);
Assertion `m_node.chainman' failed errors on early startup: let’s add a note that it’s important to do this before any IPC interface listens, since the wait_loaded mechanism relies on kernel notifications.
re: #34661 (review)
let’s add a note that it’s important to do this before any IPC interface listens
Thanks, added comment
1303@@ -1304,16 +1304,12 @@ static ChainstateLoadResult InitAndLoadChainstate(
1304 const ArgsManager& args)
1305 {
1306 // This function may be called twice, so any dirty state must be reset.
1307- node.notifications.reset(); // Drop state, such as a cached tip block
1308+ node.notifications->setChainstateLoaded(false); // Drop state, such as a cached tip block
In 0564f1e449e83ea875d64aff8f227fa1353a928d ipc mining: Prevent Assertion `m_node.chainman' failed errors on early startup: it seems a bit of an indirection to have setChainstateLoaded be responsible for “any dirty state must be reset”.
In practice it’s trivial, for now.
I think it would be good to have a prep commit that introduces setChainstateLoaded (or a a reset helper function). Its commit message could explain why e.g. it’s safe to drop ReadNotificationArgs below.
re: #34661 (review)
it seems a bit of an indirection to have
setChainstateLoadedbe responsible for “any dirty state must be reset”.
Thanks, I moved the state into a separate struct to clarify and avoid need to reset each piece of state individually. Hopefully that helps and addresses the concern. I was also thinking of renaming the method, but wasn’t sure about that.
I think it would be good to have a prep commit that introduces
setChainstateLoaded(or a a reset helper function). Its commit message could explain why e.g. it’s safe to dropReadNotificationArgsbelow.
Hopefully it’s clear ReadNotificationArgs call is only moving not deleted, but happy to make another update if it’s not or if something else is off here.
Instead of having the InitAndLoadChainstate function delete and create the
KernelNotifications object each time it is called (it can be called twice when
reindexing) to clear cached state, create it just one time and add a
setChainstateLoaded() method to manage state as it is loaded and unloaded.
This refactoring should make sense by itself to be more explicit about how
KernelNotifications state is cleared, but it's also needed to make outside code
accessing KernelNotifications state (currently just mining code) safe during
node startup and shutdown so the KernelNofications mutex can be used for
synchronization and does not get recreated itself.
This fixes ``Assertion `m_node.chainman' failed`` errors first reported
https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602551596 when
IPC mining methods are called before ChainstateManager is loaded.
The fix works by making the `Init.makeMining` method block until chainstate
data is loaded.
Thanks for the reviews!
Updated 0564f1e449e83ea875d64aff8f227fa1353a928d -> bbc8f1e0a7e5739f15b2e646a4ace338083309a3 (pr/waitmine.1 -> pr/waitmine.2, compare) implementing suggestions and also fixing uninitilized variable causing test failures https://github.com/bitcoin/bitcoin/actions/runs/22353321948/job/64685956677#step:11:4187
159@@ -160,7 +160,11 @@ class Mining
160 };
161
162 //! Return implementation of Mining interface.
163-std::unique_ptr<Mining> MakeMining(node::NodeContext& node);
164+//!
165+//! @param[in] wait_loaded waits for chainstate data to be loaded before
166+//! returning. Used to prevent external clients from
167+//! being able to crash the node during startup.
168+std::unique_ptr<Mining> MakeMining(node::NodeContext& node, bool wait_loaded=true);
In “ipc mining: Prevent Assertion `m_node.chainman' failed errors on early startup”
Why are we enabling the creation of mining without chainstate being loaded, is it necessary? perhaps just delete the flag and make this standard behavior?
28@@ -29,6 +29,18 @@ namespace node {
29 class Warnings;
30 static constexpr int DEFAULT_STOPATHEIGHT{0};
31
32+//! State tracked by the KernelNotifications interface meant to be used by
33+//! mining code, index code, RPCs, and other code sitting above the validation
In “init refactor: Only initialize node.notifications one time” a7cabf92e4de83c87f6b9274ddd2fb70712d29f8
Comments that explicitly mention users here can easily go stale.
I’d rather we just say “meant to be used code sitting above the validation layer”
1022+{
1023+ if (wait_loaded) {
1024+ node::KernelNotifications& kernel_notifications(*Assert(context.notifications));
1025+ util::SignalInterrupt& interrupt(*Assert(context.shutdown_signal));
1026+ WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock);
1027+ kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
In “ipc mining: Prevent Assertion `m_node.chainman' failed errors on early startup” bbc8f1e0a7e5739f15b2e646a4ace338083309a3
It makes sense to wait on m_tip_block_cv here because we are certain that chainstate loading will notify this conditional variable first before blockTip does. Maybe add a comment?
1206@@ -1207,7 +1207,9 @@ bool AppInitLockDirectories()
1207 bool AppInitInterfaces(NodeContext& node)
1208 {
1209 node.chain = interfaces::MakeChain(node);
1210- node.mining = interfaces::MakeMining(node);
1211+ // Specify wait_loaded=false so internal mining interface can be initialized
1212+ // on early startup and does not need to be tied to chainstate loading.
In “ipc mining: Prevent Assertion `m_node.chainman' failed errors on early startup” bbc8f1e0a7e5739f15b2e646a4ace338083309a3
why should the internal mining interface not be tied to chainstate loading?
ACK bbc8f1e0a7e5739f15b2e646a4ace338083309a3
Should this be in 31.0?
Hopefully yes.