ssvlabs / ssv-spec

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

Reduce redundant validation calls #366

Open MatheusFranco99 opened 6 months ago

MatheusFranco99 commented 6 months ago

Drop redundant calls

SignedMessage.Validate() is called multiple times for the same message, as pointed by https://github.com/bloxapp/ssv-spec/issues/326. This PR aims to solve this duplication.

The current flow is shown below.

current_flow

Notice that isValidProposal calls validRoundChangeForData and validSignedPrepareForHeightRoundAndRoot due to the justification messages.

If we want to keep the higher level validation calls we could get the following

eliminate_low_level

The problem is that the Prepare and Round-Change messages would still do double validation. And the checks can't be dropped due to the isValidProposal function.

If we were to keep the lower level calls, we would get

eliminate_higher_level

This produces no duplicated calls but, from a design point of view, it's bad to do a light validation check late.

Another option is to change the Validation function to also validate nested messages (which I think is best), as below.

new_validate

Closes https://github.com/bloxapp/ssv-spec/issues/326.

MatheusFranco99 commented 6 months ago

@GalRogozinski @y0sher @olegshmuelov @moshe-blox I would like to know your opinion on this issue before I start the changes. The PR description explains the options.

y0sher commented 3 weeks ago

@MatheusFranco99 I agree with you on this that last option is the best. I think another option exists where we only validate the top message. but then when we call Validate on underlying message we are only validating the underlying message. I think the point is not to validate same data more than once. but also a nice to have property is that we do not validate data that we don't need to. for example if we deem the message as bad and not go forward after first validate and some other checks, in the last option you mentioned we might be validating internal messages without needing to.