Ensure Slashing Protection Exports and Keymanager API Work According to Spec (#9938)

* password compliance

* delete keys tests

* changes to slashing protection exports

* export tests pass

* fix up failures

* gaz

* table driven tests for delete keystores

* comment

* rem deletion logic

* look ma, no db

* fix up tests

* ineff

* gaz

* broken test fix

* Update validator/keymanager/imported/delete.go

* rem

validator/rpc/BUILD.bazel

@@ -27,6 +27,7 @@ go_library(
         "//config/features:go_default_library",
         "//crypto/bls:go_default_library",
         "//crypto/rand:go_default_library",
+        "//encoding/bytesutil:go_default_library",
         "//io/file:go_default_library",
         "//io/logs:go_default_library",
         "//io/prompt:go_default_library",
@@ -45,6 +46,7 @@ go_library(
         "//validator/keymanager/derived:go_default_library",
         "//validator/keymanager/imported:go_default_library",
         "//validator/slashing-protection-history:go_default_library",
+        "//validator/slashing-protection-history/format:go_default_library",
         "@com_github_fsnotify_fsnotify//:go_default_library",
         "@com_github_golang_jwt_jwt//:go_default_library",
         "@com_github_grpc_ecosystem_go_grpc_middleware//:go_default_library",

validator/rpc/standard_api.go

@@ -7,10 +7,12 @@ import (
 	"fmt"
 
 	"github.com/golang/protobuf/ptypes/empty"
+	"github.com/prysmaticlabs/prysm/encoding/bytesutil"
 	ethpbservice "github.com/prysmaticlabs/prysm/proto/eth/service"
 	"github.com/prysmaticlabs/prysm/validator/keymanager"
 	"github.com/prysmaticlabs/prysm/validator/keymanager/derived"
 	slashingprotection "github.com/prysmaticlabs/prysm/validator/slashing-protection-history"
+	"github.com/prysmaticlabs/prysm/validator/slashing-protection-history/format"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
@@ -51,6 +53,12 @@ func (s *Server) ImportKeystores(
 	if !ok {
 		return nil, status.Error(codes.Internal, "Keymanager kind cannot import keys")
 	}
+	if len(req.Passwords) == 0 {
+		return nil, status.Error(codes.Internal, "No passwords provided for keystores")
+	}
+	if len(req.Passwords) != len(req.Keystores) {
+		return nil, status.Error(codes.Internal, "Number of passwords does not match number of keystores")
+	}
 	keystores := make([]*keymanager.Keystore, len(req.Keystores))
 	for i := 0; i < len(req.Keystores); i++ {
 		k := &keymanager.Keystore{}
@@ -72,6 +80,9 @@ func (s *Server) ImportKeystores(
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "Could not import keystores: %v", err)
 	}
+
+	// If any of the keys imported had a slashing protection history before, we
+	// stop marking them as deleted from our validator database.
 	return &ethpbservice.ImportKeystoresResponse{Statuses: statuses}, nil
 }
 
@@ -90,14 +101,21 @@ func (s *Server) DeleteKeystores(
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "Could not delete keys: %v", err)
 	}
-	// We select keys that were deleted for retrieving slashing protection history.
-	filteredKeys := make([][]byte, 0, len(req.PublicKeys))
-	for i, st := range statuses {
-		if st.Status != ethpbservice.DeletedKeystoreStatus_ERROR {
-			filteredKeys = append(filteredKeys, req.PublicKeys[i])
-		}
+	if len(statuses) != len(req.PublicKeys) {
+		return nil, status.Errorf(
+			codes.Internal,
+			"Wanted same amount of statuses %d as public keys %d",
+			len(statuses),
+			len(req.PublicKeys),
+		)
 	}
-	exportedHistory, err := slashingprotection.ExportStandardProtectionJSON(ctx, s.valDB, filteredKeys...)
+
+	statuses, err = s.transformDeletedKeysStatuses(ctx, req.PublicKeys, statuses)
+	if err != nil {
+		return nil, status.Errorf(codes.Internal, "Could not transform deleted keys statuses: %v", err)
+	}
+
+	exportedHistory, err := s.slashingProtectionHistoryForDeletedKeys(ctx, req.PublicKeys, statuses)
 	if err != nil {
 		return nil, status.Errorf(
 			codes.Internal,
@@ -109,7 +127,7 @@ func (s *Server) DeleteKeystores(
 	if err != nil {
 		return nil, status.Errorf(
 			codes.Internal,
-			"Could not export slashing protection history: %v",
+			"Could not JSON marshal slashing protection history: %v",
 			err,
 		)
 	}
@@ -118,3 +136,59 @@ func (s *Server) DeleteKeystores(
 		SlashingProtection: string(jsonHist),
 	}, nil
 }
+
+// For a list of deleted keystore statuses, we check if any NOT_FOUND status actually
+// has a corresponding public key in the database. In this case, we transform the status
+// to NOT_ACTIVE, as we do have slashing protection history for it and should not mark it
+// as NOT_FOUND when returning a response to the caller.
+func (s *Server) transformDeletedKeysStatuses(
+	ctx context.Context, pubKeys [][]byte, statuses []*ethpbservice.DeletedKeystoreStatus,
+) ([]*ethpbservice.DeletedKeystoreStatus, error) {
+	pubKeysInDB, err := s.publicKeysInDB(ctx)
+	if err != nil {
+		return nil, status.Errorf(codes.Internal, "Could not get public keys from DB: %v", err)
+	}
+	if len(pubKeysInDB) > 0 {
+		for i := 0; i < len(pubKeys); i++ {
+			keyExistsInDB := pubKeysInDB[bytesutil.ToBytes48(pubKeys[i])]
+			if keyExistsInDB && statuses[i].Status == ethpbservice.DeletedKeystoreStatus_NOT_FOUND {
+				statuses[i].Status = ethpbservice.DeletedKeystoreStatus_NOT_ACTIVE
+			}
+		}
+	}
+	return statuses, nil
+}
+
+// Gets a map of all public keys in the database, useful for O(1) lookups.
+func (s *Server) publicKeysInDB(ctx context.Context) (map[[48]byte]bool, error) {
+	pubKeysInDB := make(map[[48]byte]bool)
+	attestedPublicKeys, err := s.valDB.AttestedPublicKeys(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("could not get attested public keys from DB: %v", err)
+	}
+	proposedPublicKeys, err := s.valDB.ProposedPublicKeys(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("could not get proposed public keys from DB: %v", err)
+	}
+	for _, pk := range append(attestedPublicKeys, proposedPublicKeys...) {
+		pubKeysInDB[pk] = true
+	}
+	return pubKeysInDB, nil
+}
+
+// Exports slashing protection data for a list of DELETED or NOT_ACTIVE keys only to be used
+// as part of the DeleteKeystores endpoint.
+func (s *Server) slashingProtectionHistoryForDeletedKeys(
+	ctx context.Context, pubKeys [][]byte, statuses []*ethpbservice.DeletedKeystoreStatus,
+) (*format.EIPSlashingProtectionFormat, error) {
+	// We select the keys that were DELETED or NOT_ACTIVE from the previous action
+	// and use that to filter our slashing protection export.
+	filteredKeys := make([][]byte, 0, len(pubKeys))
+	for i, pk := range pubKeys {
+		if statuses[i].Status == ethpbservice.DeletedKeystoreStatus_DELETED ||
+			statuses[i].Status == ethpbservice.DeletedKeystoreStatus_NOT_ACTIVE {
+			filteredKeys = append(filteredKeys, pk)
+		}
+	}
+	return slashingprotection.ExportStandardProtectionJSON(ctx, s.valDB, filteredKeys...)
+}

validator/rpc/standard_api_test.go

@@ -20,6 +20,7 @@ import (
 	"github.com/prysmaticlabs/prysm/validator/db/kv"
 	"github.com/prysmaticlabs/prysm/validator/keymanager"
 	"github.com/prysmaticlabs/prysm/validator/keymanager/derived"
+	"github.com/prysmaticlabs/prysm/validator/slashing-protection-history/format"
 	mocks "github.com/prysmaticlabs/prysm/validator/testing"
 	keystorev4 "github.com/wealdtech/go-eth2-wallet-encryptor-keystorev4"
 )
@@ -115,6 +116,20 @@ func TestServer_ImportKeystores(t *testing.T) {
 		})
 		require.NotNil(t, err)
 	})
+	t.Run("error if no passwords in request", func(t *testing.T) {
+		_, err := s.ImportKeystores(context.Background(), &ethpbservice.ImportKeystoresRequest{
+			Keystores: []string{"hi"},
+			Passwords: []string{},
+		})
+		require.ErrorContains(t, "No passwords provided", err)
+	})
+	t.Run("error if number of passwords does not match number of keystores", func(t *testing.T) {
+		_, err := s.ImportKeystores(context.Background(), &ethpbservice.ImportKeystoresRequest{
+			Keystores: []string{"hi"},
+			Passwords: []string{"hi", "hi"},
+		})
+		require.ErrorContains(t, "Number of passwords does not match", err)
+	})
 	t.Run("prevents importing if faulty slashing protection data", func(t *testing.T) {
 		numKeystores := 5
 		password := "12345678"
@@ -135,12 +150,14 @@ func TestServer_ImportKeystores(t *testing.T) {
 		numKeystores := 5
 		password := "12345678"
 		keystores := make([]*keymanager.Keystore, numKeystores)
+		passwords := make([]string, numKeystores)
 		publicKeys := make([][48]byte, numKeystores)
 		for i := 0; i < numKeystores; i++ {
 			keystores[i] = createRandomKeystore(t, password)
 			pubKey, err := hex.DecodeString(keystores[i].Pubkey)
 			require.NoError(t, err)
 			publicKeys[i] = bytesutil.ToBytes48(pubKey)
+			passwords[i] = password
 		}
 
 		// Create a validator database.
@@ -176,7 +193,7 @@ func TestServer_ImportKeystores(t *testing.T) {
 
 		resp, err := s.ImportKeystores(context.Background(), &ethpbservice.ImportKeystoresRequest{
 			Keystores:          encodedKeystores,
-			Passwords:          []string{password},
+			Passwords:          passwords,
 			SlashingProtection: string(encodedSlashingProtection),
 		})
 		require.NoError(t, err)
@@ -189,46 +206,23 @@ func TestServer_ImportKeystores(t *testing.T) {
 
 func TestServer_DeleteKeystores(t *testing.T) {
 	ctx := context.Background()
-	t.Run("wallet not ready", func(t *testing.T) {
-		s := Server{}
-		_, err := s.DeleteKeystores(context.Background(), nil)
-		require.ErrorContains(t, "Wallet not ready", err)
-	})
-	localWalletDir := setupWalletDir(t)
-	defaultWalletPath = localWalletDir
-	w, err := accounts.CreateWalletWithKeymanager(ctx, &accounts.CreateWalletConfig{
-		WalletCfg: &wallet.Config{
-			WalletDir:      defaultWalletPath,
-			KeymanagerKind: keymanager.Derived,
-			WalletPassword: strongPass,
-		},
-		SkipMnemonicConfirm: true,
-	})
-	require.NoError(t, err)
-	km, err := w.InitializeKeymanager(ctx, iface.InitKeymanagerConfig{ListenForChanges: false})
-	require.NoError(t, err)
+	srv := setupServerWithWallet(t)
 
-	s := &Server{
-		keymanager:        km,
-		walletInitialized: true,
-		wallet:            w,
-	}
-	numAccounts := 50
-	dr, ok := km.(*derived.Keymanager)
+	// We recover 3 accounts from a test mnemonic.
+	numAccounts := 3
+	dr, ok := srv.keymanager.(*derived.Keymanager)
 	require.Equal(t, true, ok)
-	err = dr.RecoverAccountsFromMnemonic(ctx, mocks.TestMnemonic, "", numAccounts)
+	err := dr.RecoverAccountsFromMnemonic(ctx, mocks.TestMnemonic, "", numAccounts)
 	require.NoError(t, err)
-
-	publicKeys, err := km.FetchValidatingPublicKeys(ctx)
+	publicKeys, err := dr.FetchValidatingPublicKeys(ctx)
 	require.NoError(t, err)
-	require.Equal(t, numAccounts, len(publicKeys))
 
 	// Create a validator database.
 	validatorDB, err := kv.NewKVStore(ctx, defaultWalletPath, &kv.Config{
 		PubKeys: publicKeys,
 	})
 	require.NoError(t, err)
-	s.valDB = validatorDB
+	srv.valDB = validatorDB
 
 	// Have to close it after import is done otherwise it complains db is not open.
 	defer func() {
@@ -248,39 +242,115 @@ func TestServer_DeleteKeystores(t *testing.T) {
 	encoded, err := json.Marshal(mockJSON)
 	require.NoError(t, err)
 
-	_, err = s.ImportSlashingProtection(ctx, &validatorpb.ImportSlashingProtectionRequest{
+	_, err = srv.ImportSlashingProtection(ctx, &validatorpb.ImportSlashingProtectionRequest{
 		SlashingProtectionJson: string(encoded),
 	})
 	require.NoError(t, err)
-	rawPubKeys := make([][]byte, numAccounts)
-	for i := 0; i < numAccounts; i++ {
-		rawPubKeys[i] = publicKeys[i][:]
+
+	// For ease of test setup, we'll give each public key a string identifier.
+	publicKeysWithId := map[string][48]byte{
+		"a": publicKeys[0],
+		"b": publicKeys[1],
+		"c": publicKeys[2],
 	}
 
-	// Deletes properly and returns slashing protection history.
-	resp, err := s.DeleteKeystores(ctx, &ethpbservice.DeleteKeystoresRequest{
-		PublicKeys: rawPubKeys,
-	})
-	require.NoError(t, err)
-	require.Equal(t, numAccounts, len(resp.Statuses))
-	for _, status := range resp.Statuses {
-		require.Equal(t, ethpbservice.DeletedKeystoreStatus_DELETED, status.Status)
+	type keyCase struct {
+		id                 string
+		wantProtectionData bool
 	}
-	publicKeys, err = km.FetchValidatingPublicKeys(ctx)
-	require.NoError(t, err)
-	require.Equal(t, 0, len(publicKeys))
-	require.Equal(t, numAccounts, len(mockJSON.Data))
+	tests := []struct {
+		keys         []*keyCase
+		wantStatuses []ethpbservice.DeletedKeystoreStatus_Status
+	}{
+		{
+			keys: []*keyCase{
+				{id: "a", wantProtectionData: true},
+				{id: "a", wantProtectionData: true},
+				{id: "d"},
+				{id: "c", wantProtectionData: true},
+			},
+			wantStatuses: []ethpbservice.DeletedKeystoreStatus_Status{
+				ethpbservice.DeletedKeystoreStatus_DELETED,
+				ethpbservice.DeletedKeystoreStatus_NOT_ACTIVE,
+				ethpbservice.DeletedKeystoreStatus_NOT_FOUND,
+				ethpbservice.DeletedKeystoreStatus_DELETED,
+			},
+		},
+		{
+			keys: []*keyCase{
+				{id: "a", wantProtectionData: true},
+				{id: "c", wantProtectionData: true},
+			},
+			wantStatuses: []ethpbservice.DeletedKeystoreStatus_Status{
+				ethpbservice.DeletedKeystoreStatus_NOT_ACTIVE,
+				ethpbservice.DeletedKeystoreStatus_NOT_ACTIVE,
+			},
+		},
+		{
+			keys: []*keyCase{
+				{id: "x"},
+			},
+			wantStatuses: []ethpbservice.DeletedKeystoreStatus_Status{
+				ethpbservice.DeletedKeystoreStatus_NOT_FOUND,
+			},
+		},
+	}
+	for _, tc := range tests {
+		keys := make([][]byte, len(tc.keys))
+		for i := 0; i < len(tc.keys); i++ {
+			pk := publicKeysWithId[tc.keys[i].id]
+			keys[i] = pk[:]
+		}
+		resp, err := srv.DeleteKeystores(ctx, &ethpbservice.DeleteKeystoresRequest{PublicKeys: keys})
+		require.NoError(t, err)
+		require.Equal(t, len(keys), len(resp.Statuses))
+		slashingProtectionData := &format.EIPSlashingProtectionFormat{}
+		require.NoError(t, json.Unmarshal([]byte(resp.SlashingProtection), slashingProtectionData))
+		require.Equal(t, true, len(slashingProtectionData.Data) > 0)
 
-	// Returns slashing protection history if already deleted.
-	resp, err = s.DeleteKeystores(ctx, &ethpbservice.DeleteKeystoresRequest{
-		PublicKeys: rawPubKeys,
+		for i := 0; i < len(tc.keys); i++ {
+			require.Equal(
+				t,
+				tc.wantStatuses[i],
+				resp.Statuses[i].Status,
+				fmt.Sprintf("Checking status for key %s", tc.keys[i].id),
+			)
+			if tc.keys[i].wantProtectionData {
+				// We check that we can find the key in the slashing protection data.
+				var found bool
+				for _, dt := range slashingProtectionData.Data {
+					if dt.Pubkey == fmt.Sprintf("%#x", keys[i]) {
+						found = true
+						break
+					}
+				}
+				require.Equal(t, true, found)
+			}
+		}
+	}
+}
+
+func setupServerWithWallet(t testing.TB) *Server {
+	ctx := context.Background()
+	localWalletDir := setupWalletDir(t)
+	defaultWalletPath = localWalletDir
+	w, err := accounts.CreateWalletWithKeymanager(ctx, &accounts.CreateWalletConfig{
+		WalletCfg: &wallet.Config{
+			WalletDir:      defaultWalletPath,
+			KeymanagerKind: keymanager.Derived,
+			WalletPassword: strongPass,
+		},
+		SkipMnemonicConfirm: true,
 	})
 	require.NoError(t, err)
-	require.Equal(t, numAccounts, len(resp.Statuses))
-	for _, status := range resp.Statuses {
-		require.Equal(t, ethpbservice.DeletedKeystoreStatus_NOT_FOUND, status.Status)
+	km, err := w.InitializeKeymanager(ctx, iface.InitKeymanagerConfig{ListenForChanges: false})
+	require.NoError(t, err)
+
+	return &Server{
+		keymanager:        km,
+		walletInitialized: true,
+		wallet:            w,
 	}
-	require.Equal(t, numAccounts, len(mockJSON.Data))
 }
 
 func createRandomKeystore(t testing.TB, password string) *keymanager.Keystore {