Skip to content

Commit fa9710f

Browse files
jnewberyjnewbery
committed
[addrman] Add deterministic argument to CAddrMan ctor
Removes the need for tests to update nKey and insecure_rand after constructing a CAddrMan.
1 parent ee458d8 commit fa9710f

File tree

10 files changed

+31
-43
lines changed

10 files changed

+31
-43
lines changed

‎src/addrman.h‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -493,9 +493,11 @@ class CAddrMan
493493
mapAddr.clear();
494494
}
495495

496-
CAddrMan()
496+
explicit CAddrMan(bool deterministic)
497+
: insecure_rand{deterministic}
497498
{
498499
Clear();
500+
if (deterministic) nKey.SetNull();
499501
}
500502

501503
~CAddrMan()
@@ -637,13 +639,13 @@ class CAddrMan
637639
//! secret key to randomize bucket select with
638640
uint256 nKey;
639641

640-
//! Source of random numbers for randomization in inner loops
641-
mutable FastRandomContext insecure_rand GUARDED_BY(cs);
642-
643642
//! A mutex to protect the inner data structures.
644643
mutable Mutex cs;
645644

646645
private:
646+
//! Source of random numbers for randomization in inner loops
647+
mutable FastRandomContext insecure_rand GUARDED_BY(cs);
648+
647649
//! Serialization versions.
648650
enum Format : uint8_t {
649651
V0_HISTORICAL = 0, //!< historic format, before commit e6b343d88

‎src/bench/addrman.cpp‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ static void AddrManAdd(benchmark::Bench& bench)
7272
{
7373
CreateAddresses();
7474

75-
CAddrMan addrman;
75+
CAddrMan addrman(/* deterministic */ false);
7676

7777
bench.run([&] {
7878
AddAddressesToAddrMan(addrman);
@@ -82,7 +82,7 @@ static void AddrManAdd(benchmark::Bench& bench)
8282

8383
static void AddrManSelect(benchmark::Bench& bench)
8484
{
85-
CAddrMan addrman;
85+
CAddrMan addrman(/* deterministic */ false);
8686

8787
FillAddrMan(addrman);
8888

@@ -94,7 +94,7 @@ static void AddrManSelect(benchmark::Bench& bench)
9494

9595
static void AddrManGetAddr(benchmark::Bench& bench)
9696
{
97-
CAddrMan addrman;
97+
CAddrMan addrman(/* deterministic */ false);
9898

9999
FillAddrMan(addrman);
100100

@@ -112,10 +112,12 @@ static void AddrManGood(benchmark::Bench& bench)
112112
* we want to do the same amount of work in every loop iteration. */
113113

114114
bench.epochs(5).epochIterations(1);
115+
const size_t addrman_count{bench.epochs() * bench.epochIterations()};
115116

116-
std::vector<CAddrMan> addrmans(bench.epochs() * bench.epochIterations());
117-
for (auto& addrman : addrmans) {
118-
FillAddrMan(addrman);
117+
std::vector<std::unique_ptr<CAddrMan>> addrmans(addrman_count);
118+
for (size_t i{0}; i < addrman_count; ++i) {
119+
addrmans[i] = std::make_unique<CAddrMan>(/* deterministic */ false);
120+
FillAddrMan(*addrmans[i]);
119121
}
120122

121123
auto markSomeAsGood = [](CAddrMan& addrman) {
@@ -130,7 +132,7 @@ static void AddrManGood(benchmark::Bench& bench)
130132

131133
uint64_t i = 0;
132134
bench.run([&] {
133-
markSomeAsGood(addrmans.at(i));
135+
markSomeAsGood(*addrmans.at(i));
134136
++i;
135137
});
136138
}

‎src/init.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1164,7 +1164,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
11641164
const bool ignores_incoming_txs{args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)};
11651165

11661166
assert(!node.addrman);
1167-
node.addrman = std::make_unique<CAddrMan>();
1167+
node.addrman = std::make_unique<CAddrMan>(/* deterministic */ false);
11681168
assert(!node.banman);
11691169
node.banman = std::make_unique<BanMan>(gArgs.GetDataDirNet() / "banlist", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
11701170
assert(!node.connman);

‎src/test/addrman_tests.cpp‎

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,24 +21,13 @@ class CAddrManTest : public CAddrMan
2121
bool deterministic;
2222
public:
2323
explicit CAddrManTest(bool makeDeterministic = true,
24-
std::vector<bool> asmap = std::vector<bool>())
24+
std::vector<bool> asmap = std::vector<bool>())
25+
: CAddrMan(makeDeterministic)
2526
{
26-
if (makeDeterministic) {
27-
// Set addrman addr placement to be deterministic.
28-
MakeDeterministic();
29-
}
3027
deterministic = makeDeterministic;
3128
m_asmap = asmap;
3229
}
3330

34-
//! Ensure that bucket placement is always the same for testing purposes.
35-
void MakeDeterministic()
36-
{
37-
LOCK(cs);
38-
nKey.SetNull();
39-
insecure_rand = FastRandomContext(true);
40-
}
41-
4231
CAddrInfo* Find(const CNetAddr& addr, int* pnId = nullptr)
4332
{
4433
LOCK(cs);

‎src/test/fuzz/addrman.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,8 @@ class CAddrManDeterministic : public CAddrMan
2929
FuzzedDataProvider& m_fuzzed_data_provider;
3030

3131
explicit CAddrManDeterministic(FuzzedDataProvider& fuzzed_data_provider)
32-
: m_fuzzed_data_provider(fuzzed_data_provider)
32+
: CAddrMan(/* deterministic */ true)
33+
, m_fuzzed_data_provider(fuzzed_data_provider)
3334
{
3435
WITH_LOCK(cs, insecure_rand = FastRandomContext{ConsumeUInt256(fuzzed_data_provider)});
3536
if (fuzzed_data_provider.ConsumeBool()) {

‎src/test/fuzz/connman.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ FUZZ_TARGET_INIT(connman, initialize_connman)
2525
{
2626
FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
2727
SetMockTime(ConsumeTime(fuzzed_data_provider));
28-
CAddrMan addrman;
28+
CAddrMan addrman(/* deterministic */ false);
2929
CConnman connman{fuzzed_data_provider.ConsumeIntegral<uint64_t>(), fuzzed_data_provider.ConsumeIntegral<uint64_t>(), addrman, fuzzed_data_provider.ConsumeBool()};
3030
CNetAddr random_netaddr;
3131
CNode random_node = ConsumeNode(fuzzed_data_provider);

‎src/test/fuzz/data_stream.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,6 @@ FUZZ_TARGET_INIT(data_stream_addr_man, initialize_data_stream_addr_man)
2121
{
2222
FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
2323
CDataStream data_stream = ConsumeDataStream(fuzzed_data_provider);
24-
CAddrMan addr_man;
24+
CAddrMan addr_man(/* deterministic */ false);
2525
CAddrDB::Read(addr_man, data_stream);
2626
}

‎src/test/fuzz/deserialize.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ FUZZ_TARGET_DESERIALIZE(blockmerkleroot, {
188188
BlockMerkleRoot(block, &mutated);
189189
})
190190
FUZZ_TARGET_DESERIALIZE(addrman_deserialize, {
191-
CAddrMan am;
191+
CAddrMan am(/* deterministic */ false);
192192
DeserializeFromFuzzingInput(buffer, am);
193193
})
194194
FUZZ_TARGET_DESERIALIZE(blockheader_deserialize, {

‎src/test/net_tests.cpp‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -34,13 +34,9 @@ class CAddrManSerializationMock : public CAddrMan
3434
public:
3535
virtual void Serialize(CDataStream& s) const = 0;
3636

37-
//! Ensure that bucket placement is always the same for testing purposes.
38-
void MakeDeterministic()
39-
{
40-
LOCK(cs);
41-
nKey.SetNull();
42-
insecure_rand = FastRandomContext(true);
43-
}
37+
CAddrManSerializationMock()
38+
: CAddrMan(/* deterministic */ true)
39+
{}
4440
};
4541

4642
class CAddrManUncorrupted : public CAddrManSerializationMock
@@ -105,7 +101,6 @@ BOOST_AUTO_TEST_CASE(cnode_listen_port)
105101
BOOST_AUTO_TEST_CASE(caddrdb_read)
106102
{
107103
CAddrManUncorrupted addrmanUncorrupted;
108-
addrmanUncorrupted.MakeDeterministic();
109104

110105
CService addr1, addr2, addr3;
111106
BOOST_CHECK(Lookup("250.7.1.1", addr1, 8333, false));
@@ -124,7 +119,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read)
124119
// Test that the de-serialization does not throw an exception.
125120
CDataStream ssPeers1 = AddrmanToStream(addrmanUncorrupted);
126121
bool exceptionThrown = false;
127-
CAddrMan addrman1;
122+
CAddrMan addrman1(/* deterministic */ false);
128123

129124
BOOST_CHECK(addrman1.size() == 0);
130125
try {
@@ -141,7 +136,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read)
141136
// Test that CAddrDB::Read creates an addrman with the correct number of addrs.
142137
CDataStream ssPeers2 = AddrmanToStream(addrmanUncorrupted);
143138

144-
CAddrMan addrman2;
139+
CAddrMan addrman2(/* deterministic */ false);
145140
BOOST_CHECK(addrman2.size() == 0);
146141
BOOST_CHECK(CAddrDB::Read(addrman2, ssPeers2));
147142
BOOST_CHECK(addrman2.size() == 3);
@@ -151,12 +146,11 @@ BOOST_AUTO_TEST_CASE(caddrdb_read)
151146
BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted)
152147
{
153148
CAddrManCorrupted addrmanCorrupted;
154-
addrmanCorrupted.MakeDeterministic();
155149

156150
// Test that the de-serialization of corrupted addrman throws an exception.
157151
CDataStream ssPeers1 = AddrmanToStream(addrmanCorrupted);
158152
bool exceptionThrown = false;
159-
CAddrMan addrman1;
153+
CAddrMan addrman1(/* deterministic */ false);
160154
BOOST_CHECK(addrman1.size() == 0);
161155
try {
162156
unsigned char pchMsgTmp[4];
@@ -172,7 +166,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted)
172166
// Test that CAddrDB::Read leaves addrman in a clean state if de-serialization fails.
173167
CDataStream ssPeers2 = AddrmanToStream(addrmanCorrupted);
174168

175-
CAddrMan addrman2;
169+
CAddrMan addrman2(/* deterministic */ false);
176170
BOOST_CHECK(addrman2.size() == 0);
177171
BOOST_CHECK(!CAddrDB::Read(addrman2, ssPeers2));
178172
BOOST_CHECK(addrman2.size() == 0);

‎src/test/util/setup_common.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
193193
throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", state.ToString()));
194194
}
195195

196-
m_node.addrman = std::make_unique<CAddrMan>();
196+
m_node.addrman = std::make_unique<CAddrMan>(/* deterministic */ false);
197197
m_node.banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
198198
m_node.connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests.
199199
m_node.peerman = PeerManager::make(chainparams, *m_node.connman, *m_node.addrman,

0 commit comments

Comments
 (0)