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

net: Send post-verack handshake messages at most once #20146

Merged
merged 1 commit into from
Oct 15, 2020
Merged

net: Send post-verack handshake messages at most once #20146

merged 1 commit into from
Oct 15, 2020

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Oct 14, 2020

There is no need to send SENDHEADERS and SENDCMPCT messages as a reply to each VERACK that is received. For alive checks, a PING/PONG can be used.

@sipa
Copy link
Member

sipa commented Oct 14, 2020

wont-hurt-ACK

@laanwj
Copy link
Member

laanwj commented Oct 14, 2020

Code review ACK fa1f6f2
This is how I'd expected it to work in the first place, so the change imo adheres to the principle of least surprise.

@laanwj laanwj added the P2P label Oct 14, 2020
@naumenkogs
Copy link
Member

ACK fa1f6f2

jonatack
jonatack reviewed Oct 14, 2020
View reviewed changes
Copy link
Contributor

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

Concept ACK fa1f6f2 this is the only code section that sets fCurrentlyConnected and fSuccessfullyConnected to true. Could add a test. I did not verify if this code is actually being called repeatedly post initial verack; was it?

hebasto
hebasto approved these changes Oct 14, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK fa1f6f2, I have reviewed the code and it looks OK, I agree it can be merged.

@fanquake fanquake merged commit 661fe5d into bitcoin:master Oct 15, 2020
@maflcko maflcko deleted the 2010-netPostVerackHandshake branch October 15, 2020 07:56
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 16, 2020
@fanquake
Merge bitcoin#20146: net: Send post-verack handshake messages at most…
267633c 
… once

fa1f6f2 net: Send post-verack handshake messages at most once (MarcoFalke)

Pull request description:

  There is no need to send `SENDHEADERS` and `SENDCMPCT` messages as a reply to each `VERACK` that is received. For alive checks, a `PING`/`PONG` can be used.

ACKs for top commit:
  jonatack:
    Concept ACK fa1f6f2 this is the only code section that sets `fCurrentlyConnected` and `fSuccessfullyConnected` to true. Could add a test. I did not verify if this code is actually being called repeatedly post initial verack; was it?
  hebasto:
    ACK fa1f6f2, I have reviewed the code and it looks OK, I agree it can be merged.
  naumenkogs:
    ACK fa1f6f2
  laanwj:
    Code review ACK fa1f6f2

Tree-SHA512: c841d5d3807254a49463bbcfac3b32881b34a9d3206899544c86322c20988e17ad2ae243cba227fd3825a914f0cb2584451edda2414aecee6d5e3f5a0636f08a
@sipa
Copy link
Member

sipa commented Oct 17, 2020

Arguably this was a protocol violation, so perhaps we want to backport?

@maflcko
Copy link
Member Author

maflcko commented Oct 17, 2020

Shouldn't matter much, but I certainly don't mind a backport, as we are doing a release anyway.

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 19, 2020
@fanquake
net: Send post-verack handshake messages at most once
6113b54 
Github-Pull: bitcoin#20146
Rebased-From: fa1f6f2
@fanquake
Copy link
Member

Have added this to #20166 for backporting to the 0.20 branch.

@fanquake fanquake mentioned this pull request Oct 19, 2020
Merged
maflcko pushed a commit that referenced this pull request Nov 18, 2020
Merge #20166: [0.20] Backports
a2fa11f 
7566af4 doc: Update data directory path comments (Hennadii Stepanov)
09261de util: Add StripRedundantLastElementsOfPath function (Hennadii Stepanov)
8ef0dac macOS deploy: use the new plistlib API (Jonas Schnelli)
314e795 build: fix mutex detection when building bdb on macOS (fanquake)
1f67a30 random: fixes read buffer resizing in RandAddSeedPerfmon (Ethan Heilman)
6113b54 net: Send post-verack handshake messages at most once (MarcoFalke)
bdf15d0 rpc: Adjust witness-tx deserialize error message (MarcoFalke)
731502a rpc: Properly deserialize txs with witness before signing (MarcoFalke)
ee0082b Avoid the use of abs64 in timedata (Pieter Wuille)
05bd0c2 docs: Correct description for getblockstats's txs field (Nadav Ivgi)

