Skip to content
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

feature: (alan) filter committee duties per slot validator duties #467

Merged
merged 6 commits into from
Jul 22, 2024

Conversation

y0sher
Copy link
Contributor

@y0sher y0sher commented Jul 17, 2024

Description

Committee struct holds all the shares for the validators of that committee, though a CommitteeRunner only lives for a certain slot and thereby only needs shares of validators that have duty for that slot.

Changes

Filter the shares of Committee to create a subset that has only shares of validators that have duties for this slot.

ssv/committee.go Outdated
dutyShares := make(map[spec.ValidatorIndex]*types.Share)
for _, bduty := range duty.ValidatorDuties {
if _, exists := c.Share[bduty.ValidatorIndex]; !exists {
return fmt.Errorf("no share for validator %d", bduty.ValidatorIndex)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be continue simply?
are we sure we want to fail the run just because of one miss?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, from the spec perspective, I would also say continue is better. On the other hand, in ssv, it's strange that the operator doesn't the share for the duty since he requested the duty for this validator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree

Copy link
Contributor

@MatheusFranco99 MatheusFranco99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@y0sher Approving, but it seems to me that continue makes more sense (for the above issue)

@y0sher
Copy link
Contributor Author

y0sher commented Jul 22, 2024

@y0sher Approving, but it seems to me that continue makes more sense (for the above issue)

please change as you wish and merge when you're ready.

@MatheusFranco99
Copy link
Contributor

@y0sher @GalRogozinski
Doing the change and adding tests...

)

// SharesWithFractionOfDutyValidators performs a complete duty execution for a runner that only has a fraction of the shares for the duty's validators
func SharesWithFractionOfDutyValidators() tests.SpecTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to MissingSomeShares

dutyValidators := []int{1, 2, 3, 4, 5, 6, 7, 8, 9, 10}

multiSpecTest := &committee.MultiCommitteeSpecTest{
Name: "shares with fraction of duty validators",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Name: "shares with fraction of duty validators",
Name: "start committee duty with missing shares",

@GalRogozinski GalRogozinski changed the base branch from main to dev July 22, 2024 13:22
@GalRogozinski GalRogozinski merged commit 9f64a7b into dev Jul 22, 2024
2 checks passed
@GalRogozinski GalRogozinski deleted the feature/filter-committee-duty-shares branch July 22, 2024 13:23
GalRogozinski pushed a commit that referenced this pull request Sep 12, 2024
* Filter shares for slot `CommitteeRunner` based on validators that have duty for that slot.

* Filter duty and create share map according to owned validators

* Add test: start duty with no shares for duty's validators

* Add test: happy flow for committee with fraction of duty's validators

* Generate JSON tests

* Apply suggestions

---------

Co-authored-by: MatheusFranco99 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants