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

1. 
   Sjors commented at 2:56 pm on February 3, 2025: member

   The Mining interface waitTipChanged() method 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.

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

   The getblocktemplate, waitfornewblock and waitforblockheight RPC calls now explicitly handle the shutdown scenario.

2. 
   DrahtBot commented at 2:57 pm on February 3, 2025: 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/31785.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #31117 (miner: Reorg Testnet4 minimum difficulty blocks by fjahr)
   • #30635 (rpc: add optional blockhash to waitfornewblock, unhide in help by Sjors)

   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.

3. in src/node/interfaces.cpp:995 in af85987a16 outdated

   988@@ -989,6 +989,12 @@ class MinerImpl : public Mining
   989 
   990     std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options) override
   991     {
   992+        {
   993+            // Ensure m_tip_block is set so consumers of BlockTemplate can rely on that.
   994+            WAIT_LOCK(notifications().m_tip_block_mutex, lock);
   995+            if (!notifications().TipBlock()) return nullptr;
   

   ---

   ---

   

   l0rinc commented at 3:03 pm on February 3, 2025:

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

   ---

   

   Sjors commented at 3:06 pm on February 3, 2025:

   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.
4. 
   Sjors commented at 3:05 pm on February 3, 2025: member

   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 https://github.com/Sjors/bitcoin/pull/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:

   https://github.com/bitcoin/bitcoin/blob/1172bc4157eefe80d1aaf0b56459857ec651e535/src/init.cpp#L1817-L1833

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

   https://github.com/bitcoin/bitcoin/blob/1172bc4157eefe80d1aaf0b56459857ec651e535/src/init.cpp#L1358-L1367

   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.

5. in src/node/interfaces.cpp:995 in af85987a16 outdated

   988@@ -989,6 +989,12 @@ class MinerImpl : public Mining
   989 
   990     std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options) override
   

   ---

   ---

   

   maflcko commented at 3:21 pm on February 3, 2025:

   Would be good to update the docs, because CreateNewBlock never returns null and always returns a block.

   ---

   

   Sjors commented at 5:17 pm on February 3, 2025:

   The latest push has waitTipChanged and createNewBlock return null if the node shuts down, and documentation is updated to reflect that.
6. Sjors force-pushed on Feb 3, 2025
7. 
   Sjors commented at 5:13 pm on February 3, 2025: member

   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.

8. Sjors renamed this:
   Have createNewBlock() ensure m_tip_block is set
   Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods
   on Feb 3, 2025
9. 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 calls now explicitly handle the shutdown scenario.

   ad3af401c1
10. Have createNewBlock() wait for a tip

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

    be15f2e769
11. Sjors force-pushed on Feb 3, 2025

Contributors
Sjors DrahtBot l0rinc maflcko