log: improve addrman logging by mzumsande · Pull Request #22839 · bitcoin/bitcoin

mzumsande · 2021-08-30T22:09:02Z

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().

theStack · 2021-08-31T09:23:02Z

Concept ACK

jnewbery

Code review ACK 632e648fd8a4d7b71be45139a51c75ffff305a1b

theStack

Code-review ACK 632e648fd8a4d7b71be45139a51c75ffff305a1b

practicalswift · 2021-09-04T13:48:36Z

Concept ACK

DrahtBot · 2021-09-07T20:53:32Z

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

Conflicts

No conflicts as of last run.

src/addrman.cpp

naumenkogs · 2021-09-17T09:34:39Z

ACK 632e648fd8a4d7b71be45139a51c75ffff305a1b

mzumsande · 2021-10-05T17:20:49Z

Rebased and addressed feedback by @naumenkogs .

theStack

re-ACK 2ac363a1f731b06d3f3c8b2fd7a02d0f846bb39b

vasild

ACK 2ac363a1f731b06d3f3c8b2fd7a02d0f846bb39b

Some non-blocker comments below.

src/addrman.cpp

jnewbery · 2021-10-06T09:49:59Z

utACK 2ac363a1f731b06d3f3c8b2fd7a02d0f846bb39b

naumenkogs · 2021-10-06T10:23:44Z

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

fanquake · 2021-10-07T05:42:04Z

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 · 2021-10-07T07:28:58Z

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

mzumsande · 2021-10-07T23:59:57Z

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

ACK 2ceab68079601072a8ab6f084e4b3123025d0463

src/addrman.cpp

naumenkogs · 2021-10-08T08:15:13Z

ACK 2ceab68079601072a8ab6f084e4b3123025d0463

jnewbery · 2021-10-08T09:14:51Z

ACK 2ceab68079601072a8ab6f084e4b3123025d0463

mzumsande · 2021-10-11T20:47:18Z

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

src/addrman.cpp

vasild

ACK e1d1788027e1339b0013fc8787e0251862d85117

Thanks!

src/addrman.cpp

jnewbery

ACK e1d1788027e1339b0013fc8787e0251862d85117

One non-essential suggestion inline.

vasild

ACK b65a25a

jnewbery · 2021-10-20T08:38:52Z

ACK b65a25a

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

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

DrahtBot added P2P Utils/log/libs labels Aug 30, 2021

fanquake requested review from amitiuttarwar and jnewbery August 31, 2021 00:38

jnewbery reviewed Aug 31, 2021

View reviewed changes

theStack approved these changes Sep 2, 2021

View reviewed changes

DrahtBot mentioned this pull request Sep 7, 2021

net: Encapsulate asmap in NetGroupManager #22910

Merged

DrahtBot mentioned this pull request Sep 11, 2021

[p2p] Pimpl AddrMan to abstract implementation details #22950

Merged

naumenkogs reviewed Sep 17, 2021

View reviewed changes

src/addrman.cpp Outdated Show resolved Hide resolved

Bossday mentioned this pull request Sep 21, 2021

. #23055

Closed

This was referenced Oct 1, 2021

Make CAddrman::Select_ select buckets, not positions, first #23140

Merged

[fuzz] Use public methods in addrman fuzz tests #23053

Merged

DrahtBot added the Needs rebase label Oct 5, 2021

mzumsande force-pushed the 202108_log_addrman branch from 632e648 to 2ac363a Compare October 5, 2021 17:13

DrahtBot removed the Needs rebase label Oct 5, 2021

theStack approved these changes Oct 5, 2021

View reviewed changes

vasild approved these changes Oct 6, 2021

View reviewed changes

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 Outdated Show resolved Hide resolved

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

vasild approved these changes Oct 8, 2021

View reviewed changes

src/addrman.cpp Outdated Show resolved Hide resolved

naumenkogs reviewed Oct 8, 2021

View reviewed changes

src/addrman.cpp Outdated Show resolved Hide resolved

naumenkogs reviewed Oct 8, 2021

View reviewed changes

src/addrman.cpp Outdated Show resolved Hide resolved

mzumsande force-pushed the 202108_log_addrman branch from 2ceab68 to 12350e1 Compare October 11, 2021 20:39

vasild reviewed Oct 13, 2021

View reviewed changes

src/addrman.cpp Outdated Show resolved Hide resolved

mzumsande force-pushed the 202108_log_addrman branch from 12350e1 to e1d1788 Compare October 13, 2021 22:38

vasild approved these changes Oct 14, 2021

View reviewed changes

jnewbery reviewed Oct 15, 2021

View reviewed changes

src/addrman.cpp Outdated Show resolved Hide resolved

jnewbery reviewed Oct 15, 2021

View reviewed changes

log: improve addrman logging

b65a25a

mzumsande force-pushed the 202108_log_addrman branch from e1d1788 to b65a25a Compare October 18, 2021 22:22

vasild approved these changes Oct 19, 2021

View reviewed changes

fanquake requested a review from naumenkogs October 20, 2021 03:43

fanquake merged commit 69986de into bitcoin:master Oct 21, 2021

bitcoin locked and limited conversation to collaborators Oct 30, 2022

mzumsande deleted the 202108_log_addrman branch November 3, 2022 00:02

Conversation

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

Uh oh!

Uh oh!

theStack commented Aug 31, 2021

Uh oh!

jnewbery left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Sep 4, 2021

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

Uh oh!

naumenkogs commented Sep 17, 2021

Uh oh!

mzumsande commented Oct 5, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Oct 6, 2021

Uh oh!

naumenkogs commented Oct 6, 2021 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzumsande commented Oct 7, 2021

Uh oh!

mzumsande commented Oct 7, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

naumenkogs commented Oct 8, 2021

Uh oh!

jnewbery commented Oct 8, 2021

Uh oh!

mzumsande commented Oct 11, 2021

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Oct 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

mzumsande commented Aug 30, 2021 •

edited

Loading

jnewbery left a comment •

edited

Loading

DrahtBot commented Sep 7, 2021 •

edited

Loading

naumenkogs commented Oct 6, 2021 •

edited by fanquake

Loading

fanquake commented Oct 7, 2021 •

edited

Loading