From ed48a889bf380e97ff8d18788941e0bd749ef341 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com> Date: Sun, 12 Dec 2021 21:27:51 +0530 Subject: [PATCH] merge bitcoin#15632: Remove ResendWalletTransactions from the Validation Interface --- src/interfaces/chain.cpp | 13 ++----- src/interfaces/chain.h | 5 ++- src/net_processing.cpp | 19 ---------- src/validationinterface.cpp | 7 ---- src/validationinterface.h | 3 -- src/wallet/load.cpp | 3 +- src/wallet/wallet.cpp | 37 +++++++++++++++---- src/wallet/wallet.h | 11 +++++- .../wallet_resendwallettransactions.py | 6 +++ 9 files changed, 56 insertions(+), 48 deletions(-) diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 056ee0d3ba..53e7346cb0 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -217,17 +217,11 @@ public: { m_notifications->BlockDisconnected(*block); } - void ChainStateFlushed(const CBlockLocator& locator) override { m_notifications->ChainStateFlushed(locator); } - void ResendWalletTransactions(int64_t best_block_time, CConnman*) override + void UpdatedBlockTip(const CBlockIndex* index, const CBlockIndex* fork_index, bool is_ibd) 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); + m_notifications->UpdatedBlockTip(); } + void ChainStateFlushed(const CBlockLocator& locator) override { m_notifications->ChainStateFlushed(locator); } void NotifyChainLock(const CBlockIndex* pindexChainLock, const std::shared_ptr& clsig) override { m_notifications->NotifyChainLock(pindexChainLock, clsig); @@ -365,6 +359,7 @@ public: CAmount maxTxFee() override { return ::maxTxFee; } bool getPruneMode() override { return ::fPruneMode; } bool p2pEnabled() override { return g_connman != nullptr; } + bool isReadyToBroadcast() override { return !::fImporting && !::fReindex && !::ChainstateActive().IsInitialBlockDownload(); } bool isInitialBlockDownload() override { return ::ChainstateActive().IsInitialBlockDownload(); } bool shutdownRequested() override { return ShutdownRequested(); } int64_t getAdjustedTime() override { return GetAdjustedTime(); } diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index aa4fe8afd9..72b431be68 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -223,6 +223,9 @@ public: //! Check if p2p enabled. virtual bool p2pEnabled() = 0; + //! Check if the node is ready to broadcast transactions. + virtual bool isReadyToBroadcast() = 0; + //! Check if in IBD. virtual bool isInitialBlockDownload() = 0; @@ -256,8 +259,8 @@ public: virtual void TransactionRemovedFromMempool(const CTransactionRef& ptx, MemPoolRemovalReason reason) {} virtual void BlockConnected(const CBlock& block, const std::vector& tx_conflicted) {} virtual void BlockDisconnected(const CBlock& block) {} + virtual void UpdatedBlockTip() {} virtual void ChainStateFlushed(const CBlockLocator& locator) {} - virtual void ResendWalletTransactions(Lock& locked_chain, int64_t best_block_time) {} virtual void NotifyChainLock(const CBlockIndex* pindexChainLock, const std::shared_ptr& clsig) {} virtual void NotifyTransactionLock(const CTransactionRef &tx, const std::shared_ptr& islock) {} }; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e0d03c21c0..fe89b16c23 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -237,8 +237,6 @@ namespace { /** Expiration-time ordered list of (expire time, relay map entry) pairs. */ std::deque> vRelayExpiration GUARDED_BY(cs_main); - std::atomic nTimeBestReceived(0); // Used only to inform the wallet of when we last received a block - struct IteratorComparator { template @@ -1314,8 +1312,6 @@ void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CB }); connman->WakeMessageHandler(); } - - nTimeBestReceived = GetTime(); } /** @@ -4381,21 +4377,6 @@ bool PeerLogicValidation::SendMessages(CNode* pto) } } - // Resend wallet transactions that haven't gotten in a block yet - // Except during reindex, importing and IBD, when old wallet - // transactions become unconfirmed and spams other nodes. - if (!fReindex && !fImporting && !::ChainstateActive().IsInitialBlockDownload()) - { - static int64_t nLastBroadcastTime = 0; - // HACK: Call this only once every few seconds. SendMessages is called once per peer, which makes this signal very expensive - // The proper solution would be to move this out of here, but this is not worth the effort right now as bitcoin#15632 will later do this. - // Luckily, the Broadcast signal is not used for anything else then CWallet::ResendWalletTransactionsBefore. - if (nNow - nLastBroadcastTime >= 5000000) { - GetMainSignals().Broadcast(nTimeBestReceived, connman); - nLastBroadcastTime = nNow; - } - } - // // Try sending block announcements via headers // diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 2340c468b1..9e1584083e 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -25,7 +25,6 @@ struct ValidationInterfaceConnections { boost::signals2::scoped_connection BlockDisconnected; boost::signals2::scoped_connection TransactionRemovedFromMempool; boost::signals2::scoped_connection ChainStateFlushed; - boost::signals2::scoped_connection Broadcast; boost::signals2::scoped_connection BlockChecked; boost::signals2::scoped_connection NewPoWValidBlock; boost::signals2::scoped_connection AcceptedBlockHeader; @@ -48,7 +47,6 @@ struct MainSignalsInstance { boost::signals2::signal &, const CBlockIndex* pindexDisconnected)> BlockDisconnected; boost::signals2::signal TransactionRemovedFromMempool; boost::signals2::signal ChainStateFlushed; - boost::signals2::signal Broadcast; boost::signals2::signal BlockChecked; boost::signals2::signal&)> NewPoWValidBlock; boost::signals2::signalAcceptedBlockHeader; @@ -122,7 +120,6 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) { conns.NotifyChainLock = g_signals.m_internals->NotifyChainLock.connect(std::bind(&CValidationInterface::NotifyChainLock, pwalletIn, std::placeholders::_1, std::placeholders::_2)); conns.TransactionRemovedFromMempool = g_signals.m_internals->TransactionRemovedFromMempool.connect(std::bind(&CValidationInterface::TransactionRemovedFromMempool, pwalletIn, std::placeholders::_1, std::placeholders::_2)); conns.ChainStateFlushed = g_signals.m_internals->ChainStateFlushed.connect(std::bind(&CValidationInterface::ChainStateFlushed, pwalletIn, std::placeholders::_1)); - conns.Broadcast = g_signals.m_internals->Broadcast.connect(std::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, std::placeholders::_1, std::placeholders::_2)); conns.BlockChecked = g_signals.m_internals->BlockChecked.connect(std::bind(&CValidationInterface::BlockChecked, pwalletIn, std::placeholders::_1, std::placeholders::_2)); conns.NewPoWValidBlock = g_signals.m_internals->NewPoWValidBlock.connect(std::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, std::placeholders::_1, std::placeholders::_2)); conns.NotifyGovernanceObject = g_signals.m_internals->NotifyGovernanceObject.connect(std::bind(&CValidationInterface::NotifyGovernanceObject, pwalletIn, std::placeholders::_1)); @@ -205,10 +202,6 @@ void CMainSignals::ChainStateFlushed(const CBlockLocator &locator) { }); } -void CMainSignals::Broadcast(int64_t nBestBlockTime, CConnman* connman) { - m_internals->Broadcast(nBestBlockTime, connman); -} - void CMainSignals::BlockChecked(const CBlock& block, const CValidationState& state) { m_internals->BlockChecked(block, state); } diff --git a/src/validationinterface.h b/src/validationinterface.h index 451aaf61bc..435260d308 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -156,8 +156,6 @@ protected: * Called on a background thread. */ virtual void ChainStateFlushed(const CBlockLocator &locator) {} - /** Tells listeners to broadcast their data. */ - virtual void ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman) {} /** * Notifies listeners of a block validation result. * If the provided CValidationState IsValid, the provided block @@ -216,7 +214,6 @@ public: void NotifyRecoveredSig(const std::shared_ptr &sig); void NotifyMasternodeListChanged(bool undo, const CDeterministicMNList& oldMNList, const CDeterministicMNListDiff& diff); void ChainStateFlushed(const CBlockLocator &); - void Broadcast(int64_t nBestBlockTime, CConnman* connman); void BlockChecked(const CBlock&, const CValidationState&); void NewPoWValidBlock(const CBlockIndex *, const std::shared_ptr&); }; diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index a7d5ad82ab..4b3f465f52 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -83,8 +83,9 @@ void StartWallets(CScheduler& scheduler) pwallet->postInitProcess(); } - // Run a thread to flush wallet periodically + // Schedule periodic wallet flushes and tx rebroadcasts scheduler.scheduleEvery(MaybeCompactWalletDB, 500); + scheduler.scheduleEvery(MaybeResendWalletTxs, 1000); if (!fMasternodeMode && CCoinJoinClientOptions::IsEnabled()) { scheduler.scheduleEvery(std::bind(&DoCoinJoinMaintenance, std::ref(*g_connman)), 1 * 1000); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c024c04614..a219acf0a4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1473,6 +1473,10 @@ void CWallet::BlockDisconnected(const CBlock& block) { fAnonymizableTallyCachedNonDenom = false; } +void CWallet::UpdatedBlockTip() +{ + m_best_block_time = GetTime(); +} void CWallet::BlockUntilSyncedToCurrentChain() { @@ -2598,8 +2602,21 @@ bool CWalletTx::IsEquivalentTo(const CWalletTx& _tx) const return CTransaction(tx1) == CTransaction(tx2); } -void CWallet::ResendWalletTransactions(interfaces::Chain::Lock& locked_chain, int64_t nBestBlockTime) +// Rebroadcast transactions from the wallet. We do this on a random timer +// to slightly obfuscate which transactions come from our wallet. +// +// Ideally, we'd only resend transactions that we think should have been +// mined in the most recent block. Any transaction that wasn't in the top +// blockweight of transactions in the mempool shouldn't have been mined, +// and so is probably just sitting in the mempool waiting to be confirmed. +// Rebroadcasting does nothing to speed up confirmation and only damages +// privacy. +void CWallet::ResendWalletTransactions() { + // During reindex, importing and IBD, old wallet transactions become + // unconfirmed. Don't resend them as that would spam other nodes. + if (!chain().isReadyToBroadcast()) return; + // Do this infrequently and randomly to avoid giving away // that these are our transactions. if (GetTime() < nNextResend || !fBroadcastTransactions) return; @@ -2608,12 +2625,13 @@ void CWallet::ResendWalletTransactions(interfaces::Chain::Lock& locked_chain, in if (fFirst) return; // Only do it if there's been a new block since last time - if (nBestBlockTime < nLastResend) return; + if (m_best_block_time < nLastResend) return; nLastResend = GetTime(); int relayed_tx_count = 0; - { // mempool.cs and cs_wallet scope + { // locked_chain, mempool.cs and cs_wallet scope + auto locked_chain = chain().lock(); LOCK2(mempool.cs, cs_wallet); // Relay transactions @@ -2621,10 +2639,10 @@ void CWallet::ResendWalletTransactions(interfaces::Chain::Lock& locked_chain, in CWalletTx& wtx = item.second; // only rebroadcast unconfirmed txes older than 5 minutes before the // last block was found - if (wtx.nTimeReceived > nBestBlockTime - 5 * 60) continue; - relayed_tx_count += wtx.RelayWalletTransaction(*locked_chain) ? 1 : 0; + if (wtx.nTimeReceived > m_best_block_time - 5 * 60) continue; + if (wtx.RelayWalletTransaction(*locked_chain)) ++relayed_tx_count; } - } // mempool.cs and cs_wallet + } // locked_chain, mempool.cs and cs_wallet if (relayed_tx_count > 0) { WalletLogPrintf("%s: rebroadcast %u unconfirmed transactions\n", __func__, relayed_tx_count); @@ -2633,7 +2651,12 @@ void CWallet::ResendWalletTransactions(interfaces::Chain::Lock& locked_chain, in /** @} */ // end of mapWallet - +void MaybeResendWalletTxs() +{ + for (const std::shared_ptr& pwallet : GetWallets()) { + pwallet->ResendWalletTransactions(); + } +} /** @defgroup Actions diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 5011195914..e17274c504 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -657,6 +657,8 @@ private: int64_t nNextResend = 0; int64_t nLastResend = 0; bool fBroadcastTransactions = false; + // Local time that the tip block was received. Used to schedule wallet rebroadcasts. + std::atomic m_best_block_time {0}; mutable bool fAnonymizableTallyCached = false; mutable std::vector vecAnonymizableTallyCached; @@ -983,6 +985,7 @@ public: void TransactionAddedToMempool(const CTransactionRef& tx, int64_t nAcceptTime) override; void BlockConnected(const CBlock& block, const std::vector& vtxConflicted) override; void BlockDisconnected(const CBlock& block) override; + void UpdatedBlockTip() override; int64_t RescanFromTime(int64_t startTime, const WalletRescanReserver& reserver, bool update); struct ScanResult { @@ -1003,7 +1006,7 @@ public: ScanResult ScanForWalletTransactions(const uint256& first_block, const uint256& last_block, const WalletRescanReserver& reserver, bool fUpdate); void TransactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) override; void ReacceptWalletTransactions(); - void ResendWalletTransactions(interfaces::Chain::Lock& locked_chain, int64_t nBestBlockTime) override; + void ResendWalletTransactions(); struct Balance { CAmount m_mine_trusted{0}; //!< Trusted, at depth=GetBalance.min_depth or more CAmount m_mine_untrusted_pending{0}; //!< Untrusted, but in mempool (pending) @@ -1282,6 +1285,12 @@ public: bool GetKeyOrigin(const CKeyID& keyid, KeyOriginInfo& info) const override; }; +/** + * Called periodically by the schedule thread. Prompts individual wallets to resend + * their transactions. Actual rebroadcast schedule is managed by the wallets themselves. + */ +void MaybeResendWalletTxs(); + /** A key allocated from the key pool. */ class CReserveKey final : public CReserveScript { diff --git a/test/functional/wallet_resendwallettransactions.py b/test/functional/wallet_resendwallettransactions.py index d8f16eaba2..d4c50f8e9c 100755 --- a/test/functional/wallet_resendwallettransactions.py +++ b/test/functional/wallet_resendwallettransactions.py @@ -39,6 +39,12 @@ class ResendWalletTransactionsTest(BitcoinTestFramework): self.log.info("Create a new transaction and wait until it's broadcast") txid = int(node.sendtoaddress(node.getnewaddress(), 1), 16) + # Wallet rebroadcast is first scheduled 1 sec after startup (see + # nNextResend in ResendWalletTransactions()). Sleep for just over a + # second to be certain that it has been called before the first + # setmocktime call below. + time.sleep(1.1) + # Can take a few seconds due to transaction trickling def wait_p2p(): self.bump_mocktime(1)