rpc: add optional blockhash to waitfornewblock, unhide wait methods in help

Sjors commented at 11:42 am on August 12, 2024: member

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, the waitfor{block,newblock,blockheight} RPC methods are no longer hidden in help:

the changes in #30409 ensured these methods could work in the GUI
#31785 removed the guards that prevented GUI users from using them
this PR makes waitfornewblock reliable

So there’s no more reason to hide them.

DrahtBot commented at 11:42 am on August 12, 2024: contributor

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/30635.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale 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.

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.

DrahtBot added the label RPC/REST/ZMQ on Aug 12, 2024

Sjors force-pushed on Aug 12, 2024

Sjors force-pushed on Aug 13, 2024

DrahtBot added the label Needs rebase on Sep 2, 2024

Sjors force-pushed on Sep 3, 2024

DrahtBot removed the label Needs rebase on Sep 3, 2024

DrahtBot added the label Needs rebase on Sep 3, 2024

Sjors force-pushed on Sep 4, 2024

DrahtBot removed the label Needs rebase on Sep 4, 2024

DrahtBot added the label CI failed on Sep 7, 2024

DrahtBot removed the label CI failed on Sep 9, 2024

DrahtBot added the label Needs rebase on Sep 9, 2024

Sjors force-pushed on Sep 10, 2024

DrahtBot removed the label Needs rebase on Sep 10, 2024

DrahtBot added the label CI failed on Sep 10, 2024

DrahtBot added the label Needs rebase on Sep 12, 2024

Sjors force-pushed on Sep 17, 2024

DrahtBot removed the label Needs rebase on Sep 17, 2024

DrahtBot removed the label CI failed on Sep 17, 2024

Sjors force-pushed on Sep 26, 2024

Sjors commented at 8:14 am on September 26, 2024: member

Now that #30409 landed this is a single commit, and ready for review.

Sjors marked this as ready for review on Sep 26, 2024

Panga9309 approved

in src/rpc/blockchain.cpp:303 in 13b867fb9b outdated

298+            block.hash = block_index->GetBlockHash();
299+            block.height = block_index->nHeight;
300+        }
301+    }
302+
303     if (IsRPCRunning()) {

maflcko commented at 8:23 am on September 27, 2024:

unrelated bug: But the GUI never sets this, unless -server is passed. So no code will run and what was passed in will be returned, incorrectly.

Sjors commented at 11:05 am on September 27, 2024:

So this effects GUI users that use the console, rather than bitcoin-cli?

Sjors commented at 3:50 pm on November 19, 2024:

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).

in src/rpc/blockchain.cpp:265 in 13b867fb9b outdated

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."},

ryanofsky commented at 2:55 pm on October 8, 2024:

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.

in src/rpc/blockchain.cpp:305 in 13b867fb9b outdated

300+        }
301+    }
302+
303     if (IsRPCRunning()) {
304         block = timeout ? miner.waitTipChanged(block.hash, std::chrono::milliseconds(timeout)) : miner.waitTipChanged(block.hash);
305     }

ryanofsky commented at 3:20 pm on October 8, 2024:

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:

If an unrecognized block hash was passed, it would make the RPC return the current block tip instead of throwing an exception.
If no block hash was passed and tip was null, it would make the RPC wait for a block to be connected instead of throwing an exception.
It would fix the bug marco described #30635 (review) where this function does not wait in the GUI console.

ryanofsky approved

ryanofsky commented at 3:25 pm on October 8, 2024: contributor

Code review ACK 13b867fb9b37fef3f14732904172e4ecba6fc973. I think implementation could be a ilttle simpler and more friendly though (see suggestion below)

in src/rpc/blockchain.cpp:300 in 13b867fb9b outdated

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+        }

luke-jr commented at 9:50 pm on November 14, 2024:

Why do all this? Can’t we just compare hash to block and return?

Sjors commented at 7:23 am on December 16, 2024:

@luke-jr it should be simpler now

Sjors commented at 8:24 am on November 15, 2024: member

@ryanofsky & @luke-jr thanks for reviewing, will address feedback soon(tm).

Sjors force-pushed on Nov 19, 2024

Sjors commented at 3:47 pm on November 19, 2024: member

I took @ryanofsky’s suggestion: #30635 (review)

in src/rpc/blockchain.cpp:305 in c305ad5b06 outdated

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")};

ryanofsky commented at 1:15 am on November 21, 2024:

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.

Sjors commented at 12:35 pm on November 21, 2024:

Added

