From aa88e23f6b9c5d59adbe9b7232e192aee6a812a8 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 9 Jul 2018 20:06:39 -0400 Subject: [PATCH 01/10] Remove useless mapRequest tracking that just effects Qt display. I thought we had removed this a long time ago, TBH, its really confusing feedback to users that we display whether a tx was broadcast to immediate neighbor nodes, given that has little indication of whether the tx propagated very far. --- src/main.cpp | 6 ------ src/validationinterface.cpp | 3 --- src/validationinterface.h | 3 --- src/wallet/wallet.cpp | 42 ------------------------------------- src/wallet/wallet.h | 12 ----------- 5 files changed, 66 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index a4b8ffe81..7f3c58006 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -6395,9 +6395,6 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam } } - // Track requests for our stuff. - GetMainSignals().Inventory(inv.hash); - if (inv.type == MSG_BLOCK || inv.type == MSG_FILTERED_BLOCK) break; } @@ -6764,9 +6761,6 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string pfrom->AskFor(inv); } - // Track requests for our stuff - GetMainSignals().Inventory(inv.hash); - if (pfrom->nSendSize > (SendBufferSize() * 2)) { Misbehaving(pfrom->GetId(), 50); return error("send buffer size() = %u", pfrom->nSendSize); diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 0dc5292e7..f490c5e12 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -37,7 +37,6 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) { g_signals.EraseTransaction.connect(boost::bind(&CValidationInterface::EraseFromWallet, pwalletIn, _1)); g_signals.UpdatedTransaction.connect(boost::bind(&CValidationInterface::UpdatedTransaction, pwalletIn, _1)); g_signals.ChainTip.connect(boost::bind(&CValidationInterface::ChainTip, pwalletIn, _1, _2, _3)); - g_signals.Inventory.connect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1)); g_signals.Broadcast.connect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1)); g_signals.BlockChecked.connect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2)); g_signals.AddressForMining.connect(boost::bind(&CValidationInterface::GetAddressForMining, pwalletIn, _1)); @@ -49,7 +48,6 @@ void UnregisterValidationInterface(CValidationInterface* pwalletIn) { g_signals.AddressForMining.disconnect(boost::bind(&CValidationInterface::GetAddressForMining, pwalletIn, _1)); g_signals.BlockChecked.disconnect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2)); g_signals.Broadcast.disconnect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1)); - g_signals.Inventory.disconnect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1)); g_signals.ChainTip.disconnect(boost::bind(&CValidationInterface::ChainTip, pwalletIn, _1, _2, _3)); g_signals.UpdatedTransaction.disconnect(boost::bind(&CValidationInterface::UpdatedTransaction, pwalletIn, _1)); g_signals.EraseTransaction.disconnect(boost::bind(&CValidationInterface::EraseFromWallet, pwalletIn, _1)); @@ -63,7 +61,6 @@ void UnregisterAllValidationInterfaces() { g_signals.AddressForMining.disconnect_all_slots(); g_signals.BlockChecked.disconnect_all_slots(); g_signals.Broadcast.disconnect_all_slots(); - g_signals.Inventory.disconnect_all_slots(); g_signals.ChainTip.disconnect_all_slots(); g_signals.UpdatedTransaction.disconnect_all_slots(); g_signals.EraseTransaction.disconnect_all_slots(); diff --git a/src/validationinterface.h b/src/validationinterface.h index 691e0f14b..300318296 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -92,7 +92,6 @@ protected: virtual void EraseFromWallet(const uint256 &hash) {} virtual void ChainTip(const CBlockIndex *pindex, const CBlock *pblock, std::optional added) {} virtual void UpdatedTransaction(const uint256 &hash) {} - virtual void Inventory(const uint256 &hash) {} virtual void ResendWalletTransactions(int64_t nBestBlockTime) {} virtual void BlockChecked(const CBlock&, const CValidationState&) {} virtual void GetAddressForMining(std::optional&) {}; @@ -160,8 +159,6 @@ struct CMainSignals { boost::signals2::signal UpdatedTransaction; /** Notifies listeners of a change to the tip of the active block chain. */ boost::signals2::signal)> ChainTip; - /** Notifies listeners about an inventory item being seen on the network. */ - boost::signals2::signal Inventory; /** Tells listeners to broadcast their data. */ boost::signals2::signal Broadcast; /** Notifies listeners of a block validation result */ diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 898cf37dc..234a14621 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4455,45 +4455,6 @@ int64_t CWalletTx::GetTxTime() const return n ? n : nTimeReceived; } -int CWalletTx::GetRequestCount() const -{ - // Returns -1 if it wasn't being tracked - int nRequests = -1; - { - LOCK(pwallet->cs_wallet); - if (IsCoinBase()) - { - // Generated block - if (!hashBlock.IsNull()) - { - map::const_iterator mi = pwallet->mapRequestCount.find(hashBlock); - if (mi != pwallet->mapRequestCount.end()) - nRequests = (*mi).second; - } - } - else - { - // Did anyone request this transaction? - map::const_iterator mi = pwallet->mapRequestCount.find(GetHash()); - if (mi != pwallet->mapRequestCount.end()) - { - nRequests = (*mi).second; - - // How about the block it's in? - if (nRequests == 0 && !hashBlock.IsNull()) - { - map::const_iterator _mi = pwallet->mapRequestCount.find(hashBlock); - if (_mi != pwallet->mapRequestCount.end()) - nRequests = (*_mi).second; - else - nRequests = 1; // If it's in someone else's block it must have got out - } - } - } - } - return nRequests; -} - // GetAmounts will determine the transparent debits and credits for a given wallet tx. void CWalletTx::GetAmounts(std::list& listReceived, std::list& listSent, CAmount& nFee, const isminefilter& filter) const @@ -5914,9 +5875,6 @@ bool CWallet::CommitTransaction(CWalletTx& wtxNew, std::optional& asOfHeight) const; int64_t GetTxTime() const; - int GetRequestCount() const; bool RelayWalletTransaction(); @@ -1416,7 +1415,6 @@ public: TxItems wtxOrdered; int64_t nOrderPosNext; - std::map mapRequestCount; std::map mapAddressBook; @@ -1971,16 +1969,6 @@ public: void UpdatedTransaction(const uint256 &hashTx); - void Inventory(const uint256 &hash) - { - { - LOCK(cs_wallet); - std::map::iterator mi = mapRequestCount.find(hash); - if (mi != mapRequestCount.end()) - (*mi).second++; - } - } - void GetAddressForMining(std::optional &minerAddress); void ResetRequestCount(const uint256 &hash) { From 7c739e2b20ff629bb4bf8150ea690b8be6c6d3d4 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Wed, 30 Jun 2021 11:52:40 -0700 Subject: [PATCH 02/10] Rate limit the processing of incoming addr messages While limitations on the influence of attackers on addrman already exist (affected buckets are restricted to a subset based on incoming IP / network group), there is no reason to permit them to let them feed us addresses at more than a multiple of the normal network rate. This commit introduces a "token bucket" rate limiter for the processing of addresses in incoming ADDR and ADDRV2 messages. Every connection gets an associated token bucket. Processing an address in an ADDR or ADDRV2 message from non-whitelisted peers consumes a token from the bucket. If the bucket is empty, the address is ignored (it is not forwarded or processed). The token counter increases at a rate of 0.1 tokens per second, and will accrue up to a maximum of 1000 tokens (the maximum we accept in a single ADDR or ADDRV2). When a GETADDR is sent to a peer, it immediately gets 1000 additional tokens, as we actively desire many addresses from such peers (this may temporarily cause the token count to exceed 1000). The rate limit of 0.1 addr/s was chosen based on observation of honest nodes on the network. Activity in general from most nodes is either 0, or up to a maximum around 0.025 addr/s for recent Bitcoin Core nodes. A few (self-identified, through subver) crawler nodes occasionally exceed 0.1 addr/s. (cherry-picked from commit bitcoin/bitcoin@0d64b8f709b4655d8702f810d4876cd8d96ded82) --- src/main.cpp | 21 +++++++++++++++++++++ src/net.h | 13 +++++++++++++ 2 files changed, 34 insertions(+) diff --git a/src/main.cpp b/src/main.cpp index a4b8ffe81..1561a5bc1 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -6564,6 +6564,10 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string { pfrom->PushMessage("getaddr"); pfrom->fGetAddr = true; + + // When requesting a getaddr, accept an additional MAX_ADDR_TO_SEND addresses in response + // (bypassing the MAX_ADDR_PROCESSING_TOKEN_BUCKET limit). + pfrom->m_addr_token_bucket += MAX_ADDR_TO_SEND; } addrman.Good(pfrom->addr); } else { @@ -6656,10 +6660,27 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string vector vAddrOk; int64_t nNow = GetTime(); int64_t nSince = nNow - 10 * 60; + + // Update/increment addr rate limiting bucket. + const int64_t current_time = GetTimeMicros(); + if (pfrom->m_addr_token_bucket < MAX_ADDR_PROCESSING_TOKEN_BUCKET) { + // Don't increment bucket if it's already full + const auto time_diff = std::max(current_time - pfrom->m_addr_token_timestamp, (int64_t) 0); + const double increment = (time_diff / 1000000) * MAX_ADDR_RATE_PER_SECOND; + pfrom->m_addr_token_bucket = std::min(pfrom->m_addr_token_bucket + increment, MAX_ADDR_PROCESSING_TOKEN_BUCKET); + } + pfrom->m_addr_token_timestamp = current_time; + for (CAddress& addr : vAddr) { boost::this_thread::interruption_point(); + // Apply rate limiting if the address is not whitelisted + if (!pfrom->IsWhitelistedRange(addr)) { + if (pfrom->m_addr_token_bucket < 1.0) break; + pfrom->m_addr_token_bucket -= 1.0; + } + if (addr.nTime <= 100000000 || addr.nTime > nNow + 10 * 60) addr.nTime = nNow - 5 * 24 * 60 * 60; pfrom->AddAddressKnown(addr); diff --git a/src/net.h b/src/net.h index 874c10fa6..e7e655c39 100644 --- a/src/net.h +++ b/src/net.h @@ -50,6 +50,13 @@ static const int TIMEOUT_INTERVAL = 20 * 60; static const unsigned int MAX_INV_SZ = 50000; /** The maximum number of new addresses to accumulate before announcing. */ static const unsigned int MAX_ADDR_TO_SEND = 1000; +/** The maximum rate of address records we're willing to process on average. Can be bypassed using + * the NetPermissionFlags::Addr permission. */ +static constexpr double MAX_ADDR_RATE_PER_SECOND{0.1}; +/** The soft limit of the address processing token bucket (the regular MAX_ADDR_RATE_PER_SECOND + * based increments won't go above this, but the MAX_ADDR_TO_SEND increment following GETADDR + * is exempt from this limit. */ +static constexpr size_t MAX_ADDR_PROCESSING_TOKEN_BUCKET{MAX_ADDR_TO_SEND}; /** Maximum length of incoming protocol messages (no message over 2 MiB is currently acceptable). */ static const unsigned int MAX_PROTOCOL_MESSAGE_LENGTH = 2 * 1024 * 1024; /** Maximum length of strSubVer in `version` message */ @@ -339,6 +346,12 @@ public: int64_t nNextAddrSend; int64_t nNextLocalAddrSend; + /** Number of addr messages that can be processed from this peer. Start at 1 to + * permit self-announcement. */ + double m_addr_token_bucket{1.0}; + /** When m_addr_token_bucket was last updated */ + int64_t m_addr_token_timestamp{GetTimeMicros()}; + // inventory based relay CRollingBloomFilter filterInventoryKnown; // Set of transaction ids we still have to announce. From 2c48eddfa5f1f3248d2df845e662a82bac957742 Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Fri, 17 Feb 2023 12:50:32 -0700 Subject: [PATCH 03/10] Remove `ResetRequestCount` There is no longer any `mapRequestCount` to reset. This also removes the `BlockFound` signal, as its only purpose was to call `ResetRequestCount`. --- src/miner.cpp | 3 --- src/validationinterface.cpp | 3 --- src/validationinterface.h | 3 --- src/wallet/wallet.h | 5 ----- 4 files changed, 14 deletions(-) diff --git a/src/miner.cpp b/src/miner.cpp index f3e6264d1..7383fd7a4 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -877,9 +877,6 @@ static bool ProcessBlockFound(const CBlock* pblock, const CChainParams& chainpar return error("ZcashMiner: generated block is stale"); } - // Inform about the new block - GetMainSignals().BlockFound(pblock->GetHash()); - // Process this block the same as if we had received it from another node CValidationState state; if (!ProcessNewBlock(state, chainparams, NULL, pblock, true, NULL)) diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index f490c5e12..ec11ef190 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -40,11 +40,9 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) { g_signals.Broadcast.connect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1)); g_signals.BlockChecked.connect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2)); g_signals.AddressForMining.connect(boost::bind(&CValidationInterface::GetAddressForMining, pwalletIn, _1)); - g_signals.BlockFound.connect(boost::bind(&CValidationInterface::ResetRequestCount, pwalletIn, _1)); } void UnregisterValidationInterface(CValidationInterface* pwalletIn) { - g_signals.BlockFound.disconnect(boost::bind(&CValidationInterface::ResetRequestCount, pwalletIn, _1)); g_signals.AddressForMining.disconnect(boost::bind(&CValidationInterface::GetAddressForMining, pwalletIn, _1)); g_signals.BlockChecked.disconnect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2)); g_signals.Broadcast.disconnect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1)); @@ -57,7 +55,6 @@ void UnregisterValidationInterface(CValidationInterface* pwalletIn) { } void UnregisterAllValidationInterfaces() { - g_signals.BlockFound.disconnect_all_slots(); g_signals.AddressForMining.disconnect_all_slots(); g_signals.BlockChecked.disconnect_all_slots(); g_signals.Broadcast.disconnect_all_slots(); diff --git a/src/validationinterface.h b/src/validationinterface.h index 300318296..c8ddb55bd 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -95,7 +95,6 @@ protected: virtual void ResendWalletTransactions(int64_t nBestBlockTime) {} virtual void BlockChecked(const CBlock&, const CValidationState&) {} virtual void GetAddressForMining(std::optional&) {}; - virtual void ResetRequestCount(const uint256 &hash) {}; friend void ::RegisterValidationInterface(CValidationInterface*); friend void ::UnregisterValidationInterface(CValidationInterface*); friend void ::UnregisterAllValidationInterfaces(); @@ -165,8 +164,6 @@ struct CMainSignals { boost::signals2::signal BlockChecked; /** Notifies listeners that an address for mining is required (coinbase) */ boost::signals2::signal&)> AddressForMining; - /** Notifies listeners that a block has been successfully mined */ - boost::signals2::signal BlockFound; }; CMainSignals& GetMainSignals(); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 0af1bab8a..ccad82ab9 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1970,11 +1970,6 @@ public: void UpdatedTransaction(const uint256 &hashTx); void GetAddressForMining(std::optional &minerAddress); - void ResetRequestCount(const uint256 &hash) - { - LOCK(cs_wallet); - mapRequestCount[hash] = 0; - }; unsigned int GetKeyPoolSize() { From c8cdfcffd084fd1ec2240b133c25cf247f9fb861 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Thu, 15 Jul 2021 12:59:23 -0700 Subject: [PATCH 04/10] Randomize the order of addr processing (cherry picked from commit bitcoin/bitcoin@5648138f5949013331c017c740646e2f4013bc24) --- src/main.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main.cpp b/src/main.cpp index 1561a5bc1..1fd96aca7 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -37,6 +37,7 @@ #include "wallet/asyncrpcoperation_shieldcoinbase.h" #include "warnings.h" +#include #include #include #include @@ -6671,6 +6672,7 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string } pfrom->m_addr_token_timestamp = current_time; + std::shuffle(vAddr.begin(), vAddr.end(), ZcashRandomEngine()); for (CAddress& addr : vAddr) { boost::this_thread::interruption_point(); From 1dddc1337ca520e686e8ae48b7424dadff7a4781 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Wed, 7 Jul 2021 11:44:40 -0700 Subject: [PATCH 05/10] Add logging and addr rate limiting statistics Includes logging improvements by Vasil Dimov and John Newbery. (cherry picked from commit bitcoin/bitcoin@f424d601e1b6870e20bc60f5ccba36d2e210377b) --- src/main.cpp | 17 ++++++++++++++++- src/net.cpp | 3 +++ src/net.h | 7 +++++++ src/rpc/net.cpp | 2 ++ 4 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/main.cpp b/src/main.cpp index 1fd96aca7..00e57ac51 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -6672,6 +6672,8 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string } pfrom->m_addr_token_timestamp = current_time; + uint64_t num_proc = 0; + uint64_t num_rate_limit = 0; std::shuffle(vAddr.begin(), vAddr.end(), ZcashRandomEngine()); for (CAddress& addr : vAddr) { @@ -6679,13 +6681,17 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string // Apply rate limiting if the address is not whitelisted if (!pfrom->IsWhitelistedRange(addr)) { - if (pfrom->m_addr_token_bucket < 1.0) break; + if (pfrom->m_addr_token_bucket < 1.0) { + ++num_rate_limit; + continue; + } pfrom->m_addr_token_bucket -= 1.0; } if (addr.nTime <= 100000000 || addr.nTime > nNow + 10 * 60) addr.nTime = nNow - 5 * 24 * 60 * 60; pfrom->AddAddressKnown(addr); + ++num_proc; bool fReachable = IsReachable(addr); if (addr.nTime > nSince && !pfrom->fGetAddr && vAddr.size() <= 10 && addr.IsRoutable()) { @@ -6716,6 +6722,15 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string if (fReachable) vAddrOk.push_back(addr); } + pfrom->m_addr_processed += num_proc; + pfrom->m_addr_rate_limited += num_rate_limit; + LogPrintf("ProcessMessage: Received addr: %u addresses (%u processed, %u rate-limited) from peer=%d%s\n", + vAddr.size(), + num_proc, + num_rate_limit, + pfrom->GetId(), + fLogIPs ? ", peeraddr=" + pfrom->addr.ToString() : ""); + addrman.Add(vAddrOk, pfrom->addr, 2 * 60 * 60); if (vAddr.size() < 1000) pfrom->fGetAddr = false; diff --git a/src/net.cpp b/src/net.cpp index aea34c857..7364afc67 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -700,6 +700,9 @@ void CNode::copyStats(CNodeStats &stats) stats.dPingTime = (((double)nPingUsecTime) / 1e6); stats.dPingWait = (((double)nPingUsecWait) / 1e6); + stats.m_addr_processed = m_addr_processed.load(); + stats.m_addr_rate_limited = m_addr_rate_limited.load(); + // Leave string empty if addrLocal invalid (not filled in yet) CService addrLocalUnlocked = GetAddrLocal(); stats.addrLocal = addrLocalUnlocked.IsValid() ? addrLocalUnlocked.ToString() : ""; diff --git a/src/net.h b/src/net.h index e7e655c39..498b55c08 100644 --- a/src/net.h +++ b/src/net.h @@ -212,6 +212,8 @@ public: double dPingTime; double dPingWait; std::string addrLocal; + uint64_t m_addr_processed{0}; + uint64_t m_addr_rate_limited{0}; }; @@ -351,6 +353,11 @@ public: double m_addr_token_bucket{1.0}; /** When m_addr_token_bucket was last updated */ int64_t m_addr_token_timestamp{GetTimeMicros()}; + /** Total number of addresses that were dropped due to rate limiting. */ + std::atomic m_addr_rate_limited{0}; + /** Total number of addresses that were processed (excludes rate limited ones). */ + std::atomic m_addr_processed{0}; + // inventory based relay CRollingBloomFilter filterInventoryKnown; diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index e00e15493..06c2b38cb 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -158,6 +158,8 @@ UniValue getpeerinfo(const UniValue& params, bool fHelp) } obj.pushKV("inflight", heights); } + obj.pushKV("addr_processed", stats.m_addr_processed); + obj.pushKV("addr_rate_limited", stats.m_addr_rate_limited); obj.pushKV("whitelisted", stats.fWhitelisted); ret.push_back(obj); From 9555b5e8a4d9f4dcf70936e0b089f57768fa6de8 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Sat, 31 Jul 2021 21:56:59 +0200 Subject: [PATCH 06/10] p2p, rpc, test: address rate-limiting follow-ups (cherry picked from commit bitcoin/bitcoin@d930c7f5b091687eb4208a5ffe8a2abe311d8054) --- src/main.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 00e57ac51..8a8a5a9d2 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -6680,11 +6680,12 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string boost::this_thread::interruption_point(); // Apply rate limiting if the address is not whitelisted - if (!pfrom->IsWhitelistedRange(addr)) { - if (pfrom->m_addr_token_bucket < 1.0) { + if (pfrom->m_addr_token_bucket < 1.0) { + if (!pfrom->IsWhitelistedRange(addr)) { ++num_rate_limit; continue; } + } else { pfrom->m_addr_token_bucket -= 1.0; } From 0a39cc67271d64bb541573e7994dc97f18a7089e Mon Sep 17 00:00:00 2001 From: Daira Hopwood Date: Fri, 17 Feb 2023 20:38:04 +0000 Subject: [PATCH 07/10] Enable a CRollingBloomFilter to be reset to a state where it takes little memory. Co-authored-by: Jack Grigg Signed-off-by: Daira Hopwood --- src/bloom.cpp | 29 +++++++++++++++++++---------- src/bloom.h | 6 ++++++ src/test/bloom_tests.cpp | 26 ++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 10 deletions(-) diff --git a/src/bloom.cpp b/src/bloom.cpp index 8f0729e71..4f75be990 100644 --- a/src/bloom.cpp +++ b/src/bloom.cpp @@ -187,15 +187,7 @@ CRollingBloomFilter::CRollingBloomFilter(const unsigned int nElements, const dou * => nFilterBits = -nHashFuncs * nMaxElements / log(1.0 - pow(fpRate, 1.0 / nHashFuncs)) * => nFilterBits = -nHashFuncs * nMaxElements / log(1.0 - exp(logFpRate / nHashFuncs)) */ - uint32_t nFilterBits = (uint32_t)ceil(-1.0 * nHashFuncs * nMaxElements / log(1.0 - exp(logFpRate / nHashFuncs))); - data.clear(); - /* For each data element we need to store 2 bits. If both bits are 0, the - * bit is treated as unset. If the bits are (01), (10), or (11), the bit is - * treated as set in generation 1, 2, or 3 respectively. - * These bits are stored in separate integers: position P corresponds to bit - * (P & 63) of the integers data[(P >> 6) * 2] and data[(P >> 6) * 2 + 1]. */ - data.resize(((nFilterBits + 63) / 64) << 1); - reset(); + nFilterBits = (uint32_t)ceil(-1.0 * nHashFuncs * nMaxElements / log(1.0 - exp(logFpRate / nHashFuncs))); } /* Similar to CBloomFilter::Hash */ @@ -213,6 +205,9 @@ static inline uint32_t FastMod(uint32_t x, size_t n) { void CRollingBloomFilter::insert(const std::vector& vKey) { + if (data.empty()) { + initialize(); + } if (nEntriesThisGeneration == nEntriesPerGeneration) { nEntriesThisGeneration = 0; nGeneration++; @@ -250,6 +245,9 @@ void CRollingBloomFilter::insert(const uint256& hash) bool CRollingBloomFilter::contains(const std::vector& vKey) const { + if (data.empty()) { + return false; + } for (int n = 0; n < nHashFuncs; n++) { uint32_t h = RollingBloomHash(n, nTweak, vKey); int bit = h & 0x3F; @@ -268,8 +266,19 @@ bool CRollingBloomFilter::contains(const uint256& hash) const return contains(vData); } -void CRollingBloomFilter::reset() +void CRollingBloomFilter::reset() { + std::vector().swap(data); +} + +void CRollingBloomFilter::initialize() { + /* For each data element we need to store 2 bits. If both bits are 0, the + * bit is treated as unset. If the bits are (01), (10), or (11), the bit is + * treated as set in generation 1, 2, or 3 respectively. + * These bits are stored in separate integers: position P corresponds to bit + * (P & 63) of the integers data[(P >> 6) * 2] and data[(P >> 6) * 2 + 1]. */ + data.resize(((nFilterBits + 63) / 64) << 1); + nTweak = GetRand(std::numeric_limits::max()); nEntriesThisGeneration = 0; nGeneration = 1; diff --git a/src/bloom.h b/src/bloom.h index 9a75658f9..071bbe608 100644 --- a/src/bloom.h +++ b/src/bloom.h @@ -131,7 +131,13 @@ public: void reset(); +protected: + bool is_data_empty() const { return data.empty(); } + private: + void initialize(); + + uint32_t nFilterBits; int nEntriesPerGeneration; int nEntriesThisGeneration; int nGeneration; diff --git a/src/test/bloom_tests.cpp b/src/test/bloom_tests.cpp index ab80d3dc3..56bdf9199 100644 --- a/src/test/bloom_tests.cpp +++ b/src/test/bloom_tests.cpp @@ -538,4 +538,30 @@ BOOST_AUTO_TEST_CASE(rolling_bloom) } } +BOOST_AUTO_TEST_CASE(rolling_bloom_reset) +{ + struct TestRollingBloomFilter : CRollingBloomFilter + { + TestRollingBloomFilter() : CRollingBloomFilter(100, 0.01) {} + bool is_data_empty() const { return CRollingBloomFilter::is_data_empty(); } + }; + + TestRollingBloomFilter rb; + BOOST_CHECK(rb.is_data_empty()); + + std::vector d = RandomData(); + rb.insert(d); + BOOST_CHECK(!rb.is_data_empty()); + BOOST_CHECK(rb.contains(d)); + + // reset() should ensure minimal memory usage. + rb.reset(); + BOOST_CHECK(rb.is_data_empty()); + BOOST_CHECK(!rb.contains(d)); + + rb.insert(d); + BOOST_CHECK(!rb.is_data_empty()); + BOOST_CHECK(rb.contains(d)); +} + BOOST_AUTO_TEST_SUITE_END() From c5b8807ce721beb3c5cbb87bc41a116065094b52 Mon Sep 17 00:00:00 2001 From: Daira Hopwood Date: Mon, 20 Feb 2023 20:14:55 +0000 Subject: [PATCH 08/10] Ensure that CNode::{addrKnown, filterInventoryKnown} immediately take little memory when we disconnect the node. Co-authored-by: Kris Nuttycombe Co-authored-by: Jack Grigg Signed-off-by: Daira Hopwood --- src/main.cpp | 11 ++++++++--- src/net.cpp | 9 ++++++++- src/net.h | 38 ++++++++++++++++++++++++++++---------- 3 files changed, 44 insertions(+), 14 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index a4b8ffe81..07182ce00 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -6662,7 +6662,7 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string if (addr.nTime <= 100000000 || addr.nTime > nNow + 10 * 60) addr.nTime = nNow - 5 * 24 * 60 * 60; - pfrom->AddAddressKnown(addr); + pfrom->AddAddressIfNotAlreadyKnown(addr); bool fReachable = IsReachable(addr); if (addr.nTime > nSince && !pfrom->fGetAddr && vAddr.size() <= 10 && addr.IsRoutable()) { @@ -7603,9 +7603,8 @@ bool SendMessages(const Consensus::Params& params, CNode* pto) vAddr.reserve(pto->vAddrToSend.size()); for (const CAddress& addr : pto->vAddrToSend) { - if (!pto->addrKnown.contains(addr.GetKey())) + if (pto->AddAddressIfNotAlreadyKnown(addr)) { - pto->addrKnown.insert(addr.GetKey()); vAddr.push_back(addr); // receiver rejects addr messages larger than 1000 if (vAddr.size() >= 1000) @@ -7678,6 +7677,12 @@ bool SendMessages(const Consensus::Params& params, CNode* pto) vector vInv; { LOCK(pto->cs_inventory); + // Avoid possibly adding to pto->filterInventoryKnown after it has been reset in CloseSocketDisconnect. + if (pto->fDisconnect) { + // We can safely return here because SendMessages would, in any case, do nothing after + // this block if pto->fDisconnect is set. + return true; + } vInv.reserve(std::max(pto->vInventoryBlockToSend.size(), INVENTORY_BROADCAST_MAX)); // Add blocks diff --git a/src/net.cpp b/src/net.cpp index aea34c857..bbac942f4 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -440,6 +440,14 @@ void CNode::CloseSocketDisconnect() CloseSocket(hSocket); } } + { + LOCK(cs_addrKnown); + addrKnown.reset(); + } + { + LOCK(cs_inventory); + filterInventoryKnown.reset(); + } // in case this fails, we'll empty the recv buffer when the CNode is deleted TRY_LOCK(cs_vRecvMsg, lockRecv); @@ -2221,7 +2229,6 @@ CNode::CNode(SOCKET hSocketIn, const CAddress& addrIn, const std::string& addrNa nSendOffset = 0; hashContinue = uint256(); nStartingHeight = -1; - filterInventoryKnown.reset(); fSendMempool = false; fGetAddr = false; nNextLocalAddrSend = 0; diff --git a/src/net.h b/src/net.h index 874c10fa6..56f88308b 100644 --- a/src/net.h +++ b/src/net.h @@ -304,6 +304,8 @@ public: CBloomFilter* pfilter; NodeId id; std::atomic nRefCount; + CRollingBloomFilter addrKnown; + mutable CCriticalSection cs_addrKnown; const uint64_t nKeyedNetGroup; @@ -333,7 +335,6 @@ public: // flood relay std::vector vAddrToSend; - CRollingBloomFilter addrKnown; bool fGetAddr; std::set setKnown; int64_t nNextAddrSend; @@ -344,7 +345,7 @@ public: // Set of transaction ids we still have to announce. // They are sorted by the mempool before relay, so the order is not important. std::set setInventoryTxToSend; - // List of block ids we still have announce. + // List of block ids we still have to announce. // There is no final sorting before sending, as they are always sent immediately // and in the order requested. std::vector vInventoryBlockToSend; @@ -448,10 +449,25 @@ public: } - - void AddAddressKnown(const CAddress& addr) + bool AddAddressIfNotAlreadyKnown(const CAddress& addr) { - addrKnown.insert(addr.GetKey()); + LOCK(cs_addrKnown); + // Avoid adding to addrKnown after it has been reset in CloseSocketDisconnect. + if (fDisconnect) { + return false; + } + if (!addrKnown.contains(addr.GetKey())) { + addrKnown.insert(addr.GetKey()); + return true; + } else { + return false; + } + } + + bool IsAddressKnown(const CAddress& addr) const + { + LOCK(cs_addrKnown); + return addrKnown.contains(addr.GetKey()); } void PushAddress(const CAddress& addr, FastRandomContext &insecure_rand) @@ -459,7 +475,7 @@ public: // Known checking here is only to save space from duplicates. // SendMessages will filter it again for knowns that were added // after addresses were pushed. - if (addr.IsValid() && !addrKnown.contains(addr.GetKey())) { + if (addr.IsValid() && !IsAddressKnown(addr)) { if (vAddrToSend.size() >= MAX_ADDR_TO_SEND) { vAddrToSend[insecure_rand.randrange(vAddrToSend.size())] = addr; } else { @@ -471,8 +487,8 @@ public: void AddKnownTx(const WTxId& wtxid) { - { - LOCK(cs_inventory); + LOCK(cs_inventory); + if (!fDisconnect) { filterInventoryKnown.insert(wtxid.ToBytes()); } } @@ -480,7 +496,7 @@ public: void PushTxInventory(const WTxId& wtxid) { LOCK(cs_inventory); - if (!filterInventoryKnown.contains(wtxid.ToBytes())) { + if (!fDisconnect && !filterInventoryKnown.contains(wtxid.ToBytes())) { setInventoryTxToSend.insert(wtxid.hash); } } @@ -488,7 +504,9 @@ public: void PushBlockInventory(const uint256& hash) { LOCK(cs_inventory); - vInventoryBlockToSend.push_back(hash); + if (!fDisconnect) { + vInventoryBlockToSend.push_back(hash); + } } void AskFor(const CInv& inv); From 074e6337985e36dd0030366e75a49745a011f328 Mon Sep 17 00:00:00 2001 From: Daira Hopwood Date: Mon, 20 Feb 2023 23:33:04 +0000 Subject: [PATCH 09/10] Improve the encapsulation of `CNode::filterInventoryKnown`. Co-authored-by: Jack Grigg Signed-off-by: Daira Hopwood --- src/main.cpp | 10 +++++----- src/net.h | 24 ++++++++++++++++++++---- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 07182ce00..2efe8ba76 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -6757,7 +6757,7 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string } else { - pfrom->AddKnownTx(WTxId(inv.hash, inv.hashAux)); + pfrom->AddKnownWTxId(WTxId(inv.hash, inv.hashAux)); if (fBlocksOnly) LogPrint("net", "transaction (%s) inv sent in violation of protocol peer=%d\n", inv.hash.ToString(), pfrom->id); else if (!fAlreadyHave && !IsInitialBlockDownload(chainparams.GetConsensus())) @@ -6906,7 +6906,7 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string LOCK(cs_main); - pfrom->AddKnownTx(wtxid); + pfrom->AddKnownWTxId(wtxid); bool fMissingInputs = false; CValidationState state; @@ -7730,7 +7730,7 @@ bool SendMessages(const Consensus::Params& params, CNode* pto) if (pto->pfilter) { if (!pto->pfilter->IsRelevantAndUpdate(*txinfo.tx)) continue; } - pto->filterInventoryKnown.insert(hash); + pto->AddKnownTxId(hash); vInv.push_back(inv); if (vInv.size() == MAX_INV_SZ) { pto->PushMessage("inv", vInv); @@ -7765,7 +7765,7 @@ bool SendMessages(const Consensus::Params& params, CNode* pto) // Remove it from the to-be-sent set pto->setInventoryTxToSend.erase(it); // Check if not in the filter already - if (pto->filterInventoryKnown.contains(hash)) { + if (pto->HasKnownTxId(hash)) { continue; } // Not in the mempool anymore? don't bother sending it. @@ -7800,7 +7800,7 @@ bool SendMessages(const Consensus::Params& params, CNode* pto) pto->PushMessage("inv", vInv); vInv.clear(); } - pto->filterInventoryKnown.insert(hash); + pto->AddKnownTxId(hash); } } } diff --git a/src/net.h b/src/net.h index 56f88308b..b391da020 100644 --- a/src/net.h +++ b/src/net.h @@ -307,6 +307,10 @@ public: CRollingBloomFilter addrKnown; mutable CCriticalSection cs_addrKnown; + // Inventory based relay + // This filter is protected by cs_inventory and contains both txids and wtxids. + CRollingBloomFilter filterInventoryKnown; + const uint64_t nKeyedNetGroup; // Stored so we can pass a pointer to it across the Rust FFI for span. @@ -340,8 +344,6 @@ public: int64_t nNextAddrSend; int64_t nNextLocalAddrSend; - // inventory based relay - CRollingBloomFilter filterInventoryKnown; // Set of transaction ids we still have to announce. // They are sorted by the mempool before relay, so the order is not important. std::set setInventoryTxToSend; @@ -349,7 +351,7 @@ public: // There is no final sorting before sending, as they are always sent immediately // and in the order requested. std::vector vInventoryBlockToSend; - CCriticalSection cs_inventory; + mutable CCriticalSection cs_inventory; std::set setAskFor; std::multimap mapAskFor; int64_t nNextInvSend; @@ -485,7 +487,7 @@ public: } - void AddKnownTx(const WTxId& wtxid) + void AddKnownWTxId(const WTxId& wtxid) { LOCK(cs_inventory); if (!fDisconnect) { @@ -493,6 +495,20 @@ public: } } + void AddKnownTxId(const uint256& txid) + { + LOCK(cs_inventory); + if (!fDisconnect) { + filterInventoryKnown.insert(txid); + } + } + + bool HasKnownTxId(const uint256& txid) const + { + LOCK(cs_inventory); + return filterInventoryKnown.contains(txid); + } + void PushTxInventory(const WTxId& wtxid) { LOCK(cs_inventory); From 1478dc1e7f1ccd89bac76fc738650877fbfd1422 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 20 Feb 2023 19:37:38 -0700 Subject: [PATCH 10/10] Update release notes for v5.3.3 hotfix --- doc/release-notes.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/doc/release-notes.md b/doc/release-notes.md index a29094b51..e3b26957f 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -4,3 +4,7 @@ release-notes at release time) Notable changes =============== +This hotfix remediates memory exhaustion vulnerabilities that zcashd inherited +as a fork of bitcoind. These bugs could allow an attacker to use peer-to-peer +messages to fill the memory of a node, resulting in a crash. +