p2p: log addrman consistency checks #22696

1. 
   jonatack commented at 9:36 am on August 13, 2021: member

   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.

   0$ ./src/bitcoind -signet -checkaddrman=20 -debug=addrman
   1...
   22021-08-13T11:14:45Z Addrman checks started: new 3352, tried 89, total 3441
   32021-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.

2. 
   jonatack commented at 9:42 am on August 13, 2021: member

   I used this logging while reviewing and testing #20233 and while running the checks over the past months.
3. DrahtBot added the label P2P on Aug 13, 2021
4. 
   jnewbery commented at 10:32 am on August 13, 2021: member

   Code review ACK d3ae9edaec56e5c8c808a4520f8f9eefb5264ccc. This mirrors the logging done in mempool when the consistency checks are run:

   https://github.com/bitcoin/bitcoin/blob/803ef70fd9f65ef800567ff9456fac525bc3e3c2/src/txmempool.cpp#L691

   Two potential enhancements for your consideration:

   • print nNew and nTried in the log, so it’s possible to see how much work the Check_() function has to do.
   • print a “Addrman checks successful” log at the end of the funcion, so it’s possible to see how long the consistency checks took to run.
5. 
   Zero-1729 commented at 11:16 am on August 13, 2021: contributor

   crACK d3ae9edaec56e5c8c808a4520f8f9eefb5264ccc
6. p2p: log addrman consistency checks 4844b74ba7
7. 
   jonatack commented at 11:17 am on August 13, 2021: member

   Updated per @jnewbery’s feedback (good ideas, thanks!)

   0$ ./src/bitcoind -signet -checkaddrman=20 -debug=addrman
   1...
   22021-08-13T11:14:45Z Addrman checks started: new 3352, tried 89, total 3441
   32021-08-13T11:14:45Z Addrman checks completed successfully
   

8. jonatack force-pushed on Aug 13, 2021
9. Zero-1729 approved
10. 
    Zero-1729 commented at 11:29 am on August 13, 2021: contributor

    tACK 4844b74ba73ecc6d336a52b4dc4cd144a01b0ea2

    Nice update, LGTM 🧉

    0$ src/bitcoind -signet -checkaddrman=20 -debug=addrman
    1...
    22021-08-13T11:22:11Z Addrman checks started: new 431, tried 16, total 447
    32021-08-13T11:22:11Z Addrman checks completed successfully
    4...
    

11. 
    jonatack commented at 11:30 am on August 13, 2021: member

    Should the number of tried be logged before the number of new, to reflect the order of the checks and the Add() logging? e.g. in src/addrman.h

    0LogPrint(BCLog::ADDRMAN, "Added %s from %s: %i tried, %i new\n", addr.ToStringIPPort(), source.ToString(), nTried, nNew); 
    

12. 
    jnewbery commented at 11:41 am on August 13, 2021: member

    Code review ACK 4844b74ba73ecc6d336a52b4dc4cd144a01b0ea2

    Should the number of tried be logged before the number of new, to reflect the order of the checks and the Add() logging?

    I don’t think it matters.

13. 
    Zero-1729 commented at 11:43 am on August 13, 2021: contributor

    Should the number of tried be logged before the number of new, to reflect the order of the checks and the Add() logging?

    No strong opinions here, but in this case, I think reflecting the order of checks makes sense since we are logging.

14. theStack approved
15. 
    theStack commented at 1:15 pm on August 13, 2021: member

    Concept and code-review ACK 4844b74ba73ecc6d336a52b4dc4cd144a01b0ea2 ♟️

    Quickly tested on signet, as inspired by jonatack and Zero-1729:

     0$ ./src/bitcoind -signet -checkaddrman=10 -debug=addrman | grep "Addrman checks"
     12021-08-13T13:14:23Z Addrman checks started: new 292, tried 18, total 310
     22021-08-13T13:14:23Z Addrman checks completed successfully
     32021-08-13T13:14:24Z Addrman checks started: new 292, tried 18, total 310
     42021-08-13T13:14:24Z Addrman checks completed successfully
     52021-08-13T13:14:24Z Addrman checks started: new 292, tried 18, total 310
     62021-08-13T13:14:24Z Addrman checks completed successfully
     72021-08-13T13:14:29Z Addrman checks started: new 292, tried 18, total 310
     82021-08-13T13:14:29Z Addrman checks completed successfully
     92021-08-13T13:14:34Z Addrman checks started: new 292, tried 18, total 310
    102021-08-13T13:14:34Z Addrman checks completed successfully
    11...
    

16. fanquake merged this on Aug 14, 2021
17. fanquake closed this on Aug 14, 2021

    ---

18. jonatack deleted the branch on Aug 14, 2021
19. sidhujag referenced this in commit 5218da89f3 on Aug 15, 2021
20. DrahtBot locked this on Aug 18, 2022

Contributors
jonatack jnewbery Zero-1729 theStack

Labels
P2P