foster/bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-03-05 14:06:27 -05:00
Code Issues Activity

Merge bitcoin/bitcoin#25215: [kernel 2d/n] Reduce CTxMemPool constructor call sites

Browse source 
d273e53b6e bench/rpc_mempool: Create ChainTestingSetup, use its CTxMemPool (Carl Dong)
020caba3df bench: Use existing CTxMemPool in TestingSetup (Carl Dong)
86e732def3 scripted-diff: test: Use CTxMemPool in TestingSetup (Carl Dong)
213457e170 test/policyestimator: Use ChainTestingSetup's CTxMemPool (Carl Dong)
319f0ceeeb rest/getutxos: Don't construct empty mempool (Carl Dong)
03574b956a tree-wide: clang-format CTxMemPool references (Carl Dong)

Pull request description:

  This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18

  This PR reduces the number of call sites where we explicitly construct CTxMemPool. This is done in preparation for later PRs which decouple the mempool module from `ArgsManager`, eventually all of libbitcoinkernel will be decoupled from `ArgsManager`.

  The changes in this PR:

  - Allows us to have less code churn as we modify `CTxMemPool`'s constructor in later PRs
  - In many cases, we can make use of existing `CTxMemPool` instances, getting rid of extraneous constructions
  - In other cases, we construct a `ChainTestingSetup` and use the `CTxMemPool` there, so that we can rely on the logic in `setup_common` to set things up correctly

  ## Notes for Reviewers

  ### A note on using existing mempools

  When evaluating whether or not it's appropriate to use an existing mempool in a `*TestingSetup` struct, the key is to make sure that the mempool has the same lifetime as the `*TestingSetup` struct.

  Example 1: In [`src/fuzz/tx_pool.cpp`](b4f686952a/src/test/fuzz/tx_pool.cpp), the `TestingSetup` is initialized in `initialize_tx_pool` and lives as a static global, while the `CTxMemPool` is in the `tx_pool_standard` fuzz target, meaning that each time the `tx_pool_standard` fuzz target gets run, a new `CTxMemPool` is created. If we were to use the static global `TestingSetup`'s CTxMemPool we might run into problems since its `CTxMemPool` will carry state between subsequent runs. This is why we don't modify `src/fuzz/tx_pool.cpp` in this PR.

  Example 2: In [`src/bench/mempool_eviction.cpp`](b4f686952a/src/bench/mempool_eviction.cpp), we see that the `TestingSetup` is in the same scope as the constructed `CTxMemPool`, so it is safe to use its `CTxMemPool`.

  ### A note on checking `CTxMemPool` ctor call sites

  After the "tree-wide: clang-format CTxMemPool references" commit, you can find all `CTxMemPool` ctor call sites with the following command:

  ```sh
  git grep -E -e 'make_unique<CTxMemPool>' \
              -e '\bCTxMemPool\s+[^({;]+[({]' \
              -e '\bCTxMemPool\s+[^;]+;' \
              -e '\bnew\s+CTxMemPool\b'
  ```

  At the end of the PR, you will find that there are still quite a few call sites that we can seemingly get rid of:

  ```sh
  $ git grep -E -e 'make_unique<CTxMemPool>' -e '\bCTxMemPool\s+[^({;]+[({]' -e '\bCTxMemPool\s+[^;]+;' -e '\bnew\s+CTxMemPool\b'
  # rearranged for easier explication
  src/init.cpp:        node.mempool = std::make_unique<CTxMemPool>(node.fee_estimator.get(), mempool_check_ratio);
  src/test/util/setup_common.cpp:    m_node.mempool = std::make_unique<CTxMemPool>(m_node.fee_estimator.get(), 1);
  src/rpc/mining.cpp:        CTxMemPool empty_mempool;
  src/test/util/setup_common.cpp:    CTxMemPool empty_pool;
  src/bench/mempool_stress.cpp:    CTxMemPool pool;
  src/bench/mempool_stress.cpp:    CTxMemPool pool;
  src/test/fuzz/rbf.cpp:    CTxMemPool pool;
  src/test/fuzz/tx_pool.cpp:    CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
  src/test/fuzz/tx_pool.cpp:    CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
  src/test/fuzz/validation_load_mempool.cpp:    CTxMemPool pool{};
  src/txmempool.h:    /** Create a new CTxMemPool.
  ```
  Let's break them down one by one:

  ```
  src/init.cpp:        node.mempool = std::make_unique<CTxMemPool>(node.fee_estimator.get(), mempool_check_ratio);
  src/test/util/setup_common.cpp:    m_node.mempool = std::make_unique<CTxMemPool>(m_node.fee_estimator.get(), 1);
  ```

  Necessary

  -----

  ```
  src/rpc/mining.cpp:        CTxMemPool empty_mempool;
  src/test/util/setup_common.cpp:    CTxMemPool empty_pool;
  ```

  These are fixed in #25223 where we stop requiring the `BlockAssembler` to have a `CTxMemPool` if it's not going to consult it anyway (as is the case in these two call sites)

  -----

  ```
  src/bench/mempool_stress.cpp:    CTxMemPool pool;
  src/bench/mempool_stress.cpp:    CTxMemPool pool;
  ```

  Fixed in #24927.

  -----

  ```
  src/test/fuzz/rbf.cpp:    CTxMemPool pool;
  src/test/fuzz/tx_pool.cpp:    CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
  src/test/fuzz/tx_pool.cpp:    CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
  src/test/fuzz/validation_load_mempool.cpp:    CTxMemPool pool{};
  ```

  These are all cases where we don't want the `CTxMemPool` state to persist between runs, see the previous section "A note on using existing mempools"

  -----

  ```
  src/txmempool.h:    /** Create a new CTxMemPool.
  ```

  It's a comment (someone link me to a grep that understands syntax plz thx)

