This PR cleans up the interface_ipc.py test, fixing broken checks, fixing missing await calls, removing to_dict calls, renaming variables, reducing .result accesses, and giving template objects explicit lifetimes. More details are in the commit messages.
The first commit changes a lot of indentation so is easiest to review ignoring whitespace.
Use context managers to destroy block templates. Previously, block templates
were not being destroyed before disconnecting because the destroy coroutines
were called but never awaited. It's not necessary to explicitly destroy the
templates since they will get garbage collected asynchronously, but it's good
to destroy them to make the test more predictable, and to make the destroy
calls that are present actually do something.
This change also removes `await waitnext` expressions without changing
behavior, because the previous code was misleading about what order waitNext
calls were executed.
This change is easiest to review ignoring whitespace.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34003.
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, this pull request conflicts with the following ones:
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.
Destroy calls were being made at the end of the test instead of after templates were no longer needed.
I take advantage of that in #33922, but I’ll figure out a way to rebase if needed. E.g. I can just add another test with multiple templates.
I tested on macOS 26.1 that the tests still pass.
Can you split this into a few commits as it’s quite a long list of changes now.
Updated d179f713792432c82befd39a288ab16fec13bab1 -> 6ef092c0e4343fc421c323ff09d3c791fb4bc33a (pr/pyipc.1 -> pr/pyipc.2, compare) splitting into two commits
re: #34003 (comment)
Destroy calls were being made at the end of the test instead of after templates were no longer needed.
I take advantage of that in #33922, but I’ll figure out a way to rebase if needed. E.g. I can just add another test with multiple templates.
I think this shouldn’t change #33922 too much. You should be able to just add the check for nonzero memory usage inside the with block and the check for zero memory usage after it.
Can you split this into a few commits as it’s quite a long list of changes now.
Yes, I split off a minimal change just introducing the context managers as the first commit, since it changes a lot of indentation. It should be pretty easy to review ignoring whitespace. The second commit contains all the other changes and should be more straightforward now. I could split it up more if desired, but it might be actually more work to review if the same lines change multiple times, so I kept it together for now.
ACK 6ef092c0e4343fc421c323ff09d3c791fb4bc33a
Thanks for the cleanup! Have another suggestion for saving a few lines and naming clarity:
0diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
1index 804f7f8f64..f8f3a76ec7 100755
2--- a/test/functional/interface_ipc.py
3+++ b/test/functional/interface_ipc.py
4@@ -176,2 +176 @@ class IPCInterfaceTest(BitcoinTestFramework):
5- template3 = await template2.waitNext(ctx, waitoptions)
6- assert_equal(template3._has("result"), False)
7+ assert_equal((await template2.waitNext(ctx, waitoptions))._has("result"), False)
8@@ -181,2 +180,2 @@ class IPCInterfaceTest(BitcoinTestFramework):
9- template4 = await stack.enter_async_context(destroying((await template2.waitNext(ctx, waitoptions)).result, ctx))
10- block3 = await self.parse_and_deserialize_block(template4, ctx)
11+ template3 = await stack.enter_async_context(destroying((await template2.waitNext(ctx, waitoptions)).result, ctx))
12+ block3 = await self.parse_and_deserialize_block(template3, ctx)
13@@ -187,2 +186,2 @@ class IPCInterfaceTest(BitcoinTestFramework):
14- template5 = await stack.enter_async_context(destroying((await template4.waitNext(ctx, waitoptions)).result, ctx))
15- block4 = await self.parse_and_deserialize_block(template5, ctx)
16+ template4 = await stack.enter_async_context(destroying((await template3.waitNext(ctx, waitoptions)).result, ctx))
17+ block4 = await self.parse_and_deserialize_block(template4, ctx)
18@@ -194,3 +193,3 @@ class IPCInterfaceTest(BitcoinTestFramework):
19- template6 = await stack.enter_async_context(destroying((await template5.waitNext(ctx, waitoptions)).result, ctx))
20- block4 = await self.parse_and_deserialize_block(template6, ctx)
21- assert_equal(len(block4.vtx), 3)
22+ template5 = await stack.enter_async_context(destroying((await template4.waitNext(ctx, waitoptions)).result, ctx))
23+ block5 = await self.parse_and_deserialize_block(template5, ctx)
24+ assert_equal(len(block5.vtx), 3)
25@@ -199,2 +198 @@ class IPCInterfaceTest(BitcoinTestFramework):
26- template7 = await template6.waitNext(ctx, waitoptions)
27- assert_equal(template7._has("result"), False)
28+ assert_equal((await template5.waitNext(ctx, waitoptions))._has("result"), False)
29@@ -209,2 +207 @@ class IPCInterfaceTest(BitcoinTestFramework):
30- template7 = await template6.waitNext(ctx, new_waitoptions)
31- assert_equal(template7._has("result"), False)
32+ assert_equal((await template5.waitNext(ctx, new_waitoptions))._has("result"), False)
33@@ -215 +212 @@ class IPCInterfaceTest(BitcoinTestFramework):
34- template6.interruptWait()
35+ template5.interruptWait()
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
templatevariable was assigned twice but only (attempted to be) destroyed once. Fix these problems by using pythonwith: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.
237- res = await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())
238- assert_equal(res.result, False)
239+ check = await mining.checkBlock(block.serialize(), check_opts)
240+ assert_equal(check.result, False)
241+ assert_equal(check.reason, "bad-version(0x00000000)")
242+ submit = await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())
In 6ef092c0e4343fc421c323ff09d3c791fb4bc33a “test: interface_ipc.py minor fixes and cleanup”
A lot of result accesses like
template.resultmining.resultwere repeated unnecessarily because variables liketemplateandminingwere assigned response objects instead of result objects. These variables are now changed to point directly to results.
Along the same lines:
0diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
1index 90e22285e6..756c115c7e 100755
2--- a/test/functional/interface_ipc.py
3+++ b/test/functional/interface_ipc.py
4@@ -234,23 +234,23 @@ class IPCInterfaceTest(BitcoinTestFramework):
5 check = await mining.checkBlock(block.serialize(), check_opts)
6 assert_equal(check.result, False)
7 assert_equal(check.reason, "bad-version(0x00000000)")
8- submit = await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())
9- assert_equal(submit.result, False)
10+ submitted = (await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())).result
11+ assert_equal(submitted, False)
12 self.log.debug("Submit a valid block")
13 block.nVersion = original_version
14 block.solve()
15
16 self.log.debug("First call checkBlock()")
17- check = await mining.checkBlock(block.serialize(), check_opts)
18- assert_equal(check.result, True)
19+ block_valid = (await mining.checkBlock(block.serialize(), check_opts)).result
20+ assert_equal(block_valid, True)
21
22 # The remote template block will be mutated, capture the original:
23 remote_block_before = await self.parse_and_deserialize_block(template, ctx)
24
25 self.log.debug("Submitted coinbase must include witness")
26 assert_not_equal(coinbase.serialize_without_witness().hex(), coinbase.serialize().hex())
27- submit = await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize_without_witness())
28- assert_equal(submit.result, False)
29+ submitted = (await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize_without_witness())).result
30+ assert_equal(submitted, False)
31
32 self.log.debug("Even a rejected submitBlock() mutates the template's block")
33 # Can be used by clients to download and inspect the (rejected)
34@@ -259,8 +259,8 @@ class IPCInterfaceTest(BitcoinTestFramework):
35 assert_not_equal(remote_block_before.serialize().hex(), remote_block_after.serialize().hex())
36
37 self.log.debug("Submit again, with the witness")
38- submit = await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())
39- assert_equal(submit.result, True)
40+ submitted = (await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())).result
41+ assert_equal(submitted, True)
42
43 self.log.debug("Block should propagate")
44 # Check that the IPC node actually updates its own chain
re: #34003 (review)
Make sense, applied the suggestion
Concept ACK 6ef092c0e4343fc421c323ff09d3c791fb4bc33a
I’m new to this test, I built the bitcoin-node executable using -DENABLE_IPC=ON and ran the test using venv as mentioned in the readme file. I spent some time trying to understand the interfaces involving capnp.
The changes seem to clean the test quite a bit and makes it easier to go through.
Concept ACK #34003
I ran the interface_ipc.py test locally and it passes. The PR cleans up the test by fixing broken checks, missing await calls, and variable naming.
I’m new to PR reviews, but from what I can tell, the changes seem correct and improve readability. I don’t see any issues with the changes.
199+ self.log.debug("Create a template")
200+ opts = self.capnp_modules['mining'].BlockCreateOptions()
201+ opts.useMempool = True
202+ opts.blockReservedWeight = 4000
203+ opts.coinbaseOutputMaxAdditionalSigops = 0
204+ template = await stack.enter_async_context(destroying((await mining.result.createNewBlock(opts)).result, ctx))
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:
0diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
1index 3d28bba136..e01c753d24 100755
2--- a/test/functional/interface_ipc.py
3+++ b/test/functional/interface_ipc.py
4@@ -96,6 +96,14 @@ class IPCInterfaceTest(BitcoinTestFramework):
5 tx.deserialize(coinbase_data)
6 return tx
7
8+ async def create_block_template(self, stack, ctx, mining, opts):
9+ # Destroyed when stack exits.
10+ return await stack.enter_async_context(destroying((await mining.result.createNewBlock(opts)).result, ctx))
11+
12+ async def wait_next_template(self, stack, ctx, template, waitoptions):
13+ # Destroyed when stack exits.
14+ return await stack.enter_async_context(destroying((await template.waitNext(ctx, waitoptions)).result, ctx))
15+
16 def run_echo_test(self):
17 self.log.info("Running echo test")
18 async def async_routine():
19@@ -148,7 +156,7 @@ class IPCInterfaceTest(BitcoinTestFramework):
20 opts.useMempool = True
21 opts.blockReservedWeight = 4000
22 opts.coinbaseOutputMaxAdditionalSigops = 0
23- template = await stack.enter_async_context(destroying((await mining.result.createNewBlock(opts)).result, ctx))
24+ template = await self.create_block_template(stack, ctx, mining, opts)
25 self.log.debug("Test some inspectors of Template")
26 header = await template.getBlockHeader(ctx)
27 assert_equal(len(header.result), block_header_size)
28@@ -168,7 +176,7 @@ class IPCInterfaceTest(BitcoinTestFramework):
29 waitoptions.timeout = timeout
30 waitoptions.feeThreshold = 1
31 self.generate(self.nodes[0], 1)
32- template2 = await stack.enter_async_context(destroying((await template.waitNext(ctx, waitoptions)).result, ctx))
33+ template2 = await self.wait_next_template(stack, ctx, template, waitoptions)
34 block2 = await self.parse_and_deserialize_block(template2, ctx)
35 assert_equal(len(block2.vtx), 1)
36 self.log.debug("Wait for another, but time out")
37@@ -176,18 +184,18 @@ class IPCInterfaceTest(BitcoinTestFramework):
38 assert_equal(template3.to_dict(), {})
39 self.log.debug("Wait for another, get one after increase in fees in the mempool")
40 miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])
41- template4 = await stack.enter_async_context(destroying((await template2.waitNext(ctx, waitoptions)).result, ctx))
42+ template4 = await self.wait_next_template(stack, ctx, template2, waitoptions)
43 block3 = await self.parse_and_deserialize_block(template4, ctx)
44 assert_equal(len(block3.vtx), 2)
45 self.log.debug("Wait again, this should return the same template, since the fee threshold is zero")
46 waitoptions.feeThreshold = 0
47- template5 = await stack.enter_async_context(destroying((await template4.waitNext(ctx, waitoptions)).result, ctx))
48+ template5 = await self.wait_next_template(stack, ctx, template4, waitoptions)
49 block4 = await self.parse_and_deserialize_block(template5, ctx)
50 assert_equal(len(block4.vtx), 2)
51 waitoptions.feeThreshold = 1
52 self.log.debug("Wait for another, get one after increase in fees in the mempool")
53 miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])
54- template6 = await stack.enter_async_context(destroying((await template5.waitNext(ctx, waitoptions)).result, ctx))
55+ template6 = await self.wait_next_template(stack, ctx, template5, waitoptions)
56 block4 = await self.parse_and_deserialize_block(template6, ctx)
57 assert_equal(len(block4.vtx), 3)
58 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
219+ self.log.debug("Wait for a new template")
220+ waitoptions = self.capnp_modules['mining'].BlockWaitOptions()
221+ waitoptions.timeout = timeout
222+ waitoptions.feeThreshold = 1
223+ self.generate(self.nodes[0], 1)
224+ template2 = await stack.enter_async_context(destroying((await template.waitNext(ctx, waitoptions)).result, ctx))
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
generateinterrupts an ongoingwaitNext().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:
0waitnext = template.result.waitNext(ctx, waitoptions)
1self.generate(self.nodes[0], 1)
2template2 = await waitnext
new code is:
0self.generate(self.nodes[0], 1)
1template2 = 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.
237+ template5 = await stack.enter_async_context(destroying((await template4.waitNext(ctx, waitoptions)).result, ctx))
238+ block4 = await self.parse_and_deserialize_block(template5, ctx)
239+ assert_equal(len(block4.vtx), 2)
240+ waitoptions.feeThreshold = 1
241+ self.log.debug("Wait for another, get one after increase in fees in the mempool")
242+ miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])
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:
0waitnext = template5.result.waitNext(ctx, waitoptions)
1miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])
2template6 = await waitnext
So waitNext call would not even begin to be executed until after the transfer, equivalent to the new code:
0miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])
1template6 = 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.
There are a few things that are incorrect or messy in the interface_ipc.py test.
This commit tries to clean them up:
- isTestChain and isInitialBlockDownload asserts were not checking the results
of those calls, only that calls were, made because they were not checking the
responses' .result member.
- 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.
- Some coroutine calls were assigned to temporary `wait` before being awaited.
This was unnecessarily confusing and would make code not run in top-down
order.
- `to_dict` calls were being made to check if result variables were unset. This
was inefficient and indirect because it iterates over all fields in response
structs instead of just checking whether the result field is present. The
to_dict calls are now replaced with more direct `_has('result')` calls.
- The `res` variables used to hold various responses did not have descriptive
names. These are replaced with clearer names.
Co-authored-by: rkrux <rkrux.connect@gmail.com>
As pointed out by Sjors in
https://github.com/bitcoin/bitcoin/pull/34003#discussion_r2598844209 and
https://github.com/bitcoin/bitcoin/pull/34003#discussion_r2598858386 the
original intention of having waitNext and waitTipChanged calls in the test was
to ensure that if new blocks were connected or fees were increased *during* the
waits, that the calls would wake up and return.
But the tests were written incorrectly, to generate blocks and transactions
before the wait calls instead of during the calls. So the tests were less
meaningful then they should be.
There was also a similar problem in the interruptWait test. The test was
intended to test the interruptWait method, but it was never actually calling
the method due to a missing await keyword. Instead it was testing that
miniwallet.send_self_transfer would interrupt the wait.
This commit fixes these issues by introducing a wait_and_do() helper function
to start parallel tasks and trigger an action after a wait call is started.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
Thanks for the reviews!
Updated 6ef092c0e4343fc421c323ff09d3c791fb4bc33a -> c358910b8cd6d1db51c10becb0cbcb58709b738f (pr/pyipc.2 -> pr/pyipc.3, compare) applying suggestions and adding a new commit to test actions during waitNext calls instead of before them.
I tried to do the same thing with the waitTipChanged call as well, but for some reason waitTipChanged always seems to return right away even when the tip has not changed, and I’m not sure why that is, so I left it alone for now. Change I tried was:
0- self.generate(self.nodes[0], 1)
1- newblockref = (await mining.waitTipChanged(ctx, blockref.result.hash)).result
2+ newblockref = (await wait_and_do(mining.waitTipChanged(ctx, blockref.result.hash), lambda: self.generate(self.nodes[0], 1))).result
ACK a5e61b1917aff7567599beb59cc7093978b7e336
Though it would be nice to also fix this:
for some reason
waitTipChangedalways seems to return right away even when the tip has not changed
Because the timeout argument defaults to 0 when called from Python.
0diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
1index 913aea16de..5242d61c1e 100755
2--- a/test/functional/interface_ipc.py
3+++ b/test/functional/interface_ipc.py
4@@ -162,8 +162,10 @@ class IPCInterfaceTest(BitcoinTestFramework):
5 current_block_height = self.nodes[0].getchaintips()[0]["height"]
6 assert blockref.result.height == current_block_height
7 self.log.debug("Mine a block")
8- self.generate(self.nodes[0], 1)
9- newblockref = (await mining.waitTipChanged(ctx, blockref.result.hash)).result
10+ newblockref = (await wait_and_do(
11+ mining.waitTipChanged(ctx, blockref.result.hash, timeout),
12+ lambda: self.generate(self.nodes[0], 1),
13+ )).result
14 assert_equal(len(newblockref.hash), block_hash_size)
15 assert_equal(newblockref.height, current_block_height + 1)
16 self.log.debug("Wait for timeout")
(I find it more readable to have the wait and do arguments on separate lines)
Nice suggestion!
Updated c358910b8cd6d1db51c10becb0cbcb58709b738f -> d8fe5f0326c5c0505e0f7e6e978961d24c27b175 (pr/pyipc.3 -> pr/pyipc.4, compare) applying waitTipChanged suggestion and reformatting wait_and_do calls
57+ if inspect.isawaitable(do_fn):
58+ await do_fn
59+ else:
60+ do_fn()
61+
62+ await asyncio.gather(wait(), do())
In d8fe5f0326c5c0505e0f7e6e978961d24c27b175 “test: improve interface_ipc.py waitNext tests”
All are nits now I believe.
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,
From an event’s POV, it is set in wait() and waited for in do(); suggested s/wait/setter and s/do/waiter.
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.
0diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
1index 7c8f51a81c..0da75d25cd 100755
2--- a/test/functional/interface_ipc.py
3+++ b/test/functional/interface_ipc.py
4@@ -40,18 +40,18 @@ async def wait_next_template(template, stack, ctx, opts):
5 return await stack.enter_async_context(destroying((await template.waitNext(ctx, opts)).result, ctx))
6
7 async def wait_and_do(wait_fn, do_fn):
8- """Call wait_fn, then sleep, then call do_fn in a parallel task. Wait for
9- both tasks to complete."""
10- wait_started = asyncio.Event()
11+ """Call wait_fn and do_fn in parallel tasks while ensuring do_fn is
12+ called after wait_fn is started. Wait for both tasks to complete."""
13+ event_over = asyncio.Event()
14 result = None
15
16- async def wait():
17+ async def setter():
18 nonlocal result
19- wait_started.set()
20+ event_over.set()
21 result = await wait_fn
22
23- async def do():
24- await wait_started.wait()
25+ async def waiter():
26+ await event_over.wait()
27 await asyncio.sleep(0.1)
28 # Let do_fn be either a callable or an awaitable object
29 if inspect.isawaitable(do_fn):
30@@ -59,7 +59,7 @@ async def wait_and_do(wait_fn, do_fn):
31 else:
32 do_fn()
33
34- await asyncio.gather(wait(), do())
35+ await asyncio.gather(setter(), waiter())
36 return result
37
38 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.
- I don’t suppose the following is guaranteed? When event is set, then the wait of
dois 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:
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.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.
Post-merge now as it got merged while I was reviewing.
ACK d8fe5f0326c5c0505e0f7e6e978961d24c27b175
I have focussed only on the changes in this PR and have limited understanding of the mining/ipc domain.