Assert in CAddrMan::Check() #22500

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2107-addrmanCheckAssert changing 2 files +26 −61

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

Assert in CAddrMan::Check()

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:

     //! Consistency check
-    void Check()
-        EXCLUSIVE_LOCKS_REQUIRED(cs);
+    void Check() EXCLUSIVE_LOCKS_REQUIRED(cs);

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

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

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:

$ 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
}

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

Contributors

Labels

Linked (view graph)

#22503 Addrman check failure info.nLastSuccess is 0 even though info.fInTried #22504 Addrman check failure info.nRefCount is non-zero even though !info.fInTried #22519 Addrman check failure: info.nLastSuccess is negative

Assert in CAddrMan::Check() #22500

Conflicts