interfaces: enable cancelling running `waitNext` calls #33676

ismaelsadeeq commented at 10:14 AM on October 22, 2025: member

This is an attempt to fix #33575 see the issue for background and the usefulness of this feature.

This PR uses one of the suggested approaches: adding a new interruptWaitNext() method to the mining interface.

It introduces a new boolean variable, m_interrupt_wait, which is set to false when the thread starts waiting. The interruptWaitNext() method wakes the thread and sets m_interrupt_wait to true. Whenever the thread wakes up, it checks whether the wait was aborted; if so, it simply set m_interrupt_wait to false and returnnullptr.

This PR also adds a functional test for the new method. The test uses asyncio to spawn two tasks and attempts to ensure that the wait is executed before the interrupt by using an event monitor. It adds a 0.1-second buffer to ensure the wait has started executing. If that buffer elapses without waitNext executing, the test will fail because a transaction is created after the buffer.

DrahtBot renamed this:
~~interfaces: enable cancelling running `waitNext` calls~~
interfaces: enable cancelling running `waitNext` calls
on Oct 22, 2025

DrahtBot added the label interfaces on Oct 22, 2025

DrahtBot commented at 10:14 AM on October 22, 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/33676.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, furszy, Sjors, TheCharlatan
Concept ACK	Eunovo

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:

#33421 (node: add BlockTemplateCache by ismaelsadeeq)

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.

ismaelsadeeq marked this as ready for review on Oct 22, 2025

ryanofsky commented at 3:48 PM on October 22, 2025: contributor

Code review 0e0cf1822416956b6853e593517f4a9fab157a62. This looks very good and I did not see any issues at a quick glance. I think I might suggest shortening interruptWaitNext to interruptWait, but no strong opinion.

ismaelsadeeq force-pushed on Oct 23, 2025

ismaelsadeeq commented at 9:58 AM on October 23, 2025: member

I took your suggestion, make the test robust by generating a transaction after interrupting, and took @furszy suggestion by initializing the waiting inside the lock.

cc @Eunovo

furszy commented at 3:48 PM on October 25, 2025: member

Concept ACK. Will review soon.

in src/node/interfaces.cpp:935 in f4e63cc807

 931 | +
 932 |      const BlockAssembler::Options m_assemble_options;
 933 |  
 934 |      const std::unique_ptr<CBlockTemplate> m_block_template;
 935 |  
 936 | +    bool waiting{false};

Eunovo commented at 3:09 PM on October 27, 2025:

Consider using m_interrupt_wait instead.

ismaelsadeeq commented at 6:22 PM on October 27, 2025:

Done

in src/node/miner.h:254 in f4e63cc807

 249 | @@ -247,7 +250,8 @@ std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainma
 250 |                                                        CTxMemPool* mempool,
 251 |                                                        const std::unique_ptr<CBlockTemplate>& block_template,
 252 |                                                        const BlockWaitOptions& options,
 253 | -                                                      const BlockAssembler::Options& assemble_options);
 254 | +                                                      const BlockAssembler::Options& assemble_options,
 255 | +                                                      bool& wait);

Eunovo commented at 3:11 PM on October 27, 2025:

consider bool& interrupt_wait

ismaelsadeeq commented at 6:22 PM on October 27, 2025:

Done

Eunovo commented at 3:39 PM on October 27, 2025: contributor

Concept ACK https://github.com/bitcoin/bitcoin/pull/33676/commits/f4e63cc807f8c34a780604bd3802739c37209b75

I took a first look and left some nits. I'll take a second look.

ryanofsky approved

ryanofsky commented at 4:14 PM on October 27, 2025: contributor

Code review ACK f4e63cc807f8c34a780604bd3802739c37209b75. Thanks for the PR! This should be very helpful and close a significant gap in the mining interface, so clients are not forced to use shorter timeouts or keep around extra waitNext threads they don't actually need.

I do think there is a minor race condition that would be good to address here, but it could also be addressed in a followup:

In current implementation if a client makes a waitNext call followed by an interruptWait call, because the waitNext call is asynchronous while the interruptWait call is synchronous, it means InterruptWait could set waiting = false before waitNext() sets waiting = true, and the interrupt might be ignored. This could be a real problem because the clients wouldn't have a good way of knowing whether a particular interruptWait call was effective and if they needed to make another one.

