miner: fix empty mempool case for waitNext()

Block template fees are calculated by looping over new_tmpl->vTxFees
and return (early) once the fee_threshold is exceeded.

This left an edge case when the mempool is empty, which this commit
fixes and adds a test for. It does so by using std::accumulate instead
of manual loops.

Also update interface_ipc.py to account for the new behavior.

Co-authored-by: Raimo33 <claudio.raimondi@protonmail.com>

Github-Pull: #33566
Rebased-From: 8f7673257a

src/node/miner.cpp

@@ -29,6 +29,7 @@
 
 #include <algorithm>
 #include <utility>
+#include <numeric>
 
 namespace node {
 
@@ -522,18 +523,13 @@ std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainma
 
             // Calculate the original template total fees if we haven't already
             if (current_fees == -1) {
-                current_fees = 0;
-                for (CAmount fee : block_template->vTxFees) {
-                    current_fees += fee;
-                }
+                current_fees = std::accumulate(block_template->vTxFees.begin(), block_template->vTxFees.end(), CAmount{0});
             }
 
-            CAmount new_fees = 0;
-            for (CAmount fee : new_tmpl->vTxFees) {
-                new_fees += fee;
-                Assume(options.fee_threshold != MAX_MONEY);
-                if (new_fees >= current_fees + options.fee_threshold) return new_tmpl;
-            }
+            // Check if fees increased enough to return the new template
+            const CAmount new_fees = std::accumulate(new_tmpl->vTxFees.begin(), new_tmpl->vTxFees.end(), CAmount{0});
+            Assume(options.fee_threshold != MAX_MONEY);
+            if (new_fees >= current_fees + options.fee_threshold) return new_tmpl;
         }
 
         now = NodeClock::now();

src/test/miner_tests.cpp

@@ -113,6 +113,22 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const
     options.coinbase_output_script = scriptPubKey;
 
     LOCK(tx_mempool.cs);
+    BOOST_CHECK(tx_mempool.size() == 0);
+
+    // Block template should only have a coinbase when there's nothing in the mempool
+    std::unique_ptr<BlockTemplate> block_template = mining->createNewBlock(options);
+    BOOST_REQUIRE(block_template);
+    CBlock block{block_template->getBlock()};
+    BOOST_REQUIRE_EQUAL(block.vtx.size(), 1U);
+
+    // waitNext() on an empty mempool should return nullptr because there is no better template
+    auto should_be_nullptr = block_template->waitNext({.timeout = MillisecondsDouble{0}, .fee_threshold = 1});
+    BOOST_REQUIRE(should_be_nullptr == nullptr);
+
+    // Unless fee_threshold is 0
+    block_template = block_template->waitNext({.timeout = MillisecondsDouble{0}, .fee_threshold = 0});
+    BOOST_REQUIRE(block_template);
+
     // Test the ancestor feerate transaction selection.
     TestMemPoolEntryHelper entry;
 
@@ -144,9 +160,9 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const
     const auto high_fee_tx{entry.Fee(50000).Time(Now<NodeSeconds>()).SpendsCoinbase(false).FromTx(tx)};
     AddToMempool(tx_mempool, high_fee_tx);
 
-    std::unique_ptr<BlockTemplate> block_template = mining->createNewBlock(options);
+    block_template = mining->createNewBlock(options);
     BOOST_REQUIRE(block_template);
-    CBlock block{block_template->getBlock()};
+    block = block_template->getBlock();
     BOOST_REQUIRE_EQUAL(block.vtx.size(), 4U);
     BOOST_CHECK(block.vtx[1]->GetHash() == hashParentTx);
     BOOST_CHECK(block.vtx[2]->GetHash() == hashHighFeeTx);
@@ -184,7 +200,7 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const
     AddToMempool(tx_mempool, entry.Fee(feeToUse).FromTx(tx));
 
     // waitNext() should return nullptr because there is no better template
-    auto should_be_nullptr = block_template->waitNext({.timeout = MillisecondsDouble{0}, .fee_threshold = 1});
+    should_be_nullptr = block_template->waitNext({.timeout = MillisecondsDouble{0}, .fee_threshold = 1});
     BOOST_REQUIRE(should_be_nullptr == nullptr);
 
     block = block_template->getBlock();

test/functional/interface_ipc.py

@@ -153,6 +153,7 @@ class IPCInterfaceTest(BitcoinTestFramework):
             self.log.debug("Wait for a new template")
             waitoptions = self.capnp_modules['mining'].BlockWaitOptions()
             waitoptions.timeout = timeout
+            waitoptions.feeThreshold = 1
             waitnext = template.result.waitNext(ctx, waitoptions)
             self.generate(self.nodes[0], 1)
             template2 = await waitnext
@@ -168,6 +169,7 @@ class IPCInterfaceTest(BitcoinTestFramework):
             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 template4.result.waitNext(ctx, waitoptions)
             block4 = await self.parse_and_deserialize_block(template5, ctx)
             assert_equal(len(block4.vtx), 2)