-
Notifications
You must be signed in to change notification settings - Fork 84
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
Feat: Mark invalid shares #1313
base: stage
Are you sure you want to change the base?
Conversation
eth/eventhandler/handlers.go
Outdated
if isOperatorShare && !validatorShare.InvalidSecret { | ||
eh.metrics.ValidatorInactive(event.PublicKey) | ||
ownShare = validatorShare | ||
logger = logger.With(zap.Bool("own_validator", isOperatorShare)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can log that its our validator regardless of invalid secret, and also we should probably log if its an invalid secret (so its hard to miss in Kibana)
if isOperatorShare && !validatorShare.InvalidSecret { | |
eh.metrics.ValidatorInactive(event.PublicKey) | |
ownShare = validatorShare | |
logger = logger.With(zap.Bool("own_validator", isOperatorShare)) | |
if isOperatorShare { | |
eh.metrics.ValidatorInactive(event.PublicKey) | |
logger = logger.With(zap.Bool("own_validator", isOperatorShare)) | |
if validatorShare.InvalidSecret { | |
logger = logger.With(zap.Bool("invalid_secret", true)) | |
logger.Debug("registered validator has an invalid share secret, it will not be started") | |
} else { | |
ownShare = validatorShare | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Added this
if err != nil { | ||
return nil, fmt.Errorf("could not extract validator share from event: %w", err) | ||
|
||
if err != nil && !share.Metadata.InvalidSecret { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the 2nd check needed? as far as i can see that method returns no error when the secret is invalid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@moshe-blox, No we can't remove the 2nd check because we have to check that there is no error and share is valid.
Next, in case the share contains invalid secret, then we have to save it and mark as invalid what we do on this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sry, i thought that we're not returning error in validatorAddedEventToShare
when only the share is invalid, thats what i previously meant in my comment there
can we try this?
i'm interested to see how it would look, we can just log the reason for invalid secret instead of returning it as error (for example logger.Debug("invalid secret error: could not decrypt share private")
)
then this check becomes just a simple err != nil
and naturally theres no error that keeps going upstream (back to high level funcs who called us)
|
||
publicKey, err := ssvtypes.DeserializeBLSPublicKey(event.PublicKey) | ||
if err != nil { | ||
return nil, nil, &MalformedEventError{ | ||
return validatorShare, nil, &MalformedEventError{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did we do this change? is returning an empty share anything better or different than returning nil?
…-shares-stage-fix # Conflicts: # eth/eventhandler/handlers.go # operator/validator/controller.go
Codecov ReportAttention: Patch coverage is
Additional details and impacted files☔ View full report in Codecov by Sentry. |
…-shares-stage-fix # Conflicts: # eth/eventhandler/handlers.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@moshe-blox lets decide if we still want to have this.
hey @MatusKysel @y0sher yes, and i think we ended up saying my suggestion did not work out well, and this PR would be better in the original form where it returned an error on invalid share (rather than only setting the field and not returning error) |
No description provided.