p2p, doc: log DEBUG_ADDRMAN consistency checks, add developer notes info

jonatack commented at 3:43 pm on July 17, 2021: member

Provide logging and info to help people

use the DEBUG_ADDRMAN consistency checks
verify the checks are running
see when and how often they are being performed

DrahtBot added the label Docs on Jul 17, 2021

DrahtBot commented at 0:11 am 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:

#20233 (addrman: Make consistency checks a runtime option 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.

jarolrod commented at 4:50 am on July 19, 2021: member

Concept ACK

I see how this can be useful and is a nice addition. After following your instructions, my debug file is filled with thousands of lines of:

02021-07-19T04:37:25Z Check addrman
12021-07-19T04:37:25Z ADDRMAN CONSISTENCY CHECK FAILED!!! err=-19

Wonder what other useful information could be logged.

in doc/developer-notes.md:289 in eeed7dd969 outdated

282@@ -282,6 +283,20 @@ configure option adds `-DDEBUG_LOCKORDER` to the compiler flags. This inserts
283 run-time checks to keep track of which locks are held and adds warnings to the
284 `debug.log` file if inconsistencies are detected.
285 
286+### DEBUG_ADDRMAN
287+
288+Defining `DEBUG_ADDRMAN` performs frequent (and expensive) consistency checks on
289+the entire addrman data structure. Add this line to enable it, e.g. in

jarolrod commented at 4:51 am on July 19, 2021:

is src/addrman.h the only file we could define this in? Are there other files we could do this in. If there are I feel as though we could improve the note here. Otherwise, we could remove e.g.

jonatack commented at 10:19 am on July 19, 2021:

Good point. Improved as follows:

0 Defining `DEBUG_ADDRMAN` performs frequent (and expensive) consistency checks on
1-the entire addrman data structure. Add this line to enable it, e.g. in
2-`src/addrman.h`
3+the entire addrman data structure. To enable it, add this line in `src/addrman.h`
4+before `void Check()`

jonatack commented at 5:36 am on July 19, 2021: member

Thanks @jarolrod for testing. I’ve not seen this check fail before; could you describe your config? It’s hitting src/addrman.cpp::L524. How many tor and i2p addresses are returned if you run -addrinfo?

jonatack force-pushed on Jul 19, 2021

theStack commented at 11:30 am on July 19, 2021: member

Concept ACK

in doc/developer-notes.md:296 in 68ebaa03f4 outdated

291+
292+```cpp
293+#define DEBUG_ADDRMAN
294+```
295+
296+and then build (`make`) and run bitcoind. You can use the `-debug=addrman`

RiccardoMasutti commented at 5:50 pm on July 21, 2021:

0then build (`make`) and run bitcoind. You can use the `-debug=addrman`

RiccardoMasutti changes_requested

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

cr ACK 68ebaa03f42732e70ab6922def36b3eaaeee8b50

Making this more clear makes sense since one might incorrectly assume that --enable-debug would define DEBUG_ADDRMAN.

mzumsande commented at 9:30 pm on July 25, 2021: member

ACK 68ebaa03f42732e70ab6922def36b3eaaeee8b50

I remember enabling DEBUG_ADDRMAN once as described, but asking myself if I was really supposed to do it this way.

in doc/developer-notes.md:289 in 68ebaa03f4 outdated

282@@ -282,6 +283,20 @@ configure option adds `-DDEBUG_LOCKORDER` to the compiler flags. This inserts
283 run-time checks to keep track of which locks are held and adds warnings to the
284 `debug.log` file if inconsistencies are detected.
285 
286+### DEBUG_ADDRMAN
287+
288+Defining `DEBUG_ADDRMAN` performs frequent (and expensive) consistency checks on
289+the entire addrman data structure. To enable it, add this line in `src/addrman.h`

fanquake commented at 1:03 am on July 29, 2021:

You can also just add -DDEBUG_ADDRMAN to your CPPFLAGS, rather than modifying the source.

jonatack commented at 3:30 pm on July 31, 2021:

Thanks, added.

p2p: log DEBUG_ADDRMAN consistency checks 74216f0b58

doc: add DEBUG_ADDRMAN info to the developer notes baac29c74e

jonatack force-pushed on Jul 31, 2021

jonatack commented at 3:34 pm on July 31, 2021: member

Updated with @fanquake’s suggestion per git range-diff 6499928 68ebaa0 baac29c

practicalswift commented at 4:08 pm on July 31, 2021: contributor

ACK baac29c74e475a1e7239355e710f2c6ad0cd19e0

mzumsande commented at 6:16 pm on August 1, 2021: member

ACK baac29c74e475a1e7239355e710f2c6ad0cd19e0

theStack approved

theStack commented at 8:15 pm on August 2, 2021: member

ACK baac29c74e475a1e7239355e710f2c6ad0cd19e0

Tested on signet. #20233 seems to be a very promising long-term alternative by allowing activating addrman consistency checks at runtime. Right now, the documentation introduced in this PR will serve as a good and useful guide in how to properly use the compile-time debug option 👍. Logging the consistency checks also makes a lot of sense.

fanquake commented at 9:45 am on August 5, 2021: member

Given it’s likely that #20233 is going to be merged shortly, which just removes DEBUG_ADDRMAN, I’m going to close this PR in favour of that change.

fanquake closed this on Aug 5, 2021

fanquake deleted a comment on Aug 5, 2021

jonatack commented at 10:11 am on August 5, 2021: member

Oh, I didn’t see the two changes as mutually exclusive. I think the logging is still pertinent and was planning to rebase and drop the doc commit if that PR is merged, and perhaps add some follow-ups.

fanquake referenced this in commit 803ef70fd9 on Aug 13, 2021

jonatack commented at 9:24 am on August 13, 2021: member

Hm, seems I don’t have the privileges required to re-open my PR for the logging. Will open a new one, I guess.

jonatack deleted the branch on Aug 13, 2021

jonatack commented at 9:40 am on August 13, 2021: member

Thanks for the reviews everyone. Feel free to re-ACK in #22696 if you think it is helpful.

fanquake referenced this in commit adf9bcfc37 on Aug 14, 2021

sidhujag referenced this in commit 5218da89f3 on Aug 15, 2021

DrahtBot locked this on Aug 18, 2022

p2p, doc: log DEBUG_ADDRMAN consistency checks, add developer notes info #22479

Conflicts