[addrman] Add deterministic argument to CAddrMan ctor

Removes the need for tests to update nKey and insecure_rand after constructing
a CAddrMan.

Author: jnewbery

src/addrman.h

@@ -493,9 +493,11 @@ class CAddrMan
         mapAddr.clear();
     }
 
-    CAddrMan()
+    explicit CAddrMan(bool deterministic)
+        : insecure_rand{deterministic}
     {
         Clear();
+        if (deterministic) nKey.SetNull();
     }
 
     ~CAddrMan()
@@ -637,13 +639,13 @@ class CAddrMan
     //! 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

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<CAddrMan> addrmans(bench.epochs() * bench.epochIterations());
-    for (auto& addrman : addrmans) {
-        FillAddrMan(addrman);
+    std::vector<std::unique_ptr<CAddrMan>> addrmans(addrman_count);
+    for (size_t i{0}; i < addrman_count; ++i) {
+        addrmans[i] = std::make_unique<CAddrMan>(/* 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;
     });
 }

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<CAddrMan>();
+    node.addrman = std::make_unique<CAddrMan>(/* deterministic */ false);
     assert(!node.banman);
     node.banman = std::make_unique<BanMan>(gArgs.GetDataDirNet() / "banlist", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
     assert(!node.connman);

src/test/addrman_tests.cpp

@@ -21,24 +21,13 @@ class CAddrManTest : public CAddrMan
     bool deterministic;
 public:
     explicit CAddrManTest(bool makeDeterministic = true,
-        std::vector<bool> asmap = std::vector<bool>())
+                          std::vector<bool> asmap = std::vector<bool>())
+        : 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);

src/test/fuzz/addrman.cpp

@@ -29,7 +29,8 @@ class CAddrManDeterministic : public CAddrMan
     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()) {

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<uint64_t>(), fuzzed_data_provider.ConsumeIntegral<uint64_t>(), addrman, fuzzed_data_provider.ConsumeBool()};
     CNetAddr random_netaddr;
     CNode random_node = ConsumeNode(fuzzed_data_provider);

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);
 }

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, {

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);

src/test/util/setup_common.cpp

@@ -193,7 +193,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
         throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", state.ToString()));
     }
 
-    m_node.addrman = std::make_unique<CAddrMan>();
+    m_node.addrman = std::make_unique<CAddrMan>(/* deterministic */ false);
     m_node.banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
     m_node.connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests.
     m_node.peerman = PeerManager::make(chainparams, *m_node.connman, *m_node.addrman,