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

Reduce redundant validation calls #366

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MatheusFranco99
Copy link
Contributor

Drop redundant calls

SignedMessage.Validate() is called multiple times for the same message, as pointed by #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 #326.

@MatheusFranco99 MatheusFranco99 self-assigned this Mar 1, 2024
@MatheusFranco99
Copy link
Contributor Author

@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
Copy link
Contributor

y0sher commented Aug 19, 2024

@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.

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.

Optimization Suggestion: Reduce Redundant Calls to SignedMessage.Validate in QBFT Message Processing
2 participants