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

pull Sjors wants to merge 2 commits into bitcoin:master from Sjors:2024/08/waitforblock changing 3 files +26 −7
  1. 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.

  2. 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
    ACK TheCharlatan
    Stale ACK ryanofsky

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

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label RPC/REST/ZMQ on Aug 12, 2024
  4. Sjors force-pushed on Aug 12, 2024
  5. Sjors force-pushed on Aug 12, 2024
  6. Sjors force-pushed on Aug 12, 2024
  7. Sjors force-pushed on Aug 13, 2024
  8. DrahtBot added the label Needs rebase on Sep 2, 2024
  9. Sjors force-pushed on Sep 3, 2024
  10. DrahtBot removed the label Needs rebase on Sep 3, 2024
  11. DrahtBot added the label Needs rebase on Sep 3, 2024
  12. Sjors force-pushed on Sep 4, 2024
  13. DrahtBot removed the label Needs rebase on Sep 4, 2024
  14. DrahtBot added the label CI failed on Sep 7, 2024
  15. DrahtBot removed the label CI failed on Sep 9, 2024
  16. DrahtBot added the label Needs rebase on Sep 9, 2024
  17. Sjors force-pushed on Sep 10, 2024
  18. DrahtBot removed the label Needs rebase on Sep 10, 2024
  19. DrahtBot added the label CI failed on Sep 10, 2024
  20. DrahtBot added the label Needs rebase on Sep 12, 2024
  21. Sjors force-pushed on Sep 17, 2024
  22. DrahtBot removed the label Needs rebase on Sep 17, 2024
  23. DrahtBot removed the label CI failed on Sep 17, 2024
  24. Sjors force-pushed on Sep 26, 2024
  25. Sjors commented at 8:14 am on September 26, 2024: member
    Now that #30409 landed this is a single commit, and ready for review.
  26. Sjors marked this as ready for review on Sep 26, 2024
  27. Panga9309 approved
  28. 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).

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

  30. 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.
  31. ryanofsky approved
  32. 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)
  33. 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
  34. Sjors commented at 8:24 am on November 15, 2024: member
    @ryanofsky & @luke-jr thanks for reviewing, will address feedback soon(tm).
  35. Sjors force-pushed on Nov 19, 2024
  36. Sjors commented at 3:47 pm on November 19, 2024: member
    I took @ryanofsky’s suggestion: #30635 (review)
  37. in src/rpc/blockchain.cpp:301 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
  38. 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

  39. ryanofsky approved
  40. 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.
  41. Sjors force-pushed on Nov 21, 2024
  42. Sjors commented at 12:36 pm on November 21, 2024: member
    Rebased, added comment and expanded commit message to note the GUI improvement.
  43. 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.

  44. Sjors renamed this:
    rpc: add optional blockhash to waitfornewblock
    rpc: add optional blockhash to waitfornewblock, unhide in help
    on Nov 21, 2024
  45. TheCharlatan approved
  46. TheCharlatan commented at 9:55 am on November 22, 2024: contributor

    ACK 1f8808d411fc0ed2b91f185e08ad8c1ee8f8432f

    Tested in the GUI too.

  47. DrahtBot requested review from ryanofsky on Nov 22, 2024
  48. ryanofsky approved
  49. 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

  50. Sjors requested review from luke-jr on Dec 16, 2024
  51. Sjors requested review from maflcko on Jan 6, 2025
  52. DrahtBot added the label Needs rebase on Jan 31, 2025
  53. Sjors force-pushed on Feb 1, 2025
  54. Sjors commented at 9:05 am on February 1, 2025: member
    Rebased after #31600.
  55. DrahtBot removed the label Needs rebase on Feb 4, 2025
  56. ryanofsky approved
  57. 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.
  58. DrahtBot requested review from TheCharlatan on Feb 18, 2025
  59. Sjors marked this as a draft on Feb 19, 2025
  60. 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.
  61. Sjors force-pushed on Feb 19, 2025
  62. 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.
  63. 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
  64. Sjors force-pushed on Feb 19, 2025
  65. DrahtBot added the label CI failed on Feb 19, 2025
  66. DrahtBot removed the label CI failed on Feb 19, 2025
  67. ryanofsky approved
  68. ryanofsky commented at 1:06 pm on February 26, 2025: contributor
    Code review ACK 798bc8d400b354d626678dc7b54c3cb0f464aa7a. Changes since last review were rebasing on top of #31785 to simplify this PR, updating comments and release notes, and also unhiding waitforblock and waitforblockheight methods
  69. Sjors force-pushed on Mar 13, 2025
  70. ryanofsky approved
  71. ryanofsky commented at 3:51 pm on March 23, 2025: contributor
    Code review ACK e47b20f2f310f05832c879401660860cc40a6a09 with no changes since last review other than rebase
  72. Sjors commented at 8:52 am on March 24, 2025: member
    My latest change to #31785 only changed the commit message, so I’m not rebasing this yet.
  73. Sjors force-pushed on Apr 17, 2025
  74. Sjors commented at 1:01 pm on April 17, 2025: member
    Ready for review now that #30635 landed.
  75. Sjors marked this as ready for review on Apr 17, 2025
  76. in src/rpc/blockchain.cpp:299 in 0625f26b09 outdated 
    295+     * If the caller did not provide a current tip hash, call getTip() to get
