build: debug enable addrman consistency checks #27300

willcl-ark · 2023-03-22T13:52:10Z

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 · 2023-03-22T13:52:13Z

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.

maflcko

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?

maflcko · 2023-03-22T14:02:46Z

src/addrman.h

+#ifdef DEBUG_ADDRMAN
+static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{1};
+#else
 static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{0};
+#endif


Suggested change

#ifdef DEBUG_ADDRMAN

static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{1};

#else

static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{0};

#endif

static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{

#ifdef DEBUG_ADDRMAN

1

#else

0

#endif

};

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

mzumsande

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 · 2023-03-22T15:25:25Z

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

willcl-ark · 2023-03-22T16:27:22Z

@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?

ajtowns · 2023-05-19T07:20:20Z

src/addrman.h

@@ -27,7 +27,11 @@ class InvalidAddrManVersionError : public std::ios_base::failure
 class AddrManImpl;

 /** Default for -checkaddrman */
+#ifdef DEBUG_ADDRMAN
+static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{1};


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?

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 · 2023-05-22T14:49:17Z

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.

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.

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

maflcko · 2023-06-27T08:23:29Z

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

willcl-ark · 2023-06-27T13:26:30Z

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

DrahtBot added the Build system label Mar 22, 2023

maflcko reviewed Mar 22, 2023

View reviewed changes

mzumsande reviewed Mar 22, 2023

View reviewed changes

maflcko mentioned this pull request May 19, 2023

CPU DoS on mainnet in debug mode #27586

Closed

ajtowns reviewed May 19, 2023

View reviewed changes

ajtowns mentioned this pull request May 19, 2023

Enable consistency checks by default with --enable-debug #24709

Closed

willcl-ark force-pushed the enable-addrman-consist branch from 61e3f8a to 7b32641 Compare May 22, 2023 14:37

DrahtBot added CI failed and removed CI failed labels May 22, 2023

DrahtBot mentioned this pull request May 25, 2023

refactor: Replace std::optional<bilingual_str> with util::Result #25977

Merged

DrahtBot added the Needs rebase label May 26, 2023

willcl-ark added 4 commits May 30, 2023 09:22

build: enable addrman consistency checks in debug

bd6fa0b

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.

doc: Add DDEBUG_ADDRMAN to developer-notes.md

d52626f

build: enable mempool consistency checks in debug

fe97e6b

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

doc: Add DDEBUG_MEMPOOL to developer-notes.md

9c2f4ca

willcl-ark force-pushed the enable-addrman-consist branch from 7b32641 to 9c2f4ca Compare May 30, 2023 08:23

DrahtBot added CI failed and removed Needs rebase labels May 30, 2023

willcl-ark closed this Jun 27, 2023

bitcoin locked and limited conversation to collaborators Jun 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

build: debug enable addrman consistency checks #27300

build: debug enable addrman consistency checks #27300

Uh oh!

willcl-ark commented Mar 22, 2023 •

edited

Loading

Uh oh!

DrahtBot commented Mar 22, 2023 •

edited

Loading

Uh oh!

maflcko left a comment

Uh oh!

maflcko Mar 22, 2023

Uh oh!

mzumsande left a comment •

edited

Loading

Uh oh!

maflcko commented Mar 22, 2023

Uh oh!

willcl-ark commented Mar 22, 2023

Uh oh!

ajtowns May 19, 2023

Uh oh!

willcl-ark May 19, 2023

Uh oh!

willcl-ark commented May 22, 2023 •

edited

Loading

Uh oh!

maflcko commented Jun 27, 2023

Uh oh!

willcl-ark commented Jun 27, 2023

Uh oh!

Uh oh!

build: debug enable addrman consistency checks #27300

build: debug enable addrman consistency checks #27300

Uh oh!

Conversation

willcl-ark commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

mzumsande 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!

maflcko commented Mar 22, 2023

Uh oh!

willcl-ark commented Mar 22, 2023

Uh oh!

ajtowns May 19, 2023

Choose a reason for hiding this comment

Uh oh!

willcl-ark May 19, 2023

Choose a reason for hiding this comment

Uh oh!

willcl-ark commented May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jun 27, 2023

Uh oh!

willcl-ark commented Jun 27, 2023

Uh oh!

Uh oh!

willcl-ark commented Mar 22, 2023 •

edited

Loading

DrahtBot commented Mar 22, 2023 •

edited

Loading

mzumsande left a comment •

edited

Loading

willcl-ark commented May 22, 2023 •

edited

Loading