diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -222,6 +222,9 @@ //! 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; @@ -260,7 +263,6 @@ virtual void BlockDisconnected(const CBlock &block) {} virtual void UpdatedBlockTip() {} virtual void ChainStateFlushed(const CBlockLocator &locator) {} - virtual void ResendWalletTransactions(Lock &locked_chain) {} }; //! Register handler for notifications. diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -210,15 +210,6 @@ void ChainStateFlushed(const CBlockLocator &locator) override { m_notifications->ChainStateFlushed(locator); } - void ResendWalletTransactions(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); - } Chain &m_chain; Chain::Notifications *m_notifications; }; @@ -355,6 +346,9 @@ Amount maxTxFee() override { return ::maxTxFee; } bool getPruneMode() override { return ::fPruneMode; } bool p2pEnabled() override { return g_connman != nullptr; } + bool isReadyToBroadcast() override { + return !::fImporting && !::fReindex && !IsInitialBlockDownload(); + } bool isInitialBlockDownload() override { return IsInitialBlockDownload(); } diff --git a/src/net_processing.cpp b/src/net_processing.cpp --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4299,13 +4299,6 @@ } } - // 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 && !IsInitialBlockDownload()) { - GetMainSignals().Broadcast(connman); - } - // // Try sending block announcements via headers // diff --git a/src/validationinterface.h b/src/validationinterface.h --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -141,8 +141,6 @@ * Called on a background thread. */ virtual void ChainStateFlushed(const CBlockLocator &locator) {} - /** Tells listeners to broadcast their data. */ - virtual void ResendWalletTransactions(CConnman *connman) {} /** * Notifies listeners of a block validation result. * If the provided CValidationState IsValid, the provided block @@ -205,7 +203,6 @@ const std::shared_ptr> &); void BlockDisconnected(const std::shared_ptr &); void ChainStateFlushed(const CBlockLocator &); - void Broadcast(CConnman *connman); void BlockChecked(const CBlock &, const CValidationState &); void NewPoWValidBlock(const CBlockIndex *, const std::shared_ptr &); diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -23,7 +23,6 @@ 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; }; @@ -43,7 +42,6 @@ boost::signals2::signal TransactionRemovedFromMempool; boost::signals2::signal ChainStateFlushed; - boost::signals2::signal Broadcast; boost::signals2::signal BlockChecked; boost::signals2::signalChainStateFlushed.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)); conns.BlockChecked = g_signals.m_internals->BlockChecked.connect( std::bind(&CValidationInterface::BlockChecked, pwalletIn, std::placeholders::_1, std::placeholders::_2)); @@ -214,10 +209,6 @@ [locator, this] { m_internals->ChainStateFlushed(locator); }); } -void CMainSignals::Broadcast(CConnman *connman) { - m_internals->Broadcast(connman); -} - void CMainSignals::BlockChecked(const CBlock &block, const CValidationState &state) { m_internals->BlockChecked(block, state); diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -100,13 +100,19 @@ pwallet->postInitProcess(); } - // Run a thread to flush wallet periodically + // Schedule periodic wallet flushes and tx rebroadcasts scheduler.scheduleEvery( [] { MaybeCompactWalletDB(); return true; }, 500); + scheduler.scheduleEvery( + [] { + MaybeResendWalletTxs(); + return true; + }, + 1000); } void FlushWallets() { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1063,8 +1063,7 @@ bool fUpdate); void TransactionRemovedFromMempool(const CTransactionRef &ptx) override; void ReacceptWalletTransactions(); - void - ResendWalletTransactions(interfaces::Chain::Lock &locked_chain) override; + void ResendWalletTransactions(interfaces::Chain::Lock &locked_chain); Amount GetBalance(const isminefilter &filter = ISMINE_SPENDABLE, const int min_depth = 0) const; Amount GetUnconfirmedBalance() const; @@ -1423,6 +1422,13 @@ friend struct WalletTestingSetup; }; +/** + * 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 { protected: diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2248,7 +2248,22 @@ return CTransaction(tx1) == CTransaction(tx2); } +// 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(interfaces::Chain::Lock &locked_chain) { + // 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) { @@ -2294,6 +2309,13 @@ /** @} */ // end of mapWallet +void MaybeResendWalletTxs() { + for (const std::shared_ptr &pwallet : GetWallets()) { + auto locked_chain = pwallet->chain().lock(); + pwallet->ResendWalletTransactions(*locked_chain); + } +} + /** * @defgroup Actions * diff --git a/test/functional/wallet_resendwallettransactions.py b/test/functional/wallet_resendwallettransactions.py --- a/test/functional/wallet_resendwallettransactions.py +++ b/test/functional/wallet_resendwallettransactions.py @@ -41,6 +41,12 @@ 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 wait_until( lambda: node.p2p.tx_invs_received[txid] >= 1,