Merge 8e86e77311 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/bitcoin-chainstate.cpp

@@ -64,10 +64,10 @@ int main(int argc, char* argv[])
 
     // SETUP: Context
     kernel::Context kernel_context{};
-    // We can't use a goto here, but we can use an assert since none of the
-    // things instantiated so far requires running the epilogue to be torn down
-    // properly
-    assert(kernel::SanityChecks(kernel_context));
+    if (!kernel::SanityChecks(kernel_context)) {
+        std::cerr << "Failed sanity check.";
+        return 1;
+    }
 
     ValidationSignals validation_signals{std::make_unique<util::ImmediateTaskRunner>()};
 
@@ -131,12 +131,12 @@ int main(int argc, char* argv[])
     auto [status, error] = node::LoadChainstate(chainman, cache_sizes, options);
     if (status != node::ChainstateLoadStatus::SUCCESS) {
         std::cerr << "Failed to load Chain state from your datadir." << std::endl;
-        goto epilogue;
+        return 1;
     } else {
         std::tie(status, error) = node::VerifyLoadedChainstate(chainman, options);
         if (status != node::ChainstateLoadStatus::SUCCESS) {
             std::cerr << "Failed to verify loaded Chain state from your datadir." << std::endl;
-            goto epilogue;
+            return 1;
         }
     }
 
@@ -144,7 +144,7 @@ int main(int argc, char* argv[])
         BlockValidationState state;
         if (!chainstate->ActivateBestChain(state, nullptr)) {
             std::cerr << "Failed to connect best block (" << state.ToString() << ")" << std::endl;
-            goto epilogue;
+            return 1;
         }
     }
 
@@ -255,18 +255,4 @@ int main(int argc, char* argv[])
             break;
         }
     }
-
-epilogue:
-    // Without this precise shutdown sequence, there will be a lot of nullptr
-    // dereferencing and UB.
-    validation_signals.FlushBackgroundCallbacks();
-    {
-        LOCK(cs_main);
-        for (Chainstate* chainstate : chainman.GetAll()) {
-            if (chainstate->CanFlushToDisk()) {
-                chainstate->ForceFlushStateToDisk();
-                chainstate->ResetCoinsViews();
-            }
-        }
-    }
 }

src/init.cpp

@@ -346,8 +346,13 @@ void Shutdown(NodeContext& node)
         }
     }
 
-    // After there are no more peers/RPC left to give us new data which may generate
-    // CValidationInterface callbacks, flush them...
+    // After there are no more peers/RPC left to give us new data which may
+    // generate CValidationInterface callbacks, flush them. Any future
+    // callbacks will be dropped. This should absolutely be safe - if missing a
+    // callback results in an unrecoverable situation, unclean shutdown would
+    // too. The only reason to do the above flushes is to let the wallet and
+    // indexes catch up with our current chain to avoid any strange pruning
+    // edge cases and make next startup faster by avoiding rescan.
     if (node.validation_signals) node.validation_signals->FlushBackgroundCallbacks();
 
     // Stop and delete all indexes only after flushing background callbacks.
@@ -357,21 +362,6 @@ void Shutdown(NodeContext& node)
     DestroyAllBlockFilterIndexes();
     node.indexes.clear(); // all instances are nullptr now
 
-    // Any future callbacks will be dropped. This should absolutely be safe - if
-    // missing a callback results in an unrecoverable situation, unclean shutdown
-    // would too. The only reason to do the above flushes is to let the wallet catch
-    // up with our current chain to avoid any strange pruning edge cases and make
-    // next startup faster by avoiding rescan.
-
-    if (node.chainman) {
-        LOCK(cs_main);
-        for (Chainstate* chainstate : node.chainman->GetAll()) {
-            if (chainstate->CanFlushToDisk()) {
-                chainstate->ForceFlushStateToDisk();
-                chainstate->ResetCoinsViews();
-            }
-        }
-    }
     for (const auto& client : node.chain_clients) {
         client->stop();
     }