ACKs for top commit:
  laanwj:
    Code review ACK d273e53b6e

Tree-SHA512: c4ff3d23217a7cc4a7145defc7b901725073ef73bcac3a252ed75f672c87e98ca0368d1d8c3f606b5b49f641e7d8387d26ef802141b650b215876f191fb6d5f9
This commit is contained in:
laanwj 2022-06-16 19:37:16 +02:00
commit 489b587669
No known key found for this signature in database
GPG key ID: 1E4AED62986CD25D
9 changed files with 29 additions and 26 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
src/bench/mempool_eviction.cpp
View file

@ -108,7 +108,7 @@ static void MempoolEviction(benchmark::Bench& bench)
tx7.vout[1].scriptPubKey = CScript() << OP_7 << OP_EQUAL;
tx7.vout[1].nValue = 10 * COIN;
CTxMemPool pool;
CTxMemPool& pool = *Assert(testing_setup->m_node.mempool);
LOCK2(cs_main, pool.cs);
// Create transaction references outside the "hot loop"
const CTransactionRef tx1_r{MakeTransactionRef(tx1)};

5
src/bench/rpc_mempool.cpp
View file

@ -3,7 +3,9 @@
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <bench/bench.h>
#include <chainparamsbase.h>
#include <rpc/mempool.h>
#include <test/util/setup_common.h>
#include <txmempool.h>
#include <univalue.h>
@ -17,7 +19,8 @@ static void AddTx(const CTransactionRef& tx, const CAmount& fee, CTxMemPool& poo
static void RpcMempool(benchmark::Bench& bench)
{
CTxMemPool pool;
const auto testing_setup = MakeNoLogFileContext<const ChainTestingSetup>(CBaseChainParams::MAIN);
CTxMemPool& pool = *Assert(testing_setup->m_node.mempool);
LOCK2(cs_main, pool.cs);
for (int i = 0; i < 1000; ++i) {

2
src/net_processing.cpp
View file

@ -4635,7 +4635,7 @@ void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, Peer& peer, std::chrono::mi
namespace {
class CompareInvMempoolOrder
{
CTxMemPool *mp;
CTxMemPool* mp;
bool m_wtxid_relay;
public:
explicit CompareInvMempoolOrder(CTxMemPool *_mempool, bool use_wtxid)

10
src/rest.cpp
View file

@ -799,10 +799,10 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
if (!maybe_chainman) return false;
ChainstateManager& chainman = *maybe_chainman;
{
auto process_utxos = [&vOutPoints, &outs, &hits](const CCoinsView& view, const CTxMemPool& mempool) {
auto process_utxos = [&vOutPoints, &outs, &hits](const CCoinsView& view, const CTxMemPool* mempool) {
for (const COutPoint& vOutPoint : vOutPoints) {
Coin coin;
bool hit = !mempool.isSpent(vOutPoint) && view.GetCoin(vOutPoint, coin);
bool hit = (!mempool || !mempool->isSpent(vOutPoint)) && view.GetCoin(vOutPoint, coin);
hits.push_back(hit);
if (hit) outs.emplace_back(std::move(coin));
}
@ -815,10 +815,10 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
LOCK2(cs_main, mempool->cs);
CCoinsViewCache& viewChain = chainman.ActiveChainstate().CoinsTip();
CCoinsViewMemPool viewMempool(&viewChain, *mempool);
process_utxos(viewMempool, *mempool);
process_utxos(viewMempool, mempool);
} else {
LOCK(cs_main); // no need to lock mempool!
process_utxos(chainman.ActiveChainstate().CoinsTip(), CTxMemPool());
LOCK(cs_main);
process_utxos(chainman.ActiveChainstate().CoinsTip(), nullptr);
}
for (size_t i = 0; i < hits.size(); ++i) {

8
src/test/blockencodings_tests.cpp
View file

@ -54,7 +54,7 @@ constexpr long SHARED_TX_OFFSET{3};
BOOST_AUTO_TEST_CASE(SimpleRoundTripTest)
{
CTxMemPool pool;
CTxMemPool& pool = *Assert(m_node.mempool);
TestMemPoolEntryHelper entry;
CBlock block(BuildBlockTestCase());
@ -137,7 +137,7 @@ public:
BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest)
{
CTxMemPool pool;
CTxMemPool& pool = *Assert(m_node.mempool);
TestMemPoolEntryHelper entry;
CBlock block(BuildBlockTestCase());
@ -207,7 +207,7 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest)
BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest)
{
CTxMemPool pool;
CTxMemPool& pool = *Assert(m_node.mempool);
TestMemPoolEntryHelper entry;
CBlock block(BuildBlockTestCase());
@ -258,7 +258,7 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest)
BOOST_AUTO_TEST_CASE(EmptyBlockRoundTripTest)
{
CTxMemPool pool;
CTxMemPool& pool = *Assert(m_node.mempool);
CMutableTransaction coinbase;
coinbase.vin.resize(1);
coinbase.vin[0].scriptSig.resize(10);

18
src/test/mempool_tests.cpp
View file

@ -56,7 +56,7 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest)
}
CTxMemPool testPool;
CTxMemPool& testPool = *Assert(m_node.mempool);
LOCK2(cs_main, testPool.cs);
// Nothing in pool, remove should do nothing:
@ -108,12 +108,12 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest)
BOOST_CHECK_EQUAL(testPool.size(), 0U);
}
template<typename name>
static void CheckSort(CTxMemPool &pool, std::vector<std::string> &sortedOrder) EXCLUSIVE_LOCKS_REQUIRED(pool.cs)
template <typename name>
static void CheckSort(CTxMemPool& pool, std::vector<std::string>& sortedOrder) EXCLUSIVE_LOCKS_REQUIRED(pool.cs)
{
BOOST_CHECK_EQUAL(pool.size(), sortedOrder.size());
typename CTxMemPool::indexed_transaction_set::index<name>::type::iterator it = pool.mapTx.get<name>().begin();
int count=0;
int count = 0;
for (; it != pool.mapTx.get<name>().end(); ++it, ++count) {
BOOST_CHECK_EQUAL(it->GetTx().GetHash().ToString(), sortedOrder[count]);
}
@ -121,7 +121,7 @@ static void CheckSort(CTxMemPool &pool, std::vector<std::string> &sortedOrder) E
BOOST_AUTO_TEST_CASE(MempoolIndexingTest)
{
CTxMemPool pool;
CTxMemPool& pool = *Assert(m_node.mempool);
LOCK2(cs_main, pool.cs);
TestMemPoolEntryHelper entry;
@ -294,7 +294,7 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest)
BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest)
{
CTxMemPool pool;
CTxMemPool& pool = *Assert(m_node.mempool);
LOCK2(cs_main, pool.cs);
TestMemPoolEntryHelper entry;
@ -423,7 +423,7 @@ BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest)
BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest)
{
CTxMemPool pool;
CTxMemPool& pool = *Assert(m_node.mempool);
LOCK2(cs_main, pool.cs);
TestMemPoolEntryHelper entry;
@ -594,7 +594,7 @@ BOOST_AUTO_TEST_CASE(MempoolAncestryTests)
{
size_t ancestors, descendants;
CTxMemPool pool;
CTxMemPool& pool = *Assert(m_node.mempool);
LOCK2(cs_main, pool.cs);
TestMemPoolEntryHelper entry;
@ -753,7 +753,7 @@ BOOST_AUTO_TEST_CASE(MempoolAncestryTestsDiamond)
{
size_t ancestors, descendants;
CTxMemPool pool;
CTxMemPool& pool = *Assert(m_node.mempool);
LOCK2(::cs_main, pool.cs);
TestMemPoolEntryHelper entry;

6
src/test/policyestimator_tests.cpp
View file

@ -12,12 +12,12 @@
#include <boost/test/unit_test.hpp>
BOOST_FIXTURE_TEST_SUITE(policyestimator_tests, BasicTestingSetup)
BOOST_FIXTURE_TEST_SUITE(policyestimator_tests, ChainTestingSetup)
BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
{
CBlockPolicyEstimator feeEst;
CTxMemPool mpool(&feeEst);
CBlockPolicyEstimator& feeEst = *Assert(m_node.fee_estimator);
CTxMemPool& mpool = *Assert(m_node.mempool);
LOCK2(cs_main, mpool.cs);
TestMemPoolEntryHelper entry;
CAmount basefee(2000);

2
src/test/validation_chainstate_tests.cpp
View file

@ -30,7 +30,7 @@ BOOST_AUTO_TEST_CASE(validation_chainstate_resize_caches)
ChainstateManager manager{chainman_opts};
WITH_LOCK(::cs_main, manager.m_blockman.m_block_tree_db = std::make_unique<CBlockTreeDB>(1 << 20, true));
CTxMemPool mempool;
CTxMemPool& mempool = *Assert(m_node.mempool);
//! Create and add a Coin with DynamicMemoryUsage of 80 bytes to the given view.
auto add_coin = [](CCoinsViewCache& coins_view) -> COutPoint {

2
src/test/validation_flush_tests.cpp
View file

@ -20,7 +20,7 @@ BOOST_FIXTURE_TEST_SUITE(validation_flush_tests, ChainTestingSetup)
//!
BOOST_AUTO_TEST_CASE(getcoinscachesizestate)
{
CTxMemPool mempool;
CTxMemPool& mempool = *Assert(m_node.mempool);
BlockManager blockman{};
CChainState chainstate{&mempool, blockman, *Assert(m_node.chainman)};
chainstate.InitCoinsDB(/*cache_size_bytes=*/1 << 10, /*in_memory=*/true, /*should_wipe=*/false);