From b0f75847eaf5a85cd614c4f990f20172ad8185fc Mon Sep 17 00:00:00 2001 From: Simon Date: Fri, 13 May 2016 15:48:10 -0700 Subject: [PATCH 1/7] Fix issue #717 where if addrman is starved of addresses (e.g. on testnet) the Select_() function will loop endlessly trying to find an address, and therefore eat up 100% cpu time on the 'opencon' thread. Solution is to (1) add a delay to the loop and (2) restrict the number of attempts to find an address. On exiting the loop, we return to an outer loop in net.cpp which will sleep, add seed nodes and calcualte new addresses. --- src/addrman.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index c41ee3f9f..82c4ea277 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -336,11 +336,14 @@ CAddrInfo CAddrMan::Select_() if (size() == 0) return CAddrInfo(); + // Track number of attempts to find a table entry, before giving up + int nRetries = 0; + // Use a 50% chance for choosing between tried and new table entries. if (nTried > 0 && (nNew == 0 || GetRandInt(2) == 0)) { // use a tried node double fChanceFactor = 1.0; - while (1) { + while (nRetries++ < ADDRMAN_TRIED_BUCKET_COUNT) { int nKBucket = GetRandInt(ADDRMAN_TRIED_BUCKET_COUNT); int nKBucketPos = GetRandInt(ADDRMAN_BUCKET_SIZE); if (vvTried[nKBucket][nKBucketPos] == -1) @@ -351,11 +354,12 @@ CAddrInfo CAddrMan::Select_() if (GetRandInt(1 << 30) < fChanceFactor * info.GetChance() * (1 << 30)) return info; fChanceFactor *= 1.2; + MilliSleep(100); } } else { // use a new node double fChanceFactor = 1.0; - while (1) { + while (nRetries++ < ADDRMAN_NEW_BUCKET_COUNT) { int nUBucket = GetRandInt(ADDRMAN_NEW_BUCKET_COUNT); int nUBucketPos = GetRandInt(ADDRMAN_BUCKET_SIZE); if (vvNew[nUBucket][nUBucketPos] == -1) @@ -366,8 +370,11 @@ CAddrInfo CAddrMan::Select_() if (GetRandInt(1 << 30) < fChanceFactor * info.GetChance() * (1 << 30)) return info; fChanceFactor *= 1.2; + MilliSleep(100); } } + + return CAddrInfo(); } #ifdef DEBUG_ADDRMAN From 8375e1a3e7f8f35362536b07812b98cb894c2e47 Mon Sep 17 00:00:00 2001 From: EthanHeilman Date: Tue, 22 Sep 2015 15:24:16 -0400 Subject: [PATCH 2/7] Creates unittests for addrman, makes addrman testable. Adds several unittests for addrman to verify it works as expected. Makes small modifications to addrman to allow deterministic and targeted tests. Signed-off-by: Simon --- src/Makefile.test.include | 1 + src/addrman.cpp | 8 +- src/addrman.h | 16 +++- src/test/addrman_tests.cpp | 180 +++++++++++++++++++++++++++++++++++++ 4 files changed, 199 insertions(+), 6 deletions(-) create mode 100644 src/test/addrman_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 72d38e7c0..40a6e5f6f 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -34,6 +34,7 @@ GENERATED_TEST_FILES = $(JSON_TEST_FILES:.json=.json.h) $(RAW_TEST_FILES:.raw=.r BITCOIN_TESTS =\ test/arith_uint256_tests.cpp \ test/bignum.h \ + test/addrman_tests.cpp \ test/alert_tests.cpp \ test/allocator_tests.cpp \ test/base32_tests.cpp \ diff --git a/src/addrman.cpp b/src/addrman.cpp index 82c4ea277..ca208f0c8 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -331,7 +331,7 @@ void CAddrMan::Attempt_(const CService& addr, int64_t nTime) info.nAttempts++; } -CAddrInfo CAddrMan::Select_() +CAddrInfo CAddrMan::Select_(bool newOnly) { if (size() == 0) return CAddrInfo(); @@ -339,8 +339,12 @@ CAddrInfo CAddrMan::Select_() // Track number of attempts to find a table entry, before giving up int nRetries = 0; + if (newOnly && nNew == 0) + return CAddrInfo(); + // Use a 50% chance for choosing between tried and new table entries. - if (nTried > 0 && (nNew == 0 || GetRandInt(2) == 0)) { + if (!newOnly && + (nTried > 0 && (nNew == 0 || GetRandInt(2) == 0))) { // use a tried node double fChanceFactor = 1.0; while (nRetries++ < ADDRMAN_TRIED_BUCKET_COUNT) { diff --git a/src/addrman.h b/src/addrman.h index 2db166399..4927f76c6 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -22,6 +22,8 @@ */ class CAddrInfo : public CAddress { + + public: //! last try whatsoever by us (memory only) int64_t nLastTry; @@ -230,8 +232,8 @@ protected: //! Mark an entry as attempted to connect. void Attempt_(const CService &addr, int64_t nTime); - //! Select an address to connect to. - CAddrInfo Select_(); + //! Select an address to connect to, if newOnly is set to true, only the new table is selected from. + CAddrInfo Select_(bool newOnly); #ifdef DEBUG_ADDRMAN //! Perform consistency check. Returns an error code or zero. @@ -532,13 +534,13 @@ public: /** * Choose an address to connect to. */ - CAddrInfo Select() + CAddrInfo Select(bool newOnly = false) { CAddrInfo addrRet; { LOCK(cs); Check(); - addrRet = Select_(); + addrRet = Select_(newOnly); Check(); } return addrRet; @@ -567,6 +569,12 @@ public: Check(); } } + + //! Ensure that bucket placement is always the same for testing purposes. + void MakeDeterministic(){ + nKey.SetNull(); //Do not use outside of tests. + } + }; #endif // BITCOIN_ADDRMAN_H diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp new file mode 100644 index 000000000..cfcdd9abb --- /dev/null +++ b/src/test/addrman_tests.cpp @@ -0,0 +1,180 @@ +// Copyright (c) 2012-2013 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include "addrman.h" +#include "test/test_bitcoin.h" +#include +#include + +#include "random.h" + +using namespace std; + +class CAddrManTest : public CAddrMan{}; + +BOOST_FIXTURE_TEST_SUITE(addrman_tests, BasicTestingSetup) + +BOOST_AUTO_TEST_CASE(addrman_simple) +{ + CAddrManTest addrman; + + // Set addrman addr placement to be deterministic. + addrman.MakeDeterministic(); + + CNetAddr source = CNetAddr("252.2.2.2:8333"); + + // Test 1: Does Addrman respond correctly when empty. + BOOST_CHECK(addrman.size() == 0); + CAddrInfo addr_null = addrman.Select(); + BOOST_CHECK(addr_null.ToString() == "[::]:0"); + + // Test 2: Does Addrman::Add work as expected. + CService addr1 = CService("250.1.1.1:8333"); + addrman.Add(CAddress(addr1), source); + BOOST_CHECK(addrman.size() == 1); + CAddrInfo addr_ret1 = addrman.Select(); + BOOST_CHECK(addr_ret1.ToString() == "250.1.1.1:8333"); + + // Test 3: Does IP address deduplication work correctly. + // Expected dup IP should not be added. + CService addr1_dup = CService("250.1.1.1:8333"); + addrman.Add(CAddress(addr1_dup), source); + BOOST_CHECK(addrman.size() == 1); + + + // Test 5: New table has one addr and we add a diff addr we should + // have two addrs. + CService addr2 = CService("250.1.1.2:8333"); + addrman.Add(CAddress(addr2), source); + BOOST_CHECK(addrman.size() == 2); + + // Test 6: AddrMan::Clear() should empty the new table. + addrman.Clear(); + BOOST_CHECK(addrman.size() == 0); + CAddrInfo addr_null2 = addrman.Select(); + BOOST_CHECK(addr_null2.ToString() == "[::]:0"); +} + +BOOST_AUTO_TEST_CASE(addrman_ports) +{ + CAddrManTest addrman; + + // Set addrman addr placement to be deterministic. + addrman.MakeDeterministic(); + + CNetAddr source = CNetAddr("252.2.2.2:8333"); + + BOOST_CHECK(addrman.size() == 0); + + // Test 7; Addr with same IP but diff port does not replace existing addr. + CService addr1 = CService("250.1.1.1:8333"); + addrman.Add(CAddress(addr1), source); + BOOST_CHECK(addrman.size() == 1); + + CService addr1_port = CService("250.1.1.1:8334"); + addrman.Add(CAddress(addr1_port), source); + BOOST_CHECK(addrman.size() == 1); + CAddrInfo addr_ret2 = addrman.Select(); + BOOST_CHECK(addr_ret2.ToString() == "250.1.1.1:8333"); + + // Test 8: Add same IP but diff port to tried table, it doesn't get added. + // Perhaps this is not ideal behavior but it is the current behavior. + addrman.Good(CAddress(addr1_port)); + BOOST_CHECK(addrman.size() == 1); + bool newOnly = true; + CAddrInfo addr_ret3 = addrman.Select(newOnly); + BOOST_CHECK(addr_ret3.ToString() == "250.1.1.1:8333"); +} + + +BOOST_AUTO_TEST_CASE(addrman_select) +{ + CAddrManTest addrman; + + // Set addrman addr placement to be deterministic. + addrman.MakeDeterministic(); + + CNetAddr source = CNetAddr("252.2.2.2:8333"); + + // Test 9: Select from new with 1 addr in new. + CService addr1 = CService("250.1.1.1:8333"); + addrman.Add(CAddress(addr1), source); + BOOST_CHECK(addrman.size() == 1); + + bool newOnly = true; + CAddrInfo addr_ret1 = addrman.Select(newOnly); + BOOST_CHECK(addr_ret1.ToString() == "250.1.1.1:8333"); + + + // Test 10: move addr to tried, select from new expected nothing returned. + addrman.Good(CAddress(addr1)); + BOOST_CHECK(addrman.size() == 1); + CAddrInfo addr_ret2 = addrman.Select(newOnly); + BOOST_CHECK(addr_ret2.ToString() == "[::]:0"); + + CAddrInfo addr_ret3 = addrman.Select(); + BOOST_CHECK(addr_ret3.ToString() == "250.1.1.1:8333"); +} + +BOOST_AUTO_TEST_CASE(addrman_new_collisions) +{ + CAddrManTest addrman; + + // Set addrman addr placement to be deterministic. + addrman.MakeDeterministic(); + + CNetAddr source = CNetAddr("252.2.2.2:8333"); + + BOOST_CHECK(addrman.size() == 0); + + for (unsigned int i = 1; i < 4; i++){ + CService addr = CService("250.1.1."+boost::to_string(i)); + addrman.Add(CAddress(addr), source); + + //Test 11: No collision in new table yet. + BOOST_CHECK(addrman.size() == i); + } + + //Test 12: new table collision! + CService addr1 = CService("250.1.1.4"); + addrman.Add(CAddress(addr1), source); + BOOST_CHECK(addrman.size() == 3); + + CService addr2 = CService("250.1.1.5"); + addrman.Add(CAddress(addr2), source); + BOOST_CHECK(addrman.size() == 4); +} + +BOOST_AUTO_TEST_CASE(addrman_tried_collisions) +{ + CAddrManTest addrman; + + // Set addrman addr placement to be deterministic. + addrman.MakeDeterministic(); + + CNetAddr source = CNetAddr("252.2.2.2:8333"); + + BOOST_CHECK(addrman.size() == 0); + + for (unsigned int i = 1; i < 75; i++){ + CService addr = CService("250.1.1."+boost::to_string(i)); + addrman.Add(CAddress(addr), source); + addrman.Good(CAddress(addr)); + + //Test 13: No collision in tried table yet. + BOOST_TEST_MESSAGE(addrman.size()); + BOOST_CHECK(addrman.size() == i); + } + + //Test 14: tried table collision! + CService addr1 = CService("250.1.1.76"); + addrman.Add(CAddress(addr1), source); + BOOST_CHECK(addrman.size() == 74); + + CService addr2 = CService("250.1.1.77"); + addrman.Add(CAddress(addr2), source); + BOOST_CHECK(addrman.size() == 75); +} + + +BOOST_AUTO_TEST_SUITE_END() \ No newline at end of file From 1bf2cb1a06461f0d5c01f673d52f2a64ebc63e48 Mon Sep 17 00:00:00 2001 From: Ethan Heilman Date: Sat, 12 Dec 2015 22:34:08 -0500 Subject: [PATCH 3/7] Increase test coverage for addrman and addrinfo Adds several unittests for CAddrMan and CAddrInfo. Increases the accuracy of addrman tests. Removes non-determinism in tests by overriding the random number generator. Extracts testing code from addrman class to test class. Signed-off-by: Simon --- src/addrman.cpp | 36 ++-- src/addrman.h | 13 +- src/test/addrman_tests.cpp | 403 ++++++++++++++++++++++++++++++++++--- 3 files changed, 399 insertions(+), 53 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index ca208f0c8..402bb5b48 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -222,7 +222,7 @@ void CAddrMan::Good_(const CService& addr, int64_t nTime) return; // find a bucket it is in now - int nRnd = GetRandInt(ADDRMAN_NEW_BUCKET_COUNT); + int nRnd = RandomInt(ADDRMAN_NEW_BUCKET_COUNT); int nUBucket = -1; for (unsigned int n = 0; n < ADDRMAN_NEW_BUCKET_COUNT; n++) { int nB = (n + nRnd) % ADDRMAN_NEW_BUCKET_COUNT; @@ -279,7 +279,7 @@ bool CAddrMan::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimeP int nFactor = 1; for (int n = 0; n < pinfo->nRefCount; n++) nFactor *= 2; - if (nFactor > 1 && (GetRandInt(nFactor) != 0)) + if (nFactor > 1 && (RandomInt(nFactor) != 0)) return false; } else { pinfo = Create(addr, source, &nId); @@ -344,18 +344,20 @@ CAddrInfo CAddrMan::Select_(bool newOnly) // Use a 50% chance for choosing between tried and new table entries. if (!newOnly && - (nTried > 0 && (nNew == 0 || GetRandInt(2) == 0))) { + (nTried > 0 && (nNew == 0 || RandomInt(2) == 0))) { // use a tried node double fChanceFactor = 1.0; while (nRetries++ < ADDRMAN_TRIED_BUCKET_COUNT) { - int nKBucket = GetRandInt(ADDRMAN_TRIED_BUCKET_COUNT); - int nKBucketPos = GetRandInt(ADDRMAN_BUCKET_SIZE); - if (vvTried[nKBucket][nKBucketPos] == -1) - continue; + int nKBucket = RandomInt(ADDRMAN_TRIED_BUCKET_COUNT); + int nKBucketPos = RandomInt(ADDRMAN_BUCKET_SIZE); + while (vvTried[nKBucket][nKBucketPos] == -1) { + nKBucket = (nKBucket + insecure_rand()) % ADDRMAN_TRIED_BUCKET_COUNT; + nKBucketPos = (nKBucketPos + insecure_rand()) % ADDRMAN_BUCKET_SIZE; + } int nId = vvTried[nKBucket][nKBucketPos]; assert(mapInfo.count(nId) == 1); CAddrInfo& info = mapInfo[nId]; - if (GetRandInt(1 << 30) < fChanceFactor * info.GetChance() * (1 << 30)) + if (RandomInt(1 << 30) < fChanceFactor * info.GetChance() * (1 << 30)) return info; fChanceFactor *= 1.2; MilliSleep(100); @@ -364,14 +366,16 @@ CAddrInfo CAddrMan::Select_(bool newOnly) // use a new node double fChanceFactor = 1.0; while (nRetries++ < ADDRMAN_NEW_BUCKET_COUNT) { - int nUBucket = GetRandInt(ADDRMAN_NEW_BUCKET_COUNT); - int nUBucketPos = GetRandInt(ADDRMAN_BUCKET_SIZE); - if (vvNew[nUBucket][nUBucketPos] == -1) - continue; + int nUBucket = RandomInt(ADDRMAN_NEW_BUCKET_COUNT); + int nUBucketPos = RandomInt(ADDRMAN_BUCKET_SIZE); + while (vvNew[nUBucket][nUBucketPos] == -1) { + nUBucket = (nUBucket + insecure_rand()) % ADDRMAN_NEW_BUCKET_COUNT; + nUBucketPos = (nUBucketPos + insecure_rand()) % ADDRMAN_BUCKET_SIZE; + } int nId = vvNew[nUBucket][nUBucketPos]; assert(mapInfo.count(nId) == 1); CAddrInfo& info = mapInfo[nId]; - if (GetRandInt(1 << 30) < fChanceFactor * info.GetChance() * (1 << 30)) + if (RandomInt(1 << 30) < fChanceFactor * info.GetChance() * (1 << 30)) return info; fChanceFactor *= 1.2; MilliSleep(100); @@ -470,7 +474,7 @@ void CAddrMan::GetAddr_(std::vector& vAddr) if (vAddr.size() >= nNodes) break; - int nRndPos = GetRandInt(vRandom.size() - n) + n; + int nRndPos = RandomInt(vRandom.size() - n) + n; SwapRandom(n, nRndPos); assert(mapInfo.count(vRandom[n]) == 1); @@ -499,3 +503,7 @@ void CAddrMan::Connected_(const CService& addr, int64_t nTime) if (nTime - info.nTime > nUpdateInterval) info.nTime = nTime; } + +int CAddrMan::RandomInt(int nMax){ + return GetRandInt(nMax); +} diff --git a/src/addrman.h b/src/addrman.h index 4927f76c6..7ee8b63ee 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -175,9 +175,6 @@ private: //! critical section to protect the inner data structures mutable CCriticalSection cs; - //! secret key to randomize bucket select with - uint256 nKey; - //! last used nId int nIdCount; @@ -203,6 +200,8 @@ private: int vvNew[ADDRMAN_NEW_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE]; protected: + //! secret key to randomize bucket select with + uint256 nKey; //! Find an entry. CAddrInfo* Find(const CNetAddr& addr, int *pnId = NULL); @@ -235,6 +234,9 @@ protected: //! Select an address to connect to, if newOnly is set to true, only the new table is selected from. CAddrInfo Select_(bool newOnly); + //! Wraps GetRandInt to allow tests to override RandomInt and make it determinismistic. + virtual int RandomInt(int nMax); + #ifdef DEBUG_ADDRMAN //! Perform consistency check. Returns an error code or zero. int Check_(); @@ -569,11 +571,6 @@ public: Check(); } } - - //! Ensure that bucket placement is always the same for testing purposes. - void MakeDeterministic(){ - nKey.SetNull(); //Do not use outside of tests. - } }; diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index cfcdd9abb..e34750eb8 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -6,11 +6,49 @@ #include #include +#include "hash.h" #include "random.h" using namespace std; -class CAddrManTest : public CAddrMan{}; +class CAddrManTest : public CAddrMan +{ + uint64_t state; + +public: + CAddrManTest() + { + state = 1; + } + + //! Ensure that bucket placement is always the same for testing purposes. + void MakeDeterministic() + { + nKey.SetNull(); + seed_insecure_rand(true); + } + + int RandomInt(int nMax) + { + state = (CHashWriter(SER_GETHASH, 0) << state).GetHash().GetCheapHash(); + return (unsigned int)(state % nMax); + } + + CAddrInfo* Find(const CNetAddr& addr, int* pnId = NULL) + { + return CAddrMan::Find(addr, pnId); + } + + CAddrInfo* Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId = NULL) + { + return CAddrMan::Create(addr, addrSource, pnId); + } + + void Delete(int nId) + { + CAddrMan::Delete(nId); + } +}; BOOST_FIXTURE_TEST_SUITE(addrman_tests, BasicTestingSetup) @@ -21,7 +59,7 @@ BOOST_AUTO_TEST_CASE(addrman_simple) // Set addrman addr placement to be deterministic. addrman.MakeDeterministic(); - CNetAddr source = CNetAddr("252.2.2.2:8333"); + CNetAddr source = CNetAddr("252.2.2.2"); // Test 1: Does Addrman respond correctly when empty. BOOST_CHECK(addrman.size() == 0); @@ -29,26 +67,26 @@ BOOST_AUTO_TEST_CASE(addrman_simple) BOOST_CHECK(addr_null.ToString() == "[::]:0"); // Test 2: Does Addrman::Add work as expected. - CService addr1 = CService("250.1.1.1:8333"); + CService addr1 = CService("250.1.1.1", 8333); addrman.Add(CAddress(addr1), source); BOOST_CHECK(addrman.size() == 1); CAddrInfo addr_ret1 = addrman.Select(); BOOST_CHECK(addr_ret1.ToString() == "250.1.1.1:8333"); - // Test 3: Does IP address deduplication work correctly. + // Test 3: Does IP address deduplication work correctly. // Expected dup IP should not be added. - CService addr1_dup = CService("250.1.1.1:8333"); + CService addr1_dup = CService("250.1.1.1", 8333); addrman.Add(CAddress(addr1_dup), source); BOOST_CHECK(addrman.size() == 1); // Test 5: New table has one addr and we add a diff addr we should // have two addrs. - CService addr2 = CService("250.1.1.2:8333"); + CService addr2 = CService("250.1.1.2", 8333); addrman.Add(CAddress(addr2), source); BOOST_CHECK(addrman.size() == 2); - // Test 6: AddrMan::Clear() should empty the new table. + // Test 6: AddrMan::Clear() should empty the new table. addrman.Clear(); BOOST_CHECK(addrman.size() == 0); CAddrInfo addr_null2 = addrman.Select(); @@ -62,16 +100,16 @@ BOOST_AUTO_TEST_CASE(addrman_ports) // Set addrman addr placement to be deterministic. addrman.MakeDeterministic(); - CNetAddr source = CNetAddr("252.2.2.2:8333"); + CNetAddr source = CNetAddr("252.2.2.2"); BOOST_CHECK(addrman.size() == 0); // Test 7; Addr with same IP but diff port does not replace existing addr. - CService addr1 = CService("250.1.1.1:8333"); + CService addr1 = CService("250.1.1.1", 8333); addrman.Add(CAddress(addr1), source); BOOST_CHECK(addrman.size() == 1); - CService addr1_port = CService("250.1.1.1:8334"); + CService addr1_port = CService("250.1.1.1", 8334); addrman.Add(CAddress(addr1_port), source); BOOST_CHECK(addrman.size() == 1); CAddrInfo addr_ret2 = addrman.Select(); @@ -94,10 +132,10 @@ BOOST_AUTO_TEST_CASE(addrman_select) // Set addrman addr placement to be deterministic. addrman.MakeDeterministic(); - CNetAddr source = CNetAddr("252.2.2.2:8333"); + CNetAddr source = CNetAddr("252.2.2.2"); // Test 9: Select from new with 1 addr in new. - CService addr1 = CService("250.1.1.1:8333"); + CService addr1 = CService("250.1.1.1", 8333); addrman.Add(CAddress(addr1), source); BOOST_CHECK(addrman.size() == 1); @@ -105,7 +143,6 @@ BOOST_AUTO_TEST_CASE(addrman_select) CAddrInfo addr_ret1 = addrman.Select(newOnly); BOOST_CHECK(addr_ret1.ToString() == "250.1.1.1:8333"); - // Test 10: move addr to tried, select from new expected nothing returned. addrman.Good(CAddress(addr1)); BOOST_CHECK(addrman.size() == 1); @@ -114,6 +151,39 @@ BOOST_AUTO_TEST_CASE(addrman_select) CAddrInfo addr_ret3 = addrman.Select(); BOOST_CHECK(addr_ret3.ToString() == "250.1.1.1:8333"); + + BOOST_CHECK(addrman.size() == 1); + + + // Add three addresses to new table. + CService addr2 = CService("250.3.1.1", 8333); + CService addr3 = CService("250.3.2.2", 9999); + CService addr4 = CService("250.3.3.3", 9999); + + addrman.Add(CAddress(addr2), CService("250.3.1.1", 8333)); + addrman.Add(CAddress(addr3), CService("250.3.1.1", 8333)); + addrman.Add(CAddress(addr4), CService("250.4.1.1", 8333)); + + // Add three addresses to tried table. + CService addr5 = CService("250.4.4.4", 8333); + CService addr6 = CService("250.4.5.5", 7777); + CService addr7 = CService("250.4.6.6", 8333); + + addrman.Add(CAddress(addr5), CService("250.3.1.1", 8333)); + addrman.Good(CAddress(addr5)); + addrman.Add(CAddress(addr6), CService("250.3.1.1", 8333)); + addrman.Good(CAddress(addr6)); + addrman.Add(CAddress(addr7), CService("250.1.1.3", 8333)); + addrman.Good(CAddress(addr7)); + + // Test 11: 6 addrs + 1 addr from last test = 7. + BOOST_CHECK(addrman.size() == 7); + + // Test 12: Select pulls from new and tried regardless of port number. + BOOST_CHECK(addrman.Select().ToString() == "250.4.6.6:8333"); + BOOST_CHECK(addrman.Select().ToString() == "250.3.2.2:9999"); + BOOST_CHECK(addrman.Select().ToString() == "250.3.3.3:9999"); + BOOST_CHECK(addrman.Select().ToString() == "250.4.4.4:8333"); } BOOST_AUTO_TEST_CASE(addrman_new_collisions) @@ -123,26 +193,26 @@ BOOST_AUTO_TEST_CASE(addrman_new_collisions) // Set addrman addr placement to be deterministic. addrman.MakeDeterministic(); - CNetAddr source = CNetAddr("252.2.2.2:8333"); + CNetAddr source = CNetAddr("252.2.2.2"); BOOST_CHECK(addrman.size() == 0); - for (unsigned int i = 1; i < 4; i++){ - CService addr = CService("250.1.1."+boost::to_string(i)); + for (unsigned int i = 1; i < 18; i++) { + CService addr = CService("250.1.1." + boost::to_string(i)); addrman.Add(CAddress(addr), source); - //Test 11: No collision in new table yet. + //Test 13: No collision in new table yet. BOOST_CHECK(addrman.size() == i); } - //Test 12: new table collision! - CService addr1 = CService("250.1.1.4"); + //Test 14: new table collision! + CService addr1 = CService("250.1.1.18"); addrman.Add(CAddress(addr1), source); - BOOST_CHECK(addrman.size() == 3); + BOOST_CHECK(addrman.size() == 17); - CService addr2 = CService("250.1.1.5"); + CService addr2 = CService("250.1.1.19"); addrman.Add(CAddress(addr2), source); - BOOST_CHECK(addrman.size() == 4); + BOOST_CHECK(addrman.size() == 18); } BOOST_AUTO_TEST_CASE(addrman_tried_collisions) @@ -152,29 +222,300 @@ BOOST_AUTO_TEST_CASE(addrman_tried_collisions) // Set addrman addr placement to be deterministic. addrman.MakeDeterministic(); - CNetAddr source = CNetAddr("252.2.2.2:8333"); + CNetAddr source = CNetAddr("252.2.2.2"); BOOST_CHECK(addrman.size() == 0); - for (unsigned int i = 1; i < 75; i++){ - CService addr = CService("250.1.1."+boost::to_string(i)); + for (unsigned int i = 1; i < 80; i++) { + CService addr = CService("250.1.1." + boost::to_string(i)); addrman.Add(CAddress(addr), source); addrman.Good(CAddress(addr)); - //Test 13: No collision in tried table yet. + //Test 15: No collision in tried table yet. BOOST_TEST_MESSAGE(addrman.size()); BOOST_CHECK(addrman.size() == i); } - //Test 14: tried table collision! - CService addr1 = CService("250.1.1.76"); + //Test 16: tried table collision! + CService addr1 = CService("250.1.1.80"); addrman.Add(CAddress(addr1), source); - BOOST_CHECK(addrman.size() == 74); + BOOST_CHECK(addrman.size() == 79); - CService addr2 = CService("250.1.1.77"); + CService addr2 = CService("250.1.1.81"); addrman.Add(CAddress(addr2), source); - BOOST_CHECK(addrman.size() == 75); + BOOST_CHECK(addrman.size() == 80); +} + +BOOST_AUTO_TEST_CASE(addrman_find) +{ + CAddrManTest addrman; + + // Set addrman addr placement to be deterministic. + addrman.MakeDeterministic(); + + BOOST_CHECK(addrman.size() == 0); + + CAddress addr1 = CAddress(CService("250.1.2.1", 8333)); + CAddress addr2 = CAddress(CService("250.1.2.1", 9999)); + CAddress addr3 = CAddress(CService("251.255.2.1", 8333)); + + CNetAddr source1 = CNetAddr("250.1.2.1"); + CNetAddr source2 = CNetAddr("250.1.2.2"); + + addrman.Add(addr1, source1); + addrman.Add(addr2, source2); + addrman.Add(addr3, source1); + + // Test 17: ensure Find returns an IP matching what we searched on. + CAddrInfo* info1 = addrman.Find(addr1); + BOOST_CHECK(info1); + if (info1) + BOOST_CHECK(info1->ToString() == "250.1.2.1:8333"); + + // Test 18; Find does not discriminate by port number. + CAddrInfo* info2 = addrman.Find(addr2); + BOOST_CHECK(info2); + if (info2) + BOOST_CHECK(info2->ToString() == info1->ToString()); + + // Test 19: Find returns another IP matching what we searched on. + CAddrInfo* info3 = addrman.Find(addr3); + BOOST_CHECK(info3); + if (info3) + BOOST_CHECK(info3->ToString() == "251.255.2.1:8333"); +} + +BOOST_AUTO_TEST_CASE(addrman_create) +{ + CAddrManTest addrman; + + // Set addrman addr placement to be deterministic. + addrman.MakeDeterministic(); + + BOOST_CHECK(addrman.size() == 0); + + CAddress addr1 = CAddress(CService("250.1.2.1", 8333)); + CNetAddr source1 = CNetAddr("250.1.2.1"); + + int nId; + CAddrInfo* pinfo = addrman.Create(addr1, source1, &nId); + + // Test 20: The result should be the same as the input addr. + BOOST_CHECK(pinfo->ToString() == "250.1.2.1:8333"); + + CAddrInfo* info2 = addrman.Find(addr1); + BOOST_CHECK(info2->ToString() == "250.1.2.1:8333"); } +BOOST_AUTO_TEST_CASE(addrman_delete) +{ + CAddrManTest addrman; + + // Set addrman addr placement to be deterministic. + addrman.MakeDeterministic(); + + BOOST_CHECK(addrman.size() == 0); + + CAddress addr1 = CAddress(CService("250.1.2.1", 8333)); + CNetAddr source1 = CNetAddr("250.1.2.1"); + + int nId; + addrman.Create(addr1, source1, &nId); + + // Test 21: Delete should actually delete the addr. + BOOST_CHECK(addrman.size() == 1); + addrman.Delete(nId); + BOOST_CHECK(addrman.size() == 0); + CAddrInfo* info2 = addrman.Find(addr1); + BOOST_CHECK(info2 == NULL); +} + +BOOST_AUTO_TEST_CASE(addrman_getaddr) +{ + CAddrManTest addrman; + + // Set addrman addr placement to be deterministic. + addrman.MakeDeterministic(); + + // Test 22: Sanity check, GetAddr should never return anything if addrman + // is empty. + BOOST_CHECK(addrman.size() == 0); + vector vAddr1 = addrman.GetAddr(); + BOOST_CHECK(vAddr1.size() == 0); + + CAddress addr1 = CAddress(CService("250.250.2.1", 8333)); + addr1.nTime = GetAdjustedTime(); // Set time so isTerrible = false + CAddress addr2 = CAddress(CService("250.251.2.2", 9999)); + addr2.nTime = GetAdjustedTime(); + CAddress addr3 = CAddress(CService("251.252.2.3", 8333)); + addr3.nTime = GetAdjustedTime(); + CAddress addr4 = CAddress(CService("252.253.3.4", 8333)); + addr4.nTime = GetAdjustedTime(); + CAddress addr5 = CAddress(CService("252.254.4.5", 8333)); + addr5.nTime = GetAdjustedTime(); + CNetAddr source1 = CNetAddr("250.1.2.1"); + CNetAddr source2 = CNetAddr("250.2.3.3"); + + // Test 23: Ensure GetAddr works with new addresses. + addrman.Add(addr1, source1); + addrman.Add(addr2, source2); + addrman.Add(addr3, source1); + addrman.Add(addr4, source2); + addrman.Add(addr5, source1); + + // GetAddr returns 23% of addresses, 23% of 5 is 1 rounded down. + BOOST_CHECK(addrman.GetAddr().size() == 1); + + // Test 24: Ensure GetAddr works with new and tried addresses. + addrman.Good(CAddress(addr1)); + addrman.Good(CAddress(addr2)); + BOOST_CHECK(addrman.GetAddr().size() == 1); + + // Test 25: Ensure GetAddr still returns 23% when addrman has many addrs. + for (unsigned int i = 1; i < (8 * 256); i++) { + int octet1 = i % 256; + int octet2 = (i / 256) % 256; + int octet3 = (i / (256 * 2)) % 256; + string strAddr = boost::to_string(octet1) + "." + boost::to_string(octet2) + "." + boost::to_string(octet3) + ".23"; + CAddress addr = CAddress(CService(strAddr)); + + // Ensure that for all addrs in addrman, isTerrible == false. + addr.nTime = GetAdjustedTime(); + addrman.Add(addr, CNetAddr(strAddr)); + if (i % 8 == 0) + addrman.Good(addr); + } + vector vAddr = addrman.GetAddr(); + + size_t percent23 = (addrman.size() * 23) / 100; + BOOST_CHECK(vAddr.size() == percent23); + BOOST_CHECK(vAddr.size() == 461); + // (Addrman.size() < number of addresses added) due to address collisons. + BOOST_CHECK(addrman.size() == 2007); +} + + +BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket) +{ + CAddrManTest addrman; + + // Set addrman addr placement to be deterministic. + addrman.MakeDeterministic(); + + CAddress addr1 = CAddress(CService("250.1.1.1", 8333)); + CAddress addr2 = CAddress(CService("250.1.1.1", 9999)); + + CNetAddr source1 = CNetAddr("250.1.1.1"); + + + CAddrInfo info1 = CAddrInfo(addr1, source1); + + uint256 nKey1 = (uint256)(CHashWriter(SER_GETHASH, 0) << 1).GetHash(); + uint256 nKey2 = (uint256)(CHashWriter(SER_GETHASH, 0) << 2).GetHash(); + + + BOOST_CHECK(info1.GetTriedBucket(nKey1) == 40); + + // Test 26: Make sure key actually randomizes bucket placement. A fail on + // this test could be a security issue. + BOOST_CHECK(info1.GetTriedBucket(nKey1) != info1.GetTriedBucket(nKey2)); + + // Test 27: Two addresses with same IP but different ports can map to + // different buckets because they have different keys. + CAddrInfo info2 = CAddrInfo(addr2, source1); + + BOOST_CHECK(info1.GetKey() != info2.GetKey()); + BOOST_CHECK(info1.GetTriedBucket(nKey1) != info2.GetTriedBucket(nKey1)); + + set buckets; + for (int i = 0; i < 255; i++) { + CAddrInfo infoi = CAddrInfo( + CAddress(CService("250.1.1." + boost::to_string(i))), + CNetAddr("250.1.1." + boost::to_string(i))); + int bucket = infoi.GetTriedBucket(nKey1); + buckets.insert(bucket); + } + // Test 28: IP addresses in the same group (\16 prefix for IPv4) should + // never get more than 8 buckets + BOOST_CHECK(buckets.size() == 8); + + buckets.clear(); + for (int j = 0; j < 255; j++) { + CAddrInfo infoj = CAddrInfo( + CAddress(CService("250." + boost::to_string(j) + ".1.1")), + CNetAddr("250." + boost::to_string(j) + ".1.1")); + int bucket = infoj.GetTriedBucket(nKey1); + buckets.insert(bucket); + } + // Test 29: IP addresses in the different groups should map to more than + // 8 buckets. + BOOST_CHECK(buckets.size() == 160); +} + +BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket) +{ + CAddrManTest addrman; + + // Set addrman addr placement to be deterministic. + addrman.MakeDeterministic(); + + CAddress addr1 = CAddress(CService("250.1.2.1", 8333)); + CAddress addr2 = CAddress(CService("250.1.2.1", 9999)); + + CNetAddr source1 = CNetAddr("250.1.2.1"); + + CAddrInfo info1 = CAddrInfo(addr1, source1); + + uint256 nKey1 = (uint256)(CHashWriter(SER_GETHASH, 0) << 1).GetHash(); + uint256 nKey2 = (uint256)(CHashWriter(SER_GETHASH, 0) << 2).GetHash(); + + BOOST_CHECK(info1.GetNewBucket(nKey1) == 786); + + // Test 30: Make sure key actually randomizes bucket placement. A fail on + // this test could be a security issue. + BOOST_CHECK(info1.GetNewBucket(nKey1) != info1.GetNewBucket(nKey2)); + + // Test 31: Ports should not effect bucket placement in the addr + CAddrInfo info2 = CAddrInfo(addr2, source1); + BOOST_CHECK(info1.GetKey() != info2.GetKey()); + BOOST_CHECK(info1.GetNewBucket(nKey1) == info2.GetNewBucket(nKey1)); + + set buckets; + for (int i = 0; i < 255; i++) { + CAddrInfo infoi = CAddrInfo( + CAddress(CService("250.1.1." + boost::to_string(i))), + CNetAddr("250.1.1." + boost::to_string(i))); + int bucket = infoi.GetNewBucket(nKey1); + buckets.insert(bucket); + } + // Test 32: IP addresses in the same group (\16 prefix for IPv4) should + // always map to the same bucket. + BOOST_CHECK(buckets.size() == 1); + + buckets.clear(); + for (int j = 0; j < 4 * 255; j++) { + CAddrInfo infoj = CAddrInfo(CAddress( + CService( + boost::to_string(250 + (j / 255)) + "." + boost::to_string(j % 256) + ".1.1")), + CNetAddr("251.4.1.1")); + int bucket = infoj.GetNewBucket(nKey1); + buckets.insert(bucket); + } + // Test 33: IP addresses in the same source groups should map to no more + // than 64 buckets. + BOOST_CHECK(buckets.size() <= 64); + + buckets.clear(); + for (int p = 0; p < 255; p++) { + CAddrInfo infoj = CAddrInfo( + CAddress(CService("250.1.1.1")), + CNetAddr("250." + boost::to_string(p) + ".1.1")); + int bucket = infoj.GetNewBucket(nKey1); + buckets.insert(bucket); + } + // Test 34: IP addresses in the different source groups should map to more + // than 64 buckets. + BOOST_CHECK(buckets.size() > 64); +} BOOST_AUTO_TEST_SUITE_END() \ No newline at end of file From 7bfb552f635ebc5abeb62df489f0dee108fa4f61 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Sat, 23 Apr 2016 22:21:52 -0700 Subject: [PATCH 4/7] CAddrMan::Deserialize handle corrupt serializations better. Signed-off-by: Simon --- src/addrman.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/addrman.h b/src/addrman.h index 7ee8b63ee..cde9430ae 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -349,6 +349,14 @@ public: nUBuckets ^= (1 << 30); } + if (nNew > ADDRMAN_NEW_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE) { + throw std::ios_base::failure("Corrupt CAddrMan serialization, nNew exceeds limit."); + } + + if (nTried > ADDRMAN_TRIED_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE) { + throw std::ios_base::failure("Corrupt CAddrMan serialization, nTried exceeds limit."); + } + // Deserialize entries from the new table. for (int n = 0; n < nNew; n++) { CAddrInfo &info = mapInfo[n]; From 0116e20ea1b98ccee4aff0cc4922a6c2d26289a0 Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Mon, 15 Jun 2015 14:45:19 +0200 Subject: [PATCH 5/7] remove using namespace std from addrman.cpp Signed-off-by: Simon --- src/addrman.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 402bb5b48..654cea1d2 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -8,8 +8,6 @@ #include "serialize.h" #include "streams.h" -using namespace std; - int CAddrInfo::GetTriedBucket(const uint256& nKey) const { uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << GetKey()).GetHash().GetCheapHash(); @@ -68,7 +66,7 @@ double CAddrInfo::GetChance(int64_t nNow) const fChance *= 0.01; // deprioritize 66% after each failed attempt, but at most 1/28th to avoid the search taking forever or overly penalizing outages. - fChance *= pow(0.66, min(nAttempts, 8)); + fChance *= pow(0.66, std::min(nAttempts, 8)); return fChance; } @@ -258,7 +256,7 @@ bool CAddrMan::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimeP bool fCurrentlyOnline = (GetAdjustedTime() - addr.nTime < 24 * 60 * 60); int64_t nUpdateInterval = (fCurrentlyOnline ? 60 * 60 : 24 * 60 * 60); if (addr.nTime && (!pinfo->nTime || pinfo->nTime < addr.nTime - nUpdateInterval - nTimePenalty)) - pinfo->nTime = max((int64_t)0, addr.nTime - nTimePenalty); + pinfo->nTime = std::max((int64_t)0, addr.nTime - nTimePenalty); // add services pinfo->nServices |= addr.nServices; @@ -283,7 +281,7 @@ bool CAddrMan::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimeP return false; } else { pinfo = Create(addr, source, &nId); - pinfo->nTime = max((int64_t)0, (int64_t)pinfo->nTime - nTimePenalty); + pinfo->nTime = std::max((int64_t)0, (int64_t)pinfo->nTime - nTimePenalty); nNew++; fNew = true; } From 0dd2bf94dda8dd13992e2bd64b38f6611776e460 Mon Sep 17 00:00:00 2001 From: Simon Date: Tue, 24 May 2016 23:56:31 -0700 Subject: [PATCH 6/7] Declare constants for the maximum number of retries, when to sleep between retries and how long for. --- src/addrman.cpp | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 654cea1d2..fa4e866cb 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -334,8 +334,10 @@ CAddrInfo CAddrMan::Select_(bool newOnly) if (size() == 0) return CAddrInfo(); - // Track number of attempts to find a table entry, before giving up - int nRetries = 0; + // Track number of attempts to find a table entry, before giving up to avoid infinite loop + const int kMaxRetries = 200000; // magic number so unit tests can pass + const int kRetriesBetweenSleep = 1000; + const int kRetrySleepInterval = 100; // milliseconds if (newOnly && nNew == 0) return CAddrInfo(); @@ -345,12 +347,17 @@ CAddrInfo CAddrMan::Select_(bool newOnly) (nTried > 0 && (nNew == 0 || RandomInt(2) == 0))) { // use a tried node double fChanceFactor = 1.0; - while (nRetries++ < ADDRMAN_TRIED_BUCKET_COUNT) { + while (1) { + int i = 0; int nKBucket = RandomInt(ADDRMAN_TRIED_BUCKET_COUNT); int nKBucketPos = RandomInt(ADDRMAN_BUCKET_SIZE); while (vvTried[nKBucket][nKBucketPos] == -1) { nKBucket = (nKBucket + insecure_rand()) % ADDRMAN_TRIED_BUCKET_COUNT; nKBucketPos = (nKBucketPos + insecure_rand()) % ADDRMAN_BUCKET_SIZE; + if (i++ > kMaxRetries) + return CAddrInfo(); + if (i % kRetriesBetweenSleep == 0) + MilliSleep(kRetrySleepInterval); } int nId = vvTried[nKBucket][nKBucketPos]; assert(mapInfo.count(nId) == 1); @@ -358,17 +365,21 @@ CAddrInfo CAddrMan::Select_(bool newOnly) if (RandomInt(1 << 30) < fChanceFactor * info.GetChance() * (1 << 30)) return info; fChanceFactor *= 1.2; - MilliSleep(100); } } else { // use a new node double fChanceFactor = 1.0; - while (nRetries++ < ADDRMAN_NEW_BUCKET_COUNT) { + while (1) { + int i = 0; int nUBucket = RandomInt(ADDRMAN_NEW_BUCKET_COUNT); int nUBucketPos = RandomInt(ADDRMAN_BUCKET_SIZE); while (vvNew[nUBucket][nUBucketPos] == -1) { nUBucket = (nUBucket + insecure_rand()) % ADDRMAN_NEW_BUCKET_COUNT; nUBucketPos = (nUBucketPos + insecure_rand()) % ADDRMAN_BUCKET_SIZE; + if (i++ > kMaxRetries) + return CAddrInfo(); + if (i % kRetriesBetweenSleep == 0) + MilliSleep(kRetrySleepInterval); } int nId = vvNew[nUBucket][nUBucketPos]; assert(mapInfo.count(nId) == 1); @@ -376,7 +387,6 @@ CAddrInfo CAddrMan::Select_(bool newOnly) if (RandomInt(1 << 30) < fChanceFactor * info.GetChance() * (1 << 30)) return info; fChanceFactor *= 1.2; - MilliSleep(100); } } From edab3ddd2e1d27e0ea5d02f995898e471686cbc3 Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 6 Jun 2016 17:11:15 +0800 Subject: [PATCH 7/7] Implement issue #997 to reduce time for test_bitcoin due to sleeps in addrman. Related to issue #717. --- src/addrman.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index fa4e866cb..c4a2e6e80 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -356,7 +356,7 @@ CAddrInfo CAddrMan::Select_(bool newOnly) nKBucketPos = (nKBucketPos + insecure_rand()) % ADDRMAN_BUCKET_SIZE; if (i++ > kMaxRetries) return CAddrInfo(); - if (i % kRetriesBetweenSleep == 0) + if (i % kRetriesBetweenSleep == 0 && !nKey.IsNull()) MilliSleep(kRetrySleepInterval); } int nId = vvTried[nKBucket][nKBucketPos]; @@ -378,7 +378,7 @@ CAddrInfo CAddrMan::Select_(bool newOnly) nUBucketPos = (nUBucketPos + insecure_rand()) % ADDRMAN_BUCKET_SIZE; if (i++ > kMaxRetries) return CAddrInfo(); - if (i % kRetriesBetweenSleep == 0) + if (i % kRetriesBetweenSleep == 0 && !nKey.IsNull()) MilliSleep(kRetrySleepInterval); } int nId = vvNew[nUBucket][nUBucketPos];