Would recommend changing the implementation slightly to avoid this. Could replace bool waiting{false} variable with an bool m_interrupt_wait{false} variable as eunovo suggested. And instead of having the WaitAndCreateNewBlock method set waiting = true when it is first called, instead of have it set interrupt_wait = false before it returns. This might make the WaitAndCreateNewBlock implementation a little more complicated, but should be nicer for clients because they can rely on interruptWait causing the current waitNext call to return even if hasn't started waiting yet.

DrahtBot requested review from Eunovo on Oct 27, 2025

DrahtBot requested review from furszy on Oct 27, 2025

interfaces: add interruptWait method

- This method can be used to cancel a running
  waitNext().

- This commit also adds a test case for interruptWait method

dcb56fd4cb

ismaelsadeeq force-pushed on Oct 27, 2025

ismaelsadeeq commented at 6:23 PM on October 27, 2025: member

Thanks @Eunovo and @ryanofsky

I've fixed the comments in latest push diff

furszy commented at 6:49 PM on October 27, 2025: member

Code ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3

nit: is there any way you could verify that there is someone waiting before setting interrupt_wait? It seems fragile to be able to interrupt a procedure that hasn't started.

DrahtBot requested review from ryanofsky on Oct 27, 2025

furszy commented at 7:48 PM on October 27, 2025: member

nit: is there any way you could verify that there is someone waiting before setting interrupt_wait? It seems fragile to be able to interrupt a procedure that hasn't started.

Quick idea; could provide an optional flag. So the value exists whenever someone is waiting on it and is destroyed (nullopt) when there is no one doing it.

Index: src/node/interfaces.cpp
===================================================================
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
--- a/src/node/interfaces.cpp	(revision dcb56fd4cb59e6857c110dd87019459989dc1ec3)
+++ b/src/node/interfaces.cpp	(date 1761593941511)
@@ -932,7 +932,7 @@
 
     const std::unique_ptr<CBlockTemplate> m_block_template;
 
-    bool m_interrupt_wait{false};
+    std::optional<bool> m_interrupt_wait{std::nullopt};
     ChainstateManager& chainman() { return *Assert(m_node.chainman); }
     KernelNotifications& notifications() { return *Assert(m_node.notifications); }
     NodeContext& m_node;
Index: src/node/miner.cpp
===================================================================
diff --git a/src/node/miner.cpp b/src/node/miner.cpp
--- a/src/node/miner.cpp	(revision dcb56fd4cb59e6857c110dd87019459989dc1ec3)
+++ b/src/node/miner.cpp	(date 1761594250147)
@@ -453,9 +453,10 @@
     block.hashMerkleRoot = BlockMerkleRoot(block);
 }
 
-void InterruptWait(KernelNotifications& kernel_notifications, bool& interrupt_wait)
+void InterruptWait(KernelNotifications& kernel_notifications, std::optional<bool>& interrupt_wait)
 {
     LOCK(kernel_notifications.m_tip_block_mutex);
+    if (!interrupt_wait) throw std::runtime_error("Interrupt requested while no wait operation is active");
     interrupt_wait = true;
     kernel_notifications.m_tip_block_cv.notify_all();
 }
@@ -466,7 +467,7 @@
                                                       const std::unique_ptr<CBlockTemplate>& block_template,
                                                       const BlockWaitOptions& options,
                                                       const BlockAssembler::Options& assemble_options,
-                                                      bool& interrupt_wait)
+                                                      std::optional<bool>& interrupt_wait)
 {
     // Delay calculating the current template fees, just in case a new block
     // comes in before the next tick.
@@ -483,6 +484,7 @@
         bool tip_changed{false};
         {
             WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock);
+            interrupt_wait = false;
             // Note that wait_until() checks the predicate before waiting
             kernel_notifications.m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
                 AssertLockHeld(kernel_notifications.m_tip_block_mutex);
@@ -491,10 +493,10 @@
                 // method on BlockTemplate and no template could have been
                 // generated before a tip exists.
                 tip_changed = Assume(tip_block) && tip_block != block_template->block.hashPrevBlock;
-                return tip_changed || chainman.m_interrupt || interrupt_wait;
+                return tip_changed || chainman.m_interrupt || (!interrupt_wait || interrupt_wait.value());
             });
             if (interrupt_wait) {
-                interrupt_wait = false;
+                interrupt_wait.reset();
                 return nullptr;
             }
         }
