From 4d8f4dcb450d31e4847804e62bf91545b949fa14 Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Mon, 23 Sep 2019 13:54:21 -0400 Subject: [PATCH] validation: pass ChainstateRole for validationinterface calls This allows consumers to decide how to handle events from background or assumedvalid chainstates. --- src/bench/wallet_create_tx.cpp | 2 +- src/index/base.cpp | 4 ++-- src/index/base.h | 4 ++-- src/interfaces/chain.h | 5 +++-- src/net_processing.cpp | 8 ++++++-- src/node/interfaces.cpp | 8 +++++--- src/test/coinstatsindex_tests.cpp | 2 +- src/test/util/validation.cpp | 8 ++++++-- src/test/util/validation.h | 6 +++++- src/test/validation_block_tests.cpp | 2 +- src/test/validationinterface_tests.cpp | 1 + src/validation.cpp | 5 +++-- src/validationinterface.cpp | 13 +++++++------ src/validationinterface.h | 17 ++++++++++------- src/wallet/test/fuzz/notifications.cpp | 5 +++-- src/wallet/wallet.cpp | 9 +++++---- src/wallet/wallet.h | 4 ++-- src/zmq/zmqnotificationinterface.cpp | 3 ++- src/zmq/zmqnotificationinterface.h | 2 +- 19 files changed, 66 insertions(+), 42 deletions(-) diff --git a/src/bench/wallet_create_tx.cpp b/src/bench/wallet_create_tx.cpp index 5e5bc76fd21..160534b63ca 100644 --- a/src/bench/wallet_create_tx.cpp +++ b/src/bench/wallet_create_tx.cpp @@ -70,7 +70,7 @@ void generateFakeBlock(const CChainParams& params, // notify wallet const auto& pindex = WITH_LOCK(::cs_main, return context.chainman->ActiveChain().Tip()); - wallet.blockConnected(kernel::MakeBlockInfo(pindex, &block)); + wallet.blockConnected(ChainstateRole::NORMAL, kernel::MakeBlockInfo(pindex, &block)); } struct PreSelectInputs { diff --git a/src/index/base.cpp b/src/index/base.cpp index f18205a76ff..98a8bad102d 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -250,7 +250,7 @@ bool BaseIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_ti return true; } -void BaseIndex::BlockConnected(const std::shared_ptr& block, const CBlockIndex* pindex) +void BaseIndex::BlockConnected(ChainstateRole role, const std::shared_ptr& block, const CBlockIndex* pindex) { if (!m_synced) { return; @@ -296,7 +296,7 @@ void BaseIndex::BlockConnected(const std::shared_ptr& block, const } } -void BaseIndex::ChainStateFlushed(const CBlockLocator& locator) +void BaseIndex::ChainStateFlushed(ChainstateRole role, const CBlockLocator& locator) { if (!m_synced) { return; diff --git a/src/index/base.h b/src/index/base.h index 9b2a41dc92b..b93103eb366 100644 --- a/src/index/base.h +++ b/src/index/base.h @@ -102,9 +102,9 @@ protected: Chainstate* m_chainstate{nullptr}; const std::string m_name; - void BlockConnected(const std::shared_ptr& block, const CBlockIndex* pindex) override; + void BlockConnected(ChainstateRole role, const std::shared_ptr& block, const CBlockIndex* pindex) override; - void ChainStateFlushed(const CBlockLocator& locator) override; + void ChainStateFlushed(ChainstateRole role, const CBlockLocator& locator) override; /// Initialize internal state from the database and block index. [[nodiscard]] virtual bool CustomInit(const std::optional& block) { return true; } diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index b5243725ad0..dea868f844d 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -27,6 +27,7 @@ class Coin; class uint256; enum class MemPoolRemovalReason; enum class RBFTransactionState; +enum class ChainstateRole; struct bilingual_str; struct CBlockLocator; struct FeeCalculation; @@ -310,10 +311,10 @@ public: virtual ~Notifications() {} virtual void transactionAddedToMempool(const CTransactionRef& tx) {} virtual void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) {} - virtual void blockConnected(const BlockInfo& block) {} + virtual void blockConnected(ChainstateRole role, const BlockInfo& block) {} virtual void blockDisconnected(const BlockInfo& block) {} virtual void updatedBlockTip() {} - virtual void chainStateFlushed(const CBlockLocator& locator) {} + virtual void chainStateFlushed(ChainstateRole role, const CBlockLocator& locator) {} }; //! Register handler for notifications. diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 46759423663..12dca182c38 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -483,7 +484,7 @@ public: CTxMemPool& pool, Options opts); /** Overridden from CValidationInterface. */ - void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) override + void BlockConnected(ChainstateRole role, const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) override EXCLUSIVE_LOCKS_REQUIRED(!m_recent_confirmed_transactions_mutex); void BlockDisconnected(const std::shared_ptr &block, const CBlockIndex* pindex) override EXCLUSIVE_LOCKS_REQUIRED(!m_recent_confirmed_transactions_mutex); @@ -1911,7 +1912,10 @@ void PeerManagerImpl::StartScheduledTasks(CScheduler& scheduler) * announcements for them. Also save the time of the last tip update and * possibly reduce dynamic block stalling timeout. */ -void PeerManagerImpl::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindex) +void PeerManagerImpl::BlockConnected( + ChainstateRole role, + const std::shared_ptr& pblock, + const CBlockIndex* pindex) { m_orphanage.EraseForBlock(*pblock); m_last_tip_update = GetTime(); diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index e0c40036d90..4baa0da67cd 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -434,9 +434,9 @@ public: { m_notifications->transactionRemovedFromMempool(tx, reason); } - void BlockConnected(const std::shared_ptr& block, const CBlockIndex* index) override + void BlockConnected(ChainstateRole role, const std::shared_ptr& block, const CBlockIndex* index) override { - m_notifications->blockConnected(kernel::MakeBlockInfo(index, block.get())); + m_notifications->blockConnected(role, kernel::MakeBlockInfo(index, block.get())); } void BlockDisconnected(const std::shared_ptr& block, const CBlockIndex* index) override { @@ -446,7 +446,9 @@ public: { m_notifications->updatedBlockTip(); } - void ChainStateFlushed(const CBlockLocator& locator) override { m_notifications->chainStateFlushed(locator); } + void ChainStateFlushed(ChainstateRole role, const CBlockLocator& locator) override { + m_notifications->chainStateFlushed(role, locator); + } std::shared_ptr m_notifications; }; diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp index 787a196a0ca..50f3f7d8336 100644 --- a/src/test/coinstatsindex_tests.cpp +++ b/src/test/coinstatsindex_tests.cpp @@ -105,7 +105,7 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_unclean_shutdown, TestChain100Setup) // 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); + ValidationInterfaceTest::BlockConnected(ChainstateRole::NORMAL, index, new_block, new_block_index); index.Stop(); } diff --git a/src/test/util/validation.cpp b/src/test/util/validation.cpp index 2d5562ae66c..bcd6a7a7dc3 100644 --- a/src/test/util/validation.cpp +++ b/src/test/util/validation.cpp @@ -22,7 +22,11 @@ void TestChainstateManager::JumpOutOfIbd() Assert(!IsInitialBlockDownload()); } -void ValidationInterfaceTest::BlockConnected(CValidationInterface& obj, const std::shared_ptr& block, const CBlockIndex* pindex) +void ValidationInterfaceTest::BlockConnected( + ChainstateRole role, + CValidationInterface& obj, + const std::shared_ptr& block, + const CBlockIndex* pindex) { - obj.BlockConnected(block, pindex); + obj.BlockConnected(role, block, pindex); } diff --git a/src/test/util/validation.h b/src/test/util/validation.h index 64654f3fb61..45ef773409a 100644 --- a/src/test/util/validation.h +++ b/src/test/util/validation.h @@ -19,7 +19,11 @@ struct TestChainstateManager : public ChainstateManager { class ValidationInterfaceTest { public: - static void BlockConnected(CValidationInterface& obj, const std::shared_ptr& block, const CBlockIndex* pindex); + static void BlockConnected( + ChainstateRole role, + CValidationInterface& obj, + const std::shared_ptr& block, + const CBlockIndex* pindex); }; #endif // BITCOIN_TEST_UTIL_VALIDATION_H diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index d1463634cc2..411371f7c16 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -43,7 +43,7 @@ struct TestSubscriber final : public CValidationInterface { BOOST_CHECK_EQUAL(m_expected_tip, pindexNew->GetBlockHash()); } - void BlockConnected(const std::shared_ptr& block, const CBlockIndex* pindex) override + void BlockConnected(ChainstateRole role, const std::shared_ptr& block, const CBlockIndex* pindex) override { BOOST_CHECK_EQUAL(m_expected_tip, block->hashPrevBlock); BOOST_CHECK_EQUAL(m_expected_tip, pindex->pprev->GetBlockHash()); diff --git a/src/test/validationinterface_tests.cpp b/src/test/validationinterface_tests.cpp index fcd0b25b388..5979441057c 100644 --- a/src/test/validationinterface_tests.cpp +++ b/src/test/validationinterface_tests.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include diff --git a/src/validation.cpp b/src/validation.cpp index 4873eb964ca..8c657839e8d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5,6 +5,7 @@ #include +#include #include #include @@ -2645,7 +2646,7 @@ bool Chainstate::FlushStateToDisk( } if (full_flush_completed) { // Update best block in wallet (so we can detect restored wallets). - GetMainSignals().ChainStateFlushed(m_chain.GetLocator()); + GetMainSignals().ChainStateFlushed(this->GetRole(), m_chain.GetLocator()); } } catch (const std::runtime_error& e) { return FatalError(m_chainman.GetNotifications(), state, std::string("System error while flushing: ") + e.what()); @@ -3239,7 +3240,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) { assert(trace.pblock && trace.pindex); - GetMainSignals().BlockConnected(trace.pblock, trace.pindex); + GetMainSignals().BlockConnected(this->GetRole(), trace.pblock, trace.pindex); } // This will have been toggled in diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index d344c8bfbda..9241395ad54 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -223,9 +224,9 @@ void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, MemP RemovalReasonToString(reason)); } -void CMainSignals::BlockConnected(const std::shared_ptr &pblock, const CBlockIndex *pindex) { - auto event = [pblock, pindex, this] { - m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockConnected(pblock, pindex); }); +void CMainSignals::BlockConnected(ChainstateRole role, const std::shared_ptr &pblock, const CBlockIndex *pindex) { + auto event = [role, pblock, pindex, this] { + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockConnected(role, pblock, pindex); }); }; ENQUEUE_AND_LOG_EVENT(event, "%s: block hash=%s block height=%d", __func__, pblock->GetHash().ToString(), @@ -242,9 +243,9 @@ void CMainSignals::BlockDisconnected(const std::shared_ptr& pblock pindex->nHeight); } -void CMainSignals::ChainStateFlushed(const CBlockLocator &locator) { - auto event = [locator, this] { - m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.ChainStateFlushed(locator); }); +void CMainSignals::ChainStateFlushed(ChainstateRole role, const CBlockLocator &locator) { + auto event = [role, locator, this] { + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.ChainStateFlushed(role, locator); }); }; ENQUEUE_AND_LOG_EVENT(event, "%s: block hash=%s", __func__, locator.IsNull() ? "null" : locator.vHave.front().ToString()); diff --git a/src/validationinterface.h b/src/validationinterface.h index 5bdd7e01238..eb15aa4d5f7 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -7,6 +7,7 @@ #define BITCOIN_VALIDATIONINTERFACE_H #include +#include #include // CTransaction(Ref) #include @@ -136,11 +137,12 @@ protected: * * Called on a background thread. */ - virtual void BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex) {} + virtual void BlockConnected(ChainstateRole role, const std::shared_ptr &block, const CBlockIndex *pindex) {} /** * Notifies listeners of a block being disconnected * - * Called on a background thread. + * Called on a background thread. Only called for the active chainstate, since + * background chainstates should never disconnect blocks. */ virtual void BlockDisconnected(const std::shared_ptr &block, const CBlockIndex* pindex) {} /** @@ -159,17 +161,18 @@ protected: * * Called on a background thread. */ - virtual void ChainStateFlushed(const CBlockLocator &locator) {} + virtual void ChainStateFlushed(ChainstateRole role, const CBlockLocator &locator) {} /** * Notifies listeners of a block validation result. * If the provided BlockValidationState IsValid, the provided block * is guaranteed to be the current best block at the time the - * callback was generated (not necessarily now) + * callback was generated (not necessarily now). */ virtual void BlockChecked(const CBlock&, const BlockValidationState&) {} /** * Notifies listeners that a block which builds directly on our current tip - * has been received and connected to the headers tree, though not validated yet */ + * has been received and connected to the headers tree, though not validated yet. + */ virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& block) {}; friend class CMainSignals; friend class ValidationInterfaceTest; @@ -199,9 +202,9 @@ public: void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload); void TransactionAddedToMempool(const CTransactionRef&, uint64_t mempool_sequence); void TransactionRemovedFromMempool(const CTransactionRef&, MemPoolRemovalReason, uint64_t mempool_sequence); - void BlockConnected(const std::shared_ptr &, const CBlockIndex *pindex); + void BlockConnected(ChainstateRole, const std::shared_ptr &, const CBlockIndex *pindex); void BlockDisconnected(const std::shared_ptr &, const CBlockIndex* pindex); - void ChainStateFlushed(const CBlockLocator &); + void ChainStateFlushed(ChainstateRole, const CBlockLocator &); void BlockChecked(const CBlock&, const BlockValidationState&); void NewPoWValidBlock(const CBlockIndex *, const std::shared_ptr&); }; diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index 42accafe5b0..abd788f96fb 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -2,6 +2,7 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include #include #include #include @@ -145,8 +146,8 @@ FUZZ_TARGET(wallet_notifications, .init = initialize_setup) // time to the maximum value. This ensures that the wallet's birth time is always // earlier than this maximum time. info.chain_time_max = std::numeric_limits::max(); - a.wallet->blockConnected(info); - b.wallet->blockConnected(info); + a.wallet->blockConnected(ChainstateRole::NORMAL, info); + b.wallet->blockConnected(ChainstateRole::NORMAL, info); // Store the coins for the next block Coins coins_new; for (const auto& tx : block.vtx) { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 24599084195..c840c2ee1f3 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -626,7 +627,7 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, return false; } -void CWallet::chainStateFlushed(const CBlockLocator& loc) +void CWallet::chainStateFlushed(ChainstateRole role, const CBlockLocator& loc) { // Don't update the best block until the chain is attached so that in case of a shutdown, // the rescan will be restarted at next startup. @@ -1462,7 +1463,7 @@ void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRe } } -void CWallet::blockConnected(const interfaces::BlockInfo& block) +void CWallet::blockConnected(ChainstateRole role, const interfaces::BlockInfo& block) { assert(block.data); LOCK(cs_wallet); @@ -2941,7 +2942,7 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri } if (chain) { - walletInstance->chainStateFlushed(chain->getTipLocator()); + walletInstance->chainStateFlushed(ChainstateRole::NORMAL, chain->getTipLocator()); } } else if (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS) { // Make it impossible to disable private keys after creation @@ -3227,7 +3228,7 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf } } walletInstance->m_attaching_chain = false; - walletInstance->chainStateFlushed(chain.getTipLocator()); + walletInstance->chainStateFlushed(ChainstateRole::NORMAL, chain.getTipLocator()); walletInstance->GetDatabase().IncrementUpdateCounter(); } walletInstance->m_attaching_chain = false; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 5adb8b6e270..9333493a6ee 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -599,7 +599,7 @@ 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) override; - void blockConnected(const interfaces::BlockInfo& block) override; + void blockConnected(ChainstateRole role, 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); @@ -777,7 +777,7 @@ public: /** should probably be renamed to IsRelevantToMe */ bool IsFromMe(const CTransaction& tx) const; CAmount GetDebit(const CTransaction& tx, const isminefilter& filter) const; - void chainStateFlushed(const CBlockLocator& loc) override; + void chainStateFlushed(ChainstateRole role, const CBlockLocator& loc) override; DBErrors LoadWallet(); DBErrors ZapSelectTx(std::vector& vHashIn, std::vector& vHashOut) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index 6755368249f..97355b45a7a 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -170,7 +171,7 @@ void CZMQNotificationInterface::TransactionRemovedFromMempool(const CTransaction }); } -void CZMQNotificationInterface::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) +void CZMQNotificationInterface::BlockConnected(ChainstateRole role, const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) { for (const CTransactionRef& ptx : pblock->vtx) { const CTransaction& tx = *ptx; diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index ce67633b30f..4246c53bd37 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -33,7 +33,7 @@ protected: // CValidationInterface void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) override; void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) override; - void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) override; + void BlockConnected(ChainstateRole role, const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) override; void BlockDisconnected(const std::shared_ptr& pblock, const CBlockIndex* pindexDisconnected) override; void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override;