Skip to content

net: Make p2p recv buffer timeout 20 minutes for all peers #20651

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 16, 2020
Merged

net: Make p2p recv buffer timeout 20 minutes for all peers #20651

merged 1 commit into from
Dec 16, 2020

Conversation

jnewbery
Copy link
Contributor

The timeout interval for the send and recv buffers was changed from 90
minutes to 20 minutes in commit f1920e8 in 2013, except for peers that
did not support the pong message (where the recv buffer timeout remained
at 90 minutes). A few observations:

  • for peers that support BIP 31 (pong messages), this recv buffer
    timeout is almost redundant with the ping timeout. We send a ping
    message every two minutes, and set a timeout of twenty minutes to
    receive the pong response. If the recv buffer was really timing out,
    then the pong response would also time out.
  • BIP 31 is supported by all nodes of p2p version 60000 and higher, and
    has been in widespread use since 2013. I'd be very surprised if there
    are many nodes on the network that don't support pong messages.
  • The recv buffer timeout is not specified in any p2p BIP. We're free to
    set it at any value we want.
  • A peer that doesn't support BIP 31 and hasn't sent any message to us
    at all in 90 minutes is unlikely to be useful for us, and is more likely
    to be evicted AttemptToEvictConnection() since it'll have the worst
    possible ping time and isn't providing blocks/transactions.

Therefore, we remove this check, and set the recv buffer timeout to 20
minutes for all peers. This removes the final p2p version dependent
logic from the net layer, so all p2p version data can move into the
net_processing layer.

Alternative approaches:

  • Set the recv buffer timeout to 90 minutes for all peers. This almost
    wouldn't be a behaviour change at all (pre-BIP 31 peers would still
    have the same recv buffer timeout, and we can't ever reach a recv buffer
    timeout higher than 21 minutes for post-BIP31 peers, because the pong
    timeout would be hit first).
  • Stop supporting peers that don't support BIP 31. BIP 31 has been in
    use since 2012, and implementing it is trivial.

@jnewbery
[net] Make p2p recv buffer timeout 20 minutes for all peers
ea36a45 
The timeout interval for the send and recv buffers was changed from 90
minutes to 20 minutes in commit f1920e8 in 2013, except for peers that
did not support the pong message (where the recv buffer timeout remained
at 90 minutes). A few observations:

- for peers that support BIP 31 (pong messages), this recv buffer
  timeout is almost redundant with the ping timeout. We send a ping
  message every two minutes, and set a timeout of twenty minutes to
  receive the pong response. If the recv buffer was really timing out,
  then the pong response would also time out.
- BIP 31 is supported by all nodes of p2p version 60000 and higher, and
  has been in widespread use since 2013. I'd be very surprised if there
  are many nodes on the network that don't support pong messages.
- The recv buffer timeout is not specified in any p2p BIP. We're free to
  set it at any value we want.
- A peer that doesn't support BIP 31 and hasn't sent any message to us
  at all in 90 minutes is unlikely to be useful for us, and is more likely
  to be evicted AttemptToEvictConnection() since it'll have the worst
  possible ping time and isn't providing blocks/transactions.

Therefore, we remove this check, and sent the recv buffer timeout to 20
minutes for all peers. This removes the final p2p version dependent
logic from the net layer, so all p2p version data can move into the
net_processing layer.

Alternative approaches:

- Set the recv buffer timeout to 90 minutes for all peers. This almost
  wouldn't be a behaviour change at all (pre-BIP 31 peers would still
  have the same recv buffer timeout, and we can't ever reach a recv buffer
  timeout higher than 21 minutes for post-BIP31 peers, because the pong
  timeout would be hit first).
- Stop supporting peers that don't support BIP 31. BIP 31 has been in
  use since 2012, and implementing it is trivial.
@fanquake fanquake added the P2P label Dec 14, 2020
@jnewbery jnewbery mentioned this pull request Dec 14, 2020
Closed
16 tasks
@maflcko
Copy link
Member

maflcko commented Dec 14, 2020

review ACK ea36a45

@promag
Copy link
Contributor

promag commented Dec 14, 2020

Code review ACK ea36a45.

nit, commit is from 2014.

@jnewbery
Copy link
Contributor Author

nit, commit is from 2014.

Good catch. I'll update the commit message if I need to retouch this PR.

@practicalswift
Copy link
Contributor

Concept ACK

While touch the timeout handling, consider moving these log messages to BCLog::NET:

