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

pull jonatack wants to merge 2 commits into bitcoin:master from jonatack:debug-addrman changing 2 files +17 −0
  1. 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
  2. DrahtBot added the label Docs on Jul 17, 2021
  3. DrahtBot commented at 12:11 AM on July 19, 2021: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  4. 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:

    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.

  5. 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:

     Defining `DEBUG_ADDRMAN` performs frequent (and expensive) consistency checks on
-the entire addrman data structure. Add this line to enable it, e.g. in
-`src/addrman.h`
+the entire addrman data structure. To enable it, add this line in `src/addrman.h`
+before `void Check()`
  6. 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?

  7. jonatack force-pushed on Jul 19, 2021
  8. theStack commented at 11:30 AM on July 19, 2021: member

    Concept ACK

  9. 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: 
    then build (`make`) and run bitcoind. You can use the `-debug=addrman`
  10. RiccardoMasutti changes_requested
  11. 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.

  12. 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.

  13. 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.

  14. p2p: log DEBUG_ADDRMAN consistency checks 74216f0b58
  15. doc: add DEBUG_ADDRMAN info to the developer notes baac29c74e
  16. jonatack force-pushed on Jul 31, 2021
  17. jonatack commented at 3:34 PM on July 31, 2021: member

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

  18. practicalswift commented at 4:08 PM on July 31, 2021: contributor

    ACK baac29c74e475a1e7239355e710f2c6ad0cd19e0

  19. mzumsande commented at 6:16 PM on August 1, 2021: member

    ACK baac29c74e475a1e7239355e710f2c6ad0cd19e0

  20. theStack approved
  21. 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.

  22. 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.

  23. fanquake closed this on Aug 5, 2021
  24. fanquake deleted a comment on Aug 5, 2021
  25. 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.

  26. fanquake referenced this in commit 803ef70fd9 on Aug 13, 2021
  27. 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.

  28. jonatack deleted the branch on Aug 13, 2021
  29. 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.

  30. fanquake referenced this in commit adf9bcfc37 on Aug 14, 2021
  31. sidhujag referenced this in commit 5218da89f3 on Aug 15, 2021
  32. DrahtBot locked this on Aug 18, 2022
Contributors
jonatackDrahtBotjarolrodtheStackRiccardoMasuttipracticalswiftmzumsandefanquake
Labels
Docs
Linked (view graph)
#22696 p2p: log addrman consistency checks
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-22 18:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me