Pull request description:

  Backports the following PRs to the 0.20 branch:
  * #19777 - docs: Correct description for getblockstats's txs field
  * #19836 - rpc: Properly deserialize txs with witness before signing
  * #20080 - Strip any trailing `/` in -datadir and -blocksdir paths
  * #20082 - [bugfix] random: fixes read buffer to use min rather than max
  * #20141 - Avoid the use of abs64 in timedata
  * #20146 - net: Send post-verack handshake messages at most once
  * #20195 - build: fix mutex detection when building bdb on macOS
  * #20298 - macOS deploy: use the new plistlib API

  Will add additional commits as they become available.

ACKs for top commit:
  MarcoFalke:
    review ACK 7566af4 🗡

Tree-SHA512: add6bb978313c12c3e07bc232636ae9d1ab0edd0b816705c5c70eeb1cc04097165fd5e29d60c706886943ceb1f749a422020766b4aa2d23be51e9f839157a4bb
MarkLTZ pushed a commit to MarkLTZ/bitcoin that referenced this pull request Nov 21, 2020
@MarkLTZ
net: Send post-verack handshake messages at most once
ea258e3 
Github-Pull: bitcoin#20146
Rebased-From: fa1f6f2
MarkLTZ pushed a commit to MarkLTZ/bitcoin that referenced this pull request Nov 21, 2020
@MarkLTZ
Merge bitcoin#20166: [0.20] Backports
c61e643 
7566af4 doc: Update data directory path comments (Hennadii Stepanov)
09261de util: Add StripRedundantLastElementsOfPath function (Hennadii Stepanov)
8ef0dac macOS deploy: use the new plistlib API (Jonas Schnelli)
314e795 build: fix mutex detection when building bdb on macOS (fanquake)
1f67a30 random: fixes read buffer resizing in RandAddSeedPerfmon (Ethan Heilman)
6113b54 net: Send post-verack handshake messages at most once (MarcoFalke)
bdf15d0 rpc: Adjust witness-tx deserialize error message (MarcoFalke)
731502a rpc: Properly deserialize txs with witness before signing (MarcoFalke)
ee0082b Avoid the use of abs64 in timedata (Pieter Wuille)
05bd0c2 docs: Correct description for getblockstats's txs field (Nadav Ivgi)

Pull request description:

  Backports the following PRs to the 0.20 branch:
  * bitcoin#19777 - docs: Correct description for getblockstats's txs field
  * bitcoin#19836 - rpc: Properly deserialize txs with witness before signing
  * bitcoin#20080 - Strip any trailing `/` in -datadir and -blocksdir paths
  * bitcoin#20082 - [bugfix] random: fixes read buffer to use min rather than max
  * bitcoin#20141 - Avoid the use of abs64 in timedata
  * bitcoin#20146 - net: Send post-verack handshake messages at most once
  * bitcoin#20195 - build: fix mutex detection when building bdb on macOS
  * bitcoin#20298 - macOS deploy: use the new plistlib API

  Will add additional commits as they become available.

ACKs for top commit:
  MarcoFalke:
    review ACK 7566af4 🗡

Tree-SHA512: add6bb978313c12c3e07bc232636ae9d1ab0edd0b816705c5c70eeb1cc04097165fd5e29d60c706886943ceb1f749a422020766b4aa2d23be51e9f839157a4bb
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 17, 2021
@PiRK
net: Send post-verack handshake messages at most once
d0db96b 
Summary:
There is no need to send SENDHEADERS and SENDCMPCT messages as a reply to each VERACK that is received. For alive checks, a PING/PONG can be used.

This is a backport of [[bitcoin/bitcoin#20146 | core#20146]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10469
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonatack jonatack jonatack left review comments

@hebasto hebasto hebasto approved these changes

Assignees
No one assigned
Labels
P2P
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@maflcko @sipa @laanwj @naumenkogs @fanquake @jonatack @hebasto