Skip to content

p2p: log addrman consistency checks#22696

Merged
fanquake merged 1 commit intobitcoin:masterfrom
jonatack:log-addrman-consistency-checks
Aug 14, 2021
Merged

p2p: log addrman consistency checks#22696
fanquake merged 1 commit intobitcoin:masterfrom
jonatack:log-addrman-consistency-checks

Conversation

@jonatack
Copy link
Member

@jonatack jonatack commented Aug 13, 2021 •
edited
Loading

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.

@jonatack jonatack mentioned this pull request Aug 13, 2021
Closed
@jonatack
Copy link
Member Author

jonatack commented Aug 13, 2021

I used this logging while reviewing and testing #20233 and while running the checks over the past months.

@DrahtBot DrahtBot added the P2P label Aug 13, 2021
@jnewbery
Copy link
Contributor

jnewbery commented Aug 13, 2021

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

bitcoin/src/txmempool.cpp

Line 691 in 803ef70

LogPrint(BCLog::MEMPOOL, "Checking mempool with %u transactions and %u inputs\n", (unsigned int)mapTx.size(), (unsigned int)mapNextTx.size());

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.

Zero-1729
Zero-1729 reviewed Aug 13, 2021
View reviewed changes
Copy link
Contributor

@Zero-1729 Zero-1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crACK d3ae9edaec56e5c8c808a4520f8f9eefb5264ccc

@jonatack
Copy link
Member Author

jonatack commented Aug 13, 2021

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

$ ./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

@jonatack jonatack force-pushed the log-addrman-consistency-checks branch from d3ae9ed to 4844b74 Compare August 13, 2021 11:19
Zero-1729
Zero-1729 approved these changes Aug 13, 2021
View reviewed changes
Copy link
Contributor

@Zero-1729 Zero-1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 4844b74

Nice update, LGTM 🧉

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

@jonatack
Copy link
Member Author

jonatack commented Aug 13, 2021

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

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

@jnewbery
Copy link
Contributor

jnewbery commented Aug 13, 2021

Code review ACK 4844b74

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.

@Zero-1729
Copy link
Contributor

Zero-1729 commented Aug 13, 2021 •
edited
Loading

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.

theStack
theStack approved these changes Aug 13, 2021
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept and code-review ACK 4844b74 ♟️

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

$ ./src/bitcoind -signet -checkaddrman=10 -debug=addrman | grep "Addrman checks"
2021-08-13T13:14:23Z Addrman checks started: new 292, tried 18, total 310
2021-08-13T13:14:23Z Addrman checks completed successfully
2021-08-13T13:14:24Z Addrman checks started: new 292, tried 18, total 310
2021-08-13T13:14:24Z Addrman checks completed successfully
2021-08-13T13:14:24Z Addrman checks started: new 292, tried 18, total 310
2021-08-13T13:14:24Z Addrman checks completed successfully
2021-08-13T13:14:29Z Addrman checks started: new 292, tried 18, total 310
2021-08-13T13:14:29Z Addrman checks completed successfully
2021-08-13T13:14:34Z Addrman checks started: new 292, tried 18, total 310
2021-08-13T13:14:34Z Addrman checks completed successfully
...

@fanquake fanquake merged commit adf9bcf into bitcoin:master Aug 14, 2021
@jonatack jonatack deleted the log-addrman-consistency-checks branch August 14, 2021 07:39
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 15, 2021
@fanquake
Merge bitcoin#22696: p2p: log addrman consistency checks
5218da8 
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@theStack theStack theStack approved these changes

@Zero-1729 Zero-1729 Zero-1729 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jonatack @jnewbery @Zero-1729 @theStack @fanquake @DrahtBot