Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods

[Sjors]

This PR prevents Mining interface methods from sometimes crashing when called during startup before a tip is connected. It also makes other improvements like making more RPC methods usable from the GUI. Specifically this PR:

• Adds an Assume check to disallow passing negative timeout values to Mining::waitTipChanged
• Makes waitfornewblock, waitforblock and waitforblockheight RPC methods usable from the GUI when -server=1 is not set.
• Changes Mining::waitTipChanged to return optional<BlockRef> instead of BlockRef and return nullopt instead of crashing if there is a timeout or if the node is shut down before a tip is connected.
• Changes Mining::waitTipChanged to not time out before a tip is connected, so it is convenient and safe to call during startup, and only returns nullopt on early shutdowns.
• Changes Mining::createNewBlock to block and wait for a tip to be connected if it is called on startup instead of crashing. Also documents that it will return null on early shutdowns.

This allows waitNext() (added in #31283) to safely assume TipBlock() isn't null, not even during a scenario of early shutdown.

Finally this PR clarifies long poll behaviour, mostly by adding code comments, but also through an early break.

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31785.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, TheCharlatan, vasild, achow101
Concept ACK	ismaelsadeeq

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #31117 (miner: Reorg Testnet4 minimum difficulty blocks by fjahr)

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.

If it's not too difficult, can we reproduce this with a test?

This depends on the node initialization (and shutdown) sequence. I'm not sure how to reproduce that in a test. And we might change that sequence in a way that this never happens in the first place.

The Stratum v2 Template Provider that I implemented first calls mining.waitTipChanged(uint256::ZERO) before calling mining.createNewBlock(). This ensures that no matter how we order the node & IPC startup sequence, it won't prematurely try to make a block.

See Sjors#49 (src/sv2/template_provider.cpp in Sv2TemplateProvider::ThreadSv2Handler()).

But other implementers may not realise this, so having createNewBlock() return nothing when called too early seems like a good precaution.

See this comment for more details on the init sequence:

bitcoin/src/init.cpp

Lines 1817 to 1833 in 1172bc4

	/*
	* Wait for genesis block to be processed. Typically kernel_notifications.m_tip_block
	* has already been set by a call to LoadChainTip() in CompleteChainstateInitialization().
	* But this is skipped if the chainstate doesn't exist yet or is being wiped:
	*
	* 1. first startup with an empty datadir
	* 2. reindex
	* 3. reindex-chainstate
	*
	* In these case it's connected by a call to ActivateBestChain() in the initload thread.
	*/
	{
	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.TipBlock() || ShutdownRequested(node);
	});
	}

Also note that currently the IPC server starts listening in AppInitMain before the above wait so the race condition is possible.

bitcoin/src/init.cpp

Lines 1358 to 1367 in 1172bc4

	if (interfaces::Ipc* ipc = node.init->ipc()) {
	for (std::string address : gArgs.GetArgs("-ipcbind")) {
	try {
	ipc->listenAddress(address);
	} catch (const std::exception& e) {
	return InitError(Untranslated(strprintf("Unable to bind to IPC address '%s'. %s", address, e.what())));
	}
	LogPrintf("Listening for IPC requests on address %s\n", address);
	}
	}

A good client application might wait for -startupnotify before trying to connect via IPC. StartupNotify() is called at the end AppInitMain. But we shouldn't assume all clients do.

There were slightly more worms in this can.

Instead I've now changed createNewBlock() to first call waitTipChanged() and the latter to return null during a shutdown. This shifts some work to the RPC, but I think makes the overall behaviour easier to understand.

Approach ACK be15f2e769987b1df2cf1c8f327c5aafe064dbb3

Suggested change

	* @retval empty if node is shutting down
	* @retval std::nullopt if the given `timeout` passes or the node is shut down before the tip is connected (there is no tip during startup).

re: #31785 (comment)

I don't think suggested text is accurate because if timeout is exceeded while waiting for a new tip, this returns information about the current tip. I don't think that behavior should be changed.

Returning null only when node is shutting down should be the simplest and best behavior.

if timeout is exceeded while waiting for a new tip, this returns information about the current tip

