Track updates for validators (#939)

* Implement equality checks for PublicKey and ValidatorMetadata.
Expand ValidatorSetDelta

* Handle updates in ApplyDelta

* Additional tests

consensus/polybft/extra.go

@@ -144,7 +144,8 @@ func (i *Extra) UnmarshalRLPWith(v *fastrlp.Value) error {
 // createValidatorSetDelta calculates ValidatorSetDelta based on the provided old and new validator sets
 func createValidatorSetDelta(log hclog.Logger, oldValidatorSet,
 	newValidatorSet AccountSet) (*ValidatorSetDelta, error) {
-	var addedValidators AccountSet
+	addedValidators := make(AccountSet, 0)
+	updatedValidators := make(AccountSet, 0)
 
 	oldValidatorSetMap := make(map[types.Address]*ValidatorMetadata)
 	removedValidators := map[types.Address]int{}
@@ -158,15 +159,19 @@ func createValidatorSetDelta(log hclog.Logger, oldValidatorSet,
 
 	for _, newValidator := range newValidatorSet {
 		// Check if the validator is among both old and new validator set
-		oldValidator, ok := oldValidatorSetMap[newValidator.Address]
-		if ok {
-			if !oldValidator.Equals(newValidator) {
+		oldValidator, validatorExists := oldValidatorSetMap[newValidator.Address]
+		if validatorExists {
+			if !oldValidator.EqualAddressAndBlsKey(newValidator) {
 				return nil, fmt.Errorf("validator '%s' found in both old and new validator set, but its BLS keys differ",
 					newValidator.Address.String())
 			}
 
 			// If it is, then discard it from removed validators...
 			delete(removedValidators, newValidator.Address)
+
+			if !oldValidator.Equals(newValidator) {
+				updatedValidators = append(updatedValidators, newValidator)
+			}
 		} else {
 			// ...otherwise it is added
 			addedValidators = append(addedValidators, newValidator)
@@ -180,6 +185,7 @@ func createValidatorSetDelta(log hclog.Logger, oldValidatorSet,
 
 	delta := &ValidatorSetDelta{
 		Added:   addedValidators,
+		Updated: updatedValidators,
 		Removed: removedValsBitmap,
 	}
 
@@ -188,8 +194,10 @@ func createValidatorSetDelta(log hclog.Logger, oldValidatorSet,
 
 // ValidatorSetDelta holds information about added and removed validators compared to the previous epoch
 type ValidatorSetDelta struct {
-	// Added is the list of new validators for the epoch
+	// Added is the slice of added validators
 	Added AccountSet
+	// Updated is the slice of updated valiadtors
+	Updated AccountSet
 	// Removed is a bitmap of the validators removed from the set
 	Removed bitmap.Bitmap
 }
@@ -256,9 +264,11 @@ func (d *ValidatorSetDelta) UnmarshalRLPWith(v *fastrlp.Value) error {
 	return nil
 }
 
-// IsEmpty returns indication whether delta is empty (namely added and removed slices are empty)
+// IsEmpty returns indication whether delta is empty (namely added, updated slices and removed bitmap are empty)
 func (d *ValidatorSetDelta) IsEmpty() bool {
-	return len(d.Added) == 0 && d.Removed.Len() == 0
+	return len(d.Added) == 0 &&
+		len(d.Updated) == 0 &&
+		d.Removed.Len() == 0
 }
 
 // Copy creates deep copy of ValidatorSetDelta

consensus/polybft/extra_test.go

@@ -2,6 +2,7 @@ package polybft
 
 import (
 	"crypto/rand"
+	"math/big"
 	mrand "math/rand"
 	"testing"
 
@@ -350,13 +351,15 @@ func TestExtra_CreateValidatorSetDelta_Cases(t *testing.T) {
 		oldSet  []string
 		newSet  []string
 		added   []string
+		updated []string
 		removed []uint64
 	}{
 		{
 			"Simple",
 			[]string{"A", "B", "C", "E", "F"},
 			[]string{"B", "E", "H"},
 			[]string{"H"},
+			[]string{"B", "E"},
 			[]uint64{0, 2, 4},
 		},
 	}
@@ -375,20 +378,34 @@ func TestExtra_CreateValidatorSetDelta_Cases(t *testing.T) {
 			}
 
 			oldValidatorSet := vals.getPublicIdentities(c.oldSet...)
+			// update voting power to random value
+			maxVotingPower := big.NewInt(100)
+			for _, name := range c.updated {
+				v := vals.getValidator(name)
+				vp, err := rand.Int(rand.Reader, maxVotingPower)
+				require.NoError(t, err)
+				v.votingPower = vp.Uint64()
+			}
 			newValidatorSet := vals.getPublicIdentities(c.newSet...)
 
 			delta, err := createValidatorSetDelta(hclog.NewNullLogger(), oldValidatorSet, newValidatorSet)
 			require.NoError(t, err)
 
-			// added items
-			assert.Len(t, delta.Added, len(c.added))
-			for index, item := range c.added {
-				assert.Equal(t, delta.Added[index].Address, vals.getValidator(item).Address())
+			// added validators
+			require.Len(t, delta.Added, len(c.added))
+			for i, name := range c.added {
+				require.Equal(t, delta.Added[i].Address, vals.getValidator(name).Address())
 			}
 
-			// removed items
+			// removed validators
 			for _, i := range c.removed {
-				assert.True(t, delta.Removed.IsSet(i))
+				require.True(t, delta.Removed.IsSet(i))
+			}
+
+			// updated validators
+			require.Len(t, delta.Updated, len(c.updated))
+			for i, name := range c.updated {
+				require.Equal(t, delta.Updated[i].Address, vals.getValidator(name).Address())
 			}
 		})
 	}

consensus/polybft/mocks_test.go

@@ -396,6 +396,23 @@ func (v *testValidators) toValidatorSet() *validatorSet {
 	return newValidatorSet(types.Address{}, v.getPublicIdentities())
 }
 
+func (v *testValidators) updateVotingPowers(votingPowersMap map[string]uint64) AccountSet {
+	if len(votingPowersMap) == 0 {
+		return AccountSet{}
+	}
+
+	aliases := []string{}
+	for alias := range votingPowersMap {
+		aliases = append(aliases, alias)
+	}
+
+	v.iterAcct(aliases, func(t *testValidator) {
+		t.votingPower = votingPowersMap[t.alias]
+	})
+
+	return v.getPublicIdentities(aliases...)
+}
+
 type testValidator struct {
 	alias       string
 	account     *wallet.Account

consensus/polybft/validator_metadata.go

@@ -26,12 +26,21 @@ type ValidatorMetadata struct {
 	VotingPower uint64
 }
 
-// Equals compares ValidatorMetadata equality
+// Equals checks ValidatorMetadata equality
 func (v *ValidatorMetadata) Equals(b *ValidatorMetadata) bool {
 	if b == nil {
 		return false
 	}
 
+	return v.EqualAddressAndBlsKey(b) && v.VotingPower == b.VotingPower
+}
+
+// EqualAddressAndBlsKey checks ValidatorMetadata equality against Address and BlsKey fields
+func (v *ValidatorMetadata) EqualAddressAndBlsKey(b *ValidatorMetadata) bool {
+	if b == nil {
+		return false
+	}
+
 	return v.Address == b.Address && reflect.DeepEqual(v.BlsKey, b.BlsKey)
 }
 
@@ -268,6 +277,17 @@ func (as AccountSet) ApplyDelta(validatorsDelta *ValidatorSetDelta) (AccountSet,
 		validators = append(validators, addedValidator)
 	}
 
+	// Handle updated validators (find them in the validators slice and insert to appropriate index)
+	for _, updatedValidator := range validatorsDelta.Updated {
+		validatorIndex := validators.Index(updatedValidator.Address)
+		if validatorIndex == -1 {
+			return nil, fmt.Errorf("incorrect delta provided: validator %s is marked as updated but not found in the validators",
+				updatedValidator.Address)
+		}
+
+		validators[validatorIndex] = updatedValidator
+	}
+
 	return validators, nil
 }
 

consensus/polybft/validator_metadata_test.go

@@ -10,6 +10,36 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func TestValidatorMetadata_Equals(t *testing.T) {
+	t.Parallel()
+
+	v := newTestValidator("A", 10)
+	validatorAcc := v.ValidatorMetadata()
+	// proper validator metadata instance doesn't equal to nil
+	require.False(t, validatorAcc.Equals(nil))
+	// same instances of validator metadata are equal
+	require.True(t, validatorAcc.Equals(v.ValidatorMetadata()))
+
+	// update voting power => validator metadata instances aren't equal
+	validatorAcc.VotingPower = 50
+	require.False(t, validatorAcc.Equals(v.ValidatorMetadata()))
+}
+
+func TestValidatorMetadata_EqualAddressAndBlsKey(t *testing.T) {
+	t.Parallel()
+
+	v := newTestValidator("A", 10)
+	validatorAcc := v.ValidatorMetadata()
+	// proper validator metadata instance doesn't equal to nil
+	require.False(t, validatorAcc.EqualAddressAndBlsKey(nil))
+	// same instances of validator metadata are equal
+	require.True(t, validatorAcc.EqualAddressAndBlsKey(v.ValidatorMetadata()))
+
+	// update voting power => validator metadata instances aren't equal
+	validatorAcc.Address = types.BytesToAddress(generateRandomBytes(t))
+	require.False(t, validatorAcc.EqualAddressAndBlsKey(v.ValidatorMetadata()))
+}
+
 func TestAccountSet_GetAddresses(t *testing.T) {
 	t.Parallel()
 
@@ -85,13 +115,12 @@ func TestAccountSet_Len(t *testing.T) {
 func TestAccountSet_ApplyDelta(t *testing.T) {
 	t.Parallel()
 
-	// Add a couple of validators to the snapshot => validators are present in the snapshot after applying such delta
-	vals := newTestValidatorsWithAliases([]string{"A", "B", "C", "D", "E", "F"})
-
 	type Step struct {
-		added  []string
-		remove []uint64
-		expect []string
+		added    []string
+		updated  map[string]uint64
+		removed  []uint64
+		expected map[string]uint64
+		errMsg   string
 	}
 
 	cases := []struct {
@@ -104,13 +133,19 @@ func TestAccountSet_ApplyDelta(t *testing.T) {
 				{
 					[]string{"A", "B", "C", "D"},
 					nil,
-					[]string{"A", "B", "C", "D"},
+					nil,
+					map[string]uint64{"A": 1, "B": 1, "C": 1, "D": 1},
+					"",
 				},
 				{
-					// add two new items and remove 3 (one does not exists)
+					// add two new validators and remove 3 (one does not exists)
+					// update voting powers to subset of validators
+					// (two of them added in the previous step and one added in the current one)
 					[]string{"E", "F"},
+					map[string]uint64{"A": 30, "D": 10, "E": 5},
 					[]uint64{1, 2, 5},
-					[]string{"A", "D", "E", "F"},
+					map[string]uint64{"A": 30, "D": 10, "E": 5, "F": 1},
+					"",
 				},
 			},
 		},
@@ -119,48 +154,97 @@ func TestAccountSet_ApplyDelta(t *testing.T) {
 			steps: []*Step{
 				{
 					[]string{"A"},
+					nil,
 					[]uint64{0},
-					[]string{"A"},
+					map[string]uint64{"A": 1},
+					"",
+				},
+			},
+		},
+		{
+			name: "AddSameValidatorTwice",
+			steps: []*Step{
+				{
+					[]string{"A", "A"},
+					nil,
+					nil,
+					nil,
+					"is already present in the validators snapshot",
+				},
+			},
+		},
+		{
+			name: "UpdateNonExistingValidator",
+			steps: []*Step{
+				{
+					nil,
+					map[string]uint64{"B": 5},
+					nil,
+					nil,
+					"incorrect delta provided: validator",
 				},
 			},
 		},
 	}
 
 	for _, cc := range cases {
-		snapshot := AccountSet{}
-
+		cc := cc
 		t.Run(cc.name, func(t *testing.T) {
 			t.Parallel()
 
+			snapshot := AccountSet{}
+			// Add a couple of validators to the snapshot => validators are present in the snapshot after applying such delta
+			vals := newTestValidatorsWithAliases([]string{"A", "B", "C", "D", "E", "F"})
+
 			for _, step := range cc.steps {
+				addedValidators := AccountSet{}
+				if step.added != nil {
+					addedValidators = vals.getPublicIdentities(step.added...)
+				}
 				delta := &ValidatorSetDelta{
-					Added:   vals.getPublicIdentities(step.added...),
+					Added:   addedValidators,
 					Removed: bitmap.Bitmap{},
 				}
-				for _, i := range step.remove {
+				for _, i := range step.removed {
 					delta.Removed.Set(i)
 				}
 
+				// update voting powers
+				delta.Updated = vals.updateVotingPowers(step.updated)
+
 				// apply delta
 				var err error
 				snapshot, err = snapshot.ApplyDelta(delta)
-				assert.NoError(t, err)
+				if step.errMsg != "" {
+					require.ErrorContains(t, err, step.errMsg)
+					require.Nil(t, snapshot)
 
-				// validate validator set
-				if len(step.expect) != snapshot.Len() {
-					t.Fatal("incorrect length")
+					return
 				}
-				for _, acct := range step.expect {
-					v := vals.getValidator(acct)
-					if !snapshot.ContainsAddress(v.ValidatorMetadata().Address) {
-						t.Fatalf("not found '%s'", acct)
-					}
+				require.NoError(t, err)
+
+				// validate validator set
+				require.Equal(t, len(step.expected), snapshot.Len())
+				for validatorAlias, votingPower := range step.expected {
+					v := vals.getValidator(validatorAlias).ValidatorMetadata()
+					require.True(t, snapshot.ContainsAddress(v.Address), "validator '%s' not found in snapshot", validatorAlias)
+					require.Equal(t, votingPower, v.VotingPower)
 				}
 			}
 		})
 	}
 }
 
+func TestAccountSet_ApplyEmptyDelta(t *testing.T) {
+	t.Parallel()
+
+	v := newTestValidatorsWithAliases([]string{"A", "B", "C", "D", "E", "F"})
+	validatorAccs := v.getPublicIdentities()
+	validators, err := validatorAccs.ApplyDelta(nil)
+	require.NoError(t, err)
+	require.Equal(t, validatorAccs, validators)
+}
+
 func TestAccountSet_Hash(t *testing.T) {
 	t.Parallel()