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

CMakeLists.txt

@@ -172,6 +172,8 @@ option(BUILD_BENCH "Build bench_bitcoin executable." OFF)
 option(BUILD_FUZZ_BINARY "Build fuzz binary." OFF)
 option(BUILD_FOR_FUZZING "Build for fuzzing. Enabling this will disable all other targets and override BUILD_FUZZ_BINARY." OFF)
 
+option(BUILD_FOR_COVERAGE "Build with Code Coverage instrumentation" OFF)
+
 option(INSTALL_MAN "Install man pages." ON)
 
 set(APPEND_CPPFLAGS "" CACHE STRING "Preprocessor flags that are appended to the command line after all other flags added by the build system. This variable is intended for debugging and special builds.")
@@ -381,6 +383,77 @@ if(BUILD_FUZZ_BINARY)
   )
 endif()
 
+# The coverage_interface library is intended to be used
+# only for code whose coverage report is of interest.
+add_library(coverage_interface INTERFACE)
+if(BUILD_FOR_COVERAGE)
+  if(NOT CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
+    message(FATAL_ERROR "BUILD_FOR_COVERAGE can only be specified when compiling with Clang.")
+  endif()
+  if(NOT PROFILE_FILE_PATTERN)
+    if(NOT PROFILE_DATA_DIR)
+      file(TO_NATIVE_PATH "${PROJECT_BINARY_DIR}/profiles" PROFILE_DATA_DIR)
+    endif()
+    file(TO_NATIVE_PATH "${PROFILE_DATA_DIR}/%m.profraw" PROFILE_FILE_PATTERN)
+  endif()
+  target_compile_options(coverage_interface INTERFACE
+    -fprofile-instr-generate=${PROFILE_FILE_PATTERN}
+    -fcoverage-mapping
+  )
+  target_link_options(coverage_interface INTERFACE
+    -fprofile-instr-generate=${PROFILE_FILE_PATTERN}
+    -fcoverage-mapping
+  )
+
+  set(coverage_ignore_paths
+    src/leveldb/
+    src/crc32c/
+    src/crypto/ctaes/
+    src/minisketch/
+    src/secp256k1/
+  )
+  list(TRANSFORM coverage_ignore_paths PREPEND "-ignore-filename-regex=")
+
+  if(NOT BUILD_FOR_FUZZING)
+    add_custom_command(
+      OUTPUT ${PROJECT_BINARY_DIR}/test_bitcoin.coverage/index.html
+      COMMAND ${CMAKE_CTEST_COMMAND} --test-dir ${PROJECT_BINARY_DIR} --exclude-regex secp256k1_
+      COMMAND llvm-profdata merge --instr ${PROFILE_DATA_DIR}/*.profraw --output ${PROFILE_DATA_DIR}/test_bitcoin.profdata
+      COMMAND llvm-cov show -instr-profile=${PROFILE_DATA_DIR}/test_bitcoin.profdata -object=$<TARGET_FILE:test_bitcoin> -object=$<TARGET_FILE:bitcoin-util> -object=$<TARGET_FILE:bitcoin-tx> -output-dir=${PROJECT_BINARY_DIR}/test_bitcoin.coverage -format=html ${coverage_ignore_paths}
+      DEPENDS test_bitcoin bitcoin-util bitcoin-tx unitester object
+    )
+    add_custom_target(test_bitcoin_coverage
+      DEPENDS ${PROJECT_BINARY_DIR}/test_bitcoin.coverage/index.html
+    )
+
+    add_custom_command(
+      OUTPUT ${PROJECT_BINARY_DIR}/total.coverage/index.html
+      COMMAND Python3::Interpreter ${PROJECT_BINARY_DIR}/test/functional/test_runner.py
+      COMMAND llvm-profdata merge --instr ${PROFILE_DATA_DIR}/*.profraw --output ${PROFILE_DATA_DIR}/total.profdata
+      COMMAND llvm-cov show -instr-profile=${PROFILE_DATA_DIR}/total.profdata -object=$<TARGET_FILE:bitcoind> -object=$<TARGET_FILE:bitcoin-cli> -object=$<TARGET_FILE:bitcoin-wallet> -object=$<TARGET_FILE:test_bitcoin> -object=$<TARGET_FILE:bitcoin-util> -object=$<TARGET_FILE:bitcoin-tx> -output-dir=${PROJECT_BINARY_DIR}/total.coverage -format=html ${coverage_ignore_paths}
+      DEPENDS bitcoind bitcoin-cli bitcoin-wallet
+    )
+    add_custom_target(total_coverage
+      DEPENDS ${PROJECT_BINARY_DIR}/total.coverage/index.html
+    )
+    add_dependencies(total_coverage test_bitcoin_coverage)
+  else()
+    if(NOT FUZZ_CORPORA_DIR)
+      set(FUZZ_CORPORA_DIR ${CMAKE_CURRENT_SOURCE_DIR}/qa-assets/fuzz_corpora)
+    endif()
+    add_custom_command(
+      OUTPUT ${PROJECT_BINARY_DIR}/fuzz.coverage/index.html
+      COMMAND Python3::Interpreter test/fuzz/test_runner.py ${FUZZ_CORPORA_DIR} --loglevel DEBUG
+      COMMAND llvm-profdata merge --instr ${PROFILE_DATA_DIR}/*.profraw --output ${PROFILE_DATA_DIR}/fuzz.profdata
+      COMMAND llvm-cov show -instr-profile=${PROFILE_DATA_DIR}/fuzz.profdata -object=$<TARGET_FILE:fuzz> -output-dir=${PROJECT_BINARY_DIR}/fuzz.coverage -format=html ${coverage_ignore_paths}
+      DEPENDS fuzz
+    )
+    add_custom_target(fuzz_coverage
+      DEPENDS ${PROJECT_BINARY_DIR}/fuzz.coverage/index.html
+    )
+  endif()
+endif()
+
 include(AddBoostIfNeeded)
 add_boost_if_needed()
 
@@ -633,6 +706,7 @@ message("  test_bitcoin ........................ ${BUILD_TESTS}")
 message("  test_bitcoin-qt ..................... ${BUILD_GUI_TESTS}")
 message("  bench_bitcoin ....................... ${BUILD_BENCH}")
 message("  fuzz binary ......................... ${BUILD_FUZZ_BINARY}")
+message("  Code Coverage instrumentation ....... ${BUILD_FOR_COVERAGE}")
 message("")
 if(CMAKE_CROSSCOMPILING)
   set(cross_status "TRUE, for ${CMAKE_SYSTEM_NAME}, ${CMAKE_SYSTEM_PROCESSOR}")

src/CMakeLists.txt

@@ -20,6 +20,7 @@ add_library(bitcoin_clientversion OBJECT EXCLUDE_FROM_ALL
 target_link_libraries(bitcoin_clientversion
   PRIVATE
     core_interface
+    coverage_interface
 )
 add_dependencies(bitcoin_clientversion generate_build_info)
 
@@ -95,6 +96,7 @@ add_library(bitcoin_consensus STATIC EXCLUDE_FROM_ALL
 target_link_libraries(bitcoin_consensus
   PRIVATE
     core_interface
+    coverage_interface
     bitcoin_crypto
     secp256k1
 )
@@ -159,6 +161,7 @@ add_library(bitcoin_common STATIC EXCLUDE_FROM_ALL
 target_link_libraries(bitcoin_common
   PRIVATE
     core_interface
+    coverage_interface
     bitcoin_consensus
     bitcoin_util
     univalue
@@ -182,6 +185,7 @@ if(ENABLE_WALLET)
     add_windows_resources(bitcoin-wallet bitcoin-wallet-res.rc)
     target_link_libraries(bitcoin-wallet
       core_interface
+      coverage_interface
       bitcoin_wallet
       bitcoin_common
       bitcoin_util
@@ -292,6 +296,7 @@ add_library(bitcoin_node STATIC EXCLUDE_FROM_ALL
 target_link_libraries(bitcoin_node
   PRIVATE
     core_interface
+    coverage_interface
     bitcoin_common
     bitcoin_util
     $<TARGET_NAME_IF_EXISTS:bitcoin_zmq>
@@ -315,6 +320,7 @@ if(BUILD_DAEMON)
   add_windows_resources(bitcoind bitcoind-res.rc)
   target_link_libraries(bitcoind
     core_interface
+    coverage_interface
     bitcoin_node
     $<TARGET_NAME_IF_EXISTS:bitcoin_wallet>
   )
@@ -327,6 +333,7 @@ if(WITH_MULTIPROCESS)
   )
   target_link_libraries(bitcoin-node
     core_interface
+    coverage_interface
     bitcoin_node
     bitcoin_ipc
     $<TARGET_NAME_IF_EXISTS:bitcoin_wallet>
@@ -358,6 +365,7 @@ add_library(bitcoin_cli STATIC EXCLUDE_FROM_ALL
 target_link_libraries(bitcoin_cli
   PUBLIC
     core_interface
+    coverage_interface
     univalue
 )
 
@@ -368,6 +376,7 @@ if(BUILD_CLI)
   add_windows_resources(bitcoin-cli bitcoin-cli-res.rc)
   target_link_libraries(bitcoin-cli
     core_interface
+    coverage_interface
     bitcoin_cli
     bitcoin_common
     bitcoin_util
@@ -383,6 +392,7 @@ if(BUILD_TX)
   add_windows_resources(bitcoin-tx bitcoin-tx-res.rc)
   target_link_libraries(bitcoin-tx
     core_interface
+    coverage_interface
     bitcoin_common
     bitcoin_util
     univalue
@@ -396,6 +406,7 @@ if(BUILD_UTIL)
   add_windows_resources(bitcoin-util bitcoin-util-res.rc)
   target_link_libraries(bitcoin-util
     core_interface
+    coverage_interface
     bitcoin_common
     bitcoin_util
   )
@@ -428,6 +439,7 @@ if(BUILD_UTIL_CHAINSTATE)
   target_link_libraries(bitcoin-chainstate
     PRIVATE
       core_interface
+      coverage_interface
       bitcoinkernel
   )
 endif()

src/crypto/CMakeLists.txt

@@ -25,6 +25,7 @@ add_library(bitcoin_crypto STATIC EXCLUDE_FROM_ALL
 target_link_libraries(bitcoin_crypto
   PRIVATE
     core_interface
+    coverage_interface
 )
 
 if(HAVE_SSE41)
@@ -33,7 +34,7 @@ if(HAVE_SSE41)
   )
   target_compile_definitions(bitcoin_crypto_sse41 PUBLIC ENABLE_SSE41)
   target_compile_options(bitcoin_crypto_sse41 PRIVATE ${SSE41_CXXFLAGS})
-  target_link_libraries(bitcoin_crypto_sse41 PRIVATE core_interface)
+  target_link_libraries(bitcoin_crypto_sse41 PRIVATE core_interface coverage_interface)
   target_link_libraries(bitcoin_crypto PRIVATE bitcoin_crypto_sse41)
 endif()
 
@@ -43,7 +44,7 @@ if(HAVE_AVX2)
   )
   target_compile_definitions(bitcoin_crypto_avx2 PUBLIC ENABLE_AVX2)
   target_compile_options(bitcoin_crypto_avx2 PRIVATE ${AVX2_CXXFLAGS})
-  target_link_libraries(bitcoin_crypto_avx2 PRIVATE core_interface)
+  target_link_libraries(bitcoin_crypto_avx2 PRIVATE core_interface coverage_interface)
   target_link_libraries(bitcoin_crypto PRIVATE bitcoin_crypto_avx2)
 endif()
 
@@ -53,7 +54,7 @@ if(HAVE_SSE41 AND HAVE_X86_SHANI)
   )
   target_compile_definitions(bitcoin_crypto_x86_shani PUBLIC ENABLE_SSE41 ENABLE_X86_SHANI)
   target_compile_options(bitcoin_crypto_x86_shani PRIVATE ${X86_SHANI_CXXFLAGS})
-  target_link_libraries(bitcoin_crypto_x86_shani PRIVATE core_interface)
+  target_link_libraries(bitcoin_crypto_x86_shani PRIVATE core_interface coverage_interface)
   target_link_libraries(bitcoin_crypto PRIVATE bitcoin_crypto_x86_shani)
 endif()
 
@@ -63,6 +64,6 @@ if(HAVE_ARM_SHANI)
   )
   target_compile_definitions(bitcoin_crypto_arm_shani PUBLIC ENABLE_ARM_SHANI)
   target_compile_options(bitcoin_crypto_arm_shani PRIVATE ${ARM_SHANI_CXXFLAGS})
-  target_link_libraries(bitcoin_crypto_arm_shani PRIVATE core_interface)
+  target_link_libraries(bitcoin_crypto_arm_shani PRIVATE core_interface coverage_interface)
   target_link_libraries(bitcoin_crypto PRIVATE bitcoin_crypto_arm_shani)
 endif()

src/ipc/CMakeLists.txt

@@ -19,5 +19,6 @@ target_capnp_sources(bitcoin_ipc ${PROJECT_SOURCE_DIR}
 target_link_libraries(bitcoin_ipc
   PRIVATE
     core_interface
+    coverage_interface
     univalue
 )

src/kernel/CMakeLists.txt

@@ -81,6 +81,7 @@ add_library(bitcoinkernel
 target_link_libraries(bitcoinkernel
   PRIVATE
     core_interface
+    coverage_interface
     bitcoin_clientversion
     bitcoin_crypto
     leveldb

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

@@ -147,6 +147,7 @@ add_executable(test_bitcoin
 
 target_link_libraries(test_bitcoin
   core_interface
+  coverage_interface
   test_util
   bitcoin_cli
   bitcoin_node
@@ -165,6 +166,7 @@ if(WITH_MULTIPROCESS)
   target_link_libraries(bitcoin_ipc_test
     PRIVATE
       core_interface
+      coverage_interface
       univalue
   )
 

src/test/fuzz/CMakeLists.txt

@@ -135,6 +135,7 @@ add_executable(fuzz
 )
 target_link_libraries(fuzz
   core_interface
+  coverage_interface
   test_fuzz
   bitcoin_cli
   bitcoin_common

src/test/fuzz/util/CMakeLists.txt

@@ -14,6 +14,7 @@ add_library(test_fuzz STATIC EXCLUDE_FROM_ALL
 target_link_libraries(test_fuzz
   PRIVATE
     core_interface
+    coverage_interface
     test_util
     bitcoin_node
     Boost::headers

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

@@ -23,6 +23,7 @@ add_library(test_util STATIC EXCLUDE_FROM_ALL
 target_link_libraries(test_util
   PRIVATE
     core_interface
+    coverage_interface
     Boost::headers
   PUBLIC
     univalue

src/univalue/CMakeLists.txt

@@ -12,7 +12,11 @@ target_include_directories(univalue
   PUBLIC
     $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
 )
-target_link_libraries(univalue PRIVATE core_interface)
+target_link_libraries(univalue
+  PRIVATE
+    core_interface
+    coverage_interface
+)
 
 if(BUILD_TESTS)
   include(GenerateHeaders)
@@ -132,6 +136,7 @@ if(BUILD_TESTS)
   target_link_libraries(unitester
     PRIVATE
       core_interface
+      coverage_interface
       univalue
   )
   add_test(NAME univalue_test
@@ -142,6 +147,7 @@ if(BUILD_TESTS)
   target_link_libraries(object
     PRIVATE
       core_interface
+      coverage_interface
       univalue
   )
   add_test(NAME univalue_object_test

src/util/CMakeLists.txt

@@ -39,6 +39,7 @@ add_library(bitcoin_util STATIC EXCLUDE_FROM_ALL
 target_link_libraries(bitcoin_util
   PRIVATE
     core_interface
+    coverage_interface
     bitcoin_clientversion
     bitcoin_crypto
     $<$<PLATFORM_ID:Windows>:ws2_32>

src/wallet/CMakeLists.txt

@@ -36,6 +36,7 @@ add_library(bitcoin_wallet STATIC EXCLUDE_FROM_ALL
 target_link_libraries(bitcoin_wallet
   PRIVATE
     core_interface
+    coverage_interface
     bitcoin_common
     univalue
     Boost::headers

src/zmq/CMakeLists.txt

@@ -16,6 +16,7 @@ target_compile_definitions(bitcoin_zmq
 target_link_libraries(bitcoin_zmq
   PRIVATE
     core_interface
+    coverage_interface
     univalue
     zeromq
 )