Assert in CAddrMan::Check()

MarcoFalke commented at 2:51 pm on July 19, 2021: member

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.

cccc47b0ef

jonatack commented at 3:10 pm on July 19, 2021: member

Approach ACK

DrahtBot added the label P2P on Jul 19, 2021

in src/addrman.cpp:499 in cccc47b0ef outdated

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]);

jonatack commented at 4:13 pm on July 19, 2021:

nit, spacing here is off by one

in src/addrman.h:725 in cccc47b0ef outdated

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);

jonatack commented at 4:13 pm on July 19, 2021:

0     //! Consistency check
1-    void Check()
2-        EXCLUSIVE_LOCKS_REQUIRED(cs);
3+    void Check() EXCLUSIVE_LOCKS_REQUIRED(cs);

jonatack commented at 4:14 pm on July 19, 2021: member

ACK cccc47b0ef5af66c8e6349d7cef60b902b80bc09

Tested with the following changes

 0--- a/src/addrman.cpp
 1+++ b/src/addrman.cpp
 2@@ -463,6 +463,7 @@ void CAddrMan::Check()
 3 {
 4 #ifdef DEBUG_ADDRMAN
 5     AssertLockHeld(cs);
 6+    LogPrint(BCLog::ADDRMAN, "Check addrman\n");
 7
 8--- a/src/addrman.h
 9+++ b/src/addrman.h
10@@ -122,6 +122,7 @@ public:
11  *    * Several indexes are kept for high performance. Defining DEBUG_ADDRMAN will introduce frequent (and expensive)
12  *      consistency checks for the entire data structure.
13  */
14
15+#define DEBUG_ADDRMAN
16 
17 //! total number of buckets for tried addresses
18 #define ADDRMAN_TRIED_BUCKET_COUNT_LOG2 8

Fix whitespace

Requested in https://github.com/bitcoin/bitcoin/pull/22500#discussion_r672440473

fa4bfef479

lsilva01 approved

lsilva01 commented at 6:30 pm on July 19, 2021: contributor

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.

DrahtBot commented at 6:49 pm on July 19, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22498 by MarcoFalke
#22479 by jonatack
#21940 by MarcoFalke
#20233 by jnewbery

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.

theStack approved

theStack commented at 9:29 am on July 20, 2021: member

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)

jonatack commented at 10:10 am on July 20, 2021: member

re-ACK fa4bfef4798466e986569baf0d8e3213f17cafc8 have been testing this rebased to current master with DEBUG_ADDRMAN defined

could squash

jnewbery referenced this in commit be6d4ac4bb on Jul 21, 2021

jnewbery commented at 9:32 am on July 21, 2021: member

~I’ve added these two commits to #20233, since the two PRs are doing overlapping things.~

EDIT: removed them again after discussing with Marco.

MarcoFalke marked this as a draft on Jul 21, 2021

lsilva01 commented at 8:31 pm on July 21, 2021: contributor

If DEBUG_ADDRMAN is set, the following error will occur when addrman_tests is run:

 0$ src/test/test_bitcoin --run_test=addrman_tests --log_level=unit_scope
 1Running 18 test cases...
 2Entering test module "Bitcoin Core Test Suite"
 3test/addrman_tests.cpp(124): Entering test suite "addrman_tests"
 4test/addrman_tests.cpp(126): Entering test case "addrman_simple"
 5test_bitcoin: addrman.cpp:518: void CAddrMan::Check(): Assertion `!nKey.IsNull()' failed.
 6unknown location(0): fatal error: in "addrman_tests/addrman_simple": signal: SIGABRT (application abort requested)
 7test/addrman_tests.cpp(133): last checkpoint
 8test/addrman_tests.cpp(126): Leaving test case "addrman_simple"; testing time: 144057us
 9
10test/addrman_tests.cpp(175): Entering test case "addrman_ports"
11test_bitcoin: util/system.cpp:654: void ArgsManager::AddArg(const std::string &, const std::string &, unsigned int, const OptionsCategory &): Assertion `ret.second' failed.
12unknown location(0): fatal error: in "addrman_tests/addrman_ports": signal: SIGABRT (application abort requested)
13test/addrman_tests.cpp(175): last checkpoint: "addrman_ports" fixture ctor
14test/addrman_tests.cpp(175): Leaving test case "addrman_ports"; testing time: 287us
15....

If the line 518 (assert(!nKey.IsNull())) of addrman.cpp is commented out, the test will run successfully.

0void CAddrMan::Check()
1{
2#ifdef DEBUG_ADDRMAN
3    AssertLockHeld(cs);
4// ....
5    assert(!setTried.size());
6    assert(!mapNew.size());
7    // assert(!nKey.IsNull());
8#endif
9}

jnewbery commented at 8:38 pm on July 21, 2021: member

If DEBUG_ADDRMAN is set, the following error will occur when addrman_tests is run … @lsilva01 this is fixed in the first commit of #20233.

practicalswift commented at 7:39 pm on July 24, 2021: contributor

Concept ACK

vasild commented at 8:48 am on July 26, 2021: member

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.

MarcoFalke closed this on Sep 10, 2021

MarcoFalke deleted the branch on Sep 10, 2021

DrahtBot locked this on Oct 30, 2022

Assert in CAddrMan::Check() #22500

Conflicts