Assert in CAddrMan::Check() by maflcko · Pull Request #22500 · bitcoin/bitcoin

maflcko · 2021-07-19T14:51:33Z

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.

jonatack · 2021-07-19T15:10:10Z

Approach ACK

jonatack

ACK cccc47b

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

jonatack · 2021-07-19T16:13:20Z

src/addrman.cpp

+                assert(setTried.count(vvTried[n][i]));
+                assert(mapInfo[vvTried[n][i]].GetTriedBucket(nKey, m_asmap) == n);
+                assert(mapInfo[vvTried[n][i]].GetBucketPosition(nKey, false, n) == i);
+                setTried.erase(vvTried[n][i]);


nit, spacing here is off by one

jonatack · 2021-07-19T16:13:45Z

src/addrman.h

-    //! Perform consistency check. Returns an error code or zero.
-    int Check_() EXCLUSIVE_LOCKS_REQUIRED(cs);
-#endif
+        EXCLUSIVE_LOCKS_REQUIRED(cs);


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

Requested in bitcoin#22500 (comment)

lsilva01

Code Review and tested ACK fa4bfef on Ubuntu 20.04. The change simplifies the CAddrMan::Check() code.

DrahtBot · 2021-07-19T18:49:04Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Check that CAddrMan::nKey is not null after deserialize #22498 by MarcoFalke
p2p, doc: log DEBUG_ADDRMAN consistency checks, add developer notes info #22479 by jonatack
refactor: Mark CAddrMan::Select and GetAddr const #21940 by MarcoFalke
addrman: Make consistency checks a runtime option #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

Concept and code review ACK fa4bfef

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 · 2021-07-20T10:10:59Z

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

could squash

jnewbery · 2021-07-21T09:32:21Z

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

EDIT: removed them again after discussing with Marco.

lsilva01 · 2021-07-21T20:31:42Z

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 · 2021-07-21T20:38:06Z

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 · 2021-07-24T19:39:36Z

Concept ACK

vasild · 2021-07-26T08:48:17Z

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.

Assert in CAddrMan::Check()

cccc47b

The same is done in CTxMemPool::check() and other debug checks for developers, like lock order debugging.

DrahtBot added the P2P label Jul 19, 2021

jonatack reviewed Jul 19, 2021

View reviewed changes

Fix whitespace

fa4bfef

Requested in bitcoin#22500 (comment)

lsilva01 approved these changes Jul 19, 2021

View reviewed changes

This was referenced Jul 20, 2021

Addrman check failure info.nLastSuccess is 0 even though info.fInTried #22503

Closed

Addrman check failure info.nRefCount is non-zero even though !info.fInTried #22504

Closed

theStack approved these changes Jul 20, 2021

View reviewed changes

maflcko marked this pull request as draft July 21, 2021 09:38

maflcko mentioned this pull request Jul 21, 2021

Addrman check failure: info.nLastSuccess is negative #22519

Closed

maflcko mentioned this pull request Jul 28, 2021

[Refactor] Apply clang-format to netaddress.h #22562

Closed

maflcko closed this Sep 10, 2021

maflcko deleted the 2107-addrmanCheckAssert branch September 10, 2021 15:21

bitcoin locked and limited conversation to collaborators Oct 30, 2022

Conversation

maflcko commented Jul 19, 2021

Uh oh!

jonatack commented Jul 19, 2021

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Jul 19, 2021

Choose a reason for hiding this comment

Uh oh!

lsilva01 left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 19, 2021

Conflicts

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lsilva01 commented Jul 21, 2021

Uh oh!

jnewbery commented Jul 21, 2021

Uh oh!

practicalswift commented Jul 24, 2021

Uh oh!

vasild commented Jul 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jonatack left a comment •

edited

Loading

jonatack Jul 19, 2021 •

edited

Loading

jonatack commented Jul 20, 2021 •

edited

Loading

jnewbery commented Jul 21, 2021 •

edited

Loading