This adds coverage for when BlockWaitOptions's feeThreshold is set to MAX_MONEY. In other words, when the IPC client is only interested in tip updates, not in fee increases.
This tested ended up in #33922 at some point #33922 (comment), but doesn't really belong there.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35598.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Stale ACK | enirox001 |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
In 4f74b73 (test: cover feeThreshold = MAX_MONEY)
Would be nice if there were a comment explaining the assertion here. Something like
If waitNext had returned for the fee increase instead of the tip update,
this template would include the mempool transaction.
ACK 4f74b73
Mostly reviewing this in isolation, as I have not looked at #33922 in depth, it makes sense that this should be a PR on its own
The change updates the functional test to explicitly ignore mempool fee increases while waiting for a tip update, then restores the lower fee threshold for subsequent checks.
These changes look good to me. Left a minor non-blocking suggestion below,
Added the suggested comment.
as I have not looked at #33922 in depth, it makes sense that this should be a PR on its own
It's unrelated. That PR is about memory tracking and management, this test is about adding coverages to pre-existing waitNext() behavior. The PR touches the same test file, so it was easy to add. But now that I'm considering alternative approaches for #33922, the test becomes a distraction - and it might be a long time before it lands.
272 | + waitoptions.feeThreshold = MAX_MONEY 273 | + self.miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0]) 274 | template2 = await wait_and_do( 275 | mining_wait_next_template(template, stack, ctx, waitoptions), 276 | + # This mines the transaction, so it won't be in the next template 277 | lambda: self.generate(self.nodes[0], 1))
https://github.com/bitcoin/bitcoin/pull/35598/changes/bd1771fc0cd248438e87b028b096f59ffc976d18 (test: cover feeThreshold = MAX_MONEY)
The test still passes with waitoptions.feeThreshold = 1
That is because WaitAndCreateNewBlock() checks for a tip update immediately and then goes to sleep waiting for a tip update, only waking up 1 second after being called or at the timeout deadline before checking for a fee-based update:
and wait_and_do only sleeps for 0.1s:
so the fee check never even has a chance, since WaitAndCreateNewBlock() is still asleep waiting for a new tip when wait_and_do produces one, returns, and the fee check has never even had a chance..
To fix it, easier shown than explained I think:
- waitoptions.timeout = self.default_ipc_timeout
+
+ # Temporarily increase the timeout to allow a fee ticks (1s) to
+ # happen before the deadline
+ waitoptions.timeout = 2000
# Ignore fee increases, wait only for the tip update
waitoptions.feeThreshold = MAX_MONEY
self.miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])
template2 = await wait_and_do(
mining_wait_next_template(template, stack, ctx, waitoptions),
# This mines the transaction, so it won't be in the next template
- lambda: self.generate(self.nodes[0], 1))
+ lambda: self.generate(self.nodes[0], 1),
+ sleep_time=1.1)
(because of my other suggestion I've omitted setting waitoptions.timeout back to
self.default_ipc_timeoutin this diff but that needs to be done at some point)
and adding an arg to wait_and_do:
--- a/test/functional/test_framework/ipc_util.py
+++ b/test/functional/test_framework/ipc_util.py
@@ -45,7 +45,7 @@ async def destroying(obj, ctx):
await obj.destroy(ctx)
-async def wait_and_do(wait_fn, do_fn):
+async def wait_and_do(wait_fn, do_fn, sleep_time=0.1):
"""Call wait_fn, then sleep, then call do_fn in a parallel task. Wait for
both tasks to complete."""
wait_started = asyncio.Event()
@@ -58,7 +58,7 @@ async def wait_and_do(wait_fn, do_fn):
async def do():
await wait_started.wait()
- await asyncio.sleep(0.1)
+ await asyncio.sleep(sleep_time)
# Let do_fn be either a callable or an awaitable object
if inspect.isawaitable(do_fn):
await do_fn
281 | + # update, this template would include the mempool transaction.
282 | assert_equal(len(block2.vtx), 1)
283 |
284 | self.log.debug("Wait for another, but time out")
285 | template3 = await mining_wait_next_template(template2, stack, ctx, waitoptions)
286 | assert template3 is None
https://github.com/bitcoin/bitcoin/pull/35598/changes/bd1771fc0cd248438e87b028b096f59ffc976d18 (test: cover feeThreshold = MAX_MONEY)
(see comment above: https://github.com/bitcoin/bitcoin/pull/35598/changes/bd1771fc0cd248438e87b028b096f59ffc976d18#r3509745119)
Should probably add a transaction here to check in another way that MAX_MONEY works to prevent fee-based template updates:
@@ -280,9 +284,14 @@ class IPCMiningTest(BitcoinTestFramework):
assert_equal(len(block2.vtx), 1)
self.log.debug("Wait for another, but time out")
+ # Temporarily decrease the timeout since we want to hit the
+ # deadline and perform a fee check and see what happens.
+ waitoptions.timeout = 100
+ self.miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])
template3 = await mining_wait_next_template(template2, stack, ctx, waitoptions)
assert template3 is None
+ waitoptions.timeout = self.default_ipc_timeout
self.log.debug("Wait for another, get one after increase in fees in the mempool")
waitoptions.feeThreshold = 1
template4 = await wait_and_do(
@@ -290,14 +299,14 @@ class IPCMiningTest(BitcoinTestFramework):
lambda: self.miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0]))
assert template4 is not None
block3 = await mining_get_block(template4, ctx)
- assert_equal(len(block3.vtx), 2)
+ assert_equal(len(block3.vtx), 3)
self.log.debug("Wait again, this should return the same template, since the fee threshold is zero")
waitoptions.feeThreshold = 0
template5 = await mining_wait_next_template(template4, stack, ctx, waitoptions)
assert template5 is not None
block4 = await mining_get_block(template5, ctx)
- assert_equal(len(block4.vtx), 2)
+ assert_equal(len(block4.vtx), 3)
waitoptions.feeThreshold = 1
self.log.debug("Wait for another, get one after increase in fees in the mempool")
@@ -306,7 +315,7 @@ class IPCMiningTest(BitcoinTestFramework):
lambda: self.miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0]))
assert template6 is not None
block4 = await mining_get_block(template6, ctx)
- assert_equal(len(block4.vtx), 3)
+ assert_equal(len(block4.vtx), 4)
self.log.debug("Wait for another, but time out, since the fee threshold is set now")
template7 = await mining_wait_next_template(template6, stack, ctx, waitoptions)