test: interface_ipc.py minor fixes and cleanup #34003

In 6ef092c0e4343fc421c323ff09d3c791fb4bc33a "test: interface_ipc.py minor fixes and cleanup"

In commit message:

Destroy calls were being made at the end of the test instead of after templates were no longer needed. The template variable was assigned twice but only (attempted to be) destroyed once. Fix these problems by using python with: blocks

Wasn't this done in the first commit fac12f7cf9ebc0b906bbb20ae126cf7e7552fdbb?

re: #34003 (review)

Thanks, fixed message. I forgot to update it when the commit was split up.

In 6ef092c0e4343fc421c323ff09d3c791fb4bc33a "test: interface_ipc.py minor fixes and cleanup"

A lot of result accesses like template.result mining.result were repeated unnecessarily because variables like template and mining were assigned response objects instead of result objects. These variables are now changed to point directly to results.

Along the same lines:

diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index 90e22285e6..756c115c7e 100755
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -234,23 +234,23 @@ class IPCInterfaceTest(BitcoinTestFramework):
                 check = await mining.checkBlock(block.serialize(), check_opts)
                 assert_equal(check.result, False)
                 assert_equal(check.reason, "bad-version(0x00000000)")
-                submit = await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())
-                assert_equal(submit.result, False)
+                submitted = (await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())).result
+                assert_equal(submitted, False)
                 self.log.debug("Submit a valid block")
                 block.nVersion = original_version
                 block.solve()
 
                 self.log.debug("First call checkBlock()")
-                check = await mining.checkBlock(block.serialize(), check_opts)
-                assert_equal(check.result, True)
+                block_valid = (await mining.checkBlock(block.serialize(), check_opts)).result
+                assert_equal(block_valid, True)
 
                 # The remote template block will be mutated, capture the original:
                 remote_block_before = await self.parse_and_deserialize_block(template, ctx)
 
                 self.log.debug("Submitted coinbase must include witness")
                 assert_not_equal(coinbase.serialize_without_witness().hex(), coinbase.serialize().hex())
-                submit = await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize_without_witness())
-                assert_equal(submit.result, False)
+                submitted = (await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize_without_witness())).result
+                assert_equal(submitted, False)
 
                 self.log.debug("Even a rejected submitBlock() mutates the template's block")
                 # Can be used by clients to download and inspect the (rejected)
@@ -259,8 +259,8 @@ class IPCInterfaceTest(BitcoinTestFramework):
                 assert_not_equal(remote_block_before.serialize().hex(), remote_block_after.serialize().hex())
 
                 self.log.debug("Submit again, with the witness")
-                submit = await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())
-                assert_equal(submit.result, True)
+                submitted = (await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())).result
+                assert_equal(submitted, True)
 
             self.log.debug("Block should propagate")
             # Check that the IPC node actually updates its own chain

re: #34003 (review)

Make sense, applied the suggestion

