Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

log: improve addrman logging #22839

Merged
merged 1 commit into from
Oct 21, 2021
Merged

log: improve addrman logging #22839

merged 1 commit into from
Oct 21, 2021

Conversation

mzumsande
Copy link
Contributor

@mzumsande mzumsande commented Aug 30, 2021 •
edited
Loading

The addrman helper functions GetNewBucket() and GetTriedBucket()

  1. log into the wrong category (BCLog::NET instead of BCLog::ADDRMAN)
  2. log too unspecifically - especially GetTriedBucket() gets called from many different places (e.g. Check_(), Serialize()), it seems sufficient to me logging these when moving an address from new to tried. Running a node with -checkaddrman=1and net logging currently results in a lot of repetitive log entries.

This PR moves these log entries to Add_() and Good_() and also adds logging for Select_() (allowing statistics about New/Tried success probabilities), GetAddr_(), ClearNew() and MakeTried().

@theStack
Copy link
Contributor

Concept ACK

jnewbery
jnewbery reviewed Aug 31, 2021
View reviewed changes
Copy link
Contributor

@jnewbery jnewbery left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code review ACK 632e648

theStack
theStack approved these changes Sep 2, 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.

Code-review ACK 632e648

@practicalswift
Copy link
Contributor

Concept ACK

@DrahtBot DrahtBot mentioned this pull request Sep 7, 2021
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 7, 2021 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@DrahtBot DrahtBot mentioned this pull request Sep 11, 2021
Merged
naumenkogs
naumenkogs reviewed Sep 17, 2021
View reviewed changes
src/addrman.cpp Outdated Show resolved Hide resolved
@naumenkogs
Copy link
Member

ACK 632e648

@Bossday Bossday mentioned this pull request Sep 21, 2021
Closed
This was referenced Oct 1, 2021
Merged
Merged
@mzumsande
Copy link
Contributor Author

Rebased and addressed feedback by @naumenkogs .

theStack
theStack approved these changes Oct 5, 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.

re-ACK 2ac363a

vasild
vasild approved these changes Oct 6, 2021
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK 2ac363a

Some non-blocker comments below.

src/addrman.cpp Outdated Show resolved Hide resolved
src/addrman.cpp Outdated Show resolved Hide resolved
src/addrman.cpp Outdated Show resolved Hide resolved
src/addrman.cpp Show resolved Hide resolved
@jnewbery
Copy link
Contributor

jnewbery commented Oct 6, 2021

utACK 2ac363a

@naumenkogs
Copy link
Member

naumenkogs commented Oct 6, 2021 •
edited by fanquake
Loading

ACK 2ac363a, but it'd be great to apply what vasild suggested.

@fanquake
Copy link
Member

fanquake commented Oct 7, 2021 •
edited
Loading

Given this change is quite small, and re-ACKing is easy, I'd suggest addressing the outstanding review comments before we merge this. Rather than having a followup to change the same lines again.

Also @naumenkogs I edited your ACK to remove the @ mention, otherwise it would have ended up in the merge message.

@mzumsande
Copy link
Contributor Author

Makes sense of course, I'll push an update this evening.

@mzumsande mzumsande force-pushed the 202108_log_addrman branch 2 times, most recently from 8706b5d to 2ceab68 Compare October 7, 2021 23:47
@mzumsande
Copy link
Contributor Author

I addressed @vasild's suggestions, thanks for the review!
Note that now this PR introduces more additional BCLog::ADDRMAN log calls. Although I originally had the intention to only log from the functions belonging to the public interface, I think that the entries in ClearNew and MakeTried can be helpful for debugging.

vasild
vasild approved these changes Oct 8, 2021
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK 2ceab68

src/addrman.cpp Outdated Show resolved Hide resolved
naumenkogs
naumenkogs reviewed Oct 8, 2021
View reviewed changes
src/addrman.cpp Outdated Show resolved Hide resolved
naumenkogs
naumenkogs reviewed Oct 8, 2021
View reviewed changes
src/addrman.cpp Outdated Show resolved Hide resolved
@naumenkogs
Copy link
Member

