From 181a1207ba6bd179d181f3e2534ef8676565ce72 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 5 Aug 2021 11:09:55 +0100 Subject: [PATCH 1/6] [addrman] Move peers.dat parsing to init.cpp --- src/init.cpp | 13 +++++++++++++ src/net.cpp | 16 ---------------- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 9154fc0a6f..2ca6cebee7 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1167,6 +1167,19 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) assert(!node.addrman); auto check_addrman = std::clamp(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000); node.addrman = std::make_unique(/* deterministic */ false, /* consistency_check_ratio */ check_addrman); + { + // Load addresses from peers.dat + uiInterface.InitMessage(_("Loading P2P addresses…").translated); + int64_t nStart = GetTimeMillis(); + CAddrDB adb; + if (adb.Read(*node.addrman)) { + LogPrintf("Loaded %i addresses from peers.dat %dms\n", node.addrman->size(), GetTimeMillis() - nStart); + } else { + node.addrman->Clear(); // Addrman can be in an inconsistent state after failure, reset it + LogPrintf("Recreating peers.dat\n"); + adb.Write(*node.addrman); + } + } assert(!node.banman); node.banman = std::make_unique(gArgs.GetDataDirNet() / "banlist", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME)); assert(!node.connman); diff --git a/src/net.cpp b/src/net.cpp index be419648d3..57b8844d6b 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2534,22 +2534,6 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) AddAddrFetch(strDest); } - if (m_client_interface) { - m_client_interface->InitMessage(_("Loading P2P addresses…").translated); - } - // Load addresses from peers.dat - int64_t nStart = GetTimeMillis(); - { - CAddrDB adb; - if (adb.Read(addrman)) - LogPrintf("Loaded %i addresses from peers.dat %dms\n", addrman.size(), GetTimeMillis() - nStart); - else { - addrman.Clear(); // Addrman can be in an inconsistent state after failure, reset it - LogPrintf("Recreating peers.dat\n"); - DumpAddresses(); - } - } - if (m_use_addrman_outgoing) { // Load addresses from anchors.dat m_anchors = ReadAnchors(gArgs.GetDataDirNet() / ANCHORS_DATABASE_FILENAME); From e8e7392311edf44278d76743bebe902d4ac94662 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 5 Aug 2021 11:15:35 +0100 Subject: [PATCH 2/6] [addrman] Don't call Clear() if parsing peers.dat fails Now that we manage the lifespan of node.addrman, we can just reset node.addrman to a newly initialized CAddrMan if parsing peers.dat fails, eliminating the possibility that Clear() leaves some old state behind. --- src/init.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/init.cpp b/src/init.cpp index 2ca6cebee7..d741badc92 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1175,7 +1175,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) if (adb.Read(*node.addrman)) { LogPrintf("Loaded %i addresses from peers.dat %dms\n", node.addrman->size(), GetTimeMillis() - nStart); } else { - node.addrman->Clear(); // Addrman can be in an inconsistent state after failure, reset it + // Addrman can be in an inconsistent state after failure, reset it + node.addrman = std::make_unique(/* deterministic */ false, /* consistency_check_ratio */ check_addrman); LogPrintf("Recreating peers.dat\n"); adb.Write(*node.addrman); } From ed9ba8af08f857bda3ce2f77413317374c22d7b4 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 5 Aug 2021 13:51:52 +0100 Subject: [PATCH 3/6] [tests] Remove CAddrMan.Clear() call from CAddrDB::Read() `bool CAddrDB::Read(CAddrMan& addr, CDataStream& ssPeers)` is _only_ called from the tests, and the call to addr.Clear() only exists so that a test that Clear() is called passes. Remove that test and the call. --- src/addrdb.cpp | 7 +------ src/test/addrman_tests.cpp | 3 +-- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/src/addrdb.cpp b/src/addrdb.cpp index c3e224ee83..a5383be7cf 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -244,12 +244,7 @@ bool CAddrDB::Read(CAddrMan& addr) bool CAddrDB::Read(CAddrMan& addr, CDataStream& ssPeers) { - bool ret = DeserializeDB(ssPeers, addr, false); - if (!ret) { - // Ensure addrman is left in a clean state - addr.Clear(); - } - return ret; + return DeserializeDB(ssPeers, addr, false); } void DumpAnchors(const fs::path& anchors_db_path, const std::vector& anchors) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index c52baa4e8b..255b5076e7 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -1083,13 +1083,12 @@ BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted) BOOST_CHECK(addrman1.size() == 1); BOOST_CHECK(exceptionThrown); - // Test that CAddrDB::Read leaves addrman in a clean state if de-serialization fails. + // Test that CAddrDB::Read fails if peers.dat is corrupt CDataStream ssPeers2 = AddrmanToStream(addrmanCorrupted); CAddrMan addrman2(/* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman2.size() == 0); BOOST_CHECK(!CAddrDB::Read(addrman2, ssPeers2)); - BOOST_CHECK(addrman2.size() == 0); } From 406be5ff9699874dc1d38d11f036e33cbdb820c9 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 5 Aug 2021 14:08:48 +0100 Subject: [PATCH 4/6] [addrman] Remove all public uses of CAddrMan.Clear() from the tests Just use unique_ptrs and reset the pointer if a frest addrman is required. Also make CAddrMan::Clear() private to ensure that no call sites are missed. --- src/addrman.h | 2 + src/bench/addrman.cpp | 4 +- src/test/addrman_tests.cpp | 113 ++++++++++++++++--------------------- src/test/fuzz/addrman.cpp | 10 ++-- 4 files changed, 56 insertions(+), 73 deletions(-) diff --git a/src/addrman.h b/src/addrman.h index fa155fb00a..c2df87e986 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -471,6 +471,7 @@ public: Check(); } +private: void Clear() EXCLUSIVE_LOCKS_REQUIRED(!cs) { @@ -496,6 +497,7 @@ public: mapAddr.clear(); } +public: explicit CAddrMan(bool deterministic, int32_t consistency_check_ratio) : insecure_rand{deterministic}, m_consistency_check_ratio{consistency_check_ratio} diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp index 5ae2dafd5a..d69a651811 100644 --- a/src/bench/addrman.cpp +++ b/src/bench/addrman.cpp @@ -72,11 +72,9 @@ static void AddrManAdd(benchmark::Bench& bench) { CreateAddresses(); - CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0); - bench.run([&] { + CAddrMan addrman{/* deterministic */ false, /* consistency_check_ratio */ 0}; AddAddressesToAddrMan(addrman); - addrman.Clear(); }); } diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 255b5076e7..50be342630 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -132,16 +132,6 @@ public: int64_t nLastTry = GetAdjustedTime()-61; Attempt(addr, count_failure, nLastTry); } - - void Clear() - { - CAddrMan::Clear(); - if (deterministic) { - LOCK(cs); - nKey = uint256{1}; - insecure_rand = FastRandomContext(true); - } - } }; static CNetAddr ResolveIP(const std::string& ip) @@ -175,27 +165,27 @@ BOOST_FIXTURE_TEST_SUITE(addrman_tests, BasicTestingSetup) BOOST_AUTO_TEST_CASE(addrman_simple) { - CAddrManTest addrman; + auto addrman = std::make_unique(); CNetAddr source = ResolveIP("252.2.2.2"); // Test: Does Addrman respond correctly when empty. - BOOST_CHECK_EQUAL(addrman.size(), 0U); - CAddrInfo addr_null = addrman.Select(); + BOOST_CHECK_EQUAL(addrman->size(), 0U); + CAddrInfo addr_null = addrman->Select(); BOOST_CHECK_EQUAL(addr_null.ToString(), "[::]:0"); // Test: Does Addrman::Add work as expected. CService addr1 = ResolveService("250.1.1.1", 8333); - BOOST_CHECK(addrman.Add({CAddress(addr1, NODE_NONE)}, source)); - BOOST_CHECK_EQUAL(addrman.size(), 1U); - CAddrInfo addr_ret1 = addrman.Select(); + BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source)); + BOOST_CHECK_EQUAL(addrman->size(), 1U); + CAddrInfo addr_ret1 = addrman->Select(); BOOST_CHECK_EQUAL(addr_ret1.ToString(), "250.1.1.1:8333"); // Test: Does IP address deduplication work correctly. // Expected dup IP should not be added. CService addr1_dup = ResolveService("250.1.1.1", 8333); - BOOST_CHECK(!addrman.Add({CAddress(addr1_dup, NODE_NONE)}, source)); - BOOST_CHECK_EQUAL(addrman.size(), 1U); + BOOST_CHECK(!addrman->Add({CAddress(addr1_dup, NODE_NONE)}, source)); + BOOST_CHECK_EQUAL(addrman->size(), 1U); // Test: New table has one addr and we add a diff addr we should @@ -205,21 +195,16 @@ BOOST_AUTO_TEST_CASE(addrman_simple) // success. CService addr2 = ResolveService("250.1.1.2", 8333); - BOOST_CHECK(addrman.Add({CAddress(addr2, NODE_NONE)}, source)); - BOOST_CHECK(addrman.size() >= 1); + BOOST_CHECK(addrman->Add({CAddress(addr2, NODE_NONE)}, source)); + BOOST_CHECK(addrman->size() >= 1); - // Test: AddrMan::Clear() should empty the new table. - addrman.Clear(); - BOOST_CHECK_EQUAL(addrman.size(), 0U); - CAddrInfo addr_null2 = addrman.Select(); - BOOST_CHECK_EQUAL(addr_null2.ToString(), "[::]:0"); - - // Test: AddrMan::Add multiple addresses works as expected + // Test: reset addrman and test AddrMan::Add multiple addresses works as expected + addrman = std::make_unique(); std::vector vAddr; vAddr.push_back(CAddress(ResolveService("250.1.1.3", 8333), NODE_NONE)); vAddr.push_back(CAddress(ResolveService("250.1.1.4", 8333), NODE_NONE)); - BOOST_CHECK(addrman.Add(vAddr, source)); - BOOST_CHECK(addrman.size() >= 1); + BOOST_CHECK(addrman->Add(vAddr, source)); + BOOST_CHECK(addrman->size() >= 1); } BOOST_AUTO_TEST_CASE(addrman_ports) @@ -774,23 +759,23 @@ BOOST_AUTO_TEST_CASE(addrman_serialization) { std::vector asmap1 = FromBytes(asmap_raw, sizeof(asmap_raw) * 8); - CAddrManTest addrman_asmap1(true, asmap1); - CAddrManTest addrman_asmap1_dup(true, asmap1); - CAddrManTest addrman_noasmap; + auto addrman_asmap1 = std::make_unique(true, asmap1); + auto addrman_asmap1_dup = std::make_unique(true, asmap1); + auto addrman_noasmap = std::make_unique(); CDataStream stream(SER_NETWORK, PROTOCOL_VERSION); CAddress addr = CAddress(ResolveService("250.1.1.1"), NODE_NONE); CNetAddr default_source; - addrman_asmap1.Add({addr}, default_source); + addrman_asmap1->Add({addr}, default_source); - stream << addrman_asmap1; + stream << *addrman_asmap1; // serizalizing/deserializing addrman with the same asmap - stream >> addrman_asmap1_dup; + stream >> *addrman_asmap1_dup; - std::pair bucketAndEntry_asmap1 = addrman_asmap1.GetBucketAndEntry(addr); - std::pair bucketAndEntry_asmap1_dup = addrman_asmap1_dup.GetBucketAndEntry(addr); + std::pair bucketAndEntry_asmap1 = addrman_asmap1->GetBucketAndEntry(addr); + std::pair bucketAndEntry_asmap1_dup = addrman_asmap1_dup->GetBucketAndEntry(addr); BOOST_CHECK(bucketAndEntry_asmap1.second != -1); BOOST_CHECK(bucketAndEntry_asmap1_dup.second != -1); @@ -798,39 +783,39 @@ BOOST_AUTO_TEST_CASE(addrman_serialization) BOOST_CHECK(bucketAndEntry_asmap1.second == bucketAndEntry_asmap1_dup.second); // deserializing asmaped peers.dat to non-asmaped addrman - stream << addrman_asmap1; - stream >> addrman_noasmap; - std::pair bucketAndEntry_noasmap = addrman_noasmap.GetBucketAndEntry(addr); + stream << *addrman_asmap1; + stream >> *addrman_noasmap; + std::pair bucketAndEntry_noasmap = addrman_noasmap->GetBucketAndEntry(addr); BOOST_CHECK(bucketAndEntry_noasmap.second != -1); BOOST_CHECK(bucketAndEntry_asmap1.first != bucketAndEntry_noasmap.first); BOOST_CHECK(bucketAndEntry_asmap1.second != bucketAndEntry_noasmap.second); // deserializing non-asmaped peers.dat to asmaped addrman - addrman_asmap1.Clear(); - addrman_noasmap.Clear(); - addrman_noasmap.Add({addr}, default_source); - stream << addrman_noasmap; - stream >> addrman_asmap1; - std::pair bucketAndEntry_asmap1_deser = addrman_asmap1.GetBucketAndEntry(addr); + addrman_asmap1 = std::make_unique(true, asmap1); + addrman_noasmap = std::make_unique(); + addrman_noasmap->Add({addr}, default_source); + stream << *addrman_noasmap; + stream >> *addrman_asmap1; + std::pair bucketAndEntry_asmap1_deser = addrman_asmap1->GetBucketAndEntry(addr); BOOST_CHECK(bucketAndEntry_asmap1_deser.second != -1); BOOST_CHECK(bucketAndEntry_asmap1_deser.first != bucketAndEntry_noasmap.first); BOOST_CHECK(bucketAndEntry_asmap1_deser.first == bucketAndEntry_asmap1_dup.first); BOOST_CHECK(bucketAndEntry_asmap1_deser.second == bucketAndEntry_asmap1_dup.second); // used to map to different buckets, now maps to the same bucket. - addrman_asmap1.Clear(); - addrman_noasmap.Clear(); + addrman_asmap1 = std::make_unique(true, asmap1); + addrman_noasmap = std::make_unique(); CAddress addr1 = CAddress(ResolveService("250.1.1.1"), NODE_NONE); CAddress addr2 = CAddress(ResolveService("250.2.1.1"), NODE_NONE); - addrman_noasmap.Add({addr, addr2}, default_source); - std::pair bucketAndEntry_noasmap_addr1 = addrman_noasmap.GetBucketAndEntry(addr1); - std::pair bucketAndEntry_noasmap_addr2 = addrman_noasmap.GetBucketAndEntry(addr2); + addrman_noasmap->Add({addr, addr2}, default_source); + std::pair bucketAndEntry_noasmap_addr1 = addrman_noasmap->GetBucketAndEntry(addr1); + std::pair bucketAndEntry_noasmap_addr2 = addrman_noasmap->GetBucketAndEntry(addr2); BOOST_CHECK(bucketAndEntry_noasmap_addr1.first != bucketAndEntry_noasmap_addr2.first); BOOST_CHECK(bucketAndEntry_noasmap_addr1.second != bucketAndEntry_noasmap_addr2.second); - stream << addrman_noasmap; - stream >> addrman_asmap1; - std::pair bucketAndEntry_asmap1_deser_addr1 = addrman_asmap1.GetBucketAndEntry(addr1); - std::pair bucketAndEntry_asmap1_deser_addr2 = addrman_asmap1.GetBucketAndEntry(addr2); + stream << *addrman_noasmap; + stream >> *addrman_asmap1; + std::pair bucketAndEntry_asmap1_deser_addr1 = addrman_asmap1->GetBucketAndEntry(addr1); + std::pair bucketAndEntry_asmap1_deser_addr2 = addrman_asmap1->GetBucketAndEntry(addr2); BOOST_CHECK(bucketAndEntry_asmap1_deser_addr1.first == bucketAndEntry_asmap1_deser_addr2.first); BOOST_CHECK(bucketAndEntry_asmap1_deser_addr1.second != bucketAndEntry_asmap1_deser_addr2.second); } @@ -839,7 +824,7 @@ BOOST_AUTO_TEST_CASE(remove_invalid) { // Confirm that invalid addresses are ignored in unserialization. - CAddrManTest addrman; + auto addrman = std::make_unique(); CDataStream stream(SER_NETWORK, PROTOCOL_VERSION); const CAddress new1{ResolveService("5.5.5.5"), NODE_NONE}; @@ -847,12 +832,12 @@ BOOST_AUTO_TEST_CASE(remove_invalid) const CAddress tried1{ResolveService("7.7.7.7"), NODE_NONE}; const CAddress tried2{ResolveService("8.8.8.8"), NODE_NONE}; - addrman.Add({new1, tried1, new2, tried2}, CNetAddr{}); - addrman.Good(tried1); - addrman.Good(tried2); - BOOST_REQUIRE_EQUAL(addrman.size(), 4); + addrman->Add({new1, tried1, new2, tried2}, CNetAddr{}); + addrman->Good(tried1); + addrman->Good(tried2); + BOOST_REQUIRE_EQUAL(addrman->size(), 4); - stream << addrman; + stream << *addrman; const std::string str{stream.str()}; size_t pos; @@ -871,9 +856,9 @@ BOOST_AUTO_TEST_CASE(remove_invalid) BOOST_REQUIRE(pos + sizeof(tried2_raw_replacement) <= stream.size()); memcpy(stream.data() + pos, tried2_raw_replacement, sizeof(tried2_raw_replacement)); - addrman.Clear(); - stream >> addrman; - BOOST_CHECK_EQUAL(addrman.size(), 2); + addrman = std::make_unique(); + stream >> *addrman; + BOOST_CHECK_EQUAL(addrman->size(), 2); } BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision) diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index bc41180a8f..95aa53bff4 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -228,24 +228,22 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); SetMockTime(ConsumeTime(fuzzed_data_provider)); - CAddrManDeterministic addr_man{fuzzed_data_provider}; + auto addr_man_ptr = std::make_unique(fuzzed_data_provider); if (fuzzed_data_provider.ConsumeBool()) { const std::vector serialized_data{ConsumeRandomLengthByteVector(fuzzed_data_provider)}; CDataStream ds(serialized_data, SER_DISK, INIT_PROTO_VERSION); const auto ser_version{fuzzed_data_provider.ConsumeIntegral()}; ds.SetVersion(ser_version); try { - ds >> addr_man; + ds >> *addr_man_ptr; } catch (const std::ios_base::failure&) { - addr_man.Clear(); + addr_man_ptr = std::make_unique(fuzzed_data_provider); } } + CAddrManDeterministic& addr_man = *addr_man_ptr; while (fuzzed_data_provider.ConsumeBool()) { CallOneOf( fuzzed_data_provider, - [&] { - addr_man.Clear(); - }, [&] { addr_man.ResolveCollisions(); }, From 7e6e65918f75211b517fc887f5d90df8edd52ced Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 5 Aug 2021 14:10:02 +0100 Subject: [PATCH 5/6] [addrman] inline Clear() into CAddrMan ctor Clear() is now only called from the ctor, so just inline the code into that function. The LOCK(cs) can be removed, since there can be no data races in the ctor. Also move the function definition out of the header and into the cpp file. --- src/addrman.cpp | 26 ++++++++++++++++++++++ src/addrman.h | 44 +++++--------------------------------- src/test/addrman_tests.cpp | 2 +- 3 files changed, 32 insertions(+), 40 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index e8f98f727b..690ea19c30 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -77,6 +77,32 @@ double CAddrInfo::GetChance(int64_t nNow) const return fChance; } +CAddrMan::CAddrMan(bool deterministic, int32_t consistency_check_ratio) + : insecure_rand{deterministic} + , m_consistency_check_ratio{consistency_check_ratio} +{ + std::vector().swap(vRandom); + nKey = insecure_rand.rand256(); + for (size_t bucket = 0; bucket < ADDRMAN_NEW_BUCKET_COUNT; bucket++) { + for (size_t entry = 0; entry < ADDRMAN_BUCKET_SIZE; entry++) { + vvNew[bucket][entry] = -1; + } + } + for (size_t bucket = 0; bucket < ADDRMAN_TRIED_BUCKET_COUNT; bucket++) { + for (size_t entry = 0; entry < ADDRMAN_BUCKET_SIZE; entry++) { + vvTried[bucket][entry] = -1; + } + } + + nIdCount = 0; + nTried = 0; + nNew = 0; + nLastGood = 1; //Initially at 1 so that "never" is strictly worse. + mapInfo.clear(); + mapAddr.clear(); + if (deterministic) nKey = uint256{1}; +} + CAddrInfo* CAddrMan::Find(const CNetAddr& addr, int* pnId) { AssertLockHeld(cs); diff --git a/src/addrman.h b/src/addrman.h index c2df87e986..3d49c2583f 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -471,40 +471,7 @@ public: Check(); } -private: - void Clear() - EXCLUSIVE_LOCKS_REQUIRED(!cs) - { - LOCK(cs); - std::vector().swap(vRandom); - nKey = insecure_rand.rand256(); - for (size_t bucket = 0; bucket < ADDRMAN_NEW_BUCKET_COUNT; bucket++) { - for (size_t entry = 0; entry < ADDRMAN_BUCKET_SIZE; entry++) { - vvNew[bucket][entry] = -1; - } - } - for (size_t bucket = 0; bucket < ADDRMAN_TRIED_BUCKET_COUNT; bucket++) { - for (size_t entry = 0; entry < ADDRMAN_BUCKET_SIZE; entry++) { - vvTried[bucket][entry] = -1; - } - } - - nIdCount = 0; - nTried = 0; - nNew = 0; - nLastGood = 1; //Initially at 1 so that "never" is strictly worse. - mapInfo.clear(); - mapAddr.clear(); - } - -public: - explicit CAddrMan(bool deterministic, int32_t consistency_check_ratio) - : insecure_rand{deterministic}, - m_consistency_check_ratio{consistency_check_ratio} - { - Clear(); - if (deterministic) nKey = uint256{1}; - } + explicit CAddrMan(bool deterministic, int32_t consistency_check_ratio); ~CAddrMan() { @@ -626,17 +593,16 @@ public: Check(); } -protected: - //! secret key to randomize bucket select with - uint256 nKey; - +private: //! A mutex to protect the inner data structures. mutable Mutex cs; -private: //! Source of random numbers for randomization in inner loops mutable FastRandomContext insecure_rand GUARDED_BY(cs); + //! secret key to randomize bucket select with + uint256 nKey; + //! Serialization versions. enum Format : uint8_t { V0_HISTORICAL = 0, //!< historic format, before commit e6b343d88 diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 50be342630..cd5dc2370f 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -48,7 +48,7 @@ public: unsigned char nVersion = 1; s << nVersion; s << ((unsigned char)32); - s << nKey; + s << uint256::ONE; s << 10; // nNew s << 10; // nTried From 4d2fa97031a6f31da984d4c2c105447ed692c6ed Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 5 Aug 2021 14:14:21 +0100 Subject: [PATCH 6/6] [addrman] Clean up ctor Use default initialization and initializer lists, and use range-based for loops for resetting the buckets. --- src/addrman.cpp | 23 +++++++---------------- src/addrman.h | 10 +++++----- 2 files changed, 12 insertions(+), 21 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 690ea19c30..edcf97f846 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -79,28 +79,19 @@ double CAddrInfo::GetChance(int64_t nNow) const CAddrMan::CAddrMan(bool deterministic, int32_t consistency_check_ratio) : insecure_rand{deterministic} + , nKey{deterministic ? uint256{1} : insecure_rand.rand256()} , m_consistency_check_ratio{consistency_check_ratio} { - std::vector().swap(vRandom); - nKey = insecure_rand.rand256(); - for (size_t bucket = 0; bucket < ADDRMAN_NEW_BUCKET_COUNT; bucket++) { - for (size_t entry = 0; entry < ADDRMAN_BUCKET_SIZE; entry++) { - vvNew[bucket][entry] = -1; + for (auto& bucket : vvNew) { + for (auto& entry : bucket) { + entry = -1; } } - for (size_t bucket = 0; bucket < ADDRMAN_TRIED_BUCKET_COUNT; bucket++) { - for (size_t entry = 0; entry < ADDRMAN_BUCKET_SIZE; entry++) { - vvTried[bucket][entry] = -1; + for (auto& bucket : vvTried) { + for (auto& entry : bucket) { + entry = -1; } } - - nIdCount = 0; - nTried = 0; - nNew = 0; - nLastGood = 1; //Initially at 1 so that "never" is strictly worse. - mapInfo.clear(); - mapAddr.clear(); - if (deterministic) nKey = uint256{1}; } CAddrInfo* CAddrMan::Find(const CNetAddr& addr, int* pnId) diff --git a/src/addrman.h b/src/addrman.h index 3d49c2583f..e2cb60b061 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -626,7 +626,7 @@ private: static constexpr uint8_t INCOMPATIBILITY_BASE = 32; //! last used nId - int nIdCount GUARDED_BY(cs); + int nIdCount GUARDED_BY(cs){0}; //! table with information about all nIds std::unordered_map mapInfo GUARDED_BY(cs); @@ -640,19 +640,19 @@ private: mutable std::vector vRandom GUARDED_BY(cs); // number of "tried" entries - int nTried GUARDED_BY(cs); + int nTried GUARDED_BY(cs){0}; //! list of "tried" buckets int vvTried[ADDRMAN_TRIED_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE] GUARDED_BY(cs); //! number of (unique) "new" entries - int nNew GUARDED_BY(cs); + int nNew GUARDED_BY(cs){0}; //! list of "new" buckets int vvNew[ADDRMAN_NEW_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE] GUARDED_BY(cs); - //! last time Good was called (memory only) - int64_t nLastGood GUARDED_BY(cs); + //! last time Good was called (memory only). Initially set to 1 so that "never" is strictly worse. + int64_t nLastGood GUARDED_BY(cs){1}; //! Holds addrs inserted into tried table that collide with existing entries. Test-before-evict discipline used to resolve these collisions. std::set m_tried_collisions;