Skip to content

Commit 49af818

Browse files
kwvgkwvg
committed
merge bitcoin#20233: Make consistency checks a runtime option
1 parent 44f237a commit 49af818

File tree

12 files changed

+117
-108
lines changed

12 files changed

+117
-108
lines changed

‎src/addrman.cpp‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -452,12 +452,15 @@ CAddrInfo CAddrMan::Select_(bool newOnly) const
452452
}
453453
}
454454

455-
#ifdef DEBUG_ADDRMAN
456-
int CAddrMan::Check_()
455+
int CAddrMan::Check_() const
457456
{
458457
AssertLockHeld(cs);
459458
LogPrint(BCLog::ADDRMAN, "Addrman checks started: new %i, tried %i, total %u\n", nNew, nTried, vRandom.size());
460459

460+
// Run consistency checks 1 in m_consistency_check_ratio times if enabled
461+
if (m_consistency_check_ratio == 0) return 0;
462+
if (insecure_rand.randrange(m_consistency_check_ratio) >= 1) return 0;
463+
461464
std::unordered_set<int> setTried;
462465
std::unordered_map<int, int> mapNew;
463466

@@ -480,8 +483,10 @@ int CAddrMan::Check_()
480483
return -4;
481484
mapNew[n] = info.nRefCount;
482485
}
483-
if (mapAddr[info] != n)
486+
const auto it{mapAddr.find(info)};
487+
if (it == mapAddr.end() || it->second != n) {
484488
return -5;
489+
}
485490
if (info.nRandomPos < 0 || (size_t)info.nRandomPos >= vRandom.size() || vRandom[info.nRandomPos] != n)
486491
return -14;
487492
if (info.nLastTry < 0)
@@ -500,10 +505,13 @@ int CAddrMan::Check_()
500505
if (vvTried[n][i] != -1) {
501506
if (!setTried.count(vvTried[n][i]))
502507
return -11;
503-
if (mapInfo[vvTried[n][i]].GetTriedBucket(nKey, m_asmap) != n)
508+
const auto it{mapInfo.find(vvTried[n][i])};
509+
if (it == mapInfo.end() || it->second.GetTriedBucket(nKey, m_asmap) != n) {
504510
return -17;
505-
if (mapInfo[vvTried[n][i]].GetBucketPosition(nKey, false, n) != i)
511+
}
512+
if (it->second.GetBucketPosition(nKey, false, n) != i) {
506513
return -18;
514+
}
507515
setTried.erase(vvTried[n][i]);
508516
}
509517
}
@@ -514,8 +522,10 @@ int CAddrMan::Check_()
514522
if (vvNew[n][i] != -1) {
515523
if (!mapNew.count(vvNew[n][i]))
516524
return -12;
517-
if (mapInfo[vvNew[n][i]].GetBucketPosition(nKey, true, n) != i)
525+
const auto it{mapInfo.find(vvNew[n][i])};
526+
if (it == mapInfo.end() || it->second.GetBucketPosition(nKey, true, n) != i) {
518527
return -19;
528+
}
519529
if (--mapNew[vvNew[n][i]] == 0)
520530
mapNew.erase(vvNew[n][i]);
521531
}
@@ -532,7 +542,6 @@ int CAddrMan::Check_()
532542
LogPrint(BCLog::ADDRMAN, "Addrman checks completed successfully\n");
533543
return 0;
534544
}
535-
#endif
536545

537546
void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size_t max_pct, std::optional<Network> network) const
538547
{

‎src/addrman.h‎

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@
2626
#include <unordered_map>
2727
#include <vector>
2828

29+
/** Default for -checkaddrman */
30+
static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{0};
31+
2932
/**
3033
* Extended statistics about a CAddress
3134
*/
@@ -124,8 +127,8 @@ class CAddrInfo : public CAddress
124127
* attempt was unsuccessful.
125128
* * Bucket selection is based on cryptographic hashing, using a randomly-generated 256-bit key, which should not
126129
* be observable by adversaries.
127-
* * Several indexes are kept for high performance. Defining DEBUG_ADDRMAN will introduce frequent (and expensive)
128-
* consistency checks for the entire data structure.
130+
* * Several indexes are kept for high performance. Setting m_consistency_check_ratio with the -checkaddrman
131+
* configuration option will introduce (expensive) consistency checks for the entire data structure.
129132
*/
130133

131134
//! total number of buckets for tried addresses
@@ -495,10 +498,13 @@ class CAddrMan
495498
mapAddr.clear();
496499
}
497500

498-
CAddrMan(bool _discriminatePorts = false) :
501+
explicit CAddrMan(bool deterministic, int32_t consistency_check_ratio, bool _discriminatePorts = false) :
502+
insecure_rand{deterministic},
503+
m_consistency_check_ratio{consistency_check_ratio},
499504
discriminatePorts(_discriminatePorts)
500505
{
501506
Clear();
507+
if (deterministic) nKey = uint256{1};
502508
}
503509

