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.)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | Sjors, ismaelsadeeq, achow101 |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
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)
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 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);
In 0564f1e ipc mining: Prevent Assertion `m_node.chainman' failed errors on early startup: since this touches a different interface, it might be worth its own commit. If that's overkill, then just a comment in the commit message should be fine.
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);
In 0564f1e449e83ea875d64aff8f227fa1353a928d ipc mining: Prevent 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.
Thanks. I overlooked the move.
Concept ACK. Changes in bd9e0e65f561a5f846aa8f60648ff9600ec448ae mostly make sense to me. I also verified that the new test fails without the new code.
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.
Concept ACK
Thanks for the reviews!
<!-- begin push-2 -->
Updated 0564f1e449e83ea875d64aff8f227fa1353a928d -> bbc8f1e0a7e5739f15b2e646a4ace338083309a3 (pr/waitmine.1 -> pr/waitmine.2, compare)<!-- end --> implementing suggestions and also fixing uninitilized variable causing test failures https://github.com/bitcoin/bitcoin/actions/runs/22353321948/job/64685956677#step:11:4187
utACK bbc8f1e0a7e5739f15b2e646a4ace338083309a3
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
<details><summary>diff</summary> <p>
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1207,9 +1207,7 @@ bool AppInitLockDirectories()
bool AppInitInterfaces(NodeContext& node)
{
node.chain = interfaces::MakeChain(node);
- // Specify wait_loaded=false so internal mining interface can be initialized
- // on early startup and does not need to be tied to chainstate loading.
- node.mining = interfaces::MakeMining(node, /*wait_loaded=*/false);
+ node.mining = interfaces::MakeMining(node);
return true;
}
--- a/src/interfaces/mining.h
+++ b/src/interfaces/mining.h
@@ -160,11 +160,7 @@ public:
};
//! Return implementation of Mining interface.
-//!
-//! [@param](/bitcoin-bitcoin/contributor/param/)[in] wait_loaded waits for chainstate data to be loaded before
-//! returning. Used to prevent external clients from
-//! being able to crash the node during startup.
-std::unique_ptr<Mining> MakeMining(node::NodeContext& node, bool wait_loaded=true);
+std::unique_ptr<Mining> MakeMining(node::NodeContext& node);
} // namespace interfaces
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -939,22 +939,24 @@ public:
bool isTestChain() override
{
- return chainman().GetParams().IsTestChain();
+ return Params().IsTestChain();
}
bool isInitialBlockDownload() override
{
- return chainman().IsInitialBlockDownload();
+ ChainstateManager* chainman{this->chainman()};
+ return !chainman || chainman->IsInitialBlockDownload();
}
std::optional<BlockRef> getTip() override
{
- return GetTip(chainman());
+ ChainstateManager* chainman{this->chainman()};
+ return chainman ? GetTip(*chainman) : std::nullopt;
}
std::optional<BlockRef> waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override
{
- return WaitTipChanged(chainman(), notifications(), current_tip, timeout, m_interrupt_mining);
+ return WaitTipChanged(notifications(), m_node.chainman, *Assert(m_node.shutdown_signal), current_tip, timeout, m_interrupt_mining);
}
std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options, bool cooldown) override
@@ -971,6 +973,7 @@ public:
// Ensure m_tip_block is set so consumers of BlockTemplate can rely on that.
std::optional<BlockRef> maybe_tip{waitTipChanged(uint256::ZERO, MillisecondsDouble::max())};
+ ChainstateManager& chainman{*Assert(this->chainman())};
if (!maybe_tip) return {};
@@ -980,18 +983,18 @@ public:
// is useful in general, it's gated behind the cooldown argument,
// because on regtest and single miner signets this would wait
// forever if no block was mined in the past day.
- while (chainman().IsInitialBlockDownload()) {
+ while (chainman.IsInitialBlockDownload()) {
maybe_tip = waitTipChanged(maybe_tip->hash, MillisecondsDouble{1000});
- if (!maybe_tip || chainman().m_interrupt || WITH_LOCK(notifications().m_tip_block_mutex, return m_interrupt_mining)) return {};
+ if (!maybe_tip || chainman.m_interrupt || WITH_LOCK(notifications().m_tip_block_mutex, return m_interrupt_mining)) return {};
}
// Also wait during the final catch-up moments after IBD.
- if (!CooldownIfHeadersAhead(chainman(), notifications(), *maybe_tip, m_interrupt_mining)) return {};
+ if (!CooldownIfHeadersAhead(chainman, notifications(), *maybe_tip, m_interrupt_mining)) return {};
}
BlockAssembler::Options assemble_options{options};
ApplyArgsManOptions(*Assert(m_node.args), assemble_options);
- return std::make_unique<BlockTemplateImpl>(assemble_options, BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(), m_node);
+ return std::make_unique<BlockTemplateImpl>(assemble_options, BlockAssembler{chainman.ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(), m_node);
}
void interrupt() override
@@ -1001,16 +1004,26 @@ public:
bool checkBlock(const CBlock& block, const node::BlockCheckOptions& options, std::string& reason, std::string& debug) override
{
- LOCK(chainman().GetMutex());
- BlockValidationState state{TestBlockValidity(chainman().ActiveChainstate(), block, /*check_pow=*/options.check_pow, /*check_merkle_root=*/options.check_merkle_root)};
+ ChainstateManager* chainman{this->chainman()};
+ if (!chainman) {
+ reason = debug = "Chainstate not loaded.";
+ return false;
+ }
+ LOCK(chainman->GetMutex());
+ BlockValidationState state{TestBlockValidity(chainman->ActiveChainstate(), block, /*check_pow=*/options.check_pow, /*check_merkle_root=*/options.check_merkle_root)};
reason = state.GetRejectReason();
debug = state.GetDebugMessage();
return state.IsValid();
}
NodeContext* context() override { return &m_node; }
- ChainstateManager& chainman() { return *Assert(m_node.chainman); }
KernelNotifications& notifications() { return *Assert(m_node.notifications); }
+ ChainstateManager* chainman(bool wait=false)
+ {
+ LOCK(notifications().m_tip_block_mutex);
+ return notifications().m_state.chainstate_loaded ? m_node.chainman.get() : nullptr;
+ }
+
// Treat as if guarded by notifications().m_tip_block_mutex
bool m_interrupt_mining{false};
NodeContext& m_node;
@@ -1021,17 +1034,5 @@ public:
namespace interfaces {
std::unique_ptr<Node> MakeNode(node::NodeContext& context) { return std::make_unique<node::NodeImpl>(context); }
std::unique_ptr<Chain> MakeChain(node::NodeContext& context) { return std::make_unique<node::ChainImpl>(context); }
-std::unique_ptr<Mining> MakeMining(node::NodeContext& context, bool wait_loaded)
-{
- if (wait_loaded) {
- node::KernelNotifications& kernel_notifications(*Assert(context.notifications));
- util::SignalInterrupt& interrupt(*Assert(context.shutdown_signal));
- WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock);
- kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
- return kernel_notifications.m_state.chainstate_loaded || interrupt;
- });
- if (interrupt) return nullptr;
- }
- return std::make_unique<node::MinerImpl>(context);
-}
+std::unique_ptr<Mining> MakeMining(node::NodeContext& context) { return std::make_unique<node::MinerImpl>(context); }
} // namespace interfaces
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -489,7 +489,11 @@ bool CooldownIfHeadersAhead(ChainstateManager& chainman, KernelNotifications& ke
return true;
}
-std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout, bool& interrupt)
+std::optional<BlockRef> WaitTipChanged(
+ KernelNotifications& kernel_notifications,
+ const std::unique_ptr<ChainstateManager>& chainman,
+ const util::SignalInterrupt& shutdown_signal,
+ const uint256& current_tip, MillisecondsDouble& timeout, bool& interrupt)
{
Assume(timeout >= 0ms); // No internal callers should use a negative timeout
if (timeout < 0ms) timeout = 0ms;
@@ -502,18 +506,21 @@ std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifi
// always returns valid tip information when possible and only
// returns null when shutting down, not when timing out.
kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
- return kernel_notifications.TipBlock() || chainman.m_interrupt || interrupt;
+ return (kernel_notifications.m_state.chainstate_loaded && kernel_notifications.TipBlock()) || shutdown_signal || interrupt;
});
- if (chainman.m_interrupt || interrupt) {
+
+ if (shutdown_signal || interrupt) {
interrupt = false;
return {};
}
+ Assert(chainman);
+ Assert(&chainman->m_interrupt == &shutdown_signal);
// At this point TipBlock is set, so continue to wait until it is
// different then `current_tip` provided by caller.
kernel_notifications.m_tip_block_cv.wait_until(lock, deadline, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
- return Assume(kernel_notifications.TipBlock()) != current_tip || chainman.m_interrupt || interrupt;
+ return Assume(kernel_notifications.TipBlock()) != current_tip || chainman->m_interrupt || interrupt;
});
- if (chainman.m_interrupt || interrupt) {
+ if (chainman->m_interrupt || interrupt) {
interrupt = false;
return {};
}
@@ -521,7 +528,7 @@ std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifi
// Must release m_tip_block_mutex before getTip() locks cs_main, to
// avoid deadlocks.
- return GetTip(chainman);
+ return GetTip(*chainman);
}
} // namespace node
--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -30,6 +30,10 @@ class CScript;
class Chainstate;
class ChainstateManager;
+namespace util {
+class SignalInterrupt;
+} // namespace util
+
namespace Consensus { struct Params; };
using interfaces::BlockRef;
@@ -162,7 +166,13 @@ std::optional<BlockRef> GetTip(ChainstateManager& chainman);
* Returns the current tip, or nullopt if the node is shutting down or interrupt()
* is called.
*/
-std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout, bool& interrupt);
+std::optional<BlockRef> WaitTipChanged(
+ KernelNotifications& kernel_notifications,
+ const std::unique_ptr<ChainstateManager>& chainman_ptr,
+ const util::SignalInterrupt& shutdown_signal,
+ const uint256& current_tip,
+ MillisecondsDouble& timeout,
+ bool& interrupt);
/**
* Wait while the best known header extends the current chain tip AND at least
--- a/src/test/miner_tests.cpp
+++ b/src/test/miner_tests.cpp
@@ -68,7 +68,7 @@ struct MinerTestingSetup : public TestingSetup {
}
std::unique_ptr<Mining> MakeMining()
{
- return interfaces::MakeMining(m_node, /*wait_loaded=*/false);
+ return interfaces::MakeMining(m_node);
}
};
} // namespace miner_tests
--- a/src/test/testnet4_miner_tests.cpp
+++ b/src/test/testnet4_miner_tests.cpp
@@ -22,7 +22,7 @@ namespace testnet4_miner_tests {
struct Testnet4MinerTestingSetup : public Testnet4Setup {
std::unique_ptr<Mining> MakeMining()
{
- return interfaces::MakeMining(m_node, /*wait_loaded=*/false);
+ return interfaces::MakeMining(m_node);
}
};
} // namespace testnet4_miner_tests
</p> </details>
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?
Should this be in 31.0?
ACK bbc8f1e0a7e5739f15b2e646a4ace338083309a3
Should this be in 31.0?
Hopefully yes.
ACK bbc8f1e0a7e5739f15b2e646a4ace338083309a3