In fac12f7cf9ebc0b906bbb20ae126cf7e7552fdbb test: fix interface_ipc.py template destruction: await stack.enter_async_context(destroying(( is hard to read, so maybe move it into a helper:

diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index 3d28bba136..e01c753d24 100755
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -96,6 +96,14 @@ class IPCInterfaceTest(BitcoinTestFramework):
         tx.deserialize(coinbase_data)
         return tx

+    async def create_block_template(self, stack, ctx, mining, opts):
+        # Destroyed when stack exits.
+        return await stack.enter_async_context(destroying((await mining.result.createNewBlock(opts)).result, ctx))
+
+    async def wait_next_template(self, stack, ctx, template, waitoptions):
+        # Destroyed when stack exits.
+        return await stack.enter_async_context(destroying((await template.waitNext(ctx, waitoptions)).result, ctx))
+
     def run_echo_test(self):
         self.log.info("Running echo test")
         async def async_routine():
@@ -148,7 +156,7 @@ class IPCInterfaceTest(BitcoinTestFramework):
                 opts.useMempool = True
                 opts.blockReservedWeight = 4000
                 opts.coinbaseOutputMaxAdditionalSigops = 0
-                template = await stack.enter_async_context(destroying((await mining.result.createNewBlock(opts)).result, ctx))
+                template = await self.create_block_template(stack, ctx, mining, opts)
                 self.log.debug("Test some inspectors of Template")
                 header = await template.getBlockHeader(ctx)
                 assert_equal(len(header.result), block_header_size)
@@ -168,7 +176,7 @@ class IPCInterfaceTest(BitcoinTestFramework):
                 waitoptions.timeout = timeout
                 waitoptions.feeThreshold = 1
                 self.generate(self.nodes[0], 1)
-                template2 = await stack.enter_async_context(destroying((await template.waitNext(ctx, waitoptions)).result, ctx))
+                template2 = await self.wait_next_template(stack, ctx, template, waitoptions)
                 block2 = await self.parse_and_deserialize_block(template2, ctx)
                 assert_equal(len(block2.vtx), 1)
                 self.log.debug("Wait for another, but time out")
@@ -176,18 +184,18 @@ class IPCInterfaceTest(BitcoinTestFramework):
                 assert_equal(template3.to_dict(), {})
                 self.log.debug("Wait for another, get one after increase in fees in the mempool")
                 miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])
-                template4 = await stack.enter_async_context(destroying((await template2.waitNext(ctx, waitoptions)).result, ctx))
+                template4 = await self.wait_next_template(stack, ctx, template2, waitoptions)
                 block3 = await self.parse_and_deserialize_block(template4, ctx)
                 assert_equal(len(block3.vtx), 2)
                 self.log.debug("Wait again, this should return the same template, since the fee threshold is zero")
                 waitoptions.feeThreshold = 0
-                template5 = await stack.enter_async_context(destroying((await template4.waitNext(ctx, waitoptions)).result, ctx))
+                template5 = await self.wait_next_template(stack, ctx, template4, waitoptions)
                 block4 = await self.parse_and_deserialize_block(template5, ctx)
                 assert_equal(len(block4.vtx), 2)
                 waitoptions.feeThreshold = 1
                 self.log.debug("Wait for another, get one after increase in fees in the mempool")
                 miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])
-                template6 = await stack.enter_async_context(destroying((await template5.waitNext(ctx, waitoptions)).result, ctx))
+                template6 = await self.wait_next_template(stack, ctx, template5, waitoptions)
                 block4 = await self.parse_and_deserialize_block(template6, ctx)
                 assert_equal(len(block4.vtx), 3)
                 self.log.debug("Wait for another, but time out, since the fee threshold is set now")

re: #34003 (review)

Nice suggestion. Applied with a few tweaks

In fac12f7cf9ebc0b906bbb20ae126cf7e7552fdbb test: fix interface_ipc.py template destruction:

I think the original intention here was to demonstrate that generate interrupts an ongoing waitNext().

Now it only shows that waitNext() returns immediately if the tip has changes before calling. Though perhaps it always did that by accident.

re: #34003 (review)

I think the original intention here was to demonstrate that generate interrupts an ongoing waitNext().

Now it only shows that waitNext() returns immediately if the tip has changes before calling. Though perhaps it always did that by accident.

Yes behavior should be unchanged. Original code was:

waitnext = template.result.waitNext(ctx, waitoptions)
self.generate(self.nodes[0], 1)
template2 = await waitnext

new code is:

self.generate(self.nodes[0], 1)
template2 = await stack.enter_async_context(destroying((await template.waitNext(ctx, waitoptions)).result, ctx))

In both cases, the waitNext() request would never be made until after the generate call because waitNext is an async method that's not executed until it's awaited. In order to actually test that a new block interrupts the waitNext the code would need to do something like the interruptWait test is doing and start two asynchronous tasks that run in parallel: one starting the wait right away, and the other sleeping for a short amount of time and calling generate.

I guess it's worth doing that in this PR in a followup commit, even though this commit is a refactoring.

I should also add a note about why this commit is not changing behavior in the commit message.

re: #34003 (review)

Added a new commit to make this check work as originally intended and call generate during the wait instead of before.

In https://github.com/bitcoin/bitcoin/commit/fac12f7cf9ebc0b906bbb20ae126cf7e7552fdbb test: fix interface_ipc.py template destruction: similar to my comment above, this only shows that waitNext() returns immediately if fees have gone up before the call, but the original intent was to show that a fee increase causes the wait to be interrupted.

re: #34003 (review)

