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
133 changes: 78 additions & 55 deletions eth/eventhandler/event_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@ func TestHandleBlockEventsStream(t *testing.T) {
// Corrupt the encrypted last share key of the 4th operator
malformedSharesData[len(malformedSharesData)-1] ^= 1

valPubKey := validatorData2.masterPubKey.Serialize()
// Call the contract method
_, err = boundContract.SimcontractTransactor.RegisterValidator(
auth,
Expand Down Expand Up @@ -454,65 +455,88 @@ func TestHandleBlockEventsStream(t *testing.T) {
// Check that validator was registered for op1,
shares = eh.nodeStorage.Shares().List(nil)
require.Equal(t, 2, len(shares))
valShare := eh.nodeStorage.Shares().Get(nil, valPubKey)
require.NotNil(t, valShare)
require.False(t, valShare.Invalid)
// and nonce was bumped
nonce, err = eh.nodeStorage.GetNextNonce(nil, testAddr)
require.NoError(t, err)
require.Equal(t, registrystorage.Nonce(3), nonce)
})

// Share for 1st operator is malformed; check nonce is bumped correctly; validator wasn't added
// slashing protection data is not added
t.Run("test malformed ValidatorAdded and nonce is bumped", func(t *testing.T) {
malformedSharesData := sharesData3[:]

operatorCount := len(ops)
signatureOffset := phase0.SignatureLength
pubKeysOffset := phase0.PublicKeyLength*operatorCount + signatureOffset

// Corrupt the encrypted share key of the operator 1
malformedSharesData[pubKeysOffset+encryptedKeyLength-1] ^= 1

// Call the contract method
_, err = boundContract.SimcontractTransactor.RegisterValidator(
auth,
validatorData3.masterPubKey.Serialize(),
[]uint64{1, 2, 3, 4},
malformedSharesData,
big.NewInt(100_000_000),
simcontract.CallableCluster{
ValidatorCount: 1,
NetworkFeeIndex: 1,
Index: 2,
Active: true,
Balance: big.NewInt(100_000_000),
})
require.NoError(t, err)
sim.Commit()

block = <-logs
require.NotEmpty(t, block.Logs)
require.Equal(t, ethcommon.HexToHash("0x48a3ea0796746043948f6341d17ff8200937b99262a0b48c2663b951ed7114e5"), block.Logs[0].Topics[0])

eventsCh = make(chan executionclient.BlockLogs)
go func() {
defer close(eventsCh)
eventsCh <- block
}()

lastProcessedBlock, err = eh.HandleBlockEventsStream(eventsCh, false)
t.Run("test invalid encrypted shares", func(t *testing.T) {
validatorData, err := createNewValidator(ops)
require.NoError(t, err)
require.Equal(t, blockNum+1, lastProcessedBlock)
blockNum++

requireKeyManagerDataToNotExist(t, eh, 2, validatorData3)

// Check that validator was not registered
shares = eh.nodeStorage.Shares().List(nil)
require.Equal(t, 2, len(shares))
// and nonce was bumped
nonce, err = eh.nodeStorage.GetNextNonce(nil, testAddr)
require.NoError(t, err)
require.Equal(t, registrystorage.Nonce(4), nonce)
t.Run("valid signature + wrong op1 key, nonce is bumped, share is saved, marked as invalid", func(t *testing.T) {
sharesData, err := generateSharesData(validatorData, ops, testAddr, 3)
require.NoError(t, err)

// Generating a new list of operators, but with changed op1 rsa keys
pb, sk, _ := rsaencryption.GenerateKeys()
wrongOps := make([]*testOperator, 0)
wrongOps = append(wrongOps, &testOperator{id: 1, pub: pb, priv: sk})

wrongOps = append(wrongOps, ops[1:]...)

require.Equal(t, len(wrongOps), len(ops))
require.NotEqual(t, wrongOps[0].pub, ops[0].pub)

incorrectSharesData, err := generateSharesData(validatorData, wrongOps, testAddr, 4)
require.NoError(t, err)

signatureOffset := phase0.SignatureLength
// Replace incorrect signature with a correct one
for i := 0; i < signatureOffset; i++ {
incorrectSharesData[i] = sharesData[i]
}

valPubKey := validatorData.masterPubKey.Serialize()
// Call the contract method
_, err = boundContract.SimcontractTransactor.RegisterValidator(
auth,
valPubKey,
[]uint64{1, 2, 3, 4},
incorrectSharesData,
big.NewInt(100_000_000),
simcontract.CallableCluster{
ValidatorCount: 1,
NetworkFeeIndex: 1,
Index: 2,
Active: true,
Balance: big.NewInt(100_000_000),
})
require.NoError(t, err)
sim.Commit()

block = <-logs
require.NotEmpty(t, block.Logs)
require.Equal(t, ethcommon.HexToHash("0x48a3ea0796746043948f6341d17ff8200937b99262a0b48c2663b951ed7114e5"), block.Logs[0].Topics[0])

eventsCh = make(chan executionclient.BlockLogs)
go func() {
defer close(eventsCh)
eventsCh <- block
}()

lastProcessedBlock, err = eh.HandleBlockEventsStream(eventsCh, false)
require.NoError(t, err)
require.Equal(t, blockNum+1, lastProcessedBlock)
blockNum++

requireKeyManagerDataToNotExist(t, eh, 2, validatorData)

// Check that validator was not registered
shares = eh.nodeStorage.Shares().List(nil)
require.Equal(t, 3, len(shares))
valShare := eh.nodeStorage.Shares().Get(nil, valPubKey)
require.NotNil(t, valShare)
require.True(t, valShare.Invalid)
// and nonce was bumped
nonce, err = eh.nodeStorage.GetNextNonce(nil, testAddr)
require.NoError(t, err)
require.Equal(t, registrystorage.Nonce(4), nonce)
})
})

// Correct event; check nonce is bumped correctly; validator is added
Expand Down Expand Up @@ -557,7 +581,7 @@ func TestHandleBlockEventsStream(t *testing.T) {

// Check that validator was registered
shares = eh.nodeStorage.Shares().List(nil)
require.Equal(t, 3, len(shares))
require.Equal(t, 4, len(shares))
// and nonce was bumped
nonce, err = eh.nodeStorage.GetNextNonce(nil, testAddr)
require.NoError(t, err)
Expand Down Expand Up @@ -607,7 +631,7 @@ func TestHandleBlockEventsStream(t *testing.T) {

// Check that validator was registered
shares = eh.nodeStorage.Shares().List(nil)
require.Equal(t, 4, len(shares))
require.Equal(t, 5, len(shares))
// and nonce was bumped
nonce, err = eh.nodeStorage.GetNextNonce(nil, testAddr2)
require.NoError(t, err)
Expand Down Expand Up @@ -736,7 +760,7 @@ func TestHandleBlockEventsStream(t *testing.T) {

// Check the validator was removed from the validator shares storage.
shares := eh.nodeStorage.Shares().List(nil)
require.Equal(t, 3, len(shares))
require.Equal(t, 4, len(shares))
valShare = eh.nodeStorage.Shares().Get(nil, valPubKey)
require.Nil(t, valShare)
requireKeyManagerDataToNotExist(t, eh, 3, validatorData1)
Expand Down Expand Up @@ -1464,7 +1488,6 @@ func generateSharesData(validatorData *testValidatorData, operators []*testOpera

pubkeys = append(pubkeys, validatorData.operatorsShares[i].pub.Serialize()...)
encryptedShares = append(encryptedShares, ciphertext...)

}

tosign := fmt.Sprintf("%s:%d", owner.String(), nonce)
Expand Down
45 changes: 32 additions & 13 deletions eth/eventhandler/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,12 +179,12 @@ func (eh *EventHandler) handleValidatorAdded(txn basedb.Txn, event *contract.Con
}

validatorShare := eh.nodeStorage.Shares().Get(txn, event.PublicKey)
var malformedEventError *MalformedEventError

if validatorShare == nil {
createdShare, err := eh.handleShareCreation(txn, event, sharePublicKeys, encryptedKeys)
if err != nil {
var malformedEventError *MalformedEventError
if errors.As(err, &malformedEventError) {
if errors.As(err, &malformedEventError) && malformedEventError != nil && !malformedEventError.IsInvalidEncryptedShare {
logger.Warn("malformed event", zap.Error(err))

return nil, err
Expand All @@ -209,7 +209,7 @@ func (eh *EventHandler) handleValidatorAdded(txn basedb.Txn, event *contract.Con
}

isOperatorShare := validatorShare.BelongsToOperator(eh.operatorData.GetOperatorData().ID)
if isOperatorShare {
if isOperatorShare && !validatorShare.Invalid {
eh.metrics.ValidatorInactive(event.PublicKey)
ownShare = validatorShare
logger = logger.With(zap.Bool("own_validator", isOperatorShare))
Expand All @@ -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


if err != nil {
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?


share.Metadata.Invalid = true
} else {
return nil, fmt.Errorf("could not extract validator share from event: %w", err)
}
Comment on lines +240 to +247
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.

}

if share.BelongsToOperator(eh.operatorData.GetOperatorData().ID) {
if malformedEventError == nil && share.BelongsToOperator(eh.operatorData.GetOperatorData().ID) {
if shareSecret == nil {
return nil, errors.New("could not decode shareSecret")
}
Expand All @@ -253,7 +263,11 @@ func (eh *EventHandler) handleShareCreation(
return nil, fmt.Errorf("could not save validator share: %w", err)
}

return share, nil
if malformedEventError == nil {
return share, nil
}

return share, malformedEventError
}

func validatorAddedEventToShare(
Expand Down Expand Up @@ -301,18 +315,21 @@ func validatorAddedEventToShare(
shareSecret = &bls.SecretKey{}
decryptedSharePrivateKey, err := rsaencryption.DecodeKey(operatorPrivateKey, encryptedKeys[i])
if err != nil {
return nil, nil, &MalformedEventError{
Err: fmt.Errorf("could not decrypt share private key: %w", err),
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,
}
Comment on lines +318 to 333
Copy link
Contributor

@moshe-blox moshe-blox Feb 12, 2024

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

}
}
Expand All @@ -321,6 +338,7 @@ func validatorAddedEventToShare(
validatorShare.DomainType = ssvtypes.GetDefaultDomain()
validatorShare.Committee = committee
validatorShare.Graffiti = []byte("ssv.network")
validatorShare.Metadata.Invalid = false

return &validatorShare, shareSecret, nil
}
Expand Down Expand Up @@ -520,7 +538,8 @@ func (eh *EventHandler) processClusterEvent(

// MalformedEventError is returned when event is malformed
type MalformedEventError struct {
Err error
Err error
IsInvalidEncryptedShare bool
}

func (e *MalformedEventError) Error() string {
Expand Down
4 changes: 4 additions & 0 deletions operator/validator/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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?

c.logger.Warn("skipping validator with invalid share", fields.PubKey(share.ValidatorPubKey))
return false, nil
}

if err := c.setShareFeeRecipient(share, c.recipientsStorage.GetRecipientData); err != nil {
return false, fmt.Errorf("could not set share fee recipient: %w", err)
Expand Down
1 change: 1 addition & 0 deletions protocol/v2/types/ssvshare.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,4 +103,5 @@ type Metadata struct {
BeaconMetadata *beaconprotocol.ValidatorMetadata
OwnerAddress common.Address
Liquidated bool
Invalid bool
Copy link
Contributor

@moshe-blox moshe-blox Feb 12, 2024

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)

}
Loading