dkg: retry transient p2p send failures during ceremonies - #4684
dkg: retry transient p2p send failures during ceremonies#4684KaloyanTanev wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new retry implementation changes p2p_send_duration_seconds observation semantics under retries (per-attempt and excluding backoff) in a way that contradicts the metric’s documented “wall-clock call duration” meaning, and WithRetries should defensively clamp negative values.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds an opt-in retry mechanism to synchronous P2P sends (and send/receive), and wires it into DKG ceremony transports so transient outbound stream failures don’t abort an entire ceremony. It also improves error diagnostics by including the destination peer name in p2p.Send/SendReceive errors.
Changes:
- Add
p2p.WithRetriesplus a retry loop with exponential backoff, and refactorSend/SendReceiveinto retry wrappers + single-attempt helpers. - Attach
peerstructured field to key send-path errors for better diagnosability. - Add test infrastructure (
FlakyHost) and new tests validating retry behavior and peer-field presence, and enable retries in DKG transports (exchanger, bcast, pedersen board, frost round 1).
File summaries
| File | Description |
|---|---|
| testutil/random.go | Adds FlakyHost test helper to simulate transient NewStream failures. |
| p2p/sender.go | Implements WithRetries, retry loop/backoff, peer-tagged errors; refactors send functions for retrying. |
| p2p/sender_test.go | Adds tests for peer field presence in errors and for retry behavior in Send/SendReceive. |
| dkg/pedersen/board.go | Enables retries for pedersen board bundle broadcasts. |
| dkg/pedersen/board_test.go | Adds regression test ensuring board broadcasts survive transient send failures. |
| dkg/frostp2p.go | Enables retries for frost round-1 p2p sends. |
| dkg/exchanger.go | Wraps ParSigEx send function to retry during DKG exchanges. |
| dkg/exchanger_internal_test.go | Adds regression test ensuring exchanger survives transient send failures. |
| dkg/bcast/impl_test.go | Adds regression test ensuring reliable broadcast survives transient send failures. |
| dkg/bcast/helpers.go | Adds DKG bcast send retry constant/documentation. |
| dkg/bcast/client.go | Enables retries for signature requests and message sends in DKG bcast client. |
Review details
Suppressed comments (2)
p2p/sender.go:359
- send_duration_seconds is documented as the wall-clock duration of a Send/SendReceive call, but with retries this observation is now per-attempt (and excludes backoff time) since it lives inside sendReceive(). This can under-report caller-perceived latency and inflate sample counts when retries happen.
This issue also appears on line 425 of the same file.
defer func() {
sendDurations.WithLabelValues(PeerName(peerID), protoLabel, o.metricTopic).Observe(time.Since(tStart).Seconds())
}()
p2p/sender.go:427
- WithRetries wraps send() here, but send_duration_seconds is still observed inside send() (per attempt) and does not include retry backoff time; when retries happen this diverges from the metric's documented meaning as the wall-clock duration of a Send call.
return withRetries(ctx, o.retries, func() error {
return send(ctx, p2pNode, protoID, peerID, msg, o)
})
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4684 +/- ##
==========================================
+ Coverage 65.38% 65.44% +0.05%
==========================================
Files 247 247
Lines 29983 30022 +39
==========================================
+ Hits 19605 19648 +43
+ Misses 10377 10373 -4
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
Context cancellation is masked by stale transport errors, and final broadcast-send retry coverage is ineffective.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The newly retry-enabled FROST transport lacks direct transient-failure coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Per-attempt minute-long deadlines allow retries to exceed the intended ceremony timeout by several minutes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
dkg/bcast/client.go:136
- The final message send can now consume six 62-second attempt deadlines before returning an error. Because the normal DKG context has no configured deadline, one stalled peer delays ceremony failure by about six minutes rather than respecting the protocol timeout. Keep all retries for this phase within a single bounded deadline.
err := c.sendFunc(ctx, c.p2pNode, protocolIDMsg, pID, bcastMsg, p2p.WithSendTimeout(sendTimeout), p2p.WithRetries(sendRetries))
- Files reviewed: 14/14 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Stream creation remains outside the advertised total timeout budget in both send paths.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
p2p/sender.go:442
- As in
SendReceive, this deadline only reaches the stream afterNewStreamcompletes. An unbounded caller context therefore allows dialing or protocol negotiation to exceed the documented total send budget, which can still stall a ceremony beyond its configured timeout. Pass a context carrying this deadline into the retry attempts.
// The send timeout is the total budget for the call: all attempts share one deadline.
deadline := time.Now().Add(o.sendTimeout)
return withRetries(ctx, o.retries, deadline, func() error {
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Retry budgeting prevents retries after full-deadline stalls and can reset through the asynchronous relay retry path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
dkg/pedersen/board.go:268
SendAsyncwrapsp2p.Sendin the legacywithRelayRetry. If the retry sequence ends withnetwork.ErrResetorErrResourceScopeClosed, that wrapper invokesSendagain, which creates a fresh one-minute deadline and another six attempts. This path can therefore perform up to 12 attempts over roughly two minutes despiteWithRetries(5)and the documented total budget. Fold the relay retry into the same retry budget, or suppress the legacy wrapper whenWithRetriesis supplied.
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Retry deadlines do not bound stream setup, negative retry counts can panic, and response slicing rejects valid slow broadcast peers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
p2p/sender.go:235
WithRetries(-1)no longer behaves like zero retries: both send paths evaluateo.retries+1as zero when computing the per-attempt timeout and panic. Clamp the public option to a non-negative count.
opts.retries = retries
p2p/sender.go:471
- The per-attempt deadline is applied only after stream creation, while
sendreceives the overall context here. A stalled dial or protocol negotiation can consume the full budget on the first attempt and prevent any retry. Derive an attempt-scoped context fromattemptDLbefore callingsend.
return send(ctx, p2pNode, protoID, peerID, msg, o, attemptDeadline(attemptTimeout, deadline))
dkg/bcast/client.go:68
- This now divides the 62-second signature-request budget across six attempts, so each response gets only about 10.3 seconds. The server explicitly permits
checkMessageto run forreceiveTimeout(60 seconds), meaning a valid peer taking 11–60 seconds will time out on every attempt; the new slow-peer test's 8-second delay does not cover this range. Preserve the legal response-processing window while reserving retry slices for stream creation/write failures.
err := c.sendRecvFunc(ctx, c.p2pNode, pID, sigReq, sigResp, protocolIDSig, p2p.WithSendTimeout(sendTimeout), p2p.WithRetries(sendRetries))
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Retry deadline slicing rejects valid slow broadcast responses, and negative retry counts can panic.
Review details
Suppressed comments (2)
dkg/bcast/client.go:68
SendReceivedivides the 62s budget across all six attempts, so this signature request now gets only about 10.3s to receive each response. The server explicitly allowscheckMessageto run forreceiveTimeout(1 minute), and the new slow-peer test only waits 8s, so any valid peer taking 11–60s will have every attempt terminated and the ceremony will fail. Preserve the full allowed response-processing window while applying shorter retry deadlines only to stream creation/write (or otherwise distinguish stalled transport from a request that has already been delivered).
err := c.sendRecvFunc(ctx, c.p2pNode, pID, sigReq, sigResp, protocolIDSig, p2p.WithSendTimeout(sendTimeout), p2p.WithRetries(sendRetries))
p2p/sender.go:236
- A retry count of
-1now causes bothSendandSendReceiveto panic when they computesendTimeout / (retries+1). This was previously equivalent to zero retries, so clamp negative values when applying the option.
func WithRetries(retries int) func(*sendRecvOpts) {
return func(opts *sendRecvOpts) {
opts.retries = retries
}
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
SendReceive multiplies the documented total timeout by the number of attempts, allowing DKG broadcasts to stall for roughly six minutes.
Review details
Suppressed comments (1)
p2p/sender.go:358
WithSendTimeoutis documented as the total budget for all retries, but multiplying it by the attempt count makesSendReceiverun for up to(retries+1) * sendTimeout. For the DKG broadcast caller this turns the 62-second limit into roughly 6 minutes, reintroducing the ceremony stall that the total-budget change was meant to prevent. Keep one overallsendTimeoutdeadline; retries may use the remaining budget, while a response that consumes the full budget should end the operation rather than extend it.
deadline := time.Now().Add(attemptTimeout * time.Duration(o.retries+1))
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
SendReceive multiplies the configured total timeout across retries, allowing DKG broadcasts to stall for several minutes.
Review details
Suppressed comments (1)
p2p/sender.go:358
SendReceivemultiplies the configured timeout by the number of attempts, contradictingWithSendTimeout's total-budget contract. For DKG broadcast this turns the 62s limit into up to 372s (sendRetries == 5), reintroducing the multi-minute stall the retry budget was intended to avoid. Keep the overall deadline atnow + o.sendTimeout; if slow responses need special treatment, budget connection/write retries separately from the response wait rather than extending the whole call per retry.
deadline := time.Now().Add(attemptTimeout * time.Duration(o.retries+1))
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Addressed the suppressed comment from review 5117913950 in 969e27a — this converges the two competing constraints from the last rounds:
One-way |
|
There was a problem hiding this comment.
🔵 Needs a closer look
A stalled SendReceive request write can exhaust the entire timeout before any retry occurs.
Review details
Suppressed comments (1)
p2p/sender.go:359
SendReceivestill gives the first attempt the entire overall deadline. IfWriteMsgstalls until that deadline—the transient relay-stream failure this PR is intended to recover from—withRetrieshas no budget left and never opens a fresh stream. This leaves the bcast signature-request transport retry-enabled only for fast failures. Bound dialing and request writing per attempt, then extend only the response read to the overall deadline so legitimate slow handlers retain their full response window.
// The send timeout is the total budget for the whole call. Unlike a one-way Send, each
// SendReceive attempt may use the full remaining budget rather than a fixed slice: the
// response wait is a legitimate long operation (a peer may take up to its receive
// timeout to reply), so slicing it would abort valid slow responses. Retries therefore
// only fire on attempts that fail fast enough to leave budget (e.g. dial errors); a
// stalled attempt consumes the budget and is not retried, since a delivered request to
// a slow peer must be waited out, not re-sent.
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Adds an opt-in
p2p.WithRetriessend option and wires it into all DKG ceremony transports (exchanger,bcast, pedersen board, frost round 1), so a single transient send failure (e.g. a stalled relay stream) no longer aborts a whole ceremony. All DKG receive paths already deduplicate re-delivered messages, so retries are safe; the core workflow keeps zero retries. Also includes the peer name inp2p.Send/SendReceiveerrors, so failures name the unreachable peer instead of just the protocol.category: bug
ticket: #4685