ACK 2ceab68

1 similar comment
@jnewbery
Copy link
Contributor

jnewbery commented Oct 8, 2021

ACK 2ceab68

@mzumsande
Copy link
Contributor Author

Latest push: I added the bucket position everywhere where only the bucket was logged before and changed to a consistent notation new[i][j], tried[i][j].

vasild
vasild reviewed Oct 13, 2021
View reviewed changes
src/addrman.cpp Outdated Show resolved Hide resolved
vasild
vasild approved these changes Oct 14, 2021
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK e1d1788

Thanks!

jnewbery
jnewbery reviewed Oct 15, 2021
View reviewed changes
src/addrman.cpp Outdated Show resolved Hide resolved
jnewbery
jnewbery reviewed Oct 15, 2021
View reviewed changes
Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

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

ACK e1d1788

One non-essential suggestion inline.

vasild
vasild approved these changes Oct 19, 2021
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK b65a25a

@jnewbery
Copy link
Contributor

ACK b65a25a

@fanquake fanquake merged commit 69986de into bitcoin:master Oct 21, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 21, 2021
@fanquake
Merge bitcoin#22839: log: improve addrman logging
2f6538e 
b65a25a log: improve addrman logging (Martin Zumsande)

Pull request description:

  The addrman helper functions `GetNewBucket()` and `GetTriedBucket()`
  1) log into the wrong category (`BCLog::NET` instead of `BCLog::ADDRMAN`)
  2)  log too unspecifically - especially `GetTriedBucket()` gets called from many  different places (e.g. `Check_()`, `Serialize()`), it seems sufficient to me logging these when moving an address from new to tried. Running a node with `-checkaddrman=1`and net logging currently results in a lot of repetitive log entries.

  This PR moves these log entries to `Add_()` and `Good_()` and also adds logging for `Select_()` (allowing statistics about New/Tried success probabilities), `GetAddr_()`, `ClearNew()` and `MakeTried()`.

ACKs for top commit:
  jnewbery:
    ACK b65a25a
  vasild:
    ACK b65a25a

Tree-SHA512: 90ab0f64eb44b7388a198efccb613577b74989fea73194bda7de8bfbd50bdb19127cb12f5ec645c7859afdb89290614a79e255f3af0a63a58d4f21aa8fe7b696
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 21, 2022
@mzumsande @Fabcien
log: improve addrman logging
1136c0b 
Summary:
```

The addrman helper functions GetNewBucket() and GetTriedBucket()

    log into the wrong category (BCLog::NET instead of BCLog::ADDRMAN)
    log too unspecifically - especially GetTriedBucket() gets called from many different places (e.g. Check_(), Serialize()), it seems sufficient to me logging these when moving an address from new to tried. Running a node with -checkaddrman=1and net logging currently results in a lot of repetitive log entries.

This PR moves these log entries to Add_() and Good_() and also adds logging for Select_() (allowing statistics about New/Tried success probabilities), GetAddr_(), ClearNew() and MakeTried().
```

Backport of [[bitcoin/bitcoin#22839 | core#22839]].

Depends on D12338.

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D12339
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
@mzumsande mzumsande deleted the 202108_log_addrman branch November 3, 2022 00:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jnewbery jnewbery jnewbery left review comments

@maflcko maflcko maflcko left review comments

@theStack theStack theStack approved these changes

@vasild vasild vasild approved these changes

@amitiuttarwar amitiuttarwar Awaiting requested review from amitiuttarwar

@naumenkogs naumenkogs Awaiting requested review from naumenkogs

Assignees
No one assigned
Labels
P2P Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@mzumsande @theStack @practicalswift @DrahtBot @naumenkogs @jnewbery @fanquake @vasild @maflcko