Index: src/node/miner.h
===================================================================
diff --git a/src/node/miner.h b/src/node/miner.h
--- a/src/node/miner.h	(revision dcb56fd4cb59e6857c110dd87019459989dc1ec3)
+++ b/src/node/miner.h	(date 1761594137149)
@@ -240,7 +240,7 @@
 
 
 /* Interrupt the current wait for the next block template. */
-void InterruptWait(KernelNotifications& kernel_notifications, bool& interrupt_wait);
+void InterruptWait(KernelNotifications& kernel_notifications, std::optional<bool>& interrupt_wait);
 /**
  * Return a new block template when fees rise to a certain threshold or after a
  * new tip; return nullopt if timeout is reached.
@@ -251,7 +251,7 @@
                                                       const std::unique_ptr<CBlockTemplate>& block_template,
                                                       const BlockWaitOptions& options,
                                                       const BlockAssembler::Options& assemble_options,
-                                                      bool& interrupt_wait);
+                                                      std::optional<bool>& interrupt_wait);
 
 /* Locks cs_main and returns the block hash and block height of the active chain if it exists; otherwise, returns nullopt.*/
 std::optional<BlockRef> GetTip(ChainstateManager& chainman);

Note: Haven't thought much about what would happen with multiple waiters. But it shouldn't be a problem.

Still, I might have gone too far and this might not be needed. Could enforce this rule at the API doc level too.

ryanofsky commented at 8:41 PM on October 27, 2025: contributor

re: #33676#pullrequestreview-3384985314, #33676 (comment)

+    if (!interrupt_wait) throw std::runtime_error("Interrupt requested while no wait operation is active");

I think this would just re-introduce the race condition we were trying to get rid of. The waitNext call is asynchronous and there's no way for a client to know if a waitNext is active or not. They can only know if they've made a call, not whether the IPC request has been sent to the socket, or whether server has received the request, or whether the server has started to execute the call after receiving it.

In theory, the server could send notifications to the client as execution progresses, or let the client query the current state, or throw exceptions in certain states, but these approaches would add complexity and expose clients to information they couldn't use reliably and probably shouldn't have. It should be simpler to just decide interruptWait calls will take precedence over waitNext calls and not care about the order of the calls like the current implementation in dcb56fd4cb59e6857c110dd87019459989dc1ec3.

I will say that current implementation will not work well if clients make multiple waitNext calls on the same block simultaneously, because clients will need to make separate interuptWait calls to cancel each waitNext, but the cancellations will be lost each time a waitNext call returns. A reasonable solution to this could be to change m_interrupt_wait from a bool to an int and increment it when interruptWait is called (instead of setting it to true) and decrement it when a waitNext call is interrupted (instead of setting it it to false). I think in practice though we are not expecting multiple waitNext calls on the same block, and could even potentially return an error if simultaneous calls are attempted.

ryanofsky approved

ryanofsky commented at 12:00 PM on October 28, 2025: contributor

Code review ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3, just tweaking semantics slightly since last review so if an interruptWait call is made shortly after a waitNext call it will reliably cause the waitNext call to return right away without blocking, even if the waitNext call had not begun to execute or wait yet.

furszy commented at 1:59 PM on October 28, 2025: member

Code review ACK https://github.com/bitcoin/bitcoin/commit/dcb56fd4cb59e6857c110dd87019459989dc1ec3

in test/functional/interface_ipc.py:198 in dcb56fd4cb

 193 | +
 194 | +            async def interrupt_wait():
 195 | +                await wait_started.wait() # Wait for confirmation wait started
 196 | +                await asyncio.sleep(0.1)  # Minimal buffer
 197 | +                template6.result.interruptWait()
 198 | +                miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])

Eunovo commented at 6:07 PM on October 28, 2025:

https://github.com/bitcoin/bitcoin/pull/33676/commits/dcb56fd4cb59e6857c110dd87019459989dc1ec3:

What's this line for?

