Skip to content

Commit

Permalink
Merge bitcoin#22415: Make m_mempool optional in CChainState
Browse files Browse the repository at this point in the history 
ceb7b35 refactor: move UpdateTip into CChainState (James O'Beirne)
4abf077 refactor: no mempool arg to GetCoinsCacheSizeState (James O'Beirne)
46e3efd refactor: move UpdateMempoolForReorg into CChainState (James O'Beirne)
6176617 validation: make CChainState::m_mempool optional (James O'Beirne)

Pull request description:

  Make `CChainState::m_mempool` optional by making it a pointer instead of a reference. This will allow a simplification to assumeutxo semantics (see bitcoin#15606 (review)) and help facilitate the `-nomempool` option.

ACKs for top commit:
  jnewbery:
    ACK ceb7b35
  naumenkogs:
    ACK ceb7b35
  ryanofsky:
    Code review ACK ceb7b35 (just minor style and test tweaks since last review)
  lsilva01:
    Code review ACK and tested on Signet ACK bitcoin@ceb7b35
  MarcoFalke:
    review ACK ceb7b35 😌

Tree-SHA512: cc445ad33439d5918cacf80a6354eea8f3d33bb7719573ed5b970fad1a0dab410bcd70be44c862b8aba1b71263b82d79876688c553e339362653dfb3d8ec81e6
  • Loading branch information
MarcoFalke committed Jul 15, 2021
2 parents 97153a7 + ceb7b35 commit c0224bc
Show file tree
Hide file tree
Showing 7 changed files with 118 additions and 86 deletions.
2 changes: 1 addition & 1 deletion src/init.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1349,7 +1349,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
const int64_t load_block_index_start_time = GetTimeMillis();
try {
LOCK(cs_main);
chainman.InitializeChainstate(*Assert(node.mempool));
chainman.InitializeChainstate(Assert(node.mempool.get()));
chainman.m_total_coinstip_cache = nCoinCacheUsage;
chainman.m_total_coinsdb_cache = nCoinDBCache;

Expand Down
2 changes: 1 addition & 1 deletion src/test/util/setup_common.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
// instead of unit tests, but for now we need these here.
RegisterAllCoreRPCCommands(tableRPC);

m_node.chainman->InitializeChainstate(*m_node.mempool);
m_node.chainman->InitializeChainstate(m_node.mempool.get());
m_node.chainman->ActiveChainstate().InitCoinsDB(
/* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
assert(!m_node.chainman->ActiveChainstate().CanFlushToDisk());
Expand Down
2 changes: 1 addition & 1 deletion src/test/validation_chainstate_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ BOOST_AUTO_TEST_CASE(validation_chainstate_resize_caches)
return outp;
};

CChainState& c1 = WITH_LOCK(cs_main, return manager.InitializeChainstate(mempool));
CChainState& c1 = WITH_LOCK(cs_main, return manager.InitializeChainstate(&mempool));
c1.InitCoinsDB(
/* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
WITH_LOCK(::cs_main, c1.InitCoinsCache(1 << 23));
Expand Down
8 changes: 4 additions & 4 deletions src/test/validation_chainstatemanager_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)

// Create a legacy (IBD) chainstate.
//
CChainState& c1 = WITH_LOCK(::cs_main, return manager.InitializeChainstate(mempool));
CChainState& c1 = WITH_LOCK(::cs_main, return manager.InitializeChainstate(&mempool));
chainstates.push_back(&c1);
c1.InitCoinsDB(
/* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
Expand Down Expand Up @@ -66,7 +66,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
//
const uint256 snapshot_blockhash = GetRandHash();
CChainState& c2 = WITH_LOCK(::cs_main, return manager.InitializeChainstate(
mempool, snapshot_blockhash));
&mempool, snapshot_blockhash));
chainstates.push_back(&c2);

BOOST_CHECK_EQUAL(manager.SnapshotBlockhash().value(), snapshot_blockhash);
Expand Down Expand Up @@ -129,7 +129,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches)

// Create a legacy (IBD) chainstate.
//
CChainState& c1 = WITH_LOCK(cs_main, return manager.InitializeChainstate(mempool));
CChainState& c1 = WITH_LOCK(cs_main, return manager.InitializeChainstate(&mempool));
chainstates.push_back(&c1);
c1.InitCoinsDB(
/* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
Expand All @@ -147,7 +147,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches)

// Create a snapshot-based chainstate.
//
CChainState& c2 = WITH_LOCK(cs_main, return manager.InitializeChainstate(mempool, GetRandHash()));
CChainState& c2 = WITH_LOCK(cs_main, return manager.InitializeChainstate(&mempool, GetRandHash()));
chainstates.push_back(&c2);
c2.InitCoinsDB(
/* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
Expand Down
25 changes: 12 additions & 13 deletions src/test/validation_flush_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,9 @@ BOOST_AUTO_TEST_CASE(getcoinscachesizestate)
{
CTxMemPool mempool;
BlockManager blockman{};
CChainState chainstate{mempool, blockman};
CChainState chainstate{&mempool, blockman};
chainstate.InitCoinsDB(/*cache_size_bytes*/ 1 << 10, /*in_memory*/ true, /*should_wipe*/ false);
WITH_LOCK(::cs_main, chainstate.InitCoinsCache(1 << 10));
CTxMemPool tx_pool{};

constexpr bool is_64_bit = sizeof(void*) == 8;

Expand Down Expand Up @@ -57,7 +56,7 @@ BOOST_AUTO_TEST_CASE(getcoinscachesizestate)

// Without any coins in the cache, we shouldn't need to flush.
BOOST_CHECK_EQUAL(
chainstate.GetCoinsCacheSizeState(&tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0),
chainstate.GetCoinsCacheSizeState(MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0),
CoinsCacheSizeState::OK);

// If the initial memory allocations of cacheCoins don't match these common
Expand All @@ -72,7 +71,7 @@ BOOST_AUTO_TEST_CASE(getcoinscachesizestate)
}

BOOST_CHECK_EQUAL(
chainstate.GetCoinsCacheSizeState(&tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0),
chainstate.GetCoinsCacheSizeState(MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0),
CoinsCacheSizeState::CRITICAL);

BOOST_TEST_MESSAGE("Exiting cache flush tests early due to unsupported arch");
Expand All @@ -93,34 +92,34 @@ BOOST_AUTO_TEST_CASE(getcoinscachesizestate)
print_view_mem_usage(view);
BOOST_CHECK_EQUAL(view.AccessCoin(res).DynamicMemoryUsage(), COIN_SIZE);
BOOST_CHECK_EQUAL(
chainstate.GetCoinsCacheSizeState(&tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0),
chainstate.GetCoinsCacheSizeState(MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0),
CoinsCacheSizeState::OK);
}

// Adding some additional coins will push us over the edge to CRITICAL.
for (int i{0}; i < 4; ++i) {
add_coin(view);
print_view_mem_usage(view);
if (chainstate.GetCoinsCacheSizeState(&tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0) ==
if (chainstate.GetCoinsCacheSizeState(MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0) ==
CoinsCacheSizeState::CRITICAL) {
break;
}
}

BOOST_CHECK_EQUAL(
chainstate.GetCoinsCacheSizeState(&tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0),
chainstate.GetCoinsCacheSizeState(MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0),
CoinsCacheSizeState::CRITICAL);

// Passing non-zero max mempool usage should allow us more headroom.
BOOST_CHECK_EQUAL(
chainstate.GetCoinsCacheSizeState(&tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 1 << 10),
chainstate.GetCoinsCacheSizeState(MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 1 << 10),
CoinsCacheSizeState::OK);

for (int i{0}; i < 3; ++i) {
add_coin(view);
print_view_mem_usage(view);
BOOST_CHECK_EQUAL(
chainstate.GetCoinsCacheSizeState(&tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 1 << 10),
chainstate.GetCoinsCacheSizeState(MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 1 << 10),
CoinsCacheSizeState::OK);
}

Expand All @@ -136,31 +135,31 @@ BOOST_AUTO_TEST_CASE(getcoinscachesizestate)
BOOST_CHECK(usage_percentage >= 0.9);
BOOST_CHECK(usage_percentage < 1);
BOOST_CHECK_EQUAL(
chainstate.GetCoinsCacheSizeState(&tx_pool, MAX_COINS_CACHE_BYTES, 1 << 10),
chainstate.GetCoinsCacheSizeState(MAX_COINS_CACHE_BYTES, 1 << 10),
CoinsCacheSizeState::LARGE);
}

// Using the default max_* values permits way more coins to be added.
for (int i{0}; i < 1000; ++i) {
add_coin(view);
BOOST_CHECK_EQUAL(
chainstate.GetCoinsCacheSizeState(&tx_pool),
chainstate.GetCoinsCacheSizeState(),
CoinsCacheSizeState::OK);
}

// Flushing the view doesn't take us back to OK because cacheCoins has
// preallocated memory that doesn't get reclaimed even after flush.

BOOST_CHECK_EQUAL(
chainstate.GetCoinsCacheSizeState(&tx_pool, MAX_COINS_CACHE_BYTES, 0),
chainstate.GetCoinsCacheSizeState(MAX_COINS_CACHE_BYTES, 0),
CoinsCacheSizeState::CRITICAL);

view.SetBestBlock(InsecureRand256());
BOOST_CHECK(view.Flush());
print_view_mem_usage(view);

BOOST_CHECK_EQUAL(
chainstate.GetCoinsCacheSizeState(&tx_pool, MAX_COINS_CACHE_BYTES, 0),
chainstate.GetCoinsCacheSizeState(MAX_COINS_CACHE_BYTES, 0),
CoinsCacheSizeState::CRITICAL);
}

Expand Down
Loading

0 comments on commit c0224bc

Please sign in to comment.