ssvlabs / ssv-spec

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

Validate signing roots for CommitteeRunner #415

Closed MatheusFranco99 closed 4 months ago

MatheusFranco99 commented 4 months ago

This PR adds a validation on the number of signing roots (and their content) for the CommitteeRunner and aligns the existing tests with the new error.

moshe-blox commented 4 months ago

Good!

The only thing we have to think about before merging this PR is that for committee runner the code was initially designed to work if a post-consensus was missing a root (in case there was an issue with a certain validator).

Not sure if we need to really care for this case though... Let's talk about it tomorrow

Yes, either missing or has extra roots, because currently SSV doesn't guarantee equal validator sets between operators at any given time.

If a validator registered with 1 corrupt BLS share, only 1 operator would know it and they would send post consensus messages with 1 missing signature to the other operators, and the opposite for that operator which would see 1 extra signature from others.

The other reason for mismatching validator sets is contract event syncing disparity.

This should be solved in the future, but its not part of the plan for Committee Consensus.

Maybe we can just check that an operator did not send more than some fixed reasonable limit of signatures and that they are all with unique validator indices?

GalRogozinski commented 4 months ago

So @MatheusFranco99 It seems that we should allow subset of roots

moshe-blox commented 4 months ago

So @MatheusFranco99 It seems that we should allow subset of roots

Yes, and also a superset so that you can ignore signatures for validator indices you dont recognize.

GalRogozinski commented 4 months ago

Yes, and also a superset so that you can ignore signatures for validator indices you dont recognize.

So @MatheusFranco99 maybe we need to rethink this PR...

MatheusFranco99 commented 4 months ago

@GalRogozinski Yes, let's discuss what should be done here

GalRogozinski commented 4 months ago

@MatheusFranco99 Add @moshe-blox comments to the SIP, and make sure that the current code meets the criteria

MatheusFranco99 commented 4 months ago

@GalRogozinski Added it in https://github.com/ssvlabs/SIPs/pull/42/commits/8df6b0d118792bba3886d60d85317b5314b24da1