True. What I meant was that if there is no tip within the timeout. The function does wait_for(... tip_hash && tip_hash != current_tip ...) - tip_hash to be set and to be different than the current. I meant that it could remain unset while the timeout passes. I see how the comment I suggested is misleading. What about:

@retval std::nullopt if the given `timeout` passes before the tip is
connected (there is no tip during startup) or the node is shut down.

?

Ah, I think I see the problem. There's a potential scenario where node startup is unusually slow, longer than timeout.

I think it's better in that case to ignore the timeout and instead keep waiting for the node to set a tip.

Yes, this scenario I had in mind.

in that case to ignore the timeout and instead keep waiting for the node to set a tip

Hmm. What if the tip is not being connected for whatever reason? Wait forever? The current behavior of respecting the timeout looks safer. Would be an odd user experience to provide a timeout and the program to decide to wait longer for whatever reason.

Let's continue here: #31785 (comment)

Just before the lock and return there was a period when both m_tip_block_mutex and cs_main were unlocked, so tip_hash may be stale here. So this could return an inconsistent hash+height - the hash of one block and the height of another. The previous code was ok because it retrieved both the hash and the height from chainman().ActiveChain().Tip() under cs_main. I think this will fix it:

-        LOCK(::cs_main);
-        return BlockRef{*Assume(tip_hash), Assume(chainman().ActiveChain().Tip())->nHeight};
+        return getTip();

That seems simpler anyway.

In commit "rpc: handle shutdown during long poll and wait methods" (ad3af401c19b8d05ce69011a359db3090b7018e1)

Nice suggestion. If this suggestion is applied should also simplify the code above

-        if (!tip_hash || chainman().m_interrupt) return {};
+        if (chainman().m_interrupt) return {};

since tip_hash is no longer relevant. Can also reduce the scope of the tip_hash variable.

Given that "no tip" is expected here (unlikely but is not a "programming logic error"), maybe avoid CHECK_NONFATAL() and use JSONRPCError:

std::optional<BlockRef> old_block{miner.getTip()};
std::optional<BlockRef> new_block;
if (IsRPCRunning()) {
    const uint256 h{old_block.has_value() ? old_block->hash : uint256::ZERO};
    new_block = timeout > 0 ? miner.waitTipChanged(h, std::chrono::milliseconds(timeout)) : miner.waitTipChanged(h);
}

if (!new_block.has_value()) {
    throw JSONRPCError(RPC_CLIENT_NOT_CONNECTED, "No tip within timeout or shutting down");
}

Oh, you mean old_block could be nullopt (assuming the RPC is loaded at this point).

But the suggested error here would change the current behavior - if there's a timeout we return the old block. I could do an early error return though if the first getTip() doesn't give us a tip.

I mean to avoid CHECK_NONFATAL() if the tip is null and use JSONRPCError instead.

If the tip is null at the start, then wait for it to be set, if not set within the timeout, then JSONRPCError.

I ended up with something similar...

Maybe elaborate the message that we could be here if no tip was connected within the timeout: "No tip within timeout or shutting down".

I'm introducing a new RPC_SHUTDOWN_ERROR which makes sense here. I don't think the message itself matters, since the user knows they shut down the node - and the exact details of where and why the RPC call fails aren't important.

nit: I think it is redundant to check that maybe_tip has value with CHECK_NONFATAL() just after if (!maybe_tip). That's like:

if (A) {
     throw ...
}
assert(!A);

tip = maybe_tip->hash; seems fine as well.

Indeed, but on the other hand code changes over time, stuff appears between the if statement and the usage of tip, then someone changes the if statement, and now we have a crash...

Ok

Previously it would return the last known tip during shutdown, but this creates an ambiguous circumstance in the scenario where the node is started and quickly shutdown, before notifications().TipBlock() is set.

New behavior seems good, but I am confused by this description. It looks to me like previous behavior was to segfault in this case, because it would returnTip()->GetBlockHash() and Tip() would be null? It would be helpful if description said more specifically what previous behavior was.

Code review be15f2e769987b1df2cf1c8f327c5aafe064dbb3. Mostly looks good but I think vasild's suggestion #31785 (comment) should be used so waitTipChanged can't return an inconsistent hash and height

