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

mark invalid encrypted shares as 'invalid' #1151

Closed
wants to merge 10 commits into from

Conversation

AKorpusenko
Copy link
Contributor

No description provided.

@AKorpusenko AKorpusenko changed the title DRAFT: WIP: mark invalid encrypted shares as 'invalid' mark invalid encrypted shares as 'invalid' Oct 9, 2023
MatusKysel
MatusKysel previously approved these changes Oct 9, 2023
Copy link
Contributor

@MatusKysel MatusKysel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -233,11 +233,21 @@ func (eh *EventHandler) handleShareCreation(
sharePublicKeys,
encryptedKeys,
)

var malformedEventError *MalformedEventError = nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

= nil is redundant

return nil, fmt.Errorf("could not extract validator share from event: %w", err)
if errors.As(err, &malformedEventError) &&
malformedEventError.IsInvalidEncryptedShare &&
share.BelongsToOperator(eh.operatorData.GetOperatorData().ID) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this error can't ever happen if this is not your validator, because you only try to decrypt your own shares, so maybe this check is redundant?

Comment on lines +240 to +247
if errors.As(err, &malformedEventError) &&
malformedEventError.IsInvalidEncryptedShare &&
share.BelongsToOperator(eh.operatorData.GetOperatorData().ID) {

share.Metadata.Invalid = true
} else {
return nil, fmt.Errorf("could not extract validator share from event: %w", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets invert the condition to return if it's not the error we want, otherwise we continue with setting the invalid flag.

@@ -686,6 +686,10 @@ func (c *controller) onShareStart(share *ssvtypes.SSVShare) (bool, error) {
c.logger.Warn("skipping validator until it becomes active", fields.PubKey(share.ValidatorPubKey))
return false, nil
}
if share.Invalid {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can add it to the filter above in setupValidators like the ByNotLiquidated?

Comment on lines +19 to +25
// InitMaxPossibleShareSize - initial protocol max share size
InitMaxPossibleShareSize = 1245

// CurrentMaxPossibleShareSize - Current protocol version max share size;
// can be generated by generateMaxPossibleShare
// Used only for testing purposes
CurrentMaxPossibleShareSize = 1257
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we can delete these and instead just update the old value MaxPossibleShareSize to 1257

@@ -103,4 +112,5 @@ type Metadata struct {
BeaconMetadata *beaconprotocol.ValidatorMetadata
OwnerAddress common.Address
Liquidated bool
Invalid bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can rename this to InvalidSecret so its more self-explanatory?

otherwise one might assume that something else in the event may have been invalid (such as invalid nonce)

Comment on lines +318 to 333
return &validatorShare, nil, &MalformedEventError{
Err: fmt.Errorf("could not decrypt share private key: %w", err),
IsInvalidEncryptedShare: true,
}
}
if err = shareSecret.SetHexString(string(decryptedSharePrivateKey)); err != nil {
return nil, nil, &MalformedEventError{
Err: fmt.Errorf("could not set decrypted share private key: %w", err),
return &validatorShare, nil, &MalformedEventError{
Err: fmt.Errorf("could not set decrypted share private key: %w", err),
IsInvalidEncryptedShare: true,
}
}
if !bytes.Equal(shareSecret.GetPublicKey().Serialize(), validatorShare.SharePubKey) {
return nil, nil, &MalformedEventError{
Err: errors.New("share private key does not match public key"),
return &validatorShare, nil, &MalformedEventError{
Err: fmt.Errorf("share private key does not match public key"),
IsInvalidEncryptedShare: true,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please try to instead of returning malformed error, instead to just set the Invalid flag on the share and return no error?

with this you'd be able to just ask if the share is invalid rather than if there was a malformedError

Copy link
Contributor

@moshe-blox moshe-blox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks correct! good job

just a few suggestions after reviewing with Matus and Yosher

@MatusKysel MatusKysel closed this Feb 14, 2024
@AKorpusenko
Copy link
Contributor Author

replaced with #1313

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.

None yet

4 participants