highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-06-02 17:24:58 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#27861: kernel: Rm ShutdownRequested and AbortNode from validation code.

Browse Source 
6eb33bd0c2 kernel: Add fatalError method to notifications (TheCharlatan)
7320db96f8 kernel: Add flushError method to notifications (TheCharlatan)
3fa9094b92 scripted-diff: Rename FatalError to FatalErrorf (TheCharlatan)
edb55e2777 kernel: Pass interrupt reference to chainman (TheCharlatan)
e2d680a32d util: Add SignalInterrupt class and use in shutdown.cpp (TheCharlatan)

Pull request description:

  Get rid of all `ShutdownRequested` calls in validation code by introducing an interrupt object that applications can use to cancel long-running kernel operations.

  Replace all `AbortNode` calls in validation code with new fatal error and flush error notifications so kernel applications can be notified about failures and choose how to handle them.

  ---

  This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/27587 https://github.com/orgs/bitcoin/projects/3 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel".

  The pull request mostly allows dropping the kernel dependency on shutdown.cpp. The only dependency left after this is a `StartShutdown` call which will be removed in followup PR https://github.com/bitcoin/bitcoin/pull/27711. This PR also drops the last reference to the `uiInterface` global in kernel code. The process of moving the `uiInterface` out of the kernel was started in https://github.com/bitcoin/bitcoin/pull/27636.

  This pull request contains a subset of patches originally proposed in #27711. It will be part of a series of changes required to make handling of interrupts (or in other words the current shutdown procedure) in the kernel library more transparent and less reliable on global mutable state. The set of patches contained here was originally proposed by @ryanofsky [here](https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1580779869).

ACKs for top commit:
  achow101:
    light ACK 6eb33bd0c2
  hebasto:
    ACK 6eb33bd0c2, I have reviewed the code and it looks OK.
  ryanofsky:
    Code review ACK 6eb33bd0c2. No changes since last review other than rebase.

Tree-SHA512: 7d2d05fa4805428a09466d43c11ae32946cbb25aa5e741b1eec9cd142e4de4bb311e13ebf1bb125ae490c9d08274f2d56c93314e10f3d69e7fec7445e504987c
This commit is contained in:
Ryan Ofsky
2023-07-06 17:06:55 -04:00
parent c325f0fbae 6eb33bd0c2
commit 75135c673e
31 changed files with 394 additions and 199 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
src/test/blockmanager_tests.cpp
View File

