From a0b5b4ae5a24536d333cbce2ea584f2d935c651f Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 13 Jan 2022 07:55:18 -0500 Subject: [PATCH 1/7] interfaces, refactor: Add more block information to block connected notifications Add new interfaces::BlockInfo struct to be able to pass extra block information (file and undo information) to indexes which they are updated to use high level interfaces::Chain notifications. This commit does not change behavior in any way. --- src/Makefile.am | 2 ++ src/interfaces/chain.h | 18 ++++++++++++++++-- src/kernel/chain.cpp | 26 ++++++++++++++++++++++++++ src/kernel/chain.h | 19 +++++++++++++++++++ src/node/interfaces.cpp | 5 +++-- src/wallet/test/fuzz/notifications.cpp | 18 ++++++++++++++---- src/wallet/wallet.cpp | 23 ++++++++++++----------- src/wallet/wallet.h | 4 ++-- 8 files changed, 94 insertions(+), 21 deletions(-) create mode 100644 src/kernel/chain.cpp create mode 100644 src/kernel/chain.h diff --git a/src/Makefile.am b/src/Makefile.am index 02ec5c098eb..87c7a056b1d 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -171,6 +171,7 @@ BITCOIN_CORE_H = \ interfaces/ipc.h \ interfaces/node.h \ interfaces/wallet.h \ + kernel/chain.h \ kernel/chainstatemanager_opts.h \ kernel/checks.h \ kernel/coinstats.h \ @@ -365,6 +366,7 @@ libbitcoin_node_a_SOURCES = \ index/coinstatsindex.cpp \ index/txindex.cpp \ init.cpp \ + kernel/chain.cpp \ kernel/checks.cpp \ kernel/coinstats.cpp \ kernel/context.cpp \ diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index ff994b8edcb..6396d0d3599 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -18,6 +18,7 @@ class ArgsManager; class CBlock; +class CBlockUndo; class CFeeRate; class CRPCCommand; class CScheduler; @@ -67,6 +68,19 @@ public: mutable bool found = false; }; +//! Block data sent with blockConnected, blockDisconnected notifications. +struct BlockInfo { + const uint256& hash; + const uint256* prev_hash = nullptr; + int height = -1; + int file_number = -1; + unsigned data_pos = 0; + const CBlock* data = nullptr; + const CBlockUndo* undo_data = nullptr; + + BlockInfo(const uint256& hash LIFETIMEBOUND) : hash(hash) {} +}; + //! Interface giving clients (wallet processes, maybe other analysis tools in //! the future) ability to access to the chain state, receive notifications, //! estimate fees, and submit transactions. @@ -239,8 +253,8 @@ public: virtual ~Notifications() {} virtual void transactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) {} virtual void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) {} - virtual void blockConnected(const CBlock& block, int height) {} - virtual void blockDisconnected(const CBlock& block, int height) {} + virtual void blockConnected(const BlockInfo& block) {} + virtual void blockDisconnected(const BlockInfo& block) {} virtual void updatedBlockTip() {} virtual void chainStateFlushed(const CBlockLocator& locator) {} }; diff --git a/src/kernel/chain.cpp b/src/kernel/chain.cpp new file mode 100644 index 00000000000..82e77125d7f --- /dev/null +++ b/src/kernel/chain.cpp @@ -0,0 +1,26 @@ +// Copyright (c) 2022 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 + +class CBlock; + +namespace kernel { +interfaces::BlockInfo MakeBlockInfo(const CBlockIndex* index, const CBlock* data) +{ + interfaces::BlockInfo info{index ? *index->phashBlock : uint256::ZERO}; + if (index) { + info.prev_hash = index->pprev ? index->pprev->phashBlock : nullptr; + info.height = index->nHeight; + LOCK(::cs_main); + info.file_number = index->nFile; + info.data_pos = index->nDataPos; + } + info.data = data; + return info; +} +} // namespace kernel diff --git a/src/kernel/chain.h b/src/kernel/chain.h new file mode 100644 index 00000000000..f0750f82663 --- /dev/null +++ b/src/kernel/chain.h @@ -0,0 +1,19 @@ +// Copyright (c) 2022 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_KERNEL_CHAIN_H +#define BITCOIN_KERNEL_CHAIN_H + +class CBlock; +class CBlockIndex; +namespace interfaces { +struct BlockInfo; +} // namespace interfaces + +namespace kernel { +//! Return data from block index. +interfaces::BlockInfo MakeBlockInfo(const CBlockIndex* block_index, const CBlock* data = nullptr); +} // namespace kernel + +#endif // BITCOIN_KERNEL_CHAIN_H diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 46e0efb9b64..2562d81e48b 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -426,11 +427,11 @@ public: } void BlockConnected(const std::shared_ptr& block, const CBlockIndex* index) override { - m_notifications->blockConnected(*block, index->nHeight); + m_notifications->blockConnected(kernel::MakeBlockInfo(index, block.get())); } void BlockDisconnected(const std::shared_ptr& block, const CBlockIndex* index) override { - m_notifications->blockDisconnected(*block, index->nHeight); + m_notifications->blockDisconnected(kernel::MakeBlockInfo(index, block.get())); } void UpdatedBlockTip(const CBlockIndex* index, const CBlockIndex* fork_index, bool is_ibd) override { diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index 816bef6148a..5c173773e47 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -137,8 +137,13 @@ FUZZ_TARGET_INIT(wallet_notifications, initialize_setup) block.vtx.emplace_back(MakeTransactionRef(tx)); } // Mine block - a.wallet->blockConnected(block, chain.size()); - b.wallet->blockConnected(block, chain.size()); + const uint256& hash = block.GetHash(); + interfaces::BlockInfo info{hash}; + info.prev_hash = &block.hashPrevBlock; + info.height = chain.size(); + info.data = █ + a.wallet->blockConnected(info); + b.wallet->blockConnected(info); // Store the coins for the next block Coins coins_new; for (const auto& tx : block.vtx) { @@ -154,8 +159,13 @@ FUZZ_TARGET_INIT(wallet_notifications, initialize_setup) auto& [coins, block]{chain.back()}; if (block.vtx.empty()) return; // Can only disconnect if the block was submitted first // Disconnect block - a.wallet->blockDisconnected(block, chain.size() - 1); - b.wallet->blockDisconnected(block, chain.size() - 1); + const uint256& hash = block.GetHash(); + interfaces::BlockInfo info{hash}; + info.prev_hash = &block.hashPrevBlock; + info.height = chain.size() - 1; + info.data = █ + a.wallet->blockDisconnected(info); + b.wallet->blockDisconnected(info); chain.pop_back(); }); auto& [coins, first_block]{chain.front()}; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 890e1719131..c8d6ccd48b5 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1314,30 +1314,31 @@ void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRe } } -void CWallet::blockConnected(const CBlock& block, int height) +void CWallet::blockConnected(const interfaces::BlockInfo& block) { - const uint256& block_hash = block.GetHash(); + assert(block.data); LOCK(cs_wallet); - m_last_block_processed_height = height; - m_last_block_processed = block_hash; - for (size_t index = 0; index < block.vtx.size(); index++) { - SyncTransaction(block.vtx[index], TxStateConfirmed{block_hash, height, static_cast(index)}); - transactionRemovedFromMempool(block.vtx[index], MemPoolRemovalReason::BLOCK, 0 /* mempool_sequence */); + m_last_block_processed_height = block.height; + m_last_block_processed = block.hash; + for (size_t index = 0; index < block.data->vtx.size(); index++) { + SyncTransaction(block.data->vtx[index], TxStateConfirmed{block.hash, block.height, static_cast(index)}); + transactionRemovedFromMempool(block.data->vtx[index], MemPoolRemovalReason::BLOCK, 0 /* mempool_sequence */); } } -void CWallet::blockDisconnected(const CBlock& block, int height) +void CWallet::blockDisconnected(const interfaces::BlockInfo& block) { + assert(block.data); LOCK(cs_wallet); // At block disconnection, this will change an abandoned transaction to // be unconfirmed, whether or not the transaction is added back to the mempool. // User may have to call abandontransaction again. It may be addressed in the // future with a stickier abandoned state or even removing abandontransaction call. - m_last_block_processed_height = height - 1; - m_last_block_processed = block.hashPrevBlock; - for (const CTransactionRef& ptx : block.vtx) { + m_last_block_processed_height = block.height - 1; + m_last_block_processed = *Assert(block.prev_hash); + for (const CTransactionRef& ptx : Assert(block.data)->vtx) { SyncTransaction(ptx, TxStateInactive{}); } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index e2c3d76438f..15f59170401 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -508,8 +508,8 @@ public: CWalletTx* AddToWallet(CTransactionRef tx, const TxState& state, const UpdateWalletTxFn& update_wtx=nullptr, bool fFlushOnClose=true, bool rescanning_old_block = false); bool LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void transactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) override; - void blockConnected(const CBlock& block, int height) override; - void blockDisconnected(const CBlock& block, int height) override; + void blockConnected(const interfaces::BlockInfo& block) override; + void blockDisconnected(const interfaces::BlockInfo& block) override; void updatedBlockTip() override; int64_t RescanFromTime(int64_t startTime, const WalletRescanReserver& reserver, bool update); From 33b4d48cfcdf145f49cb2283ac3e2936a4e23fff Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 13 Jan 2022 07:57:54 -0500 Subject: [PATCH 2/7] indexes, refactor: Pass Chain interface instead of CChainState class to indexes Passing abstract Chain interface will let indexes run in separate processes. This commit does not change behavior in any way. --- src/index/base.cpp | 11 +++++++++-- src/index/base.h | 8 +++++++- src/index/blockfilterindex.cpp | 8 ++++---- src/index/blockfilterindex.h | 4 ++-- src/index/coinstatsindex.cpp | 3 ++- src/index/coinstatsindex.h | 2 +- src/index/txindex.cpp | 4 ++-- src/index/txindex.h | 2 +- src/init.cpp | 12 ++++++------ src/interfaces/chain.h | 4 ++++ src/node/interfaces.cpp | 1 + src/test/blockfilter_index_tests.cpp | 11 ++++++----- src/test/coinstatsindex_tests.cpp | 13 +++++++------ src/test/txindex_tests.cpp | 5 +++-- test/lint/lint-circular-dependencies.py | 3 +++ 15 files changed, 58 insertions(+), 33 deletions(-) diff --git a/src/index/base.cpp b/src/index/base.cpp index 323547900d1..26b3653f7bb 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -4,7 +4,9 @@ #include #include +#include #include +#include #include #include #include @@ -49,6 +51,9 @@ void BaseIndex::DB::WriteBestBlock(CDBBatch& batch, const CBlockLocator& locator batch.Write(DB_BEST_BLOCK, locator); } +BaseIndex::BaseIndex(std::unique_ptr chain) + : m_chain{std::move(chain)} {} + BaseIndex::~BaseIndex() { Interrupt(); @@ -346,9 +351,11 @@ void BaseIndex::Interrupt() m_interrupt(); } -bool BaseIndex::Start(CChainState& active_chainstate) +bool BaseIndex::Start() { - m_chainstate = &active_chainstate; + // m_chainstate member gives indexing code access to node internals. It is + // removed in followup https://github.com/bitcoin/bitcoin/pull/24230 + m_chainstate = &m_chain->context()->chainman->ActiveChainstate(); // Need to register this ValidationInterface before running Init(), so that // callbacks are not missed if Init sets m_synced to true. RegisterValidationInterface(this); diff --git a/src/index/base.h b/src/index/base.h index a8f6a18c8de..35863764594 100644 --- a/src/index/base.h +++ b/src/index/base.h @@ -6,12 +6,16 @@ #define BITCOIN_INDEX_BASE_H #include +#include #include #include class CBlock; class CBlockIndex; class CChainState; +namespace interfaces { +class Chain; +} // namespace interfaces struct IndexSummary { std::string name; @@ -79,6 +83,7 @@ private: virtual bool AllowPrune() const = 0; protected: + std::unique_ptr m_chain; CChainState* m_chainstate{nullptr}; void BlockConnected(const std::shared_ptr& block, const CBlockIndex* pindex) override; @@ -110,6 +115,7 @@ protected: void SetBestBlockIndex(const CBlockIndex* block); public: + BaseIndex(std::unique_ptr chain); /// Destructor interrupts sync thread if running and blocks until it exits. virtual ~BaseIndex(); @@ -124,7 +130,7 @@ public: /// Start initializes the sync state and registers the instance as a /// ValidationInterface so that it stays in sync with blockchain updates. - [[nodiscard]] bool Start(CChainState& active_chainstate); + [[nodiscard]] bool Start(); /// Stops the instance from staying in sync with blockchain updates. void Stop(); diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp index e7fad8eb646..15a7cfeaea6 100644 --- a/src/index/blockfilterindex.cpp +++ b/src/index/blockfilterindex.cpp @@ -94,9 +94,9 @@ struct DBHashKey { static std::map g_filter_indexes; -BlockFilterIndex::BlockFilterIndex(BlockFilterType filter_type, +BlockFilterIndex::BlockFilterIndex(std::unique_ptr chain, BlockFilterType filter_type, size_t n_cache_size, bool f_memory, bool f_wipe) - : m_filter_type(filter_type) + : BaseIndex(std::move(chain)), m_filter_type(filter_type) { const std::string& filter_name = BlockFilterTypeName(filter_type); if (filter_name.empty()) throw std::invalid_argument("unknown filter_type"); @@ -467,12 +467,12 @@ void ForEachBlockFilterIndex(std::function fn) for (auto& entry : g_filter_indexes) fn(entry.second); } -bool InitBlockFilterIndex(BlockFilterType filter_type, +bool InitBlockFilterIndex(std::function()> make_chain, BlockFilterType filter_type, size_t n_cache_size, bool f_memory, bool f_wipe) { auto result = g_filter_indexes.emplace(std::piecewise_construct, std::forward_as_tuple(filter_type), - std::forward_as_tuple(filter_type, + std::forward_as_tuple(make_chain(), filter_type, n_cache_size, f_memory, f_wipe)); return result.second; } diff --git a/src/index/blockfilterindex.h b/src/index/blockfilterindex.h index fef8b573e8b..71e150ba751 100644 --- a/src/index/blockfilterindex.h +++ b/src/index/blockfilterindex.h @@ -55,7 +55,7 @@ protected: public: /** Constructs the index, which becomes available to be queried. */ - explicit BlockFilterIndex(BlockFilterType filter_type, + explicit BlockFilterIndex(std::unique_ptr chain, BlockFilterType filter_type, size_t n_cache_size, bool f_memory = false, bool f_wipe = false); BlockFilterType GetFilterType() const { return m_filter_type; } @@ -88,7 +88,7 @@ void ForEachBlockFilterIndex(std::function fn); * Initialize a block filter index for the given type if one does not already exist. Returns true if * a new index is created and false if one has already been initialized. */ -bool InitBlockFilterIndex(BlockFilterType filter_type, +bool InitBlockFilterIndex(std::function()> make_chain, BlockFilterType filter_type, size_t n_cache_size, bool f_memory = false, bool f_wipe = false); /** diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 687e330fe0b..7373ed249db 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -102,7 +102,8 @@ struct DBHashKey { std::unique_ptr g_coin_stats_index; -CoinStatsIndex::CoinStatsIndex(size_t n_cache_size, bool f_memory, bool f_wipe) +CoinStatsIndex::CoinStatsIndex(std::unique_ptr chain, size_t n_cache_size, bool f_memory, bool f_wipe) + : BaseIndex(std::move(chain)) { fs::path path{gArgs.GetDataDirNet() / "indexes" / "coinstats"}; fs::create_directories(path); diff --git a/src/index/coinstatsindex.h b/src/index/coinstatsindex.h index cae052d9133..06846d07ab4 100644 --- a/src/index/coinstatsindex.h +++ b/src/index/coinstatsindex.h @@ -53,7 +53,7 @@ protected: public: // Constructs the index, which becomes available to be queried. - explicit CoinStatsIndex(size_t n_cache_size, bool f_memory = false, bool f_wipe = false); + explicit CoinStatsIndex(std::unique_ptr chain, size_t n_cache_size, bool f_memory = false, bool f_wipe = false); // Look up stats for a specific block using CBlockIndex std::optional LookUpStats(const CBlockIndex* block_index) const; diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp index 97c11c4383c..40395885869 100644 --- a/src/index/txindex.cpp +++ b/src/index/txindex.cpp @@ -48,8 +48,8 @@ bool TxIndex::DB::WriteTxs(const std::vector>& v_ return WriteBatch(batch); } -TxIndex::TxIndex(size_t n_cache_size, bool f_memory, bool f_wipe) - : m_db(std::make_unique(n_cache_size, f_memory, f_wipe)) +TxIndex::TxIndex(std::unique_ptr chain, size_t n_cache_size, bool f_memory, bool f_wipe) + : BaseIndex(std::move(chain)), m_db(std::make_unique(n_cache_size, f_memory, f_wipe)) {} TxIndex::~TxIndex() = default; diff --git a/src/index/txindex.h b/src/index/txindex.h index ec339abaa18..f74a7511dcf 100644 --- a/src/index/txindex.h +++ b/src/index/txindex.h @@ -31,7 +31,7 @@ protected: public: /// Constructs the index, which becomes available to be queried. - explicit TxIndex(size_t n_cache_size, bool f_memory = false, bool f_wipe = false); + explicit TxIndex(std::unique_ptr chain, size_t n_cache_size, bool f_memory = false, bool f_wipe = false); // Destructor is declared because this class contains a unique_ptr to an incomplete type. virtual ~TxIndex() override; diff --git a/src/init.cpp b/src/init.cpp index 97c823fe0c1..046857d60cd 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1594,22 +1594,22 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) return InitError(*error); } - g_txindex = std::make_unique(cache_sizes.tx_index, false, fReindex); - if (!g_txindex->Start(chainman.ActiveChainstate())) { + g_txindex = std::make_unique(interfaces::MakeChain(node), cache_sizes.tx_index, false, fReindex); + if (!g_txindex->Start()) { return false; } } for (const auto& filter_type : g_enabled_filter_types) { - InitBlockFilterIndex(filter_type, cache_sizes.filter_index, false, fReindex); - if (!GetBlockFilterIndex(filter_type)->Start(chainman.ActiveChainstate())) { + InitBlockFilterIndex([&]{ return interfaces::MakeChain(node); }, filter_type, cache_sizes.filter_index, false, fReindex); + if (!GetBlockFilterIndex(filter_type)->Start()) { return false; } } if (args.GetBoolArg("-coinstatsindex", DEFAULT_COINSTATSINDEX)) { - g_coin_stats_index = std::make_unique(/* cache size */ 0, false, fReindex); - if (!g_coin_stats_index->Start(chainman.ActiveChainstate())) { + g_coin_stats_index = std::make_unique(interfaces::MakeChain(node), /* cache size */ 0, false, fReindex); + if (!g_coin_stats_index->Start()) { return false; } } diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index 6396d0d3599..4b192c29ccc 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -304,6 +304,10 @@ public: //! Return true if an assumed-valid chain is in use. virtual bool hasAssumedValidChain() = 0; + + //! Get internal node context. Useful for testing, but not + //! accessible across processes. + virtual node::NodeContext* context() { return nullptr; } }; //! Interface to let node manage chain clients (wallets, or maybe tools for diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 2562d81e48b..414eff71256 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -781,6 +781,7 @@ public: return Assert(m_node.chainman)->IsSnapshotActive(); } + NodeContext* context() override { return &m_node; } NodeContext& m_node; }; } // namespace diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp index c31e4e51f7c..1a182209b89 100644 --- a/src/test/blockfilter_index_tests.cpp +++ b/src/test/blockfilter_index_tests.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include