In fac12f7 test: fix interface_ipc.py template destruction: similar to my comment above, this only shows that waitNext() returns immediately if fees have gone up before the call, but the original intent was to show that a fee increase causes the wait to be interrupted.

Makes sense. Again there should be no change of behavior in this commit, but this is worth following up on.

Previous code was:

waitnext = template5.result.waitNext(ctx, waitoptions)
miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])
template6 = await waitnext

So waitNext call would not even begin to be executed until after the transfer, equivalent to the new code:

miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])
template6 = await stack.enter_async_context(destroying((await template5.waitNext(ctx, waitoptions)).result, ctx))

re: #34003 (review)

Added a new commit to make this check work as originally intended and send the transaction during the wait instead of before.

In d8fe5f0326c5c0505e0f7e6e978961d24c27b175 "test: improve interface_ipc.py waitNext tests"

All are nits now I believe.

1. I don't suppose the following is guaranteed? When event is set, then the wait of do is also over that makes it call the sleep in parallel. Reworded slightly to highlight the broader intent of the wait_and_do function.

Call wait_fn, then sleep,

2. From an event's POV, it is set in wait() and waited for in do(); suggested s/wait/setter and s/do/waiter.

3. s/wait_started/event_over because I got confused by wait_started.set() that could also suggest that the event wait starts now when instead it gets over at this stage.

diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index 7c8f51a81c..0da75d25cd 100755
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -40,18 +40,18 @@ async def wait_next_template(template, stack, ctx, opts):
     return await stack.enter_async_context(destroying((await template.waitNext(ctx, opts)).result, ctx))
 
 async def wait_and_do(wait_fn, do_fn):
-    """Call wait_fn, then sleep, then call do_fn in a parallel task. Wait for
-    both tasks to complete."""
-    wait_started = asyncio.Event()
+    """Call wait_fn and do_fn in parallel tasks while ensuring do_fn is
+    called after wait_fn is started. Wait for both tasks to complete."""
+    event_over = asyncio.Event()
     result = None
 
-    async def wait():
+    async def setter():
         nonlocal result
-        wait_started.set()
+        event_over.set()
         result = await wait_fn
 
-    async def do():
-        await wait_started.wait()
+    async def waiter():
+        await event_over.wait()
         await asyncio.sleep(0.1)
         # Let do_fn be either a callable or an awaitable object
         if inspect.isawaitable(do_fn):
@@ -59,7 +59,7 @@ async def wait_and_do(wait_fn, do_fn):
         else:
             do_fn()
 
-    await asyncio.gather(wait(), do())
+    await asyncio.gather(setter(), waiter())
     return result
 
 class IPCInterfaceTest(BitcoinTestFramework):

re: #34003 (review)

Thanks for looking into this. The async code here is pretty confusing, so it's good to think through what it's doing and try to clarify things.

1. I don't suppose the following is guaranteed? When event is set, then the wait of do is also over that makes it call the sleep in parallel.

Actually the opposite should be true and setting the event happens first. When the event is set, wait_fn has not been awaited yet (meaning no waitNext or waitTipChange call has started), the sleep hasn't started yet, and do_fn has not been called.

In practice two orders are possible:

1. waitNext or waitTipChange call starts, and the sleep(0.1) is long enough that do_fn() is called during the remote wait and interrupts it, causing it to return.
2. waitNext or waitTipChange call starts, but the request is slow or the sleep(0.1) is short enough that do_fn() runs first and causes the remote wait to return right away without even blocking.

A third scenario where the remote wait times out is also technically possible but should not happen because the wait timeout should be significantly longer than the sleep(0.1) here.

The main purpose of the wait_started event is to make it impossible for do_fn() to be called before the wait() coroutine starts executing, which would be bad because do_fn() can do anything and can potentially block the thread.

Using wait_started also makes it more likely that the do_fn() runs after wait_fn() has started waiting, but it can't guarantee this, because our wait functions to not provide any signal of when they have begun to wait.

Looking at the suggested diff, I think "ensuring do_fn is called after wait_fn is started" would not be accurate because we can't ensure that. Similarly setter is probably not as a good a name as wait since main purpose of the coroutine is to start the wait, and setting the event is an implementation detail. Also the event is set when the wait should be starting, not when it is over so wait_started is probably a better name than event_over.