504510
~CAddrMan()
@@ -651,13 +657,13 @@ class CAddrMan
651657
//! secret key to randomize bucket select with
652658
uint256 nKey;
653659

654-
//! Source of random numbers for randomization in inner loops
655-
mutable FastRandomContext insecure_rand GUARDED_BY(cs);
656-
657660
//! A mutex to protect the inner data structures.
658661
mutable Mutex cs;
659662

660663
private:
664+
//! Source of random numbers for randomization in inner loops
665+
mutable FastRandomContext insecure_rand GUARDED_BY(cs);
666+
661667
//! Serialization versions.
662668
enum Format : uint8_t {
663669
V0_HISTORICAL = 0, //!< historic format, before commit e6b343d88
@@ -709,12 +715,15 @@ class CAddrMan
709715
//! last time Good was called (memory only)
710716
int64_t nLastGood GUARDED_BY(cs);
711717

712-
// discriminate entries based on port. Should be false on mainnet/testnet and can be true on devnet/regtest
713-
bool discriminatePorts GUARDED_BY(cs);
714-
715718
//! Holds addrs inserted into tried table that collide with existing entries. Test-before-evict discipline used to resolve these collisions.
716719
std::set<int> m_tried_collisions;
717720

721+
/** Perform consistency checks every m_consistency_check_ratio operations (if non-zero). */
722+
const int32_t m_consistency_check_ratio;
723+
724+
// discriminate entries based on port. Should be false on mainnet/testnet and can be true on devnet/regtest
725+
bool discriminatePorts GUARDED_BY(cs);
726+
718727
//! Find an entry.
719728
CAddrInfo* Find(const CService& addr, int *pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs);
720729

@@ -752,22 +761,19 @@ class CAddrMan
752761
CAddrInfo SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs);
753762

754763
//! Consistency check
755-
void Check() const
756-
EXCLUSIVE_LOCKS_REQUIRED(cs)
764+
void Check() const EXCLUSIVE_LOCKS_REQUIRED(cs)
757765
{
758-
#ifdef DEBUG_ADDRMAN
759766
AssertLockHeld(cs);
767+
760768
const int err = Check_();
761769
if (err) {
762770
LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err);
771+
assert(false);
763772
}
764-
#endif
765773
}
766774

767-
#ifdef DEBUG_ADDRMAN
768775
//! Perform consistency check. Returns an error code or zero.
769776
int Check_() const EXCLUSIVE_LOCKS_REQUIRED(cs);
770-
#endif
771777

772778
/**
773779
* Return all or many randomly selected addresses, optionally by network.

‎src/bench/addrman.cpp‎

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

74-
CAddrMan addrman;
74+
CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0);
7575

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

8282
static void AddrManSelect(benchmark::Bench& bench)
8383
{
84-
CAddrMan addrman;
84+
CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0);
8585

8686
FillAddrMan(addrman);
8787

@@ -93,7 +93,7 @@ static void AddrManSelect(benchmark::Bench& bench)
9393

9494
static void AddrManGetAddr(benchmark::Bench& bench)
9595
{
96-
CAddrMan addrman;
96+
CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0);
9797

9898
FillAddrMan(addrman);
9999

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

113113
bench.epochs(5).epochIterations(1);
114+
const size_t addrman_count{bench.epochs() * bench.epochIterations()};
114115

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

120122
auto markSomeAsGood = [](CAddrMan& addrman) {
@@ -129,7 +131,7 @@ static void AddrManGood(benchmark::Bench& bench)
129131

130132
uint64_t i = 0;
131133
bench.run([&] {
132-
markSomeAsGood(addrmans.at(i));
134+
markSomeAsGood(*addrmans.at(i));
133135
++i;
134136
});
135137
}

‎src/init.cpp‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -701,7 +701,8 @@ void SetupServerArgs(NodeContext& node)
701701
argsman.AddArg("-checkblockindex", strprintf("Do a consistency check for the block tree, and occasionally. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
702702
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);
703703
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);
704-
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);
704+
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);
705+
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);
705706
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);
706707
argsman.AddArg("-deprecatedrpc=<method>", "Allows deprecated RPC method(s) to be used", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
707708
argsman.AddArg("-limitancestorcount=<n>", strprintf("Do not accept transactions if number of in-mempool ancestors is <n> or more (default: %u)", DEFAULT_ANCESTOR_LIMIT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
@@ -1663,7 +1664,8 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc
16631664
const bool ignores_incoming_txs{args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)};
16641665

16651666
assert(!node.addrman);
1666-
node.addrman = std::make_unique<CAddrMan>();
1667+
auto check_addrman = std::clamp<int32_t>(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000);
1668+
node.addrman = std::make_unique<CAddrMan>(/* deterministic */ false, /* consistency_check_ratio */ check_addrman);
16671669
assert(!node.banman);
16681670
node.banman = std::make_unique<BanMan>(GetDataDir() / "banlist", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
16691671
assert(!node.connman);

0 commit comments

Comments
 (0)