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?
re: #34661 (review)
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?
IMO, the wait_loaded option is a hack, and there should be no need for chain and block data to be loaded to get an pointer to the mining interface.
I think a goal for the mining interface should be to be self-contained and not require to miners to call other interfaces to connect and get startup information, and the wait_loaded=true behavior makes it not possible to add mining methods that return startup progress. Having the wait_loaded option also makes the mining interface different from other interfaces internally. (Dropping the option and always using wait_loaded=true behavior would not really solve this problem either since it would require moving the MakeMining call in init code,)
So your comments made me think it would be better to drop wait_loaded and update mining interface methods to not crash during startup instead. This was not as difficult as I thought and the change is shown in the diff below. I am thinking about whether to make it a PR and curious if it seems reasonable and addresses your concerns
0--- a/src/init.cpp
1+++ b/src/init.cpp
2@@ -1207,9 +1207,7 @@ bool AppInitLockDirectories()
3 bool AppInitInterfaces(NodeContext& node)
4 {
5 node.chain = interfaces::MakeChain(node);
6- // Specify wait_loaded=false so internal mining interface can be initialized
7- // on early startup and does not need to be tied to chainstate loading.
8- node.mining = interfaces::MakeMining(node, /*wait_loaded=*/false);
9+ node.mining = interfaces::MakeMining(node);
10 return true;
11 }
12
13--- a/src/interfaces/mining.h
14+++ b/src/interfaces/mining.h
15@@ -160,11 +160,7 @@ public:
16 };
17
18 //! Return implementation of Mining interface.
19-//!
20-//! [@param](/bitcoin-bitcoin/contributor/param/)[in] wait_loaded waits for chainstate data to be loaded before
21-//! returning. Used to prevent external clients from
22-//! being able to crash the node during startup.
23-std::unique_ptr<Mining> MakeMining(node::NodeContext& node, bool wait_loaded=true);
24+std::unique_ptr<Mining> MakeMining(node::NodeContext& node);
25
26 } // namespace interfaces
27
28--- a/src/node/interfaces.cpp
29+++ b/src/node/interfaces.cpp
30@@ -939,22 +939,24 @@ public:
31
32 bool isTestChain() override
33 {
34- return chainman().GetParams().IsTestChain();
35+ return Params().IsTestChain();
36 }
37
38 bool isInitialBlockDownload() override
39 {
40- return chainman().IsInitialBlockDownload();
41+ ChainstateManager* chainman{this->chainman()};
42+ return !chainman || chainman->IsInitialBlockDownload();
43 }
44
45 std::optional<BlockRef> getTip() override
46 {
47- return GetTip(chainman());
48+ ChainstateManager* chainman{this->chainman()};
49+ return chainman ? GetTip(*chainman) : std::nullopt;
50 }
51
52 std::optional<BlockRef> waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override
53 {
54- return WaitTipChanged(chainman(), notifications(), current_tip, timeout, m_interrupt_mining);
55+ return WaitTipChanged(notifications(), m_node.chainman, *Assert(m_node.shutdown_signal), current_tip, timeout, m_interrupt_mining);
56 }
57
58 std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options, bool cooldown) override
59@@ -971,6 +973,7 @@ public:
60
61 // Ensure m_tip_block is set so consumers of BlockTemplate can rely on that.
62 std::optional<BlockRef> maybe_tip{waitTipChanged(uint256::ZERO, MillisecondsDouble::max())};
63+ ChainstateManager& chainman{*Assert(this->chainman())};
64
65 if (!maybe_tip) return {};
66
67@@ -980,18 +983,18 @@ public:
68 // is useful in general, it's gated behind the cooldown argument,
69 // because on regtest and single miner signets this would wait
70 // forever if no block was mined in the past day.
71- while (chainman().IsInitialBlockDownload()) {
72+ while (chainman.IsInitialBlockDownload()) {
73 maybe_tip = waitTipChanged(maybe_tip->hash, MillisecondsDouble{1000});
74- if (!maybe_tip || chainman().m_interrupt || WITH_LOCK(notifications().m_tip_block_mutex, return m_interrupt_mining)) return {};
75+ if (!maybe_tip || chainman.m_interrupt || WITH_LOCK(notifications().m_tip_block_mutex, return m_interrupt_mining)) return {};
76 }
77
78 // Also wait during the final catch-up moments after IBD.
79- if (!CooldownIfHeadersAhead(chainman(), notifications(), *maybe_tip, m_interrupt_mining)) return {};
80+ if (!CooldownIfHeadersAhead(chainman, notifications(), *maybe_tip, m_interrupt_mining)) return {};
81 }
82
83 BlockAssembler::Options assemble_options{options};
84 ApplyArgsManOptions(*Assert(m_node.args), assemble_options);
85- return std::make_unique<BlockTemplateImpl>(assemble_options, BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(), m_node);
86+ return std::make_unique<BlockTemplateImpl>(assemble_options, BlockAssembler{chainman.ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(), m_node);
87 }
88
89 void interrupt() override
90@@ -1001,16 +1004,26 @@ public:
91
92 bool checkBlock(const CBlock& block, const node::BlockCheckOptions& options, std::string& reason, std::string& debug) override
93 {
94- LOCK(chainman().GetMutex());
95- BlockValidationState state{TestBlockValidity(chainman().ActiveChainstate(), block, /*check_pow=*/options.check_pow, /*check_merkle_root=*/options.check_merkle_root)};
96+ ChainstateManager* chainman{this->chainman()};
97+ if (!chainman) {
98+ reason = debug = "Chainstate not loaded.";
99+ return false;
100+ }
101+ LOCK(chainman->GetMutex());
102+ BlockValidationState state{TestBlockValidity(chainman->ActiveChainstate(), block, /*check_pow=*/options.check_pow, /*check_merkle_root=*/options.check_merkle_root)};
103 reason = state.GetRejectReason();
104 debug = state.GetDebugMessage();
105 return state.IsValid();
106 }
107
108 NodeContext* context() override { return &m_node; }
109- ChainstateManager& chainman() { return *Assert(m_node.chainman); }
110 KernelNotifications& notifications() { return *Assert(m_node.notifications); }
111+ ChainstateManager* chainman(bool wait=false)
112+ {
113+ LOCK(notifications().m_tip_block_mutex);
114+ return notifications().m_state.chainstate_loaded ? m_node.chainman.get() : nullptr;
115+ }
116+
117 // Treat as if guarded by notifications().m_tip_block_mutex
118 bool m_interrupt_mining{false};
119 NodeContext& m_node;
120@@ -1021,17 +1034,5 @@ public:
121 namespace interfaces {
122 std::unique_ptr<Node> MakeNode(node::NodeContext& context) { return std::make_unique<node::NodeImpl>(context); }
123 std::unique_ptr<Chain> MakeChain(node::NodeContext& context) { return std::make_unique<node::ChainImpl>(context); }
124-std::unique_ptr<Mining> MakeMining(node::NodeContext& context, bool wait_loaded)
125-{
126- if (wait_loaded) {
127- node::KernelNotifications& kernel_notifications(*Assert(context.notifications));
128- util::SignalInterrupt& interrupt(*Assert(context.shutdown_signal));
129- WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock);
130- kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
131- return kernel_notifications.m_state.chainstate_loaded || interrupt;
132- });
133- if (interrupt) return nullptr;
134- }
135- return std::make_unique<node::MinerImpl>(context);
136-}
137+std::unique_ptr<Mining> MakeMining(node::NodeContext& context) { return std::make_unique<node::MinerImpl>(context); }
138 } // namespace interfaces
139--- a/src/node/miner.cpp
140+++ b/src/node/miner.cpp
141@@ -489,7 +489,11 @@ bool CooldownIfHeadersAhead(ChainstateManager& chainman, KernelNotifications& ke
142 return true;
143 }
144
145-std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout, bool& interrupt)
146+std::optional<BlockRef> WaitTipChanged(
147+ KernelNotifications& kernel_notifications,
148+ const std::unique_ptr<ChainstateManager>& chainman,
149+ const util::SignalInterrupt& shutdown_signal,
150+ const uint256& current_tip, MillisecondsDouble& timeout, bool& interrupt)
151 {
152 Assume(timeout >= 0ms); // No internal callers should use a negative timeout
153 if (timeout < 0ms) timeout = 0ms;
154@@ -502,18 +506,21 @@ std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifi
155 // always returns valid tip information when possible and only
156 // returns null when shutting down, not when timing out.
157 kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
158- return kernel_notifications.TipBlock() || chainman.m_interrupt || interrupt;
159+ return (kernel_notifications.m_state.chainstate_loaded && kernel_notifications.TipBlock()) || shutdown_signal || interrupt;
160 });
161- if (chainman.m_interrupt || interrupt) {
162+
163+ if (shutdown_signal || interrupt) {
164 interrupt = false;
165 return {};
166 }
167+ Assert(chainman);
168+ Assert(&chainman->m_interrupt == &shutdown_signal);
169 // At this point TipBlock is set, so continue to wait until it is
170 // different then `current_tip` provided by caller.
171 kernel_notifications.m_tip_block_cv.wait_until(lock, deadline, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
172- return Assume(kernel_notifications.TipBlock()) != current_tip || chainman.m_interrupt || interrupt;
173+ return Assume(kernel_notifications.TipBlock()) != current_tip || chainman->m_interrupt || interrupt;
174 });
175- if (chainman.m_interrupt || interrupt) {
176+ if (chainman->m_interrupt || interrupt) {
177 interrupt = false;
178 return {};
179 }
180@@ -521,7 +528,7 @@ std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifi
181
182 // Must release m_tip_block_mutex before getTip() locks cs_main, to
183 // avoid deadlocks.
184- return GetTip(chainman);
185+ return GetTip(*chainman);
186 }
187
188 } // namespace node
189--- a/src/node/miner.h
190+++ b/src/node/miner.h
191@@ -30,6 +30,10 @@ class CScript;
192 class Chainstate;
193 class ChainstateManager;
194
195+namespace util {
196+class SignalInterrupt;
197+} // namespace util
198+
199 namespace Consensus { struct Params; };
200
201 using interfaces::BlockRef;
202@@ -162,7 +166,13 @@ std::optional<BlockRef> GetTip(ChainstateManager& chainman);
203 * Returns the current tip, or nullopt if the node is shutting down or interrupt()
204 * is called.
205 */
206-std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout, bool& interrupt);
207+std::optional<BlockRef> WaitTipChanged(
208+ KernelNotifications& kernel_notifications,
209+ const std::unique_ptr<ChainstateManager>& chainman_ptr,
210+ const util::SignalInterrupt& shutdown_signal,
211+ const uint256& current_tip,
212+ MillisecondsDouble& timeout,
213+ bool& interrupt);
214
215 /**
216 * Wait while the best known header extends the current chain tip AND at least
217--- a/src/test/miner_tests.cpp
218+++ b/src/test/miner_tests.cpp
219@@ -68,7 +68,7 @@ struct MinerTestingSetup : public TestingSetup {
220 }
221 std::unique_ptr<Mining> MakeMining()
222 {
223- return interfaces::MakeMining(m_node, /*wait_loaded=*/false);
224+ return interfaces::MakeMining(m_node);
225 }
226 };
227 } // namespace miner_tests
228--- a/src/test/testnet4_miner_tests.cpp
229+++ b/src/test/testnet4_miner_tests.cpp
230@@ -22,7 +22,7 @@ namespace testnet4_miner_tests {
231 struct Testnet4MinerTestingSetup : public Testnet4Setup {
232 std::unique_ptr<Mining> MakeMining()
233 {
234- return interfaces::MakeMining(m_node, /*wait_loaded=*/false);
235+ return interfaces::MakeMining(m_node);
236 }
237 };
238 } // namespace testnet4_miner_tests
This indeed doesn’t look too bad.
nit: for checkBlock maybe use retry-later as the error, that’s more in the bip22 style: https://en.bitcoin.it/wiki/BIP_0022
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.