The waitfornewblock is inherently racy as the tip may have changed since the last RPC call, and can even change during initial processing of this call.
Add an optional blockhash argument so the caller can specify their current tip. Return immediately if our tip is different.
I’ve made it fail if LookupBlockIndex fails. This should never happen if the user got the block hash from our RPC in the first place.
Finally, waitfornewblock is no longer hidden in help.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30635.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | TheCharlatan, ryanofsky |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
298+ block.hash = block_index->GetBlockHash();
299+ block.height = block_index->nHeight;
300+ }
301+ }
302+
303 if (IsRPCRunning()) {
-server is passed. So no code will run and what was passed in will be returned, incorrectly.
The latest version gets rid of this check entirely.
IIUC it wasn’t needed here in the first place. The other waitfor RPC calls use this to see if they need to break out of their while loop each time a new block comes in (or interrupt happened).
261@@ -262,6 +262,7 @@ static RPCHelpMan waitfornewblock()
262 "\nMake sure to use no RPC timeout (bitcoin-cli -rpcclienttimeout=0)",
263 {
264 {"timeout", RPCArg::Type::NUM, RPCArg::Default{0}, "Time in milliseconds to wait for a response. 0 indicates no timeout."},
265+ {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, "Last known block hash. Returns immediately if the tip is different."},
In commit “rpc: add optional blockhash to waitfornewblock” (13b867fb9b37fef3f14732904172e4ecba6fc973)
Maybe use phrasing from fa4c0750331f36121ba92bbc2f22c615b7934e52 and say “Method waits for the chain tip to differ from this.” I think that’s a little better because it describes behavior in general instead of describing a specific case where it returns immediately. I think it would also be better to call this current_tip instead of blockhash like the other variable, to be more descriptive.
300+ }
301+ }
302+
303 if (IsRPCRunning()) {
304 block = timeout ? miner.waitTipChanged(block.hash, std::chrono::milliseconds(timeout)) : miner.waitTipChanged(block.hash);
305 }
I think it would be good to simplify all of this to just:
0 // Use tip as reference block, unless a block hash was provided.
1 uint256 current_tip{request.params[1].isNull()
2 ? miner.getTip().value_or(BlockRef{}).hash
3 : ParseHashV(request.params[1], "blockhash")};
4
5 BlockRef block{timeout > 0
6 ? miner.waitTipChanged(current_tip, std::chrono::milliseconds(timeout))
7 : miner.waitTipChanged(current_tip)};
In addition to being simpler, this would fix a number of problems and inconveniences of the current implementation:
295+ if (!block_index) {
296+ throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
297+ }
298+ block.hash = block_index->GetBlockHash();
299+ block.height = block_index->nHeight;
300+ }
hash to block and return?
290- block = timeout ? miner.waitTipChanged(block.hash, std::chrono::milliseconds(timeout)) : miner.waitTipChanged(block.hash);
291- }
292+ // Use tip as reference block, unless a block hash was provided.
293+ uint256 current_tip{request.params[1].isNull()
294+ ? miner.getTip().value_or(BlockRef{}).hash
295+ : ParseHashV(request.params[1], "current_tip")};
In commit “rpc: add optional blockhash to waitfornewblock” (c305ad5b06545497752b2f12d8d9f72e8a6c2941)
Might be useful if comment explained the different cases here. Maybe: // If caller provided a current_tip value, pass it to waitTipChanged. If the caller did not provide a current tip hash, call getTip() to get one and wait for the tip to be different from this value. (This mode is less reliable because if the tip changed between waitfornewblock calls, it will need to change a second time before this call returns.) If getTip() returns null, which can happen during startup, pass 0 to waitTipChanged() so it will wait for any tip hash to be set.
284@@ -283,10 +285,14 @@ static RPCHelpMan waitfornewblock()
285 NodeContext& node = EnsureAnyNodeContext(request.context);
286 Mining& miner = EnsureMining(node);
287
288- auto block{CHECK_NONFATAL(miner.getTip()).value()};
289- if (IsRPCRunning()) {
In commit “rpc: add optional blockhash to waitfornewblock” (c305ad5b06545497752b2f12d8d9f72e8a6c2941)
Note: dropping IsRPCRunning call is a minor change in behavior, but probably a good one, as it should let the waitfornewblock function in the GUI console even if the RPC server is not enabled. In any case it is safe to drop this because waitTipChanged already will return if the node is shutting down.
This RPC method now also works in the GUI console when the RPC
server is not enabled.
This method is not dangerous and since the previous commit it no longer requires GUI users to enable the RPC server.
I added a commit to make waitfornewblock visible in help. It was introduced in 2016 by #8680 as a quick fix to deal with CI failures.
At this point is seem mature enough, no longer has weird quirks like requiring GUI users to enable the RPC server, it’s probably actually useful and at least one library uses it anyway. So it seems fine to list it.
ACK 1f8808d411fc0ed2b91f185e08ad8c1ee8f8432f
Tested in the GUI too.
Code review ACK 1f8808d411fc0ed2b91f185e08ad8c1ee8f8432f
Since last review a comment was expanded, and a new commit makes the waitfornewblock RPC not hidden anymore
