Merge 5ba770089e into 66aa6a47bd

7c123c08dd  miner: add package feerate vector to CBlockTemplate (ismaelsadeeq)

Pull request description:

  This PR enables `BlockAssembler` to add all selected packages' fee and virtual size to a vector, and then return the vector as a member of `CBlockTemplate` struct.

  This PR is the first step in the https://github.com/bitcoin/bitcoin/issues/30392 project.

  The packages' vsize and fee are used in #30157 to select a percentile fee rate of the top block in the mempool.

ACKs for top commit:
  rkrux:
    tACK 7c123c08dd
  ryanofsky:
    Code review ACK 7c123c08dd. Changes since last review are rebasing due to a test conflict, giving the new field a better name and description, resolving the test conflict, and renaming a lot of test variables. The actual code change is still one-line change.
  glozow:
    reACK 7c123c08dd

Tree-SHA512: 767b0b3d4273cf1589fd2068d729a66c7414c0f9574b15989fbe293f8c85cd6c641dd783cde55bfabab32cd047d7d8a071d6897b06ed4295c0d071e588de0861

src/node/miner.cpp

@@ -28,18 +28,24 @@
 #include <utility>
 
 namespace node {
-int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev)
-{
-    int64_t nOldTime = pblock->nTime;
-    int64_t nNewTime{std::max<int64_t>(pindexPrev->GetMedianTimePast() + 1, TicksSinceEpoch<std::chrono::seconds>(NodeClock::now()))};
 
+int64_t GetMinimumTime(const CBlockIndex* pindexPrev, const Consensus::Params& consensusParams)
+{
+    int64_t min_time{pindexPrev->GetMedianTimePast() + 1};
     if (consensusParams.enforce_BIP94) {
         // Height of block to be mined.
         const int height{pindexPrev->nHeight + 1};
         if (height % consensusParams.DifficultyAdjustmentInterval() == 0) {
-            nNewTime = std::max<int64_t>(nNewTime, pindexPrev->GetBlockTime() - MAX_TIMEWARP);
+            min_time = std::max<int64_t>(min_time, pindexPrev->GetBlockTime() - MAX_TIMEWARP);
         }
     }
+    return min_time;
+}
+
+int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev)
+{
+    int64_t nOldTime = pblock->nTime;
+    int64_t nNewTime{std::max<int64_t>(GetMinimumTime(pindexPrev, consensusParams), TicksSinceEpoch<std::chrono::seconds>(NodeClock::now()))};
 
     if (nOldTime < nNewTime) {
         pblock->nTime = nNewTime;
@@ -421,6 +427,7 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda
         }
 
         ++nPackagesSelected;
+        pblocktemplate->m_package_feerates.emplace_back(packageFees, static_cast<int32_t>(packageSize));
 
         // Update transactions that depend on each of these
         nDescendantsUpdated += UpdatePackagesForAdded(mempool, ancestors, mapModifiedTx);

src/node/miner.h

@@ -10,6 +10,7 @@
 #include <policy/policy.h>
 #include <primitives/block.h>
 #include <txmempool.h>
+#include <util/feefrac.h>
 
 #include <memory>
 #include <optional>
@@ -39,6 +40,9 @@ struct CBlockTemplate
     std::vector<CAmount> vTxFees;
     std::vector<int64_t> vTxSigOpsCost;
     std::vector<unsigned char> vchCoinbaseCommitment;
+    /* A vector of package fee rates, ordered by the sequence in which
+     * packages are selected for inclusion in the block template.*/
+    std::vector<FeeFrac> m_package_feerates;
 };
 
 // Container for tracking updates to ancestor feerate as we include (parent)
@@ -207,6 +211,8 @@ private:
     void SortForBlock(const CTxMemPool::setEntries& package, std::vector<CTxMemPool::txiter>& sortedEntries);
 };
 
+int64_t GetMinimumTime(const CBlockIndex* pindexPrev, const Consensus::Params& consensusParams);
+
 int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);
 
 /** Update an old GenerateCoinbaseCommitment from CreateNewBlock after the block txs have changed */

src/rpc/mining.cpp

@@ -49,6 +49,7 @@
 using interfaces::BlockTemplate;
 using interfaces::Mining;
 using node::BlockAssembler;
