Skip to content

p2p: Clarify disconnect log message in ProcessGetBlockData, remove send bool #21235

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
maflcko merged 3 commits into from
Mar 19, 2021
Merged

p2p: Clarify disconnect log message in ProcessGetBlockData, remove send bool #21235

maflcko merged 3 commits into from
Mar 19, 2021

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Feb 19, 2021

theStack
theStack approved these changes Feb 20, 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.

ACK c56f1401b43d8235713745a35594e7e7c1b9f5e0 ♻️
Quite surprising that the PR where the second commit is taken from was never merged, at least IMHO getting rid of this unnecessary state variable is a definitive improvement w.r.t. readability.

@DrahtBot DrahtBot mentioned this pull request Feb 23, 2021
Merged
1 task
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 23, 2021 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

jnewbery
jnewbery reviewed Feb 23, 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.

utACK c56f1401b43d8235713745a35594e7e7c1b9f5e0

Thanks for doing this. I've also had a branch floating around to remove this variable.

Couple of small style suggestions inline.

@maflcko maflcko force-pushed the 2102-logDisconnect branch 2 times, most recently from fac5641 to fa55ab8 Compare February 23, 2021 10:38
@jnewbery
Copy link
Contributor

jnewbery commented Feb 23, 2021 •
edited
Loading

utACK fa55ab8

Very nice to remove the indenting in a separate commit 💯

@DrahtBot DrahtBot mentioned this pull request Feb 23, 2021
Merged
@practicalswift
Copy link
Contributor

practicalswift commented Feb 24, 2021

Concept ACK

Returning early here makes the code more robust and easier to reason about. Glad to see send go :)

@DrahtBot DrahtBot mentioned this pull request Mar 9, 2021
Merged
2 tasks
@maflcko maflcko force-pushed the 2102-logDisconnect branch from fa55ab8 to 7ed5210 Compare March 11, 2021 15:45
@maflcko
Copy link
Member Author

maflcko commented Mar 11, 2021

Rebased. Should be trivial to re-ACK

@jnewbery
Copy link
Contributor

jnewbery commented Mar 11, 2021 •
edited
Loading

utACK 7ed521036cfd9795c63aaab8a59dadc34999e79c

Checked the git range-diff. Only difference is things like using m_chainman.ActiveChain() instead of ::ChainActive() due to #21270.

@DrahtBot DrahtBot mentioned this pull request Mar 12, 2021
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 15, 2021

🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

MarcoFalke and others added 3 commits March 18, 2021 09:12
log: Clarify that block request below NODE_NETWORK_LIMITED_MIN_BLOCKS…
fae7c04 
… disconnects
@pstratem
net: Simplify ProcessGetBlockData execution by removing send flag.
fae77b9 
Setting the send flag to false can be replaced by simply returning.
style-only: Remove whitespace
fa81773 
Can be reviewed with --ignore-all-space
@maflcko maflcko force-pushed the 2102-logDisconnect branch from 7ed5210 to fa81773 Compare March 18, 2021 08:16
@maflcko
Copy link
Member Author

maflcko commented Mar 18, 2021

Rebased after conflict

sipa
sipa approved these changes Mar 19, 2021
View reviewed changes
Copy link
Member

@sipa sipa 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.

utACK fa81773

@jnewbery
Copy link
Contributor

jnewbery commented Mar 19, 2021

utACK fa81773

the range-diff was a bit messy so I just reviewed again from scratch.

@maflcko maflcko merged commit 3a12fdb into bitcoin:master Mar 19, 2021
@maflcko maflcko deleted the 2102-logDisconnect branch March 19, 2021 19:00
@adamjonas adamjonas mentioned this pull request Aug 4, 2022
Open
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sipa sipa sipa approved these changes

+2 more reviewers

@jnewbery jnewbery jnewbery left review comments

@theStack theStack theStack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P Refactoring Utils/log/libs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@maflcko @DrahtBot @jnewbery @practicalswift @theStack @sipa @pstratem