build: debug enable addrman consistency checks

willcl-ark commented at 1:52 pm on March 22, 2023: member

The --enable-debug flag should default to enabling debug consistency checks. This PR enables addrman and mempool consistency checks at a rate of once per 1000 operations, by default when running in debug mode.

To opt out of the checks when running a debug build specify the runtime -checkmempool=0 and -checkaddrman=0 debug options, as described in developer-notes.md

On my machine, configuring these checks to run every 1000 operations saw them run no more than once per minute (and often much less frequently), which did not introduce any significant overhead.

This PR does not include a -checkblockindex default consistency check for mainnet when running in debug mode.

fixes #24709

DrahtBot commented at 1:52 pm on March 22, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	MarcoFalke, mzumsande

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label Build system on Mar 22, 2023

in src/addrman.h:35 in 61e3f8ab25 outdated

26@@ -27,7 +27,11 @@ class InvalidAddrManVersionError : public std::ios_base::failure
27 class AddrManImpl;
28 
29 /** Default for -checkaddrman */
30+#ifdef DEBUG_ADDRMAN
31+static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{1};
32+#else
33 static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{0};
34+#endif

maflcko commented at 2:02 pm on March 22, 2023:

0static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{
1#ifdef DEBUG_ADDRMAN
2    1
3#else
4    0
5#endif
6};

style nit (feel free to ignore, if you don’t like it)

maflcko commented at 2:04 pm on March 22, 2023: member

Concept ACK.

Though, I haven’t checked how expensive those checks are. If they make the program unusable slow, I can see that someone would Nack this being on by default.

What about mempool checks?

mzumsande commented at 3:10 pm on March 22, 2023: contributor

Concept ACK - I’ve run with -checkaddrman=1 before, never found bitcoind to be unusably slow. Not sure about the mempool checks though.

Generally speaking, I think it’s a bit strange that --enable-debug mixes two different concepts: 1.) Compiler flags for attaching a debugger 2.) More (but costly) runtime checks (like -DDEBUG_LOCKORDER currently, plus additionally custom checks for addrman / mempool etc.)

I don’t think it’s very common that people would want both 1 and 2 at the same time: If I want to attach a debugger for better analysis of a specific part of the code, I’m not interested in checks for unrelated parts - if I want to run with more checks to find possible unknown bugs, I’m not interested in attaching a debugger. So why do we have one flag for the two?

maflcko commented at 3:25 pm on March 22, 2023: member

Maybe there can be a --enable-debug-symbols (implied by --enable-debug) to set -g for the compiler?

willcl-ark commented at 4:27 pm on March 22, 2023: member

@mzumsande FYI this PR is equivalent to running with -checkaddrman=1 which does use quite some resources as it does a full addrman consistency check on pretty much every addrman operation (Add, Good, Attempt, Get, Select, Connected etc.).

For me this manifests as saturating an entire core via a combination of the b-msghand, b-net and b-opencon threads as these all spawn consistency checks. My bitcoind process usually idles at ~5%, so to have one core @ 100% is a pretty big change, but my machine has 8 physical cores so it’s also not too terrible…

I have not profiled with perf as it is so obvious in top.

I also agree that there are two debug scenarios (which are already mixed) being further mixed by this PR, i) enabling debugging symbols and ii) running the application in “debug mode”.

In a vacuum I would agree with @MarcoFalke in introducing an --enable-debug-symbols flag, however in this project, with --enable-debug as the encumbent and there being a lot of developer expectation around what it does, perhaps the new flag should be --debug-mode which both implies --enable-debug and actually runs with all these debug mode options enabled by default?

in src/addrman.h:31 in 61e3f8ab25 outdated

26@@ -27,7 +27,11 @@ class InvalidAddrManVersionError : public std::ios_base::failure
27 class AddrManImpl;
28 
29 /** Default for -checkaddrman */
30+#ifdef DEBUG_ADDRMAN
31+static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{1};

ajtowns commented at 7:20 am on May 19, 2023:

If these checks are going to be enabled by default on some builds, shouldn’t the default be much higher; eg 100 or 1000? Doing it on every call is useful for live debugging or small scale tests, but seems overkill for a node’s runtime default?

willcl-ark commented at 7:26 am on May 19, 2023:

Yes I agree, the performance hit is too much for any kind of normal operation.

I updated the branch with @MarcoFalke’s suggestion along with a new debug option (which I called --enable-debug-checks, and not --debug-slow).

I’ll run a few runtime tests on various values for the checks to see if I can find a sweet spot for the frequency, then update the doc commit and push an update here.

willcl-ark force-pushed on May 22, 2023

willcl-ark commented at 2:49 pm on May 22, 2023: member

OK pushed an update based on feedback here, in #24709 and #27586.

PR description updated with new details of this pull.
My opinion is that running these checks at a low enough frequency on mainnet debug builds is desirable and non-intrusive
Selecting values of 1000 for both consistency checks worked with no noticable overhead for me, and appeared to run once every minute or so, although my test node is not listening so others may see higher frequency in “production debug” builds which are listening.
I am unsure if mempol_args.h is the right place for DEFAULT_MEMPOOL_DEBUG_CHECKS, and open to suggestions on that.
Happy to sqush both doc commits into their respective build commits, or indeed squash all 4 into a single commit, if preferred.

DrahtBot added the label CI failed on May 22, 2023

DrahtBot removed the label CI failed on May 22, 2023

DrahtBot added the label Needs rebase on May 26, 2023

build: enable addrman consistency checks in debug

When configured with --enable-debug run addrman consistency checks every
1000 operations by default.

Running every 1000 operations minimises the CPU overhead on the node and
keeps it viable to run a node in debug mode on mainnet.

bd6fa0be3a

doc: Add DDEBUG_ADDRMAN to developer-notes.md d52626fd4f

build: enable mempool consistency checks in debug

When configured with --enable-debug run mempool consistency checks every
1000 operations by default.

fe97e6bbae

doc: Add DDEBUG_MEMPOOL to developer-notes.md 9c2f4ca173

willcl-ark force-pushed on May 30, 2023

DrahtBot added the label CI failed on May 30, 2023

DrahtBot removed the label Needs rebase on May 30, 2023

maflcko commented at 8:23 am on June 27, 2023: member

Is this still relevant, given that the issue was closed?

willcl-ark commented at 1:26 pm on June 27, 2023: member

Nope, I also agree with the conclusions from AJ in 24709 about just setting this as a runtime option, so will close this.

willcl-ark closed this on Jun 27, 2023

bitcoin locked this on Jun 26, 2024

build: debug enable addrman consistency checks #27300

Reviews

Conflicts