ismaelsadeeq commented at 6:43 PM on October 28, 2025:

which line? you highlighted four lines.

Eunovo commented at 7:07 PM on October 28, 2025:

Line 198

ismaelsadeeq commented at 7:17 PM on October 28, 2025:

The line is important, it created a new transaction in the mempool it's what will make the test fail when we don't really interrupt a wait. Another thing that could be an indicator is also chain tip change.

in src/node/miner.cpp:497 in dcb56fd4cb

 490 | @@ -483,8 +491,12 @@ std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainma
 491 |                  // method on BlockTemplate and no template could have been
 492 |                  // generated before a tip exists.
 493 |                  tip_changed = Assume(tip_block) && tip_block != block_template->block.hashPrevBlock;
 494 | -                return tip_changed || chainman.m_interrupt;
 495 | +                return tip_changed || chainman.m_interrupt || interrupt_wait;
 496 |              });
 497 | +            if (interrupt_wait) {
 498 | +                interrupt_wait = false;

Eunovo commented at 6:21 PM on October 28, 2025:

https://github.com/bitcoin/bitcoin/pull/33676/commits/dcb56fd4cb59e6857c110dd87019459989dc1ec3:

The test doesn't break when this line is commented out.

ismaelsadeeq commented at 6:46 PM on October 28, 2025:

Interesting (that should never happen), can you share the logs and steps to reproduce? I can verify that it failed for me.

2025-10-28T18:42:24.302035Z TestFramework (INFO): PRNG seed is: 1841123518053179096
2025-10-28T18:42:24.302446Z TestFramework (INFO): Initializing test directory /var/folders/dj/d8p8jhd172n7wnq81ryfl6rc0000gn/T/bitcoin_func_test_kcqf80gj
2025-10-28T18:42:25.106568Z TestFramework (INFO): Running echo test
2025-10-28T18:42:25.113270Z TestFramework (INFO): Running mining test
2025-10-28T18:42:31.292648Z TestFramework (ERROR): Unexpected exception
Traceback (most recent call last):
  File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin-wait/test/functional/test_framework/test_framework.py", line 142, in main
    self.run_test()
    ~~~~~~~~~~~~~^^
  File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin-wait/build/test/functional/interface_ipc.py", line 255, in run_test
    self.run_mining_test()
    ~~~~~~~~~~~~~~~~~~~~^^
  File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin-wait/build/test/functional/interface_ipc.py", line 251, in run_mining_test
    asyncio.run(capnp.run(async_routine()))
    ~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Cellar/python@3.13/3.13.7/Frameworks/Python.framework/Versions/3.13/lib/python3.13/asyncio/runners.py", line 195, in run
    return runner.run(main)
           ~~~~~~~~~~^^^^^^
  File "/opt/homebrew/Cellar/python@3.13/3.13.7/Frameworks/Python.framework/Versions/3.13/lib/python3.13/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^
  File "/opt/homebrew/Cellar/python@3.13/3.13.7/Frameworks/Python.framework/Versions/3.13/lib/python3.13/asyncio/base_events.py", line 725, in run_until_complete
    return future.result()
           ~~~~~~~~~~~~~^^
  File "capnp/lib/capnp.pyx", line 1965, in run
  File "capnp/lib/capnp.pyx", line 1966, in capnp.lib.capnp.run
  File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin-wait/build/test/functional/interface_ipc.py", line 207, in async_routine
    assert_equal(result.to_dict(), {})
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin-wait/test/functional/test_framework/util.py", line 78, in assert_equal
    raise AssertionError("not(%s == %s)\n  in particular not(%s == %s)" % (thing1, thing2, d1, d2))
AssertionError: not({'result': <capnp.lib.capnp._DynamicCapabilityClient object at 0x104bea930>} == {})
  in particular not({'result': <capnp.lib.capnp._DynamicCapabilityClient object at 0x104bea930>} == {})
2025-10-28T18:42:31.355024Z TestFramework (INFO): Not stopping nodes as test failed. The dangling processes will be cleaned up later.
2025-10-28T18:42:31.355217Z TestFramework (WARNING): Not cleaning up dir /var/folders/dj/d8p8jhd172n7wnq81ryfl6rc0000gn/T/bitcoin_func_test_kcqf80gj
2025-10-28T18:42:31.355287Z TestFramework (ERROR): Test failed. Test logging available at /var/folders/dj/d8p8jhd172n7wnq81ryfl6rc0000gn/T/bitcoin_func_test_kcqf80gj/test_framework.log
2025-10-28T18:42:31.355462Z TestFramework (ERROR): 
2025-10-28T18:42:31.355687Z TestFramework (ERROR): Hint: Call /Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin-wait/test/functional/combine_logs.py '/var/folders/dj/d8p8jhd172n7wnq81ryfl6rc0000gn/T/bitcoin_func_test_kcqf80gj' to consolidate all logs
2025-10-28T18:42:31.355759Z TestFramework (ERROR): 
2025-10-28T18:42:31.355809Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2025-10-28T18:42:31.355892Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2025-10-28T18:42:31.355942Z TestFramework (ERROR): 
[node 0] Cleaning up leftover process

Eunovo commented at 7:05 PM on October 28, 2025:

Comment out line 497 and rebuild. The interface_ipc tests still pass successfully. They fail if you modify them to use two waitNext calls as shown below:

diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index 0c9de28da0..a3dc559008 100755
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -199,10 +199,14 @@ class IPCInterfaceTest(BitcoinTestFramework):
 
             wait_task = asyncio.create_task(wait_for_block())
             interrupt_task = asyncio.create_task(interrupt_wait())
+            wait2_task = asyncio.create_task(wait_for_block())
 
             result = await wait_task
             await interrupt_task
             assert_equal(result.to_dict(), {})
+            result2 = await wait2_task
+            block5 = await self.parse_and_deserialize_block(result2, ctx)
+            assert_equal(len(block5.vtx), 4)
 
             current_block_height = self.nodes[0].getchaintips()[0]["height"]
             check_opts = self.capnp_modules['mining'].BlockCheckOptions()

This verifies that the second waitNext call completes after the interrupt

ismaelsadeeq commented at 7:15 PM on October 28, 2025:

Thanks for your swift response, you highlighted multiple lines, so I commented them all that’s why it failed for me. It should pass because we still return (The issue with commenting that line is that subsequent wait for same block template will return instantly). We can add a test case to verify that after interrupting a wait, you can wait again.

Eunovo commented at 5:49 AM on November 1, 2025:

It should pass because we still return (The issue with commenting that line is that subsequent wait for same block template will return instantly). We can add a test case to verify that after interrupting a wait, you can wait again.

Yes. I commented out the line on purpose to discover ways to improve the tests.

in test/functional/interface_ipc.py:200 in dcb56fd4cb

 195 | +                await wait_started.wait() # Wait for confirmation wait started
 196 | +                await asyncio.sleep(0.1)  # Minimal buffer
 197 | +                template6.result.interruptWait()
 198 | +                miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])
 199 | +
 200 | +            wait_task = asyncio.create_task(wait_for_block())

