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 and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| Approach ACK | jeanpablojp |
| 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
Ah oops. The counter intuitive thing here is that waitNext() will ignore fee changes in the first second. This might change with cluster mempool when we get rid of the internal tick mechanism, see #35581. But the adjusted test should continue to work.
Took your suggestion with some adjustments.
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)
Also taken
Turns out it was a a good idea to make this a separate PR. Fixed #35598 (review)
Approach ACK
Left two comments.
290 | assert_equal(len(block2.vtx), 1)
291 |
292 | self.log.debug("Wait for another, but time out")
293 | + # Temporarily decrease the timeout since we want to hit the
294 | + # deadline, which in the current implementation triggers a
295 | + # final fee check, and see what happens.
Nit, feel free to ignore. With feeThreshold at MAX_MONEY the fee block in WaitAndCreateNewBlock is skipped, so nothing is checked at the deadline either. The assert is right, the comment isn't.
293 | + # Temporarily decrease the timeout since we want to hit the 294 | + # deadline, which in the current implementation triggers a 295 | + # final fee check, and see what happens. 296 | + waitoptions.timeout = 100 297 | + self.miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0]) 298 | template3 = await mining_wait_next_template(template2, stack, ctx, waitoptions)
mining_types.h describes MAX_MONEY as an optimization, and no fee delta can reach it, so the MAX_MONEY half of that guard only exists to skip building the template. Dropping it, and the Assume it makes true, leaves the test green in both release and debug builds. With the wrap below it fails. What do you think about covering that here too?
# MAX_MONEY is documented as an optimization, so waitNext()
# should not build a template at all here.
with self.nodes[0].assert_debug_log(expected_msgs=[], unexpected_msgs=["CreateNewBlock()"]):
template3 = await mining_wait_next_template(template2, stack, ctx, waitoptions)
reACK https://github.com/bitcoin/bitcoin/pull/35598/changes/0de7c2bf7912d5632c33195b7764e06c46e5f7b0
Changes look good to me. It is useful to have this coverage, and it makes sense as a standalone pr. Left non-blocking comment
299 | assert template3 is None 300 | 301 | + waitoptions.timeout = self.default_ipc_timeout 302 | self.log.debug("Wait for another, get one after increase in fees in the mempool") 303 | + waitoptions.feeThreshold = 1 304 | template4 = await wait_and_do(
In https://github.com/bitcoin/bitcoin/pull/35598/changes/0de7c2bf7912d5632c33195b7764e06c46e5f7b0: test: cover feeThreshold = MAX_MONEY
non_blocking nit
Looking at this again, my understanding is that the transaction added above is intentionally kept in the mempool to verify that feeThreshold = MAX_MONEY ignores a fee increase and still times out. That part makes sense.
After the timeout, however, that transaction remains in the mempool and is not included in template2. Therefore, once feeThreshold is changed to 1, it already satisfies the threshold before this second wait starts. waitNext is allowed to return without waiting for the transaction added by this lambda.
Verified this by delaying the lambda
index 85a7b8bf11..fe5b861614 100755
--- a/test/functional/interface_ipc_mining.py
+++ b/test/functional/interface_ipc_mining.py
@@ -299,7 +299,8 @@ class IPCMiningTest(BitcoinTestFramework):
waitoptions.feeThreshold = 1
template4 = await wait_and_do(
mining_wait_next_template(template2, stack, ctx, waitoptions),
- lambda: self.miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0]))
+ lambda: self.miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0]),
+ sleep_time=1.1 * self.options.timeout_factor)
assert template4 is not None
block3 = await mining_get_block(template4, ctx)
assert_equal(len(block3.vtx), 3)
The test fails when asserting the transaction count because it only contains 2 transactions. Would it be clearer to consume the existing fee increase directly here?
index 85a7b8bf11..57c1b60b98 100755
--- a/test/functional/interface_ipc_mining.py
+++ b/test/functional/interface_ipc_mining.py
@@ -295,21 +295,19 @@ class IPCMiningTest(BitcoinTestFramework):
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")
+ self.log.debug("Wait for another, get one for the fees already in the mempool")
waitoptions.feeThreshold = 1
- template4 = await wait_and_do(
- mining_wait_next_template(template2, stack, ctx, waitoptions),
- lambda: self.miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0]))
+ template4 = await mining_wait_next_template(template2, stack, ctx, waitoptions)
assert template4 is not None
block3 = await mining_get_block(template4, ctx)
- assert_equal(len(block3.vtx), 3)
+ 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 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), 3)
+ assert_equal(len(block4.vtx), 2)
waitoptions.feeThreshold = 1
self.log.debug("Wait for another, get one after increase in fees in the mempool")
@@ -318,7 +316,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), 4)
+ assert_equal(len(block4.vtx), 3)
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)
Later on template6 would still test adding a transaction while waitNext is active, verifying that a new template is returned
Co-authored-by: David Gumberg <davidzgumberg@gmail.com>
Rebased just in case, and addressed feedback from @jeanpablojp and @enirox001.