diff options
author | fanquake <fanquake@gmail.com> | 2021-08-13 16:39:01 +0800 |
---|---|---|
committer | fanquake <fanquake@gmail.com> | 2021-08-13 17:03:01 +0800 |
commit | 803ef70fd9f65ef800567ff9456fac525bc3e3c2 (patch) | |
tree | 1ea28e9f674fed66a0c0c99d5bfbacd29fc83244 /src/addrman.cpp | |
parent | 439e58c4d8194ca37f70346727d31f52e69592ec (diff) | |
parent | a4d78546b0858602c60c03fdf8b35ca666ab2e56 (diff) | |
download | bitcoin-803ef70fd9f65ef800567ff9456fac525bc3e3c2.tar.xz |
Merge bitcoin/bitcoin#20233: addrman: Make consistency checks a runtime option
a4d78546b0858602c60c03fdf8b35ca666ab2e56 [addrman] Make addrman consistency checks a runtime option (John Newbery)
10aac241455a3270462d49b53732477ed97623e7 [tests] Make deterministic addrman use nKey = 1 (John Newbery)
fa9710f62c29c7f8d71c9f281001c9b5e70946bf [addrman] Add deterministic argument to CAddrMan ctor (John Newbery)
ee458d84fc187d69f002ebead6fccc4f4f9c0744 Add missing const to CAddrMan::Check_() (MarcoFalke)
Pull request description:
CAddrMan has internal consistency checks. Currently, these are only run when the program is compiled with the `DEBUG_ADDRMAN` option. This option is not enabled on any of our CI builds, and it's likely that no-one is running them at all.
This PR makes consistency checks a (hidden) runtime option that can be enabled with `-checkaddrman`, where `-checkaddrman=n` will result in the consistency checks running every n operations (similar to `-checkmempool=n`). We set the ratio to 1/100 for our unit tests, and leave it disabled by default for all networks. Additionally, a consistency check failure now asserts, rather than logging and continuing. This matches the behavior of CTxMemPool and TxRequestTracker, where a failed consistency check asserts.
ACKs for top commit:
jonatack:
ACK a4d78546b0858602c60c03fdf8b35ca666ab2e56 per `git diff 00fd089 a4d7854`, tested by adding logging similar to #22479 and running with `-checkaddrman=<n>` for various values 0/1/10/100 etc, tested the updated docs with `bitcoind -help-debug | grep -A2 "checkaddrman\|checkmempool"` and verified rebased on master that compiling with `CPPFLAGS="-DDEBUG_ADDRMAN"` no longer causes the build to error.
mzumsande:
Code-review ACK a4d78546b0858602c60c03fdf8b35ca666ab2e56
theStack:
Code-review ACK a4d78546b0858602c60c03fdf8b35ca666ab2e56
Tree-SHA512: eaee003f7a99154822c5b5efbc62008d32c1efbecc6fec6e183427f6b2ae5d30b3be7924e3a7271b1a1de91517f5bd2a70011d45358c3105c6a0702f12b70f7c
Diffstat (limited to 'src/addrman.cpp')
-rw-r--r-- | src/addrman.cpp | 23 |
1 files changed, 16 insertions, 7 deletions
diff --git a/src/addrman.cpp b/src/addrman.cpp index 96139182d3..8e2fc67569 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -431,11 +431,14 @@ CAddrInfo CAddrMan::Select_(bool newOnly) const } } -#ifdef DEBUG_ADDRMAN -int CAddrMan::Check_() +int CAddrMan::Check_() const { 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; @@ -458,8 +461,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 +483,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 +500,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]); } @@ -509,7 +519,6 @@ int CAddrMan::Check_() return 0; } -#endif void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size_t max_pct, std::optional<Network> network) const { |