Eunovo commented at 6:22 PM on October 28, 2025:

https://github.com/bitcoin/bitcoin/pull/33676/commits/dcb56fd4cb59e6857c110dd87019459989dc1ec3:

You can also add a test that interrupts before calling waitNext and check that waitNext returns immediately.

ismaelsadeeq commented at 6:47 PM on October 28, 2025:

Good idea, will do if retouching.

Eunovo commented at 6:24 PM on October 28, 2025: contributor

Looks Good. I left some comments on the tests.

DrahtBot requested review from Eunovo on Oct 28, 2025

Sjors commented at 11:28 AM on October 31, 2025: member

Concept ACK

Can there ever only be one waitNext() call on per BlockTemplate? If not, does interruptWait() just stop all of them?

ryanofsky commented at 2:05 PM on October 31, 2025: contributor

re: #33676 (comment)

Can there ever only be one waitNext() call on per BlockTemplate? If not, does interruptWait() just stop all of them?

There can be multiple waitNext() calls and the current implementation of interruptWait() will interrupt exactly one of them arbitrarily. I speculated on how this could be improved in #33676 (comment), if there is a use-case for multiple waitNext calls. But I don't think there is a good use-case, so it might be a good if the current interruptWait implemention is a soft discouragment for trying to use multiple simultaneous wait calls with the same block creation options from the same client.

