search

ssvlabs / ssv-spec

GNU General Public License v3.0
25 stars 22 forks source link

Value check for beacon vote fix #471

Open GalRogozinski opened 1 month ago

GalRogozinski commented 1 month ago

Description

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

GalRogozinski commented 1 month ago

Closes #437

GalRogozinski commented 1 week ago

@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 commented 1 week ago

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 commented 1 week ago

@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