From d2d7ee0e863b286e1c9f9c54659d494fb0a7712d Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Mon, 20 Jul 2015 04:43:34 +0900 Subject: 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/main.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/main.cpp') diff --git a/src/main.cpp b/src/main.cpp index 7477d08b1..01b62bdf6 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); -- cgit v1.2.3 From 0847d9cb5fcd2fdd5a21bde699944d966cf5add9 Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Fri, 17 Jul 2015 06:46:48 -0400 Subject: 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(-) (limited to 'src/main.cpp') diff --git a/src/main.cpp b/src/main.cpp index 01b62bdf6..0865ecc50 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)) -- cgit v1.2.3 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: 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(-) (limited to 'src/main.cpp') diff --git a/src/main.cpp b/src/main.cpp index 0865ecc50..0ad41af95 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) { -- cgit v1.2.3