Sjors commented at 2:09 PM on October 31, 2025: member

I also can't think of a use case, but maybe we should fail if a client (accidentally) tries?

ismaelsadeeq commented at 2:36 PM on October 31, 2025: member

Yes, I think it’s a good idea to prevent multiple calls to waitNext if there is no explicit use case, or have interruptWait identify and cancel a targeted wait?

ryanofsky commented at 2:53 PM on October 31, 2025: contributor

Yes, I think it’s a good idea to prevent multiple calls to waitNext if there is no explicit use case, or have interruptWait identify and cancel a targeted wait?

I think I like the current implementation more than either of these two ideas, because they both seem like they would bring unneeded complexity. It seems ok to allow multiple wait calls, since they should work perfectly well, even if there is not a very convenient way to cancel them. If it ever becomes important for interruptWait to provide other ways of cancelling it could be added when that becomes apparent.

Sjors referenced this in commit b4c7d9761a on Oct 31, 2025

Sjors commented at 2:56 PM on October 31, 2025: member

I successfully tested this PR against my Template Provider implementation, see https://github.com/Sjors/sv2-tp/pull/57.

Sjors referenced this in commit 8c2c9f661f on Oct 31, 2025

in test/functional/interface_ipc.py:187 in dcb56fd4cb

 181 | @@ -184,6 +182,28 @@ async def async_routine():
 182 |              template7 = await template6.result.waitNext(ctx, waitoptions)
 183 |              assert_equal(template7.to_dict(), {})
 184 |  
 185 | +            self.log.debug("interruptWait should abort the current wait")
 186 | +            wait_started = asyncio.Event()
 187 | +            async def wait_for_block():

Sjors commented at 3:26 PM on October 31, 2025:

nit: you could move these helper methods out of the test:

diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index 0c9de28da0..7a447eba92 100755
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -82,10 +82,20 @@ class IPCInterfaceTest(BitcoinTestFramework):
         coinbase_data = BytesIO((await block_template.result.getCoinbaseTx(ctx)).result)
         tx = CTransaction()
         tx.deserialize(coinbase_data)
         return tx

+    async def wait_for_block(self, template, waitoptions, ctx, wait_started):
+        wait_started.set()
+        return await template.result.waitNext(ctx, waitoptions)
+
+    async def interrupt_wait(self, template, miniwallet, wait_started):
+        await wait_started.wait() # Wait for confirmation wait started
+        await asyncio.sleep(0.1)  # Minimal buffer
+        template.result.interruptWait()
+        miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])
+
     def run_echo_test(self):
         self.log.info("Running echo test")
         async def async_routine():
             ctx, init = await self.make_capnp_init_ctx()
             self.log.debug("Create Echo proxy object")
@@ -182,25 +192,15 @@ class IPCInterfaceTest(BitcoinTestFramework):
             template7 = await template6.result.waitNext(ctx, waitoptions)
             assert_equal(template7.to_dict(), {})

             self.log.debug("interruptWait should abort the current wait")
             wait_started = asyncio.Event()
-            async def wait_for_block():
-                new_waitoptions = self.capnp_modules['mining'].BlockWaitOptions()
-                new_waitoptions.timeout = waitoptions.timeout * 60 # 1 minute wait
-                new_waitoptions.feeThreshold = 1
-                wait_started.set()
-                return await template6.result.waitNext(ctx, new_waitoptions)

-            async def interrupt_wait():
-                await wait_started.wait() # Wait for confirmation wait started
-                await asyncio.sleep(0.1)  # Minimal buffer
-                template6.result.interruptWait()
-                miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])
-
-            wait_task = asyncio.create_task(wait_for_block())
-            interrupt_task = asyncio.create_task(interrupt_wait())
+            waitoptions.timeout = waitoptions.timeout * 60 # 1 minute wait
+            waitoptions.feeThreshold = 1
+            wait_task = asyncio.create_task(self.wait_for_block(template6, waitoptions, ctx, wait_started))
+            interrupt_task = asyncio.create_task(self.interrupt_wait(template6, miniwallet, wait_started))

             result = await wait_task
             await interrupt_task
             assert_equal(result.to_dict(), {})

ismaelsadeeq commented at 6:09 PM on October 31, 2025:

