eth/protocols/eth: fix deadlock when re-requesting partial receipts - #35537
eth/protocols/eth: fix deadlock when re-requesting partial receipts#355370xSHKWON wants to merge 3 commits into
Conversation
|
I thought about this again, and I think we need a lower bound on the response size for those incomplete receipt responses. I am worried about the case where the sender sends us small incomplete responses multiple times. This was prevented before because we shared the sent time field across subsequent requests. |
|
Agreed. The cause is It's zero-progress too: Two fixes without a magic constant:
|
|
@rjl493456442 What do you think about this ? |
When an eth/70 receipts response arrives with LastBlockIncomplete set, requestPartialReceipts held receiptBufferLock across dispatchRequest, which blocks until the dispatcher loop accepts the request. If the dispatcher is concurrently handling a cancellation for that peer (requester timeout or shutdown), its cancel branch acquires the same receiptBufferLock — an AB-BA deadlock. The dispatcher waits for the lock the re-request holds; the re-request waits to send on the channel whose only reader is the dispatcher. The dispatcher loop never runs again, so every request on that peer stalls permanently and the sync slot is never released. The race detector stays quiet — a lock-ordering problem, not a data race.
The fix releases the lock before sending, and sends the follow-up directly (tracker.Track + p2p.Send, the same pattern RequestPayload and RequestTxs already use) instead of going through the dispatcher. The follow-up reuses the original request ID: in the dispatcher’s pending map the original request and its sink stay in place, so the completed response is delivered to the original requester, while the tracker entry for the ID — just fulfilled by the partial response — is re-registered for the follow-up.
I didn’t add a committed test: driving the interleaving deterministically needs sleeps and reaches into peer internals, so it would be flaky in CI. Two further checks with the same harness:
Merely releasing the lock before dispatchRequest, without sending directly, trades the deadlock for an equivalent permanent stall — if the cancellation lands first the re-request registers itself in pending, and a later response then blocks forever on its nil sink (reader hung in 10 of 20 rounds; sending directly, 0 of 20). That is why the fix bypasses the dispatcher rather than only moving the unlock.
Normal flow is intact: with the original still pending, the follow-up is sent with the expected FirstBlockReceiptIndex and hash list, and the completing response reaches the original requester’s sink. (TestGetBlockPartialReceipts covers only the serving side, so this client path had no existing coverage.)
go test ./eth/protocols/eth/... ./eth/downloader/... ./eth/, with and without -race, passes. Happy to share the harness if it’s useful for review.