@@ -387,9 +377,9 @@ void Shutdown(NodeContext& node)
     if (node.validation_signals) {
         node.validation_signals->UnregisterAllValidationInterfaces();
     }
+    node.chainman.reset();
     node.mempool.reset();
     node.fee_estimator.reset();
-    node.chainman.reset();
     node.validation_signals.reset();
     node.scheduler.reset();
     node.ecc_context.reset();

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/package_eval.cpp

@@ -341,6 +341,8 @@ FUZZ_TARGET(ephemeral_package_eval, .init = initialize_tx_pool)
 
     node.validation_signals->UnregisterSharedValidationInterface(outpoints_updater);
 
+    chainstate.SetMempool(g_setup->m_node.mempool.get());
+
     WITH_LOCK(::cs_main, tx_pool.check(chainstate.CoinsTip(), chainstate.m_chain.Height() + 1));
 }
 
@@ -536,6 +538,8 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
 
     node.validation_signals->UnregisterSharedValidationInterface(outpoints_updater);
 
+    chainstate.SetMempool(g_setup->m_node.mempool.get());
+
     WITH_LOCK(::cs_main, tx_pool.check(chainstate.CoinsTip(), chainstate.m_chain.Height() + 1));
 }
 } // namespace

src/test/fuzz/tx_pool.cpp

@@ -93,7 +93,7 @@ void SetMempoolConstraints(ArgsManager& args, FuzzedDataProvider& fuzzed_data_pr
                      ToString(fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 999)));
 }
 
-void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Chainstate& chainstate)
+void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, DummyChainState& chainstate)
 {
     WITH_LOCK(::cs_main, tx_pool.check(chainstate.CoinsTip(), chainstate.m_chain.Height() + 1));
     {
@@ -112,6 +112,7 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Cha
         WITH_LOCK(::cs_main, tx_pool.check(chainstate.CoinsTip(), chainstate.m_chain.Height() + 1));
     }
     g_setup->m_node.validation_signals->SyncWithValidationInterfaceQueue();
+    chainstate.SetMempool(g_setup->m_node.mempool.get());
 }
 
 void MockTime(FuzzedDataProvider& fuzzed_data_provider, const Chainstate& chainstate)

src/test/fuzz/util/mempool.h

@@ -6,21 +6,10 @@
 #define BITCOIN_TEST_FUZZ_UTIL_MEMPOOL_H
 
 #include <kernel/mempool_entry.h>
-#include <validation.h>
 
 class CTransaction;
-class CTxMemPool;
 class FuzzedDataProvider;
 
-class DummyChainState final : public Chainstate
-{
-public:
-    void SetMempool(CTxMemPool* mempool)
-    {
-        m_mempool = mempool;
-    }
-};
-
 [[nodiscard]] CTxMemPoolEntry ConsumeTxMemPoolEntry(FuzzedDataProvider& fuzzed_data_provider, const CTransaction& tx) noexcept;
 
 #endif // BITCOIN_TEST_FUZZ_UTIL_MEMPOOL_H

src/test/fuzz/validation_load_mempool.cpp

@@ -4,22 +4,21 @@
 
 #include <node/mempool_persist.h>
 
-#include <node/mempool_args.h>
 #include <node/mempool_persist_args.h>
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
-#include <test/fuzz/util/mempool.h>
 #include <test/util/setup_common.h>
-#include <test/util/txmempool.h>
 #include <txmempool.h>
-#include <util/check.h>
 #include <util/time.h>
-#include <util/translation.h>
 #include <validation.h>
 
-#include <cstdint>
-#include <vector>
+#include <functional>
+#include <memory>
+
+namespace fs {
+class path;
+}
 
 using node::DumpMempool;
 using node::LoadMempool;
