Merge 0e0322767c 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/blockstorage.cpp

@@ -1074,6 +1074,13 @@ bool BlockManager::ReadBlockFromDisk(CBlock& block, const CBlockIndex& index) co
     return true;
 }
 
+void BlockManager::CleanupForFuzzing()
+{
+    m_dirty_blockindex.clear();
+    m_dirty_fileinfo.clear();
+    m_blockfile_info.resize(1);
+}
+
 bool BlockManager::ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos) const
 {
     FlatFilePos hpos = pos;

src/node/blockstorage.h

@@ -428,6 +428,8 @@ public:
     bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex& index) const;
 
     void CleanupBlockRevFiles() const;
+    /** Clear internal state (test-only, only for fuzzing) **/
+    void CleanupForFuzzing();
 };
 
 // Calls ActivateBestChain() even if no blocks are imported.

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/CMakeLists.txt

@@ -19,6 +19,7 @@ add_executable(fuzz
   block.cpp
   block_header.cpp
   block_index.cpp
+  block_index_tree.cpp
   blockfilter.cpp
   bloom_filter.cpp
   buffered_file.cpp

src/test/fuzz/block_index_tree.cpp

@@ -0,0 +1,201 @@
+// Copyright (c) 2020-2022 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 <chain.h>
+#include <chainparams.h>
+#include <cstdint>
+#include <flatfile.h>
+#include <test/fuzz/FuzzedDataProvider.h>
+#include <test/fuzz/fuzz.h>
+#include <test/fuzz/util.h>
+#include <test/util/setup_common.h>
+
+#include <optional>
+#include <ranges>
+#include <validation.h>
+#include <vector>
+
+const TestingSetup* g_setup;
+
+CBlockHeader ConsumeBlockHeader(FuzzedDataProvider& provider, uint256 prev_hash, int& nonce_counter)
+{
+    CBlockHeader header;
+    header.nVersion = provider.ConsumeIntegral<decltype(header.nVersion)>();
+    header.hashPrevBlock = prev_hash;
+    header.hashMerkleRoot = uint256{}; // never used
+    header.nTime = provider.ConsumeIntegral<decltype(header.nTime)>();
+    header.nBits = Params().GenesisBlock().nBits;
+    header.nNonce = nonce_counter++; // prevent creating multiple block headers with the same hash
+    return header;
+}
+
+void initialize_block_index_tree()
+{
+    static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
+    g_setup = testing_setup.get();
+}
+
+FUZZ_TARGET(block_index_tree, .init = initialize_block_index_tree)
+{
+    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
+    ChainstateManager& chainman = *g_setup->m_node.chainman;
+    auto& blockman = chainman.m_blockman;
+    CBlockIndex* genesis = chainman.ActiveChainstate().m_chain[0];
+    int nonce_counter = 0;
+    std::vector<CBlockIndex*> blocks;
+    blocks.push_back(genesis);
+
+    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 1000)
+    {
+        CallOneOf(
+            fuzzed_data_provider,
+            [&] {
+                // Receive a header building on an existing one. This assumes headers are valid, so PoW is not relevant here.
+                LOCK(cs_main);
+                CBlockIndex* prev_block = PickValue(fuzzed_data_provider, blocks);
+                if (!(prev_block->nStatus & BLOCK_FAILED_MASK)) {
+                    CBlockHeader header = ConsumeBlockHeader(fuzzed_data_provider, prev_block->GetBlockHash(), nonce_counter);
+                    CBlockIndex* index = blockman.AddToBlockIndex(header, chainman.m_best_header);
+                    assert(index->nStatus & BLOCK_VALID_TREE);
+                }
+            },
+            [&] {
+                // Receive a full block (valid or invalid) for an existing header, but don't attempt to connect it yet
+                LOCK(cs_main);
+                CBlockIndex* index = PickValue(fuzzed_data_provider, blocks);
+                // Must be new to us and not known to be invalid (e.g. because of an invalid ancestor).
+                if (index->nTx == 0 && !(index->nStatus & BLOCK_FAILED_MASK)) {
+                    if (fuzzed_data_provider.ConsumeBool()) { // Invalid
+                        BlockValidationState state;
+                        state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "consensus-invalid");
+                        chainman.ActiveChainstate().InvalidBlockFound(index, state);
+                    } else {
+                        size_t nTx = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, 1000);
+                        CBlock block; // Dummy block, so that ReceivedBlockTransaction can infer a nTx value.
+                        block.vtx = std::vector<CTransactionRef>(nTx);
+                        FlatFilePos pos(0, fuzzed_data_provider.ConsumeIntegralInRange<int>(1, 1000));
+                        chainman.ReceivedBlockTransactions(block, index, pos);
+                        assert(index->nStatus & BLOCK_VALID_TRANSACTIONS);
+                        assert(index->nStatus & BLOCK_HAVE_DATA);
+                    }
+                }
+            },
+            [&] {
+                // Simplified ActivateBestChain(): Try to move to a chain with more work - with the possibility of finding blocks to be invalid on the way
+                LOCK(cs_main);
+                auto& chain = chainman.ActiveChain();
+                CBlockIndex* old_tip = chain.Tip();
+                assert(old_tip);
+                do {
+                    CBlockIndex* best_tip = chainman.ActiveChainstate().FindMostWorkChain();
+                    assert(best_tip);                   // Should at least return current tip
+                    if (best_tip == chain.Tip()) break; // Nothing to do
+                    // Rewind chain to forking point
+                    const CBlockIndex* fork = chain.FindFork(best_tip);
+                    // If we can't go back to the fork point due to pruned data, abort and don't do anything. Note that this check does not exist in validation.cpp, where
+                    // the node would currently just crash in this scenario (although this is very unlikely to happen due to the minimum pruning threshold of 550MiB).
+                    CBlockIndex* it = chain.Tip();
+                    bool pruned_block{false};
+                    while (it && it->nHeight != fork->nHeight) {
+                        if (!(it->nStatus & BLOCK_HAVE_UNDO) && it->nHeight > 0) {
+                            assert(blockman.m_have_pruned);
+                            pruned_block = true;
+                            break;
+                        }
+                        it = it->pprev;
+                    }
+                    if (pruned_block) break;
+
+                    chain.SetTip(*chain[fork->nHeight]);
+
+                    // Prepare new blocks to connect
+                    std::vector<CBlockIndex*> to_connect;
+                    it = best_tip;
+                    while (it && it->nHeight != fork->nHeight) {
+                        to_connect.push_back(it);
+                        it = it->pprev;
+                    }
+                    // Connect blocks, possibly fail
+                    for (CBlockIndex* block : to_connect | std::views::reverse) {
+                        assert(!(block->nStatus & BLOCK_FAILED_MASK));
+                        assert(block->nStatus & BLOCK_HAVE_DATA);
+                        if (!block->IsValid(BLOCK_VALID_SCRIPTS)) {
+                            if (fuzzed_data_provider.ConsumeBool()) { // Invalid
+                                BlockValidationState state;
+                                state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "consensus-invalid");
+                                chainman.ActiveChainstate().InvalidBlockFound(block, state);
+                                break;
+                            } else {
+                                block->RaiseValidity(BLOCK_VALID_SCRIPTS);
+                                block->nStatus |= BLOCK_HAVE_UNDO;
+                            }
+                        }
+                        chain.SetTip(*block);
+                        chainman.ActiveChainstate().PruneBlockIndexCandidates();
+                        // ABC may release cs_main / not connect all blocks in one go - but only if we have at least much chain work as we had at the start.
+                        if (block->nChainWork > old_tip->nChainWork && fuzzed_data_provider.ConsumeBool()) {
+                            break;
+                        }
+                    }
+                } while (node::CBlockIndexWorkComparator()(chain.Tip(), old_tip));
+                assert(chain.Tip()->nChainWork >= old_tip->nChainWork);
+            },
+            [&] {
+                // Prune chain - dealing with block files is beyond the scope of this test, so just prune random blocks, making no assumptions what must
+                // be together in a block file.
+                // Also don't prune blocks outside of the chain for now - this would make the fuzzer crash because of the problem describted in
+                // https://github.com/bitcoin/bitcoin/issues/31512
+                LOCK(cs_main);
+                auto& chain = chainman.ActiveChain();
+                int prune_height = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, chain.Height());
+                CBlockIndex* prune_block{chain[prune_height]};
+                if (prune_block != chain.Tip()) {
+                    blockman.m_have_pruned = true;
+                    prune_block->nStatus &= ~BLOCK_HAVE_DATA;
+                    prune_block->nStatus &= ~BLOCK_HAVE_UNDO;
+                    prune_block->nFile = 0;
+                    prune_block->nDataPos = 0;
+                    prune_block->nUndoPos = 0;
+                    auto range = blockman.m_blocks_unlinked.equal_range(prune_block->pprev);
+                    while (range.first != range.second) {
+                        std::multimap<CBlockIndex*, CBlockIndex*>::iterator _it = range.first;
+                        range.first++;
+                        if (_it->second == prune_block) {
+                            blockman.m_blocks_unlinked.erase(_it);
+                        }
+                    }
+                }
+            });
+    }
+    chainman.CheckBlockIndex();
+
+    // clean up global state changed by last iteration and prepare for next iteration
+    {
+        LOCK(cs_main);
+        genesis->nStatus |= BLOCK_HAVE_DATA;
+        genesis->nStatus |= BLOCK_HAVE_UNDO;
+        chainman.m_best_header = genesis;
+        chainman.m_best_invalid = nullptr;
+        chainman.nBlockSequenceId = 1;
+        chainman.ActiveChain().SetTip(*genesis);
+        chainman.ActiveChainstate().setBlockIndexCandidates.clear();
+        chainman.m_failed_blocks.clear();
+        blockman.m_blocks_unlinked.clear();
+        blockman.m_have_pruned = false;
+        blockman.CleanupForFuzzing();
+        // Delete all blocks but Genesis from block index
+        uint256 genesis_hash = genesis->GetBlockHash();
+        for (auto it = blockman.m_block_index.begin(); it != blockman.m_block_index.end();) {
+            if (it->first != genesis_hash) {
+                it = blockman.m_block_index.erase(it);
+            } else {
+                ++it;
+            }
+        }
+        chainman.ActiveChainstate().TryAddBlockIndexCandidate(genesis);
+        assert(blockman.m_block_index.size() == 1);
+        assert(chainman.ActiveChainstate().setBlockIndexCandidates.size() == 1);
+        assert(chainman.ActiveChain().Height() == 0);
+    }
+}

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/validation.h

