Skip to content

doc: Update high-level addrman description #22576

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
Merged
merged 2 commits into from
Aug 4, 2021
Merged

doc: Update high-level addrman description #22576

merged 2 commits into from
Aug 4, 2021

Conversation

mzumsande
Copy link
Contributor

The high-level description of addrman has outdated information with respect to the eviction behavior, both for the New and Tried tables (at least since #5941) - this has confused me in the past.

This PR corrects this and also adds basic info about the bucket size and position.

@fanquake fanquake added the Docs label Jul 28, 2021
jnewbery
jnewbery reviewed Jul 29, 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. The current documentation is incorrect, and these new comments are clear. Thanks for documenting this!

@mzumsande mzumsande force-pushed the 202107_doc_addrman branch from d8e5aa7 to 004324c Compare July 29, 2021 22:18
jonatack
jonatack reviewed Jul 29, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

ACK; good idea, thanks for updating the documentation.

amitiuttarwar
amitiuttarwar approved these changes Jul 30, 2021
View reviewed changes
Copy link
Contributor

@amitiuttarwar amitiuttarwar left a comment

Choose a reason for hiding this comment

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

ACK 004324c thanks for correcting this!

so, funnily enough, I recently also noticed some incorrect addrman documentation- namely the description for Create(). If you're interested, feel free to cherry-pick that / any of the other addrman doc improvements that I have over here- amitiuttarwar@3b50703

lsilva01
lsilva01 approved these changes Jul 30, 2021
View reviewed changes
Copy link
Contributor

@lsilva01 lsilva01 left a comment

Choose a reason for hiding this comment

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

ACK 004324c

@jnewbery
Copy link
Contributor

ACK 004324c

(happy to reACK if you take @jonatack or @amitiuttarwar suggestions)

@mzumsande mzumsande force-pushed the 202107_doc_addrman branch from 004324c to 036d7ea Compare August 2, 2021 20:37
@mzumsande
Copy link
Contributor Author

Thanks for the reviews! I took all the suggestions on the high-level description.

@amitiuttarwar Good find with Create! For the scope of this PR, I took the short version of your correction, keeping the existing terminology ("entry") to stay in line with the rest of the documentation. I think a more general doc overhaul of the various other functions in addrman would better be a separate PR.

@amitiuttarwar
Copy link
Contributor

looks good, thanks for fixing these :)

reACK 036d7ea

@fanquake fanquake requested a review from jnewbery August 3, 2021 02:12
@fanquake
Copy link
Member

fanquake commented Aug 3, 2021

@lsilva01 want to re-ACK? (can't request a re-review).

jnewbery
jnewbery reviewed Aug 3, 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 036d7ea

One minor comment inline. Feel free to ignore.

@fanquake fanquake merged commit 3308c61 into bitcoin:master Aug 4, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 4, 2021
@fanquake
Merge bitcoin#22576: doc: Update high-level addrman description
b04bed5 
036d7ea doc: Correct description of CAddrMan::Create() (Amiti Uttarwar)
318176a doc: Update high-level addrman description (Martin Zumsande)

Pull request description:

  The high-level description of `addrman` has outdated information with respect to the eviction behavior, both for the New and Tried tables (at least since bitcoin#5941) - this has confused me in the past.

  This PR corrects this and also adds basic info about the bucket size and position.

ACKs for top commit:
  amitiuttarwar:
    reACK 036d7ea
  jnewbery:
    ACK 036d7ea

Tree-SHA512: 3f0635d765f5e580a1fae31187742a833cef66ef2286d40eeb28f2253521260038e16e5f1a65741464a2ddfdbeb5c0f1bc38bf73841e600639033d59c3c534e4
@mzumsande mzumsande deleted the 202107_doc_addrman branch August 27, 2021 16:08
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ajtowns ajtowns Awaiting requested review from ajtowns

4 more reviewers

@jnewbery jnewbery jnewbery left review comments

@jonatack jonatack jonatack left review comments

@amitiuttarwar amitiuttarwar amitiuttarwar approved these changes

@lsilva01 lsilva01 lsilva01 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mzumsande @jnewbery @amitiuttarwar @fanquake @jonatack @lsilva01