@@ -27,12 +26,12 @@ using node::LoadMempool;
 using node::MempoolPath;
 
 namespace {
-const TestingSetup* g_setup;
+TestingSetup* g_setup;
 } // namespace
 
 void initialize_validation_load_mempool()
 {
-    static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
+    static auto testing_setup = MakeNoLogFileContext<TestingSetup>();
     g_setup = testing_setup.get();
 }
 
@@ -43,12 +42,9 @@ FUZZ_TARGET(validation_load_mempool, .init = initialize_validation_load_mempool)
     SetMockTime(ConsumeTime(fuzzed_data_provider));
     FuzzedFileProvider fuzzed_file_provider{fuzzed_data_provider};
 
-    bilingual_str error;
-    CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node), error};
-    Assert(error.empty());
+    auto& pool{g_setup->ReplaceMempool()};
 
-    auto& chainstate{static_cast<DummyChainState&>(g_setup->m_node.chainman->ActiveChainstate())};
-    chainstate.SetMempool(&pool);
+    auto& chainstate{g_setup->m_node.chainman->ActiveChainstate()};
 
     auto fuzzed_fopen = [&](const fs::path&, const char*) {
         return fuzzed_file_provider.open();

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>
 
@@ -47,14 +49,7 @@ struct MinerTestingSetup : public TestingSetup {
     }
     CTxMemPool& MakeMempool()
     {
-        // Delete the previous mempool to ensure with valgrind that the old
-        // pointer is not accessed, when the new one should be accessed
-        // instead.
-        m_node.mempool.reset();
-        bilingual_str error;
-        m_node.mempool = std::make_unique<CTxMemPool>(MemPoolOptionsForTest(m_node), error);
-        Assert(error.empty());
-        return *m_node.mempool;
+        return ReplaceMempool();
     }
     std::unique_ptr<Mining> MakeMining()
     {
@@ -123,19 +118,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 +143,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/chainstate.h

@@ -74,6 +74,7 @@ CreateAndActivateUTXOSnapshot(
             CBlockIndex *orig_tip = node.chainman->ActiveChainstate().m_chain.Tip();
             uint256 gen_hash = node.chainman->ActiveChainstate().m_chain[0]->GetBlockHash();
             node.chainman->ResetChainstates();
+            Assert(node.chainman->GetAll().size() == 0);
             node.chainman->InitializeChainstate(node.mempool.get());
             Chainstate& chain = node.chainman->ActiveChainstate();
             Assert(chain.LoadGenesisBlock());

src/test/util/setup_common.cpp

@@ -272,9 +272,9 @@ ChainTestingSetup::~ChainTestingSetup()
     m_node.addrman.reset();
     m_node.netgroupman.reset();
     m_node.args = nullptr;
+    m_node.chainman.reset();
     m_node.mempool.reset();
     Assert(!m_node.fee_estimator); // Each test must create a local object, if they wish to use the fee_estimator
-    m_node.chainman.reset();
     m_node.validation_signals.reset();
     m_node.scheduler.reset();
 }
@@ -304,6 +304,20 @@ void ChainTestingSetup::LoadVerifyActivateChainstate()
     }
 }
 
+CTxMemPool& ChainTestingSetup::ReplaceMempool()
+{
+    // Delete the previous mempool to ensure with valgrind that the old
+    // pointer is not accessed, when the new one should be accessed
+    // instead.
+    m_node.mempool.reset();
+    bilingual_str error;
+    m_node.mempool = std::make_unique<CTxMemPool>(MemPoolOptionsForTest(m_node), error);
+    Assert(error.empty());
+    auto& dummy_chainstate{static_cast<DummyChainState&>(m_node.chainman->ActiveChainstate())};
+    dummy_chainstate.SetMempool(m_node.mempool.get());
+    return *m_node.mempool;
+}
+
 TestingSetup::TestingSetup(
     const ChainType chainType,
     TestOpts opts)

src/test/util/setup_common.h

