From 9a84f7ef76dc01f6de15c1e64ba5ccac01d022dd Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Tue, 31 Mar 2020 13:48:52 +0200 Subject: [PATCH 01/10] Merge #18160: gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37 gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged (João Barbosa) Pull request description: Each 250ms the slot `WalletModel::pollBalanceChanged` is called which, at worst case, calls `Wallet::GetBalance`. This is a waste of resources since most of the time there aren't new transactions or new blocks. Fix this by early checking if cache is dirty or not. The actual balance computation can still hang the GUI thread but that is tracked in #16874 and should be fixed with a solution similar to #17135. ACKs for top commit: hebasto: ACK 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged. jonasschnelli: utACK 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37 instagibbs: ACK 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37 ryanofsky: Code review ACK 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37, but I would prefer (not strongly) for #17905 to be merged first. This PR can be simpler if it is based on #17905, so tryGetBalances can just be left alone instead of changing into to a more complicated tryGetBalancesIfNeeded function, and then getting changed back later when we want to optimize it out. jonatack: ACK 0933a37078e based primarily on code review, despite a lot of manual testing with a large 177MB wallet. Tree-SHA512: 18db35bf33a7577666658c8cb0b57308c8474baa5ea95bf1468cd8531a69857d8915584f6ac505874717aa6aabeb1b506ac77630f8acdb6651afab89275e38a1 --- src/interfaces/wallet.h | 7 +++++-- src/qt/walletmodel.cpp | 19 ++++++++----------- src/wallet/interfaces.cpp | 5 +++-- 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index eeef9a68dec2..8d922dc42c5a 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -229,8 +229,11 @@ class Wallet //! Get balances. virtual WalletBalances getBalances() = 0; - //! Get balances if possible without blocking. - virtual bool tryGetBalances(WalletBalances& balances, uint256& block_hash) = 0; + //! Get balances if possible without waiting for chain and wallet locks. + virtual bool tryGetBalances(WalletBalances& balances, + uint256& block_hash, + bool force, + const uint256& cached_last_update_tip) = 0; //! Get balance. virtual CAmount getBalance() = 0; diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index faadbdaaafcb..831fb68f87fd 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -101,22 +101,19 @@ void WalletModel::pollBalanceChanged() // rescan. interfaces::WalletBalances new_balances; uint256 block_hash; - if (!m_wallet->tryGetBalances(new_balances, block_hash)) { + if (!m_wallet->tryGetBalances(new_balances, block_hash, fForceCheckBalanceChanged, m_cached_last_update_tip)) { return; } - if (fForceCheckBalanceChanged || block_hash != m_cached_last_update_tip || node().coinJoinOptions().getRounds() != cachedCoinJoinRounds) - { - fForceCheckBalanceChanged = false; + fForceCheckBalanceChanged = false; - // Balance and number of transactions might have changed - m_cached_last_update_tip = block_hash; - cachedCoinJoinRounds = node().coinJoinOptions().getRounds(); + // Balance and number of transactions might have changed + m_cached_last_update_tip = block_hash; + cachedCoinJoinRounds = node().coinJoinOptions().getRounds(); - checkBalanceChanged(new_balances); - if(transactionTableModel) - transactionTableModel->updateConfirmations(); - } + checkBalanceChanged(new_balances); + if(transactionTableModel) + transactionTableModel->updateConfirmations(); } void WalletModel::checkBalanceChanged(const interfaces::WalletBalances& new_balances) diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 200bf3e8316c..1fbb75ce7461 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -434,13 +434,14 @@ class WalletImpl : public Wallet } return result; } - bool tryGetBalances(WalletBalances& balances, uint256& block_hash) override + bool tryGetBalances(WalletBalances& balances, uint256& block_hash, bool force, const uint256& cached_last_update_tip) override { + block_hash = m_wallet->GetLastBlockHash(); + if (!force && block_hash == cached_last_update_tip) return false; TRY_LOCK(m_wallet->cs_wallet, locked_wallet); if (!locked_wallet) { return false; } - block_hash = m_wallet->GetLastBlockHash(); balances = getBalances(); return true; } From 4f00d45e531731f168cb834b5ede098f40c0e77e Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 6 Jul 2023 17:23:58 +0700 Subject: [PATCH 02/10] Merge #18587: gui: Avoid wallet tryGetBalances calls in WalletModel::pollBalanceChanged d3a56be77a9d112cde4baef4314882170b9f228f Revert "gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged" (Russell Yanofsky) bf0a510981ddc28c754881ca21c50ab18e5f2b59 gui: Avoid wallet tryGetBalances calls before TransactionChanged or BlockTip notifications (Russell Yanofsky) 2bc9b92ed8b7736ad67876398a0bb8287f57e9b3 Cancel wallet balance timer when shutdown requested (Russell Yanofsky) 83f69fab3a1ae97c5cff8ba1e6fd191b0fa264bb Switch transaction table to use wallet height not node height (Russell Yanofsky) Pull request description: Main commit `gui: Avoid wallet tryGetBalances calls` is one-line change to `WalletModel::pollBalanceChanged` that returns early if there hasn't been a new `TransactionChanged` or `BlockTip` notification since the previous poll call. This is the same behavior that was implemented in #18160, now implemented in a simpler way. The other commits are a straight revert of #18160, and two tweaks to avoid relying on `WalletModel::m_client_model` lifetime which were causing travis failures with earlier versions of this PR. Motivation for this change is to be able to revert #18160 and cut down on unnecessary cross-process calls that happen when #18160 is combined with #10102 This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).# This is a combination of 2 commits. --- src/interfaces/wallet.h | 7 ++----- src/qt/walletmodel.cpp | 19 +++++++++++-------- src/qt/walletmodel.h | 1 - src/wallet/interfaces.cpp | 5 ++--- 4 files changed, 15 insertions(+), 17 deletions(-) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 8d922dc42c5a..eeef9a68dec2 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -229,11 +229,8 @@ class Wallet //! Get balances. virtual WalletBalances getBalances() = 0; - //! Get balances if possible without waiting for chain and wallet locks. - virtual bool tryGetBalances(WalletBalances& balances, - uint256& block_hash, - bool force, - const uint256& cached_last_update_tip) = 0; + //! Get balances if possible without blocking. + virtual bool tryGetBalances(WalletBalances& balances, uint256& block_hash) = 0; //! Get balance. virtual CAmount getBalance() = 0; diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 831fb68f87fd..faadbdaaafcb 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -101,19 +101,22 @@ void WalletModel::pollBalanceChanged() // rescan. interfaces::WalletBalances new_balances; uint256 block_hash; - if (!m_wallet->tryGetBalances(new_balances, block_hash, fForceCheckBalanceChanged, m_cached_last_update_tip)) { + if (!m_wallet->tryGetBalances(new_balances, block_hash)) { return; } - fForceCheckBalanceChanged = false; + if (fForceCheckBalanceChanged || block_hash != m_cached_last_update_tip || node().coinJoinOptions().getRounds() != cachedCoinJoinRounds) + { + fForceCheckBalanceChanged = false; - // Balance and number of transactions might have changed - m_cached_last_update_tip = block_hash; - cachedCoinJoinRounds = node().coinJoinOptions().getRounds(); + // Balance and number of transactions might have changed + m_cached_last_update_tip = block_hash; + cachedCoinJoinRounds = node().coinJoinOptions().getRounds(); - checkBalanceChanged(new_balances); - if(transactionTableModel) - transactionTableModel->updateConfirmations(); + checkBalanceChanged(new_balances); + if(transactionTableModel) + transactionTableModel->updateConfirmations(); + } } void WalletModel::checkBalanceChanged(const interfaces::WalletBalances& new_balances) diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index f424053c0ae2..d27fb9bc205b 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -151,7 +151,6 @@ class WalletModel : public QObject interfaces::Node& node() const { return m_node; } interfaces::Wallet& wallet() const { return *m_wallet; } void setClientModel(ClientModel* client_model); - ClientModel& clientModel() const { return *m_client_model; } interfaces::CoinJoin::Client& coinJoin() const { return m_wallet->coinJoin(); } QString getWalletName() const; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 1fbb75ce7461..200bf3e8316c 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -434,14 +434,13 @@ class WalletImpl : public Wallet } return result; } - bool tryGetBalances(WalletBalances& balances, uint256& block_hash, bool force, const uint256& cached_last_update_tip) override + bool tryGetBalances(WalletBalances& balances, uint256& block_hash) override { - block_hash = m_wallet->GetLastBlockHash(); - if (!force && block_hash == cached_last_update_tip) return false; TRY_LOCK(m_wallet->cs_wallet, locked_wallet); if (!locked_wallet) { return false; } + block_hash = m_wallet->GetLastBlockHash(); balances = getBalances(); return true; } From d2bbf03c321483d468c5e027fd42b27949ec0272 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Mon, 6 Apr 2020 16:22:22 +0200 Subject: [PATCH 03/10] Merge #18524: refactor: drop boost::signals2 in validationinterface d6815a2313158862d448733954a73520f223deb6 refactor: drop boost::signals2 in validationinterface (Russell Yanofsky) Pull request description: Stop using boost::signals2 internally in validationinterface. Replace with std::list and Add/Remove/Clear/Iterate helper functions. Motivation for change is to reduce dependencies and avoid issues happening with boost versions before 1.59: https://github.com/bitcoin/bitcoin/issues/18517, https://github.com/bitcoin/bitcoin/pull/18471 ACKs for top commit: MarcoFalke: ACK d6815a2313158862d448733954a73520f223deb6 laanwj: ACK d6815a2313158862d448733954a73520f223deb6 hebasto: re-ACK d6815a2313158862d448733954a73520f223deb6 promag: ACK d6815a2313158862d448733954a73520f223deb6. Tree-SHA512: 4fc0f14a8446e8616cc142af6c3d36815f3254525d30348ba8e4d4bc74c249a5a8c9bc119bdd1be7ebd7abe0b784bc0c5551a3e156a766890cb2fdd891a95919 --- src/validationinterface.cpp | 165 ++++++++++++++++++------------------ src/validationinterface.h | 4 +- 2 files changed, 84 insertions(+), 85 deletions(-) diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 60c3603fda2f..577c9beecf57 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -21,56 +21,75 @@ #include #include -#include - -struct ValidationInterfaceConnections { - boost::signals2::scoped_connection UpdatedBlockTip; - boost::signals2::scoped_connection SynchronousUpdatedBlockTip; - boost::signals2::scoped_connection TransactionAddedToMempool; - boost::signals2::scoped_connection BlockConnected; - boost::signals2::scoped_connection BlockDisconnected; - boost::signals2::scoped_connection TransactionRemovedFromMempool; - boost::signals2::scoped_connection ChainStateFlushed; - boost::signals2::scoped_connection BlockChecked; - boost::signals2::scoped_connection NewPoWValidBlock; - boost::signals2::scoped_connection AcceptedBlockHeader; - boost::signals2::scoped_connection NotifyHeaderTip; - boost::signals2::scoped_connection NotifyTransactionLock; - boost::signals2::scoped_connection NotifyChainLock; - boost::signals2::scoped_connection NotifyGovernanceVote; - boost::signals2::scoped_connection NotifyGovernanceObject; - boost::signals2::scoped_connection NotifyInstantSendDoubleSpendAttempt; - boost::signals2::scoped_connection NotifyMasternodeListChanged; - boost::signals2::scoped_connection NotifyRecoveredSig; - -}; - +//! The MainSignalsInstance manages a list of shared_ptr +//! callbacks. +//! +//! A std::unordered_map is used to track what callbacks are currently +//! registered, and a std::list is to used to store the callbacks that are +//! currently registered as well as any callbacks that are just unregistered +//! and about to be deleted when they are done executing. struct MainSignalsInstance { - boost::signals2::signal UpdatedBlockTip; - boost::signals2::signal SynchronousUpdatedBlockTip; - boost::signals2::signal TransactionAddedToMempool; - boost::signals2::signal &, const CBlockIndex *pindex)> BlockConnected; - boost::signals2::signal&, const CBlockIndex* pindex)> BlockDisconnected; - boost::signals2::signal TransactionRemovedFromMempool; - boost::signals2::signal ChainStateFlushed; - boost::signals2::signal BlockChecked; - boost::signals2::signal&)> NewPoWValidBlock; - boost::signals2::signalAcceptedBlockHeader; - boost::signals2::signalNotifyHeaderTip; - boost::signals2::signal& islock)>NotifyTransactionLock; - boost::signals2::signal& clsig)>NotifyChainLock; - boost::signals2::signal& vote)>NotifyGovernanceVote; - boost::signals2::signal& object)>NotifyGovernanceObject; - boost::signals2::signalNotifyInstantSendDoubleSpendAttempt; - boost::signals2::signalNotifyMasternodeListChanged; - boost::signals2::signal& sig)>NotifyRecoveredSig; +private: + Mutex m_mutex; + //! List entries consist of a callback pointer and reference count. The + //! count is equal to the number of current executions of that entry, plus 1 + //! if it's registered. It cannot be 0 because that would imply it is + //! unregistered and also not being executed (so shouldn't exist). + struct ListEntry { std::shared_ptr callbacks; int count = 1; }; + std::list m_list GUARDED_BY(m_mutex); + std::unordered_map::iterator> m_map GUARDED_BY(m_mutex); + +public: // We are not allowed to assume the scheduler only runs in one thread, // but must ensure all callbacks happen in-order, so we end up creating // our own queue here :( SingleThreadedSchedulerClient m_schedulerClient; - std::unordered_map m_connMainSignals; explicit MainSignalsInstance(CScheduler *pscheduler) : m_schedulerClient(pscheduler) {} + + void Register(std::shared_ptr callbacks) + { + LOCK(m_mutex); + auto inserted = m_map.emplace(callbacks.get(), m_list.end()); + if (inserted.second) inserted.first->second = m_list.emplace(m_list.end()); + inserted.first->second->callbacks = std::move(callbacks); + } + + void Unregister(CValidationInterface* callbacks) + { + LOCK(m_mutex); + auto it = m_map.find(callbacks); + if (it != m_map.end()) { + if (!--it->second->count) m_list.erase(it->second); + m_map.erase(it); + } + } + + //! Clear unregisters every previously registered callback, erasing every + //! map entry. After this call, the list may still contain callbacks that + //! are currently executing, but it will be cleared when they are done + //! executing. + void Clear() + { + LOCK(m_mutex); + for (auto it = m_list.begin(); it != m_list.end();) { + it = --it->count ? std::next(it) : m_list.erase(it); + } + m_map.clear(); + } + + template void Iterate(F&& f) + { + WAIT_LOCK(m_mutex, lock); + for (auto it = m_list.begin(); it != m_list.end();) { + ++it->count; + { + REVERSE_LOCK(lock); + f(*it->callbacks); + } + it = --it->count ? std::next(it) : m_list.erase(it); + } + } }; static CMainSignals g_signals; @@ -103,25 +122,7 @@ CMainSignals& GetMainSignals() void RegisterSharedValidationInterface(std::shared_ptr pwalletIn) { // Each connection captures pwalletIn to ensure that each callback is // executed before pwalletIn is destroyed. For more details see #18338. - ValidationInterfaceConnections& conns = g_signals.m_internals->m_connMainSignals[pwalletIn.get()]; - conns.AcceptedBlockHeader = g_signals.m_internals->AcceptedBlockHeader.connect(std::bind(&CValidationInterface::AcceptedBlockHeader, pwalletIn, std::placeholders::_1)); - conns.NotifyHeaderTip = g_signals.m_internals->NotifyHeaderTip.connect(std::bind(&CValidationInterface::NotifyHeaderTip, pwalletIn, std::placeholders::_1, std::placeholders::_2)); - conns.UpdatedBlockTip = g_signals.m_internals->UpdatedBlockTip.connect(std::bind(&CValidationInterface::UpdatedBlockTip, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3)); - conns.SynchronousUpdatedBlockTip = g_signals.m_internals->SynchronousUpdatedBlockTip.connect(std::bind(&CValidationInterface::SynchronousUpdatedBlockTip, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3)); - conns.TransactionAddedToMempool = g_signals.m_internals->TransactionAddedToMempool.connect(std::bind(&CValidationInterface::TransactionAddedToMempool, pwalletIn, std::placeholders::_1, std::placeholders::_2)); - conns.BlockConnected = g_signals.m_internals->BlockConnected.connect(std::bind(&CValidationInterface::BlockConnected, pwalletIn, std::placeholders::_1, std::placeholders::_2)); - conns.BlockDisconnected = g_signals.m_internals->BlockDisconnected.connect(std::bind(&CValidationInterface::BlockDisconnected, pwalletIn, std::placeholders::_1, std::placeholders::_2)); - conns.NotifyTransactionLock = g_signals.m_internals->NotifyTransactionLock.connect(std::bind(&CValidationInterface::NotifyTransactionLock, pwalletIn, std::placeholders::_1, std::placeholders::_2)); - 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.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)); - conns.NotifyGovernanceVote = g_signals.m_internals->NotifyGovernanceVote.connect(std::bind(&CValidationInterface::NotifyGovernanceVote, pwalletIn, std::placeholders::_1)); - conns.NotifyInstantSendDoubleSpendAttempt = g_signals.m_internals->NotifyInstantSendDoubleSpendAttempt.connect(std::bind(&CValidationInterface::NotifyInstantSendDoubleSpendAttempt, pwalletIn, std::placeholders::_1, std::placeholders::_2)); - conns.NotifyRecoveredSig = g_signals.m_internals->NotifyRecoveredSig.connect(std::bind(&CValidationInterface::NotifyRecoveredSig, pwalletIn, std::placeholders::_1)); - conns.NotifyMasternodeListChanged = g_signals.m_internals->NotifyMasternodeListChanged.connect(std::bind(&CValidationInterface::NotifyMasternodeListChanged, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4)); + g_signals.m_internals->Register(std::move(pwalletIn)); } void RegisterValidationInterface(CValidationInterface* callbacks) @@ -138,7 +139,7 @@ void UnregisterSharedValidationInterface(std::shared_ptr c void UnregisterValidationInterface(CValidationInterface* pwalletIn) { if (g_signals.m_internals) { - g_signals.m_internals->m_connMainSignals.erase(pwalletIn); + g_signals.m_internals->Unregister(pwalletIn); } } @@ -146,7 +147,7 @@ void UnregisterAllValidationInterfaces() { if (!g_signals.m_internals) { return; } - g_signals.m_internals->m_connMainSignals.clear(); + g_signals.m_internals->Clear(); } void CallFunctionInValidationInterfaceQueue(std::function func) { @@ -186,7 +187,7 @@ void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockInd // in the same critical section where the chain is updated auto event = [pindexNew, pindexFork, fInitialDownload, this] { - m_internals->UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); }); }; ENQUEUE_AND_LOG_EVENT(event, "%s: new block hash=%s fork block hash=%s (in IBD=%s)", __func__, pindexNew->GetBlockHash().ToString(), @@ -195,12 +196,12 @@ void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockInd } void CMainSignals::SynchronousUpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) { - m_internals->SynchronousUpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.SynchronousUpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); }); } void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx, int64_t nAcceptTime) { auto event = [ptx, nAcceptTime, this] { - m_internals->TransactionAddedToMempool(ptx, nAcceptTime); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionAddedToMempool(ptx, nAcceptTime); }); }; ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s", __func__, ptx->GetHash().ToString()); @@ -208,7 +209,7 @@ void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx, int64_t void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) { auto event = [ptx, reason, this] { - m_internals->TransactionRemovedFromMempool(ptx, reason); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(ptx, reason); }); }; ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s", __func__, ptx->GetHash().ToString()); @@ -216,7 +217,7 @@ void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx, Mem void CMainSignals::BlockConnected(const std::shared_ptr &pblock, const CBlockIndex *pindex) { auto event = [pblock, pindex, this] { - m_internals->BlockConnected(pblock, pindex); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockConnected(pblock, pindex); }); }; ENQUEUE_AND_LOG_EVENT(event, "%s: block hash=%s block height=%d", __func__, pblock->GetHash().ToString(), @@ -225,7 +226,7 @@ void CMainSignals::BlockConnected(const std::shared_ptr &pblock, c void CMainSignals::BlockDisconnected(const std::shared_ptr &pblock, const CBlockIndex* pindex) { auto event = [pblock, pindex, this] { - m_internals->BlockDisconnected(pblock, pindex); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockDisconnected(pblock, pindex); }); }; ENQUEUE_AND_LOG_EVENT(event, "%s: block hash=%s block height=%d", __func__, pblock->GetHash().ToString(), @@ -234,7 +235,7 @@ void CMainSignals::BlockDisconnected(const std::shared_ptr &pblock void CMainSignals::ChainStateFlushed(const CBlockLocator &locator) { auto event = [locator, this] { - m_internals->ChainStateFlushed(locator); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.ChainStateFlushed(locator); }); }; ENQUEUE_AND_LOG_EVENT(event, "%s: block hash=%s", __func__, locator.IsNull() ? "null" : locator.vHave.front().ToString()); @@ -243,27 +244,27 @@ void CMainSignals::ChainStateFlushed(const CBlockLocator &locator) { void CMainSignals::BlockChecked(const CBlock& block, const BlockValidationState& state) { LOG_EVENT("%s: block hash=%s state=%s", __func__, block.GetHash().ToString(), state.ToString()); - m_internals->BlockChecked(block, state); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockChecked(block, state); }); } void CMainSignals::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr &block) { LOG_EVENT("%s: block hash=%s", __func__, block->GetHash().ToString()); - m_internals->NewPoWValidBlock(pindex, block); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NewPoWValidBlock(pindex, block); }); } void CMainSignals::AcceptedBlockHeader(const CBlockIndex *pindexNew) { LOG_EVENT("%s: accepted block header hash=%s", __func__, pindexNew->GetBlockHash().ToString()); - m_internals->AcceptedBlockHeader(pindexNew); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.AcceptedBlockHeader(pindexNew); }); } void CMainSignals::NotifyHeaderTip(const CBlockIndex *pindexNew, bool fInitialDownload) { LOG_EVENT("%s: accepted block header hash=%s initial=%d", __func__, pindexNew->GetBlockHash().ToString(), fInitialDownload); - m_internals->NotifyHeaderTip(pindexNew, fInitialDownload); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyHeaderTip(pindexNew, fInitialDownload); }); } void CMainSignals::NotifyTransactionLock(const CTransactionRef &tx, const std::shared_ptr& islock) { auto event = [tx, islock, this] { - m_internals->NotifyTransactionLock(tx, islock); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyTransactionLock(tx, islock); }); }; ENQUEUE_AND_LOG_EVENT(event, "%s: transaction lock txid=%s", __func__, tx->GetHash().ToString()); @@ -271,7 +272,7 @@ void CMainSignals::NotifyTransactionLock(const CTransactionRef &tx, const std::s void CMainSignals::NotifyChainLock(const CBlockIndex* pindex, const std::shared_ptr& clsig) { auto event = [pindex, clsig, this] { - m_internals->NotifyChainLock(pindex, clsig); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyChainLock(pindex, clsig); }); }; ENQUEUE_AND_LOG_EVENT(event, "%s: notify chainlock at block=%s cl=%s", __func__, pindex->GetBlockHash().ToString(), @@ -280,21 +281,21 @@ void CMainSignals::NotifyChainLock(const CBlockIndex* pindex, const std::shared_ void CMainSignals::NotifyGovernanceVote(const std::shared_ptr& vote) { auto event = [vote, this] { - m_internals->NotifyGovernanceVote(vote); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyGovernanceVote(vote); }); }; ENQUEUE_AND_LOG_EVENT(event, "%s: notify governance vote: %s", __func__, vote->GetHash().ToString()); } void CMainSignals::NotifyGovernanceObject(const std::shared_ptr& object) { auto event = [object, this] { - m_internals->NotifyGovernanceObject(object); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyGovernanceObject(object); }); }; ENQUEUE_AND_LOG_EVENT(event, "%s: notify governance object: %s", __func__, object->GetHash().ToString()); } void CMainSignals::NotifyInstantSendDoubleSpendAttempt(const CTransactionRef& currentTx, const CTransactionRef& previousTx) { auto event = [currentTx, previousTx, this] { - m_internals->NotifyInstantSendDoubleSpendAttempt(currentTx, previousTx); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyInstantSendDoubleSpendAttempt(currentTx, previousTx); }); }; ENQUEUE_AND_LOG_EVENT(event, "%s: notify instant doublespendattempt currenttxid=%s previoustxid=%s", __func__, currentTx->GetHash().ToString(), @@ -303,7 +304,7 @@ void CMainSignals::NotifyInstantSendDoubleSpendAttempt(const CTransactionRef& cu void CMainSignals::NotifyRecoveredSig(const std::shared_ptr& sig) { auto event = [sig, this] { - m_internals->NotifyRecoveredSig(sig); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyRecoveredSig(sig); }); }; ENQUEUE_AND_LOG_EVENT(event, "%s: notify recoveredsig=%s", __func__, sig->GetHash().ToString()); @@ -311,5 +312,5 @@ void CMainSignals::NotifyRecoveredSig(const std::shared_ptrNotifyMasternodeListChanged(undo, oldMNList, diff, connman); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyMasternodeListChanged(undo, oldMNList, diff, connman); }); } diff --git a/src/validationinterface.h b/src/validationinterface.h index 6f9b8a539b3c..4a29c8c0882b 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -194,9 +194,7 @@ class CValidationInterface { * 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 */ virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& block) {}; - friend void ::RegisterSharedValidationInterface(std::shared_ptr); - friend void ::UnregisterValidationInterface(CValidationInterface*); - friend void ::UnregisterAllValidationInterfaces(); + friend class CMainSignals; }; struct MainSignalsInstance; From 76242f9d8d4871fcbd551d82fc11e92d40e24284 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 8 Apr 2020 20:43:22 +0800 Subject: [PATCH 04/10] Merge #18551: Do not clear validationinterface entries being executed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 2276339a176f83ffe8ceefb3e41ecca8601aa13b Add test for UnregisterAllValidationInterfaces bug (Russell Yanofsky) 3c61abbbc847d725f30d169278d84655571407c1 Do not clear validationinterface entries being executed (Pieter Wuille) Pull request description: The previous code for MainSignalsInstance::Clear would decrement the reference count of every interface, including ones that were already Unregister()ed but still being executed. This fixes the issue pointed out here: https://github.com/bitcoin/bitcoin/pull/18524/files#r404395685 . It's not currently observable. ACKs for top commit: jonasschnelli: utACK 2276339a176f83ffe8ceefb3e41ecca8601aa13b - reviewed code and test (thanks @ryanofsky for adding the test). MarcoFalke: ACK 2276339a176f83ffe8ceefb3e41ecca8601aa13b 🎎 ryanofsky: Code review ACK 2276339a176f83ffe8ceefb3e41ecca8601aa13b. No change to bugfix, just rebased and new test commit added since last review Tree-SHA512: c1d68e7c681a45c6cadc84e407c2266bcb4b12d34264e1232a61c4eadb74b551231c5a3b1d041de39f507aef4dfa7d4589b8bfe1833f069c739c6270d2a05dbe --- src/Makefile.test.include | 1 + src/test/validationinterface_tests.cpp | 63 ++++++++++++++++++++++++++ src/validationinterface.cpp | 4 +- 3 files changed, 66 insertions(+), 2 deletions(-) create mode 100644 src/test/validationinterface_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index abcfeb73a658..5907bcccdcea 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -164,6 +164,7 @@ BITCOIN_TESTS =\ test/validation_chainstate_tests.cpp \ test/validation_chainstatemanager_tests.cpp \ test/validation_flush_tests.cpp \ + test/validationinterface_tests.cpp \ test/versionbits_tests.cpp if ENABLE_WALLET diff --git a/src/test/validationinterface_tests.cpp b/src/test/validationinterface_tests.cpp new file mode 100644 index 000000000000..b4aa2f0f3a08 --- /dev/null +++ b/src/test/validationinterface_tests.cpp @@ -0,0 +1,63 @@ +// Copyright (c) 2020 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +#include +#include +#include +#include +#include + +BOOST_AUTO_TEST_SUITE(validationinterface_tests) + +class TestInterface : public CValidationInterface +{ +public: + TestInterface(std::function on_call = nullptr, std::function on_destroy = nullptr) + : m_on_call(std::move(on_call)), m_on_destroy(std::move(on_destroy)) + { + } + virtual ~TestInterface() + { + if (m_on_destroy) m_on_destroy(); + } + void BlockChecked(const CBlock& block, const BlockValidationState& state) override + { + if (m_on_call) m_on_call(); + } + static void Call() + { + CBlock block; + BlockValidationState state; + GetMainSignals().BlockChecked(block, state); + } + std::function m_on_call; + std::function m_on_destroy; +}; + +// Regression test to ensure UnregisterAllValidationInterfaces calls don't +// destroy a validation interface while it is being called. Bug: +// https://github.com/bitcoin/bitcoin/pull/18551 +BOOST_AUTO_TEST_CASE(unregister_all_during_call) +{ + bool destroyed = false; + + CScheduler scheduler; + GetMainSignals().RegisterBackgroundSignalScheduler(scheduler); + RegisterSharedValidationInterface(std::make_shared( + [&] { + // First call should decrements reference count 2 -> 1 + UnregisterAllValidationInterfaces(); + BOOST_CHECK(!destroyed); + // Second call should not decrement reference count 1 -> 0 + UnregisterAllValidationInterfaces(); + BOOST_CHECK(!destroyed); + }, + [&] { destroyed = true; })); + TestInterface::Call(); + BOOST_CHECK(destroyed); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 577c9beecf57..19085d832247 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -72,8 +72,8 @@ struct MainSignalsInstance { void Clear() { LOCK(m_mutex); - for (auto it = m_list.begin(); it != m_list.end();) { - it = --it->count ? std::next(it) : m_list.erase(it); + for (const auto& entry : m_map) { + if (!--entry.second->count) m_list.erase(entry.second); } m_map.clear(); } From 363b37dfbdabcad909824684b0158c8889ae4d1d Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Mon, 6 Apr 2020 20:29:16 +0200 Subject: [PATCH 05/10] Merge #18487: rpc: Fix rpcRunLater race in walletpassphrase MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 7b8e15728d1ad058a4b7d7569fd5d5ba6806ca28 rpc: Fix rpcRunLater race in walletpassphrase (João Barbosa) Pull request description: Release locks before calling `rpcRunLater`. Quick explanation: `rpcRunLater` leads to `event_free` which calls `event_del` which can wait for the event callback to finish if it's already running and that callback will try to lock wallet mutex - which is already locked in http thread. Fixes #14995 , fixes #18482. Best reviewed with whitespace changes hidden. ACKs for top commit: MarcoFalke: ACK 7b8e15728d, only tested that this avoids the node freezing. Did not look at how libevent works or how the deadlock happens or if this breaks other stuff. 📞 ryanofsky: Code review ACK 7b8e15728d1ad058a4b7d7569fd5d5ba6806ca28. Just updated comment since last review Tree-SHA512: 17874a2fa7b0e164fb0d7ee4cb7d59650275b8c03476fb291d60af8b758495457660d3912623fb26259fefe84aeba21c0a9e0c6467982ba511f19344ed5413ab --- src/wallet/rpcwallet.cpp | 84 ++++++++++++++++++++++------------------ 1 file changed, 46 insertions(+), 38 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index a19490c298fa..169b79887117 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1934,54 +1934,62 @@ static UniValue walletpassphrase(const JSONRPCRequest& request) if (!wallet) return NullUniValue; CWallet* const pwallet = wallet.get(); - LOCK(pwallet->cs_wallet); + int64_t nSleepTime; + { + LOCK(pwallet->cs_wallet); - if (!pwallet->IsCrypted()) { - throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: running with an unencrypted wallet, but walletpassphrase was called."); - } + if (!pwallet->IsCrypted()) { + throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: running with an unencrypted wallet, but walletpassphrase was called."); + } - // Note that the walletpassphrase is stored in request.params[0] which is not mlock()ed - SecureString strWalletPass; - strWalletPass.reserve(100); - // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string) - // Alternately, find a way to make request.params[0] mlock()'d to begin with. - strWalletPass = request.params[0].get_str().c_str(); + // Note that the walletpassphrase is stored in request.params[0] which is not mlock()ed + SecureString strWalletPass; + strWalletPass.reserve(100); + // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string) + // Alternately, find a way to make request.params[0] mlock()'d to begin with. + strWalletPass = request.params[0].get_str().c_str(); - // Get the timeout - int64_t nSleepTime = request.params[1].get_int64(); - // Timeout cannot be negative, otherwise it will relock immediately - if (nSleepTime < 0) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Timeout cannot be negative."); - } - // Clamp timeout - constexpr int64_t MAX_SLEEP_TIME = 100000000; // larger values trigger a macos/libevent bug? - if (nSleepTime > MAX_SLEEP_TIME) { - nSleepTime = MAX_SLEEP_TIME; - } + // Get the timeout + nSleepTime = request.params[1].get_int64(); + // Timeout cannot be negative, otherwise it will relock immediately + if (nSleepTime < 0) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Timeout cannot be negative."); + } + // Clamp timeout + constexpr int64_t MAX_SLEEP_TIME = 100000000; // larger values trigger a macos/libevent bug? + if (nSleepTime > MAX_SLEEP_TIME) { + nSleepTime = MAX_SLEEP_TIME; + } - bool fForMixingOnly = false; - if (!request.params[2].isNull()) - fForMixingOnly = request.params[2].get_bool(); + bool fForMixingOnly = false; + if (!request.params[2].isNull()) + fForMixingOnly = request.params[2].get_bool(); - if (fForMixingOnly && !pwallet->IsLocked()) { - // Downgrading from "fuly unlocked" mode to "mixing only" one is not supported. - // Updating unlock time when current unlock mode is not changed or when it is upgraded - // from "mixing only" to "fuly unlocked" is ok. - throw JSONRPCError(RPC_WALLET_ALREADY_UNLOCKED, "Error: Wallet is already fully unlocked."); - } + if (fForMixingOnly && !pwallet->IsLocked()) { + // Downgrading from "fuly unlocked" mode to "mixing only" one is not supported. + // Updating unlock time when current unlock mode is not changed or when it is upgraded + // from "mixing only" to "fuly unlocked" is ok. + throw JSONRPCError(RPC_WALLET_ALREADY_UNLOCKED, "Error: Wallet is already fully unlocked."); + } - if (strWalletPass.empty()) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "passphrase can not be empty"); - } + if (strWalletPass.empty()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "passphrase can not be empty"); + } - if (!pwallet->Unlock(strWalletPass, fForMixingOnly)) { - throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect."); - } + if (!pwallet->Unlock(strWalletPass, fForMixingOnly)) { + throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect."); + } - pwallet->TopUpKeyPool(); + pwallet->TopUpKeyPool(); - pwallet->nRelockTime = GetTime() + nSleepTime; + pwallet->nRelockTime = GetTime() + nSleepTime; + } + // rpcRunLater must be called without cs_wallet held otherwise a deadlock + // can occur. The deadlock would happen when RPCRunLater removes the + // previous timer (and waits for the callback to finish if already running) + // and the callback locks cs_wallet. + AssertLockNotHeld(wallet->cs_wallet); // Keep a weak pointer to the wallet so that it is possible to unload the // wallet before the following callback is called. If a valid shared pointer // is acquired in the callback then the wallet is still loaded. From 2046ae1bc04a7b216d67096ef17b04a8a15cd6ac Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 7 Apr 2020 18:47:28 +0800 Subject: [PATCH 06/10] Merge #18546: Bugfix: Wallet: Safely deal with change in the address book [part 2] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 7a2ecf16df938dd95d3130a46082def7a02338eb Wallet: Change IsMine check in CWallet::DelAddressBook from assert to failure (Luke Dashjr) 2952c46b923042f2de801f319e03ed5c4c4eb735 Wallet: Replace CAddressBookData.name with GetLabel() method (Luke Dashjr) d7092c392e10889cd7a080b3d22ed6446a59b87a QA: Test that change doesn't turn into non-change when spent in an avoid-reuse wallet (Luke Dashjr) Pull request description: Follow-up to #18192, not strictly necessary for 0.20 ACKs for top commit: MarcoFalke: re-ACK 7a2ecf16df, only change is adding an assert_equal in the test 🔰 jnewbery: utACK 7a2ecf16df938dd95d3130a46082def7a02338eb Tree-SHA512: e0933ee40f705b751697dc27249e1868ed4874254b174ebdd0a7150125d8c818402e66df2371718c7eeb90e67ee2317215fb260aa9b9d7b9b45ee436de2988ff --- src/wallet/interfaces.cpp | 4 ++-- src/wallet/rpcdump.cpp | 2 +- src/wallet/rpcwallet.cpp | 20 ++++++++++---------- src/wallet/wallet.cpp | 7 +++++-- src/wallet/wallet.h | 4 ++-- test/functional/wallet_avoidreuse.py | 25 +++++++++++++++++++++++++ 6 files changed, 45 insertions(+), 17 deletions(-) diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 200bf3e8316c..d530fde5c921 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -245,7 +245,7 @@ class WalletImpl : public Wallet return false; } if (name) { - *name = it->second.name; + *name = it->second.GetLabel(); } if (is_mine) { *is_mine = m_wallet->IsMine(dest); @@ -261,7 +261,7 @@ class WalletImpl : public Wallet std::vector result; for (const auto& item : m_wallet->m_address_book) { if (item.second.IsChange()) continue; - result.emplace_back(item.first, m_wallet->IsMine(item.first), item.second.name, item.second.purpose); + result.emplace_back(item.first, m_wallet->IsMine(item.first), item.second.GetLabel(), item.second.purpose); } return result; } diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index d6706fd4aad8..f773eb2612c1 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -1014,7 +1014,7 @@ UniValue dumpwallet(const JSONRPCRequest& request) file << strprintf("%s %s ", EncodeSecret(key), strTime); const auto* address_book_entry = pwallet->FindAddressBookEntry(pkhash); if (address_book_entry) { - file << strprintf("label=%s", EncodeDumpString(address_book_entry->name)); + file << strprintf("label=%s", EncodeDumpString(address_book_entry->GetLabel())); } else if (mapKeyPool.count(keyid)) { file << "reserve=1"; } else { diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 169b79887117..0346fdcb1a8c 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -532,7 +532,7 @@ static UniValue listaddressgroupings(const JSONRPCRequest& request) { const auto* address_book_entry = pwallet->FindAddressBookEntry(address); if (address_book_entry) { - addressInfo.push_back(address_book_entry->name); + addressInfo.push_back(address_book_entry->GetLabel()); } } jsonGrouping.push_back(addressInfo); @@ -1151,7 +1151,7 @@ static UniValue ListReceived(const CWallet * const pwallet, const UniValue& para { if (item_it->second.IsChange()) continue; const CTxDestination& address = item_it->first; - const std::string& label = item_it->second.name; + const std::string& label = item_it->second.GetLabel(); auto it = mapTally.find(address); if (it == mapTally.end() && !fIncludeEmpty) continue; @@ -1354,7 +1354,7 @@ static void ListTransactions(const CWallet* const pwallet, const CWalletTx& wtx, entry.pushKV("amount", ValueFromAmount(-s.amount)); const auto* address_book_entry = pwallet->FindAddressBookEntry(s.destination); if (address_book_entry) { - entry.pushKV("label", address_book_entry->name); + entry.pushKV("label", address_book_entry->GetLabel()); } entry.pushKV("vout", s.vout); entry.pushKV("fee", ValueFromAmount(-nFee)); @@ -1373,7 +1373,7 @@ static void ListTransactions(const CWallet* const pwallet, const CWalletTx& wtx, std::string label; const auto* address_book_entry = pwallet->FindAddressBookEntry(r.destination); if (address_book_entry) { - label = address_book_entry->name; + label = address_book_entry->GetLabel(); } if (filter_label && label != *filter_label) { continue; @@ -3194,7 +3194,7 @@ static UniValue listunspent(const JSONRPCRequest& request) const auto* address_book_entry = pwallet->FindAddressBookEntry(address); if (address_book_entry) { - entry.pushKV("label", address_book_entry->name); + entry.pushKV("label", address_book_entry->GetLabel()); } std::unique_ptr provider = pwallet->GetSolvingProvider(scriptPubKey); @@ -3714,7 +3714,7 @@ static UniValue AddressBookDataToJSON(const CAddressBookData& data, const bool v { UniValue ret(UniValue::VOBJ); if (verbose) { - ret.pushKV("name", data.name); + ret.pushKV("name", data.GetLabel()); } ret.pushKV("purpose", data.purpose); return ret; @@ -3816,7 +3816,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request) // value of the name key/value pair in the labels array below. const auto* address_book_entry = pwallet->FindAddressBookEntry(dest); if (pwallet->chain().rpcEnableDeprecated("label") && address_book_entry) { - ret.pushKV("label", address_book_entry->name); + ret.pushKV("label", address_book_entry->GetLabel()); } ret.pushKV("ischange", pwallet->IsChange(scriptPubKey)); @@ -3852,7 +3852,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request) if (pwallet->chain().rpcEnableDeprecated("labelspurpose")) { labels.push_back(AddressBookDataToJSON(*address_book_entry, true)); } else { - labels.push_back(address_book_entry->name); + labels.push_back(address_book_entry->GetLabel()); } } ret.pushKV("labels", std::move(labels)); @@ -3895,7 +3895,7 @@ static UniValue getaddressesbylabel(const JSONRPCRequest& request) std::set addresses; for (const std::pair& item : pwallet->m_address_book) { if (item.second.IsChange()) continue; - if (item.second.name == label) { + if (item.second.GetLabel() == label) { std::string address = EncodeDestination(item.first); // CWallet::m_address_book is not expected to contain duplicate // address strings, but build a separate set as a precaution just in @@ -3958,7 +3958,7 @@ static UniValue listlabels(const JSONRPCRequest& request) for (const std::pair& entry : pwallet->m_address_book) { if (entry.second.IsChange()) continue; if (purpose.empty() || entry.second.purpose == purpose) { - label_set.insert(entry.second.name); + label_set.insert(entry.second.GetLabel()); } } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c82873ddc478..224ed4252f40 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3938,7 +3938,10 @@ bool CWallet::DelAddressBook(const CTxDestination& address) // If we want to delete receiving addresses, we need to take care that DestData "used" (and possibly newer DestData) gets preserved (and the "deleted" address transformed into a change entry instead of actually being deleted) // NOTE: This isn't a problem for sending addresses because they never have any DestData yet! // When adding new DestData, it should be considered here whether to retain or delete it (or move it?). - assert(!IsMine(address)); + if (IsMine(address)) { + WalletLogPrintf("%s called with IsMine address, NOT SUPPORTED. Please report this bug! %s\n", __func__, PACKAGE_BUGREPORT); + return false; + } { LOCK(cs_wallet); @@ -4190,7 +4193,7 @@ std::set CWallet::GetLabelAddresses(const std::string& label) co { if (item.second.IsChange()) continue; const CTxDestination& address = item.first; - const std::string& strName = item.second.name; + const std::string& strName = item.second.GetLabel(); if (strName == label) result.insert(address); } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 55713ead89fb..aad3641c5860 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -191,15 +191,15 @@ class CAddressBookData bool m_change{true}; std::string m_label; public: - const std::string& name; std::string purpose; - CAddressBookData() : name(m_label), purpose("unknown") {} + CAddressBookData() : purpose("unknown") {} typedef std::map StringMap; StringMap destdata; bool IsChange() const { return m_change; } + const std::string& GetLabel() const { return m_label; } void SetLabel(const std::string& label) { m_change = false; m_label = label; diff --git a/test/functional/wallet_avoidreuse.py b/test/functional/wallet_avoidreuse.py index 2f98c895011f..bff24f772d0d 100755 --- a/test/functional/wallet_avoidreuse.py +++ b/test/functional/wallet_avoidreuse.py @@ -88,6 +88,7 @@ def run_test(self): self.nodes[0].generate(110) self.sync_all() + self.test_change_remains_change(self.nodes[1]) reset_balance(self.nodes[1], self.nodes[0].getnewaddress()) self.test_sending_from_reused_address_without_avoid_reuse() reset_balance(self.nodes[1], self.nodes[0].getnewaddress()) @@ -141,6 +142,30 @@ def test_immutable(self): # Unload temp wallet self.nodes[1].unloadwallet(tempwallet) + def test_change_remains_change(self, node): + self.log.info("Test that change doesn't turn into non-change when spent") + + reset_balance(node, node.getnewaddress()) + addr = node.getnewaddress() + txid = node.sendtoaddress(addr, 1) + out = node.listunspent(minconf=0, query_options={'minimumAmount': 2}) + assert_equal(len(out), 1) + assert_equal(out[0]['txid'], txid) + changeaddr = out[0]['address'] + + # Make sure it's starting out as change as expected + assert node.getaddressinfo(changeaddr)['ischange'] + for logical_tx in node.listtransactions(): + assert logical_tx.get('address') != changeaddr + + # Spend it + reset_balance(node, node.getnewaddress()) + + # It should still be change + assert node.getaddressinfo(changeaddr)['ischange'] + for logical_tx in node.listtransactions(): + assert logical_tx.get('address') != changeaddr + def test_sending_from_reused_address_without_avoid_reuse(self): ''' Test the same as test_sending_from_reused_address_fails, except send the 10 BTC with From 560e5589cbf7c7c92be770a3a1aa2969d8443547 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 7 Apr 2020 23:45:34 +0800 Subject: [PATCH 07/10] Merge #18532: rpc: Avoid initialization-order-fiasco on static CRPCCommand tables fa1a92224dd78de817d15bcda35a8310254e1a54 rpc: Avoid initialization-order-fiasco on static CRPCCommand tables (MarcoFalke) Pull request description: Currently the fiasco is only theoretical because all content of the table are compile-time constants. However, the fiasco materializes should they ever become run-time constants (e.g. #18531). ACKs for top commit: promag: ACK fa1a92224dd78de817d15bcda35a8310254e1a54. practicalswift: ACK fa1a92224dd78de817d15bcda35a8310254e1a54 -- fiasco bad :) Tree-SHA512: cccadb0ad56194599b74f04264d74c34fa865958580a850efc6474bbdc56f30cadce6b2e9a6ad5472ff46c3f4c793366acd8090fad409a45b25d961f2d89da19 --- src/rpc/blockchain.cpp | 4 ++-- src/rpc/coinjoin.cpp | 4 ++-- src/rpc/evo.cpp | 4 ++-- src/rpc/governance.cpp | 4 ++-- src/rpc/masternode.cpp | 5 +++-- src/rpc/mining.cpp | 4 ++-- src/rpc/misc.cpp | 7 +++---- src/rpc/net.cpp | 4 ++-- src/rpc/quorums.cpp | 4 ++-- src/rpc/rawtransaction.cpp | 4 ++-- src/wallet/rpcwallet.cpp | 4 ++-- 11 files changed, 24 insertions(+), 24 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index f24e968f240d..473f58c21770 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -2946,6 +2946,8 @@ UniValue CreateUTXOSnapshot(NodeContext& node, CChainState& chainstate, CAutoFil return result; } +void RegisterBlockchainRPCCommands(CRPCTable &t) +{ // clang-format off static const CRPCCommand commands[] = { // category name actor (function) argNames @@ -2991,8 +2993,6 @@ static const CRPCCommand commands[] = }; // clang-format on -void RegisterBlockchainRPCCommands(CRPCTable &t) -{ for (const auto& command : commands) { t.appendCommand(command.name, &command); } diff --git a/src/rpc/coinjoin.cpp b/src/rpc/coinjoin.cpp index ef970c937a4a..f3e6d2993aa3 100644 --- a/src/rpc/coinjoin.cpp +++ b/src/rpc/coinjoin.cpp @@ -170,6 +170,8 @@ static UniValue getcoinjoininfo(const JSONRPCRequest& request) return obj; } +void RegisterCoinJoinRPCCommands(CRPCTable &t) +{ // clang-format off static const CRPCCommand commands[] = { // category name actor (function) argNames @@ -181,8 +183,6 @@ static const CRPCCommand commands[] = #endif // ENABLE_WALLET }; // clang-format on -void RegisterCoinJoinRPCCommands(CRPCTable &t) -{ for (const auto& command : commands) { t.appendCommand(command.name, &command); } diff --git a/src/rpc/evo.cpp b/src/rpc/evo.cpp index 536c2766ef3b..fec1e50cb075 100644 --- a/src/rpc/evo.cpp +++ b/src/rpc/evo.cpp @@ -1755,6 +1755,8 @@ static UniValue _bls(const JSONRPCRequest& request) bls_help(); } } +void RegisterEvoRPCCommands(CRPCTable &tableRPC) +{ // clang-format off static const CRPCCommand commands[] = { // category name actor (function) @@ -1763,8 +1765,6 @@ static const CRPCCommand commands[] = { "evo", "protx", &protx, {} }, }; // clang-format on -void RegisterEvoRPCCommands(CRPCTable &tableRPC) -{ for (const auto& command : commands) { tableRPC.appendCommand(command.name, &command); } diff --git a/src/rpc/governance.cpp b/src/rpc/governance.cpp index cab85368d244..9ffa8f0271f6 100644 --- a/src/rpc/governance.cpp +++ b/src/rpc/governance.cpp @@ -1191,6 +1191,8 @@ static UniValue getsuperblockbudget(const JSONRPCRequest& request) return ValueFromAmount(CSuperblock::GetPaymentsLimit(nBlockHeight)); } +void RegisterGovernanceRPCCommands(CRPCTable &t) +{ // clang-format off static const CRPCCommand commands[] = { // category name actor (function) argNames @@ -1203,8 +1205,6 @@ static const CRPCCommand commands[] = }; // clang-format on -void RegisterGovernanceRPCCommands(CRPCTable &t) -{ for (const auto& command : commands) { t.appendCommand(command.name, &command); } diff --git a/src/rpc/masternode.cpp b/src/rpc/masternode.cpp index 63b89739420a..0058e419ffaf 100644 --- a/src/rpc/masternode.cpp +++ b/src/rpc/masternode.cpp @@ -736,6 +736,9 @@ static UniValue masternodelist(const JSONRPCRequest& request, ChainstateManager& return obj; } + +void RegisterMasternodeRPCCommands(CRPCTable &t) +{ // clang-format off static const CRPCCommand commands[] = { // category name actor (function) argNames @@ -744,8 +747,6 @@ static const CRPCCommand commands[] = { "dash", "masternodelist", &masternode, {} }, }; // clang-format on -void RegisterMasternodeRPCCommands(CRPCTable &t) -{ for (const auto& command : commands) { t.appendCommand(command.name, &command); } diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 3ae7a716afa8..d0ae3585c709 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -1249,6 +1249,8 @@ static UniValue estimaterawfee(const JSONRPCRequest& request) return result; } +void RegisterMiningRPCCommands(CRPCTable &t) +{ // clang-format off static const CRPCCommand commands[] = { // category name actor (function) argNames @@ -1276,8 +1278,6 @@ static const CRPCCommand commands[] = }; // clang-format on -void RegisterMiningRPCCommands(CRPCTable &t) -{ for (const auto& command : commands) { t.appendCommand(command.name, &command); } diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp index aac2c3a28ecf..7f6f7f997876 100644 --- a/src/rpc/misc.cpp +++ b/src/rpc/misc.cpp @@ -1363,11 +1363,12 @@ static RPCHelpMan getindexinfo() }); return result; -}, +} }; } -// clang-format off +void RegisterMiscRPCCommands(CRPCTable &t) +{ static const CRPCCommand commands[] = { // category name actor (function) argNames // --------------------- ------------------------ ----------------------- ---------- @@ -1404,8 +1405,6 @@ static const CRPCCommand commands[] = }; // clang-format on -void RegisterMiscRPCCommands(CRPCTable &t) -{ for (const auto& command : commands) { t.appendCommand(command.name, &command); } diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index e972f7a94206..a63cffdb9790 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -884,6 +884,8 @@ static UniValue addpeeraddress(const JSONRPCRequest& request) return obj; } +void RegisterNetRPCCommands(CRPCTable &t) +{ // clang-format off static const CRPCCommand commands[] = { // category name actor (function) argNames @@ -906,8 +908,6 @@ static const CRPCCommand commands[] = }; // clang-format on -void RegisterNetRPCCommands(CRPCTable &t) -{ for (const auto& command : commands) { t.appendCommand(command.name, &command); } diff --git a/src/rpc/quorums.cpp b/src/rpc/quorums.cpp index 592d73d0daa2..fa620da92ee7 100644 --- a/src/rpc/quorums.cpp +++ b/src/rpc/quorums.cpp @@ -989,6 +989,8 @@ static UniValue verifyislock(const JSONRPCRequest& request) return llmq_ctx.sigman->VerifyRecoveredSig(llmqType, *llmq_ctx.qman, signHeight, id, txid, sig, 0) || llmq_ctx.sigman->VerifyRecoveredSig(llmqType, *llmq_ctx.qman, signHeight, id, txid, sig, signOffset); } +void RegisterQuorumsRPCCommands(CRPCTable &tableRPC) +{ // clang-format off static const CRPCCommand commands[] = { // category name actor (function) @@ -998,8 +1000,6 @@ static const CRPCCommand commands[] = { "evo", "verifyislock", &verifyislock, {"id", "txid", "signature", "maxHeight"} }, }; // clang-format on -void RegisterQuorumsRPCCommands(CRPCTable &tableRPC) -{ for (const auto& command : commands) { tableRPC.appendCommand(command.name, &command); } diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 07482192e2b1..6f5f172597de 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1682,6 +1682,8 @@ UniValue analyzepsbt(const JSONRPCRequest& request) return result; } +void RegisterRawTransactionRPCCommands(CRPCTable &t) +{ // clang-format off static const CRPCCommand commands[] = { // category name actor (function) argNames @@ -1708,8 +1710,6 @@ static const CRPCCommand commands[] = }; // clang-format on -void RegisterRawTransactionRPCCommands(CRPCTable &t) -{ for (const auto& command : commands) { t.appendCommand(command.name, &command); } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 0346fdcb1a8c..8fb0f0fc4c2f 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4196,6 +4196,8 @@ static UniValue upgradewallet(const JSONRPCRequest& request) return error.original; } +Span GetWalletRPCCommands() +{ // clang-format off static const CRPCCommand commands[] = { // category name actor (function) argNames @@ -4266,7 +4268,5 @@ static const CRPCCommand commands[] = }; // clang-format on -Span GetWalletRPCCommands() -{ return MakeSpan(commands); } From 7c5f9af0dc2ef152e26cb81dd67fe2e606fce3d8 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 8 Apr 2020 23:24:41 +0800 Subject: [PATCH 08/10] Merge #18563: test: Fix unregister_all_during_call cleanup 13d2a33537a403ac47a989be92109d3214375b6a Fix unregister_all_during_call cleanup (Russell Yanofsky) Pull request description: Use `TestingSetup` fixture to fix `unregister_all_during_call` test not calling `UnregisterBackgroundSignalScheduler`, which could trigger an assert in `RegisterBackgroundSignalScheduler` when called in later tests Failure reported by fanquake https://github.com/bitcoin/bitcoin/pull/18551#issuecomment-610974251 ACKs for top commit: MarcoFalke: ACK 13d2a33537a403ac47a989be92109d3214375b6a if appveyor unit tests pass Tree-SHA512: d2ec8ff14c54d97903af50031abfac1f38ec1c3aabc90371cfd5b79481fa69d3d77f339bfdf7d2178fd85e83402f72eda7cf4d339e5bbfa7e6e1a68836643b93 --- src/test/validationinterface_tests.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/test/validationinterface_tests.cpp b/src/test/validationinterface_tests.cpp index b4aa2f0f3a08..208be928521a 100644 --- a/src/test/validationinterface_tests.cpp +++ b/src/test/validationinterface_tests.cpp @@ -3,14 +3,14 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include - #include #include #include +#include #include #include -BOOST_AUTO_TEST_SUITE(validationinterface_tests) +BOOST_FIXTURE_TEST_SUITE(validationinterface_tests, TestingSetup) class TestInterface : public CValidationInterface { @@ -43,9 +43,6 @@ class TestInterface : public CValidationInterface BOOST_AUTO_TEST_CASE(unregister_all_during_call) { bool destroyed = false; - - CScheduler scheduler; - GetMainSignals().RegisterBackgroundSignalScheduler(scheduler); RegisterSharedValidationInterface(std::make_shared( [&] { // First call should decrements reference count 2 -> 1 From 5460866184840f6d87772ea07e88c9feff7359c4 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 9 Apr 2020 22:21:13 +0800 Subject: [PATCH 09/10] Merge #18561: test: Properly raise FailedToStartError when rpc shutdown before warmup finished faede1b293354560317b67f0b4e6874dcac6ef41 test: Properly raise FailedToStartError when rpc shutdown before warmup finished (MarcoFalke) Pull request description: Should fix issues such as https://travis-ci.org/github/bitcoin/bitcoin/jobs/671910152#L7034 Top commit has no ACKs. Tree-SHA512: ac659f29c5ec91985c916b734e24911cbf4e2c5c4b1f1891a7e6c2d2511ec285167550fb03848eee4a7a3cbc9f8cdb0c766f4e881d9e44368c7415d007006368 --- test/functional/test_framework/test_node.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 2a6ef84d2c91..1f7123e4469c 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -272,6 +272,10 @@ def wait_for_rpc_connection(self): # -342 Service unavailable, RPC server started but is shutting down due to error if e.error['code'] != -28 and e.error['code'] != -342: raise # unknown JSON RPC exception + except ConnectionResetError: + # This might happen when the RPC server is in warmup, but shut down before the call to getblockcount + # succeeds. Try again to properly raise the FailedToStartError + pass except OSError as e: if e.errno == errno.ETIMEDOUT: pass # Treat identical to ConnectionResetError From 2c9d41d0734c049fa0ecfb1a9c059a247fc00583 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 10 Apr 2020 10:10:23 -0400 Subject: [PATCH 10/10] Merge #18454: net: Make addr relay mockable, add test NOTE: There is slight difference with original backport due to future changes in bitcoin#19272, bitcoin#19763 - otherwise functional test p2p_addr_relay.py fails fa1da3d4bfc0511a89f5b19d5a4d89e55ff7ccde test: Add basic addr relay test (MarcoFalke) fa1793c1c44a3f75a09f9c636467b8274c541bdd net: Pass connman const when relaying address (MarcoFalke) fa47a0b003f53708b6d5df1ed4e7f8a7c68aa3ac net: Make addr relay mockable (MarcoFalke) Pull request description: As usual: * Switch to std::chrono time to be type-safe and mockable * Add basic test that relies on mocktime to add code coverage ACKs for top commit: naumenkogs: utACK fa1da3d promag: ACK fa1da3d4bfc0511a89f5b19d5a4d89e55ff7ccde (fabe56e44b6f683e24e37246a7a8851190947cb3 before https://github.com/bitcoin/bitcoin/pull/18454#issuecomment-607866453), fa5bf23d527a450e72c2bf13d013e5393b664ca3 was dropped since last review. Tree-SHA512: 0552bf8fcbe375baa3cab62acd8c23b2994efa47daff818ad1116d0ffaa0b9e520dc1bca2bbc68369b25584e85e54861fe6fd0968de4f503b95439c099df9bd7 fixup - see #19272, #19763 --- src/net.h | 4 +- src/net_processing.cpp | 16 +++---- test/functional/p2p_addr_relay.py | 69 +++++++++++++++++++++++++++++++ test/functional/test_runner.py | 1 + 4 files changed, 80 insertions(+), 10 deletions(-) create mode 100755 test/functional/p2p_addr_relay.py diff --git a/src/net.h b/src/net.h index aee79057a641..c557d6042c2b 100644 --- a/src/net.h +++ b/src/net.h @@ -1129,8 +1129,8 @@ class CNode std::vector vAddrToSend; const std::unique_ptr m_addr_known; bool fGetAddr{false}; - int64_t nNextAddrSend GUARDED_BY(cs_sendProcessing){0}; - int64_t nNextLocalAddrSend GUARDED_BY(cs_sendProcessing){0}; + std::chrono::microseconds m_next_addr_send GUARDED_BY(cs_sendProcessing){0}; + std::chrono::microseconds m_next_local_addr_send GUARDED_BY(cs_sendProcessing){0}; // Don't relay addr messages to peers that we connect to as block-relay-only // peers (to prevent adversaries from inferring these links from addr diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0effb6125002..1a26996d9073 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -138,9 +138,9 @@ static const int MAX_UNCONNECTING_HEADERS = 10; static const unsigned int NODE_NETWORK_LIMITED_MIN_BLOCKS = 288; /** Average delay between local address broadcasts in seconds. */ -static constexpr unsigned int AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL = 24 * 60 * 60; +static constexpr std::chrono::hours AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL{24}; /** Average delay between peer address broadcasts in seconds. */ -static const unsigned int AVG_ADDRESS_BROADCAST_INTERVAL = 30; +static constexpr std::chrono::seconds AVG_ADDRESS_BROADCAST_INTERVAL{30}; /** Average delay between trickled inventory transmissions in seconds. * Blocks and peers with noban permission bypass this, regular outbound peers get half this delay, * Masternode outbound peers get quarter this delay. */ @@ -1889,13 +1889,13 @@ void RelayTransaction(const uint256& txid, const CConnman& connman) }); } -static void RelayAddress(const CAddress& addr, bool fReachable, CConnman& connman) +static void RelayAddress(const CAddress& addr, bool fReachable, const CConnman& connman) { // Relay to a limited number of other nodes // Use deterministic randomness to send to the same nodes for 24 hours // at a time so the m_addr_knowns of the chosen nodes prevent repeats uint64_t hashAddr = addr.GetHash(); - const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24*60*60)); + const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24 * 60 * 60)); FastRandomContext insecure_rand; // Relay reachable addresses to 2 peers. Unreachable addresses are relayed randomly to 1 or 2 peers. @@ -4678,16 +4678,16 @@ bool PeerManagerImpl::SendMessages(CNode* pto) int64_t nNow = GetTimeMicros(); auto current_time = GetTime(); - if (pto->IsAddrRelayPeer() && !m_chainman.ActiveChainstate().IsInitialBlockDownload() && pto->nNextLocalAddrSend < nNow) { + if (pto->IsAddrRelayPeer() && !m_chainman.ActiveChainstate().IsInitialBlockDownload() && pto->m_next_local_addr_send < current_time) { AdvertiseLocal(pto); - pto->nNextLocalAddrSend = PoissonNextSend(nNow, AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL); + pto->m_next_local_addr_send = PoissonNextSend(current_time, AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL); } // // Message: addr // - if (pto->IsAddrRelayPeer() && pto->nNextAddrSend < nNow) { - pto->nNextAddrSend = PoissonNextSend(nNow, AVG_ADDRESS_BROADCAST_INTERVAL); + if (pto->IsAddrRelayPeer() && pto->m_next_addr_send < current_time) { + pto->m_next_addr_send = PoissonNextSend(current_time, AVG_ADDRESS_BROADCAST_INTERVAL); std::vector vAddr; vAddr.reserve(pto->vAddrToSend.size()); diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py new file mode 100755 index 000000000000..1f0012d95f6c --- /dev/null +++ b/test/functional/p2p_addr_relay.py @@ -0,0 +1,69 @@ +#!/usr/bin/env python3 +# Copyright (c) 2020 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +""" +Test addr relay +""" + +from test_framework.messages import ( + CAddress, + NODE_NETWORK, + msg_addr, +) +from test_framework.mininode import ( + P2PInterface, +) +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, +) +import time + +ADDRS = [] +for i in range(10): + addr = CAddress() + addr.time = int(time.time()) + i + addr.nServices = NODE_NETWORK + addr.ip = "123.123.123.{}".format(i % 256) + addr.port = 8333 + i + ADDRS.append(addr) + + +class AddrReceiver(P2PInterface): + def on_addr(self, message): + for addr in message.addrs: + assert_equal(addr.nServices, 9) + assert addr.ip.startswith('123.123.123.') + assert (8333 <= addr.port < 8343) + + +class AddrTest(BitcoinTestFramework): + def set_test_params(self): + self.setup_clean_chain = False + self.num_nodes = 1 + + def run_test(self): + self.log.info('Create connection that sends addr messages') + addr_source = self.nodes[0].add_p2p_connection(P2PInterface()) + msg = msg_addr() + + self.log.info('Send too large addr message') + msg.addrs = ADDRS * 101 + with self.nodes[0].assert_debug_log(['addr message size = 1010']): + addr_source.send_and_ping(msg) + + self.log.info('Check that addr message content is relayed and added to addrman') + addr_receiver = self.nodes[0].add_p2p_connection(AddrReceiver()) + msg.addrs = ADDRS + with self.nodes[0].assert_debug_log([ + 'Added 10 addresses from 127.0.0.1: 0 tried', + 'received: addr (301 bytes) peer=0', + ]): + addr_source.send_and_ping(msg) + self.nodes[0].setmocktime(int(time.time()) + 30 * 60) + addr_receiver.sync_with_ping() + + +if __name__ == '__main__': + AddrTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index ecb7038feb24..71d09fdb55ce 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -174,6 +174,7 @@ 'rpc_blockchain.py', 'rpc_deprecated.py', 'wallet_disable.py', + 'p2p_addr_relay.py', 'p2p_getaddr_caching.py', 'p2p_getdata.py', 'rpc_net.py',