@@ -5,14 +5,16 @@
#include <chainparams.h>
#include <node/blockstorage.h>
#include <node/context.h>
#include <node/kernel_notifications.h>
#include <util/chaintype.h>
#include <validation.h>
#include <boost/test/unit_test.hpp>
#include <test/util/setup_common.h>
using node::BlockManager;
using node::BLOCK_SERIALIZATION_HEADER_SIZE;
using node::BlockManager;
using node::KernelNotifications;
using node::MAX_BLOCKFILE_SIZE;
// use BasicTestingSetup here for the data directory configuration, setup, and cleanup
@@ -21,11 +23,13 @@ BOOST_FIXTURE_TEST_SUITE(blockmanager_tests, BasicTestingSetup)
BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos)
{
const auto params {CreateChainParams(ArgsManager{}, ChainType::MAIN)};
KernelNotifications notifications{m_node.exit_status};
const BlockManager::Options blockman_opts{
.chainparams = *params,
.blocks_dir = m_args.GetBlocksDirPath(),
.notifications = notifications,
};
BlockManager blockman{blockman_opts};
BlockManager blockman{m_node.kernel->interrupt, blockman_opts};
CChain chain {};
// simulate adding a genesis block normally
BOOST_CHECK_EQUAL(blockman.SaveBlockToDisk(params->GenesisBlock(), 0, chain, nullptr).nPos, BLOCK_SERIALIZATION_HEADER_SIZE);

5
src/test/util/setup_common.cpp
View File

@@ -184,7 +184,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto
m_cache_sizes = CalculateCacheSizes(m_args);
m_node.notifications = std::make_unique<KernelNotifications>();
m_node.notifications = std::make_unique<KernelNotifications>(m_node.exit_status);
const ChainstateManager::Options chainman_opts{
.chainparams = chainparams,
@@ -196,8 +196,9 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto
const BlockManager::Options blockman_opts{
.chainparams = chainman_opts.chainparams,
.blocks_dir = m_args.GetBlocksDirPath(),
.notifications = chainman_opts.notifications,
};
m_node.chainman = std::make_unique<ChainstateManager>(chainman_opts, blockman_opts);
m_node.chainman = std::make_unique<ChainstateManager>(m_node.kernel->interrupt, chainman_opts, blockman_opts);
m_node.chainman->m_blockman.m_block_tree_db = std::make_unique<CBlockTreeDB>(DBParams{
.path = m_args.GetDataDirNet() / "blocks" / "index",
.cache_bytes = static_cast<size_t>(m_cache_sizes.block_tree_db),

18
src/test/validation_chainstatemanager_tests.cpp
View File

@@ -379,7 +379,7 @@ struct SnapshotTestSetup : TestChain100Setup {
LOCK(::cs_main);
chainman.ResetChainstates();
BOOST_CHECK_EQUAL(chainman.GetAll().size(), 0);
m_node.notifications = std::make_unique<KernelNotifications>();
m_node.notifications = std::make_unique<KernelNotifications>(m_node.exit_status);
const ChainstateManager::Options chainman_opts{
.chainparams = ::Params(),
.datadir = chainman.m_options.datadir,
@@ -389,11 +389,12 @@ struct SnapshotTestSetup : TestChain100Setup {
const BlockManager::Options blockman_opts{
.chainparams = chainman_opts.chainparams,
.blocks_dir = m_args.GetBlocksDirPath(),
.notifications = chainman_opts.notifications,
};
// For robustness, ensure the old manager is destroyed before creating a
// new one.
m_node.chainman.reset();
m_node.chainman = std::make_unique<ChainstateManager>(chainman_opts, blockman_opts);
m_node.chainman = std::make_unique<ChainstateManager>(m_node.kernel->interrupt, chainman_opts, blockman_opts);
}
return *Assert(m_node.chainman);
}
@@ -563,7 +564,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup
auto db_cache_before_complete = active_cs.m_coinsdb_cache_size_bytes;
SnapshotCompletionResult res;
auto mock_shutdown = [](bilingual_str msg) {};
m_node.notifications->m_shutdown_on_fatal_error = false;
fs::path snapshot_chainstate_dir = *node::FindSnapshotChainstateDir(chainman.m_options.datadir);
BOOST_CHECK(fs::exists(snapshot_chainstate_dir));
@@ -573,8 +574,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup
const uint256 snapshot_tip_hash = WITH_LOCK(chainman.GetMutex(),
return chainman.ActiveTip()->GetBlockHash());
res = WITH_LOCK(::cs_main,
return chainman.MaybeCompleteSnapshotValidation(mock_shutdown));
res = WITH_LOCK(::cs_main, return chainman.MaybeCompleteSnapshotValidation());
BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::SUCCESS);
WITH_LOCK(::cs_main, BOOST_CHECK(chainman.IsSnapshotValidated()));
@@ -590,8 +590,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup
BOOST_CHECK_EQUAL(all_chainstates[0], &active_cs);
// Trying completion again should return false.
res = WITH_LOCK(::cs_main,
return chainman.MaybeCompleteSnapshotValidation(mock_shutdown));
res = WITH_LOCK(::cs_main, return chainman.MaybeCompleteSnapshotValidation());
BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::SKIPPED);
// The invalid snapshot path should not have been used.
@@ -644,7 +643,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion_hash_mismatch, Sna
Chainstate& validation_chainstate = *std::get<0>(chainstates);
ChainstateManager& chainman = *Assert(m_node.chainman);
SnapshotCompletionResult res;
auto mock_shutdown = [](bilingual_str msg) {};
m_node.notifications->m_shutdown_on_fatal_error = false;
// Test tampering with the IBD UTXO set with an extra coin to ensure it causes
// snapshot completion to fail.
@@ -660,8 +659,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion_hash_mismatch, Sna
fs::path snapshot_chainstate_dir = gArgs.GetDataDirNet() / "chainstate_snapshot";
BOOST_CHECK(fs::exists(snapshot_chainstate_dir));
res = WITH_LOCK(::cs_main,
return chainman.MaybeCompleteSnapshotValidation(mock_shutdown));
res = WITH_LOCK(::cs_main, return chainman.MaybeCompleteSnapshotValidation());
BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::HASH_MISMATCH);
auto all_chainstates = chainman.GetAll();