diff --git a/src/addrdb.cpp b/src/addrdb.cpp index 2931b098ab..74f470a93c 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -245,12 +245,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/addrman.cpp b/src/addrman.cpp index bd83461be7..0ad62571f1 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -79,6 +79,23 @@ double CAddrInfo::GetChance(int64_t nNow) const return fChance; } +CAddrMan::CAddrMan(bool deterministic, int32_t consistency_check_ratio, bool _discriminatePorts) + : insecure_rand{deterministic} + , nKey{deterministic ? uint256{1} : insecure_rand.rand256()} + , m_consistency_check_ratio{consistency_check_ratio} + , discriminatePorts{_discriminatePorts} +{ + for (auto& bucket : vvNew) { + for (auto& entry : bucket) { + entry = -1; + } + } + for (auto& bucket : vvTried) { + for (auto& entry : bucket) { + entry = -1; + } + } +} CAddrInfo* CAddrMan::Find(const CService& addr, int* pnId) { diff --git a/src/addrman.h b/src/addrman.h index 422c9aec60..1066623f6f 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -473,39 +473,7 @@ public: Check(); } - 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(); - } - - explicit CAddrMan(bool deterministic, int32_t consistency_check_ratio, bool _discriminatePorts = false) : - insecure_rand{deterministic}, - m_consistency_check_ratio{consistency_check_ratio}, - discriminatePorts(_discriminatePorts) - { - Clear(); - if (deterministic) nKey = uint256{1}; - } + explicit CAddrMan(bool deterministic, int32_t consistency_check_ratio, bool _discriminatePorts = false); ~CAddrMan() { @@ -638,17 +606,17 @@ public: } return addrRet; } -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 @@ -672,7 +640,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); @@ -686,19 +654,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; diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp index b96efb7b46..f65bc8145d 100644 --- a/src/bench/addrman.cpp +++ b/src/bench/addrman.cpp @@ -71,11 +71,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/init.cpp b/src/init.cpp index b6cbc28cb3..cb2c7e47ed 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1666,6 +1666,20 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc 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 { + // 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); + } + } assert(!node.banman); node.banman = std::make_unique(GetDataDir() / "banlist", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME)); assert(!node.connman); diff --git a/src/net.cpp b/src/net.cpp index 2e5c297cac..3b7c17248d 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3383,22 +3383,6 @@ bool CConnman::Start(CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_met AddAddrFetch(strDest); } - if (clientInterface) { - clientInterface->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(GetDataDir() / ANCHORS_DATABASE_FILENAME); diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 1fc0f6c4e6..8e4d1692ea 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -46,7 +46,7 @@ public: unsigned char nVersion = 1; s << nVersion; s << ((unsigned char)32); - s << nKey; + s << uint256::ONE; s << 10; // nNew s << 10; // nTried @@ -131,16 +131,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) @@ -174,27 +164,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 @@ -204,21 +194,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) @@ -773,23 +758,23 @@ BOOST_AUTO_TEST_CASE(addrman_serialization) { std::vector asmap1 = FromBytes(raw_tests::asmap, sizeof(raw_tests::asmap) * 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); @@ -797,39 +782,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); } @@ -838,7 +823,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}; @@ -846,12 +831,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; @@ -870,9 +855,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) @@ -1083,13 +1068,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); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index 769324c0c8..b4b742b44e 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(); },