fix: wait for send buffer space on p2p request responses - #3903
Conversation
Outbound peer messages use a bounded channel with try_send that drops when full. That is fine for broadcasts, but silently dropping a GetTransaction/GetBlock response leaves the peer without data and can stall large-tx propagation after kernel-hash advertise. Use a short blocking wait for Consumed::Response, log type/size on drop/timeout, and keep try_send for unsolicited traffic. Closes mimblewimble#3392
wiesche89
left a comment
wiesche89
left a comment
There was a problem hiding this comment.
This fixes a plausible response drop path, but I’m not sure it fully establishes the cause of #3392. The issue shows one GetTransaction request on each of eight separate peer connections, while each connection has its own 100 slot queue, and the logs do not show that any queue was full. Could we use Refs #3392 unless the large transaction case is reproduced with this fix?
| "conn_handle: send_response timed out after {:?}, dropping {:?} ({} bytes)", | ||
| RESPONSE_SEND_TIMEOUT, msg_type, body_len | ||
| ); | ||
| return Ok(()); |
There was a problem hiding this comment.
After the timeout the requested response is still dropped, but the method reports success and keeps the congested peer connected. Could this return Error::Send so the connection is closed and the requester can retry elsewhere?
| /// How long to wait when sending a *response* to a peer request. | ||
| /// Request/response traffic (e.g. full tx after GetTransaction) must not be | ||
| /// silently dropped when the outbound queue is briefly full — that is a likely | ||
| /// cause of large txs never completing broadcast (#3392). |
| } | ||
| pending = msg; | ||
| // Yield so the writer thread can drain the channel. | ||
| thread::sleep(Duration::from_millis(10)); |
There was a problem hiding this comment.
This loop runs inside the peer reader and does not check stopped, so shutdown or banning a congested peer can wait out the full five seconds. Could ConnHandle carry the existing stop flag and abort the wait here?
| let start = Instant::now(); | ||
| handle.send_response(dummy_msg()).unwrap(); | ||
| assert!( | ||
| start.elapsed() >= Duration::from_millis(40), |
There was a problem hiding this comment.
Could we make these tests outcome based rather than timing based? Please verify that send actually drops the second message and that send_response delivers it, then cover timeout and stop separately. The 40 ms assertion can race with thread scheduling.
|
|
||
| /// Serialized body length in bytes. | ||
| pub fn body_len(&self) -> usize { | ||
| self.body.len() |
There was a problem hiding this comment.
This excludes attachments, so a dropped TxHashSetArchive can be logged as a tiny response even when the attachment is huge. Could the log say “body bytes” or include the attachment size?
2 participants
Summary
Large txs were accepted to the local pool and advertised by kernel hash, peers issued
GetTransaction, but full-tx responses could be silently dropped when the outbound peer send channel was full (try_send+ drop).That matches the #3392 symptoms: 8 peers request the full tx after the kernel hash, then the large tx never completes propagation.
Changes
try_sendfor unsolicited traffic (broadcasts, stems, etc.)ConnHandle::send_responsethat waits up to 5s for queue space before droppingsend_responseforConsumed::Response(answers to GetTransaction / GetBlock / etc.)send, wait-for-space forsend_responseCloses #3392
Test plan
cargo test -p grin_p2p --lib conn::tests