+using node::GetMinimumTime;
 using node::NodeContext;
 using node::RegenerateCommitments;
 using node::UpdateTime;
@@ -954,7 +955,7 @@ static RPCHelpMan getblocktemplate()
     result.pushKV("coinbasevalue", (int64_t)block.vtx[0]->vout[0].nValue);
     result.pushKV("longpollid", tip.GetHex() + ToString(nTransactionsUpdatedLast));
     result.pushKV("target", hashTarget.GetHex());
-    result.pushKV("mintime", (int64_t)pindexPrev->GetMedianTimePast()+1);
+    result.pushKV("mintime", GetMinimumTime(pindexPrev, consensusParams));
     result.pushKV("mutable", std::move(aMutable));
     result.pushKV("noncerange", "00000000ffffffff");
     int64_t nSigOpLimit = MAX_BLOCK_SIGOPS_COST;

src/test/miner_tests.cpp

@@ -16,6 +16,7 @@
 #include <txmempool.h>
 #include <uint256.h>
 #include <util/check.h>
+#include <util/feefrac.h>
 #include <util/strencodings.h>
 #include <util/time.h>
 #include <util/translation.h>
@@ -25,6 +26,7 @@
 #include <test/util/setup_common.h>
 
 #include <memory>
+#include <vector>
 
 #include <boost/test/unit_test.hpp>
 
@@ -123,19 +125,22 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const
     tx.vout[0].nValue = 5000000000LL - 1000;
     // This tx has a low fee: 1000 satoshis
     Txid hashParentTx = tx.GetHash(); // save this txid for later use
-    AddToMempool(tx_mempool, entry.Fee(1000).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx));
+    const auto parent_tx{entry.Fee(1000).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx)};
+    AddToMempool(tx_mempool, parent_tx);
 
     // This tx has a medium fee: 10000 satoshis
     tx.vin[0].prevout.hash = txFirst[1]->GetHash();
     tx.vout[0].nValue = 5000000000LL - 10000;
     Txid hashMediumFeeTx = tx.GetHash();
-    AddToMempool(tx_mempool, entry.Fee(10000).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx));
+    const auto medium_fee_tx{entry.Fee(10000).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx)};
+    AddToMempool(tx_mempool, medium_fee_tx);
 
     // This tx has a high fee, but depends on the first transaction
     tx.vin[0].prevout.hash = hashParentTx;
     tx.vout[0].nValue = 5000000000LL - 1000 - 50000; // 50k satoshi fee
     Txid hashHighFeeTx = tx.GetHash();
-    AddToMempool(tx_mempool, entry.Fee(50000).Time(Now<NodeSeconds>()).SpendsCoinbase(false).FromTx(tx));
+    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);
     BOOST_REQUIRE(block_template);
@@ -145,6 +150,21 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const
     BOOST_CHECK(block.vtx[2]->GetHash() == hashHighFeeTx);
     BOOST_CHECK(block.vtx[3]->GetHash() == hashMediumFeeTx);
 
+    // Test the inclusion of package feerates in the block template and ensure they are sequential.
+    const auto block_package_feerates = BlockAssembler{m_node.chainman->ActiveChainstate(), &tx_mempool, options}.CreateNewBlock()->m_package_feerates;
+    BOOST_CHECK(block_package_feerates.size() == 2);
+
+    // parent_tx and high_fee_tx are added to the block as a package.
+    const auto combined_txs_fee = parent_tx.GetFee() + high_fee_tx.GetFee();
+    const auto combined_txs_size = parent_tx.GetTxSize() + high_fee_tx.GetTxSize();
+    FeeFrac package_feefrac{combined_txs_fee, combined_txs_size};
+    // The package should be added first.
+    BOOST_CHECK(block_package_feerates[0] == package_feefrac);
+
+    // The medium_fee_tx should be added next.
+    FeeFrac medium_tx_feefrac{medium_fee_tx.GetFee(), medium_fee_tx.GetTxSize()};
+    BOOST_CHECK(block_package_feerates[1] == medium_tx_feefrac);
+
     // Test that a package below the block min tx fee doesn't get included
     tx.vin[0].prevout.hash = hashHighFeeTx;
     tx.vout[0].nValue = 5000000000LL - 1000 - 50000; // 0 fee

