From bfc21c31b2186f7d30fc9a9ca7d6887ab61c6fb9 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Mon, 17 Apr 2023 21:55:17 +0200 Subject: [PATCH 1/5] refactor: Create chaintype files This is the first of a number of commits with the goal of moving the chain type definitions out of chainparamsbase to their own file and implementing them as enums instead of constant strings. The goal is to allow the kernel chainparams to no longer include chainparamsbase. The commit is part of an ongoing effort to decouple the libbitcoinkernel library from the ArgsManager and other functionality that should not be part of the kernel library. --- src/Makefile.am | 3 +++ src/util/chaintype.cpp | 39 +++++++++++++++++++++++++++++++++++++++ src/util/chaintype.h | 22 ++++++++++++++++++++++ 3 files changed, 64 insertions(+) create mode 100644 src/util/chaintype.cpp create mode 100644 src/util/chaintype.h diff --git a/src/Makefile.am b/src/Makefile.am index d12edca64e..9e1fab869d 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -280,6 +280,7 @@ BITCOIN_CORE_H = \ util/bip32.h \ util/bitdeque.h \ util/bytevectorhash.h \ + util/chaintype.h \ util/check.h \ util/epochguard.h \ util/error.h \ @@ -707,6 +708,7 @@ libbitcoin_util_a_SOURCES = \ util/asmap.cpp \ util/bip32.cpp \ util/bytevectorhash.cpp \ + util/chaintype.cpp \ util/check.cpp \ util/error.cpp \ util/exception.cpp \ @@ -956,6 +958,7 @@ libbitcoinkernel_la_SOURCES = \ txdb.cpp \ txmempool.cpp \ uint256.cpp \ + util/chaintype.cpp \ util/check.cpp \ util/exception.cpp \ util/fs.cpp \ diff --git a/src/util/chaintype.cpp b/src/util/chaintype.cpp new file mode 100644 index 0000000000..8a199e352a --- /dev/null +++ b/src/util/chaintype.cpp @@ -0,0 +1,39 @@ +// Copyright (c) 2023 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 + +#include +#include +#include + +std::string ChainTypeToString(ChainType chain) +{ + switch (chain) { + case ChainType::MAIN: + return "main"; + case ChainType::TESTNET: + return "test"; + case ChainType::SIGNET: + return "signet"; + case ChainType::REGTEST: + return "regtest"; + } + assert(false); +} + +std::optional ChainTypeFromString(std::string_view chain) +{ + if (chain == "main") { + return ChainType::MAIN; + } else if (chain == "test") { + return ChainType::TESTNET; + } else if (chain == "signet") { + return ChainType::SIGNET; + } else if (chain == "regtest") { + return ChainType::REGTEST; + } else { + return std::nullopt; + } +} diff --git a/src/util/chaintype.h b/src/util/chaintype.h new file mode 100644 index 0000000000..c73985df57 --- /dev/null +++ b/src/util/chaintype.h @@ -0,0 +1,22 @@ +// Copyright (c) 2023 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_UTIL_CHAINTYPE_H +#define BITCOIN_UTIL_CHAINTYPE_H + +#include +#include + +enum class ChainType { + MAIN, + TESTNET, + SIGNET, + REGTEST, +}; + +std::string ChainTypeToString(ChainType chain); + +std::optional ChainTypeFromString(std::string_view chain); + +#endif // BITCOIN_UTIL_CHAINTYPE_H From 401453df419af35957ec711423ac3d93ad512fe8 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Mon, 8 May 2023 22:15:17 +0200 Subject: [PATCH 2/5] refactor: Introduce ChainType getters for ArgsManager These are introduced for the next commit where the usage of the ChainType is adopted throughout the code. Co-authored-by: Russell Yanofsky Co-authored-by: TheCharlatan --- src/common/args.cpp | 36 ++++++++++++++++++++++++++---------- src/common/args.h | 20 +++++++++++++++++++- 2 files changed, 45 insertions(+), 11 deletions(-) diff --git a/src/common/args.cpp b/src/common/args.cpp index d29b8648bf..266ec5943e 100644 --- a/src/common/args.cpp +++ b/src/common/args.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -33,6 +34,7 @@ #include #include #include +#include const char * const BITCOIN_CONF_FILENAME = "bitcoin.conf"; const char * const BITCOIN_SETTINGS_FILENAME = "settings.json"; @@ -717,7 +719,21 @@ fs::path ArgsManager::GetConfigFilePath() const return GetConfigFile(*this, GetPathArg("-conf", BITCOIN_CONF_FILENAME)); } +ChainType ArgsManager::GetChainType() const +{ + std::variant arg = GetChainArg(); + if (auto* parsed = std::get_if(&arg)) return *parsed; + throw std::runtime_error(strprintf("Unknown chain %s.", std::get(arg))); +} + std::string ArgsManager::GetChainName() const +{ + auto arg = GetChainArg(); + if (auto* parsed = std::get_if(&arg)) return ChainTypeToString(*parsed); + return std::get(arg); +} + +std::variant ArgsManager::GetChainArg() const { auto get_net = [&](const std::string& arg) { LOCK(cs_args); @@ -731,20 +747,20 @@ std::string ArgsManager::GetChainName() const const bool fRegTest = get_net("-regtest"); const bool fSigNet = get_net("-signet"); const bool fTestNet = get_net("-testnet"); - const bool is_chain_arg_set = IsArgSet("-chain"); + const auto chain_arg = GetArg("-chain"); - if ((int)is_chain_arg_set + (int)fRegTest + (int)fSigNet + (int)fTestNet > 1) { + if ((int)chain_arg.has_value() + (int)fRegTest + (int)fSigNet + (int)fTestNet > 1) { throw std::runtime_error("Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one."); } - if (fRegTest) - return CBaseChainParams::REGTEST; - if (fSigNet) { - return CBaseChainParams::SIGNET; + if (chain_arg) { + if (auto parsed = ChainTypeFromString(*chain_arg)) return *parsed; + // Not a known string, so return original string + return *chain_arg; } - if (fTestNet) - return CBaseChainParams::TESTNET; - - return GetArg("-chain", CBaseChainParams::MAIN); + if (fRegTest) return ChainType::REGTEST; + if (fSigNet) return ChainType::SIGNET; + if (fTestNet) return ChainType::TESTNET; + return ChainType::MAIN; } bool ArgsManager::UseDefaultSection(const std::string& arg) const diff --git a/src/common/args.h b/src/common/args.h index 430c392e2b..66cd83ed4e 100644 --- a/src/common/args.h +++ b/src/common/args.h @@ -7,6 +7,7 @@ #include #include +#include #include #include @@ -17,6 +18,7 @@ #include #include #include +#include #include class ArgsManager; @@ -324,7 +326,15 @@ protected: /** * Returns the appropriate chain name from the program arguments. - * @return CBaseChainParams::MAIN by default; raises runtime error if an invalid combination is given. + * @return ChainType::MAIN by default; raises runtime error if an invalid + * combination, or unknown chain is given. + */ + ChainType GetChainType() const; + + /** + * Returns the appropriate chain name string from the program arguments. + * @return ChainType::MAIN string by default; raises runtime error if an + * invalid combination is given. */ std::string GetChainName() const; @@ -411,6 +421,14 @@ private: */ const fs::path& GetDataDir(bool net_specific) const; + /** + * Return -regtest/-signet/-testnet/-chain= setting as a ChainType enum if a + * recognized chain name was set, or as a string if an unrecognized chain + * name was set. Raise an exception if an invalid combination of flags was + * provided. + */ + std::variant GetChainArg() const; + // Helper function for LogArgs(). void logArgsPrefix( const std::string& prefix, From ba8fc7d788932b25864fb260ca14983aa2398c23 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Mon, 17 Apr 2023 22:20:59 +0200 Subject: [PATCH 3/5] refactor: Replace string chain name constants with ChainTypes This commit effectively moves the definition of these constants out of the chainparamsbase to their own file. Using the ChainType enums provides better type safety compared to passing around strings. The commit is part of an ongoing effort to decouple the libbitcoinkernel library from the ArgsManager and other functionality that should not be part of the kernel library. --- src/bench/checkblock.cpp | 3 +- src/bench/load_external.cpp | 3 +- src/bench/logging.cpp | 3 +- src/bench/mempool_stress.cpp | 5 ++- src/bench/rpc_blockchain.cpp | 3 +- src/bench/rpc_mempool.cpp | 3 +- src/bitcoin-chainstate.cpp | 3 +- src/bitcoin-cli.cpp | 25 +++++++---- src/bitcoin-tx.cpp | 2 +- src/bitcoin-util.cpp | 2 +- src/bitcoin-wallet.cpp | 2 +- src/chainparams.cpp | 22 ++++++---- src/chainparams.h | 5 ++- src/chainparamsbase.cpp | 23 +++++----- src/chainparamsbase.h | 16 +++---- src/common/args.cpp | 20 ++++----- src/common/args.h | 2 +- src/common/config.cpp | 5 ++- src/common/init.cpp | 2 +- src/init.cpp | 25 +++++------ src/kernel/chainparams.cpp | 9 ++-- src/kernel/chainparams.h | 9 ++-- src/node/interfaces.cpp | 2 +- src/node/mempool_args.cpp | 2 +- src/qt/bitcoin.cpp | 2 +- src/qt/guiutil.cpp | 25 +++++------ src/qt/intro.cpp | 3 +- src/qt/networkstyle.cpp | 15 ++++--- src/qt/networkstyle.h | 4 +- src/qt/rpcconsole.cpp | 2 +- src/qt/test/apptests.cpp | 2 +- src/qt/test/test_main.cpp | 3 +- src/rpc/blockchain.cpp | 2 +- src/rpc/external_signer.cpp | 2 +- src/rpc/mining.cpp | 2 +- src/rpc/net.cpp | 3 +- src/test/argsman_tests.cpp | 43 ++++++++++--------- src/test/blockmanager_tests.cpp | 3 +- src/test/checkqueue_tests.cpp | 5 ++- src/test/fuzz/addrman.cpp | 3 +- src/test/fuzz/block.cpp | 3 +- src/test/fuzz/descriptor_parse.cpp | 3 +- src/test/fuzz/headerssync.cpp | 3 +- src/test/fuzz/integer.cpp | 3 +- src/test/fuzz/key.cpp | 3 +- src/test/fuzz/key_io.cpp | 3 +- src/test/fuzz/message.cpp | 3 +- src/test/fuzz/net.cpp | 3 +- src/test/fuzz/p2p_transport_serialization.cpp | 3 +- src/test/fuzz/parse_univalue.cpp | 3 +- src/test/fuzz/pow.cpp | 3 +- src/test/fuzz/process_message.cpp | 3 +- src/test/fuzz/process_messages.cpp | 2 +- src/test/fuzz/rpc.cpp | 3 +- src/test/fuzz/script.cpp | 3 +- src/test/fuzz/script_format.cpp | 3 +- src/test/fuzz/script_sign.cpp | 3 +- src/test/fuzz/signet.cpp | 3 +- src/test/fuzz/system.cpp | 2 +- src/test/fuzz/transaction.cpp | 3 +- src/test/fuzz/utxo_snapshot.cpp | 3 +- src/test/fuzz/utxo_total_supply.cpp | 3 +- src/test/fuzz/versionbits.cpp | 3 +- src/test/key_io_tests.cpp | 11 ++--- src/test/pow_tests.cpp | 33 +++++++------- src/test/settings_tests.cpp | 8 ++-- src/test/txvalidationcache_tests.cpp | 3 +- src/test/util/setup_common.cpp | 17 ++++---- src/test/util/setup_common.h | 15 ++++--- src/test/validation_tests.cpp | 9 ++-- src/test/versionbits_tests.cpp | 9 ++-- src/util/settings.cpp | 8 ++-- src/util/settings.h | 6 +-- .../external_signer_scriptpubkeyman.cpp | 2 +- src/wallet/test/init_test_fixture.cpp | 3 +- src/wallet/test/init_test_fixture.h | 3 +- src/wallet/test/wallet_test_fixture.cpp | 5 ++- src/wallet/test/wallet_test_fixture.h | 3 +- 78 files changed, 288 insertions(+), 229 deletions(-) diff --git a/src/bench/checkblock.cpp b/src/bench/checkblock.cpp index 260c8991ce..269ac847a5 100644 --- a/src/bench/checkblock.cpp +++ b/src/bench/checkblock.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include // These are the two major time-sinks which happen after we have fully received @@ -36,7 +37,7 @@ static void DeserializeAndCheckBlockTest(benchmark::Bench& bench) stream.write({&a, 1}); // Prevent compaction ArgsManager bench_args; - const auto chainParams = CreateChainParams(bench_args, CBaseChainParams::MAIN); + const auto chainParams = CreateChainParams(bench_args, ChainType::MAIN); bench.unit("block").run([&] { CBlock block; // Note that CBlock caches its checked state, so we need to recreate it here diff --git a/src/bench/load_external.cpp b/src/bench/load_external.cpp index 0fd842c7c3..2ff72a3012 100644 --- a/src/bench/load_external.cpp +++ b/src/bench/load_external.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include /** @@ -22,7 +23,7 @@ */ static void LoadExternalBlockFile(benchmark::Bench& bench) { - const auto testing_setup{MakeNoLogFileContext(CBaseChainParams::MAIN)}; + const auto testing_setup{MakeNoLogFileContext(ChainType::MAIN)}; // Create a single block as in the blocks files (magic bytes, block size, // block data) as a stream object. diff --git a/src/bench/logging.cpp b/src/bench/logging.cpp index 9aedb26236..c97c4e151b 100644 --- a/src/bench/logging.cpp +++ b/src/bench/logging.cpp @@ -5,6 +5,7 @@ #include #include #include +#include // All but 2 of the benchmarks should have roughly similar performance: // @@ -18,7 +19,7 @@ static void Logging(benchmark::Bench& bench, const std::vector& ext LogInstance().DisableCategory(BCLog::LogFlags::ALL); TestingSetup test_setup{ - CBaseChainParams::REGTEST, + ChainType::REGTEST, extra_args, }; diff --git a/src/bench/mempool_stress.cpp b/src/bench/mempool_stress.cpp index 80c959cdfb..826da73800 100644 --- a/src/bench/mempool_stress.cpp +++ b/src/bench/mempool_stress.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -88,7 +89,7 @@ static void ComplexMemPool(benchmark::Bench& bench) childTxs = static_cast(bench.complexityN()); } std::vector ordered_coins = CreateOrderedCoins(det_rand, childTxs, /*min_ancestors=*/1); - const auto testing_setup = MakeNoLogFileContext(CBaseChainParams::MAIN); + const auto testing_setup = MakeNoLogFileContext(ChainType::MAIN); CTxMemPool& pool = *testing_setup.get()->m_node.mempool; LOCK2(cs_main, pool.cs); bench.run([&]() NO_THREAD_SAFETY_ANALYSIS { @@ -103,7 +104,7 @@ static void ComplexMemPool(benchmark::Bench& bench) static void MempoolCheck(benchmark::Bench& bench) { FastRandomContext det_rand{true}; - auto testing_setup = MakeNoLogFileContext(CBaseChainParams::REGTEST, {"-checkmempool=1"}); + auto testing_setup = MakeNoLogFileContext(ChainType::REGTEST, {"-checkmempool=1"}); CTxMemPool& pool = *testing_setup.get()->m_node.mempool; LOCK2(cs_main, pool.cs); testing_setup->PopulateMempool(det_rand, 400, true); diff --git a/src/bench/rpc_blockchain.cpp b/src/bench/rpc_blockchain.cpp index f68b6acb5b..a9b197b190 100644 --- a/src/bench/rpc_blockchain.cpp +++ b/src/bench/rpc_blockchain.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include @@ -15,7 +16,7 @@ namespace { struct TestBlockAndIndex { - const std::unique_ptr testing_setup{MakeNoLogFileContext(CBaseChainParams::MAIN)}; + const std::unique_ptr testing_setup{MakeNoLogFileContext(ChainType::MAIN)}; CBlock block{}; uint256 blockHash{}; CBlockIndex blockindex{}; diff --git a/src/bench/rpc_mempool.cpp b/src/bench/rpc_mempool.cpp index e3e1a07c83..af1fa7c572 100644 --- a/src/bench/rpc_mempool.cpp +++ b/src/bench/rpc_mempool.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include @@ -21,7 +22,7 @@ static void AddTx(const CTransactionRef& tx, const CAmount& fee, CTxMemPool& poo static void RpcMempool(benchmark::Bench& bench) { - const auto testing_setup = MakeNoLogFileContext(CBaseChainParams::MAIN); + const auto testing_setup = MakeNoLogFileContext(ChainType::MAIN); CTxMemPool& pool = *Assert(testing_setup->m_node.mempool); LOCK2(cs_main, pool.cs); diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index 2b139cf908..52e697a78d 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -25,6 +25,7 @@ #include #include #include