merge queue: checking epbs (68acc2e) and #1125 together - #1153
Closed
mergify[bot] wants to merge 10 commits into
Closed
merge queue: checking epbs (68acc2e) and #1125 together#1153mergify[bot] wants to merge 10 commits into
mergify[bot] wants to merge 10 commits into
Conversation
Sign ProposerPreferences via a single-validator partial-signature collection round: domain keyed on proposal_slot's epoch, envelope slot stamped with the duty's proposal_slot (SIP-94 §5/§7, not the send slot), no slashing-DB interaction. Add a reconstruction-failure reporter and metric, and generalize the shared collect_signature failure classifier. Part of #1063.
…ure telemetry The NoSignature arm labelled every failed reconstruction signing_root_divergence and warned that operators had likely diverged on the signing root. That class is reached only via QueueClosedError, which conflates threshold-not-reached, too-few operators, and delivery loss, and the wire carries no preference fields, so the collector cannot know a signing-root split occurred. The sibling PTC path already labels the identical class honestly. Rename the label to insufficient_partial_signatures, reword the warn to list signing-root divergence as one of several possible causes, and update the two metric tests. Document CollectionTimeout in the classifier as reserved and not currently produced by the collector. Addresses review feedback on #1125.
Rename signed_proposer_preferences_total to anchor_signed_proposer_preferences_total so the top-line signing counter carries the same anchor_ prefix as its paired failure counter (anchor_proposer_preferences_reconstruction_failures_total) and the rest of the file. Choosing the prefix before ship avoids a later rename breaking dashboards. Addresses review feedback on #1125.
Wrap the per-validator signature collection in a slot-derived tokio timeout (2 slots) mapped to CollectionError::CollectionTimeout. Without a deadline the await resolves only when the collector is reaped at proposal_slot + 2 (up to ~13 min for a next-epoch lookahead emission); because the LH ProposerPreferencesService awaits each validator sequentially in one task, a single no-quorum validator (an expected SIP-94 §5 state such as a target_gas_limit config mismatch or a dependent_root observation split) blocks both epochs' emissions for that duration. The collector outlives the deadline, so a later per-slot retry still reconstructs if quorum forms. Also drop the run_and_update_metrics wrapper, whose catch-all arm error-logged a no-quorum failure and miscounted it as other_error on top of the reporter's correct warn and insufficient_partial_signatures. Match sign_payload_attestation: the reporter owns failure telemetry, and success is counted explicitly on the Ok path. Addresses review feedback on #1125; closes the bounded-timeout AC of #1063.
Three tests asserted conditions that held regardless of the behavior they named, so none could catch its target regression: - the signing-domain recompute ran under ChainSpec::mainnet(), where the send and proposal epochs share the genesis fork version, so keying the domain on the send epoch would produce an identical root; - one test asserted attribution labels the production code never writes, so the zero deltas held even if the whole reporting arm were deleted; - comments claimed the on-wire PartialSignatureMessages.slot was asserted, but the mock captures metadata.slot at the trait boundary before create_message runs. Fix by mirroring the payload_attestation sibling suite: - run the success test on a spec with Gloas activated at the lookahead epoch, so a fork boundary sits between the send and proposal epochs, and assert the root matches the proposal-epoch domain and differs from the send-epoch domain; - drop the vacuous attribution-label test; add an infra-class test (EmptySignature) that gives the Infra classification arm its first coverage, and a zero-delta infra check on the QueueClosedError test; - add a slashing-protection tripwire; - reword the slot comments to state metadata.slot is what is asserted. Addresses review feedback on #1125.
sign_proposer_preferences now synthesizes CollectionTimeout when its bounded collection-wait deadline elapses, so the note claiming the variant is not produced and has no deadline path is stale. Reword it to describe the caller that produces it.
…path Exercise issue #1063 AC7: a no-quorum collection must fail per-validator with a bounded timeout and never hang the caller. Add a mock-collector hang mode that returns a never-resolving future, and a test that drives the production tokio::time::timeout to elapse under paused virtual time (start_paused), asserting the result is SignatureCollectionFailed(CollectionTimeout) and that it lands in the insufficient_partial_signatures bucket, not infra. Without the production timeout the test would hang on the pending collector future rather than return, which is the "never hangs the caller" property AC7 requires.
|
Your PR title doesn't follow the Conventional Commit guidelines. Example of valid titles:
Usage:
Breaking Changes Breaking changes are noted by using an exclamation mark. For example:
Help For more information, follow the guidelines here: https://www.conventionalcommits.org/en/v1.0.0/ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## epbs #1153 +/- ##
=======================================
Coverage ? 68.03%
=======================================
Files ? 164
Lines ? 30942
Branches ? 0
=======================================
Hits ? 21050
Misses ? 9892
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎉 This pull request has been checked successfully and will be merged soon. 🎉
Branch epbs (68acc2e) and #1125 are queued together for merge.
This pull request has been created by Mergify to check the mergeability of #1125.
You don't need to do anything. Mergify will close this pull request automatically when it is complete.
Required conditions of queue rule
defaultfor merge:check-success=test-suite-successRequired conditions to stay in the queue:
#approved-reviews-by >= 1check-success=license/clacheck-success=target-branch-checklabel!=do-not-merge