From 91868e6288abf9d133620b585bc6de793a11e0e3 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Sun, 30 Jul 2017 16:00:56 -0400 Subject: Remove use CValidationInterface in wallet code This commit does not change behavior. --- src/interfaces/chain.cpp | 56 +++++++++++++++++++++++++++++++++ src/interfaces/chain.h | 26 +++++++++++++++ src/wallet/test/wallet_test_fixture.cpp | 7 +---- src/wallet/test/wallet_test_fixture.h | 1 - src/wallet/wallet.cpp | 28 ++++++++--------- src/wallet/wallet.h | 14 ++++++--- 6 files changed, 107 insertions(+), 25 deletions(-) (limited to 'src') diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 2eecea28d0..8ce718cc50 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -22,6 +23,7 @@ #include #include #include +#include #include #include @@ -161,6 +163,55 @@ class LockingStateImpl : public LockImpl, public UniqueLock using UniqueLock::UniqueLock; }; +class NotificationsHandlerImpl : public Handler, CValidationInterface +{ +public: + explicit NotificationsHandlerImpl(Chain& chain, Chain::Notifications& notifications) + : m_chain(chain), m_notifications(¬ifications) + { + RegisterValidationInterface(this); + } + ~NotificationsHandlerImpl() override { disconnect(); } + void disconnect() override + { + if (m_notifications) { + m_notifications = nullptr; + UnregisterValidationInterface(this); + } + } + void TransactionAddedToMempool(const CTransactionRef& tx) override + { + m_notifications->TransactionAddedToMempool(tx); + } + void TransactionRemovedFromMempool(const CTransactionRef& tx) override + { + m_notifications->TransactionRemovedFromMempool(tx); + } + void BlockConnected(const std::shared_ptr& block, + const CBlockIndex* index, + const std::vector& tx_conflicted) override + { + m_notifications->BlockConnected(*block, tx_conflicted); + } + void BlockDisconnected(const std::shared_ptr& block) override + { + m_notifications->BlockDisconnected(*block); + } + void ChainStateFlushed(const CBlockLocator& locator) override { m_notifications->ChainStateFlushed(locator); } + void ResendWalletTransactions(int64_t best_block_time, CConnman*) override + { + // `cs_main` is always held when this method is called, so it is safe to + // call `assumeLocked`. This is awkward, and the `assumeLocked` method + // should be able to be removed entirely if `ResendWalletTransactions` + // is replaced by a wallet timer as suggested in + // https://github.com/bitcoin/bitcoin/issues/15619 + auto locked_chain = m_chain.assumeLocked(); + m_notifications->ResendWalletTransactions(*locked_chain, best_block_time); + } + Chain& m_chain; + Chain::Notifications* m_notifications; +}; + class ChainImpl : public Chain { public: @@ -254,6 +305,11 @@ public: void initWarning(const std::string& message) override { InitWarning(message); } void initError(const std::string& message) override { InitError(message); } void loadWallet(std::unique_ptr wallet) override { ::uiInterface.LoadWallet(wallet); } + std::unique_ptr handleNotifications(Notifications& notifications) override + { + return MakeUnique(*this, notifications); + } + void waitForNotifications() override { SyncWithValidationInterfaceQueue(); } }; } // namespace diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index 037e8e9ff5..72862617b3 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -25,6 +25,7 @@ struct FeeCalculation; namespace interfaces { +class Handler; class Wallet; //! Interface giving clients (wallet processes, maybe other analysis tools in @@ -40,6 +41,12 @@ class Wallet; //! asynchronously //! (https://github.com/bitcoin/bitcoin/pull/10973#issuecomment-380101269). //! +//! * The isPotentialTip() and waitForNotifications() methods are too low-level +//! and should be replaced with a higher level +//! waitForNotificationsUpTo(block_hash) method that the wallet can call +//! instead +//! (https://github.com/bitcoin/bitcoin/pull/10973#discussion_r266995234). +//! //! * The relayTransactions() and submitToMemoryPool() methods could be replaced //! with a higher-level broadcastTransaction method //! (https://github.com/bitcoin/bitcoin/pull/14978#issuecomment-459373984). @@ -217,6 +224,25 @@ public: //! Send wallet load notification to the GUI. virtual void loadWallet(std::unique_ptr wallet) = 0; + + //! Chain notifications. + class Notifications + { + public: + virtual ~Notifications() {} + virtual void TransactionAddedToMempool(const CTransactionRef& tx) {} + virtual void TransactionRemovedFromMempool(const CTransactionRef& ptx) {} + virtual void BlockConnected(const CBlock& block, const std::vector& tx_conflicted) {} + virtual void BlockDisconnected(const CBlock& block) {} + virtual void ChainStateFlushed(const CBlockLocator& locator) {} + virtual void ResendWalletTransactions(Lock& locked_chain, int64_t best_block_time) {} + }; + + //! Register handler for notifications. + virtual std::unique_ptr handleNotifications(Notifications& notifications) = 0; + + //! Wait for pending notifications to be handled. + virtual void waitForNotifications() = 0; }; //! Interface to let node manage chain clients (wallets, or maybe tools for diff --git a/src/wallet/test/wallet_test_fixture.cpp b/src/wallet/test/wallet_test_fixture.cpp index a5fb1db86c..6a9922b3cd 100644 --- a/src/wallet/test/wallet_test_fixture.cpp +++ b/src/wallet/test/wallet_test_fixture.cpp @@ -13,12 +13,7 @@ WalletTestingSetup::WalletTestingSetup(const std::string& chainName): { bool fFirstRun; m_wallet.LoadWallet(fFirstRun); - RegisterValidationInterface(&m_wallet); + m_wallet.m_chain_notifications_handler = m_chain->handleNotifications(m_wallet); RegisterWalletRPCCommands(tableRPC); } - -WalletTestingSetup::~WalletTestingSetup() -{ - UnregisterValidationInterface(&m_wallet); -} diff --git a/src/wallet/test/wallet_test_fixture.h b/src/wallet/test/wallet_test_fixture.h index e6fe8c9473..edc06a8351 100644 --- a/src/wallet/test/wallet_test_fixture.h +++ b/src/wallet/test/wallet_test_fixture.h @@ -17,7 +17,6 @@ */ struct WalletTestingSetup: public TestingSetup { explicit WalletTestingSetup(const std::string& chainName = CBaseChainParams::MAIN); - ~WalletTestingSetup(); std::unique_ptr m_chain = interfaces::MakeChain(); CWallet m_wallet; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 02f3a265d9..f6acec2d3e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -96,7 +96,7 @@ static void ReleaseWallet(CWallet* wallet) wallet->WalletLogPrintf("Releasing wallet\n"); wallet->BlockUntilSyncedToCurrentChain(); wallet->Flush(); - UnregisterValidationInterface(wallet); + wallet->m_chain_notifications_handler.reset(); delete wallet; // Wallet is now released, notify UnloadWallet, if any. { @@ -1243,7 +1243,8 @@ void CWallet::TransactionRemovedFromMempool(const CTransactionRef &ptx) { } } -void CWallet::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) { +void CWallet::BlockConnected(const CBlock& block, const std::vector& vtxConflicted) { + const uint256& block_hash = block.GetHash(); auto locked_chain = chain().lock(); LOCK(cs_wallet); // TODO: Temporarily ensure that mempool removals are notified before @@ -1258,19 +1259,19 @@ void CWallet::BlockConnected(const std::shared_ptr& pblock, const SyncTransaction(ptx, {} /* block hash */, 0 /* position in block */); TransactionRemovedFromMempool(ptx); } - for (size_t i = 0; i < pblock->vtx.size(); i++) { - SyncTransaction(pblock->vtx[i], pindex->GetBlockHash(), i); - TransactionRemovedFromMempool(pblock->vtx[i]); + for (size_t i = 0; i < block.vtx.size(); i++) { + SyncTransaction(block.vtx[i], block_hash, i); + TransactionRemovedFromMempool(block.vtx[i]); } - m_last_block_processed = pindex->GetBlockHash(); + m_last_block_processed = block_hash; } -void CWallet::BlockDisconnected(const std::shared_ptr& pblock) { +void CWallet::BlockDisconnected(const CBlock& block) { auto locked_chain = chain().lock(); LOCK(cs_wallet); - for (const CTransactionRef& ptx : pblock->vtx) { + for (const CTransactionRef& ptx : block.vtx) { SyncTransaction(ptx, {} /* block hash */, 0 /* position in block */); } } @@ -1297,7 +1298,7 @@ void CWallet::BlockUntilSyncedToCurrentChain() { // ...otherwise put a callback in the validation interface queue and wait // for the queue to drain enough to execute it (indicating we are caught up // at least with the time we entered this function). - SyncWithValidationInterfaceQueue(); + chain().waitForNotifications(); } @@ -2137,7 +2138,7 @@ std::vector CWallet::ResendWalletTransactionsBefore(interfaces::Chain:: return result; } -void CWallet::ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman) +void CWallet::ResendWalletTransactions(interfaces::Chain::Lock& locked_chain, int64_t nBestBlockTime) { // Do this infrequently and randomly to avoid giving away // that these are our transactions. @@ -2155,8 +2156,7 @@ void CWallet::ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman // Rebroadcast unconfirmed txes older than 5 minutes before the last // block was found: - auto locked_chain = chain().assumeLocked(); // Temporary. Removed in upcoming lock cleanup - std::vector relayed = ResendWalletTransactionsBefore(*locked_chain, nBestBlockTime-5*60); + std::vector relayed = ResendWalletTransactionsBefore(locked_chain, nBestBlockTime-5*60); if (!relayed.empty()) WalletLogPrintf("%s: rebroadcast %u unconfirmed transactions\n", __func__, relayed.size()); } @@ -4385,8 +4385,8 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, chain.loadWallet(interfaces::MakeWallet(walletInstance)); - // Register with the validation interface. It's ok to do this after rescan since we're still holding cs_main. - RegisterValidationInterface(walletInstance.get()); + // Register with the validation interface. It's ok to do this after rescan since we're still holding locked_chain. + walletInstance->m_chain_notifications_handler = chain.handleNotifications(*walletInstance); walletInstance->SetBroadcastTransactions(gArgs.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST)); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 51e3edac34..02f1788ddb 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -637,7 +638,7 @@ class WalletRescanReserver; //forward declarations for ScanForWalletTransactions * A CWallet is an extension of a keystore, which also maintains a set of transactions and balances, * and provides the ability to create new transactions. */ -class CWallet final : public CCryptoKeyStore, public CValidationInterface +class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notifications { private: std::atomic fAbortRescan{false}; @@ -808,6 +809,9 @@ public: std::set setLockedCoins GUARDED_BY(cs_wallet); + /** Registered interfaces::Chain::Notifications handler. */ + std::unique_ptr m_chain_notifications_handler; + /** Interface for accessing chain state. */ interfaces::Chain& chain() const { return m_chain; } @@ -920,8 +924,8 @@ public: bool AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose=true); void LoadToWallet(const CWalletTx& wtxIn) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void TransactionAddedToMempool(const CTransactionRef& tx) override; - void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) override; - void BlockDisconnected(const std::shared_ptr& pblock) override; + void BlockConnected(const CBlock& block, const std::vector& vtxConflicted) override; + void BlockDisconnected(const CBlock& block) override; int64_t RescanFromTime(int64_t startTime, const WalletRescanReserver& reserver, bool update); struct ScanResult { @@ -942,7 +946,7 @@ public: ScanResult ScanForWalletTransactions(const uint256& first_block, const uint256& last_block, const WalletRescanReserver& reserver, bool fUpdate); void TransactionRemovedFromMempool(const CTransactionRef &ptx) override; void ReacceptWalletTransactions(); - void ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman) override EXCLUSIVE_LOCKS_REQUIRED(cs_main); + void ResendWalletTransactions(interfaces::Chain::Lock& locked_chain, int64_t nBestBlockTime) override; // ResendWalletTransactionsBefore may only be called if fBroadcastTransactions! std::vector ResendWalletTransactionsBefore(interfaces::Chain::Lock& locked_chain, int64_t nTime); CAmount GetBalance(const isminefilter& filter=ISMINE_SPENDABLE, const int min_depth=0) const; @@ -1220,6 +1224,8 @@ public: /** Add a KeyOriginInfo to the wallet */ bool AddKeyOrigin(const CPubKey& pubkey, const KeyOriginInfo& info); + + friend struct WalletTestingSetup; }; /** A key allocated from the key pool. */ -- cgit v1.2.3