Time left checks + cleanup#7892
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/testnet-fixes #7892 +/- ##
======================================================
- Coverage 77.69% 77.68% -0.02%
======================================================
Files 885 885
Lines 125797 125790 -7
======================================================
- Hits 97742 97715 -27
- Misses 21585 21594 +9
- Partials 6470 6481 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| func (sr *subroundSignature) isTimeLeft() bool { | ||
| timeLeft := sr.RoundHandler().RemainingTime(sr.RoundHandler().TimeStamp(), time.Duration(sr.EndTime())) |
There was a problem hiding this comment.
This uses sr.EndTime() of the subround signature. In the specs, there is a new const, signatureSendDeadline which is used in this case which is < sr.EndTime().
There was a problem hiding this comment.
the signature subround was already reduced with the new changes in subrounds config, i think that's not very applicable anymore; but tbd after we complete and fine tune all the parts
| defer cancel() | ||
|
|
||
| sr.waitForSingatures(timeLeft) | ||
| isTimeLeft := sr.waitForSignatures(timeLeft) |
There was a problem hiding this comment.
If not all sigs are done, this will return false. Isn't there a way to use only the signatures that are done instead of ignoring all? What is the downside of doing that?
There was a problem hiding this comment.
if timeout here, it means there is no time for sending any signature, so the signatures that were already created will not be useful anyway
| return false | ||
| } | ||
|
|
||
| signatureShare, err := sr.SigningHandler().CreateSignatureShareForPublicKey( |
There was a problem hiding this comment.
Will there be any advantage if we also optimistically sign for the singleKey node instead of creating the signature here? Or is it cheap enough to not matter for a single key?
There was a problem hiding this comment.
the initial intend was for multi-key nodes; yea, i don't think it's worth adding more complexity for a single key node
There was a problem hiding this comment.
Pull request overview
This PR tightens timeout handling in the BLS v2 SPoS signature flow and performs related cleanup in both implementation and tests.
Changes:
- Add explicit “time left” guards in signature-job execution paths and propagate wait results from the optimistic-signature wait logic.
- Simplify competing-block waiting by removing the node-level helper and updating tests/exports accordingly.
- Clean up tests by removing a retry-until-context-cancelled scenario and unused imports.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| consensus/spos/bls/v2/subroundSignature.go | Adds time-left checks, refactors signature waiting to return a status, and removes managed-key fallback signature creation. |
| consensus/spos/bls/v2/subroundEndRound.go | Adds a remaining-time guard to stop processing signatures after the end-round signature wait window. |
| consensus/spos/bls/v2/subroundSignatureCompetingBlock_test.go | Updates tests to call WaitIfCompetingBlock instead of the removed node-level helper. |
| consensus/spos/bls/v2/subroundSignature_test.go | Removes a test that depended on the removed “retry create signature share” behavior and drops unused atomic import. |
| consensus/spos/bls/v2/export_test.go | Removes the exported test helper WaitIfCompetingBlockForNode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| remainingTime := sr.remainingTime() | ||
| if remainingTime <= 0 { | ||
| return false | ||
| } |
| result := sr.WaitIfCompetingBlock(context.Background(), []byte{}, 100, []byte("current_hash")) | ||
| assert.False(t, result, "should return false when no key has a competing block") |
Reasoning behind the pull request
Proposed changes
Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?