Skip to content
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

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

Merged
merged 3 commits into from
Mar 19, 2021
Merged

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

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 c56f140 ♻️
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 c56f140

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

Couple of small style suggestions inline.

src/net_processing.cpp Show resolved Hide resolved
src/net_processing.cpp Outdated Show resolved Hide resolved
@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

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
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 7ed5210

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

🕵️ @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
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

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

@jnewbery jnewbery jnewbery left review comments

@theStack theStack theStack approved these changes

@sipa sipa sipa approved these changes

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