In commit "rpc: handle shutdown during long poll and wait methods" (ad3af401c19b8d05ce69011a359db3090b7018e1)

Nice suggestion. If this suggestion is applied should also simplify the code above

-        if (!tip_hash || chainman().m_interrupt) return {};
+        if (chainman().m_interrupt) return {};

since tip_hash is no longer relevant. Can also reduce the scope of the tip_hash variable.

re: #31785 (comment)

I don't think suggested text is accurate because if timeout is exceeded while waiting for a new tip, this returns information about the current tip. I don't think that behavior should be changed.

Returning null only when node is shutting down should be the simplest and best behavior.

I changed waitTipChanged() to only return nullopt during shutdown. This is achieved by extending the timeout in the unlikely event that node initialization takes longer.

I introduced a new RPC_SHUTDOWN_ERROR code and use it in case waitfornewblock and friends are called really early and the node shuts down (probably very hard to reach in practice). See #31785 (comment)

It would be helpful if description said more specifically what previous behavior was.

Updated the description.

On question about long polling I still need to investigate: #31785 (comment)

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/37158437645}

Hints

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.

I pushed a commit to clarify long poll behavior.

I mixed up the commits in my last push, will fix...

Concept ACK

In 64a2795 "rpc: handle shutdown during long poll and wait methods"

is there a use for returning the last known tip?

If not, should we simplify the behavior by always returning nullopt when the tip has not changed or no better block is available?

Or It will be more consistent to return the last known tip if it is set, when this edge case happen handle it by returning nullopt.

@ismaelsadeeq I think this just preserves existing RPC behavior. What to return during shutdown is a bit arbitrary anyway, because a connected client will have to handle shutdown more generally, potentially by also shutting down.

I also haven't checked if any of the other tests rely on the existing behavior.

So maybe better for a followup?

ACK cc1001f3bf17b31512c05fb359e09483a07fb2a3

Code review ACK cc1001f3bf17b31512c05fb359e09483a07fb2a3. Only change since last review was rebasing after #31283

re: #31785 (review)

is a use for returning the last known tip?

I think the main use for returning the last known tip when the tip hasn't changed is just to allow the caller distinguish between the case where the tip has not changed from the case where the node is shutting down. If it returned null in both cases we would need to have to add a separate isShuttingDown method to distinguish them and code calling waitTipChanged in a loop would be more complicated.

I don't think there is a use for returning the last known tip when the known is shutting down, just because caller can assume the tip at that point is the same value they passed as the current_tip argument.

In commit "Have createNewBlock() wait for a tip" (db14ca3556ca792546bf4343feb733271333690f)

Commit message is a little confusing because it doesn't mention the timeout change. Would be clearer if it said the commit was changing two things (1) returning null on shutdown instead of last tip (2) ignoring timeout value during startup instead of returning 0 if timeout elapsed before tip was connected

I improved the commit message.

re: #31785 (comment)

I improved the commit message.

Thanks for the update. In case you wind up updating again, maybe consider extending to make a little more readable:

-- return null on shutdown instead of the last tip
-- ignore timeout value node initialization
+Update createNewBlock to:
+
+- ignore timeout value during startup and wait for a tip to be connected instead of returning 0
+- return null on shutdown instead of the last tip to make shutdown easier to detect
 
 This allows consumers of BlockTemplate to safely
 assume that a tip is connected, instead of having
 to account for startup and early shutdown scenarios.

Will do if I need to retouch.

Code review ACK 05117e6, just updated a commit message since last review

re: #31785 (comment)

I improved the commit message.

Thanks for the update. In case you wind up updating again, maybe consider extending to make a little more readable:

-- return null on shutdown instead of the last tip
-- ignore timeout value node initialization
+Update createNewBlock to:
+
+- ignore timeout value during startup and wait for a tip to be connected instead of returning 0
+- return null on shutdown instead of the last tip to make shutdown easier to detect
 
 This allows consumers of BlockTemplate to safely
 assume that a tip is connected, instead of having
 to account for startup and early shutdown scenarios.

ACK 05117e6

Nit: This comment seems a bit misplaced here now. Maybe place it further up before calling WaitLock?

Will look into moving it if I need to retouch more.

ACK 05117e6

ACK 05117e6