diff options
Diffstat (limited to 'src')
36 files changed, 439 insertions, 146 deletions
diff --git a/src/Makefile.qttest.include b/src/Makefile.qttest.include index 8e6fa2eb0d..29c322fbc2 100644 --- a/src/Makefile.qttest.include +++ b/src/Makefile.qttest.include @@ -7,6 +7,7 @@ TESTS += qt/test/test_bitcoin-qt TEST_QT_MOC_CPP = \ qt/test/moc_apptests.cpp \ + qt/test/moc_optiontests.cpp \ qt/test/moc_rpcnestedtests.cpp \ qt/test/moc_uritests.cpp @@ -19,6 +20,7 @@ endif # ENABLE_WALLET TEST_QT_H = \ qt/test/addressbooktests.h \ qt/test/apptests.h \ + qt/test/optiontests.h \ qt/test/rpcnestedtests.h \ qt/test/uritests.h \ qt/test/util.h \ @@ -30,6 +32,7 @@ qt_test_test_bitcoin_qt_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BITCOIN_ qt_test_test_bitcoin_qt_SOURCES = \ init/bitcoin-qt.cpp \ qt/test/apptests.cpp \ + qt/test/optiontests.cpp \ qt/test/rpcnestedtests.cpp \ qt/test/test_main.cpp \ qt/test/uritests.cpp \ diff --git a/src/addrman.cpp b/src/addrman.cpp index f91a979934..2fd8143c1c 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -246,12 +246,18 @@ void AddrManImpl::Unserialize(Stream& s_) uint8_t compat; s >> compat; + if (compat < INCOMPATIBILITY_BASE) { + throw std::ios_base::failure(strprintf( + "Corrupted addrman database: The compat value (%u) " + "is lower than the expected minimum value %u.", + compat, INCOMPATIBILITY_BASE)); + } const uint8_t lowest_compatible = compat - INCOMPATIBILITY_BASE; if (lowest_compatible > FILE_FORMAT) { throw InvalidAddrManVersionError(strprintf( "Unsupported format of addrman database: %u. It is compatible with formats >=%u, " "but the maximum supported by this version of %s is %u.", - uint8_t{format}, uint8_t{lowest_compatible}, PACKAGE_NAME, uint8_t{FILE_FORMAT})); + uint8_t{format}, lowest_compatible, PACKAGE_NAME, uint8_t{FILE_FORMAT})); } s >> nKey; diff --git a/src/bench/bench_bitcoin.cpp b/src/bench/bench_bitcoin.cpp index 3f8bff4bcf..d6f9c0f8b5 100644 --- a/src/bench/bench_bitcoin.cpp +++ b/src/bench/bench_bitcoin.cpp @@ -109,8 +109,8 @@ int main(int argc, char** argv) args.asymptote = parseAsymptote(argsman.GetArg("-asymptote", "")); args.is_list_only = argsman.GetBoolArg("-list", false); args.min_time = std::chrono::milliseconds(argsman.GetIntArg("-min_time", DEFAULT_MIN_TIME_MS)); - args.output_csv = fs::PathFromString(argsman.GetArg("-output_csv", "")); - args.output_json = fs::PathFromString(argsman.GetArg("-output_json", "")); + args.output_csv = argsman.GetPathArg("-output_csv"); + args.output_json = argsman.GetPathArg("-output_json"); args.regex_filter = argsman.GetArg("-filter", DEFAULT_BENCH_FILTER); benchmark::BenchRunner::RunAll(args); diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index f93197350d..72b8fefcc7 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -256,7 +256,7 @@ epilogue: } GetMainSignals().UnregisterBackgroundSignalScheduler(); - UnloadBlockIndex(nullptr, chainman); + WITH_LOCK(::cs_main, UnloadBlockIndex(nullptr, chainman)); init::UnsetGlobals(); } diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index a1c8a5937c..386eb67ce9 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -228,10 +228,9 @@ bool CoinStatsIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex) m_muhash.Finalize(out); value.second.muhash = out; - CDBBatch batch(*m_db); - batch.Write(DBHeightKey(pindex->nHeight), value); - batch.Write(DB_MUHASH, m_muhash); - return m_db->WriteBatch(batch); + // Intentionally do not update DB_MUHASH here so it stays in sync with + // DB_BEST_BLOCK, and the index is not corrupted if there is an unclean shutdown. + return m_db->Write(DBHeightKey(pindex->nHeight), value); } static bool CopyHeightIndexToHashIndex(CDBIterator& db_it, CDBBatch& batch, @@ -388,6 +387,14 @@ bool CoinStatsIndex::Init() return true; } +bool CoinStatsIndex::CommitInternal(CDBBatch& batch) +{ + // DB_MUHASH should always be committed in a batch together with DB_BEST_BLOCK + // to prevent an inconsistent state of the DB. + batch.Write(DB_MUHASH, m_muhash); + return BaseIndex::CommitInternal(batch); +} + // Reverse a single block as part of a reorg bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex) { @@ -489,5 +496,5 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex Assert(m_total_unspendables_scripts == read_out.second.total_unspendables_scripts); Assert(m_total_unspendables_unclaimed_rewards == read_out.second.total_unspendables_unclaimed_rewards); - return m_db->Write(DB_MUHASH, m_muhash); + return true; } diff --git a/src/index/coinstatsindex.h b/src/index/coinstatsindex.h index d2a6c9c964..24190ac137 100644 --- a/src/index/coinstatsindex.h +++ b/src/index/coinstatsindex.h @@ -39,6 +39,8 @@ private: protected: bool Init() override; + bool CommitInternal(CDBBatch& batch) override; + bool WriteBlock(const CBlock& block, const CBlockIndex* pindex) override; bool Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_tip) override; diff --git a/src/init.cpp b/src/init.cpp index a3d53c3fae..bad402e56e 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -135,7 +135,7 @@ static const char* BITCOIN_PID_FILENAME = "bitcoind.pid"; static fs::path GetPidFile(const ArgsManager& args) { - return AbsPathForConfigVal(fs::PathFromString(args.GetArg("-pid", BITCOIN_PID_FILENAME))); + return AbsPathForConfigVal(args.GetPathArg("-pid", BITCOIN_PID_FILENAME)); } [[nodiscard]] static bool CreatePidFile(const ArgsManager& args) @@ -462,7 +462,7 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-onion=<ip:port>", "Use separate SOCKS5 proxy to reach peers via Tor onion services, set -noonion to disable (default: -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-i2psam=<ip:port>", "I2P SAM proxy to reach I2P peers and accept I2P connections (default: none)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-i2pacceptincoming", "If set and -i2psam is also set then incoming I2P connections are accepted via the SAM proxy. If this is not set but -i2psam is set then only outgoing connections will be made to the I2P network. Ignored if -i2psam is not set. Listening for incoming I2P connections is done through the SAM proxy, not by binding to a local address and port (default: 1)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); - argsman.AddArg("-onlynet=<net>", "Make automatic outgoing connections only through network <net> (" + Join(GetNetworkNames(), ", ") + "). Incoming connections are not affected by this option. This option can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); + argsman.AddArg("-onlynet=<net>", "Make automatic outbound connections only to network <net> (" + Join(GetNetworkNames(), ", ") + "). Inbound and manual connections are not affected by this option. It can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-peerbloomfilters", strprintf("Support filtering of blocks and transaction with bloom filters (default: %u)", DEFAULT_PEERBLOOMFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-peerblockfilters", strprintf("Serve compact block filters to peers per BIP 157 (default: %u)", DEFAULT_PEERBLOCKFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-permitbaremultisig", strprintf("Relay non-P2SH multisig (default: %u)", DEFAULT_PERMIT_BAREMULTISIG), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); @@ -1229,10 +1229,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // Read asmap file if configured std::vector<bool> asmap; if (args.IsArgSet("-asmap")) { - fs::path asmap_path = fs::PathFromString(args.GetArg("-asmap", "")); - if (asmap_path.empty()) { - asmap_path = fs::PathFromString(DEFAULT_ASMAP_FILENAME); - } + fs::path asmap_path = args.GetPathArg("-asmap", DEFAULT_ASMAP_FILENAME); if (!asmap_path.is_absolute()) { asmap_path = gArgs.GetDataDirNet() / asmap_path; } diff --git a/src/init/common.cpp b/src/init/common.cpp index 38c60366e3..688471b35d 100644 --- a/src/init/common.cpp +++ b/src/init/common.cpp @@ -81,7 +81,7 @@ void AddLoggingArgs(ArgsManager& argsman) void SetLoggingOptions(const ArgsManager& args) { LogInstance().m_print_to_file = !args.IsArgNegated("-debuglogfile"); - LogInstance().m_file_path = AbsPathForConfigVal(fs::PathFromString(args.GetArg("-debuglogfile", DEFAULT_DEBUGLOGFILE))); + LogInstance().m_file_path = AbsPathForConfigVal(args.GetPathArg("-debuglogfile", DEFAULT_DEBUGLOGFILE)); LogInstance().m_print_to_console = args.GetBoolArg("-printtoconsole", !args.GetBoolArg("-daemon", false)); LogInstance().m_log_timestamps = args.GetBoolArg("-logtimestamps", DEFAULT_LOGTIMESTAMPS); LogInstance().m_log_time_micros = args.GetBoolArg("-logtimemicros", DEFAULT_LOGTIMEMICROS); diff --git a/src/leveldb/util/env_posix.cc b/src/leveldb/util/env_posix.cc index 9f5863a0f3..18626b327c 100644 --- a/src/leveldb/util/env_posix.cc +++ b/src/leveldb/util/env_posix.cc @@ -850,7 +850,7 @@ class SingletonEnv { public: SingletonEnv() { #if !defined(NDEBUG) - env_initialized_.store(true, std::memory_order::memory_order_relaxed); + env_initialized_.store(true, std::memory_order_relaxed); #endif // !defined(NDEBUG) static_assert(sizeof(env_storage_) >= sizeof(EnvType), "env_storage_ will not fit the Env"); @@ -867,7 +867,7 @@ class SingletonEnv { static void AssertEnvNotInitialized() { #if !defined(NDEBUG) - assert(!env_initialized_.load(std::memory_order::memory_order_relaxed)); + assert(!env_initialized_.load(std::memory_order_relaxed)); #endif // !defined(NDEBUG) } diff --git a/src/leveldb/util/env_windows.cc b/src/leveldb/util/env_windows.cc index 1834206562..4dcba222a1 100644 --- a/src/leveldb/util/env_windows.cc +++ b/src/leveldb/util/env_windows.cc @@ -798,7 +798,7 @@ class SingletonEnv { public: SingletonEnv() { #if !defined(NDEBUG) - env_initialized_.store(true, std::memory_order::memory_order_relaxed); + env_initialized_.store(true, std::memory_order_relaxed); #endif // !defined(NDEBUG) static_assert(sizeof(env_storage_) >= sizeof(EnvType), "env_storage_ will not fit the Env"); @@ -815,7 +815,7 @@ class SingletonEnv { static void AssertEnvNotInitialized() { #if !defined(NDEBUG) - assert(!env_initialized_.load(std::memory_order::memory_order_relaxed)); + assert(!env_initialized_.load(std::memory_order_relaxed)); #endif // !defined(NDEBUG) } diff --git a/src/net_processing.cpp b/src/net_processing.cpp index aee1e4c30c..59cd83e493 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -415,7 +415,7 @@ private: void RelayAddress(NodeId originator, const CAddress& addr, bool fReachable); /** Send `feefilter` message. */ - void MaybeSendFeefilter(CNode& node, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + void MaybeSendFeefilter(CNode& node, std::chrono::microseconds current_time); const CChainParams& m_chainparams; CConnman& m_connman; @@ -4506,8 +4506,6 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, std::chrono::microseconds current_time) { - AssertLockHeld(cs_main); - if (m_ignore_incoming_txs) return; if (!pto.m_tx_relay) return; if (pto.GetCommonVersion() < FEEFILTER_VERSION) return; @@ -5054,8 +5052,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) if (!vGetData.empty()) m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData)); - - MaybeSendFeefilter(*pto, current_time); } // release cs_main + MaybeSendFeefilter(*pto, current_time); return true; } diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 8a99130fd0..7392830261 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -32,35 +32,39 @@ static FILE* OpenUndoFile(const FlatFilePos& pos, bool fReadOnly = false); static FlatFileSeq BlockFileSeq(); static FlatFileSeq UndoFileSeq(); -CBlockIndex* BlockManager::LookupBlockIndex(const uint256& hash) const +CBlockIndex* BlockManager::LookupBlockIndex(const uint256& hash) +{ + AssertLockHeld(cs_main); + BlockMap::iterator it = m_block_index.find(hash); + return it == m_block_index.end() ? nullptr : &it->second; +} + +const CBlockIndex* BlockManager::LookupBlockIndex(const uint256& hash) const { AssertLockHeld(cs_main); BlockMap::const_iterator it = m_block_index.find(hash); - return it == m_block_index.end() ? nullptr : it->second; + return it == m_block_index.end() ? nullptr : &it->second; } CBlockIndex* BlockManager::AddToBlockIndex(const CBlockHeader& block) { AssertLockHeld(cs_main); - // Check for duplicate - uint256 hash = block.GetHash(); - BlockMap::iterator it = m_block_index.find(hash); - if (it != m_block_index.end()) { - return it->second; + auto [mi, inserted] = m_block_index.try_emplace(block.GetHash(), block); + if (!inserted) { + return &mi->second; } + CBlockIndex* pindexNew = &(*mi).second; - // Construct new block index object - CBlockIndex* pindexNew = new CBlockIndex(block); // We assign the sequence id to blocks only when the full data is available, // to avoid miners withholding blocks but broadcasting headers, to get a // competitive advantage. pindexNew->nSequenceId = 0; - BlockMap::iterator mi = m_block_index.insert(std::make_pair(hash, pindexNew)).first; + pindexNew->phashBlock = &((*mi).first); BlockMap::iterator miPrev = m_block_index.find(block.hashPrevBlock); if (miPrev != m_block_index.end()) { - pindexNew->pprev = (*miPrev).second; + pindexNew->pprev = &(*miPrev).second; pindexNew->nHeight = pindexNew->pprev->nHeight + 1; pindexNew->BuildSkip(); } @@ -80,8 +84,8 @@ void BlockManager::PruneOneBlockFile(const int fileNumber) AssertLockHeld(cs_main); LOCK(cs_LastBlockFile); - for (const auto& entry : m_block_index) { - CBlockIndex* pindex = entry.second; + for (auto& entry : m_block_index) { + CBlockIndex* pindex = &entry.second; if (pindex->nFile == fileNumber) { pindex->nStatus &= ~BLOCK_HAVE_DATA; pindex->nStatus &= ~BLOCK_HAVE_UNDO; @@ -199,18 +203,13 @@ CBlockIndex* BlockManager::InsertBlockIndex(const uint256& hash) return nullptr; } - // Return existing - BlockMap::iterator mi = m_block_index.find(hash); - if (mi != m_block_index.end()) { - return (*mi).second; + // Return existing or create new + auto [mi, inserted] = m_block_index.try_emplace(hash); + CBlockIndex* pindex = &(*mi).second; + if (inserted) { + pindex->phashBlock = &((*mi).first); } - - // Create new - CBlockIndex* pindexNew = new CBlockIndex(); - mi = m_block_index.insert(std::make_pair(hash, pindexNew)).first; - pindexNew->phashBlock = &((*mi).first); - - return pindexNew; + return pindex; } bool BlockManager::LoadBlockIndex( @@ -224,8 +223,8 @@ bool BlockManager::LoadBlockIndex( // Calculate nChainWork std::vector<std::pair<int, CBlockIndex*>> vSortedByHeight; vSortedByHeight.reserve(m_block_index.size()); - for (const std::pair<const uint256, CBlockIndex*>& item : m_block_index) { - CBlockIndex* pindex = item.second; + for (auto& [_, block_index] : m_block_index) { + CBlockIndex* pindex = &block_index; vSortedByHeight.push_back(std::make_pair(pindex->nHeight, pindex)); } sort(vSortedByHeight.begin(), vSortedByHeight.end()); @@ -327,10 +326,6 @@ void BlockManager::Unload() { m_blocks_unlinked.clear(); - for (const BlockMap::value_type& entry : m_block_index) { - delete entry.second; - } - m_block_index.clear(); m_blockfile_info.clear(); @@ -386,8 +381,8 @@ bool BlockManager::LoadBlockIndexDB(ChainstateManager& chainman) // Check presence of blk files LogPrintf("Checking all blk files are present...\n"); std::set<int> setBlkDataFiles; - for (const std::pair<const uint256, CBlockIndex*>& item : m_block_index) { - CBlockIndex* pindex = item.second; + for (const auto& [_, block_index] : m_block_index) { + const CBlockIndex* pindex = &block_index; if (pindex->nStatus & BLOCK_HAVE_DATA) { setBlkDataFiles.insert(pindex->nFile); } diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 42e46797d2..12224f7a5d 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -5,6 +5,7 @@ #ifndef BITCOIN_NODE_BLOCKSTORAGE_H #define BITCOIN_NODE_BLOCKSTORAGE_H +#include <chain.h> #include <fs.h> #include <protocol.h> // For CMessageHeader::MessageStartChars #include <sync.h> @@ -20,7 +21,6 @@ class ArgsManager; class BlockValidationState; class CBlock; class CBlockFileInfo; -class CBlockIndex; class CBlockUndo; class CChain; class CChainParams; @@ -52,7 +52,11 @@ extern bool fPruneMode; /** Number of MiB of block files that we're trying to stay below. */ extern uint64_t nPruneTarget; -typedef std::unordered_map<uint256, CBlockIndex*, BlockHasher> BlockMap; +// Because validation code takes pointers to the map's CBlockIndex objects, if +// we ever switch to another associative container, we need to either use a +// container that has stable addressing (true of all std associative +// containers), or make the key a `std::unique_ptr<CBlockIndex>` +using BlockMap = std::unordered_map<uint256, CBlockIndex, BlockHasher>; struct CBlockIndexWorkComparator { bool operator()(const CBlockIndex* pa, const CBlockIndex* pb) const; @@ -144,7 +148,8 @@ public: //! Mark one block file as pruned (modify associated database entries) void PruneOneBlockFile(const int fileNumber) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - CBlockIndex* LookupBlockIndex(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); + CBlockIndex* LookupBlockIndex(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + const CBlockIndex* LookupBlockIndex(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** Get block file info entry for one block file */ CBlockFileInfo* GetBlockFileInfo(size_t n); diff --git a/src/qt/guiutil.cpp b/src/qt/guiutil.cpp index d6706fd009..3108c93d7c 100644 --- a/src/qt/guiutil.cpp +++ b/src/qt/guiutil.cpp @@ -883,11 +883,7 @@ void PolishProgressDialog(QProgressDialog* dialog) int TextWidth(const QFontMetrics& fm, const QString& text) { -#if (QT_VERSION >= QT_VERSION_CHECK(5, 11, 0)) return fm.horizontalAdvance(text); -#else - return fm.width(text); -#endif } void LogQtInfo() diff --git a/src/qt/test/optiontests.cpp b/src/qt/test/optiontests.cpp new file mode 100644 index 0000000000..51894e1915 --- /dev/null +++ b/src/qt/test/optiontests.cpp @@ -0,0 +1,31 @@ +// Copyright (c) 2018 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 <qt/bitcoin.h> +#include <qt/test/optiontests.h> +#include <test/util/setup_common.h> +#include <util/system.h> + +#include <QSettings> +#include <QTest> + +#include <univalue.h> + +//! Entry point for BitcoinApplication tests. +void OptionTests::optionTests() +{ + // Test regression https://github.com/bitcoin/bitcoin/issues/24457. Ensure + // that setting integer prune value doesn't cause an exception to be thrown + // in the OptionsModel constructor + gArgs.LockSettings([&](util::Settings& settings) { + settings.forced_settings.erase("prune"); + settings.rw_settings["prune"] = 3814; + }); + gArgs.WriteSettingsFile(); + OptionsModel{}; + gArgs.LockSettings([&](util::Settings& settings) { + settings.rw_settings.erase("prune"); + }); + gArgs.WriteSettingsFile(); +} diff --git a/src/qt/test/optiontests.h b/src/qt/test/optiontests.h new file mode 100644 index 0000000000..779d4cc209 --- /dev/null +++ b/src/qt/test/optiontests.h @@ -0,0 +1,25 @@ +// Copyright (c) 2019 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_QT_TEST_OPTIONTESTS_H +#define BITCOIN_QT_TEST_OPTIONTESTS_H + +#include <qt/optionsmodel.h> + +#include <QObject> + +class OptionTests : public QObject +{ + Q_OBJECT +public: + explicit OptionTests(interfaces::Node& node) : m_node(node) {} + +private Q_SLOTS: + void optionTests(); + +private: + interfaces::Node& m_node; +}; + +#endif // BITCOIN_QT_TEST_OPTIONTESTS_H diff --git a/src/qt/test/test_main.cpp b/src/qt/test/test_main.cpp index 10b7e2ffe7..07d256f05a 100644 --- a/src/qt/test/test_main.cpp +++ b/src/qt/test/test_main.cpp @@ -10,6 +10,7 @@ #include <interfaces/node.h> #include <qt/bitcoin.h> #include <qt/test/apptests.h> +#include <qt/test/optiontests.h> #include <qt/test/rpcnestedtests.h> #include <qt/test/uritests.h> #include <test/util/setup_common.h> @@ -89,6 +90,10 @@ int main(int argc, char* argv[]) if (QTest::qExec(&app_tests) != 0) { fInvalid = true; } + OptionTests options_tests(app.node()); + if (QTest::qExec(&options_tests) != 0) { + fInvalid = true; + } URITests test1; if (QTest::qExec(&test1) != 0) { fInvalid = true; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 9817c80cbd..86dfbbae35 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1753,10 +1753,10 @@ static RPCHelpMan getchaintips() std::set<const CBlockIndex*> setOrphans; std::set<const CBlockIndex*> setPrevs; - for (const std::pair<const uint256, CBlockIndex*>& item : chainman.BlockIndex()) { - if (!active_chain.Contains(item.second)) { - setOrphans.insert(item.second); - setPrevs.insert(item.second->pprev); + for (const auto& [_, block_index] : chainman.BlockIndex()) { + if (!active_chain.Contains(&block_index)) { + setOrphans.insert(&block_index); + setPrevs.insert(block_index.pprev); } } diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp index 92de4ec7ba..5b73481bc1 100644 --- a/src/test/coinstatsindex_tests.cpp +++ b/src/test/coinstatsindex_tests.cpp @@ -2,8 +2,10 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include <chainparams.h> #include <index/coinstatsindex.h> #include <test/util/setup_common.h> +#include <test/util/validation.h> #include <util/time.h> #include <validation.h> @@ -16,6 +18,17 @@ using node::CoinStatsHashType; BOOST_AUTO_TEST_SUITE(coinstatsindex_tests) +static void IndexWaitSynced(BaseIndex& index) +{ + // Allow the CoinStatsIndex to catch up with the block index that is syncing + // in a background thread. + const auto timeout = GetTime<std::chrono::seconds>() + 120s; + while (!index.BlockUntilSyncedToCurrentChain()) { + BOOST_REQUIRE(timeout > GetTime<std::chrono::milliseconds>()); + UninterruptibleSleep(100ms); + } +} + BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup) { CoinStatsIndex coin_stats_index{1 << 20, true}; @@ -36,13 +49,7 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup) BOOST_REQUIRE(coin_stats_index.Start(m_node.chainman->ActiveChainstate())); - // Allow the CoinStatsIndex to catch up with the block index that is syncing - // in a background thread. - const auto timeout = GetTime<std::chrono::seconds>() + 120s; - while (!coin_stats_index.BlockUntilSyncedToCurrentChain()) { - BOOST_REQUIRE(timeout > GetTime<std::chrono::milliseconds>()); - UninterruptibleSleep(100ms); - } + IndexWaitSynced(coin_stats_index); // Check that CoinStatsIndex works for genesis block. const CBlockIndex* genesis_block_index; @@ -78,4 +85,44 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup) // Rest of shutdown sequence and destructors happen in ~TestingSetup() } +// Test shutdown between BlockConnected and ChainStateFlushed notifications, +// make sure index is not corrupted and is able to reload. +BOOST_FIXTURE_TEST_CASE(coinstatsindex_unclean_shutdown, TestChain100Setup) +{ + CChainState& chainstate = Assert(m_node.chainman)->ActiveChainstate(); + const CChainParams& params = Params(); + { + CoinStatsIndex index{1 << 20}; + BOOST_REQUIRE(index.Start(chainstate)); + IndexWaitSynced(index); + std::shared_ptr<const CBlock> new_block; + CBlockIndex* new_block_index = nullptr; + { + const CScript script_pub_key{CScript() << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG}; + const CBlock block = this->CreateBlock({}, script_pub_key, chainstate); + + new_block = std::make_shared<CBlock>(block); + + LOCK(cs_main); + BlockValidationState state; + BOOST_CHECK(CheckBlock(block, state, params.GetConsensus())); + BOOST_CHECK(chainstate.AcceptBlock(new_block, state, &new_block_index, true, nullptr, nullptr)); + CCoinsViewCache view(&chainstate.CoinsTip()); + BOOST_CHECK(chainstate.ConnectBlock(block, state, new_block_index, view)); + } + // Send block connected notification, then stop the index without + // sending a chainstate flushed notification. Prior to #24138, this + // would cause the index to be corrupted and fail to reload. + ValidationInterfaceTest::BlockConnected(index, new_block, new_block_index); + index.Stop(); + } + + { + CoinStatsIndex index{1 << 20}; + // Make sure the index can be loaded. + BOOST_REQUIRE(index.Start(chainstate)); + index.Stop(); + } +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/fs_tests.cpp b/src/test/fs_tests.cpp index 5875f0218f..d2554483d4 100644 --- a/src/test/fs_tests.cpp +++ b/src/test/fs_tests.cpp @@ -46,8 +46,8 @@ BOOST_AUTO_TEST_CASE(fsbridge_fstream) { fs::path tmpfolder = m_args.GetDataDirBase(); // tmpfile1 should be the same as tmpfile2 - fs::path tmpfile1 = tmpfolder / "fs_tests_₿_🏃"; - fs::path tmpfile2 = tmpfolder / "fs_tests_₿_🏃"; + fs::path tmpfile1 = tmpfolder / fs::u8path("fs_tests_₿_🏃"); + fs::path tmpfile2 = tmpfolder / fs::u8path("fs_tests_₿_🏃"); { std::ofstream file{tmpfile1}; file << "bitcoin"; @@ -86,7 +86,7 @@ BOOST_AUTO_TEST_CASE(fsbridge_fstream) } { // Join an absolute path and a relative path. - fs::path p = fsbridge::AbsPathJoin(tmpfolder, "fs_tests_₿_🏃"); + fs::path p = fsbridge::AbsPathJoin(tmpfolder, fs::u8path("fs_tests_₿_🏃")); BOOST_CHECK(p.is_absolute()); BOOST_CHECK_EQUAL(tmpfile1, p); } diff --git a/src/test/getarg_tests.cpp b/src/test/getarg_tests.cpp index ef9d72dcde..c877105fe7 100644 --- a/src/test/getarg_tests.cpp +++ b/src/test/getarg_tests.cpp @@ -3,6 +3,8 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include <test/util/setup_common.h> +#include <univalue.h> +#include <util/settings.h> #include <util/strencodings.h> #include <util/system.h> @@ -41,6 +43,116 @@ void SetupArgs(ArgsManager& local_args, const std::vector<std::pair<std::string, } } +// Test behavior of GetArg functions when string, integer, and boolean types +// are specified in the settings.json file. GetArg functions are convenience +// functions. The GetSetting method can always be used instead of GetArg +// methods to retrieve original values, and there's not always an objective +// answer to what GetArg behavior is best in every case. This test makes sure +// there's test coverage for whatever the current behavior is, so it's not +// broken or changed unintentionally. +BOOST_AUTO_TEST_CASE(setting_args) +{ + ArgsManager args; + SetupArgs(args, {{"-foo", ArgsManager::ALLOW_ANY}}); + + auto set_foo = [&](const util::SettingsValue& value) { + args.LockSettings([&](util::Settings& settings) { + settings.rw_settings["foo"] = value; + }); + }; + + set_foo("str"); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "\"str\""); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "str"); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 0); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), false); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), false); + + set_foo("99"); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "\"99\""); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "99"); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 99); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), true); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), true); + + set_foo("3.25"); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "\"3.25\""); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "3.25"); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 3); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), true); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), true); + + set_foo("0"); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "\"0\""); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "0"); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 0); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), false); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), false); + + set_foo(""); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "\"\""); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), ""); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 0); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), true); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), true); + + set_foo(99); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "99"); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "99"); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 99); + BOOST_CHECK_THROW(args.GetBoolArg("foo", true), std::runtime_error); + BOOST_CHECK_THROW(args.GetBoolArg("foo", false), std::runtime_error); + + set_foo(3.25); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "3.25"); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "3.25"); + BOOST_CHECK_THROW(args.GetIntArg("foo", 100), std::runtime_error); + BOOST_CHECK_THROW(args.GetBoolArg("foo", true), std::runtime_error); + BOOST_CHECK_THROW(args.GetBoolArg("foo", false), std::runtime_error); + + set_foo(0); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "0"); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "0"); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 0); + BOOST_CHECK_THROW(args.GetBoolArg("foo", true), std::runtime_error); + BOOST_CHECK_THROW(args.GetBoolArg("foo", false), std::runtime_error); + + set_foo(true); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "true"); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "1"); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 1); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), true); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), true); + + set_foo(false); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "false"); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "0"); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 0); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), false); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), false); + + set_foo(UniValue::VOBJ); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "{}"); + BOOST_CHECK_THROW(args.GetArg("foo", "default"), std::runtime_error); + BOOST_CHECK_THROW(args.GetIntArg("foo", 100), std::runtime_error); + BOOST_CHECK_THROW(args.GetBoolArg("foo", true), std::runtime_error); + BOOST_CHECK_THROW(args.GetBoolArg("foo", false), std::runtime_error); + + set_foo(UniValue::VARR); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "[]"); + BOOST_CHECK_THROW(args.GetArg("foo", "default"), std::runtime_error); + BOOST_CHECK_THROW(args.GetIntArg("foo", 100), std::runtime_error); + BOOST_CHECK_THROW(args.GetBoolArg("foo", true), std::runtime_error); + BOOST_CHECK_THROW(args.GetBoolArg("foo", false), std::runtime_error); + + set_foo(UniValue::VNULL); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "null"); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "default"); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 100); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), true); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), false); +} + BOOST_AUTO_TEST_CASE(boolarg) { ArgsManager local_args; @@ -245,6 +357,24 @@ BOOST_AUTO_TEST_CASE(patharg) ResetArgs(local_args, "-dir=user/.bitcoin/.//"); BOOST_CHECK_EQUAL(local_args.GetPathArg("-dir"), relative_path); + + // Check negated and default argument handling. Specifying an empty argument + // is the same as not specifying the argument. This is convenient for + // scripting so later command line arguments can override earlier command + // line arguments or bitcoin.conf values. Currently the -dir= case cannot be + // distinguished from -dir case with no assignment, but #16545 would add the + // ability to distinguish these in the future (and treat the no-assign case + // like an imperative command or an error). + ResetArgs(local_args, ""); + BOOST_CHECK_EQUAL(local_args.GetPathArg("-dir", "default"), fs::path{"default"}); + ResetArgs(local_args, "-dir=override"); + BOOST_CHECK_EQUAL(local_args.GetPathArg("-dir", "default"), fs::path{"override"}); + ResetArgs(local_args, "-dir="); + BOOST_CHECK_EQUAL(local_args.GetPathArg("-dir", "default"), fs::path{"default"}); + ResetArgs(local_args, "-dir"); + BOOST_CHECK_EQUAL(local_args.GetPathArg("-dir", "default"), fs::path{"default"}); + ResetArgs(local_args, "-nodir"); + BOOST_CHECK_EQUAL(local_args.GetPathArg("-dir", "default"), fs::path{""}); } BOOST_AUTO_TEST_CASE(doubledash) diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index c968e4d124..211153f06c 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -180,10 +180,9 @@ ChainTestingSetup::~ChainTestingSetup() m_node.banman.reset(); m_node.addrman.reset(); m_node.args = nullptr; - UnloadBlockIndex(m_node.mempool.get(), *m_node.chainman); + WITH_LOCK(::cs_main, UnloadBlockIndex(m_node.mempool.get(), *m_node.chainman)); m_node.mempool.reset(); m_node.scheduler.reset(); - m_node.chainman->Reset(); m_node.chainman.reset(); } diff --git a/src/test/util/validation.cpp b/src/test/util/validation.cpp index 1aed492c3c..49535855f9 100644 --- a/src/test/util/validation.cpp +++ b/src/test/util/validation.cpp @@ -7,6 +7,7 @@ #include <util/check.h> #include <util/time.h> #include <validation.h> +#include <validationinterface.h> void TestChainState::ResetIbd() { @@ -20,3 +21,8 @@ void TestChainState::JumpOutOfIbd() m_cached_finished_ibd = true; Assert(!IsInitialBlockDownload()); } + +void ValidationInterfaceTest::BlockConnected(CValidationInterface& obj, const std::shared_ptr<const CBlock>& block, const CBlockIndex* pindex) +{ + obj.BlockConnected(block, pindex); +} diff --git a/src/test/util/validation.h b/src/test/util/validation.h index b13aa0be60..b0bc717b6c 100644 --- a/src/test/util/validation.h +++ b/src/test/util/validation.h @@ -7,6 +7,8 @@ #include <validation.h> +class CValidationInterface; + struct TestChainState : public CChainState { /** Reset the ibd cache to its initial state */ void ResetIbd(); @@ -14,4 +16,10 @@ struct TestChainState : public CChainState { void JumpOutOfIbd(); }; +class ValidationInterfaceTest +{ +public: + static void BlockConnected(CValidationInterface& obj, const std::shared_ptr<const CBlock>& block, const CBlockIndex* pindex); +}; + #endif // BITCOIN_TEST_UTIL_VALIDATION_H diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 9f78215de2..1881573e7a 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -17,6 +17,7 @@ #include <util/message.h> // For MessageSign(), MessageVerify(), MESSAGE_MAGIC #include <util/moneystr.h> #include <util/overflow.h> +#include <util/readwritefile.h> #include <util/spanparsing.h> #include <util/strencodings.h> #include <util/string.h> @@ -24,9 +25,10 @@ #include <util/vector.h> #include <array> -#include <optional> +#include <fstream> #include <limits> #include <map> +#include <optional> #include <stdint.h> #include <string.h> #include <thread> @@ -2592,4 +2594,49 @@ BOOST_AUTO_TEST_CASE(util_ParseByteUnits) BOOST_CHECK(!ParseByteUnits("1x", noop)); } +BOOST_AUTO_TEST_CASE(util_ReadBinaryFile) +{ + fs::path tmpfolder = m_args.GetDataDirBase(); + fs::path tmpfile = tmpfolder / "read_binary.dat"; + std::string expected_text; + for (int i = 0; i < 30; i++) { + expected_text += "0123456789"; + } + { + std::ofstream file{tmpfile}; + file << expected_text; + } + { + // read all contents in file + auto [valid, text] = ReadBinaryFile(tmpfile); + BOOST_CHECK(valid); + BOOST_CHECK_EQUAL(text, expected_text); + } + { + // read half contents in file + auto [valid, text] = ReadBinaryFile(tmpfile, expected_text.size() / 2); + BOOST_CHECK(valid); + BOOST_CHECK_EQUAL(text, expected_text.substr(0, expected_text.size() / 2)); + } + { + // read from non-existent file + fs::path invalid_file = tmpfolder / "invalid_binary.dat"; + auto [valid, text] = ReadBinaryFile(invalid_file); + BOOST_CHECK(!valid); + BOOST_CHECK(text.empty()); + } +} + +BOOST_AUTO_TEST_CASE(util_WriteBinaryFile) +{ + fs::path tmpfolder = m_args.GetDataDirBase(); + fs::path tmpfile = tmpfolder / "write_binary.dat"; + std::string expected_text = "bitcoin"; + auto valid = WriteBinaryFile(tmpfile, expected_text); + std::string actual_text; + std::ifstream file{tmpfile}; + file >> actual_text; + BOOST_CHECK(valid); + BOOST_CHECK_EQUAL(actual_text, expected_text); +} BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp index 1beef5cf04..b0d7389d39 100644 --- a/src/test/validation_chainstate_tests.cpp +++ b/src/test/validation_chainstate_tests.cpp @@ -72,9 +72,6 @@ BOOST_AUTO_TEST_CASE(validation_chainstate_resize_caches) // The view cache should be empty since we had to destruct to downsize. BOOST_CHECK(!c1.CoinsTip().HaveCoinInCache(outpoint)); } - - // Avoid triggering the address sanitizer. - WITH_LOCK(::cs_main, manager.Unload()); } //! Test UpdateTip behavior for both active and background chainstates. diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 26392e690d..5d0ec593e3 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -99,8 +99,6 @@ BOOST_AUTO_TEST_CASE(chainstatemanager) // Let scheduler events finish running to avoid accessing memory that is going to be unloaded SyncWithValidationInterfaceQueue(); - - WITH_LOCK(::cs_main, manager.Unload()); } //! Test rebalancing the caches associated with each chainstate. diff --git a/src/util/readwritefile.cpp b/src/util/readwritefile.cpp index a45c41d367..628e6a3980 100644 --- a/src/util/readwritefile.cpp +++ b/src/util/readwritefile.cpp @@ -18,7 +18,7 @@ std::pair<bool,std::string> ReadBinaryFile(const fs::path &filename, size_t maxs std::string retval; char buffer[128]; do { - const size_t n = fread(buffer, 1, sizeof(buffer), f); + const size_t n = fread(buffer, 1, std::min(sizeof(buffer), maxsize - retval.size()), f); // Check for reading errors so we don't return any data if we couldn't // read the entire file (or up to maxsize) if (ferror(f)) { @@ -26,7 +26,7 @@ std::pair<bool,std::string> ReadBinaryFile(const fs::path &filename, size_t maxs return std::make_pair(false,""); } retval.append(buffer, buffer+n); - } while (!feof(f) && retval.size() <= maxsize); + } while (!feof(f) && retval.size() < maxsize); fclose(f); return std::make_pair(true,retval); } diff --git a/src/util/system.cpp b/src/util/system.cpp index aa9122106b..8e45453d31 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -387,9 +387,12 @@ std::optional<unsigned int> ArgsManager::GetArgFlags(const std::string& name) co return std::nullopt; } -fs::path ArgsManager::GetPathArg(std::string pathlike_arg) const +fs::path ArgsManager::GetPathArg(std::string arg, const fs::path& default_value) const { - auto result = fs::PathFromString(GetArg(pathlike_arg, "")).lexically_normal(); + if (IsArgNegated(arg)) return fs::path{}; + std::string path_str = GetArg(arg, ""); + if (path_str.empty()) return default_value; + fs::path result = fs::PathFromString(path_str).lexically_normal(); // Remove trailing slash, if present. return result.has_filename() ? result : result.parent_path(); } @@ -516,12 +519,12 @@ bool ArgsManager::InitSettings(std::string& error) bool ArgsManager::GetSettingsPath(fs::path* filepath, bool temp) const { - if (IsArgNegated("-settings")) { + fs::path settings = GetPathArg("-settings", fs::path{BITCOIN_SETTINGS_FILENAME}); + if (settings.empty()) { return false; } if (filepath) { - std::string settings = GetArg("-settings", BITCOIN_SETTINGS_FILENAME); - *filepath = fsbridge::AbsPathJoin(GetDataDirNet(), fs::PathFromString(temp ? settings + ".tmp" : settings)); + *filepath = fsbridge::AbsPathJoin(GetDataDirNet(), temp ? settings + ".tmp" : settings); } return true; } @@ -588,7 +591,7 @@ bool ArgsManager::IsArgNegated(const std::string& strArg) const std::string ArgsManager::GetArg(const std::string& strArg, const std::string& strDefault) const { const util::SettingsValue value = GetSetting(strArg); - return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.get_str(); + return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.isNum() ? value.getValStr() : value.get_str(); } int64_t ArgsManager::GetIntArg(const std::string& strArg, int64_t nDefault) const diff --git a/src/util/system.h b/src/util/system.h index f193c8ac0b..a66b597d41 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -271,16 +271,6 @@ protected: std::optional<const Command> GetCommand() const; /** - * Get a normalized path from a specified pathlike argument - * - * It is guaranteed that the returned path has no trailing slashes. - * - * @param pathlike_arg Pathlike argument to get a path from (e.g., "-datadir", "-blocksdir" or "-walletdir") - * @return Normalized path which is get from a specified pathlike argument - */ - fs::path GetPathArg(std::string pathlike_arg) const; - - /** * Get blocks directory path * * @return Blocks path which is network specific @@ -343,6 +333,18 @@ protected: std::string GetArg(const std::string& strArg, const std::string& strDefault) const; /** + * Return path argument or default value + * + * @param arg Argument to get a path from (e.g., "-datadir", "-blocksdir" or "-walletdir") + * @param default_value Optional default value to return instead of the empty path. + * @return normalized path if argument is set, with redundant "." and ".." + * path components and trailing separators removed (see patharg unit test + * for examples or implementation for details). If argument is empty or not + * set, default_value is returned unchanged. + */ + fs::path GetPathArg(std::string arg, const fs::path& default_value = {}) const; + + /** * Return integer argument or default value * * @param strArg Argument to get (e.g. "-foo") diff --git a/src/validation.cpp b/src/validation.cpp index 214112e2bd..d80e2576d2 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1978,7 +1978,7 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state, // effectively caching the result of part of the verification. BlockMap::const_iterator it = m_blockman.m_block_index.find(hashAssumeValid); if (it != m_blockman.m_block_index.end()) { - if (it->second->GetAncestor(pindex->nHeight) == pindex && + if (it->second.GetAncestor(pindex->nHeight) == pindex && pindexBestHeader->GetAncestor(pindex->nHeight) == pindex && pindexBestHeader->nChainWork >= nMinimumChainWork) { // This block is a member of the assumed verified chain and an ancestor of the best header. @@ -3035,8 +3035,8 @@ bool CChainState::InvalidateBlock(BlockValidationState& state, CBlockIndex* pind { LOCK(cs_main); - for (const auto& entry : m_blockman.m_block_index) { - CBlockIndex *candidate = entry.second; + for (auto& entry : m_blockman.m_block_index) { + CBlockIndex* candidate = &entry.second; // We don't need to put anything in our active chain into the // multimap, because those candidates will be found and considered // as we disconnect. @@ -3133,12 +3133,10 @@ bool CChainState::InvalidateBlock(BlockValidationState& state, CBlockIndex* pind // it up here, this should be an essentially unobservable error. // Loop back over all block index entries and add any missing entries // to setBlockIndexCandidates. - BlockMap::iterator it = m_blockman.m_block_index.begin(); - while (it != m_blockman.m_block_index.end()) { - if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && it->second->HaveTxsDownloaded() && !setBlockIndexCandidates.value_comp()(it->second, m_chain.Tip())) { - setBlockIndexCandidates.insert(it->second); + for (auto& [_, block_index] : m_blockman.m_block_index) { + if (block_index.IsValid(BLOCK_VALID_TRANSACTIONS) && block_index.HaveTxsDownloaded() && !setBlockIndexCandidates.value_comp()(&block_index, m_chain.Tip())) { + setBlockIndexCandidates.insert(&block_index); } - it++; } InvalidChainFound(to_mark_failed); @@ -3157,21 +3155,19 @@ void CChainState::ResetBlockFailureFlags(CBlockIndex *pindex) { int nHeight = pindex->nHeight; // Remove the invalidity flag from this block and all its descendants. - BlockMap::iterator it = m_blockman.m_block_index.begin(); - while (it != m_blockman.m_block_index.end()) { - if (!it->second->IsValid() && it->second->GetAncestor(nHeight) == pindex) { - it->second->nStatus &= ~BLOCK_FAILED_MASK; - m_blockman.m_dirty_blockindex.insert(it->second); - if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && it->second->HaveTxsDownloaded() && setBlockIndexCandidates.value_comp()(m_chain.Tip(), it->second)) { - setBlockIndexCandidates.insert(it->second); + for (auto& [_, block_index] : m_blockman.m_block_index) { + if (!block_index.IsValid() && block_index.GetAncestor(nHeight) == pindex) { + block_index.nStatus &= ~BLOCK_FAILED_MASK; + m_blockman.m_dirty_blockindex.insert(&block_index); + if (block_index.IsValid(BLOCK_VALID_TRANSACTIONS) && block_index.HaveTxsDownloaded() && setBlockIndexCandidates.value_comp()(m_chain.Tip(), &block_index)) { + setBlockIndexCandidates.insert(&block_index); } - if (it->second == m_chainman.m_best_invalid) { + if (&block_index == m_chainman.m_best_invalid) { // Reset invalid block marker if it was pointing to one of those. m_chainman.m_best_invalid = nullptr; } - m_chainman.m_failed_blocks.erase(it->second); + m_chainman.m_failed_blocks.erase(&block_index); } - it++; } // Remove the invalidity flag from all ancestors too. @@ -3500,7 +3496,7 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida if (hash != chainparams.GetConsensus().hashGenesisBlock) { if (miSelf != m_blockman.m_block_index.end()) { // Block header is already known. - CBlockIndex* pindex = miSelf->second; + CBlockIndex* pindex = &(miSelf->second); if (ppindex) *ppindex = pindex; if (pindex->nStatus & BLOCK_FAILED_MASK) { @@ -3522,7 +3518,7 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida LogPrint(BCLog::VALIDATION, "%s: %s prev block not found\n", __func__, hash.ToString()); return state.Invalid(BlockValidationResult::BLOCK_MISSING_PREV, "prev-blk-not-found"); } - pindexPrev = (*mi).second; + pindexPrev = &((*mi).second); if (pindexPrev->nStatus & BLOCK_FAILED_MASK) { LogPrint(BCLog::VALIDATION, "%s: %s prev block invalid\n", __func__, hash.ToString()); return state.Invalid(BlockValidationResult::BLOCK_INVALID_PREV, "bad-prevblk"); @@ -3994,13 +3990,13 @@ bool CChainState::ReplayBlocks() if (m_blockman.m_block_index.count(hashHeads[0]) == 0) { return error("ReplayBlocks(): reorganization to unknown block requested"); } - pindexNew = m_blockman.m_block_index[hashHeads[0]]; + pindexNew = &(m_blockman.m_block_index[hashHeads[0]]); if (!hashHeads[1].IsNull()) { // The old tip is allowed to be 0, indicating it's the first flush. if (m_blockman.m_block_index.count(hashHeads[1]) == 0) { return error("ReplayBlocks(): reorganization from unknown block requested"); } - pindexOld = m_blockman.m_block_index[hashHeads[1]]; + pindexOld = &(m_blockman.m_block_index[hashHeads[1]]); pindexFork = LastCommonAncestor(pindexOld, pindexNew); assert(pindexFork != nullptr); } @@ -4070,7 +4066,7 @@ void CChainState::UnloadBlockIndex() // block index state void UnloadBlockIndex(CTxMemPool* mempool, ChainstateManager& chainman) { - LOCK(cs_main); + AssertLockHeld(::cs_main); chainman.Unload(); pindexBestHeader = nullptr; if (mempool) mempool->clear(); @@ -4267,8 +4263,8 @@ void CChainState::CheckBlockIndex() // Build forward-pointing map of the entire block tree. std::multimap<CBlockIndex*,CBlockIndex*> forward; - for (const std::pair<const uint256, CBlockIndex*>& entry : m_blockman.m_block_index) { - forward.insert(std::make_pair(entry.second->pprev, entry.second)); + for (auto& [_, block_index] : m_blockman.m_block_index) { + forward.emplace(block_index.pprev, &block_index); } assert(forward.size() == m_blockman.m_block_index.size()); @@ -5056,15 +5052,6 @@ void ChainstateManager::Unload() m_best_invalid = nullptr; } -void ChainstateManager::Reset() -{ - LOCK(::cs_main); - m_ibd_chainstate.reset(); - m_snapshot_chainstate.reset(); - m_active_chainstate = nullptr; - m_snapshot_validated = false; -} - void ChainstateManager::MaybeRebalanceCaches() { AssertLockHeld(::cs_main); diff --git a/src/validation.h b/src/validation.h index 7766d77a88..cc2247239f 100644 --- a/src/validation.h +++ b/src/validation.h @@ -138,7 +138,7 @@ extern CBlockIndex *pindexBestHeader; extern const std::vector<std::string> CHECKLEVEL_DOC; /** Unload database information */ -void UnloadBlockIndex(CTxMemPool* mempool, ChainstateManager& chainman); +void UnloadBlockIndex(CTxMemPool* mempool, ChainstateManager& chainman) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** Run instances of script checking worker threads */ void StartScriptCheckWorkerThreads(int threads_num); /** Stop all of the script checking worker threads */ @@ -991,17 +991,13 @@ public: //! Unload block index and chain data before shutdown. void Unload() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - //! Clear (deconstruct) chainstate data. - void Reset(); - //! Check to see if caches are out of balance and if so, call //! ResizeCoinsCaches() as needed. void MaybeRebalanceCaches() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); ~ChainstateManager() { LOCK(::cs_main); - UnloadBlockIndex(/* mempool */ nullptr, *this); - Reset(); + UnloadBlockIndex(/*mempool=*/nullptr, *this); } }; diff --git a/src/validationinterface.h b/src/validationinterface.h index 7c3ce00fbc..ac62f8b467 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -174,6 +174,7 @@ protected: * has been received and connected to the headers tree, though not validated yet */ virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& block) {}; friend class CMainSignals; + friend class ValidationInterfaceTest; }; struct MainSignalsInstance; diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp index eef2c13ee1..ad94ce4b32 100644 --- a/src/wallet/rpc/transactions.cpp +++ b/src/wallet/rpc/transactions.cpp @@ -34,6 +34,7 @@ static void WalletTxToJSON(const CWallet& wallet, const CWalletTx& wtx, UniValue } uint256 hash = wtx.GetHash(); entry.pushKV("txid", hash.GetHex()); + entry.pushKV("wtxid", wtx.GetWitnessHash().GetHex()); UniValue conflicts(UniValue::VARR); for (const uint256& conflict : wallet.GetTxConflicts(wtx)) conflicts.push_back(conflict.GetHex()); @@ -431,6 +432,7 @@ static const std::vector<RPCResult> TransactionDescriptionString() {RPCResult::Type::NUM, "blockindex", /*optional=*/true, "The index of the transaction in the block that includes it."}, {RPCResult::Type::NUM_TIME, "blocktime", /*optional=*/true, "The block time expressed in " + UNIX_EPOCH_TIME + "."}, {RPCResult::Type::STR_HEX, "txid", "The transaction id."}, + {RPCResult::Type::STR_HEX, "wtxid", "The hash of serialized transaction, including witness data."}, {RPCResult::Type::ARR, "walletconflicts", "Conflicting transaction ids.", { {RPCResult::Type::STR_HEX, "txid", "The transaction id."}, diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 7693c9c0e8..c59f7e6f05 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -367,10 +367,10 @@ static int64_t AddTx(ChainstateManager& chainman, CWallet& wallet, uint32_t lock CBlockIndex* block = nullptr; if (blockTime > 0) { LOCK(cs_main); - auto inserted = chainman.BlockIndex().emplace(GetRandHash(), new CBlockIndex); + auto inserted = chainman.BlockIndex().emplace(std::piecewise_construct, std::make_tuple(GetRandHash()), std::make_tuple()); assert(inserted.second); const uint256& hash = inserted.first->first; - block = inserted.first->second; + block = &inserted.first->second; block->nTime = blockTime; block->phashBlock = &hash; state = TxStateConfirmed{hash, block->nHeight, /*position_in_block=*/0}; diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h index 00f9c9f154..271d698e56 100644 --- a/src/wallet/transaction.h +++ b/src/wallet/transaction.h @@ -296,6 +296,7 @@ public: bool isUnconfirmed() const { return !isAbandoned() && !isConflicted() && !isConfirmed(); } bool isConfirmed() const { return state<TxStateConfirmed>(); } const uint256& GetHash() const { return tx->GetHash(); } + const uint256& GetWitnessHash() const { return tx->GetWitnessHash(); } bool IsCoinBase() const { return tx->IsCoinBase(); } // Disable copying of CWalletTx objects to prevent bugs where instances get |