From bbe41088c61f2ad328766e851ffe6169aa80935a Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Fri, 17 Jul 2015 06:42:43 -0400 Subject: [PATCH 1/6] Add uint256 support to CRollingBloomFilter --- src/bloom.cpp | 22 ++++++++++++++++++++++ src/bloom.h | 2 ++ 2 files changed, 24 insertions(+) diff --git a/src/bloom.cpp b/src/bloom.cpp index 36cba491c4eb6..e15bc32f974ba 100644 --- a/src/bloom.cpp +++ b/src/bloom.cpp @@ -234,6 +234,20 @@ void CRollingBloomFilter::insert(const std::vector& vKey) } } +void CRollingBloomFilter::insert(const uint256& hash) +{ + if (nInsertions == 0) { + b1.clear(); + } else if (nInsertions == nBloomSize / 2) { + b2.clear(); + } + b1.insert(hash); + b2.insert(hash); + if (++nInsertions == nBloomSize) { + nInsertions = 0; + } +} + bool CRollingBloomFilter::contains(const std::vector& vKey) const { if (nInsertions < nBloomSize / 2) { @@ -242,6 +256,14 @@ bool CRollingBloomFilter::contains(const std::vector& vKey) const return b1.contains(vKey); } +bool CRollingBloomFilter::contains(const uint256& hash) const +{ + if (nInsertions < nBloomSize / 2) { + return b2.contains(hash); + } + return b1.contains(hash); +} + void CRollingBloomFilter::clear() { b1.clear(); diff --git a/src/bloom.h b/src/bloom.h index bb17f59c863ce..0daa3728ed0e4 100644 --- a/src/bloom.h +++ b/src/bloom.h @@ -114,7 +114,9 @@ class CRollingBloomFilter CRollingBloomFilter(unsigned int nElements, double nFPRate, unsigned int nTweak); void insert(const std::vector& vKey); + void insert(const uint256& hash); bool contains(const std::vector& vKey) const; + bool contains(const uint256& hash) const; void clear(); From a3d65fedaa18686f0cc007d0a13dba6545250300 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Mon, 27 Jul 2015 18:38:45 +0200 Subject: [PATCH 2/6] Reuse vector hashing code for uint256 --- src/bloom.cpp | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/src/bloom.cpp b/src/bloom.cpp index e15bc32f974ba..3f50b1da917e0 100644 --- a/src/bloom.cpp +++ b/src/bloom.cpp @@ -236,16 +236,8 @@ void CRollingBloomFilter::insert(const std::vector& vKey) void CRollingBloomFilter::insert(const uint256& hash) { - if (nInsertions == 0) { - b1.clear(); - } else if (nInsertions == nBloomSize / 2) { - b2.clear(); - } - b1.insert(hash); - b2.insert(hash); - if (++nInsertions == nBloomSize) { - nInsertions = 0; - } + vector data(hash.begin(), hash.end()); + insert(data); } bool CRollingBloomFilter::contains(const std::vector& vKey) const @@ -258,10 +250,8 @@ bool CRollingBloomFilter::contains(const std::vector& vKey) const bool CRollingBloomFilter::contains(const uint256& hash) const { - if (nInsertions < nBloomSize / 2) { - return b2.contains(hash); - } - return b1.contains(hash); + vector data(hash.begin(), hash.end()); + return contains(data); } void CRollingBloomFilter::clear() From d2d7ee0e863b286e1c9f9c54659d494fb0a7712d Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Mon, 20 Jul 2015 04:43:34 +0900 Subject: [PATCH 3/6] Make CRollingBloomFilter set nTweak for you While CBloomFilter is usually used with an explicitly set nTweak, CRollingBloomFilter is only used internally. Requiring every caller to set nTweak is error-prone and redundant; better to have the class handle that for you with a high-quality randomness source. Additionally when clearing the filter it makes sense to change nTweak as well to recover from a bad setting, e.g. due to insufficient randomness at initialization, so the clear() method is replaced by a reset() method that sets a new, random, nTweak value. --- src/bloom.cpp | 19 +++++++++++++++---- src/bloom.h | 12 +++++++++--- src/main.cpp | 2 +- src/net.cpp | 2 +- src/test/bloom_tests.cpp | 6 +++--- 5 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/bloom.cpp b/src/bloom.cpp index 3f50b1da917e0..89959d7326797 100644 --- a/src/bloom.cpp +++ b/src/bloom.cpp @@ -8,6 +8,7 @@ #include "hash.h" #include "script/script.h" #include "script/standard.h" +#include "random.h" #include "streams.h" #include @@ -121,6 +122,12 @@ void CBloomFilter::clear() isEmpty = true; } +void CBloomFilter::reset(unsigned int nNewTweak) +{ + clear(); + nTweak = nNewTweak; +} + bool CBloomFilter::IsWithinSizeConstraints() const { return vData.size() <= MAX_BLOOM_FILTER_SIZE && nHashFuncs <= MAX_HASH_FUNCS; @@ -217,7 +224,8 @@ CRollingBloomFilter::CRollingBloomFilter(unsigned int nElements, double fpRate, // inserted, so at least one always contains the last nElements // inserted. nBloomSize = nElements * 2; - nInsertions = 0; + + reset(nTweak); } void CRollingBloomFilter::insert(const std::vector& vKey) @@ -254,9 +262,12 @@ bool CRollingBloomFilter::contains(const uint256& hash) const return contains(data); } -void CRollingBloomFilter::clear() +void CRollingBloomFilter::reset(unsigned int nNewTweak) { - b1.clear(); - b2.clear(); + if (!nNewTweak) + nNewTweak = GetRand(std::numeric_limits::max()); + + b1.reset(nNewTweak); + b2.reset(nNewTweak); nInsertions = 0; } diff --git a/src/bloom.h b/src/bloom.h index 0daa3728ed0e4..12bf6d99a88c6 100644 --- a/src/bloom.h +++ b/src/bloom.h @@ -89,6 +89,7 @@ class CBloomFilter bool contains(const uint256& hash) const; void clear(); + void reset(unsigned int nNewTweak); //! True if the size is <= MAX_BLOOM_FILTER_SIZE and the number of hash functions is <= MAX_HASH_FUNCS //! (catch a filter which was just deserialized which was too big) @@ -103,7 +104,11 @@ class CBloomFilter /** * RollingBloomFilter is a probabilistic "keep track of most recently inserted" set. - * Construct it with the number of items to keep track of, and a false-positive rate. + * Construct it with the number of items to keep track of, and a false-positive + * rate. Unlike CBloomFilter, by default nTweak is set to a cryptographically + * secure random value for you. Similarly rather than clear() the method + * reset() is provided, which also changes nTweak to decrease the impact of + * false-positives. * * contains(item) will always return true if item was one of the last N things * insert()'ed ... but may also return true for items that were not inserted. @@ -111,14 +116,15 @@ class CBloomFilter class CRollingBloomFilter { public: - CRollingBloomFilter(unsigned int nElements, double nFPRate, unsigned int nTweak); + CRollingBloomFilter(unsigned int nElements, double nFPRate, + unsigned int nTweak = 0); void insert(const std::vector& vKey); void insert(const uint256& hash); bool contains(const std::vector& vKey) const; bool contains(const uint256& hash) const; - void clear(); + void reset(unsigned int nNewTweak = 0); private: unsigned int nBloomSize; diff --git a/src/main.cpp b/src/main.cpp index 7477d08b18e1e..01b62bdf608d8 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4812,7 +4812,7 @@ bool SendMessages(CNode* pto, bool fSendTrickle) { // Periodically clear addrKnown to allow refresh broadcasts if (nLastRebroadcast) - pnode->addrKnown.clear(); + pnode->addrKnown.reset(); // Rebroadcast our address AdvertizeLocal(pnode); diff --git a/src/net.cpp b/src/net.cpp index 3cece520de683..176fd7195ba62 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2060,7 +2060,7 @@ unsigned int SendBufferSize() { return 1000*GetArg("-maxsendbuffer", 1*1000); } CNode::CNode(SOCKET hSocketIn, const CAddress& addrIn, const std::string& addrNameIn, bool fInboundIn) : ssSend(SER_NETWORK, INIT_PROTO_VERSION), - addrKnown(5000, 0.001, insecure_rand()), + addrKnown(5000, 0.001), setInventoryKnown(SendBufferSize() / 1000) { nServices = 0; diff --git a/src/test/bloom_tests.cpp b/src/test/bloom_tests.cpp index 1bda8a7ea161b..d927be6b81973 100644 --- a/src/test/bloom_tests.cpp +++ b/src/test/bloom_tests.cpp @@ -469,7 +469,7 @@ static std::vector RandomData() BOOST_AUTO_TEST_CASE(rolling_bloom) { // last-100-entry, 1% false positive: - CRollingBloomFilter rb1(100, 0.01, 0); + CRollingBloomFilter rb1(100, 0.01, 1); // Overfill: static const int DATASIZE=399; @@ -500,7 +500,7 @@ BOOST_AUTO_TEST_CASE(rolling_bloom) BOOST_CHECK(nHits < 175); BOOST_CHECK(rb1.contains(data[DATASIZE-1])); - rb1.clear(); + rb1.reset(1); BOOST_CHECK(!rb1.contains(data[DATASIZE-1])); // Now roll through data, make sure last 100 entries @@ -527,7 +527,7 @@ BOOST_AUTO_TEST_CASE(rolling_bloom) BOOST_CHECK(nHits < 100); // last-1000-entry, 0.01% false positive: - CRollingBloomFilter rb2(1000, 0.001, 0); + CRollingBloomFilter rb2(1000, 0.001, 1); for (int i = 0; i < DATASIZE; i++) { rb2.insert(data[i]); } From d741371d7d27e228aa64c618c50b23fb5449c3e1 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Mon, 27 Jul 2015 18:58:00 +0200 Subject: [PATCH 4/6] Only use randomly created nonces in CRollingBloomFilter. --- src/bloom.cpp | 13 ++++++------- src/bloom.h | 8 +++++--- src/test/bloom_tests.cpp | 6 +++--- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/bloom.cpp b/src/bloom.cpp index 89959d7326797..de87206592c35 100644 --- a/src/bloom.cpp +++ b/src/bloom.cpp @@ -216,16 +216,17 @@ void CBloomFilter::UpdateEmptyFull() isEmpty = empty; } -CRollingBloomFilter::CRollingBloomFilter(unsigned int nElements, double fpRate, unsigned int nTweak) : - b1(nElements * 2, fpRate, nTweak), b2(nElements * 2, fpRate, nTweak) +CRollingBloomFilter::CRollingBloomFilter(unsigned int nElements, double fpRate) : + b1(nElements * 2, fpRate, 0), b2(nElements * 2, fpRate, 0) { // Implemented using two bloom filters of 2 * nElements each. // We fill them up, and clear them, staggered, every nElements // inserted, so at least one always contains the last nElements // inserted. + nInsertions = 0; nBloomSize = nElements * 2; - reset(nTweak); + reset(); } void CRollingBloomFilter::insert(const std::vector& vKey) @@ -262,11 +263,9 @@ bool CRollingBloomFilter::contains(const uint256& hash) const return contains(data); } -void CRollingBloomFilter::reset(unsigned int nNewTweak) +void CRollingBloomFilter::reset() { - if (!nNewTweak) - nNewTweak = GetRand(std::numeric_limits::max()); - + unsigned int nNewTweak = GetRand(std::numeric_limits::max()); b1.reset(nNewTweak); b2.reset(nNewTweak); nInsertions = 0; diff --git a/src/bloom.h b/src/bloom.h index 12bf6d99a88c6..a4dba8cb4f718 100644 --- a/src/bloom.h +++ b/src/bloom.h @@ -116,15 +116,17 @@ class CBloomFilter class CRollingBloomFilter { public: - CRollingBloomFilter(unsigned int nElements, double nFPRate, - unsigned int nTweak = 0); + // A random bloom filter calls GetRand() at creation time. + // Don't create global CRollingBloomFilter objects, as they may be + // constructed before the randomizer is properly initialized. + CRollingBloomFilter(unsigned int nElements, double nFPRate); void insert(const std::vector& vKey); void insert(const uint256& hash); bool contains(const std::vector& vKey) const; bool contains(const uint256& hash) const; - void reset(unsigned int nNewTweak = 0); + void reset(); private: unsigned int nBloomSize; diff --git a/src/test/bloom_tests.cpp b/src/test/bloom_tests.cpp index d927be6b81973..6b30d6aa8ae7a 100644 --- a/src/test/bloom_tests.cpp +++ b/src/test/bloom_tests.cpp @@ -469,7 +469,7 @@ static std::vector RandomData() BOOST_AUTO_TEST_CASE(rolling_bloom) { // last-100-entry, 1% false positive: - CRollingBloomFilter rb1(100, 0.01, 1); + CRollingBloomFilter rb1(100, 0.01); // Overfill: static const int DATASIZE=399; @@ -500,7 +500,7 @@ BOOST_AUTO_TEST_CASE(rolling_bloom) BOOST_CHECK(nHits < 175); BOOST_CHECK(rb1.contains(data[DATASIZE-1])); - rb1.reset(1); + rb1.reset(); BOOST_CHECK(!rb1.contains(data[DATASIZE-1])); // Now roll through data, make sure last 100 entries @@ -527,7 +527,7 @@ BOOST_AUTO_TEST_CASE(rolling_bloom) BOOST_CHECK(nHits < 100); // last-1000-entry, 0.01% false positive: - CRollingBloomFilter rb2(1000, 0.001, 1); + CRollingBloomFilter rb2(1000, 0.001); for (int i = 0; i < DATASIZE; i++) { rb2.insert(data[i]); } From 0847d9cb5fcd2fdd5a21bde699944d966cf5add9 Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Fri, 17 Jul 2015 06:46:48 -0400 Subject: [PATCH 5/6] Keep track of recently rejected transactions Nodes can have divergent policies on which transactions they will accept and relay. This can cause you to repeatedly request and reject the same tx after its inved to you from various peers which have accepted it. Here we add rolling bloom filter to keep track of such rejections, clearing the filter every time the chain tip changes. Credit goes to Alex Morcos, who created the patch that this code is based on. Original code by Peter Todd. Refactored to not construct the filter at startup time by Pieter Wuille. --- src/main.cpp | 68 +++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 59 insertions(+), 9 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 01b62bdf608d8..0865ecc5063ec 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -162,6 +162,29 @@ namespace { */ map mapBlockSource; + /** + * Filter for transactions that were recently rejected by + * AcceptToMemoryPool. These are not rerequested until the chain tip + * changes, at which point the entire filter is reset. Protected by + * cs_main. + * + * Without this filter we'd be re-requesting txs from each of our peers, + * increasing bandwidth consumption considerably. For instance, with 100 + * peers, half of which relay a tx we don't accept, that might be a 50x + * bandwidth increase. A flooding attacker attempting to roll-over the + * filter using minimum-sized, 60byte, transactions might manage to send + * 1000/sec if we have fast peers, so we pick 120,000 to give our peers a + * two minute window to send invs to us. + * + * Decreasing the false positive rate is fairly cheap, so we pick one in a + * million to make it highly unlikely for users to have issues with this + * filter. + * + * Memory used: 1.7MB + */ + boost::scoped_ptr recentRejects; + uint256 hashRecentRejectsChainTip; + /** Blocks that are in flight, and that are in the queue to be downloaded. Protected by cs_main. */ struct QueuedBlock { uint256 hash; @@ -3267,6 +3290,7 @@ void UnloadBlockIndex() setDirtyBlockIndex.clear(); setDirtyFileInfo.clear(); mapNodeState.clear(); + recentRejects.reset(NULL); BOOST_FOREACH(BlockMap::value_type& entry, mapBlockIndex) { delete entry.second; @@ -3320,6 +3344,9 @@ bool InitBlockIndex() { } } + // Initialize global variables that cannot be constructed at startup. + recentRejects.reset(new CRollingBloomFilter(120000, 0.000001)); + return true; } @@ -3689,10 +3716,20 @@ bool static AlreadyHave(const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { case MSG_TX: { - bool txInMap = false; - txInMap = mempool.exists(inv.hash); - return txInMap || mapOrphanTransactions.count(inv.hash) || - pcoinsTip->HaveCoins(inv.hash); + if (chainActive.Tip()->GetBlockHash() != hashRecentRejectsChainTip) + { + // If the chain tip has changed previously rejected transactions + // might be now valid, e.g. due to a nLockTime'd tx becoming valid, + // or a double-spend. Reset the rejects filter and give those + // txs a second chance. + hashRecentRejectsChainTip = chainActive.Tip()->GetBlockHash(); + recentRejects->reset(); + } + + return recentRejects->contains(inv.hash) || + mempool.exists(inv.hash) || + mapOrphanTransactions.count(inv.hash) || + pcoinsTip->HaveCoins(inv.hash); } case MSG_BLOCK: return mapBlockIndex.count(inv.hash); @@ -4292,6 +4329,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, // Probably non-standard or insufficient fee/priority LogPrint("mempool", " removed orphan tx %s\n", orphanHash.ToString()); vEraseQueue.push_back(orphanHash); + recentRejects->insert(orphanHash); } mempool.check(pcoinsTip); } @@ -4309,11 +4347,23 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, unsigned int nEvicted = LimitOrphanTxSize(nMaxOrphanTx); if (nEvicted > 0) LogPrint("mempool", "mapOrphan overflow, removed %u tx\n", nEvicted); - } else if (pfrom->fWhitelisted) { - // Always relay transactions received from whitelisted peers, even - // if they are already in the mempool (allowing the node to function - // as a gateway for nodes hidden behind it). - RelayTransaction(tx); + } else { + // AcceptToMemoryPool() returned false, possibly because the tx is + // already in the mempool; if the tx isn't in the mempool that + // means it was rejected and we shouldn't ask for it again. + if (!mempool.exists(tx.GetHash())) { + recentRejects->insert(tx.GetHash()); + } + if (pfrom->fWhitelisted) { + // Always relay transactions received from whitelisted peers, even + // if they were rejected from the mempool, allowing the node to + // function as a gateway for nodes hidden behind it. + // + // FIXME: This includes invalid transactions, which means a + // whitelisted peer could get us banned! We may want to change + // that. + RelayTransaction(tx); + } } int nDoS = 0; if (state.IsInvalid(nDoS)) From a8d0407c4fcf7c4e8ed0e8edabd204f7a4efa477 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 31 Jul 2015 17:55:05 +0200 Subject: [PATCH 6/6] Move recentRejects initialization to top of InitBlockIndex This avoids that premature return in the condition that a new chain is initialized results in NULL pointer errors due to recentReject not being constructed. Also add assertions where it is used. --- src/main.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 0865ecc5063ec..0ad41af955296 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3311,6 +3311,10 @@ bool LoadBlockIndex() bool InitBlockIndex() { const CChainParams& chainparams = Params(); LOCK(cs_main); + + // Initialize global variables that cannot be constructed at startup. + recentRejects.reset(new CRollingBloomFilter(120000, 0.000001)); + // Check whether we're already initialized if (chainActive.Genesis() != NULL) return true; @@ -3344,9 +3348,6 @@ bool InitBlockIndex() { } } - // Initialize global variables that cannot be constructed at startup. - recentRejects.reset(new CRollingBloomFilter(120000, 0.000001)); - return true; } @@ -3716,6 +3717,7 @@ bool static AlreadyHave(const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { case MSG_TX: { + assert(recentRejects); if (chainActive.Tip()->GetBlockHash() != hashRecentRejectsChainTip) { // If the chain tip has changed previously rejected transactions @@ -4329,6 +4331,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, // Probably non-standard or insufficient fee/priority LogPrint("mempool", " removed orphan tx %s\n", orphanHash.ToString()); vEraseQueue.push_back(orphanHash); + assert(recentRejects); recentRejects->insert(orphanHash); } mempool.check(pcoinsTip); @@ -4352,6 +4355,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, // already in the mempool; if the tx isn't in the mempool that // means it was rejected and we shouldn't ask for it again. if (!mempool.exists(tx.GetHash())) { + assert(recentRejects); recentRejects->insert(tx.GetHash()); } if (pfrom->fWhitelisted) {