Just writing to a log will be easily missed.
The same is done in CTxMemPool::check() and other debug checks for developers, like lock order debugging.
Can be reviewed with https://en.wikipedia.org/wiki/De_Morgan%27s_laws and
git difftool --tool=meld cccc47b0ef~ cccc47b0ef
The same is done in CTxMemPool::check() and other debug checks for
developers, like lock order debugging.
Approach ACK
525 | - return -18; 526 | - setTried.erase(vvTried[n][i]); 527 | + assert(setTried.count(vvTried[n][i])); 528 | + assert(mapInfo[vvTried[n][i]].GetTriedBucket(nKey, m_asmap) == n); 529 | + assert(mapInfo[vvTried[n][i]].GetBucketPosition(nKey, false, n) == i); 530 | + setTried.erase(vvTried[n][i]);
nit, spacing here is off by one
735 | - 736 | -#ifdef DEBUG_ADDRMAN 737 | - //! Perform consistency check. Returns an error code or zero. 738 | - int Check_() EXCLUSIVE_LOCKS_REQUIRED(cs); 739 | -#endif 740 | + EXCLUSIVE_LOCKS_REQUIRED(cs);
//! Consistency check
- void Check()
- EXCLUSIVE_LOCKS_REQUIRED(cs);
+ void Check() EXCLUSIVE_LOCKS_REQUIRED(cs);
ACK cccc47b0ef5af66c8e6349d7cef60b902b80bc09
Tested with the following changes
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -463,6 +463,7 @@ void CAddrMan::Check()
{
#ifdef DEBUG_ADDRMAN
AssertLockHeld(cs);
+ LogPrint(BCLog::ADDRMAN, "Check addrman\n");
--- a/src/addrman.h
+++ b/src/addrman.h
@@ -122,6 +122,7 @@ public:
* * Several indexes are kept for high performance. Defining DEBUG_ADDRMAN will introduce frequent (and expensive)
* consistency checks for the entire data structure.
*/
+#define DEBUG_ADDRMAN
//! total number of buckets for tried addresses
#define ADDRMAN_TRIED_BUCKET_COUNT_LOG2 8
Requested in https://github.com/bitcoin/bitcoin/pull/22500#discussion_r672440473
Code Review and tested ACK https://github.com/bitcoin/bitcoin/pull/22500/commits/fa4bfef4798466e986569baf0d8e3213f17cafc8 on Ubuntu 20.04. The change simplifies the CAddrMan::Check() code.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Concept and code review ACK fa4bfef4798466e986569baf0d8e3213f17cafc8
Also built locally with #define DEBUG_ADDRMAN set in src/addrman.cpp (the CI result for this PR obviously doesn't have any value, as the modified method CAddrMan::Check() is empty after the pre-processor run)
re-ACK fa4bfef4798466e986569baf0d8e3213f17cafc8 have been testing this rebased to current master with DEBUG_ADDRMAN defined
could squash
If DEBUG_ADDRMAN is set, the following error will occur when addrman_tests is run:
$ src/test/test_bitcoin --run_test=addrman_tests --log_level=unit_scope
Running 18 test cases...
Entering test module "Bitcoin Core Test Suite"
test/addrman_tests.cpp(124): Entering test suite "addrman_tests"
test/addrman_tests.cpp(126): Entering test case "addrman_simple"
test_bitcoin: addrman.cpp:518: void CAddrMan::Check(): Assertion `!nKey.IsNull()' failed.
unknown location(0): fatal error: in "addrman_tests/addrman_simple": signal: SIGABRT (application abort requested)
test/addrman_tests.cpp(133): last checkpoint
test/addrman_tests.cpp(126): Leaving test case "addrman_simple"; testing time: 144057us
test/addrman_tests.cpp(175): Entering test case "addrman_ports"
test_bitcoin: util/system.cpp:654: void ArgsManager::AddArg(const std::string &, const std::string &, unsigned int, const OptionsCategory &): Assertion `ret.second' failed.
unknown location(0): fatal error: in "addrman_tests/addrman_ports": signal: SIGABRT (application abort requested)
test/addrman_tests.cpp(175): last checkpoint: "addrman_ports" fixture ctor
test/addrman_tests.cpp(175): Leaving test case "addrman_ports"; testing time: 287us
....
If the line 518 (assert(!nKey.IsNull())) of addrman.cpp is commented out, the test will run successfully.
void CAddrMan::Check()
{
#ifdef DEBUG_ADDRMAN
AssertLockHeld(cs);
// ....
assert(!setTried.size());
assert(!mapNew.size());
// assert(!nKey.IsNull());
#endif
}
Concept ACK
This will make it inconvenient to do something other than abort() on Check() failure.
For example, it might make sense to do the check right after Unserialize() and if it fails - throw std::ios_base::failure as if disk corruption has occurred.
In general assert() should be used when a possible programming error has been detected, not on bad input.