test: cover feeThreshold = MAX_MONEY in interface_ipc_mining.py #35598

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.

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:

https://github.com/bitcoin/bitcoin/blob/a8823c099618559fdf6116fe17c9afd311e88890/src/node/miner.cpp#L452-L460

and wait_and_do only sleeps for 0.1s:

https://github.com/bitcoin/bitcoin/blob/a8823c099618559fdf6116fe17c9afd311e88890/test/functional/test_framework/ipc_util.py#L59-L67

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_timeout in 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.

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