From 7ca8214a16e323e94e76fce6280f4ec67b7c7a5c Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com> Date: Sun, 5 Feb 2023 11:21:12 +0000 Subject: [PATCH 1/9] refactor: replace references to feeEstimator global with references --- src/coinjoin/client.cpp | 37 +++++++++++++++--------------- src/coinjoin/client.h | 17 +++++++------- src/coinjoin/util.cpp | 4 ++-- src/coinjoin/util.h | 3 ++- src/init.cpp | 2 +- src/rpc/coinjoin.cpp | 2 +- src/wallet/test/coinjoin_tests.cpp | 4 ++-- 7 files changed, 35 insertions(+), 34 deletions(-) diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp index 01942d5f68..bbb4531f67 100644 --- a/src/coinjoin/client.cpp +++ b/src/coinjoin/client.cpp @@ -756,7 +756,7 @@ bool CCoinJoinClientManager::CheckAutomaticBackup() // // Passively run mixing in the background to mix funds based on the given configuration. // -bool CCoinJoinClientSession::DoAutomaticDenominating(CTxMemPool& mempool, CConnman& connman, bool fDryRun) +bool CCoinJoinClientSession::DoAutomaticDenominating(CConnman& connman, CBlockPolicyEstimator& fee_estimator, CTxMemPool& mempool, bool fDryRun) { if (fMasternodeMode) return false; // no client-side mixing on masternodes if (nState != POOL_STATE_IDLE) return false; @@ -875,12 +875,12 @@ bool CCoinJoinClientSession::DoAutomaticDenominating(CTxMemPool& mempool, CConnm // there are funds to denominate and denominated balance does not exceed // max amount to mix yet. if (nBalanceAnonimizableNonDenom >= nValueMin + CCoinJoin::GetCollateralAmount() && nBalanceToDenominate > 0) { - CreateDenominated(nBalanceToDenominate); + CreateDenominated(fee_estimator, nBalanceToDenominate); } //check if we have the collateral sized inputs if (!mixingWallet.HasCollateralInputs()) { - return !mixingWallet.HasCollateralInputs(false) && MakeCollateralAmounts(); + return !mixingWallet.HasCollateralInputs(false) && MakeCollateralAmounts(fee_estimator); } if (nSessionID) { @@ -936,7 +936,7 @@ bool CCoinJoinClientSession::DoAutomaticDenominating(CTxMemPool& mempool, CConnm return false; } -bool CCoinJoinClientManager::DoAutomaticDenominating(CTxMemPool& mempool, CConnman& connman, bool fDryRun) +bool CCoinJoinClientManager::DoAutomaticDenominating(CConnman& connman, CBlockPolicyEstimator& fee_estimator, CTxMemPool& mempool, bool fDryRun) { if (fMasternodeMode) return false; // no client-side mixing on masternodes if (!CCoinJoinClientOptions::IsEnabled() || !IsMixing()) return false; @@ -978,7 +978,7 @@ bool CCoinJoinClientManager::DoAutomaticDenominating(CTxMemPool& mempool, CConnm return false; } - fResult &= session.DoAutomaticDenominating(mempool, connman, fDryRun); + fResult &= session.DoAutomaticDenominating(connman, fee_estimator, mempool, fDryRun); } return fResult; @@ -1372,7 +1372,7 @@ bool CCoinJoinClientSession::PrepareDenominate(int nMinRounds, int nMaxRounds, s } // Create collaterals by looping through inputs grouped by addresses -bool CCoinJoinClientSession::MakeCollateralAmounts() +bool CCoinJoinClientSession::MakeCollateralAmounts(const CBlockPolicyEstimator& fee_estimator) { if (!CCoinJoinClientOptions::IsEnabled()) return false; @@ -1395,13 +1395,13 @@ bool CCoinJoinClientSession::MakeCollateralAmounts() // First try to use only non-denominated funds for (const auto& item : vecTally) { - if (!MakeCollateralAmounts(item, false)) continue; + if (!MakeCollateralAmounts(fee_estimator, item, false)) continue; return true; } // There should be at least some denominated funds we should be able to break in pieces to continue mixing for (const auto& item : vecTally) { - if (!MakeCollateralAmounts(item, true)) continue; + if (!MakeCollateralAmounts(fee_estimator, item, true)) continue; return true; } @@ -1411,7 +1411,7 @@ bool CCoinJoinClientSession::MakeCollateralAmounts() } // Split up large inputs or create fee sized inputs -bool CCoinJoinClientSession::MakeCollateralAmounts(const CompactTallyItem& tallyItem, bool fTryDenominated) +bool CCoinJoinClientSession::MakeCollateralAmounts(const CBlockPolicyEstimator& fee_estimator, const CompactTallyItem& tallyItem, bool fTryDenominated) { AssertLockHeld(mixingWallet.cs_wallet); @@ -1434,7 +1434,7 @@ bool CCoinJoinClientSession::MakeCollateralAmounts(const CompactTallyItem& tally return false; } - CTransactionBuilder txBuilder(pwallet, tallyItem); + CTransactionBuilder txBuilder(pwallet, tallyItem, fee_estimator); LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::%s -- Start %s\n", __func__, txBuilder.ToString()); @@ -1553,7 +1553,7 @@ bool CCoinJoinClientSession::CreateCollateralTransaction(CMutableTransaction& tx } // Create denominations by looping through inputs grouped by addresses -bool CCoinJoinClientSession::CreateDenominated(CAmount nBalanceToDenominate) +bool CCoinJoinClientSession::CreateDenominated(CBlockPolicyEstimator& fee_estimator, CAmount nBalanceToDenominate) { if (!CCoinJoinClientOptions::IsEnabled()) return false; @@ -1577,7 +1577,7 @@ bool CCoinJoinClientSession::CreateDenominated(CAmount nBalanceToDenominate) bool fCreateMixingCollaterals = !mixingWallet.HasCollateralInputs(); for (const auto& item : vecTally) { - if (!CreateDenominated(nBalanceToDenominate, item, fCreateMixingCollaterals)) continue; + if (!CreateDenominated(fee_estimator, nBalanceToDenominate, item, fCreateMixingCollaterals)) continue; return true; } @@ -1586,7 +1586,7 @@ bool CCoinJoinClientSession::CreateDenominated(CAmount nBalanceToDenominate) } // Create denominations -bool CCoinJoinClientSession::CreateDenominated(CAmount nBalanceToDenominate, const CompactTallyItem& tallyItem, bool fCreateMixingCollaterals) +bool CCoinJoinClientSession::CreateDenominated(CBlockPolicyEstimator& fee_estimator, CAmount nBalanceToDenominate, const CompactTallyItem& tallyItem, bool fCreateMixingCollaterals) { AssertLockHeld(mixingWallet.cs_wallet); @@ -1604,7 +1604,7 @@ bool CCoinJoinClientSession::CreateDenominated(CAmount nBalanceToDenominate, con return false; } - CTransactionBuilder txBuilder(pwallet, tallyItem); + CTransactionBuilder txBuilder(pwallet, tallyItem, fee_estimator); LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::%s -- Start %s\n", __func__, txBuilder.ToString()); @@ -1815,7 +1815,7 @@ void CCoinJoinClientQueueManager::DoMaintenance() CheckQueue(); } -void CCoinJoinClientManager::DoMaintenance(CTxMemPool& mempool, CConnman& connman) +void CCoinJoinClientManager::DoMaintenance(CConnman& connman, CBlockPolicyEstimator& fee_estimator, CTxMemPool& mempool) { if (!CCoinJoinClientOptions::IsEnabled()) return; if (m_mn_sync == nullptr) return; @@ -1830,7 +1830,7 @@ void CCoinJoinClientManager::DoMaintenance(CTxMemPool& mempool, CConnman& connma CheckTimeout(); ProcessPendingDsaRequest(connman); if (nDoAutoNextRun == nTick) { - DoAutomaticDenominating(mempool, connman); + DoAutomaticDenominating(connman, fee_estimator, mempool); nDoAutoNextRun = nTick + COINJOIN_AUTO_TIMEOUT_MIN + GetRandInt(COINJOIN_AUTO_TIMEOUT_MAX - COINJOIN_AUTO_TIMEOUT_MIN); } } @@ -1867,14 +1867,13 @@ void CCoinJoinClientManager::GetJsonInfo(UniValue& obj) const obj.pushKV("sessions", arrSessions); } -void DoCoinJoinMaintenance(CTxMemPool& mempool, CConnman& connman) +void DoCoinJoinMaintenance(CConnman& connman, CBlockPolicyEstimator& fee_estimator, CTxMemPool& mempool) { if (coinJoinClientQueueManager != nullptr) { coinJoinClientQueueManager->DoMaintenance(); } for (const auto& pair : coinJoinClientManagers) { - pair.second->DoMaintenance(mempool, connman); + pair.second->DoMaintenance(connman, fee_estimator, mempool); } } - diff --git a/src/coinjoin/client.h b/src/coinjoin/client.h index c981445f18..2ec7406201 100644 --- a/src/coinjoin/client.h +++ b/src/coinjoin/client.h @@ -18,6 +18,7 @@ using CDeterministicMNCPtr = std::shared_ptr; class CCoinJoinClientManager; class CCoinJoinClientQueueManager; +class CBlockPolicyEstimator; class CConnman; class CNode; class CTxMemPool; @@ -88,12 +89,12 @@ private: CWallet& mixingWallet; /// Create denominations - bool CreateDenominated(CAmount nBalanceToDenominate); - bool CreateDenominated(CAmount nBalanceToDenominate, const CompactTallyItem& tallyItem, bool fCreateMixingCollaterals); + bool CreateDenominated(CBlockPolicyEstimator& fee_estimator, CAmount nBalanceToDenominate); + bool CreateDenominated(CBlockPolicyEstimator& fee_estimator, CAmount nBalanceToDenominate, const CompactTallyItem& tallyItem, bool fCreateMixingCollaterals); /// Split up large inputs or make fee sized inputs - bool MakeCollateralAmounts(); - bool MakeCollateralAmounts(const CompactTallyItem& tallyItem, bool fTryDenominated); + bool MakeCollateralAmounts(const CBlockPolicyEstimator& fee_estimator); + bool MakeCollateralAmounts(const CBlockPolicyEstimator& fee_estimator, const CompactTallyItem& tallyItem, bool fTryDenominated); bool CreateCollateralTransaction(CMutableTransaction& txCollateral, std::string& strReason); @@ -138,7 +139,7 @@ public: bool GetMixingMasternodeInfo(CDeterministicMNCPtr& ret) const; /// Passively run mixing in the background according to the configuration in settings - bool DoAutomaticDenominating(CTxMemPool& mempool, CConnman& connman, bool fDryRun = false) LOCKS_EXCLUDED(cs_coinjoin); + bool DoAutomaticDenominating(CConnman& connman, CBlockPolicyEstimator& fee_estimator, CTxMemPool& mempool, bool fDryRun = false) LOCKS_EXCLUDED(cs_coinjoin); /// As a client, submit part of a future mixing transaction to a Masternode to start the process bool SubmitDenominate(CConnman& connman); @@ -221,7 +222,7 @@ public: bool GetMixingMasternodesInfo(std::vector& vecDmnsRet) const LOCKS_EXCLUDED(cs_deqsessions); /// Passively run mixing in the background according to the configuration in settings - bool DoAutomaticDenominating(CTxMemPool& mempool, CConnman& connman, bool fDryRun = false) LOCKS_EXCLUDED(cs_deqsessions); + bool DoAutomaticDenominating(CConnman& connman, CBlockPolicyEstimator& fee_estimator, CTxMemPool& mempool, bool fDryRun = false) LOCKS_EXCLUDED(cs_deqsessions); bool TrySubmitDenominate(const CService& mnAddr, CConnman& connman) LOCKS_EXCLUDED(cs_deqsessions); bool MarkAlreadyJoinedQueueAsTried(CCoinJoinQueue& dsq) const LOCKS_EXCLUDED(cs_deqsessions); @@ -237,12 +238,12 @@ public: void UpdatedBlockTip(const CBlockIndex* pindex); - void DoMaintenance(CTxMemPool& mempool, CConnman& connman); + void DoMaintenance(CConnman& connman, CBlockPolicyEstimator& fee_estimator, CTxMemPool& mempool); void GetJsonInfo(UniValue& obj) const LOCKS_EXCLUDED(cs_deqsessions); }; -void DoCoinJoinMaintenance(CTxMemPool& mempool, CConnman& connman); +void DoCoinJoinMaintenance(CConnman& connman, CBlockPolicyEstimator& fee_estimator, CTxMemPool& mempool); #endif // BITCOIN_COINJOIN_CLIENT_H diff --git a/src/coinjoin/util.cpp b/src/coinjoin/util.cpp index 19fa47369c..58ad46aa48 100644 --- a/src/coinjoin/util.cpp +++ b/src/coinjoin/util.cpp @@ -107,7 +107,7 @@ bool CTransactionBuilderOutput::UpdateAmount(const CAmount nNewAmount) return true; } -CTransactionBuilder::CTransactionBuilder(std::shared_ptr pwalletIn, const CompactTallyItem& tallyItemIn) : +CTransactionBuilder::CTransactionBuilder(std::shared_ptr pwalletIn, const CompactTallyItem& tallyItemIn, const CBlockPolicyEstimator& fee_estimator) : pwallet(pwalletIn), dummyReserveDestination(pwalletIn.get()), tallyItem(tallyItemIn) @@ -115,7 +115,7 @@ CTransactionBuilder::CTransactionBuilder(std::shared_ptr pwalletIn, con // Generate a feerate which will be used to consider if the remainder is dust and will go into fees or not coinControl.m_discard_feerate = ::GetDiscardRate(*pwallet); // Generate a feerate which will be used by calculations of this class and also by CWallet::CreateTransaction - coinControl.m_feerate = std::max(::feeEstimator.estimateSmartFee(int(pwallet->m_confirm_target), nullptr, true), pwallet->m_pay_tx_fee); + coinControl.m_feerate = std::max(fee_estimator.estimateSmartFee(int(pwallet->m_confirm_target), nullptr, true), pwallet->m_pay_tx_fee); // Change always goes back to origin coinControl.destChange = tallyItemIn.txdest; // Only allow tallyItems inputs for tx creation diff --git a/src/coinjoin/util.h b/src/coinjoin/util.h index 4a4c8ed3da..26c17b5a22 100644 --- a/src/coinjoin/util.h +++ b/src/coinjoin/util.h @@ -7,6 +7,7 @@ #include +class CBlockPolicyEstimator; class CTransactionBuilder; struct bilingual_str; @@ -100,7 +101,7 @@ class CTransactionBuilder friend class CTransactionBuilderOutput; public: - CTransactionBuilder(std::shared_ptr pwalletIn, const CompactTallyItem& tallyItemIn); + CTransactionBuilder(std::shared_ptr pwalletIn, const CompactTallyItem& tallyItemIn, const CBlockPolicyEstimator& fee_estimator); ~CTransactionBuilder(); /// Check it would be possible to add a single output with the amount nAmount. Returns true if its possible and false if not. bool CouldAddOutput(CAmount nAmountOutput) const; diff --git a/src/init.cpp b/src/init.cpp index 1fccb986dd..9001e6cbcd 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -2397,7 +2397,7 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc node.scheduler->scheduleEvery(std::bind(&llmq::CDKGSessionManager::CleanupOldContributions, std::ref(*node.llmq_ctx->qdkgsman)), std::chrono::hours{1}); #ifdef ENABLE_WALLET } else if(CCoinJoinClientOptions::IsEnabled()) { - node.scheduler->scheduleEvery(std::bind(&DoCoinJoinMaintenance, std::ref(*node.mempool), std::ref(*node.connman)), std::chrono::seconds{1}); + node.scheduler->scheduleEvery(std::bind(&DoCoinJoinMaintenance, std::ref(*node.connman), std::ref(::feeEstimator), std::ref(*node.mempool)), std::chrono::seconds{1}); #endif // ENABLE_WALLET } diff --git a/src/rpc/coinjoin.cpp b/src/rpc/coinjoin.cpp index 2db0a80aeb..99662ff71a 100644 --- a/src/rpc/coinjoin.cpp +++ b/src/rpc/coinjoin.cpp @@ -64,7 +64,7 @@ static UniValue coinjoin(const JSONRPCRequest& request) const NodeContext& node = EnsureNodeContext(request.context); CTxMemPool& mempool = EnsureMemPool(request.context); - bool result = it->second->DoAutomaticDenominating(mempool, *node.connman); + bool result = it->second->DoAutomaticDenominating(*node.connman, ::feeEstimator, mempool); return "Mixing " + (result ? "started successfully" : ("start failed: " + it->second->GetStatuses().original + ", will retry")); } diff --git a/src/wallet/test/coinjoin_tests.cpp b/src/wallet/test/coinjoin_tests.cpp index c42c6da8ef..00bcaea342 100644 --- a/src/wallet/test/coinjoin_tests.cpp +++ b/src/wallet/test/coinjoin_tests.cpp @@ -229,7 +229,7 @@ BOOST_FIXTURE_TEST_CASE(CTransactionBuilderTest, CTransactionBuilderTestSetup) // Tests with single outpoint tallyItem { CompactTallyItem tallyItem = GetTallyItem({4999}); - CTransactionBuilder txBuilder(wallet, tallyItem); + CTransactionBuilder txBuilder(wallet, tallyItem, ::feeEstimator); BOOST_CHECK_EQUAL(txBuilder.CountOutputs(), 0); BOOST_CHECK_EQUAL(txBuilder.GetAmountInitial(), tallyItem.nAmount); @@ -266,7 +266,7 @@ BOOST_FIXTURE_TEST_CASE(CTransactionBuilderTest, CTransactionBuilderTestSetup) // Tests with multiple outpoint tallyItem { CompactTallyItem tallyItem = GetTallyItem({10000, 20000, 30000, 40000, 50000}); - CTransactionBuilder txBuilder(wallet, tallyItem); + CTransactionBuilder txBuilder(wallet, tallyItem, ::feeEstimator); std::vector vecOutputs; bilingual_str strResult; From a5f20129fb8245381cabfc5176225f4488d80a40 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com> Date: Mon, 20 Feb 2023 19:31:40 +0000 Subject: [PATCH 2/9] merge bitcoin#20222: CTxMempool constructor clean up --- src/init.cpp | 11 ++--------- src/test/util/setup_common.cpp | 3 +-- src/txmempool.cpp | 21 +++++++-------------- src/txmempool.h | 17 +++++++++++------ 4 files changed, 21 insertions(+), 31 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 9001e6cbcd..fc35a4e8b9 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1766,16 +1766,9 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc assert(!node.connman); node.connman = std::make_unique(GetRand(std::numeric_limits::max()), GetRand(std::numeric_limits::max())); - // Make mempool generally available in the node context. For example the connection manager, wallet, or RPC threads, - // which are all started after this, may use it from the node context. assert(!node.mempool); - node.mempool = std::make_unique(&::feeEstimator); - if (node.mempool) { - int ratio = std::min(std::max(args.GetArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0), 1000000); - if (ratio != 0) { - node.mempool->setSanityCheck(1.0 / ratio); - } - } + int check_ratio = std::min(std::max(args.GetArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0), 1000000); + node.mempool = std::make_unique(node.fee_estimator.get(), check_ratio); assert(!node.chainman); node.chainman = &g_chainman; diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 7cd8e6c8c4..4e4afe6713 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -169,8 +169,7 @@ ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::ve pblocktree.reset(new CBlockTreeDB(1 << 20, true)); - m_node.mempool = std::make_unique(&::feeEstimator); - m_node.mempool->setSanityCheck(1.0); + m_node.mempool = std::make_unique(&::feeEstimator, 1); m_node.chainman = &::g_chainman; diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 199f3dea3d..77e5ef90f1 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -334,15 +334,10 @@ void CTxMemPoolEntry::UpdateAncestorState(int64_t modifySize, CAmount modifyFee, assert(int(nSigOpCountWithAncestors) >= 0); } -CTxMemPool::CTxMemPool(CBlockPolicyEstimator* estimator) - : nTransactionsUpdated(0), minerPolicyEstimator(estimator), m_epoch(0), m_has_epoch_guard(false) +CTxMemPool::CTxMemPool(CBlockPolicyEstimator* estimator, int check_ratio) + : m_check_ratio(check_ratio), minerPolicyEstimator(estimator) { _clear(); //lock free clear - - // Sanity checks off by default for performance, because otherwise - // accepting transactions becomes O(N^2) where N is the number - // of transactions in the pool - nCheckFrequency = 0; } bool CTxMemPool::isSpent(const COutPoint& outpoint) const @@ -776,7 +771,7 @@ void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMem if (it2 != mapTx.end()) continue; const Coin &coin = pcoins->AccessCoin(txin.prevout); - if (nCheckFrequency != 0) assert(!coin.IsSpent()); + if (m_check_ratio != 0) assert(!coin.IsSpent()); if (coin.IsSpent() || (coin.IsCoinBase() && ((signed long)nMemPoolHeight) - coin.nHeight < COINBASE_MATURITY)) { txToRemove.insert(it); break; @@ -1035,13 +1030,11 @@ static void CheckInputsAndUpdateCoins(const CTransaction& tx, CCoinsViewCache& m void CTxMemPool::check(const CCoinsViewCache *pcoins) const { + if (m_check_ratio == 0) return; + + if (GetRand(m_check_ratio) >= 1) return; + LOCK(cs); - if (nCheckFrequency == 0) - return; - - if (GetRand(std::numeric_limits::max()) >= nCheckFrequency) - return; - LogPrint(BCLog::MEMPOOL, "Checking mempool with %u transactions and %u inputs\n", (unsigned int)mapTx.size(), (unsigned int)mapNextTx.size()); uint64_t checkTotal = 0; diff --git a/src/txmempool.h b/src/txmempool.h index 58c609dd04..23d9b6a297 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -449,8 +449,8 @@ public: class CTxMemPool { private: - uint32_t nCheckFrequency GUARDED_BY(cs); //!< Value n means that n times in 2^32 we check. - std::atomic nTransactionsUpdated; //!< Used by getblocktemplate to trigger CreateNewBlock() invocation + const int m_check_ratio; //!< Value n means that 1 times in n we check. + std::atomic nTransactionsUpdated{0}; //!< Used by getblocktemplate to trigger CreateNewBlock() invocation CBlockPolicyEstimator* minerPolicyEstimator; uint64_t totalTxSize; //!< sum of all mempool tx' byte sizes @@ -459,8 +459,8 @@ private: mutable int64_t lastRollingFeeUpdate; mutable bool blockSinceLastRollingFeeBump; mutable double rollingMinimumFeeRate; //!< minimum fee to get into the pool, decreases exponentially - mutable uint64_t m_epoch; - mutable bool m_has_epoch_guard; + mutable uint64_t m_epoch{0}; + mutable bool m_has_epoch_guard{false}; void trackPackageRemoved(const CFeeRate& rate) EXCLUSIVE_LOCKS_REQUIRED(cs); @@ -581,8 +581,14 @@ public: std::map mapDeltas; /** Create a new CTxMemPool. + * Sanity checks will be off by default for performance, because otherwise + * accepting transactions becomes O(N^2) where N is the number of transactions + * in the pool. + * + * @param[in] estimator is used to estimate appropriate transaction fees. + * @param[in] check_ratio is the ratio used to determine how often sanity checks will run. */ - explicit CTxMemPool(CBlockPolicyEstimator* estimator = nullptr); + explicit CTxMemPool(CBlockPolicyEstimator* estimator = nullptr, int check_ratio = 0); /** * If sanity-checking is turned on, check makes sure the pool is @@ -591,7 +597,6 @@ public: * check does nothing. */ void check(const CCoinsViewCache *pcoins) const; - void setSanityCheck(double dFrequency = 1.0) { LOCK(cs); nCheckFrequency = static_cast(dFrequency * 4294967295.0); } // addUnchecked must updated state for all ancestors of a given transaction, // to track size/count of descendant transactions. First version of From 2d2814e5fa648b44166bc3593034574aaca19878 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com> Date: Wed, 22 Feb 2023 07:53:20 +0000 Subject: [PATCH 3/9] merge bitcoin#18766: disable fee estimation in blocksonly mode (by removing the fee estimates global) --- src/context.h | 2 + src/init.cpp | 47 +++++++++-------------- src/interfaces/chain.cpp | 6 ++- src/interfaces/node.cpp | 9 ----- src/interfaces/node.h | 3 -- src/node/context.cpp | 1 + src/node/context.h | 2 + src/policy/fees.cpp | 19 +++++++++ src/policy/fees.h | 3 ++ src/rpc/blockchain.cpp | 10 +++++ src/rpc/blockchain.h | 4 +- src/rpc/coinjoin.cpp | 2 +- src/rpc/mining.cpp | 16 +++++--- src/test/util/setup_common.cpp | 4 +- src/validation.cpp | 3 -- src/validation.h | 2 - src/wallet/test/coinjoin_tests.cpp | 7 ++-- src/wallet/test/wallet_tests.cpp | 15 +++++--- test/functional/feature_fee_estimation.py | 7 ++++ test/lint/lint-circular-dependencies.sh | 1 - 20 files changed, 98 insertions(+), 65 deletions(-) diff --git a/src/context.h b/src/context.h index 7b1efeac62..a2d0c3ac7b 100644 --- a/src/context.h +++ b/src/context.h @@ -10,6 +10,7 @@ class ChainstateManager; class CTxMemPool; +class CBlockPolicyEstimator; struct LLMQContext; struct NodeContext; struct WalletContext; @@ -19,6 +20,7 @@ using CoreContext = std::variant, std::reference_wrapper, std::reference_wrapper, + std::reference_wrapper, std::reference_wrapper>; template diff --git a/src/init.cpp b/src/init.cpp index fc35a4e8b9..04924694fc 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -120,7 +120,6 @@ #include #endif -static bool fFeeEstimatesInitialized = false; static const bool DEFAULT_PROXYRANDOMIZE = true; static const bool DEFAULT_REST_ENABLE = false; static const bool DEFAULT_STOPAFTERBLOCKIMPORT = false; @@ -136,8 +135,6 @@ static CDSNotificationInterface* pdsNotificationInterface = nullptr; #define MIN_CORE_FILEDESCRIPTORS 150 #endif -static const char* FEE_ESTIMATES_FILENAME="fee_estimates.dat"; - static const char* DEFAULT_ASMAP_FILENAME="ip_asn.map"; /** * The PID file facilities. @@ -286,17 +283,8 @@ void PrepareShutdown(NodeContext& node) DumpMempool(*node.mempool); } - if (fFeeEstimatesInitialized) - { - ::feeEstimator.FlushUnconfirmed(); - fs::path est_path = GetDataDir() / FEE_ESTIMATES_FILENAME; - CAutoFile est_fileout(fsbridge::fopen(est_path, "wb"), SER_DISK, CLIENT_VERSION); - if (!est_fileout.IsNull()) - ::feeEstimator.Write(est_fileout); - else - LogPrintf("%s: Failed to write fee estimates to %s\n", __func__, est_path.string()); - fFeeEstimatesInitialized = false; - } + // Drop transactions we were still watching, and record fee estimations. + if (node.fee_estimator) node.fee_estimator->Flush(); // FlushStateToDisk generates a ChainStateFlushed callback, which we should avoid missing if (node.chainman) { @@ -405,6 +393,7 @@ void Shutdown(NodeContext& node) globalVerifyHandle.reset(); ECC_Stop(); node.mempool.reset(); + node.fee_estimator.reset(); node.chainman = nullptr; node.scheduler.reset(); @@ -1761,11 +1750,21 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc // is not yet setup and may end up being set up twice if we // need to reindex later. + // see Step 2: parameter interactions for more information about these + fListen = args.GetBoolArg("-listen", DEFAULT_LISTEN); + fDiscover = args.GetBoolArg("-discover", true); + g_relay_txes = !args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY); + assert(!node.banman); node.banman = std::make_unique(GetDataDir() / "banlist.dat", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME)); assert(!node.connman); node.connman = std::make_unique(GetRand(std::numeric_limits::max()), GetRand(std::numeric_limits::max())); + assert(!node.fee_estimator); + // Don't initialize fee estimation with old data if we don't relay transactions, + // as they would never get updated. + if (g_relay_txes) node.fee_estimator = std::make_unique(); + assert(!node.mempool); int check_ratio = std::min(std::max(args.GetArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0), 1000000); node.mempool = std::make_unique(node.fee_estimator.get(), check_ratio); @@ -1887,11 +1886,6 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc } } - // see Step 2: parameter interactions for more information about these - fListen = args.GetBoolArg("-listen", DEFAULT_LISTEN); - fDiscover = args.GetBoolArg("-discover", true); - g_relay_txes = !args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY); - for (const std::string& strAddr : args.GetArgs("-externalip")) { CService addrLocal; if (Lookup(strAddr, addrLocal, GetListenPort(), fNameLookup) && addrLocal.IsValid()) @@ -2257,13 +2251,6 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc return false; } - fs::path est_path = GetDataDir() / FEE_ESTIMATES_FILENAME; - CAutoFile est_filein(fsbridge::fopen(est_path, "rb"), SER_DISK, CLIENT_VERSION); - // Allowed to fail as this file IS missing on first startup. - if (!est_filein.IsNull()) - ::feeEstimator.Read(est_filein); - fFeeEstimatesInitialized = true; - // ********************************************************* Step 8: start indexers if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) { g_txindex = std::make_unique(nTxIndexCache, false, fReindex); @@ -2318,7 +2305,9 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc ::coinJoinServer = std::make_unique(*node.mempool, *node.connman, ::masternodeSync); #ifdef ENABLE_WALLET - ::coinJoinClientQueueManager = std::make_unique(*node.connman, ::masternodeSync); + if (g_relay_txes) { + ::coinJoinClientQueueManager = std::make_unique(*node.connman, ::masternodeSync); + } #endif // ENABLE_WALLET g_wallet_init_interface.InitCoinJoinSettings(); @@ -2389,8 +2378,8 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc node.scheduler->scheduleEvery(std::bind(&CCoinJoinServer::DoMaintenance, std::ref(*::coinJoinServer)), std::chrono::seconds{1}); node.scheduler->scheduleEvery(std::bind(&llmq::CDKGSessionManager::CleanupOldContributions, std::ref(*node.llmq_ctx->qdkgsman)), std::chrono::hours{1}); #ifdef ENABLE_WALLET - } else if(CCoinJoinClientOptions::IsEnabled()) { - node.scheduler->scheduleEvery(std::bind(&DoCoinJoinMaintenance, std::ref(*node.connman), std::ref(::feeEstimator), std::ref(*node.mempool)), std::chrono::seconds{1}); + } else if (g_relay_txes && CCoinJoinClientOptions::IsEnabled()) { + node.scheduler->scheduleEvery(std::bind(&DoCoinJoinMaintenance, std::ref(*node.connman), std::ref(*node.fee_estimator), std::ref(*node.mempool)), std::chrono::seconds{1}); #endif // ENABLE_WALLET } diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index fc15d1b703..b723c94848 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -312,11 +312,13 @@ public: } CFeeRate estimateSmartFee(int num_blocks, bool conservative, FeeCalculation* calc) override { - return ::feeEstimator.estimateSmartFee(num_blocks, calc, conservative); + if (!m_node.fee_estimator) return {}; + return m_node.fee_estimator->estimateSmartFee(num_blocks, calc, conservative); } unsigned int estimateMaxBlocks() override { - return ::feeEstimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE); + if (!m_node.fee_estimator) return 0; + return m_node.fee_estimator->HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE); } CFeeRate mempoolMinFee() override { diff --git a/src/interfaces/node.cpp b/src/interfaces/node.cpp index 8f69b14ae5..0b26159f46 100644 --- a/src/interfaces/node.cpp +++ b/src/interfaces/node.cpp @@ -332,15 +332,6 @@ public: } } bool getNetworkActive() override { return m_context->connman && m_context->connman->GetNetworkActive(); } - CFeeRate estimateSmartFee(int num_blocks, bool conservative, int* returned_target = nullptr) override - { - FeeCalculation fee_calc; - CFeeRate result = ::feeEstimator.estimateSmartFee(num_blocks, &fee_calc, conservative); - if (returned_target) { - *returned_target = fee_calc.returnedTarget; - } - return result; - } CFeeRate getDustRelayFee() override { return ::dustRelayFee; } UniValue executeRpc(const std::string& command, const UniValue& params, const std::string& uri) override { diff --git a/src/interfaces/node.h b/src/interfaces/node.h index 4b9f779f43..6d145f2a6a 100644 --- a/src/interfaces/node.h +++ b/src/interfaces/node.h @@ -253,9 +253,6 @@ public: //! Get network active. virtual bool getNetworkActive() = 0; - //! Estimate smart fee. - virtual CFeeRate estimateSmartFee(int num_blocks, bool conservative, int* returned_target = nullptr) = 0; - //! Get dust relay fee. virtual CFeeRate getDustRelayFee() = 0; diff --git a/src/node/context.cpp b/src/node/context.cpp index 4ac3f776de..cf38c28898 100644 --- a/src/node/context.cpp +++ b/src/node/context.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include diff --git a/src/node/context.h b/src/node/context.h index cb6afa7ebc..f0ec081cac 100644 --- a/src/node/context.h +++ b/src/node/context.h @@ -12,6 +12,7 @@ class ArgsManager; class BanMan; +class CBlockPolicyEstimator; class CConnman; class CScheduler; class CTxMemPool; @@ -38,6 +39,7 @@ class WalletClient; struct NodeContext { std::unique_ptr connman; std::unique_ptr mempool; + std::unique_ptr fee_estimator; std::unique_ptr peer_logic; ChainstateManager* chainman{nullptr}; // Currently a raw pointer because the memory is not managed by this struct std::unique_ptr banman; diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 67950cedca..6530448b41 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -10,6 +10,8 @@ #include #include +static const char* FEE_ESTIMATES_FILENAME="fee_estimates.dat"; + static constexpr double INF_FEERATE = 1e99; std::string StringForFeeEstimateHorizon(FeeEstimateHorizon horizon) { @@ -488,6 +490,7 @@ CBlockPolicyEstimator::CBlockPolicyEstimator() { static_assert(MIN_BUCKET_FEERATE > 0, "Min feerate must be nonzero"); size_t bucketIndex = 0; + for (double bucketBoundary = MIN_BUCKET_FEERATE; bucketBoundary <= MAX_BUCKET_FEERATE; bucketBoundary *= FEE_SPACING, bucketIndex++) { buckets.push_back(bucketBoundary); bucketMap[bucketBoundary] = bucketIndex; @@ -499,6 +502,13 @@ CBlockPolicyEstimator::CBlockPolicyEstimator() feeStats = std::make_unique(buckets, bucketMap, MED_BLOCK_PERIODS, MED_DECAY, MED_SCALE); shortStats = std::make_unique(buckets, bucketMap, SHORT_BLOCK_PERIODS, SHORT_DECAY, SHORT_SCALE); longStats = std::make_unique(buckets, bucketMap, LONG_BLOCK_PERIODS, LONG_DECAY, LONG_SCALE); + + // If the fee estimation file is present, read recorded estimations + fs::path est_filepath = GetDataDir() / FEE_ESTIMATES_FILENAME; + CAutoFile est_file(fsbridge::fopen(est_filepath, "rb"), SER_DISK, CLIENT_VERSION); + if (est_file.IsNull() || !Read(est_file)) { + LogPrintf("Failed to read fee estimates from %s. Continue anyway.\n", est_filepath.string()); + } } CBlockPolicyEstimator::~CBlockPolicyEstimator() @@ -854,6 +864,15 @@ CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget, FeeCalculation return CFeeRate(llround(median)); } +void CBlockPolicyEstimator::Flush() { + FlushUnconfirmed(); + + fs::path est_filepath = GetDataDir() / FEE_ESTIMATES_FILENAME; + CAutoFile est_file(fsbridge::fopen(est_filepath, "wb"), SER_DISK, CLIENT_VERSION); + if (est_file.IsNull() || !Write(est_file)) { + LogPrintf("Failed to write fee estimates to %s\n", est_filepath.string()); + } +} bool CBlockPolicyEstimator::Write(CAutoFile& fileout) const { diff --git a/src/policy/fees.h b/src/policy/fees.h index de45277852..acbd9dadba 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -222,6 +222,9 @@ public: /** Calculation of highest target that estimates are tracked for */ unsigned int HighestTargetTracked(FeeEstimateHorizon horizon) const; + /** Drop still unconfirmed transactions and record current estimations, if the fee estimation file is present. */ + void Flush(); + private: mutable CCriticalSection m_cs_fee_estimator; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 33dc1feeb0..a054c42a96 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -91,6 +92,15 @@ ChainstateManager& EnsureChainman(const CoreContext& context) return *node.chainman; } +CBlockPolicyEstimator& EnsureFeeEstimator(const CoreContext& context) +{ + NodeContext& node = EnsureNodeContext(context); + if (!node.fee_estimator) { + throw JSONRPCError(RPC_INTERNAL_ERROR, "Fee estimation disabled"); + } + return *node.fee_estimator; +} + LLMQContext& EnsureLLMQContext(const CoreContext& context) { NodeContext& node = EnsureNodeContext(context); diff --git a/src/rpc/blockchain.h b/src/rpc/blockchain.h index cf94c02330..aea23f1b90 100644 --- a/src/rpc/blockchain.h +++ b/src/rpc/blockchain.h @@ -16,6 +16,7 @@ extern RecursiveMutex cs_main; class CBlock; class CBlockIndex; +class CBlockPolicyEstimator; class CTxMemPool; class ChainstateManager; class UniValue; @@ -55,8 +56,9 @@ UniValue blockheaderToJSON(const CBlockIndex* tip, const CBlockIndex* blockindex void CalculatePercentilesBySize(CAmount result[NUM_GETBLOCKSTATS_PERCENTILES], std::vector>& scores, int64_t total_size); NodeContext& EnsureNodeContext(const CoreContext& context); -LLMQContext& EnsureLLMQContext(const CoreContext& context); CTxMemPool& EnsureMemPool(const CoreContext& context); ChainstateManager& EnsureChainman(const CoreContext& context); +CBlockPolicyEstimator& EnsureFeeEstimator(const CoreContext& context); +LLMQContext& EnsureLLMQContext(const CoreContext& context); #endif diff --git a/src/rpc/coinjoin.cpp b/src/rpc/coinjoin.cpp index 99662ff71a..37df23aefe 100644 --- a/src/rpc/coinjoin.cpp +++ b/src/rpc/coinjoin.cpp @@ -64,7 +64,7 @@ static UniValue coinjoin(const JSONRPCRequest& request) const NodeContext& node = EnsureNodeContext(request.context); CTxMemPool& mempool = EnsureMemPool(request.context); - bool result = it->second->DoAutomaticDenominating(*node.connman, ::feeEstimator, mempool); + bool result = it->second->DoAutomaticDenominating(*node.connman, *node.fee_estimator, mempool); return "Mixing " + (result ? "started successfully" : ("start failed: " + it->second->GetStatuses().original + ", will retry")); } diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index de4e5c448d..10da6d0791 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -1096,7 +1096,10 @@ static UniValue estimatesmartfee(const JSONRPCRequest& request) RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VSTR}); RPCTypeCheckArgument(request.params[0], UniValue::VNUM); - unsigned int max_target = ::feeEstimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE); + + CBlockPolicyEstimator& fee_estimator = EnsureFeeEstimator(request.context); + + unsigned int max_target = fee_estimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE); unsigned int conf_target = ParseConfirmTarget(request.params[0], max_target); bool conservative = true; if (!request.params[1].isNull()) { @@ -1110,7 +1113,7 @@ static UniValue estimatesmartfee(const JSONRPCRequest& request) UniValue result(UniValue::VOBJ); UniValue errors(UniValue::VARR); FeeCalculation feeCalc; - CFeeRate feeRate = ::feeEstimator.estimateSmartFee(conf_target, &feeCalc, conservative); + CFeeRate feeRate = fee_estimator.estimateSmartFee(conf_target, &feeCalc, conservative); if (feeRate != CFeeRate(0)) { result.pushKV("feerate", ValueFromAmount(feeRate.GetFeePerK())); } else { @@ -1178,7 +1181,10 @@ static UniValue estimaterawfee(const JSONRPCRequest& request) RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VNUM}, true); RPCTypeCheckArgument(request.params[0], UniValue::VNUM); - unsigned int max_target = ::feeEstimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE); + + CBlockPolicyEstimator& fee_estimator = EnsureFeeEstimator(request.context); + + unsigned int max_target = fee_estimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE); unsigned int conf_target = ParseConfirmTarget(request.params[0], max_target); double threshold = 0.95; if (!request.params[1].isNull()) { @@ -1195,9 +1201,9 @@ static UniValue estimaterawfee(const JSONRPCRequest& request) EstimationResult buckets; // Only output results for horizons which track the target - if (conf_target > ::feeEstimator.HighestTargetTracked(horizon)) continue; + if (conf_target > fee_estimator.HighestTargetTracked(horizon)) continue; - feeRate = ::feeEstimator.estimateRawFee(conf_target, threshold, horizon, &buckets); + feeRate = fee_estimator.estimateRawFee(conf_target, threshold, horizon, &buckets); UniValue horizon_result(UniValue::VOBJ); UniValue errors(UniValue::VARR); UniValue passbucket(UniValue::VOBJ); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 4e4afe6713..9f9a5ea9d0 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -169,7 +170,8 @@ ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::ve pblocktree.reset(new CBlockTreeDB(1 << 20, true)); - m_node.mempool = std::make_unique(&::feeEstimator, 1); + m_node.fee_estimator = std::make_unique(); + m_node.mempool = std::make_unique(m_node.fee_estimator.get(), 1); m_node.chainman = &::g_chainman; diff --git a/src/validation.cpp b/src/validation.cpp index a1a53d9633..46600a9aa2 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -21,7 +21,6 @@ #include #include #include -#include #include #include #include @@ -154,8 +153,6 @@ arith_uint256 nMinimumChainWork; CFeeRate minRelayTxFee = CFeeRate(DEFAULT_MIN_RELAY_TX_FEE); -CBlockPolicyEstimator feeEstimator; - // Internal stuff namespace { CBlockIndex* pindexBestInvalid = nullptr; diff --git a/src/validation.h b/src/validation.h index 60ecde0d03..d79b6ad20f 100644 --- a/src/validation.h +++ b/src/validation.h @@ -50,7 +50,6 @@ class CChainParams; class CInv; class CConnman; class CScriptCheck; -class CBlockPolicyEstimator; class CTxMemPool; class CValidationState; class ChainstateManager; @@ -128,7 +127,6 @@ struct BlockHasher }; extern CCriticalSection cs_main; -extern CBlockPolicyEstimator feeEstimator; typedef std::unordered_map BlockMap; typedef std::unordered_multimap PrevBlockMap; extern Mutex g_best_block_mutex; diff --git a/src/wallet/test/coinjoin_tests.cpp b/src/wallet/test/coinjoin_tests.cpp index 00bcaea342..aa24f0b2cf 100644 --- a/src/wallet/test/coinjoin_tests.cpp +++ b/src/wallet/test/coinjoin_tests.cpp @@ -129,7 +129,8 @@ public: CTransactionBuilderTestSetup() { CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); - node.mempool = std::make_unique(&::feeEstimator); + node.fee_estimator = std::make_unique(); + node.mempool = std::make_unique(node.fee_estimator.get()); chain = interfaces::MakeChain(node); wallet = std::make_unique(chain.get(), "", CreateMockWalletDatabase()); bool firstRun; @@ -229,7 +230,7 @@ BOOST_FIXTURE_TEST_CASE(CTransactionBuilderTest, CTransactionBuilderTestSetup) // Tests with single outpoint tallyItem { CompactTallyItem tallyItem = GetTallyItem({4999}); - CTransactionBuilder txBuilder(wallet, tallyItem, ::feeEstimator); + CTransactionBuilder txBuilder(wallet, tallyItem, *m_node.fee_estimator); BOOST_CHECK_EQUAL(txBuilder.CountOutputs(), 0); BOOST_CHECK_EQUAL(txBuilder.GetAmountInitial(), tallyItem.nAmount); @@ -266,7 +267,7 @@ BOOST_FIXTURE_TEST_CASE(CTransactionBuilderTest, CTransactionBuilderTestSetup) // Tests with multiple outpoint tallyItem { CompactTallyItem tallyItem = GetTallyItem({10000, 20000, 30000, 40000, 50000}); - CTransactionBuilder txBuilder(wallet, tallyItem, ::feeEstimator); + CTransactionBuilder txBuilder(wallet, tallyItem, *m_node.fee_estimator); std::vector vecOutputs; bilingual_str strResult; diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index feb31b5e78..3939f4f46f 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -88,7 +88,8 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) CBlockIndex* newTip = ::ChainActive().Tip(); NodeContext node; - node.mempool = std::make_unique(&::feeEstimator); + node.fee_estimator = std::make_unique(); + node.mempool = std::make_unique(node.fee_estimator.get()); auto chain = interfaces::MakeChain(node); // Verify ScanForWalletTransactions accommodates a null start block. @@ -189,7 +190,8 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup) CBlockIndex* newTip = ::ChainActive().Tip(); NodeContext node; - node.mempool = std::make_unique(&::feeEstimator); + node.fee_estimator = std::make_unique(); + node.mempool = std::make_unique(node.fee_estimator.get()); auto chain = interfaces::MakeChain(node); // Prune the older block file. @@ -260,7 +262,8 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) m_coinbase_txns.emplace_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]); NodeContext node; - node.mempool = std::make_unique(&::feeEstimator); + node.fee_estimator = std::make_unique(); + node.mempool = std::make_unique(node.fee_estimator.get()); auto chain = interfaces::MakeChain(node); std::string backup_file = (GetDataDir() / "wallet.backup").string(); @@ -393,7 +396,8 @@ BOOST_FIXTURE_TEST_CASE(rpc_getaddressinfo, TestChain100Setup) BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup) { NodeContext node; - node.mempool = std::make_unique(&::feeEstimator); + node.fee_estimator = std::make_unique(); + node.mempool = std::make_unique(node.fee_estimator.get()); auto chain = interfaces::MakeChain(node); CWallet wallet(chain.get(), "", CreateDummyWalletDatabase()); @@ -1123,7 +1127,8 @@ BOOST_FIXTURE_TEST_CASE(select_coins_grouped_by_addresses, ListCoinsTestingSetup BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup) { NodeContext node; - node.mempool = std::make_unique(&::feeEstimator); + node.fee_estimator = std::make_unique(); + node.mempool = std::make_unique(node.fee_estimator.get()); auto chain = interfaces::MakeChain(node); std::shared_ptr wallet = std::make_shared(chain.get(), "", CreateDummyWalletDatabase()); wallet->SetMinVersion(FEATURE_LATEST); diff --git a/test/functional/feature_fee_estimation.py b/test/functional/feature_fee_estimation.py index f62823c2f9..d16280ecbd 100755 --- a/test/functional/feature_fee_estimation.py +++ b/test/functional/feature_fee_estimation.py @@ -13,6 +13,7 @@ from test_framework.util import ( assert_equal, assert_greater_than, assert_greater_than_or_equal, + assert_raises_rpc_error, satoshi_round, ) @@ -259,5 +260,11 @@ class EstimateFeeTest(BitcoinTestFramework): self.log.info("Final estimates after emptying mempools") check_estimates(self.nodes[1], self.fees_per_kb) + self.log.info("Testing that fee estimation is disabled in blocksonly.") + self.restart_node(0, ["-blocksonly"]) + assert_raises_rpc_error(-32603, "Fee estimation disabled", + self.nodes[0].estimatesmartfee, 2) + + if __name__ == '__main__': EstimateFeeTest().main() diff --git a/test/lint/lint-circular-dependencies.sh b/test/lint/lint-circular-dependencies.sh index 985e763a86..b6b496d94a 100755 --- a/test/lint/lint-circular-dependencies.sh +++ b/test/lint/lint-circular-dependencies.sh @@ -75,7 +75,6 @@ EXPECTED_CIRCULAR_DEPENDENCIES=( "llmq/chainlocks -> validation -> llmq/chainlocks" "coinjoin/coinjoin -> llmq/chainlocks -> net -> coinjoin/coinjoin" "evo/deterministicmns -> llmq/utils -> net -> evo/deterministicmns" - "policy/fees -> txmempool -> validation -> policy/fees" "policy/policy -> policy/settings -> policy/policy" "evo/specialtxman -> validation -> evo/specialtxman" From cca796aeb3b3d543d23250ba5155910fa0ab3fda Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com> Date: Wed, 22 Feb 2023 07:40:22 +0000 Subject: [PATCH 4/9] merge bitcoin#14033: Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater --- src/net.cpp | 5 ----- src/net.h | 1 - src/net_processing.cpp | 14 +++----------- 3 files changed, 3 insertions(+), 17 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 49adf22924..2d04cfb11b 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3323,11 +3323,6 @@ CConnman::~CConnman() Stop(); } -size_t CConnman::GetAddressCount() const -{ - return addrman.size(); -} - void CConnman::SetServices(const CService &addr, ServiceFlags nServices) { addrman.SetServices(addr, nServices); diff --git a/src/net.h b/src/net.h index a315204e89..bbfcbd8c95 100644 --- a/src/net.h +++ b/src/net.h @@ -399,7 +399,6 @@ public: void RelayInvFiltered(CInv &inv, const uint256 &relatedTxHash, const int minProtoVersion = MIN_PEER_PROTO_VERSION); // Addrman functions - size_t GetAddressCount() const; void SetServices(const CService &addr, ServiceFlags nServices); void MarkAddressGood(const CAddress& addr); void AddNewAddresses(const std::vector& vAddr, const CAddress& addrFrom, int64_t nTimePenalty = 0); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f7a116a673..6c1ce310ca 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1603,7 +1603,7 @@ static void RelayAddress(const CAddress& addr, bool fReachable, CConnman& connma assert(nRelayNodes <= best.size()); auto sortfunc = [&best, &hasher, nRelayNodes, addr](CNode* pnode) { - if (pnode->nVersion >= CADDR_TIME_VERSION && pnode->IsAddrRelayPeer() && pnode->IsAddrCompatible(addr)) { + if (pnode->IsAddrRelayPeer() && pnode->IsAddrCompatible(addr)) { uint64_t hashKey = CSipHasher(hasher).Write(pnode->GetId()).Finalize(); for (unsigned int i = 0; i < nRelayNodes; i++) { if (hashKey > best[i].first) { @@ -2738,11 +2738,8 @@ void PeerLogicValidation::ProcessMessage( } // Get recent addresses - if (pfrom.fOneShot || pfrom.nVersion >= CADDR_TIME_VERSION || m_connman.GetAddressCount() < 1000) - { - m_connman.PushMessage(&pfrom, CNetMsgMaker(nSendVersion).Make(NetMsgType::GETADDR)); - pfrom.fGetAddr = true; - } + m_connman.PushMessage(&pfrom, CNetMsgMaker(nSendVersion).Make(NetMsgType::GETADDR)); + pfrom.fGetAddr = true; m_connman.MarkAddressGood(pfrom.addr); } @@ -2882,10 +2879,6 @@ void PeerLogicValidation::ProcessMessage( s >> vAddr; - // Don't want addr from older versions unless seeding - if (pfrom.nVersion < CADDR_TIME_VERSION && m_connman.GetAddressCount() > 1000) - return; - if (!pfrom.IsAddrRelayPeer()) { return; } @@ -2930,7 +2923,6 @@ void PeerLogicValidation::ProcessMessage( pfrom.fGetAddr = false; if (pfrom.fOneShot) pfrom.fDisconnect = true; - statsClient.gauge("peers.knownAddresses", m_connman.GetAddressCount(), 1.0f); return; } From b51344130078b93cfd5352472fd3924acccffc4d Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com> Date: Fri, 10 Jul 2020 16:48:20 +0100 Subject: [PATCH 5/9] merge bitcoin#19486: Remove unused constants CADDR_TIME_VERSION and GETHEADERS_VERSION --- src/protocol.h | 8 +++++++- src/version.h | 4 ---- test/functional/test_framework/messages.py | 12 ++++++------ 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/protocol.h b/src/protocol.h index 8b0c25db42..c2eb3a2247 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -408,7 +408,13 @@ public: READWRITE(nVersion); } if ((s.GetType() & SER_DISK) || - (nVersion >= CADDR_TIME_VERSION && !(s.GetType() & SER_GETHASH))) { + (nVersion != INIT_PROTO_VERSION && !(s.GetType() & SER_GETHASH))) { + // The only time we serialize a CAddress object without nTime is in + // the initial VERSION messages which contain two CAddress records. + // At that point, the serialization version is INIT_PROTO_VERSION. + // After the version handshake, serialization version is >= + // MIN_PEER_PROTO_VERSION and all ADDR messages are serialized with + // nTime. READWRITE(obj.nTime); } if (nVersion & ADDRV2_FORMAT) { diff --git a/src/version.h b/src/version.h index 456b44d784..c8aa41bf0e 100644 --- a/src/version.h +++ b/src/version.h @@ -22,10 +22,6 @@ static const int MIN_PEER_PROTO_VERSION = 70215; //! minimum proto version of masternode to accept in DKGs static const int MIN_MASTERNODE_PROTO_VERSION = 70221; -//! nTime field added to CAddress, starting with this version; -//! if possible, avoid requesting addresses nodes older than this -static const int CADDR_TIME_VERSION = 31402; - //! protocol version is included in MNAUTH starting with this version static const int MNAUTH_NODE_VER_VERSION = 70218; diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py index b4c12ac655..a878f95a50 100755 --- a/test/functional/test_framework/messages.py +++ b/test/functional/test_framework/messages.py @@ -255,7 +255,7 @@ class CAddress: self.ip = "0.0.0.0" self.port = 0 - def deserialize(self, f, with_time=True): + def deserialize(self, f, *, with_time=True): """Deserialize from addrv1 format (pre-BIP155)""" if with_time: # VERSION messages serialize CAddress objects without time @@ -267,7 +267,7 @@ class CAddress: self.ip = socket.inet_ntoa(f.read(4)) self.port = struct.unpack(">H", f.read(2))[0] - def serialize(self, with_time=True): + def serialize(self, *, with_time=True): """Serialize in addrv1 format (pre-BIP155)""" assert self.net == self.NET_IPV4 r = b"" @@ -1371,10 +1371,10 @@ class msg_version: self.nServices = struct.unpack(" Date: Wed, 22 Feb 2023 07:47:44 +0000 Subject: [PATCH 6/9] partial bitcoin#20187: test-before-evict bugfix and improvements for block-relay-only peers Contains only daf55531260833d597ee599e2d289ea1be0b1d9c --- src/net.cpp | 2 +- src/net.h | 2 +- src/net_processing.cpp | 6 ++++-- src/net_processing.h | 2 +- src/test/denialofservice_tests.cpp | 12 ++++++------ 5 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 2d04cfb11b..7a1194e1f5 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3310,7 +3310,7 @@ void CConnman::DeleteNode(CNode* pnode) { assert(pnode); bool fUpdateConnectionTime = false; - m_msgproc->FinalizeNode(pnode->GetId(), fUpdateConnectionTime); + m_msgproc->FinalizeNode(*pnode, fUpdateConnectionTime); if(fUpdateConnectionTime) { addrman.Connected(pnode->addr); } diff --git a/src/net.h b/src/net.h index bbfcbd8c95..116350e8c6 100644 --- a/src/net.h +++ b/src/net.h @@ -708,7 +708,7 @@ public: virtual bool ProcessMessages(CNode* pnode, std::atomic& interrupt) = 0; virtual bool SendMessages(CNode* pnode) = 0; virtual void InitializeNode(CNode* pnode) = 0; - virtual void FinalizeNode(NodeId id, bool& update_connection_time) = 0; + virtual void FinalizeNode(const CNode& node, bool& update_connection_time) = 0; protected: /** diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6c1ce310ca..765586327f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -929,7 +929,8 @@ void PeerLogicValidation::ReattemptInitialBroadcast(CScheduler& scheduler) const scheduler.scheduleFromNow([&] { ReattemptInitialBroadcast(scheduler); }, delta); } -void PeerLogicValidation::FinalizeNode(NodeId nodeid, bool& fUpdateConnectionTime) { +void PeerLogicValidation::FinalizeNode(const CNode& node, bool& fUpdateConnectionTime) { + NodeId nodeid = node.GetId(); fUpdateConnectionTime = false; LOCK(cs_main); CNodeState *state = State(nodeid); @@ -938,7 +939,8 @@ void PeerLogicValidation::FinalizeNode(NodeId nodeid, bool& fUpdateConnectionTim if (state->fSyncStarted) nSyncStarted--; - if (state->nMisbehavior == 0 && state->fCurrentlyConnected) { + if (state->nMisbehavior == 0 && state->fCurrentlyConnected && !node.m_block_relay_only_peer) { + // Note: we avoid changing visible addrman state for block-relay-only peers fUpdateConnectionTime = true; } diff --git a/src/net_processing.h b/src/net_processing.h index 7e4b1d4e01..52845660df 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -59,7 +59,7 @@ public: /** Initialize a peer by adding it to mapNodeState and pushing a message requesting its version */ void InitializeNode(CNode* pnode) override; /** Handle removal of a peer by updating various state and removing it from mapNodeState */ - void FinalizeNode(NodeId nodeid, bool& fUpdateConnectionTime) override; + void FinalizeNode(const CNode& node, bool& fUpdateConnectionTime) override; /** * Process protocol messages received from a given node * diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 8e062c3d9c..e9d86c6bbe 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -134,7 +134,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) SetMockTime(0); bool dummy; - peerLogic->FinalizeNode(dummyNode1.GetId(), dummy); + peerLogic->FinalizeNode(dummyNode1, dummy); } static void AddRandomOutboundPeer(std::vector &vNodes, PeerLogicValidation &peerLogic, CConnmanTest* connman) @@ -219,7 +219,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) bool dummy; for (const CNode *node : vNodes) { - peerLogic->FinalizeNode(node->GetId(), dummy); + peerLogic->FinalizeNode(*node, dummy); } connman->ClearNodes(); @@ -278,8 +278,8 @@ BOOST_AUTO_TEST_CASE(DoS_banning) BOOST_CHECK(banman->IsDiscouraged(addr2)); bool dummy; - peerLogic->FinalizeNode(dummyNode1.GetId(), dummy); - peerLogic->FinalizeNode(dummyNode2.GetId(), dummy); + peerLogic->FinalizeNode(dummyNode1, dummy); + peerLogic->FinalizeNode(dummyNode2, dummy); } BOOST_AUTO_TEST_CASE(DoS_banscore) @@ -328,7 +328,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore) gArgs.ForceSetArg("-banscore", ToString(DEFAULT_BANSCORE_THRESHOLD)); bool dummy; - peerLogic->FinalizeNode(dummyNode1.GetId(), dummy); + peerLogic->FinalizeNode(dummyNode1, dummy); } BOOST_AUTO_TEST_CASE(DoS_bantime) @@ -361,7 +361,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) BOOST_CHECK(banman->IsDiscouraged(addr)); bool dummy; - peerLogic->FinalizeNode(dummyNode.GetId(), dummy); + peerLogic->FinalizeNode(dummyNode, dummy); } static CTransactionRef RandomOrphan() From 07fe6d47382111f38f5633c966ad575a8dfeefd8 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com> Date: Thu, 16 Feb 2023 17:46:27 +0000 Subject: [PATCH 7/9] merge bitcoin#19607: Add Peer struct for per-peer data in net processing --- src/coinjoin/client.cpp | 3 - src/coinjoin/server.cpp | 3 - src/evo/mnauth.cpp | 6 -- src/governance/governance.cpp | 2 - src/llmq/blockprocessor.cpp | 6 +- src/llmq/chainlocks.cpp | 1 - src/llmq/dkgsessionhandler.cpp | 3 - src/llmq/dkgsessionmgr.cpp | 6 -- src/llmq/instantsend.cpp | 5 +- src/llmq/quorums.cpp | 1 - src/llmq/signing.cpp | 2 - src/llmq/signing_shares.cpp | 1 - src/net_processing.cpp | 165 +++++++++++++++++------------ src/net_processing.h | 2 +- src/spork.cpp | 2 - src/test/denialofservice_tests.cpp | 23 +--- 16 files changed, 110 insertions(+), 121 deletions(-) diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp index bbb4531f67..c7777d08cf 100644 --- a/src/coinjoin/client.cpp +++ b/src/coinjoin/client.cpp @@ -54,7 +54,6 @@ void CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, CDataStream& vRecv >> dsq; if (dsq.masternodeOutpoint.IsNull() && dsq.m_protxHash.IsNull()) { - LOCK(cs_main); Misbehaving(peer.GetId(), 100); return; } @@ -64,7 +63,6 @@ void CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, CDataStream& if (auto dmn = mnList.GetValidMN(dsq.m_protxHash)) { dsq.masternodeOutpoint = dmn->collateralOutpoint; } else { - LOCK(cs_main); Misbehaving(peer.GetId(), 10); return; } @@ -100,7 +98,6 @@ void CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, CDataStream& } if (!dsq.CheckSignature(dmn->pdmnState->pubKeyOperator.Get())) { - LOCK(cs_main); Misbehaving(peer.GetId(), 10); return; } diff --git a/src/coinjoin/server.cpp b/src/coinjoin/server.cpp index 8722d538c9..52dc6fea22 100644 --- a/src/coinjoin/server.cpp +++ b/src/coinjoin/server.cpp @@ -113,7 +113,6 @@ void CCoinJoinServer::ProcessDSQUEUE(const CNode& peer, CDataStream& vRecv) vRecv >> dsq; if (dsq.masternodeOutpoint.IsNull() && dsq.m_protxHash.IsNull()) { - LOCK(cs_main); Misbehaving(peer.GetId(), 100); return; } @@ -123,7 +122,6 @@ void CCoinJoinServer::ProcessDSQUEUE(const CNode& peer, CDataStream& vRecv) if (auto dmn = mnList.GetValidMN(dsq.m_protxHash)) { dsq.masternodeOutpoint = dmn->collateralOutpoint; } else { - LOCK(cs_main); Misbehaving(peer.GetId(), 10); return; } @@ -159,7 +157,6 @@ void CCoinJoinServer::ProcessDSQUEUE(const CNode& peer, CDataStream& vRecv) } if (!dsq.CheckSignature(dmn->pdmnState->pubKeyOperator.Get())) { - LOCK(cs_main); Misbehaving(peer.GetId(), 10); return; } diff --git a/src/evo/mnauth.cpp b/src/evo/mnauth.cpp index 252b1390c8..d7527a55cb 100644 --- a/src/evo/mnauth.cpp +++ b/src/evo/mnauth.cpp @@ -70,26 +70,22 @@ void CMNAuth::ProcessMessage(CNode& peer, CConnman& connman, std::string_view ms // only one MNAUTH allowed if (!peer.GetVerifiedProRegTxHash().IsNull()) { - LOCK(cs_main); Misbehaving(peer.GetId(), 100, "duplicate mnauth"); return; } if ((~peer.nServices) & (NODE_NETWORK | NODE_BLOOM)) { // either NODE_NETWORK or NODE_BLOOM bit is missing in node's services - LOCK(cs_main); Misbehaving(peer.GetId(), 100, "mnauth from a node with invalid services"); return; } if (mnauth.proRegTxHash.IsNull()) { - LOCK(cs_main); Misbehaving(peer.GetId(), 100, "empty mnauth proRegTxHash"); return; } if (!mnauth.sig.IsValid()) { - LOCK(cs_main); Misbehaving(peer.GetId(), 100, "invalid mnauth signature"); return; } @@ -97,7 +93,6 @@ void CMNAuth::ProcessMessage(CNode& peer, CConnman& connman, std::string_view ms const auto mnList = deterministicMNManager->GetListAtChainTip(); const auto dmn = mnList.GetMN(mnauth.proRegTxHash); if (!dmn) { - LOCK(cs_main); // in case node was unlucky and not up to date, just let it be connected as a regular node, which gives it // a chance to get up-to-date and thus realize that it's not a MN anymore. We still give it a // low DoS score. @@ -122,7 +117,6 @@ void CMNAuth::ProcessMessage(CNode& peer, CConnman& connman, std::string_view ms LogPrint(BCLog::NET_NETCONN, "CMNAuth::%s -- constructed signHash for nVersion %d, peer=%d\n", __func__, peer.nVersion, peer.GetId()); if (!mnauth.sig.VerifyInsecure(dmn->pdmnState->pubKeyOperator.Get(), signHash)) { - LOCK(cs_main); // Same as above, MN seems to not know its fate yet, so give it a chance to update. If this is a // malicious node (DoSing us), it'll get banned soon. Misbehaving(peer.GetId(), 10, "mnauth signature verification failed"); diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index 93718ea32d..b584f4a0de 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -219,7 +219,6 @@ void CGovernanceManager::ProcessMessage(CNode& peer, CConnman& connman, std::str } else { LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECTVOTE -- Rejected vote, error = %s\n", exception.what()); if ((exception.GetNodePenalty() != 0) && ::masternodeSync->IsSynced()) { - LOCK(cs_main); Misbehaving(peer.GetId(), exception.GetNodePenalty()); } return; @@ -625,7 +624,6 @@ void CGovernanceManager::SyncObjects(CNode& peer, CConnman& connman) const if (netfulfilledman.HasFulfilledRequest(peer.addr, NetMsgType::MNGOVERNANCESYNC)) { // Asking for the whole list multiple times in a short period of time is no good LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s -- peer already asked me for the list\n", __func__); - LOCK(cs_main); Misbehaving(peer.GetId(), 20); return; } diff --git a/src/llmq/blockprocessor.cpp b/src/llmq/blockprocessor.cpp index 46e233239e..3c8c1f8a33 100644 --- a/src/llmq/blockprocessor.cpp +++ b/src/llmq/blockprocessor.cpp @@ -55,14 +55,14 @@ void CQuorumBlockProcessor::ProcessMessage(const CNode& peer, std::string_view m if (qc.IsNull()) { LogPrint(BCLog::LLMQ, "CQuorumBlockProcessor::%s -- null commitment from peer=%d\n", __func__, peer.GetId()); - WITH_LOCK(cs_main, Misbehaving(peer.GetId(), 100)); + Misbehaving(peer.GetId(), 100); return; } if (!Params().HasLLMQ(qc.llmqType)) { LogPrint(BCLog::LLMQ, "CQuorumBlockProcessor::%s -- invalid commitment type %d from peer=%d\n", __func__, ToUnderlying(qc.llmqType), peer.GetId()); - WITH_LOCK(cs_main, Misbehaving(peer.GetId(), 100)); + Misbehaving(peer.GetId(), 100); return; } auto type = qc.llmqType; @@ -125,7 +125,7 @@ void CQuorumBlockProcessor::ProcessMessage(const CNode& peer, std::string_view m LogPrint(BCLog::LLMQ, "CQuorumBlockProcessor::%s -- commitment for quorum %s:%d is not valid quorumIndex[%d] nversion[%d], peer=%d\n", __func__, qc.quorumHash.ToString(), ToUnderlying(qc.llmqType), qc.quorumIndex, qc.nVersion, peer.GetId()); - WITH_LOCK(cs_main, Misbehaving(peer.GetId(), 100)); + Misbehaving(peer.GetId(), 100); return; } diff --git a/src/llmq/chainlocks.cpp b/src/llmq/chainlocks.cpp index b33311aeb7..3ba96ec118 100644 --- a/src/llmq/chainlocks.cpp +++ b/src/llmq/chainlocks.cpp @@ -125,7 +125,6 @@ void CChainLocksHandler::ProcessNewChainLock(const NodeId from, const llmq::CCha if (!llmq::CSigningManager::VerifyRecoveredSig(Params().GetConsensus().llmqTypeChainLocks, *llmq::quorumManager, clsig.getHeight(), requestId, clsig.getBlockHash(), clsig.getSig())) { LogPrint(BCLog::CHAINLOCKS, "CChainLocksHandler::%s -- invalid CLSIG (%s), peer=%d\n", __func__, clsig.ToString(), from); if (from != -1) { - LOCK(cs_main); Misbehaving(from, 10); } return; diff --git a/src/llmq/dkgsessionhandler.cpp b/src/llmq/dkgsessionhandler.cpp index 5c0cd46154..3e75d924e0 100644 --- a/src/llmq/dkgsessionhandler.cpp +++ b/src/llmq/dkgsessionhandler.cpp @@ -429,7 +429,6 @@ bool ProcessPendingMessageBatch(CDKGSession& session, CDKGPendingMessages& pendi if (!p.second) { LogPrint(BCLog::LLMQ_DKG, "%s -- failed to deserialize message, peer=%d\n", __func__, nodeId); { - LOCK(cs_main); Misbehaving(nodeId, 100); } continue; @@ -439,7 +438,6 @@ bool ProcessPendingMessageBatch(CDKGSession& session, CDKGPendingMessages& pendi if (ban) { LogPrint(BCLog::LLMQ_DKG, "%s -- banning node due to failed preverification, peer=%d\n", __func__, nodeId); { - LOCK(cs_main); Misbehaving(nodeId, 100); } } @@ -470,7 +468,6 @@ bool ProcessPendingMessageBatch(CDKGSession& session, CDKGPendingMessages& pendi session.ReceiveMessage(*p.second, ban); if (ban) { LogPrint(BCLog::LLMQ_DKG, "%s -- banning node after ReceiveMessage failed, peer=%d\n", __func__, nodeId); - LOCK(cs_main); Misbehaving(nodeId, 100); badNodes.emplace(nodeId); } diff --git a/src/llmq/dkgsessionmgr.cpp b/src/llmq/dkgsessionmgr.cpp index 907b3ba80c..c8f303cd12 100644 --- a/src/llmq/dkgsessionmgr.cpp +++ b/src/llmq/dkgsessionmgr.cpp @@ -185,7 +185,6 @@ void CDKGSessionManager::ProcessMessage(CNode& pfrom, const CQuorumManager& quor } if (vRecv.empty()) { - LOCK(cs_main); Misbehaving(pfrom.GetId(), 100); return; } @@ -198,7 +197,6 @@ void CDKGSessionManager::ProcessMessage(CNode& pfrom, const CQuorumManager& quor vRecv.Rewind(sizeof(uint8_t)); if (!Params().HasLLMQ(llmqType)) { - LOCK(cs_main); LogPrintf("CDKGSessionManager -- invalid llmqType [%d]\n", ToUnderlying(llmqType)); Misbehaving(pfrom.GetId(), 100); return; @@ -219,7 +217,6 @@ void CDKGSessionManager::ProcessMessage(CNode& pfrom, const CQuorumManager& quor if (quorumIndex == -1) { CBlockIndex* pQuorumBaseBlockIndex = WITH_LOCK(cs_main, return LookupBlockIndex(quorumHash)); if (pQuorumBaseBlockIndex == nullptr) { - LOCK(cs_main); LogPrintf("CDKGSessionManager -- unknown quorumHash %s\n", quorumHash.ToString()); // NOTE: do not insta-ban for this, we might be lagging behind Misbehaving(pfrom.GetId(), 10); @@ -227,7 +224,6 @@ void CDKGSessionManager::ProcessMessage(CNode& pfrom, const CQuorumManager& quor } if (!utils::IsQuorumTypeEnabled(llmqType, quorum_manager, pQuorumBaseBlockIndex->pprev)) { - LOCK(cs_main); LogPrintf("CDKGSessionManager -- llmqType [%d] quorums aren't active\n", ToUnderlying(llmqType)); Misbehaving(pfrom.GetId(), 100); return; @@ -239,14 +235,12 @@ void CDKGSessionManager::ProcessMessage(CNode& pfrom, const CQuorumManager& quor llmqParams.signingActiveQuorumCount - 1 : 0; if (quorumIndex > quorumIndexMax) { - LOCK(cs_main); LogPrintf("CDKGSessionManager -- invalid quorumHash %s\n", quorumHash.ToString()); Misbehaving(pfrom.GetId(), 100); return; } if (!dkgSessionHandlers.count(std::make_pair(llmqType, quorumIndex))) { - LOCK(cs_main); LogPrintf("CDKGSessionManager -- no session handlers for quorumIndex [%d]\n", quorumIndex); Misbehaving(pfrom.GetId(), 100); return; diff --git a/src/llmq/instantsend.cpp b/src/llmq/instantsend.cpp index 0a08cc5942..a94056e412 100644 --- a/src/llmq/instantsend.cpp +++ b/src/llmq/instantsend.cpp @@ -788,7 +788,6 @@ void CInstantSendManager::ProcessMessageInstantSendLock(const CNode& pfrom, cons } if (!islock->TriviallyValid()) { - LOCK(cs_main); Misbehaving(pfrom.GetId(), 100); return; } @@ -798,14 +797,14 @@ void CInstantSendManager::ProcessMessageInstantSendLock(const CNode& pfrom, cons const auto blockIndex = WITH_LOCK(cs_main, return LookupBlockIndex(islock->cycleHash)); if (blockIndex == nullptr) { // Maybe we don't have the block yet or maybe some peer spams invalid values for cycleHash - WITH_LOCK(cs_main, Misbehaving(pfrom.GetId(), 1)); + Misbehaving(pfrom.GetId(), 1); return; } // Deterministic islocks MUST use rotation based llmq auto llmqType = Params().GetConsensus().llmqTypeDIP0024InstantSend; if (blockIndex->nHeight % GetLLMQParams(llmqType).dkgInterval != 0) { - WITH_LOCK(cs_main, Misbehaving(pfrom.GetId(), 100)); + Misbehaving(pfrom.GetId(), 100); return; } } diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp index 7bd215c751..f882b5e917 100644 --- a/src/llmq/quorums.cpp +++ b/src/llmq/quorums.cpp @@ -611,7 +611,6 @@ void CQuorumManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, C auto errorHandler = [&](const std::string& strError, int nScore = 10) { LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- %s: %s, from peer=%d\n", strFunc, msg_type, strError, pfrom.GetId()); if (nScore > 0) { - LOCK(cs_main); Misbehaving(pfrom.GetId(), nScore); } }; diff --git a/src/llmq/signing.cpp b/src/llmq/signing.cpp index 3089f48c08..863193db4e 100644 --- a/src/llmq/signing.cpp +++ b/src/llmq/signing.cpp @@ -577,7 +577,6 @@ void CSigningManager::ProcessMessageRecoveredSig(const CNode& pfrom, const std:: bool ban = false; if (!PreVerifyRecoveredSig(qman, *recoveredSig, ban)) { if (ban) { - LOCK(cs_main); Misbehaving(pfrom.GetId(), 100); } return; @@ -752,7 +751,6 @@ bool CSigningManager::ProcessPendingRecoveredSigs() if (batchVerifier.badSources.count(nodeId)) { LogPrint(BCLog::LLMQ, "CSigningManager::%s -- invalid recSig from other node, banning peer=%d\n", __func__, nodeId); - LOCK(cs_main); Misbehaving(nodeId, 100); continue; } diff --git a/src/llmq/signing_shares.cpp b/src/llmq/signing_shares.cpp index 59a1411278..da63001c81 100644 --- a/src/llmq/signing_shares.cpp +++ b/src/llmq/signing_shares.cpp @@ -1402,7 +1402,6 @@ void CSigSharesManager::BanNode(NodeId nodeId) } { - LOCK(cs_main); Misbehaving(nodeId, 100); } diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 765586327f..cc3a7edc45 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -268,12 +268,6 @@ struct CNodeState { const CService address; //! Whether we have a fully established connection. bool fCurrentlyConnected; - //! Accumulated misbehaviour score for this peer. - int nMisbehavior; - //! Whether this peer should be disconnected and marked as discouraged (unless it has the noban permission). - bool m_should_discourage; - //! String name of this peer (debugging/logging purposes). - const std::string name; //! The best known block we know this peer has announced. const CBlockIndex *pindexBestKnownBlock; //! The hash of the last unknown block this peer has announced. @@ -410,13 +404,10 @@ struct CNodeState { //! Whether this peer is a manual connection bool m_is_manual_connection; - CNodeState(CAddress addrIn, std::string addrNameIn, bool is_inbound, bool is_manual) : - address(addrIn), name(std::move(addrNameIn)), m_is_inbound(is_inbound), - m_is_manual_connection (is_manual) + CNodeState(CAddress addrIn, bool is_inbound, bool is_manual) : + address(addrIn), m_is_inbound(is_inbound), m_is_manual_connection(is_manual) { fCurrentlyConnected = false; - nMisbehavior = 0; - m_should_discourage = false; pindexBestKnownBlock = nullptr; hashLastUnknownBlock.SetNull(); pindexLastCommonBlock = nullptr; @@ -453,6 +444,50 @@ static CNodeState *State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return &it->second; } +/** + * Data structure for an individual peer. This struct is not protected by + * cs_main since it does not contain validation-critical data. + * + * Memory is owned by shared pointers and this object is destructed when + * the refcount drops to zero. + * + * TODO: move most members from CNodeState to this structure. + * TODO: move remaining application-layer data members from CNode to this structure. + */ +struct Peer { + /** Same id as the CNode object for this peer */ + const NodeId m_id{0}; + + /** Protects misbehavior data members */ + Mutex m_misbehavior_mutex; + /** Accumulated misbehavior score for this peer */ + int nMisbehavior GUARDED_BY(m_misbehavior_mutex){0}; + /** Whether this peer should be disconnected and marked as discouraged (unless it has the noban permission). */ + bool m_should_discourage GUARDED_BY(m_misbehavior_mutex){false}; + + Peer(NodeId id) : m_id(id) {} +}; + +using PeerRef = std::shared_ptr; + +/** + * Map of all Peer objects, keyed by peer id. This map is protected + * by the global g_peer_mutex. Once a shared pointer reference is + * taken, the lock may be released. Individual fields are protected by + * their own locks. + */ +Mutex g_peer_mutex; +static std::map g_peer_map GUARDED_BY(g_peer_mutex); + +/** Get a shared pointer to the Peer object. + * May return nullptr if the Peer object can't be found. */ +static PeerRef GetPeerRef(NodeId id) +{ + LOCK(g_peer_mutex); + auto it = g_peer_map.find(id); + return it != g_peer_map.end() ? it->second : nullptr; +} + static void UpdatePreferredDownload(const CNode& node, CNodeState* state) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { nPreferredDownload -= state->fPreferredDownload; @@ -906,13 +941,17 @@ static bool IsOutboundDisconnectionCandidate(const CNode& node) void PeerLogicValidation::InitializeNode(CNode *pnode) { CAddress addr = pnode->addr; - std::string addrName = pnode->GetAddrName(); NodeId nodeid = pnode->GetId(); { LOCK(cs_main); - mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, std::move(addrName), pnode->fInbound, pnode->m_manual_connection)); + mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, pnode->fInbound, pnode->m_manual_connection)); } - if(!pnode->fInbound) + { + PeerRef peer = std::make_shared(nodeid); + LOCK(g_peer_mutex); + g_peer_map.emplace_hint(g_peer_map.end(), nodeid, std::move(peer)); + } + if (!pnode->fInbound) PushNodeVersion(*pnode, m_connman, GetTime()); } @@ -933,13 +972,21 @@ void PeerLogicValidation::FinalizeNode(const CNode& node, bool& fUpdateConnectio NodeId nodeid = node.GetId(); fUpdateConnectionTime = false; LOCK(cs_main); + int misbehavior{0}; + { + PeerRef peer = GetPeerRef(nodeid); + assert(peer != nullptr); + misbehavior = WITH_LOCK(peer->m_misbehavior_mutex, return peer->nMisbehavior); + LOCK(g_peer_mutex); + g_peer_map.erase(nodeid); + } CNodeState *state = State(nodeid); assert(state != nullptr); if (state->fSyncStarted) nSyncStarted--; - if (state->nMisbehavior == 0 && state->fCurrentlyConnected && !node.m_block_relay_only_peer) { + if (misbehavior == 0 && state->fCurrentlyConnected && !node.m_block_relay_only_peer) { // Note: we avoid changing visible addrman state for block-relay-only peers fUpdateConnectionTime = true; } @@ -967,17 +1014,23 @@ void PeerLogicValidation::FinalizeNode(const CNode& node, bool& fUpdateConnectio } bool GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats) { - LOCK(cs_main); - CNodeState *state = State(nodeid); - if (state == nullptr) - return false; - stats.nMisbehavior = state->nMisbehavior; - stats.nSyncHeight = state->pindexBestKnownBlock ? state->pindexBestKnownBlock->nHeight : -1; - stats.nCommonHeight = state->pindexLastCommonBlock ? state->pindexLastCommonBlock->nHeight : -1; - for (const QueuedBlock& queue : state->vBlocksInFlight) { - if (queue.pindex) - stats.vHeightInFlight.push_back(queue.pindex->nHeight); + { + LOCK(cs_main); + CNodeState* state = State(nodeid); + if (state == nullptr) + return false; + stats.nSyncHeight = state->pindexBestKnownBlock ? state->pindexBestKnownBlock->nHeight : -1; + stats.nCommonHeight = state->pindexLastCommonBlock ? state->pindexLastCommonBlock->nHeight : -1; + for (const QueuedBlock& queue : state->vBlocksInFlight) { + if (queue.pindex) + stats.vHeightInFlight.push_back(queue.pindex->nHeight); + } } + + PeerRef peer = GetPeerRef(nodeid); + if (peer == nullptr) return false; + stats.nMisbehavior = WITH_LOCK(peer->m_misbehavior_mutex, return peer->nMisbehavior); + return true; } @@ -1127,33 +1180,35 @@ void static ProcessOrphanTx(CConnman& connman, CTxMemPool& mempool, std::set 0); - CNodeState* const state = State(pnode); - if (state == nullptr) return; + PeerRef peer = GetPeerRef(pnode); + if (peer == nullptr) return; - state->nMisbehavior += howmuch; + LOCK(peer->m_misbehavior_mutex); + peer->nMisbehavior += howmuch; const int banscore = gArgs.GetArg("-banscore", DEFAULT_BANSCORE_THRESHOLD); const std::string message_prefixed = message.empty() ? "" : (": " + message); - if (state->nMisbehavior >= banscore && state->nMisbehavior - howmuch < banscore) + if (peer->nMisbehavior >= banscore && peer->nMisbehavior - howmuch < banscore) { - LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d) DISCOURAGE THRESHOLD EXCEEDED%s\n", pnode, state->nMisbehavior - howmuch, state->nMisbehavior, message_prefixed); - state->m_should_discourage = true; + LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d) DISCOURAGE THRESHOLD EXCEEDED%s\n", pnode, peer->nMisbehavior - howmuch, peer->nMisbehavior, message_prefixed); + peer->m_should_discourage = true; statsClient.inc("misbehavior.banned", 1.0f); } else { - LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d)%s\n", pnode, state->nMisbehavior - howmuch, state->nMisbehavior, message_prefixed); + LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d)%s\n", pnode, peer->nMisbehavior - howmuch, peer->nMisbehavior, message_prefixed); statsClient.count("misbehavior.amount", howmuch, 1.0); } } bool IsBanned(NodeId pnode) { - CNodeState *state = State(pnode); - if (state == nullptr) + PeerRef peer = GetPeerRef(pnode); + if (peer == nullptr) return false; - if (state->m_should_discourage) { + LOCK(peer->m_misbehavior_mutex); + if (peer->m_should_discourage) { return true; } return false; @@ -1178,7 +1233,6 @@ static bool MaybePunishNode(NodeId nodeid, const CValidationState& state, bool v case ValidationInvalidReason::CONSENSUS: case ValidationInvalidReason::BLOCK_MUTATED: if (!via_compact_block) { - LOCK(cs_main); Misbehaving(nodeid, 100, message); return true; } @@ -1202,21 +1256,14 @@ static bool MaybePunishNode(NodeId nodeid, const CValidationState& state, bool v case ValidationInvalidReason::BLOCK_INVALID_HEADER: case ValidationInvalidReason::BLOCK_CHECKPOINT: case ValidationInvalidReason::BLOCK_INVALID_PREV: - { - LOCK(cs_main); - Misbehaving(nodeid, 100, message); - } + Misbehaving(nodeid, 100, message); return true; // Conflicting (but not necessarily invalid) data or different policy: case ValidationInvalidReason::BLOCK_MISSING_PREV: case ValidationInvalidReason::BLOCK_CHAINLOCK: case ValidationInvalidReason::TX_BAD_SPECIAL: case ValidationInvalidReason::TX_CONFLICT_LOCK: - { - // TODO: Handle this much more gracefully (10 DoS points is super arbitrary) - LOCK(cs_main); - Misbehaving(nodeid, 10, message); - } + Misbehaving(nodeid, 10, message); return true; case ValidationInvalidReason::RECENT_CONSENSUS_CHANGE: case ValidationInvalidReason::BLOCK_TIME_FUTURE: @@ -2000,7 +2047,6 @@ inline void static SendBlockTransactions(const CBlock& block, const BlockTransac BlockTransactions resp(req); for (size_t i = 0; i < req.indexes.size(); i++) { if (req.indexes[i] >= block.vtx.size()) { - LOCK(cs_main); Misbehaving(pfrom.GetId(), 100, "getblocktxn with out-of-bounds tx indices"); return; } @@ -2572,7 +2618,6 @@ void PeerLogicValidation::ProcessMessage( (msg_type == NetMsgType::FILTERLOAD || msg_type == NetMsgType::FILTERADD)) { - LOCK(cs_main); Misbehaving(pfrom.GetId(), 100); return; } @@ -2581,7 +2626,6 @@ void PeerLogicValidation::ProcessMessage( // Each connection can only send one version message if (pfrom.nVersion != 0) { - LOCK(cs_main); Misbehaving(pfrom.GetId(), 1, "redundant version message"); return; } @@ -2670,7 +2714,6 @@ void PeerLogicValidation::ProcessMessage( if (Params().NetworkIDString() == CBaseChainParams::DEVNET) { if (cleanSubVer.find(strprintf("devnet.%s", gArgs.GetDevNetName())) == std::string::npos) { - LOCK(cs_main); LogPrintf("connected to wrong devnet. Reported version is %s, expected devnet name is %s\n", cleanSubVer, gArgs.GetDevNetName()); if (!pfrom.fInbound) Misbehaving(pfrom.GetId(), 100); // don't try to connect again @@ -2768,7 +2811,6 @@ void PeerLogicValidation::ProcessMessage( if (pfrom.nVersion == 0) { // Must have a version message before anything else - LOCK(cs_main); Misbehaving(pfrom.GetId(), 1, "non-version message before version handshake"); return; } @@ -2843,7 +2885,6 @@ void PeerLogicValidation::ProcessMessage( if (!pfrom.fSuccessfullyConnected) { // Must have a verack message before anything else - LOCK(cs_main); Misbehaving(pfrom.GetId(), 1, "non-verack message before version handshake"); return; } @@ -2886,7 +2927,6 @@ void PeerLogicValidation::ProcessMessage( } if (vAddr.size() > 1000) { - LOCK(cs_main); Misbehaving(pfrom.GetId(), 20, strprintf("%s message size = %u", msg_type, vAddr.size())); return; } @@ -2975,7 +3015,6 @@ void PeerLogicValidation::ProcessMessage( vRecv >> vInv; if (vInv.size() > MAX_INV_SZ) { - LOCK(cs_main); Misbehaving(pfrom.GetId(), 20, strprintf("message inv size() = %u", vInv.size())); return; } @@ -3066,7 +3105,6 @@ void PeerLogicValidation::ProcessMessage( vRecv >> vInv; if (vInv.size() > MAX_INV_SZ) { - LOCK(cs_main); Misbehaving(pfrom.GetId(), 20, strprintf("message getdata size() = %u", vInv.size())); return; } @@ -3750,7 +3788,6 @@ void PeerLogicValidation::ProcessMessage( // Bypass the normal CBlock deserialization, as we don't want to risk deserializing 2000 full blocks. unsigned int nCount = ReadCompactSize(vRecv); if (nCount > MAX_HEADERS_RESULTS) { - LOCK(cs_main); Misbehaving(pfrom.GetId(), 20, strprintf("headers message size = %u", nCount)); return; } @@ -3952,7 +3989,6 @@ void PeerLogicValidation::ProcessMessage( if (!filter.IsWithinSizeConstraints()) { // There is no excuse for sending a too-large filter - LOCK(cs_main); Misbehaving(pfrom.GetId(), 100, "too-large bloom filter"); } else if (!pfrom.m_block_relay_only_peer) @@ -3982,7 +4018,6 @@ void PeerLogicValidation::ProcessMessage( } } if (bad) { - LOCK(cs_main); Misbehaving(pfrom.GetId(), 100, "bad filteradd message"); } return; @@ -4035,7 +4070,6 @@ void PeerLogicValidation::ProcessMessage( if (msg_type == NetMsgType::MNLISTDIFF) { // we have never requested this - LOCK(cs_main); Misbehaving(pfrom.GetId(), 100, strprintf("received not-requested mnlistdiff. peer=%d", pfrom.GetId())); return; } @@ -4059,7 +4093,6 @@ void PeerLogicValidation::ProcessMessage( if (msg_type == NetMsgType::QUORUMROTATIONINFO) { // we have never requested this - LOCK(cs_main); Misbehaving(pfrom.GetId(), 100, strprintf("received not-requested quorumrotationinfo. peer=%d", pfrom.GetId())); return; } @@ -4136,15 +4169,17 @@ void PeerLogicValidation::ProcessMessage( bool PeerLogicValidation::MaybeDiscourageAndDisconnect(CNode& pnode) { const NodeId peer_id{pnode.GetId()}; + PeerRef peer = GetPeerRef(peer_id); + if (peer == nullptr) return false; + { - LOCK(cs_main); - CNodeState& state = *State(peer_id); + LOCK(peer->m_misbehavior_mutex); // There's nothing to do if the m_should_discourage flag isn't set - if (!state.m_should_discourage) return false; + if (!peer->m_should_discourage) return false; - state.m_should_discourage = false; - } // cs_main + peer->m_should_discourage = false; + } // peer.m_misbehavior_mutex if (pnode.HasPermission(PF_NOBAN)) { // We never disconnect or discourage peers for bad behavior if they have the NOBAN permission flag diff --git a/src/net_processing.h b/src/net_processing.h index 52845660df..904c266017 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -104,7 +104,7 @@ bool IsBanned(NodeId nodeid) EXCLUSIVE_LOCKS_REQUIRED(cs_main); // Upstream moved this into net_processing.cpp (13417), however since we use Misbehaving in a number of dash specific // files such as mnauth.cpp and governance.cpp it makes sense to keep it in the header /** Increase a node's misbehavior score. */ -void Misbehaving(NodeId nodeid, int howmuch, const std::string& message="") EXCLUSIVE_LOCKS_REQUIRED(cs_main); +void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message=""); void EraseObjectRequest(NodeId nodeId, const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main); void RequestObject(NodeId nodeId, const CInv& inv, std::chrono::microseconds current_time, bool fForce=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main); diff --git a/src/spork.cpp b/src/spork.cpp index 2798f09481..ef27566ea4 100644 --- a/src/spork.cpp +++ b/src/spork.cpp @@ -130,7 +130,6 @@ void CSporkManager::ProcessSpork(const CNode& peer, CConnman& connman, CDataStre } if (spork.nTimeSigned > GetAdjustedTime() + 2 * 60 * 60) { - LOCK(cs_main); LogPrint(BCLog::SPORK, "CSporkManager::ProcessSpork -- ERROR: too far into the future\n"); Misbehaving(peer.GetId(), 100); return; @@ -139,7 +138,6 @@ void CSporkManager::ProcessSpork(const CNode& peer, CConnman& connman, CDataStre auto opt_keyIDSigner = spork.GetSignerKeyID(); if (opt_keyIDSigner == std::nullopt || WITH_LOCK(cs, return !setSporkPubKeyIDs.count(*opt_keyIDSigner))) { - LOCK(cs_main); LogPrint(BCLog::SPORK, "CSporkManager::ProcessSpork -- ERROR: invalid signature\n"); Misbehaving(peer.GetId(), 100); return; diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index e9d86c6bbe..58abd23e81 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -240,10 +240,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning) peerLogic->InitializeNode(&dummyNode1); dummyNode1.nVersion = 1; dummyNode1.fSuccessfullyConnected = true; - { - LOCK(cs_main); - Misbehaving(dummyNode1.GetId(), 100); // Should get banned - } + Misbehaving(dummyNode1.GetId(), 100); // Should get banned { LOCK(dummyNode1.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); @@ -257,20 +254,14 @@ BOOST_AUTO_TEST_CASE(DoS_banning) peerLogic->InitializeNode(&dummyNode2); dummyNode2.nVersion = 1; dummyNode2.fSuccessfullyConnected = true; - { - LOCK(cs_main); - Misbehaving(dummyNode2.GetId(), 50); - } + Misbehaving(dummyNode2.GetId(), 50); { LOCK(dummyNode2.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode2)); } BOOST_CHECK(!banman->IsDiscouraged(addr2)); // 2 not banned yet... BOOST_CHECK(banman->IsDiscouraged(addr1)); // ... but 1 still should be - { - LOCK(cs_main); - Misbehaving(dummyNode2.GetId(), 50); - } + Misbehaving(dummyNode2.GetId(), 50); { LOCK(dummyNode2.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode2)); @@ -299,7 +290,6 @@ BOOST_AUTO_TEST_CASE(DoS_banscore) dummyNode1.nVersion = 1; dummyNode1.fSuccessfullyConnected = true; { - LOCK(cs_main); Misbehaving(dummyNode1.GetId(), 100); } { @@ -308,7 +298,6 @@ BOOST_AUTO_TEST_CASE(DoS_banscore) } BOOST_CHECK(!banman->IsDiscouraged(addr1)); { - LOCK(cs_main); Misbehaving(dummyNode1.GetId(), 10); } { @@ -317,7 +306,6 @@ BOOST_AUTO_TEST_CASE(DoS_banscore) } BOOST_CHECK(!banman->IsDiscouraged(addr1)); { - LOCK(cs_main); Misbehaving(dummyNode1.GetId(), 1); } { @@ -350,10 +338,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) dummyNode.nVersion = 1; dummyNode.fSuccessfullyConnected = true; - { - LOCK(cs_main); - Misbehaving(dummyNode.GetId(), 100); - } + Misbehaving(dummyNode.GetId(), 100); { LOCK(dummyNode.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode)); From 3e993abf107b3321061314652229293ce92cd715 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com> Date: Tue, 3 Nov 2020 09:15:20 +0000 Subject: [PATCH 8/9] merge bitcoin#20291: Consolidate logic around calling CAddrMan::Connected() --- src/addrman.h | 16 +++++++++++++--- src/net_processing.cpp | 8 +------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/addrman.h b/src/addrman.h index 6a031a0d51..1e162c50ce 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -268,8 +268,18 @@ protected: //! Select several addresses at once. void GetAddr_(std::vector &vAddr) EXCLUSIVE_LOCKS_REQUIRED(cs); - //! Mark an entry as currently-connected-to. - void Connected_(const CService &addr, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs); + /** We have successfully connected to this peer. Calling this function + * updates the CAddress's nTime, which is used in our IsTerrible() + * decisions and gossiped to peers. Callers should be careful that updating + * this information doesn't leak topology information to network spies. + * + * net_processing calls this function when it *disconnects* from a peer to + * not leak information about currently connected peers. + * + * @param[in] addr The address of the peer we were connected to + * @param[in] nTime The time that we were last connected to this peer + */ + void Connected_(const CService& addr, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs); //! Update an entry's service bits. void SetServices_(const CService &addr, ServiceFlags nServices) EXCLUSIVE_LOCKS_REQUIRED(cs); @@ -710,7 +720,7 @@ public: return vAddr; } - //! Mark an entry as currently-connected-to. + //! Outer function for Connected_() void Connected(const CService &addr, int64_t nTime = GetAdjustedTime()) { LOCK(cs); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index cc3a7edc45..f5b2b912d3 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -266,8 +266,6 @@ namespace { struct CNodeState { //! The peer's address const CService address; - //! Whether we have a fully established connection. - bool fCurrentlyConnected; //! The best known block we know this peer has announced. const CBlockIndex *pindexBestKnownBlock; //! The hash of the last unknown block this peer has announced. @@ -407,7 +405,6 @@ struct CNodeState { CNodeState(CAddress addrIn, bool is_inbound, bool is_manual) : address(addrIn), m_is_inbound(is_inbound), m_is_manual_connection(is_manual) { - fCurrentlyConnected = false; pindexBestKnownBlock = nullptr; hashLastUnknownBlock.SetNull(); pindexLastCommonBlock = nullptr; @@ -986,7 +983,7 @@ void PeerLogicValidation::FinalizeNode(const CNode& node, bool& fUpdateConnectio if (state->fSyncStarted) nSyncStarted--; - if (misbehavior == 0 && state->fCurrentlyConnected && !node.m_block_relay_only_peer) { + if (node.fSuccessfullyConnected && misbehavior == 0 && !node.m_block_relay_only_peer && !node.fInbound) { // Note: we avoid changing visible addrman state for block-relay-only peers fUpdateConnectionTime = true; } @@ -2825,9 +2822,6 @@ void PeerLogicValidation::ProcessMessage( pfrom.SetRecvVersion(std::min(pfrom.nVersion.load(), PROTOCOL_VERSION)); if (!pfrom.fInbound) { - // Mark this node as currently connected, so we update its timestamp later. - LOCK(cs_main); - State(pfrom.GetId())->fCurrentlyConnected = true; LogPrintf("New outbound peer connected: version: %d, blocks=%d, peer=%d%s (%s)\n", pfrom.nVersion.load(), pfrom.nStartingHeight, pfrom.GetId(), (fLogIPs ? strprintf(", peeraddr=%s", pfrom.addr.ToString()) : ""), From 40906b2bbb947ec576b156b1488db4039dce1017 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com> Date: Thu, 16 Feb 2023 06:34:06 +0000 Subject: [PATCH 9/9] partial bitcoin#20228: Make addrman a top-level component --- src/init.cpp | 7 ++- src/net.cpp | 26 ++--------- src/net.h | 9 ++-- src/net_processing.cpp | 29 +++++++----- src/net_processing.h | 6 ++- src/node/context.cpp | 1 + src/node/context.h | 2 + src/test/denialofservice_tests.cpp | 37 +++++++-------- src/test/fuzz/connman.cpp | 74 +++++++++++------------------- src/test/fuzz/net.cpp | 3 +- src/test/util/setup_common.cpp | 11 +++-- 11 files changed, 86 insertions(+), 119 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 04924694fc..094fa9a5ad 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -278,6 +278,7 @@ void PrepareShutdown(NodeContext& node) node.peer_logic.reset(); node.connman.reset(); node.banman.reset(); + node.addrman.reset(); if (node.mempool && node.mempool->IsLoaded() && node.args->GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) { DumpMempool(*node.mempool); @@ -1755,10 +1756,12 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc fDiscover = args.GetBoolArg("-discover", true); g_relay_txes = !args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY); + assert(!node.addrman); + node.addrman = std::make_unique(); assert(!node.banman); node.banman = std::make_unique(GetDataDir() / "banlist.dat", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME)); assert(!node.connman); - node.connman = std::make_unique(GetRand(std::numeric_limits::max()), GetRand(std::numeric_limits::max())); + node.connman = std::make_unique(GetRand(std::numeric_limits::max()), GetRand(std::numeric_limits::max()), *node.addrman); assert(!node.fee_estimator); // Don't initialize fee estimation with old data if we don't relay transactions, @@ -1774,7 +1777,7 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc ChainstateManager& chainman = *Assert(node.chainman); node.peer_logic.reset(new PeerLogicValidation( - *node.connman, node.banman.get(), *node.scheduler, chainman, *node.mempool, node.llmq_ctx + *node.connman, *node.addrman, node.banman.get(), *node.scheduler, chainman, *node.mempool, node.llmq_ctx )); RegisterValidationInterface(node.peer_logic.get()); diff --git a/src/net.cpp b/src/net.cpp index 7a1194e1f5..1dd7e4495f 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2949,9 +2949,8 @@ void CConnman::SetNetworkActive(bool active) uiInterface.NotifyNetworkActiveChanged(fNetworkActive); } -CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In) : - addrman(Params().AllowMultiplePorts()), - nSeed0(nSeed0In), nSeed1(nSeed1In) +CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In, CAddrMan& addrman_in) : + addrman(addrman_in), nSeed0(nSeed0In), nSeed1(nSeed1In) { SetTryNewOutboundPeer(false); @@ -3309,11 +3308,7 @@ void CConnman::Stop() void CConnman::DeleteNode(CNode* pnode) { assert(pnode); - bool fUpdateConnectionTime = false; - m_msgproc->FinalizeNode(*pnode, fUpdateConnectionTime); - if(fUpdateConnectionTime) { - addrman.Connected(pnode->addr); - } + m_msgproc->FinalizeNode(*pnode); delete pnode; } @@ -3323,21 +3318,6 @@ CConnman::~CConnman() Stop(); } -void CConnman::SetServices(const CService &addr, ServiceFlags nServices) -{ - addrman.SetServices(addr, nServices); -} - -void CConnman::MarkAddressGood(const CAddress& addr) -{ - addrman.Good(addr); -} - -void CConnman::AddNewAddresses(const std::vector& vAddr, const CAddress& addrFrom, int64_t nTimePenalty) -{ - addrman.Add(vAddr, addrFrom, nTimePenalty); -} - std::vector CConnman::GetAddresses() { return addrman.GetAddr(); diff --git a/src/net.h b/src/net.h index 116350e8c6..773b19b3bd 100644 --- a/src/net.h +++ b/src/net.h @@ -213,7 +213,7 @@ public: socketEventsMode = connOptions.socketEventsMode; } - CConnman(uint64_t seed0, uint64_t seed1); + CConnman(uint64_t seed0, uint64_t seed1, CAddrMan& addrman); ~CConnman(); bool Start(CScheduler& scheduler, const Options& options); @@ -399,9 +399,6 @@ public: void RelayInvFiltered(CInv &inv, const uint256 &relatedTxHash, const int minProtoVersion = MIN_PEER_PROTO_VERSION); // Addrman functions - void SetServices(const CService &addr, ServiceFlags nServices); - void MarkAddressGood(const CAddress& addr); - void AddNewAddresses(const std::vector& vAddr, const CAddress& addrFrom, int64_t nTimePenalty = 0); std::vector GetAddresses(); // This allows temporarily exceeding m_max_outbound_full_relay, with the goal of finding @@ -580,7 +577,7 @@ private: std::vector vhListenSocket; std::atomic fNetworkActive{true}; bool fAddressesInitialized{false}; - CAddrMan addrman; + CAddrMan& addrman; std::deque vOneShots GUARDED_BY(cs_vOneShots); CCriticalSection cs_vOneShots; std::vector vAddedNodes GUARDED_BY(cs_vAddedNodes); @@ -708,7 +705,7 @@ public: virtual bool ProcessMessages(CNode* pnode, std::atomic& interrupt) = 0; virtual bool SendMessages(CNode* pnode) = 0; virtual void InitializeNode(CNode* pnode) = 0; - virtual void FinalizeNode(const CNode& node, bool& update_connection_time) = 0; + virtual void FinalizeNode(const CNode& node) = 0; protected: /** diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f5b2b912d3..a99f2cbbbd 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -965,11 +965,11 @@ void PeerLogicValidation::ReattemptInitialBroadcast(CScheduler& scheduler) const scheduler.scheduleFromNow([&] { ReattemptInitialBroadcast(scheduler); }, delta); } -void PeerLogicValidation::FinalizeNode(const CNode& node, bool& fUpdateConnectionTime) { +void PeerLogicValidation::FinalizeNode(const CNode& node) { NodeId nodeid = node.GetId(); - fUpdateConnectionTime = false; - LOCK(cs_main); int misbehavior{0}; + LOCK(cs_main); + { { PeerRef peer = GetPeerRef(nodeid); assert(peer != nullptr); @@ -983,11 +983,6 @@ void PeerLogicValidation::FinalizeNode(const CNode& node, bool& fUpdateConnectio if (state->fSyncStarted) nSyncStarted--; - if (node.fSuccessfullyConnected && misbehavior == 0 && !node.m_block_relay_only_peer && !node.fInbound) { - // Note: we avoid changing visible addrman state for block-relay-only peers - fUpdateConnectionTime = true; - } - for (const QueuedBlock& entry : state->vBlocksInFlight) { mapBlocksInFlight.erase(entry.hash); } @@ -1007,6 +1002,15 @@ void PeerLogicValidation::FinalizeNode(const CNode& node, bool& fUpdateConnectio assert(nPeersWithValidatedDownloads == 0); assert(g_outbound_peers_with_protect_from_disconnect == 0); } + } // cs_main + + if (node.fSuccessfullyConnected && misbehavior == 0 && !node.m_block_relay_only_peer && !node.fInbound) { + // Only change visible addrman state for full outbound peers. We don't + // call Connected() for feeler connections since they don't have + // fSuccessfullyConnected set. + m_addrman.Connected(node.addr); + } + LogPrint(BCLog::NET, "Cleared nodestate for peer=%d\n", nodeid); } @@ -1296,9 +1300,10 @@ static bool BlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Para (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, consensusParams) < STALE_RELAY_AGE_LIMIT); } -PeerLogicValidation::PeerLogicValidation(CConnman& connman, BanMan* banman, CScheduler &scheduler, ChainstateManager& chainman, CTxMemPool& pool, +PeerLogicValidation::PeerLogicValidation(CConnman& connman, CAddrMan& addrman, BanMan* banman, CScheduler &scheduler, ChainstateManager& chainman, CTxMemPool& pool, std::unique_ptr& llmq_ctx) : m_connman(connman), + m_addrman(addrman), m_banman(banman), m_chainman(chainman), m_mempool(pool), @@ -2644,7 +2649,7 @@ void PeerLogicValidation::ProcessMessage( nServices = ServiceFlags(nServiceInt); if (!pfrom.fInbound) { - m_connman.SetServices(pfrom.addr, nServices); + m_addrman.SetServices(pfrom.addr, nServices); } if (!pfrom.fInbound && !pfrom.fFeeler && !pfrom.m_manual_connection && !HasAllDesirableServiceFlags(nServices)) { @@ -2782,7 +2787,7 @@ void PeerLogicValidation::ProcessMessage( // Get recent addresses m_connman.PushMessage(&pfrom, CNetMsgMaker(nSendVersion).Make(NetMsgType::GETADDR)); pfrom.fGetAddr = true; - m_connman.MarkAddressGood(pfrom.addr); + m_addrman.Good(pfrom.addr); } std::string remoteAddr; @@ -2954,7 +2959,7 @@ void PeerLogicValidation::ProcessMessage( if (fReachable) vAddrOk.push_back(addr); } - m_connman.AddNewAddresses(vAddrOk, pfrom.addr, 2 * 60 * 60); + m_addrman.Add(vAddrOk, pfrom.addr, 2 * 60 * 60); if (vAddr.size() < 1000) pfrom.fGetAddr = false; if (pfrom.fOneShot) diff --git a/src/net_processing.h b/src/net_processing.h index 904c266017..2f94dae0b2 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -11,6 +11,7 @@ #include #include +class CAddrMan; class CTxMemPool; class ChainstateManager; struct LLMQContext; @@ -28,6 +29,7 @@ class PeerLogicValidation final : public CValidationInterface, public NetEventsI private: CConnman& m_connman; BanMan* const m_banman; + CAddrMan& m_addrman; ChainstateManager& m_chainman; CTxMemPool& m_mempool; std::unique_ptr& m_llmq_ctx; @@ -35,7 +37,7 @@ private: bool MaybeDiscourageAndDisconnect(CNode& pnode); public: - PeerLogicValidation(CConnman& connman, BanMan* banman, CScheduler &scheduler, ChainstateManager& chainman, CTxMemPool& pool, + PeerLogicValidation(CConnman& connman, CAddrMan& addrman, BanMan* banman, CScheduler &scheduler, ChainstateManager& chainman, CTxMemPool& pool, std::unique_ptr& llmq_ctx); /** @@ -59,7 +61,7 @@ public: /** Initialize a peer by adding it to mapNodeState and pushing a message requesting its version */ void InitializeNode(CNode* pnode) override; /** Handle removal of a peer by updating various state and removing it from mapNodeState */ - void FinalizeNode(const CNode& node, bool& fUpdateConnectionTime) override; + void FinalizeNode(const CNode& node) override; /** * Process protocol messages received from a given node * diff --git a/src/node/context.cpp b/src/node/context.cpp index cf38c28898..2fed2df7a4 100644 --- a/src/node/context.cpp +++ b/src/node/context.cpp @@ -4,6 +4,7 @@ #include +#include #include #include #include diff --git a/src/node/context.h b/src/node/context.h index f0ec081cac..86d3dbc087 100644 --- a/src/node/context.h +++ b/src/node/context.h @@ -12,6 +12,7 @@ class ArgsManager; class BanMan; +class CAddrMan; class CBlockPolicyEstimator; class CConnman; class CScheduler; @@ -37,6 +38,7 @@ class WalletClient; //! any member functions. It should just be a collection of references that can //! be used without pulling in unwanted dependencies or functionality. struct NodeContext { + std::unique_ptr addrman; std::unique_ptr connman; std::unique_ptr mempool; std::unique_ptr fee_estimator; diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 58abd23e81..464070949c 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -80,9 +80,9 @@ BOOST_FIXTURE_TEST_SUITE(denialofservice_tests, TestingSetup) // work. BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) { - auto connman = std::make_unique(0x1337, 0x1337); + auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); auto peerLogic = std::make_unique( - *connman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx + *connman, *m_node.addrman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx ); // Mock an outbound peer @@ -133,8 +133,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) BOOST_CHECK(dummyNode1.fDisconnect == true); SetMockTime(0); - bool dummy; - peerLogic->FinalizeNode(dummyNode1, dummy); + peerLogic->FinalizeNode(dummyNode1); } static void AddRandomOutboundPeer(std::vector &vNodes, PeerLogicValidation &peerLogic, CConnmanTest* connman) @@ -153,9 +152,9 @@ static void AddRandomOutboundPeer(std::vector &vNodes, PeerLogicValidat BOOST_AUTO_TEST_CASE(stale_tip_peer_management) { - auto connman = std::make_unique(0x1337, 0x1337); + auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); auto peerLogic = std::make_unique( - *connman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx + *connman, *m_node.addrman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx ); const Consensus::Params& consensusParams = Params().GetConsensus(); @@ -217,9 +216,8 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) BOOST_CHECK(vNodes[max_outbound_full_relay-1]->fDisconnect == true); BOOST_CHECK(vNodes.back()->fDisconnect == false); - bool dummy; for (const CNode *node : vNodes) { - peerLogic->FinalizeNode(*node, dummy); + peerLogic->FinalizeNode(*node); } connman->ClearNodes(); @@ -228,9 +226,9 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) BOOST_AUTO_TEST_CASE(DoS_banning) { auto banman = std::make_unique(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME); - auto connman = std::make_unique(0x1337, 0x1337); + auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); auto peerLogic = std::make_unique( - *connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx + *connman, *m_node.addrman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx ); banman->ClearBanned(); @@ -268,17 +266,16 @@ BOOST_AUTO_TEST_CASE(DoS_banning) } BOOST_CHECK(banman->IsDiscouraged(addr2)); - bool dummy; - peerLogic->FinalizeNode(dummyNode1, dummy); - peerLogic->FinalizeNode(dummyNode2, dummy); + peerLogic->FinalizeNode(dummyNode1); + peerLogic->FinalizeNode(dummyNode2); } BOOST_AUTO_TEST_CASE(DoS_banscore) { auto banman = std::make_unique(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME); - auto connman = std::make_unique(0x1337, 0x1337); + auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); auto peerLogic = std::make_unique( - *connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx + *connman, *m_node.addrman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx ); banman->ClearBanned(); @@ -315,16 +312,15 @@ BOOST_AUTO_TEST_CASE(DoS_banscore) BOOST_CHECK(banman->IsDiscouraged(addr1)); gArgs.ForceSetArg("-banscore", ToString(DEFAULT_BANSCORE_THRESHOLD)); - bool dummy; - peerLogic->FinalizeNode(dummyNode1, dummy); + peerLogic->FinalizeNode(dummyNode1); } BOOST_AUTO_TEST_CASE(DoS_bantime) { auto banman = std::make_unique(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME); - auto connman = std::make_unique(0x1337, 0x1337); + auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); auto peerLogic = std::make_unique( - *connman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx + *connman, *m_node.addrman, banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx ); banman->ClearBanned(); @@ -345,8 +341,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) } BOOST_CHECK(banman->IsDiscouraged(addr)); - bool dummy; - peerLogic->FinalizeNode(dummyNode, dummy); + peerLogic->FinalizeNode(dummyNode); } static CTransactionRef RandomOrphan() diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index edc884ef01..f2c52f7a37 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -23,116 +23,94 @@ void initialize_connman() FUZZ_TARGET_INIT(connman, initialize_connman) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; - CConnman connman{fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral()}; - CAddress random_address; + CAddrMan addrman; + CConnman connman{fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral(), addrman}; CNetAddr random_netaddr; CNode random_node = ConsumeNode(fuzzed_data_provider); - CService random_service; CSubNet random_subnet; std::string random_string; while (fuzzed_data_provider.ConsumeBool()) { - switch (fuzzed_data_provider.ConsumeIntegralInRange(0, 28)) { + switch (fuzzed_data_provider.ConsumeIntegralInRange(0, 23)) { case 0: - random_address = ConsumeAddress(fuzzed_data_provider); - break; - case 1: random_netaddr = ConsumeNetAddr(fuzzed_data_provider); break; - case 2: - random_service = ConsumeService(fuzzed_data_provider); - break; - case 3: + case 1: random_subnet = ConsumeSubNet(fuzzed_data_provider); break; - case 4: + case 2: random_string = fuzzed_data_provider.ConsumeRandomLengthString(64); break; - case 5: { - std::vector addresses; - while (fuzzed_data_provider.ConsumeBool()) { - addresses.push_back(ConsumeAddress(fuzzed_data_provider)); - } - // Limit nTimePenalty to int32_t to avoid signed integer overflow - (void)connman.AddNewAddresses(addresses, ConsumeAddress(fuzzed_data_provider), fuzzed_data_provider.ConsumeIntegral()); - break; - } - case 6: + case 3: connman.AddNode(random_string); break; - case 7: + case 4: connman.CheckIncomingNonce(fuzzed_data_provider.ConsumeIntegral()); break; - case 8: + case 5: connman.DisconnectNode(fuzzed_data_provider.ConsumeIntegral()); break; - case 9: + case 6: connman.DisconnectNode(random_netaddr); break; - case 10: + case 7: connman.DisconnectNode(random_string); break; - case 11: + case 8: connman.DisconnectNode(random_subnet); break; - case 12: + case 9: connman.ForEachNode([](auto) {}); break; - case 13: + case 10: connman.ForEachNodeThen([](auto) {}, []() {}); break; - case 14: + case 11: (void)connman.ForNode(fuzzed_data_provider.ConsumeIntegral(), [&](auto) { return fuzzed_data_provider.ConsumeBool(); }); break; - case 15: + case 12: (void)connman.GetAddresses(); break; - case 16: { + case 13: { (void)connman.GetAddresses(); break; } - case 17: + case 14: (void)connman.GetDeterministicRandomizer(fuzzed_data_provider.ConsumeIntegral()); break; - case 18: + case 15: (void)connman.GetNodeCount(fuzzed_data_provider.PickValueInArray({CConnman::CONNECTIONS_NONE, CConnman::CONNECTIONS_IN, CConnman::CONNECTIONS_OUT, CConnman::CONNECTIONS_ALL})); break; - case 19: - connman.MarkAddressGood(random_address); - break; - case 20: + case 16: (void)connman.OutboundTargetReached(fuzzed_data_provider.ConsumeBool()); break; - case 21: + case 17: // Limit now to int32_t to avoid signed integer overflow (void)connman.PoissonNextSendInbound(fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral()); break; - case 22: { + case 18: { CSerializedNetMsg serialized_net_msg; serialized_net_msg.command = fuzzed_data_provider.ConsumeRandomLengthString(CMessageHeader::COMMAND_SIZE); serialized_net_msg.data = ConsumeRandomLengthByteVector(fuzzed_data_provider); connman.PushMessage(&random_node, std::move(serialized_net_msg)); break; } - case 23: + case 19: connman.RemoveAddedNode(random_string); break; - case 24: { + case 20: { const std::vector asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider); if (SanityCheckASMap(asmap)) { connman.SetAsmap(asmap); } break; } - case 25: + case 21: connman.SetBestHeight(fuzzed_data_provider.ConsumeIntegral()); break; - case 26: + case 22: connman.SetNetworkActive(fuzzed_data_provider.ConsumeBool()); break; - case 27: - connman.SetServices(random_service, static_cast(fuzzed_data_provider.ConsumeIntegral())); - break; - case 28: + case 23: connman.SetTryNewOutboundPeer(fuzzed_data_provider.ConsumeBool()); break; } diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp index 2cd31c4002..7a32948f8b 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -52,7 +52,8 @@ FUZZ_TARGET_INIT(net, initialize_net) while (fuzzed_data_provider.ConsumeBool()) { switch (fuzzed_data_provider.ConsumeIntegralInRange(0, 12)) { case 0: { - CConnman connman{fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral()}; + CAddrMan addrman; + CConnman connman{fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral(), addrman}; node.CloseSocketDisconnect(&connman); break; } diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 9f9a5ea9d0..8cad26fec4 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -4,6 +4,7 @@ #include +#include #include #include #include @@ -131,11 +132,12 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve SetupNetworking(); InitSignatureCache(); InitScriptExecutionCache(); + m_node.addrman = std::make_unique(); m_node.chain = interfaces::MakeChain(m_node); g_wallet_init_interface.Construct(m_node); fCheckBlockIndex = true; m_node.evodb = std::make_unique(1 << 20, true, true); - connman = std::make_unique(0x1337, 0x1337); + connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); deterministicMNManager.reset(new CDeterministicMNManager(*m_node.evodb, *connman)); llmq::quorumSnapshotManager.reset(new llmq::CQuorumSnapshotManager(*m_node.evodb)); static bool noui_connected = false; @@ -175,7 +177,7 @@ ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::ve m_node.chainman = &::g_chainman; - m_node.connman = std::make_unique(0x1337, 0x1337); // Deterministic randomness for tests. + m_node.connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests. ::sporkManager = std::make_unique(); ::governance = std::make_unique(); @@ -211,10 +213,11 @@ ChainTestingSetup::~ChainTestingSetup() ::governance.reset(); ::sporkManager.reset(); m_node.connman.reset(); + m_node.addrman.reset(); + m_node.args = nullptr; m_node.banman.reset(); UnloadBlockIndex(m_node.mempool.get()); m_node.mempool.reset(); - m_node.args = nullptr; m_node.scheduler.reset(); m_node.llmq_ctx.reset(); m_node.chainman->Reset(); @@ -242,7 +245,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME); m_node.peer_logic = std::make_unique( - *m_node.connman, m_node.banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx + *m_node.connman, *m_node.addrman, m_node.banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool, m_node.llmq_ctx ); { CConnman::Options options;