@@ -768,13 +768,13 @@ public:
     {
         return m_mempool ? &m_mempool->cs : nullptr;
     }
+    void InvalidBlockFound(CBlockIndex* pindex, const BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    CBlockIndex* FindMostWorkChain() EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
 private:
     bool ActivateBestChainStep(BlockValidationState& state, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);
     bool ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);
 
-    void InvalidBlockFound(CBlockIndex* pindex, const BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
-    CBlockIndex* FindMostWorkChain() EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     bool RollforwardBlock(const CBlockIndex* pindex, CCoinsViewCache& inputs) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
@@ -899,7 +899,6 @@ private:
     //! most-work chain.
     Chainstate* m_active_chainstate GUARDED_BY(::cs_main) {nullptr};
 
-    CBlockIndex* m_best_invalid GUARDED_BY(::cs_main){nullptr};
 
     /** The last header for which a headerTip notification was issued. */
     CBlockIndex* m_last_notified_header GUARDED_BY(GetMutex()){nullptr};
@@ -1061,6 +1060,7 @@ public:
 
     /** Best header we've seen so far (used for getheaders queries' starting points). */
     CBlockIndex* m_best_header GUARDED_BY(::cs_main){nullptr};
+    CBlockIndex* m_best_invalid GUARDED_BY(::cs_main){nullptr};
 
     //! The total number of bytes available for us to use across all in-memory
     //! coins caches. This will be split somehow across chainstates.