Skip to content

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

Merged
maflcko merged 1 commit intobitcoin:masterfrom
maflcko:2210-p2p-less-pointers-🍟
Dec 8, 2022

Hidden character warning

The head ref may contain hidden characters: "2210-p2p-less-pointers-\ud83c\udf5f"
Merged

refactor: Pass reference to last header, not pointer#26378
maflcko merged 1 commit intobitcoin:masterfrom
maflcko:2210-p2p-less-pointers-🍟

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

dergoegge commented Oct 25, 2022

Concept ACK

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

john-moffett commented Nov 1, 2022

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