From ee458d84fc187d69f002ebead6fccc4f4f9c0744 Mon Sep 17 00:00:00 2001
From: MarcoFalke <falke.marco@gmail.com>
Date: Mon, 2 Aug 2021 13:42:42 +0200
Subject: Add missing const to CAddrMan::Check_()
Also: Always compile the function signature to avoid similar issues in
the future.
---
src/addrman.cpp | 21 ++++++++++++++-------
src/addrman.h | 4 ----
2 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/src/addrman.cpp b/src/addrman.cpp
index 96139182d3..c5c6dfbb86 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -431,9 +431,9 @@ CAddrInfo CAddrMan::Select_(bool newOnly) const
}
}
-#ifdef DEBUG_ADDRMAN
-int CAddrMan::Check_()
+int CAddrMan::Check_() const
{
+#ifdef DEBUG_ADDRMAN
AssertLockHeld(cs);
std::unordered_set<int> setTried;
@@ -458,8 +458,10 @@ int CAddrMan::Check_()
return -4;
mapNew[n] = info.nRefCount;
}
- if (mapAddr[info] != n)
+ const auto it{mapAddr.find(info)};
+ if (it == mapAddr.end() || it->second != n) {
return -5;
+ }
if (info.nRandomPos < 0 || (size_t)info.nRandomPos >= vRandom.size() || vRandom[info.nRandomPos] != n)
return -14;
if (info.nLastTry < 0)
@@ -478,10 +480,13 @@ int CAddrMan::Check_()
if (vvTried[n][i] != -1) {
if (!setTried.count(vvTried[n][i]))
return -11;
- if (mapInfo[vvTried[n][i]].GetTriedBucket(nKey, m_asmap) != n)
+ const auto it{mapInfo.find(vvTried[n][i])};
+ if (it == mapInfo.end() || it->second.GetTriedBucket(nKey, m_asmap) != n) {
return -17;
- if (mapInfo[vvTried[n][i]].GetBucketPosition(nKey, false, n) != i)
+ }
+ if (it->second.GetBucketPosition(nKey, false, n) != i) {
return -18;
+ }
setTried.erase(vvTried[n][i]);
}
}
@@ -492,8 +497,10 @@ int CAddrMan::Check_()
if (vvNew[n][i] != -1) {
if (!mapNew.count(vvNew[n][i]))
return -12;
- if (mapInfo[vvNew[n][i]].GetBucketPosition(nKey, true, n) != i)
+ const auto it{mapInfo.find(vvNew[n][i])};
+ if (it == mapInfo.end() || it->second.GetBucketPosition(nKey, true, n) != i) {
return -19;
+ }
if (--mapNew[vvNew[n][i]] == 0)
mapNew.erase(vvNew[n][i]);
}
@@ -507,9 +514,9 @@ int CAddrMan::Check_()
if (nKey.IsNull())
return -16;
+#endif // DEBUG_ADDRMAN
return 0;
}
-#endif
void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size_t max_pct, std::optional<Network> network) const
{
diff --git a/src/addrman.h b/src/addrman.h
index 1dd1932421..16be374f7b 100644
--- a/src/addrman.h
+++ b/src/addrman.h
@@ -738,19 +738,15 @@ private:
void Check() const
EXCLUSIVE_LOCKS_REQUIRED(cs)
{
-#ifdef DEBUG_ADDRMAN
AssertLockHeld(cs);
const int err = Check_();
if (err) {
LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err);
}
-#endif
}
-#ifdef DEBUG_ADDRMAN
//! Perform consistency check. Returns an error code or zero.
int Check_() const EXCLUSIVE_LOCKS_REQUIRED(cs);
-#endif
/**
* Return all or many randomly selected addresses, optionally by network.
--
cgit v1.2.3
From fa9710f62c29c7f8d71c9f281001c9b5e70946bf Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
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(-)
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<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;
});
}
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<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);
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<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);
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<uint64_t>(), fuzzed_data_provider.ConsumeIntegral<uint64_t>(), 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<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,
--
cgit v1.2.3
From 10aac241455a3270462d49b53732477ed97623e7 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 19 Jul 2021 10:45:32 +0100
Subject: [tests] Make deterministic addrman use nKey = 1
addrman_tests fail when consistency checks are enabled, since the tests
set the deterministic test addrman's nKey value to zero, which is an
invalid value. Change this so that deterministic addrman's nKey value is
set to 1.
This requires updating a few tests that are using magic values derived
from nKey being set to 0.
---
src/addrman.h | 2 +-
src/test/addrman_tests.cpp | 96 ++++++++++++++++++++++++----------------------
2 files changed, 52 insertions(+), 46 deletions(-)
diff --git a/src/addrman.h b/src/addrman.h
index 2503f53a35..2c6ffbe523 100644
--- a/src/addrman.h
+++ b/src/addrman.h
@@ -497,7 +497,7 @@ public:
: insecure_rand{deterministic}
{
Clear();
- if (deterministic) nKey.SetNull();
+ if (deterministic) nKey = uint256{1};
}
~CAddrMan()
diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
index 0ab6cd0a2b..b7da1c8896 100644
--- a/src/test/addrman_tests.cpp
+++ b/src/test/addrman_tests.cpp
@@ -78,7 +78,7 @@ public:
CAddrMan::Clear();
if (deterministic) {
LOCK(cs);
- nKey.SetNull();
+ nKey = uint256{1};
insecure_rand = FastRandomContext(true);
}
}
@@ -256,24 +256,27 @@ BOOST_AUTO_TEST_CASE(addrman_new_collisions)
CNetAddr source = ResolveIP("252.2.2.2");
- BOOST_CHECK_EQUAL(addrman.size(), 0U);
+ uint32_t num_addrs{0};
+
+ BOOST_CHECK_EQUAL(addrman.size(), num_addrs);
- for (unsigned int i = 1; i < 18; i++) {
- CService addr = ResolveService("250.1.1." + ToString(i));
+ while (num_addrs < 22) { // Magic number! 250.1.1.1 - 250.1.1.22 do not collide with deterministic key = 1
+ CService addr = ResolveService("250.1.1." + ToString(++num_addrs));
BOOST_CHECK(addrman.Add(CAddress(addr, NODE_NONE), source));
//Test: No collision in new table yet.
- BOOST_CHECK_EQUAL(addrman.size(), i);
+ BOOST_CHECK_EQUAL(addrman.size(), num_addrs);
}
//Test: new table collision!
- CService addr1 = ResolveService("250.1.1.18");
+ CService addr1 = ResolveService("250.1.1." + ToString(++num_addrs));
+ uint32_t collisions{1};
BOOST_CHECK(addrman.Add(CAddress(addr1, NODE_NONE), source));
- BOOST_CHECK_EQUAL(addrman.size(), 17U);
+ BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions);
- CService addr2 = ResolveService("250.1.1.19");
+ CService addr2 = ResolveService("250.1.1." + ToString(++num_addrs));
BOOST_CHECK(addrman.Add(CAddress(addr2, NODE_NONE), source));
- BOOST_CHECK_EQUAL(addrman.size(), 18U);
+ BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions);
}
BOOST_AUTO_TEST_CASE(addrman_tried_collisions)
@@ -282,25 +285,28 @@ BOOST_AUTO_TEST_CASE(addrman_tried_collisions)
CNetAddr source = ResolveIP("252.2.2.2");
- BOOST_CHECK_EQUAL(addrman.size(), 0U);
+ uint32_t num_addrs{0};
+
+ BOOST_CHECK_EQUAL(addrman.size(), num_addrs);
- for (unsigned int i = 1; i < 80; i++) {
- CService addr = ResolveService("250.1.1." + ToString(i));
+ while (num_addrs < 64) { // Magic number! 250.1.1.1 - 250.1.1.64 do not collide with deterministic key = 1
+ CService addr = ResolveService("250.1.1." + ToString(++num_addrs));
BOOST_CHECK(addrman.Add(CAddress(addr, NODE_NONE), source));
addrman.Good(CAddress(addr, NODE_NONE));
//Test: No collision in tried table yet.
- BOOST_CHECK_EQUAL(addrman.size(), i);
+ BOOST_CHECK_EQUAL(addrman.size(), num_addrs);
}
//Test: tried table collision!
- CService addr1 = ResolveService("250.1.1.80");
+ CService addr1 = ResolveService("250.1.1." + ToString(++num_addrs));
+ uint32_t collisions{1};
BOOST_CHECK(addrman.Add(CAddress(addr1, NODE_NONE), source));
- BOOST_CHECK_EQUAL(addrman.size(), 79U);
+ BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions);
- CService addr2 = ResolveService("250.1.1.81");
+ CService addr2 = ResolveService("250.1.1." + ToString(++num_addrs));
BOOST_CHECK(addrman.Add(CAddress(addr2, NODE_NONE), source));
- BOOST_CHECK_EQUAL(addrman.size(), 80U);
+ BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions);
}
BOOST_AUTO_TEST_CASE(addrman_find)
@@ -850,9 +856,9 @@ BOOST_AUTO_TEST_CASE(addrman_noevict)
{
CAddrManTest addrman;
- // Add twenty two addresses.
+ // Add 35 addresses.
CNetAddr source = ResolveIP("252.2.2.2");
- for (unsigned int i = 1; i < 23; i++) {
+ for (unsigned int i = 1; i < 36; i++) {
CService addr = ResolveService("250.1.1."+ToString(i));
BOOST_CHECK(addrman.Add(CAddress(addr, NODE_NONE), source));
addrman.Good(addr);
@@ -862,20 +868,20 @@ BOOST_AUTO_TEST_CASE(addrman_noevict)
BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
}
- // Collision between 23 and 19.
- CService addr23 = ResolveService("250.1.1.23");
- BOOST_CHECK(addrman.Add(CAddress(addr23, NODE_NONE), source));
- addrman.Good(addr23);
+ // Collision between 36 and 19.
+ CService addr36 = ResolveService("250.1.1.36");
+ BOOST_CHECK(addrman.Add(CAddress(addr36, NODE_NONE), source));
+ addrman.Good(addr36);
- BOOST_CHECK(addrman.size() == 23);
- BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "250.1.1.19:0");
+ BOOST_CHECK(addrman.size() == 36);
+ BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().ToString(), "250.1.1.19:0");
- // 23 should be discarded and 19 not evicted.
+ // 36 should be discarded and 19 not evicted.
addrman.ResolveCollisions();
BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
// Lets create two collisions.
- for (unsigned int i = 24; i < 33; i++) {
+ for (unsigned int i = 37; i < 59; i++) {
CService addr = ResolveService("250.1.1."+ToString(i));
BOOST_CHECK(addrman.Add(CAddress(addr, NODE_NONE), source));
addrman.Good(addr);
@@ -885,17 +891,17 @@ BOOST_AUTO_TEST_CASE(addrman_noevict)
}
// Cause a collision.
- CService addr33 = ResolveService("250.1.1.33");
- BOOST_CHECK(addrman.Add(CAddress(addr33, NODE_NONE), source));
- addrman.Good(addr33);
- BOOST_CHECK(addrman.size() == 33);
+ CService addr59 = ResolveService("250.1.1.59");
+ BOOST_CHECK(addrman.Add(CAddress(addr59, NODE_NONE), source));
+ addrman.Good(addr59);
+ BOOST_CHECK(addrman.size() == 59);
- BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "250.1.1.27:0");
+ BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().ToString(), "250.1.1.10:0");
// Cause a second collision.
- BOOST_CHECK(!addrman.Add(CAddress(addr23, NODE_NONE), source));
- addrman.Good(addr23);
- BOOST_CHECK(addrman.size() == 33);
+ BOOST_CHECK(!addrman.Add(CAddress(addr36, NODE_NONE), source));
+ addrman.Good(addr36);
+ BOOST_CHECK(addrman.size() == 59);
BOOST_CHECK(addrman.SelectTriedCollision().ToString() != "[::]:0");
addrman.ResolveCollisions();
@@ -911,9 +917,9 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks)
// Empty addrman should return blank addrman info.
BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
- // Add twenty two addresses.
+ // Add 35 addresses
CNetAddr source = ResolveIP("252.2.2.2");
- for (unsigned int i = 1; i < 23; i++) {
+ for (unsigned int i = 1; i < 36; i++) {
CService addr = ResolveService("250.1.1."+ToString(i));
BOOST_CHECK(addrman.Add(CAddress(addr, NODE_NONE), source));
addrman.Good(addr);
@@ -923,34 +929,34 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks)
BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
}
- // Collision between 23 and 19.
- CService addr = ResolveService("250.1.1.23");
+ // Collision between 36 and 19.
+ CService addr = ResolveService("250.1.1.36");
BOOST_CHECK(addrman.Add(CAddress(addr, NODE_NONE), source));
addrman.Good(addr);
- BOOST_CHECK(addrman.size() == 23);
+ BOOST_CHECK_EQUAL(addrman.size(), 36);
CAddrInfo info = addrman.SelectTriedCollision();
- BOOST_CHECK(info.ToString() == "250.1.1.19:0");
+ BOOST_CHECK_EQUAL(info.ToString(), "250.1.1.19:0");
// Ensure test of address fails, so that it is evicted.
addrman.SimConnFail(info);
- // Should swap 23 for 19.
+ // Should swap 36 for 19.
addrman.ResolveCollisions();
BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
- // If 23 was swapped for 19, then this should cause no collisions.
+ // If 36 was swapped for 19, then this should cause no collisions.
BOOST_CHECK(!addrman.Add(CAddress(addr, NODE_NONE), source));
addrman.Good(addr);
BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
- // If we insert 19 is should collide with 23.
+ // If we insert 19 it should collide with 36
CService addr19 = ResolveService("250.1.1.19");
BOOST_CHECK(!addrman.Add(CAddress(addr19, NODE_NONE), source));
addrman.Good(addr19);
- BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "250.1.1.23:0");
+ BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().ToString(), "250.1.1.36:0");
addrman.ResolveCollisions();
BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
--
cgit v1.2.3
From a4d78546b0858602c60c03fdf8b35ca666ab2e56 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Fri, 23 Oct 2020 22:03:24 +0100
Subject: [addrman] Make addrman consistency checks a runtime option
Currently addrman consistency checks are a compile time option, and are not
enabled in our CI. It's unlikely anyone is running these consistency checks.
Make them a runtime option instead, where users can enable addrman
consistency checks every n operations (similar to mempool tests). Update
the addrman unit tests to do internal consistency checks every 100
operations (checking on every operations causes the test runtime to
increase by several seconds).
Also assert on a failed addrman consistency check to terminate program
execution.
---
src/addrman.cpp | 6 ++++--
src/addrman.h | 20 ++++++++++++++------
src/bench/addrman.cpp | 8 ++++----
src/init.cpp | 6 ++++--
src/test/addrman_tests.cpp | 2 +-
src/test/fuzz/addrman.cpp | 2 +-
src/test/fuzz/connman.cpp | 2 +-
src/test/fuzz/data_stream.cpp | 2 +-
src/test/fuzz/deserialize.cpp | 2 +-
src/test/net_tests.cpp | 10 +++++-----
src/test/util/setup_common.cpp | 2 +-
11 files changed, 37 insertions(+), 25 deletions(-)
diff --git a/src/addrman.cpp b/src/addrman.cpp
index c5c6dfbb86..8e2fc67569 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -433,9 +433,12 @@ CAddrInfo CAddrMan::Select_(bool newOnly) const
int CAddrMan::Check_() const
{
-#ifdef DEBUG_ADDRMAN
AssertLockHeld(cs);
+ // Run consistency checks 1 in m_consistency_check_ratio times if enabled
+ if (m_consistency_check_ratio == 0) return 0;
+ if (insecure_rand.randrange(m_consistency_check_ratio) >= 1) return 0;
+
std::unordered_set<int> setTried;
std::unordered_map<int, int> mapNew;
@@ -514,7 +517,6 @@ int CAddrMan::Check_() const
if (nKey.IsNull())
return -16;
-#endif // DEBUG_ADDRMAN
return 0;
}
diff --git a/src/addrman.h b/src/addrman.h
index 2c6ffbe523..3ee8c3ee09 100644
--- a/src/addrman.h
+++ b/src/addrman.h
@@ -26,6 +26,9 @@
#include <unordered_map>
#include <vector>
+/** Default for -checkaddrman */
+static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{0};
+
/**
* Extended statistics about a CAddress
*/
@@ -124,8 +127,8 @@ public:
* attempt was unsuccessful.
* * Bucket selection is based on cryptographic hashing, using a randomly-generated 256-bit key, which should not
* be observable by adversaries.
- * * Several indexes are kept for high performance. Defining DEBUG_ADDRMAN will introduce frequent (and expensive)
- * consistency checks for the entire data structure.
+ * * Several indexes are kept for high performance. Setting m_consistency_check_ratio with the -checkaddrman
+ * configuration option will introduce (expensive) consistency checks for the entire data structure.
*/
//! total number of buckets for tried addresses
@@ -493,8 +496,9 @@ public:
mapAddr.clear();
}
- explicit CAddrMan(bool deterministic)
- : insecure_rand{deterministic}
+ 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};
@@ -700,6 +704,9 @@ private:
//! Holds addrs inserted into tried table that collide with existing entries. Test-before-evict discipline used to resolve these collisions.
std::set<int> m_tried_collisions;
+ /** Perform consistency checks every m_consistency_check_ratio operations (if non-zero). */
+ const int32_t m_consistency_check_ratio;
+
//! Find an entry.
CAddrInfo* Find(const CNetAddr& addr, int *pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs);
@@ -737,13 +744,14 @@ private:
CAddrInfo SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs);
//! Consistency check
- void Check() const
- EXCLUSIVE_LOCKS_REQUIRED(cs)
+ void Check() const EXCLUSIVE_LOCKS_REQUIRED(cs)
{
AssertLockHeld(cs);
+
const int err = Check_();
if (err) {
LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err);
+ assert(false);
}
}
diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp
index e1175e44ec..5ae2dafd5a 100644
--- a/src/bench/addrman.cpp
+++ b/src/bench/addrman.cpp
@@ -72,7 +72,7 @@ static void AddrManAdd(benchmark::Bench& bench)
{
CreateAddresses();
- CAddrMan addrman(/* deterministic */ false);
+ CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0);
bench.run([&] {
AddAddressesToAddrMan(addrman);
@@ -82,7 +82,7 @@ static void AddrManAdd(benchmark::Bench& bench)
static void AddrManSelect(benchmark::Bench& bench)
{
- CAddrMan addrman(/* deterministic */ false);
+ CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0);
FillAddrMan(addrman);
@@ -94,7 +94,7 @@ static void AddrManSelect(benchmark::Bench& bench)
static void AddrManGetAddr(benchmark::Bench& bench)
{
- CAddrMan addrman(/* deterministic */ false);
+ CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0);
FillAddrMan(addrman);
@@ -116,7 +116,7 @@ static void AddrManGood(benchmark::Bench& bench)
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);
+ addrmans[i] = std::make_unique<CAddrMan>(/* deterministic */ false, /* consistency_check_ratio */ 0);
FillAddrMan(*addrmans[i]);
}
diff --git a/src/init.cpp b/src/init.cpp
index 2250834111..9154fc0a6f 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -501,7 +501,8 @@ void SetupServerArgs(ArgsManager& argsman)
argsman.AddArg("-checkblocks=<n>", strprintf("How many blocks to check at startup (default: %u, 0 = all)", DEFAULT_CHECKBLOCKS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
argsman.AddArg("-checklevel=<n>", strprintf("How thorough the block verification of -checkblocks is: %s (0-4, default: %u)", Join(CHECKLEVEL_DOC, ", "), DEFAULT_CHECKLEVEL), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
argsman.AddArg("-checkblockindex", strprintf("Do a consistency check for the block tree, chainstate, and other validation data structures occasionally. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
- argsman.AddArg("-checkmempool=<n>", strprintf("Run checks every <n> transactions (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
+ argsman.AddArg("-checkaddrman=<n>", strprintf("Run addrman consistency checks every <n> operations. Use 0 to disable. (default: %u)", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
+ argsman.AddArg("-checkmempool=<n>", strprintf("Run mempool consistency checks every <n> transactions. Use 0 to disable. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
argsman.AddArg("-checkpoints", strprintf("Enable rejection of any forks from the known historical chain until block %s (default: %u)", defaultChainParams->Checkpoints().GetHeight(), DEFAULT_CHECKPOINTS_ENABLED), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
argsman.AddArg("-deprecatedrpc=<method>", "Allows deprecated RPC method(s) to be used", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
argsman.AddArg("-stopafterblockimport", strprintf("Stop running after importing blocks from disk (default: %u)", DEFAULT_STOPAFTERBLOCKIMPORT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
@@ -1164,7 +1165,8 @@ 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>(/* deterministic */ false);
+ auto check_addrman = std::clamp<int32_t>(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000);
+ node.addrman = std::make_unique<CAddrMan>(/* deterministic */ false, /* consistency_check_ratio */ check_addrman);
assert(!node.banman);
node.banman = std::make_unique<BanMan>(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 b7da1c8896..3c64461605 100644
--- a/src/test/addrman_tests.cpp
+++ b/src/test/addrman_tests.cpp
@@ -22,7 +22,7 @@ private:
public:
explicit CAddrManTest(bool makeDeterministic = true,
std::vector<bool> asmap = std::vector<bool>())
- : CAddrMan(makeDeterministic)
+ : CAddrMan(makeDeterministic, /* consistency_check_ratio */ 100)
{
deterministic = makeDeterministic;
m_asmap = asmap;
diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp
index b5e946c528..60fba5730a 100644
--- a/src/test/fuzz/addrman.cpp
+++ b/src/test/fuzz/addrman.cpp
@@ -29,7 +29,7 @@ public:
FuzzedDataProvider& m_fuzzed_data_provider;
explicit CAddrManDeterministic(FuzzedDataProvider& fuzzed_data_provider)
- : CAddrMan(/* deterministic */ true)
+ : CAddrMan(/* deterministic */ true, /* consistency_check_ratio */ 0)
, m_fuzzed_data_provider(fuzzed_data_provider)
{
WITH_LOCK(cs, insecure_rand = FastRandomContext{ConsumeUInt256(fuzzed_data_provider)});
diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp
index a32cb54465..0e323ddc20 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(/* deterministic */ false);
+ CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0);
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);
diff --git a/src/test/fuzz/data_stream.cpp b/src/test/fuzz/data_stream.cpp
index 63bc4b2afe..53400082ab 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(/* deterministic */ false);
+ CAddrMan addr_man(/* deterministic */ false, /* consistency_check_ratio */ 0);
CAddrDB::Read(addr_man, data_stream);
}
diff --git a/src/test/fuzz/deserialize.cpp b/src/test/fuzz/deserialize.cpp
index 6ab3389123..49503e8dc6 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(/* deterministic */ false);
+ CAddrMan am(/* deterministic */ false, /* consistency_check_ratio */ 0);
DeserializeFromFuzzingInput(buffer, am);
})
FUZZ_TARGET_DESERIALIZE(blockheader_deserialize, {
diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index ff9e9b7a07..1915f9c7d5 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -35,7 +35,7 @@ public:
virtual void Serialize(CDataStream& s) const = 0;
CAddrManSerializationMock()
- : CAddrMan(/* deterministic */ true)
+ : CAddrMan(/* deterministic */ true, /* consistency_check_ratio */ 100)
{}
};
@@ -119,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(/* deterministic */ false);
+ CAddrMan addrman1(/* deterministic */ false, /* consistency_check_ratio */ 100);
BOOST_CHECK(addrman1.size() == 0);
try {
@@ -136,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(/* deterministic */ false);
+ CAddrMan addrman2(/* deterministic */ false, /* consistency_check_ratio */ 100);
BOOST_CHECK(addrman2.size() == 0);
BOOST_CHECK(CAddrDB::Read(addrman2, ssPeers2));
BOOST_CHECK(addrman2.size() == 3);
@@ -150,7 +150,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted)
// Test that the de-serialization of corrupted addrman throws an exception.
CDataStream ssPeers1 = AddrmanToStream(addrmanCorrupted);
bool exceptionThrown = false;
- CAddrMan addrman1(/* deterministic */ false);
+ CAddrMan addrman1(/* deterministic */ false, /* consistency_check_ratio */ 100);
BOOST_CHECK(addrman1.size() == 0);
try {
unsigned char pchMsgTmp[4];
@@ -166,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(/* deterministic */ false);
+ CAddrMan addrman2(/* deterministic */ false, /* consistency_check_ratio */ 100);
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 b1567d924b..a58f4ba25a 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<const
throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", state.ToString()));
}
- m_node.addrman = std::make_unique<CAddrMan>(/* deterministic */ false);
+ m_node.addrman = std::make_unique<CAddrMan>(/* deterministic */ false, /* consistency_check_ratio */ 0);
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,
--
cgit v1.2.3