296+     * one and wait for the tip to be different from this value. This mode is
297+     * less reliable because if the tip changed between waitfornewblock calls,
298+     * it will need to change a second time before this call returns.
299+     *
300+     * Abort if RPC came out of warmup too early.
    ryanofsky commented at 6:04 pm on April 21, 2025:

    In commit “rpc: add optional blockhash to waitfornewblock” (0625f26b09084fcbd4daa48b3456f54eff6b22de)

    Do we think “Abort if RPC came out of warmup too early.” comment is still accurate? I’ve having trouble understanding what it could mean and I don’t think there are cases where this aborted even before this change. Would probably suggest dropping this comment or trying to make it clearer.

    Also could replace /** with a normal comment above since /** normally indicates a doxygen comment.

    Sjors commented at 8:33 am on April 22, 2025:

    I replaced the /**.

    I dropped the warmup comment, but will keep the CHECK_NONFATAL. If the RPC starts listening before ActiveChain().Tip() is set, then getTip() returns nothing. I’m not sure if that can ever happen. SetRPCWarmupFinished() is called as step 13 in init.cpp with a somewhat ominous comment above it.

  77. in doc/release-30635.md:4 in 067acb5907 outdated 
    0@@ -1,4 +1,5 @@
1 Updated RPCs
2 ------------
3 
4-- The waitfornewblock RPC takes an optional `current_tip` argument. (#30635)
5+- The waitfornewblock takes an optional `current_tip` argument. It is also no longer hidden. (#30635)
    ryanofsky commented at 6:06 pm on April 21, 2025:

    In commit “rpc: unhide waitfor{block,newblock,blockheight}” (067acb5907b5b135cff702da13a1f64a02d64110)

    Maybe s/takes/now takes/ to be clear this is a change

  78. ryanofsky approved
  79. ryanofsky commented at 6:07 pm on April 21, 2025: contributor
    Code review ACK 067acb5907b5b135cff702da13a1f64a02d64110. No changes since previous review other than rebasing
  80. rpc: add optional blockhash to waitfornewblock 0786b7509a
  81. rpc: unhide waitfor{block,newblock,blockheight} 
    They are now reliable. An earlier commit dropped their
IsRPCRunning() guards so they also work in the GUI.
    c6e2c31c55
  82. Sjors force-pushed on Apr 22, 2025
  83. TheCharlatan approved
  84. TheCharlatan commented at 10:32 am on April 22, 2025: contributor
    Re-ACK c6e2c31c55123cc97b4400bcbf3c37a39b067a22
  85. DrahtBot requested review from ryanofsky on Apr 22, 2025


Sjors DrahtBot Panga9309 maflcko ryanofsky luke-jr TheCharlatan


luke-jr maflcko ryanofsky

Labels
RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-04-28 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me