$ git grep 'LogPrintf(.*socket.*timeout:'
src/net.cpp:            LogPrintf("socket sending timeout: %is\n", nTime - pnode->nLastSend);
src/net.cpp:            LogPrintf("socket receive timeout: %is\n", nTime - pnode->nLastRecv);

These two are among the least informative unconditional log messages we currently have, and they are both peer triggerable (in low volume luckily) :)

@jnewbery
Copy link
Contributor Author

While touch the timeout handling, consider moving these log messages

I'd prefer to leave that for a future PR to keep this one focused.

jonatack
jonatack reviewed Dec 15, 2020
View reviewed changes
Copy link
Member

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

Code review ACK ea36a45

@practicalswift
Copy link
Contributor

cr ACK ea36a45: patch looks correct

@ajtowns
Copy link
Contributor

ajtowns commented Dec 15, 2020

ACK ea36a45

Removes special case for old peers that don't seem to be around, and 20min timeout vs 90min timeout should be fine for any such peers anyway. Only touches one line of code so hopefully shouldn't interfere with any other PRs in the works. Don't think it has any denial-of-service impact; buggy nodes all seem like they'll announce bip31 compliant version numbers anyway, and people doing deliberate attacks can just do ping/pong to keep the connection open anyway.

@sipa
Copy link
Member

sipa commented Dec 15, 2020

utACK ea36a45

I don't see any downsides to simplifying this.

@sdaftuar
Copy link
Member

Only downside I could think of is if there's some (old) wallet software someone might be using, where this change could cause more frequent disconnects?

Presumably such software would have had to deal with the 90 minute timeout anyway though, so changing it to 20 minutes seems unlikely to be a fundamental problem.

Concept ACK.

@maflcko maflcko merged commit ae9ee5b into bitcoin:master Dec 16, 2020
@jnewbery jnewbery deleted the 2020-12-recv-buffer-timeout branch December 16, 2020 20:02
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 17, 2020
Merge bitcoin#20651: net: Make p2p recv buffer timeout 20 minutes for…
61f7e7f 
… all peers

ea36a45 [net] Make p2p recv buffer timeout 20 minutes for all peers (John Newbery)

Pull request description:

  The timeout interval for the send and recv buffers was changed from 90
  minutes to 20 minutes in commit f1920e8 in 2013, except for peers that
  did not support the pong message (where the recv buffer timeout remained
  at 90 minutes). A few observations:

  - for peers that support BIP 31 (pong messages), this recv buffer
    timeout is almost redundant with the ping timeout. We send a ping
    message every two minutes, and set a timeout of twenty minutes to
    receive the pong response. If the recv buffer was really timing out,
    then the pong response would also time out.
  - BIP 31 is supported by all nodes of p2p version 60000 and higher, and
    has been in widespread use since 2013. I'd be very surprised if there
    are many nodes on the network that don't support pong messages.
  - The recv buffer timeout is not specified in any p2p BIP. We're free to
    set it at any value we want.
  - A peer that doesn't support BIP 31 and hasn't sent any message to us
    at all in 90 minutes is unlikely to be useful for us, and is more likely
    to be evicted AttemptToEvictConnection() since it'll have the worst
    possible ping time and isn't providing blocks/transactions.

  Therefore, we remove this check, and set the recv buffer timeout to 20
  minutes for all peers. This removes the final p2p version dependent
  logic from the net layer, so all p2p version data can move into the
  net_processing layer.

  Alternative approaches:

  - Set the recv buffer timeout to 90 minutes for all peers. This almost
    wouldn't be a behaviour change at all (pre-BIP 31 peers would still
    have the same recv buffer timeout, and we can't ever reach a recv buffer
    timeout higher than 21 minutes for post-BIP31 peers, because the pong
    timeout would be hit first).
  - Stop supporting peers that don't support BIP 31. BIP 31 has been in
    use since 2012, and implementing it is trivial.

ACKs for top commit:
  MarcoFalke:
    review ACK ea36a45
  promag:
    Code review ACK ea36a45.
  practicalswift:
    cr ACK ea36a45: patch looks correct
  ajtowns:
    ACK ea36a45
  sipa:
    utACK ea36a45
  jonatack:
    Code review ACK ea36a45

Tree-SHA512: df290bb32d2b5d9e59a0125bb215baa92787f9d01542a7437245f1c478c7f9b9831e5f170d3cd0db2811e1b11b857b3e8b2e03376476b8302148e480d81aab19
@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
1 more reviewer

@jonatack jonatack jonatack left review comments

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

Successfully merging this pull request may close these issues.
9 participants
@jnewbery @maflcko @promag @practicalswift @ajtowns @sipa @sdaftuar @jonatack @fanquake