p2p, doc: log DEBUG_ADDRMAN consistency checks, add developer notes info by jonatack · Pull Request #22479 · bitcoin/bitcoin

jonatack · 2021-07-17T15:43:54Z

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 · 2021-07-19T00:11:35Z

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

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:

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

Wonder what other useful information could be logged.

doc/developer-notes.md

jonatack · 2021-07-19T05:36:03Z

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?

theStack · 2021-07-19T11:30:04Z

Concept ACK

RiccardoMasutti · 2021-07-21T17:50:14Z

doc/developer-notes.md

+#define DEBUG_ADDRMAN
+```
+
+and then build (`make`) and run bitcoind. You can use the `-debug=addrman`


Suggested change

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

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

practicalswift · 2021-07-24T19:59:11Z

cr ACK 68ebaa0

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

mzumsande · 2021-07-25T21:30:28Z

ACK 68ebaa0

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

doc/developer-notes.md

jonatack · 2021-07-31T15:34:01Z

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

practicalswift · 2021-07-31T16:08:20Z

ACK baac29c

mzumsande · 2021-08-01T18:16:52Z

ACK baac29c

theStack

ACK baac29c

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 · 2021-08-05T09:45:51Z

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.

jonatack · 2021-08-05T10:11:30Z

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.

a4d7854 [addrman] Make addrman consistency checks a runtime option (John Newbery) 10aac24 [tests] Make deterministic addrman use nKey = 1 (John Newbery) fa9710f [addrman] Add deterministic argument to CAddrMan ctor (John Newbery) ee458d8 Add missing const to CAddrMan::Check_() (MarcoFalke) Pull request description: CAddrMan has internal consistency checks. Currently, these are only run when the program is compiled with the `DEBUG_ADDRMAN` option. This option is not enabled on any of our CI builds, and it's likely that no-one is running them at all. This PR makes consistency checks a (hidden) runtime option that can be enabled with `-checkaddrman`, where `-checkaddrman=n` will result in the consistency checks running every n operations (similar to `-checkmempool=n`). We set the ratio to 1/100 for our unit tests, and leave it disabled by default for all networks. Additionally, a consistency check failure now asserts, rather than logging and continuing. This matches the behavior of CTxMemPool and TxRequestTracker, where a failed consistency check asserts. ACKs for top commit: jonatack: ACK a4d7854 per `git diff 00fd089 a4d7854`, tested by adding logging similar to #22479 and running with `-checkaddrman=<n>` for various values 0/1/10/100 etc, tested the updated docs with `bitcoind -help-debug | grep -A2 "checkaddrman\|checkmempool"` and verified rebased on master that compiling with `CPPFLAGS="-DDEBUG_ADDRMAN"` no longer causes the build to error. mzumsande: Code-review ACK a4d7854 theStack: Code-review ACK a4d7854 Tree-SHA512: eaee003f7a99154822c5b5efbc62008d32c1efbecc6fec6e183427f6b2ae5d30b3be7924e3a7271b1a1de91517f5bd2a70011d45358c3105c6a0702f12b70f7c

jonatack · 2021-08-13T09:24:56Z

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 · 2021-08-13T09:40:43Z

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

4844b74 p2p: log addrman consistency checks (Jon Atack) Pull request description: This mini-patch picks up #22479 to log addrman consistency checks in the `BCLOG::ADDRMAN` category when they are enabled with the `-checkaddrman=<n>` configuration option for values of n greater than 0. ``` $ ./src/bitcoind -signet -checkaddrman=20 -debug=addrman ... 2021-08-13T11:14:45Z Addrman checks started: new 3352, tried 89, total 3441 2021-08-13T11:14:45Z Addrman checks completed successfully ``` This allows people to - verify the checks are running - see when and how often they are being performed - see the number of new/tried/total addrman entries per check - see the start/end of the checks Thanks to John Newbery for ideas to improve this logging. ACKs for top commit: jnewbery: Code review ACK 4844b74 Zero-1729: tACK 4844b74 theStack: Concept and code-review ACK 4844b74 ♟️ Tree-SHA512: 10b51c480d52a753ea8a59dbdd1e2c4f49067e7f4afe59d58426a8fb438f52447fe3a6090fa52132bc382d876927fa338b229c906d85668086f7f8f5bd8ed38a

4844b74 p2p: log addrman consistency checks (Jon Atack) Pull request description: This mini-patch picks up bitcoin#22479 to log addrman consistency checks in the `BCLOG::ADDRMAN` category when they are enabled with the `-checkaddrman=<n>` configuration option for values of n greater than 0. ``` $ ./src/bitcoind -signet -checkaddrman=20 -debug=addrman ... 2021-08-13T11:14:45Z Addrman checks started: new 3352, tried 89, total 3441 2021-08-13T11:14:45Z Addrman checks completed successfully ``` This allows people to - verify the checks are running - see when and how often they are being performed - see the number of new/tried/total addrman entries per check - see the start/end of the checks Thanks to John Newbery for ideas to improve this logging. ACKs for top commit: jnewbery: Code review ACK 4844b74 Zero-1729: tACK 4844b74 theStack: Concept and code-review ACK 4844b74 ♟️ Tree-SHA512: 10b51c480d52a753ea8a59dbdd1e2c4f49067e7f4afe59d58426a8fb438f52447fe3a6090fa52132bc382d876927fa338b229c906d85668086f7f8f5bd8ed38a

DrahtBot added the Docs label Jul 17, 2021

jarolrod reviewed Jul 19, 2021

View reviewed changes

doc/developer-notes.md Outdated Show resolved Hide resolved

jonatack force-pushed the debug-addrman branch from eeed7dd to 68ebaa0 Compare July 19, 2021 10:18

DrahtBot mentioned this pull request Jul 19, 2021

addrman: Make consistency checks a runtime option #20233

Closed

DrahtBot mentioned this pull request Jul 19, 2021

Assert in CAddrMan::Check() #22500

Closed

RiccardoMasutti suggested changes Jul 21, 2021

View reviewed changes

fanquake reviewed Jul 29, 2021

View reviewed changes

doc/developer-notes.md Outdated Show resolved Hide resolved

jonatack added 2 commits July 31, 2021 15:36

p2p: log DEBUG_ADDRMAN consistency checks

74216f0

doc: add DEBUG_ADDRMAN info to the developer notes

baac29c

jonatack force-pushed the debug-addrman branch from 68ebaa0 to baac29c Compare July 31, 2021 15:30

theStack approved these changes Aug 2, 2021

View reviewed changes

fanquake closed this Aug 5, 2021

bitcoin deleted a comment from josedanielrojaspixel Aug 5, 2021

jonatack deleted the debug-addrman branch August 13, 2021 09:26

jonatack mentioned this pull request Aug 13, 2021

p2p: log addrman consistency checks #22696

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

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

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

theStack commented Jul 19, 2021

Uh oh!

RiccardoMasutti Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Jul 24, 2021

Uh oh!

mzumsande commented Jul 25, 2021

Uh oh!

Uh oh!

jonatack commented Jul 31, 2021

Uh oh!

practicalswift commented Jul 31, 2021

Uh oh!

mzumsande commented Aug 1, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Aug 5, 2021

Uh oh!

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

Uh oh!

Uh oh!

jonatack commented Aug 13, 2021

Uh oh!

jonatack commented Aug 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jonatack commented Jul 17, 2021 •

edited

Loading

DrahtBot commented Jul 19, 2021 •

edited

Loading

jonatack commented Jul 19, 2021 •

edited

Loading

jonatack commented Aug 5, 2021 •

edited

Loading