test/functional/mining_basic.py

@@ -136,20 +136,39 @@ class MiningTest(BitcoinTestFramework):
 
         for _ in range(n):
             t += 600
-            self.nodes[0].setmocktime(t)
+            node.setmocktime(t)
             self.generate(self.wallet, 1, sync_fun=self.no_op)
 
-        self.log.info("Create block two hours in the future")
-        self.nodes[0].setmocktime(t + MAX_FUTURE_BLOCK_TIME)
+        self.log.info("Create block MAX_TIMEWARP < t < MAX_FUTURE_BLOCK_TIME in the future")
+        # A timestamp that's more than MAX_TIMEWARP seconds in the future can
+        # happen by accident, due to a combination of pool software that doesn't
+        # use "curtime" AND has a faulty clock.
+        #
+        # But it could also be intentional, at the end of a retarget period, in
+        # order to make the next block miner violate the time-timewarp-attack rule.
+        # For this attack to succeed the victim miner needs to ignore both our
+        # "curtime" and "mintime" values AND use wall clock time. This is true even
+        # if the victim miner implements the MTP rule.
+        #
+        # The attack is illustrated below.
+        #
+        # Force the next block to have a timestamp in the future:
+        future = t + MAX_TIMEWARP + 1
+        # Witout violating the 2 hour in the future rule
+        assert_greater_than_or_equal(t + MAX_FUTURE_BLOCK_TIME, future)
+        node.setmocktime(future)
         self.generate(self.wallet, 1, sync_fun=self.no_op)
-        assert_equal(node.getblock(node.getbestblockhash())['time'], t + MAX_FUTURE_BLOCK_TIME)
+        assert_equal(node.getblock(node.getbestblockhash())['time'], future)
 
         self.log.info("First block template of retarget period can't use wall clock time")
-        self.nodes[0].setmocktime(t)
-        # The template will have an adjusted timestamp, which we then modify
+        node.setmocktime(t)
+        # The template will have an adjusted timestamp.
         tmpl = node.getblocktemplate(NORMAL_GBT_REQUEST_PARAMS)
-        assert_greater_than_or_equal(tmpl['curtime'], t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP)
+        assert_equal(tmpl['curtime'], t + 1)
+        # mintime and curtime should match
+        assert_equal(tmpl['mintime'], tmpl['curtime'])
 
+        # Check that the adjusted timestamp results in a valid block
         block = CBlock()
         block.nVersion = tmpl["version"]
         block.hashPrevBlock = int(tmpl["previousblockhash"], 16)
@@ -161,18 +180,29 @@ class MiningTest(BitcoinTestFramework):
         assert_template(node, block, None)
 
         bad_block = copy.deepcopy(block)
+        # Use wall clock instead of the adjusted timestamp. This could happen
+        # by accident if pool software ignores mintime and curtime.
         bad_block.nTime = t
         bad_block.solve()
         assert_raises_rpc_error(-25, 'time-timewarp-attack', lambda: node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex()))
 
-        self.log.info("Test timewarp protection boundary")
-        bad_block.nTime = t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP - 1
+        # It can also happen if the pool implements its own logic to adjust its
+        # timestamp to MTP + 1, but doesn't take the new timewarp rule into
+        # account (and ignores mintime).
+        mtp = node.getblock(node.getbestblockhash())["mediantime"] + 1
+        bad_block.nTime = mtp + 1
         bad_block.solve()
         assert_raises_rpc_error(-25, 'time-timewarp-attack', lambda: node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex()))
 
-        bad_block.nTime = t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP
+        self.log.info("Test timewarp protection boundary")
+        bad_block.nTime = future - MAX_TIMEWARP - 1
         bad_block.solve()
-        node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex())
+        assert_raises_rpc_error(-25, 'time-timewarp-attack', lambda: node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex()))
+
+        good_block = copy.deepcopy(bad_block)
+        good_block.nTime = future - MAX_TIMEWARP
+        good_block.solve()
+        node.submitheader(hexdata=CBlockHeader(good_block).serialize().hex())
 
     def test_pruning(self):
         self.log.info("Test that submitblock stores previously pruned block")