in src/rpc/blockchain.cpp:290 in c305ad5b06 outdated

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()) {

ryanofsky commented at 1:27 am on November 21, 2024:

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.

Sjors commented at 12:26 pm on November 21, 2024:

Good point. Just tested that that’s indeed the case. I’ll mention that in the commit message.

cc @hebasto

ryanofsky approved

ryanofsky commented at 1:29 am on November 21, 2024: contributor

Code review ACK c305ad5b06545497752b2f12d8d9f72e8a6c2941. This is a nice change that should make the RPC (and test) more reliable, and the implementation seems to be pretty simple now.

Sjors force-pushed on Nov 21, 2024

Sjors commented at 12:36 pm on November 21, 2024: member

Rebased, added comment and expanded commit message to note the GUI improvement.

Sjors commented at 12:53 pm on November 21, 2024: member

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.

Sjors renamed this:
~~rpc: add optional blockhash to waitfornewblock~~
rpc: add optional blockhash to waitfornewblock, unhide in help
on Nov 21, 2024

TheCharlatan approved

TheCharlatan commented at 9:55 am on November 22, 2024: contributor

ACK 1f8808d411fc0ed2b91f185e08ad8c1ee8f8432f

Tested in the GUI too.

DrahtBot requested review from ryanofsky on Nov 22, 2024

ryanofsky approved

ryanofsky commented at 7:15 pm on December 4, 2024: contributor

Code review ACK 1f8808d411fc0ed2b91f185e08ad8c1ee8f8432f

Since last review a comment was expanded, and a new commit makes the waitfornewblock RPC not hidden anymore

Sjors requested review from luke-jr on Dec 16, 2024

Sjors requested review from maflcko on Jan 6, 2025

DrahtBot added the label Needs rebase on Jan 31, 2025

Sjors force-pushed on Feb 1, 2025

Sjors commented at 9:05 am on February 1, 2025: member

Rebased after #31600.

DrahtBot removed the label Needs rebase on Feb 4, 2025

ryanofsky approved

ryanofsky commented at 6:21 pm on February 18, 2025: contributor

Code review ACK 3d88cad169aa3d2d8731c3888749054be6688f1b. No changes since last review other than rebase due to conflict in a test.

DrahtBot requested review from TheCharlatan on Feb 18, 2025

Sjors marked this as a draft on Feb 19, 2025

Sjors commented at 8:21 am on February 19, 2025: member

#31785 increasingly overlaps with the scope of this PR, so I’m going to rebase this on it.

Sjors force-pushed on Feb 19, 2025

Sjors commented at 3:32 pm on February 19, 2025: member

This is now based on #31785, and we now unhide all three wait methods.

Sjors renamed this:
~~rpc: add optional blockhash to waitfornewblock, unhide in help~~
rpc: add optional blockhash to waitfornewblock, unhide wait methods in help
on Feb 19, 2025

Handle negative timeout for waitTipChanged() aff5a9f5be

rpc: drop unneeded IsRPCRunning() guards

This was preventing the (hidden) waitfornewblock, waitforblock and
waitforblockheight methods from being used in the GUI.

The check was added in d6a5dc4a2eaa0d7348804254ca09e75fc3a858ab
when these RPC methods were first introduced.

They could have been dropped when dca923150e3ac10a57c23a7e29e76516d32ec10d
refactored these methods to use waitTipChanged(), which already
checks for shutdown.

Making this change now simplifies the next commit.

8844bd5b3a

rpc: handle shutdown during long poll and wait methods

The waitTipChanged() now returns nullopt if the node is shutting down.

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.

The getblocktemplate, waitfornewblock and waitforblockheight RPC
are updated to handle this. Existing behavior is preserved.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

353a41cbad

Have createNewBlock() wait for a tip

Additionally it returns null if the node started to shutdown before TipBlock() was set.

56fac7c8d8

rpc: clarify longpoll behavior

Move the comparison to hashWatchedChain inside the while loop.

Although this early return prevents the GetTransactionsUpdated()
call in cases where the tip updates, it's only done to improve
readability. The check itself is very cheap (although a more
useful check might not be).

Also add code comments.

9725cd6a5c

rpc: add optional blockhash to waitfornewblock 55691b5b1b

rpc: unhide waitfor{block,newblock,blockheight}

They are now reliable. An earlier commit dropped their
IsRPCRunning() guards so they also work in the GUI.

798bc8d400

Sjors force-pushed on Feb 19, 2025

DrahtBot added the label CI failed on Feb 19, 2025

DrahtBot removed the label CI failed on Feb 19, 2025

rpc: add optional blockhash to waitfornewblock, unhide wait methods in help #30635

Code Coverage & Benchmarks

Reviews

Conflicts