From fa9710f62c29c7f8d71c9f281001c9b5e70946bf Mon Sep 17 00:00:00 2001 From: John Newbery Date: Fri, 23 Jul 2021 10:48:34 +0100 Subject: [addrman] Add deterministic argument to CAddrMan ctor Removes the need for tests to update nKey and insecure_rand after constructing a CAddrMan. --- src/addrman.h | 10 ++++++---- src/bench/addrman.cpp | 16 +++++++++------- src/init.cpp | 2 +- src/test/addrman_tests.cpp | 15 ++------------- src/test/fuzz/addrman.cpp | 3 ++- src/test/fuzz/connman.cpp | 2 +- src/test/fuzz/data_stream.cpp | 2 +- src/test/fuzz/deserialize.cpp | 2 +- src/test/net_tests.cpp | 20 +++++++------------- src/test/util/setup_common.cpp | 2 +- 10 files changed, 31 insertions(+), 43 deletions(-) (limited to 'src') diff --git a/src/addrman.h b/src/addrman.h index 16be374f7b..2503f53a35 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -493,9 +493,11 @@ public: mapAddr.clear(); } - CAddrMan() + explicit CAddrMan(bool deterministic) + : insecure_rand{deterministic} { Clear(); + if (deterministic) nKey.SetNull(); } ~CAddrMan() @@ -637,13 +639,13 @@ protected: //! secret key to randomize bucket select with uint256 nKey; - //! Source of random numbers for randomization in inner loops - mutable FastRandomContext insecure_rand GUARDED_BY(cs); - //! 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); + //! Serialization versions. enum Format : uint8_t { V0_HISTORICAL = 0, //!< historic format, before commit e6b343d88 diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp index b7bd8a3261..e1175e44ec 100644 --- a/src/bench/addrman.cpp +++ b/src/bench/addrman.cpp @@ -72,7 +72,7 @@ static void AddrManAdd(benchmark::Bench& bench) { CreateAddresses(); - CAddrMan addrman; + CAddrMan addrman(/* deterministic */ false); bench.run([&] { AddAddressesToAddrMan(addrman); @@ -82,7 +82,7 @@ static void AddrManAdd(benchmark::Bench& bench) static void AddrManSelect(benchmark::Bench& bench) { - CAddrMan addrman; + CAddrMan addrman(/* deterministic */ false); FillAddrMan(addrman); @@ -94,7 +94,7 @@ static void AddrManSelect(benchmark::Bench& bench) static void AddrManGetAddr(benchmark::Bench& bench) { - CAddrMan addrman; + CAddrMan addrman(/* deterministic */ false); FillAddrMan(addrman); @@ -112,10 +112,12 @@ static void AddrManGood(benchmark::Bench& bench) * we want to do the same amount of work in every loop iteration. */ bench.epochs(5).epochIterations(1); + const size_t addrman_count{bench.epochs() * bench.epochIterations()}; - std::vector addrmans(bench.epochs() * bench.epochIterations()); - for (auto& addrman : addrmans) { - FillAddrMan(addrman); + std::vector> addrmans(addrman_count); + for (size_t i{0}; i < addrman_count; ++i) { + addrmans[i] = std::make_unique(/* deterministic */ false); + FillAddrMan(*addrmans[i]); } auto markSomeAsGood = [](CAddrMan& addrman) { @@ -130,7 +132,7 @@ static void AddrManGood(benchmark::Bench& bench) uint64_t i = 0; bench.run([&] { - markSomeAsGood(addrmans.at(i)); + markSomeAsGood(*addrmans.at(i)); ++i; }); } diff --git a/src/init.cpp b/src/init.cpp index 1b406bed28..2250834111 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1164,7 +1164,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) const bool ignores_incoming_txs{args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)}; assert(!node.addrman); - node.addrman = std::make_unique(); + node.addrman = std::make_unique(/* deterministic */ false); 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/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 79c7102c4f..0ab6cd0a2b 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -21,24 +21,13 @@ private: bool deterministic; public: explicit CAddrManTest(bool makeDeterministic = true, - std::vector asmap = std::vector()) + std::vector asmap = std::vector()) + : CAddrMan(makeDeterministic) { - if (makeDeterministic) { - // Set addrman addr placement to be deterministic. - MakeDeterministic(); - } deterministic = makeDeterministic; m_asmap = asmap; } - //! Ensure that bucket placement is always the same for testing purposes. - void MakeDeterministic() - { - LOCK(cs); - nKey.SetNull(); - insecure_rand = FastRandomContext(true); - } - CAddrInfo* Find(const CNetAddr& addr, int* pnId = nullptr) { LOCK(cs); diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index 4c29a8ee53..b5e946c528 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -29,7 +29,8 @@ public: FuzzedDataProvider& m_fuzzed_data_provider; explicit CAddrManDeterministic(FuzzedDataProvider& fuzzed_data_provider) - : m_fuzzed_data_provider(fuzzed_data_provider) + : CAddrMan(/* deterministic */ true) + , m_fuzzed_data_provider(fuzzed_data_provider) { WITH_LOCK(cs, insecure_rand = FastRandomContext{ConsumeUInt256(fuzzed_data_provider)}); if (fuzzed_data_provider.ConsumeBool()) { diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index bbec5943af..a32cb54465 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -25,7 +25,7 @@ FUZZ_TARGET_INIT(connman, initialize_connman) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; SetMockTime(ConsumeTime(fuzzed_data_provider)); - CAddrMan addrman; + CAddrMan addrman(/* deterministic */ false); CConnman connman{fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral(), addrman, fuzzed_data_provider.ConsumeBool()}; CNetAddr random_netaddr; CNode random_node = ConsumeNode(fuzzed_data_provider); diff --git a/src/test/fuzz/data_stream.cpp b/src/test/fuzz/data_stream.cpp index 473caec6ff..63bc4b2afe 100644 --- a/src/test/fuzz/data_stream.cpp +++ b/src/test/fuzz/data_stream.cpp @@ -21,6 +21,6 @@ FUZZ_TARGET_INIT(data_stream_addr_man, initialize_data_stream_addr_man) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; CDataStream data_stream = ConsumeDataStream(fuzzed_data_provider); - CAddrMan addr_man; + CAddrMan addr_man(/* deterministic */ false); CAddrDB::Read(addr_man, data_stream); } diff --git a/src/test/fuzz/deserialize.cpp b/src/test/fuzz/deserialize.cpp index d5b56cb7cd..6ab3389123 100644 --- a/src/test/fuzz/deserialize.cpp +++ b/src/test/fuzz/deserialize.cpp @@ -188,7 +188,7 @@ FUZZ_TARGET_DESERIALIZE(blockmerkleroot, { BlockMerkleRoot(block, &mutated); }) FUZZ_TARGET_DESERIALIZE(addrman_deserialize, { - CAddrMan am; + CAddrMan am(/* deterministic */ false); DeserializeFromFuzzingInput(buffer, am); }) FUZZ_TARGET_DESERIALIZE(blockheader_deserialize, { diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index acbbf357d2..ff9e9b7a07 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -34,13 +34,9 @@ class CAddrManSerializationMock : public CAddrMan public: virtual void Serialize(CDataStream& s) const = 0; - //! Ensure that bucket placement is always the same for testing purposes. - void MakeDeterministic() - { - LOCK(cs); - nKey.SetNull(); - insecure_rand = FastRandomContext(true); - } + CAddrManSerializationMock() + : CAddrMan(/* deterministic */ true) + {} }; class CAddrManUncorrupted : public CAddrManSerializationMock @@ -105,7 +101,6 @@ BOOST_AUTO_TEST_CASE(cnode_listen_port) BOOST_AUTO_TEST_CASE(caddrdb_read) { CAddrManUncorrupted addrmanUncorrupted; - addrmanUncorrupted.MakeDeterministic(); CService addr1, addr2, addr3; BOOST_CHECK(Lookup("250.7.1.1", addr1, 8333, false)); @@ -124,7 +119,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read) // Test that the de-serialization does not throw an exception. CDataStream ssPeers1 = AddrmanToStream(addrmanUncorrupted); bool exceptionThrown = false; - CAddrMan addrman1; + CAddrMan addrman1(/* deterministic */ false); BOOST_CHECK(addrman1.size() == 0); try { @@ -141,7 +136,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read) // Test that CAddrDB::Read creates an addrman with the correct number of addrs. CDataStream ssPeers2 = AddrmanToStream(addrmanUncorrupted); - CAddrMan addrman2; + CAddrMan addrman2(/* deterministic */ false); BOOST_CHECK(addrman2.size() == 0); BOOST_CHECK(CAddrDB::Read(addrman2, ssPeers2)); BOOST_CHECK(addrman2.size() == 3); @@ -151,12 +146,11 @@ BOOST_AUTO_TEST_CASE(caddrdb_read) BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted) { CAddrManCorrupted addrmanCorrupted; - addrmanCorrupted.MakeDeterministic(); // Test that the de-serialization of corrupted addrman throws an exception. CDataStream ssPeers1 = AddrmanToStream(addrmanCorrupted); bool exceptionThrown = false; - CAddrMan addrman1; + CAddrMan addrman1(/* deterministic */ false); BOOST_CHECK(addrman1.size() == 0); try { unsigned char pchMsgTmp[4]; @@ -172,7 +166,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted) // Test that CAddrDB::Read leaves addrman in a clean state if de-serialization fails. CDataStream ssPeers2 = AddrmanToStream(addrmanCorrupted); - CAddrMan addrman2; + CAddrMan addrman2(/* deterministic */ false); BOOST_CHECK(addrman2.size() == 0); BOOST_CHECK(!CAddrDB::Read(addrman2, ssPeers2)); BOOST_CHECK(addrman2.size() == 0); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 2d044af184..b1567d924b 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -193,7 +193,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector(); + m_node.addrman = std::make_unique(/* deterministic */ false); m_node.banman = std::make_unique(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); m_node.connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests. m_node.peerman = PeerManager::make(chainparams, *m_node.connman, *m_node.addrman, -- cgit v1.2.3