From 52b760fc6a9b26e405a0553ee8285b0f03ca1604 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 20 Mar 2019 17:46:38 -0400 Subject: [PATCH] [wallet] Schedule tx rebroadcasts in wallet Removes the now-unused Broadcast/ResendWalletTransactions interface from validationinterface. The wallet_resendwallettransactions.py needs a sleep added at the start to make sure that the rebroadcast scheduler is warmed up before the next block is mined. --- src/interfaces/chain.cpp | 11 +--------- src/interfaces/chain.h | 4 +++- src/net_processing.cpp | 8 ------- src/validationinterface.cpp | 7 ------- src/validationinterface.h | 3 --- src/wallet/init.cpp | 3 ++- src/wallet/wallet.cpp | 21 ++++++++++++++++++- src/wallet/wallet.h | 8 ++++++- .../wallet_resendwallettransactions.py | 6 ++++++ 9 files changed, 39 insertions(+), 32 deletions(-) diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 6c918b73f..f278e5de9 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -207,16 +207,6 @@ public: m_notifications->UpdatedBlockTip(); } 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; }; @@ -351,6 +341,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 && !IsInitialBlockDownload(); } bool isInitialBlockDownload() override { return 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 82e2c7eb2..4e0ef49f6 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; @@ -258,7 +261,6 @@ public: 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/net_processing.cpp b/src/net_processing.cpp index 3fd3068fb..044fcc90c 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3546,14 +3546,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 && !IsInitialBlockDownload()) - { - GetMainSignals().Broadcast(connman); - } - // // Try sending block announcements via headers // diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index abd229d56..5d0ee1d1f 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; }; @@ -37,7 +36,6 @@ struct MainSignalsInstance { boost::signals2::signal &)> BlockDisconnected; boost::signals2::signal TransactionRemovedFromMempool; boost::signals2::signal ChainStateFlushed; - boost::signals2::signal Broadcast; boost::signals2::signal BlockChecked; boost::signals2::signal&)> NewPoWValidBlock; @@ -101,7 +99,6 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) { conns.BlockDisconnected = g_signals.m_internals->BlockDisconnected.connect(std::bind(&CValidationInterface::BlockDisconnected, pwalletIn, std::placeholders::_1)); conns.TransactionRemovedFromMempool = g_signals.m_internals->TransactionRemovedFromMempool.connect(std::bind(&CValidationInterface::TransactionRemovedFromMempool, pwalletIn, std::placeholders::_1)); 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)); 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)); } @@ -175,10 +172,6 @@ void CMainSignals::ChainStateFlushed(const CBlockLocator &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/validationinterface.h b/src/validationinterface.h index 0d5bdd60f..78f3026a8 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -134,8 +134,6 @@ protected: * 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 @@ -184,7 +182,6 @@ public: void BlockConnected(const std::shared_ptr &, const CBlockIndex *pindex, 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/wallet/init.cpp b/src/wallet/init.cpp index 76a7eaa68..a8096dc67 100644 --- a/src/wallet/init.cpp +++ b/src/wallet/init.cpp @@ -211,8 +211,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); } void FlushWallets() diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 243b51a1e..cf477982c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2114,8 +2114,21 @@ bool CWalletTx::IsEquivalentTo(const CWalletTx& _tx) const 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) return; @@ -2149,7 +2162,13 @@ void CWallet::ResendWalletTransactions(interfaces::Chain::Lock& locked_chain) /** @} */ // 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/src/wallet/wallet.h b/src/wallet/wallet.h index cef58c641..a0c5f63ce 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -949,7 +949,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(interfaces::Chain::Lock& locked_chain) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - void ResendWalletTransactions(interfaces::Chain::Lock& locked_chain) override; + void ResendWalletTransactions(interfaces::Chain::Lock& locked_chain); 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) @@ -1232,6 +1232,12 @@ public: 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 { diff --git a/test/functional/wallet_resendwallettransactions.py b/test/functional/wallet_resendwallettransactions.py index 8aafa94c2..ec5e230e5 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 wait_until(lambda: node.p2p.tx_invs_received[txid] >= 1, lock=mininode_lock)