Thanks will do when retouching.

in test/functional/interface_ipc.py:205 in dcb56fd4cb

 200 | +            wait_task = asyncio.create_task(wait_for_block())
 201 | +            interrupt_task = asyncio.create_task(interrupt_wait())
 202 | +
 203 | +            result = await wait_task
 204 | +            await interrupt_task
 205 | +            assert_equal(result.to_dict(), {})

Sjors commented at 3:29 PM on October 31, 2025:

Is this checking that the response to waitNext() is a null template?

ismaelsadeeq commented at 6:07 PM on October 31, 2025:

Yes.

in src/node/miner.cpp:496 in dcb56fd4cb

 490 | @@ -484,8 +491,12 @@ std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainma
 491 |                  // method on BlockTemplate and no template could have been
 492 |                  // generated before a tip exists.
 493 |                  tip_changed = Assume(tip_block) && tip_block != block_template->block.hashPrevBlock;
 494 | -                return tip_changed || chainman.m_interrupt;
 495 | +                return tip_changed || chainman.m_interrupt || interrupt_wait;
 496 |              });
 497 | +            if (interrupt_wait) {

Sjors commented at 3:51 PM on October 31, 2025:

It's somewhat unintuitive that we're waiting for a local variable to change. It might be more clear if we make a trivial struct TemplateNotifications and have this line check template_notifications.m_interrupt_wait instead. It's more clear that something called "notifications" can change from under us.

ryanofsky commented at 4:43 PM on October 31, 2025:

re: #33676 (review)

It's somewhat unintuitive that we're waiting for a local variable to change. It might be more clear if we make a trivial struct TemplateNotifications and have this line check template_notifications.m_interrupt_wait instead. It's more clear that something called "notifications" can change from under us.

I feel like it's not that confusing once you notice it's declared as outside reference (bool& interrupt_wait), but a struct could be useful. Another solution could be to make to use a pointer (bool* interrupt_wait) to make it more obvious that it points to something. Any approach seems fine to me.

ismaelsadeeq commented at 6:14 PM on October 31, 2025:

I’m a bit on the fence here a referenced parameter already signals that the value comes from elsewhere, also prevents nullptr input (must be initialized).

Sjors commented at 6:26 PM on October 31, 2025:

I also don't think a pointer is much more clear than a reference.

furszy commented at 6:34 PM on October 31, 2025:

Probably providing a "wait context" struct (which could be a shared pointer) would be slightly more appropriate, as it clearly indicates that it is specifically used in this workflow. Rather than a ref bool which is not so clear.

Still, we might be overthinking this. I think we can continue as is for now too.

Sjors commented at 3:52 PM on October 31, 2025: member

tACK dcb56fd4cb59e6857c110dd87019459989dc1ec3

I left some suggestions for extra clarity.

sedited approved

sedited commented at 2:24 PM on November 9, 2025: contributor

ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3

fanquake merged this on Nov 10, 2025

fanquake closed this on Nov 10, 2025

sedited referenced this in commit 94f024ad07 on Nov 11, 2025

yuvicc referenced this in commit f620a2206b on Nov 16, 2025

stringintech referenced this in commit 52fffa1e5d on Nov 24, 2025

plebhash commented at 5:51 PM on November 24, 2025: none

any chance this could get backported?

Sjors commented at 6:15 PM on November 24, 2025: member

No strong opinion on whether this should be backported. In sv2-tp I found it easy to work around by calling waitNext() in a loop with a reasonable timeout: https://github.com/stratum-mining/sv2-tp/blob/fbe0984ca21e2b6ae0f554bbc5b2819332e59afc/src/sv2/template_provider.cpp#L131-L144

plebhash commented at 6:57 PM on November 24, 2025: none

we also have workarounds on https://github.com/stratum-mining/sv2-apps/pull/59 but they're substantially increasing code complexity, which will likely have a negative impact on maintainability on the mid-long term

we have https://github.com/stratum-mining/sv2-apps/issues/81 to keep track of this but we'd really appreciate if this got backported instead of landing on v31+

fanquake referenced this in commit 39d53dd8bf on Nov 26, 2025

fanquake commented at 4:50 PM on November 26, 2025: member

Backported to 30.x in #33609.