Skip to content

refactor: Pass reference to last header, not pointer #26378

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 1 commit into from
Dec 8, 2022
Merged

refactor: Pass reference to last header, not pointer #26378

merged 1 commit into from
Dec 8, 2022

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Oct 24, 2022

It is never a nullptr, otherwise an assertion would fire in UpdatePeerStateForReceivedHeaders.

Passing a reference makes the code easier to read and less brittle.

refactor: Pass reference to last header, not pointer
fa579f3 
It is never a nullptr, otherwise an assertion would fire in
UpdatePeerStateForReceivedHeaders.

Passing a reference makes the code easier to read and less brittle.
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 24, 2022 •
edited
Loading

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

Reviews

See the guideline for information on the review process.

Type Count Reviewers
ACK 2 aureleoules, john-moffett
Concept ACK 1 dergoegge

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26140 (refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer by dergoegge)
  • #25268 (refactor: Introduce EvictionManager by dergoegge)
  • #24571 (p2p: Prevent block index fingerprinting by sending additional getheaders messages by dergoegge)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

This was referenced Oct 24, 2022
Merged
Closed
@dergoegge
Copy link
Member

Concept ACK

@DrahtBot DrahtBot mentioned this pull request Oct 29, 2022
Closed
@john-moffett
Copy link
Contributor

ACK fa579f3

Code review and rebased on master. Built/tests/ran bitcoind. Agree with using assert instead of Assume. The renamed parameters are clearer.

I don't know the policy on unrelated spelling corrections, but maybe this is a good place to fix the log line at 2867:

bitcoin/src/net_processing.cpp

Line 2867 in fa579f3

LogPrint(BCLog::NET, "more getheaders (%d) to end to peer=%d (startheight:%d)\n",

I think it should read "to send to" rather than "to end to".

@maflcko
Copy link
Member Author

maflcko commented Nov 1, 2022

"to end" is correct, meaning "to their chain tip"

aureleoules
aureleoules approved these changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@aureleoules aureleoules left a comment

Choose a reason for hiding this comment

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

ACK fa579f3

@maflcko maflcko merged commit 5126e62 into bitcoin:master Dec 8, 2022
@maflcko maflcko deleted the 2210-p2p-less-pointers-🍟 branch December 8, 2022 16:08
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 8, 2022
Merge bitcoin#26378: refactor: Pass reference to last header, not poi…
fdda07d 
…nter
@bitcoin bitcoin locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@aureleoules aureleoules aureleoules approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@maflcko @DrahtBot @dergoegge @john-moffett @aureleoules