-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core/tracker: detect inconsistent partial signed data #1343
Conversation
Codecov ReportBase: 53.59% // Head: 53.75% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1343 +/- ##
==========================================
+ Coverage 53.59% 53.75% +0.16%
==========================================
Files 143 142 -1
Lines 17034 17099 +65
==========================================
+ Hits 9129 9192 +63
+ Misses 6633 6632 -1
- Partials 1272 1275 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
// expectInconsistentParSigs returns true if the duty type is expected to sometimes | ||
// produce inconsistent partial signed data. | ||
func expectInconsistentParSigs(duty core.DutyType) bool { | ||
return duty == core.DutySyncMessage || duty == core.DutySyncContribution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be
return duty == core.DutySyncMessage || duty == core.DutySyncContribution | |
return duty == core.DutySyncMessage || duty == core.DutyPrepareSyncContribution |
??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, SyncCommitteeSelection
should always be consistent, whlie ContributionAndProof
contains the beaconblockroot that VC pick themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true
core/tracker/tracker.go
Outdated
@@ -287,15 +327,49 @@ func analyseDutyFailed(duty core.Duty, allEvents map[core.Duty][]event) (bool, c | |||
return true, comp, msg | |||
} | |||
|
|||
// analyseParSigs returns a mapping partial signed data messages by peers (share index). | |||
func analyseParSigs(duty core.Duty, allEvents map[core.Duty][]event) parsigsByMsg { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also curious as to why this function needs all the events and not just the events for the duty?
will this work?
func analyseParSigs(duty core.Duty, allEvents map[core.Duty][]event) parsigsByMsg { | |
func analyseParSigs(duty core.Duty, events []event) parsigsByMsg { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, good catch
Co-authored-by: Abhishek Kumar <43061995+xenowits@users.noreply.github.com>
) Verify inner attestation aggregation selection proof, not just the aggregation signature. This should fix the sporadic aggregation failures which I suspect is due to the cluster running both validatormock and teku, and that teku is per-chance submitting aggregation using legacy selection proofs in the same slot. It is also good practice to verify all signatures. category: bug ticket: #1343
Track inconsistent partial signature data in tracker.
category: feature
ticket: #1342