HomePhabricator

Rework receive buffer pushback

Description

Rework receive buffer pushback

Summary:
It has been observed that if both sides of a P2P connection have a significant amount of data to send, a stall can occur, where both try to drain their own send queue before trying to receive. The same issue seems to apply to the current Bitcoin Core codebase, though I don't know whether it's a frequent issue for us.

The core issue is that whenever our optimistic send fails to fully send a message, we do subsequently not even select() for receiving; if it then turns out that sending is not possible either, no progress is made at all. To address this, the solution used in this PR is to still select() for both sending and receiving when an optimistic send fails, but skip receiving if sending succeeded, and (still) doesn't fully drain the send queue.

This is a significant reduction in how aggressive the "receive pushback" mechanism is, because now it will only mildly push back while sending progress is made; if the other side stops receiving entirely, the pushback disappears. I don't think that's a serious problem though:

  • We still have a pushback mechanism at the application buffer level (when the application receive buffer overflows, receiving is paused until messages in the buffer get processed; waiting on our own net_processing thread, not on the remote party).
  • There are cases where the existing mechanism is too aggressive; e.g. when the send queue is non-empty, but tiny, and can be sent with a single send() call. In that case, I think we'd prefer to still receive within the same processing loop of the network thread.

Co-authored-by: Anthony Towns <aj@erisian.com.au>

core#28287:

rpc: add test-only sendmsgtopeer rpc

This rpc can be used when we want a node to send a message, but
cannot use a python P2P object, for example for testing of low-level
net transport behavior.

test: add basic tests for sendmsgtopeer to rpc_net.py

test: add functional test for deadlock situation

This is a backport of core#27981 and core#28287

Test Plan:
ninja all check-all

Test that this passes now and the same functional test sometimes fails before this commit (>33% failure rate on my machine)
for i in {1..100}; do ~/dev/bitcoin-abc/build/test/functional/test_runner.py p2p_net_deadlock; done

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Sintayew4, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D17138

Details

Provenance
Pieter Wuille <pieter@wuille.net>Authored on Jun 13 2023, 18:20
PiRKCommitted on Tue, Nov 19, 21:04
PiRKPushed on Tue, Nov 19, 21:04
Reviewer
Restricted Project
Differential Revision
D17138: Rework receive buffer pushback
Parents
rABC6653ee9ba241: [e.cash] Remove tagline about Avalanche
Branches
Unknown
Tags
Unknown