@@ -113,6 +113,8 @@ struct ChainTestingSetup : public BasicTestingSetup {
 
     // Supplies a chainstate, if one is needed
     void LoadVerifyActivateChainstate();
+
+    CTxMemPool& ReplaceMempool();
 };
 
 /** Testing setup that configures a complete environment.

src/test/util/txmempool.h

@@ -8,6 +8,7 @@
 #include <policy/packages.h>
 #include <txmempool.h>
 #include <util/time.h>
+#include <validation.h>
 
 namespace node {
 struct NodeContext;
@@ -16,6 +17,15 @@ struct PackageMempoolAcceptResult;
 
 CTxMemPool::Options MemPoolOptionsForTest(const node::NodeContext& node);
 
+class DummyChainState final : public Chainstate
+{
+public:
+    void SetMempool(CTxMemPool* mempool)
+    {
+        m_mempool = mempool;
+    }
+};
+
 struct TestMemPoolEntryHelper {
     // Default values
     CAmount nFee{0};

src/test/validation_chainstatemanager_tests.cpp

@@ -369,23 +369,12 @@ struct SnapshotTestSetup : TestChain100Setup {
     // @returns a reference to the "restarted" ChainstateManager
     ChainstateManager& SimulateNodeRestart()
     {
-        ChainstateManager& chainman = *Assert(m_node.chainman);
-
         BOOST_TEST_MESSAGE("Simulating node restart");
         {
-            for (Chainstate* cs : chainman.GetAll()) {
-                LOCK(::cs_main);
-                cs->ForceFlushStateToDisk();
-            }
-            // Process all callbacks referring to the old manager before wiping it.
-            m_node.validation_signals->SyncWithValidationInterfaceQueue();
-            LOCK(::cs_main);
-            chainman.ResetChainstates();
-            BOOST_CHECK_EQUAL(chainman.GetAll().size(), 0);
             m_node.notifications = std::make_unique<KernelNotifications>(Assert(m_node.shutdown_request), m_node.exit_status, *Assert(m_node.warnings));
             const ChainstateManager::Options chainman_opts{
                 .chainparams = ::Params(),
-                .datadir = chainman.m_options.datadir,
+                .datadir = Assert(m_node.chainman)->m_options.datadir,
                 .notifications = *m_node.notifications,
                 .signals = m_node.validation_signals.get(),
             };
@@ -397,7 +386,11 @@ struct SnapshotTestSetup : TestChain100Setup {
             // For robustness, ensure the old manager is destroyed before creating a
             // new one.
             m_node.chainman.reset();
+            // Process all callbacks referring to the old manager before creating a
+            // new one.
+            m_node.validation_signals->SyncWithValidationInterfaceQueue();
             m_node.chainman = std::make_unique<ChainstateManager>(*Assert(m_node.shutdown_signal), chainman_opts, blockman_opts);
+            BOOST_CHECK_EQUAL(m_node.chainman->GetAll().size(), 0);
         }
         return *Assert(m_node.chainman);
     }

src/validation.cpp

@@ -6320,6 +6320,11 @@ ChainstateManager::ChainstateManager(const util::SignalInterrupt& interrupt, Opt
 ChainstateManager::~ChainstateManager()
 {
     LOCK(::cs_main);
+    for (Chainstate* chainstate : GetAll()) {
+        if (chainstate->CanFlushToDisk()) {
+            chainstate->ForceFlushStateToDisk();
+        }
+    }
 
     m_versionbitscache.Clear();
 }

src/validation.h

@@ -636,9 +636,6 @@ public:
         return Assert(m_coins_views)->m_catcherview;
     }
 
-    //! Destructs all objects related to accessing the UTXO set.
-    void ResetCoinsViews() { m_coins_views.reset(); }
-
     //! Does this chainstate have a UTXO set attached?
     bool HasCoinsViews() const { return (bool)m_coins_views; }