Value check for beacon vote fix

[GalRogozinski]

Description

The beacon vote value check is now being constructed with clear parameters

[GalRogozinski]

Closes #437

[y0sher]

duty variable is already defined, shadowing it with another variable is confusing and error prone. Also duties is only used one time so no real reason to declare a var.

Suggested change

	duties := duty.ValidatorDuties
	slashablePublicKeys := []types.ShareValidatorPK{}
	for _, duty := range duties {
	slashablePublicKeys := []types.ShareValidatorPK{}
	for _, vduty := range duty.ValidatorDuties {

[y0sher]

I think this change complicates stuff, what was wrong with leaving the previous BeaconVoteValueCheck and requiring the caller to pass in only slashable validators? this simple check can be done outside, its encapsulating and hiding the fact that not all ValidatorDuties in CommitteeDuty are slashable.

I also don't think the creation of ValueCheckF should be aware of a specific duty like here, its a dramatic change comparing to how all other value checks work.

In implementation, we are already filtering the slashable validators before passing in to the value check today, IMO spec should do the same and do the filtering in committee.go:StartDuty function (which passes validators to the value check) instead of encapsulating it here.

[GalRogozinski]

@y0sher
I remember that you did this way in impl after consulting me due to the spec being unclear.
So this is my attempt to make the spec clearer.

Currently you may notice that besides testing code, nothing calls the ValueCheckF creation logic. So I must have some way to convey on what we do the slashing checks. I can't simply do the filtering in committee.go:StartDuty. The best I was able to do is moving the core logic to createBeaconValueCheckF, while BeaconValueCheckF does the filtering logic before. Also for testing purposes it is best that BeaconValueCheckF will do all operations.

I think the biggest issue at the moment is that before the same ValueCheckF function from qbft.Config was used throughout all instances. Now we have to reconstruct the function with the duty parameter. But the Controller object assumes a Config that was only created once. So the spec is still misleading.

Do note it is not good to pass duty as a parameter to the valueCheckF along side data, because the consensus layer shouldn't be aware of Ethereum or SSV logic.

I wasn't sure how to solve this issue before. I have an idea now though... In CommitteeRunner, GetValCheckF can recreate the function each time. Then the new function can be injected to the Controller's qbft.Config. The only bad thing about this solution is that up until now Config never mutated after creation. So the SSV impl should take care of concurrency issues.

P.S.
A quick reminder: duty is required now in the ValCheck

1. For fetching the slot, since the BeaconVote object doesn't carry this data.
2. Fetching the proper slashable validator pubkeys.

Another reminder:
As long as the impl ultimately performs the same logic as spec, it is fine if the design is different.

[y0sher]

if you don't want to do it in committee.go:StartDuty I think you should just create a separate function that can filter the validators from a duty struct and call it outside of valuecheck then passing it anyway. call it wherever you want.

I think its best to separate the logic for valuecheck, it should only do value check and not filtering/other stuff. or at least make it visible and noticeable in the name of the function that it's not just value check.

[GalRogozinski]

@y0sher
It isn't that I don't want to... in the current design of spec it can't be there.
If I take the filtering out of the function constructor I need to rethink tests... will look into it but not sure it is worth it