Merge a96b84cb1b 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

@@ -421,6 +421,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)

src/test/fuzz/fuzz.cpp

@@ -116,6 +116,9 @@ void initialize()
     // - Creating a BasicTestingSetup or derived class will switch to a random seed.
     SeedRandomStateForTest(SeedRand::ZEROS);
 
+    // Set time to the genesis block timestamp for deterministic initialization.
+    SetMockTime(1231006505);
+
     // Terminate immediately if a fuzzing harness ever tries to create a socket.
     // Individual tests can override this by pointing CreateSock to a mocked alternative.
     CreateSock = [](int, int, int) -> std::unique_ptr<Sock> { std::terminate(); };

src/test/fuzz/load_external_block_file.cpp

@@ -9,6 +9,7 @@
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
 #include <test/util/setup_common.h>
+#include <util/time.h>
 #include <validation.h>
 
 #include <cstdint>
@@ -27,6 +28,7 @@ void initialize_load_external_block_file()
 FUZZ_TARGET(load_external_block_file, .init = initialize_load_external_block_file)
 {
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
+    SetMockTime(ConsumeTime(fuzzed_data_provider));
     FuzzedFileProvider fuzzed_file_provider{fuzzed_data_provider};
     AutoFile fuzzed_block_file{fuzzed_file_provider.open()};
     if (fuzzed_block_file.IsNull()) {

src/test/fuzz/mini_miner.cpp

@@ -1,3 +1,7 @@
+// Copyright (c) The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
@@ -13,6 +17,7 @@
 #include <random.h>
 #include <txmempool.h>
 #include <util/check.h>
+#include <util/time.h>
 #include <util/translation.h>
 
 #include <deque>
@@ -36,6 +41,7 @@ FUZZ_TARGET(mini_miner, .init = initialize_miner)
 {
     SeedRandomStateForTest(SeedRand::ZEROS);
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
+    SetMockTime(ConsumeTime(fuzzed_data_provider));
     bilingual_str error;
     CTxMemPool pool{CTxMemPool::Options{}, error};
     Assert(error.empty());
@@ -115,6 +121,7 @@ FUZZ_TARGET(mini_miner_selection, .init = initialize_miner)
 {
     SeedRandomStateForTest(SeedRand::ZEROS);
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
+    SetMockTime(ConsumeTime(fuzzed_data_provider));
     bilingual_str error;
     CTxMemPool pool{CTxMemPool::Options{}, error};
     Assert(error.empty());

src/test/fuzz/net.cpp

@@ -16,6 +16,7 @@
 #include <test/util/setup_common.h>
 #include <util/asmap.h>
 #include <util/chaintype.h>
+#include <util/time.h>
 
 #include <cstdint>
 #include <optional>
@@ -79,6 +80,7 @@ FUZZ_TARGET(net, .init = initialize_net)
 FUZZ_TARGET(local_address, .init = initialize_net)
 {
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
+    SetMockTime(ConsumeTime(fuzzed_data_provider));
     CService service{ConsumeService(fuzzed_data_provider)};
     CNode node{ConsumeNode(fuzzed_data_provider)};
     {

src/test/fuzz/p2p_headers_presync.cpp

@@ -154,14 +154,15 @@ void initialize()
 FUZZ_TARGET(p2p_headers_presync, .init = initialize)
 {
     SeedRandomStateForTest(SeedRand::ZEROS);
+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
+    SetMockTime(ConsumeTime(fuzzed_data_provider));
+
     ChainstateManager& chainman = *g_testing_setup->m_node.chainman;
 
     LOCK(NetEventsInterface::g_msgproc_mutex);
 
     g_testing_setup->ResetAndInitialize();
 
-    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
-
     CBlockHeader base{chainman.GetParams().GenesisBlock()};
     SetMockTime(base.nTime);
 

src/test/fuzz/partially_downloaded_block.cpp

@@ -11,6 +11,7 @@
 #include <test/util/txmempool.h>
 #include <txmempool.h>
 #include <util/check.h>
+#include <util/time.h>
 #include <util/translation.h>
 
 #include <cstddef>
@@ -46,6 +47,7 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb)
 {
     SeedRandomStateForTest(SeedRand::ZEROS);
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
+    SetMockTime(ConsumeTime(fuzzed_data_provider));
 
     auto block{ConsumeDeserializable<CBlock>(fuzzed_data_provider, TX_WITH_WITNESS)};
     if (!block || block->vtx.size() == 0 ||

src/test/fuzz/socks5.cpp

@@ -9,6 +9,7 @@
 #include <test/fuzz/util.h>
 #include <test/fuzz/util/net.h>
 #include <test/util/setup_common.h>
+#include <util/time.h>
 
 #include <cstdint>
 #include <string>
@@ -29,6 +30,7 @@ void initialize_socks5()
 FUZZ_TARGET(socks5, .init = initialize_socks5)
 {
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
+    SetMockTime(ConsumeTime(fuzzed_data_provider));
     ProxyCredentials proxy_credentials;
     proxy_credentials.username = fuzzed_data_provider.ConsumeRandomLengthString(512);
     proxy_credentials.password = fuzzed_data_provider.ConsumeRandomLengthString(512);

src/test/fuzz/txdownloadman.cpp

@@ -18,6 +18,7 @@
 #include <test/util/txmempool.h>
 #include <util/hasher.h>
 #include <util/rbf.h>
+#include <util/time.h>
 #include <txmempool.h>
 #include <validation.h>
 #include <validationinterface.h>
@@ -167,6 +168,7 @@ FUZZ_TARGET(txdownloadman, .init = initialize)
 {
     SeedRandomStateForTest(SeedRand::ZEROS);
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
+    SetMockTime(ConsumeTime(fuzzed_data_provider));
 
     // Initialize txdownloadman
     bilingual_str error;
@@ -297,6 +299,7 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize)
 {
     SeedRandomStateForTest(SeedRand::ZEROS);
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
+    SetMockTime(ConsumeTime(fuzzed_data_provider));
 
     // Initialize a TxDownloadManagerImpl
     bilingual_str error;

src/test/fuzz/util/check_globals.cpp

@@ -5,6 +5,7 @@
 #include <test/fuzz/util/check_globals.h>
 
 #include <test/util/random.h>
+#include <util/time.h>
 
 #include <iostream>
 #include <memory>
@@ -16,6 +17,8 @@ struct CheckGlobalsImpl {
     {
         g_used_g_prng = false;
         g_seeded_g_prng_zero = false;
+        g_used_system_time = false;
+        SetMockTime(0s);
     }
     ~CheckGlobalsImpl()
     {
@@ -34,6 +37,19 @@ struct CheckGlobalsImpl {
                       << std::endl;
             std::abort(); // Abort, because AFL may try to recover from a std::exit
         }
+
+        if (g_used_system_time) {
+            std::cerr << "\n\n"
+                         "The current fuzz target accessed system time.\n\n"
+
+                         "This is acceptable, but requires the fuzz target to call \n"
+                         "SetMockTime() at the beginning of processing the fuzz input.\n\n"
+
+                         "Without setting mock time, time-dependent behavior can lead \n"
+                         "to non-reproducible bugs or inefficient fuzzing.\n\n"
+                      << std::endl;
+            std::abort();
+        }
     }
 };
 

src/test/fuzz/util/check_globals.h

@@ -5,10 +5,13 @@
 #ifndef BITCOIN_TEST_FUZZ_UTIL_CHECK_GLOBALS_H
 #define BITCOIN_TEST_FUZZ_UTIL_CHECK_GLOBALS_H
 
+#include <atomic>
 #include <memory>
 #include <optional>
 #include <string>
 
+extern std::atomic<bool> g_used_system_time;
+
 struct CheckGlobalsImpl;
 struct CheckGlobals {
     CheckGlobals();

src/test/fuzz/utxo_snapshot.cpp

@@ -24,6 +24,7 @@
 #include <util/check.h>
 #include <util/fs.h>
 #include <util/result.h>
+#include <util/time.h>
 #include <validation.h>
 
 #include <cstdint>
@@ -72,6 +73,7 @@ void utxo_snapshot_fuzz(FuzzBufferType buffer)
 {
     SeedRandomStateForTest(SeedRand::ZEROS);
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
+    SetMockTime(ConsumeTime(fuzzed_data_provider, /*min=*/1296688602)); // regtest genesis block timestamp
     auto& setup{*g_setup};
     bool dirty_chainman{false}; // Re-use the global chainman, but reset it when it is dirty
     auto& chainman{*setup.m_node.chainman};

src/test/fuzz/utxo_total_supply.cpp

@@ -15,6 +15,7 @@
 #include <test/util/mining.h>
 #include <test/util/setup_common.h>
 #include <util/chaintype.h>
+#include <util/time.h>
 #include <validation.h>
 
 using node::BlockAssembler;
@@ -22,18 +23,22 @@ using node::BlockAssembler;
 FUZZ_TARGET(utxo_total_supply)
 {
     SeedRandomStateForTest(SeedRand::ZEROS);
+    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
+    const auto mock_time{ConsumeTime(fuzzed_data_provider, /*min=*/1296688602)}; // regtest genesis block timestamp
     /** The testing setup that creates a chainman only (no chainstate) */
     ChainTestingSetup test_setup{
         ChainType::REGTEST,
         {
-            .extra_args = {"-testactivationheight=bip34@2"},
+            .extra_args = {
+                "-testactivationheight=bip34@2",
+                strprintf("-mocktime=%d", mock_time).c_str()
+            },
         },
     };
     // Create chainstate
     test_setup.LoadVerifyActivateChainstate();
     auto& node{test_setup.m_node};
     auto& chainman{*Assert(test_setup.m_node.chainman)};
-    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
 
     const auto ActiveHeight = [&]() {
         LOCK(chainman.GetMutex());

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

src/test/util/setup_common.cpp

@@ -199,7 +199,9 @@ BasicTestingSetup::~BasicTestingSetup()
 {
     m_node.ecc_context.reset();
     m_node.kernel.reset();
-    SetMockTime(0s); // Reset mocktime for following tests
+    if constexpr (!G_FUZZING) {
+        SetMockTime(0s); // Reset mocktime for following tests
+    }
     LogInstance().DisconnectTestLogger();
     if (m_has_custom_datadir) {
         // Only remove the lock file, preserve the data directory.

src/util/time.cpp

@@ -20,10 +20,14 @@
 void UninterruptibleSleep(const std::chrono::microseconds& n) { std::this_thread::sleep_for(n); }
 
 static std::atomic<std::chrono::seconds> g_mock_time{}; //!< For testing
+std::atomic<bool> g_used_system_time{false};
 
 NodeClock::time_point NodeClock::now() noexcept
 {
     const auto mocktime{g_mock_time.load(std::memory_order_relaxed)};
+    if (!mocktime.count()) {
+        g_used_system_time = true;
+    }
     const auto ret{
         mocktime.count() ?
             mocktime :

src/wallet/test/fuzz/notifications.cpp

@@ -24,6 +24,7 @@
 #include <uint256.h>
 #include <util/check.h>
 #include <util/result.h>
+#include <util/time.h>
 #include <util/translation.h>
 #include <wallet/coincontrol.h>
 #include <wallet/context.h>
@@ -58,6 +59,7 @@ FUZZ_TARGET(wallet_notifications, .init = initialize_setup)
 {
     SeedRandomStateForTest(SeedRand::ZEROS);
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
+    SetMockTime(ConsumeTime(fuzzed_data_provider));
     // The total amount, to be distributed to the wallets a and b in txs
     // without fee. Thus, the balance of the wallets should always equal the
     // total amount.

src/wallet/test/fuzz/scriptpubkeyman.cpp

@@ -19,6 +19,7 @@
 #include <test/fuzz/util/descriptor.h>
 #include <test/util/setup_common.h>
 #include <util/check.h>
+#include <util/time.h>
 #include <util/translation.h>
 #include <validation.h>
 #include <wallet/scriptpubkeyman.h>
@@ -87,6 +88,7 @@ FUZZ_TARGET(scriptpubkeyman, .init = initialize_spkm)
 {
     SeedRandomStateForTest(SeedRand::ZEROS);
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
+    SetMockTime(ConsumeTime(fuzzed_data_provider));
     const auto& node{g_setup->m_node};
     Chainstate& chainstate{node.chainman->ActiveChainstate()};
     std::unique_ptr<CWallet> wallet_ptr{std::make_unique<CWallet>(node.chain.get(), "", CreateMockableWalletDatabase())};

src/wallet/test/fuzz/spend.cpp

@@ -8,6 +8,7 @@
 #include <test/fuzz/util/wallet.h>
 #include <test/util/random.h>
 #include <test/util/setup_common.h>
+#include <util/time.h>
 #include <wallet/coincontrol.h>
 #include <wallet/context.h>
 #include <wallet/spend.h>
@@ -32,6 +33,7 @@ FUZZ_TARGET(wallet_create_transaction, .init = initialize_setup)
 {
     SeedRandomStateForTest(SeedRand::ZEROS);
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
+    SetMockTime(ConsumeTime(fuzzed_data_provider));
     const auto& node = g_setup->m_node;
     Chainstate& chainstate{node.chainman->ActiveChainstate()};
     ArgsManager& args = *node.args;