Merge pull request #2133 from keep-network/failed-to-check-if-stale-f…

…or-group-public-key

Failed to check if stale for group public key

A node might run into situation when there is a slight delay with syncing 
to the recent state of the chain. It might result in error logging "Group does 
not exist", even though it was just added to the chain. In order to avoid the 
described scenario, we can skip checking the latest added group if it is a 
"stale group".

pkg/beacon/beacon.go

@@ -186,7 +186,7 @@ func Initialize(
 			registration.GroupPublicKey,
 			registration.BlockNumber,
 		)
-		go groupRegistry.UnregisterStaleGroups()
+		go groupRegistry.UnregisterStaleGroups(registration.GroupPublicKey)
 	})
 
 	return nil

pkg/beacon/relay/registry/groups.go

@@ -1,6 +1,7 @@
 package registry
 
 import (
+	"bytes"
 	"encoding/hex"
 	"fmt"
 	"sync"
@@ -83,7 +84,7 @@ func (g *Groups) GetGroup(groupPublicKey []byte) []*Membership {
 // a new operation and it cannot have an ongoing operation for which it could be
 // selected before it expired. Such a group can be safely removed from the registry
 // and archived in the underlying storage.
-func (g *Groups) UnregisterStaleGroups() {
+func (g *Groups) UnregisterStaleGroups(latestGroupPublicKey []byte) {
 	g.mutex.Lock()
 	defer g.mutex.Unlock()
 
@@ -97,41 +98,47 @@ func (g *Groups) UnregisterStaleGroups() {
 			continue
 		}
 
-		isStaleGroup, err := g.relayChain.IsStaleGroup(publicKeyBytes)
-		if err != nil {
-			logger.Errorf(
-				"failed to check if stale for group with public key [%s]: [%v]",
-				publicKey,
-				err,
-			)
-			continue
-		}
-
-		if isStaleGroup {
-			if len(memberships) == 0 {
+		// There is no need to check if the latest added group is a stale group.
+		// It is also to avoid a scenario when there is a delay to sync with the
+		// recent state of the chain which might lead to loggin a false positive
+		// error: "Group does not exist".
+		if !bytes.Equal(latestGroupPublicKey, publicKeyBytes) {
+			isStaleGroup, err := g.relayChain.IsStaleGroup(publicKeyBytes)
+			if err != nil {
 				logger.Errorf(
-					"inconsistent state; group with public key [%s] has no members",
+					"failed to check if stale for group with public key [%s]: [%v]",
 					publicKey,
+					err,
 				)
 				continue
 			}
 
-			compressedPublicKey := memberships[0].Signer.GroupPublicKeyBytesCompressed()
-			err = g.storage.archive(compressedPublicKey)
-			if err != nil {
-				logger.Errorf("failed to archive group with compressed public key [%s]: [%v]",
+			if isStaleGroup {
+				if len(memberships) == 0 {
+					logger.Errorf(
+						"inconsistent state; group with public key [%s] has no members",
+						publicKey,
+					)
+					continue
+				}
+
+				compressedPublicKey := memberships[0].Signer.GroupPublicKeyBytesCompressed()
+				err = g.storage.archive(compressedPublicKey)
+				if err != nil {
+					logger.Errorf("failed to archive group with compressed public key [%s]: [%v]",
+						hex.EncodeToString(compressedPublicKey),
+						err,
+					)
+					continue
+				}
+
+				logger.Infof(
+					"archived group with compressed public key [%s]",
 					hex.EncodeToString(compressedPublicKey),
-					err,
 				)
-				continue
-			}
-
-			logger.Infof(
-				"archived group with compressed public key [%s]",
-				hex.EncodeToString(compressedPublicKey),
-			)
 
-			delete(g.myGroups, publicKey)
+				delete(g.myGroups, publicKey)
+			}
 		}
 	}
 }

pkg/beacon/relay/registry/groups_test.go

@@ -119,6 +119,7 @@ func TestLoadGroup(t *testing.T) {
 func TestUnregisterStaleGroups(t *testing.T) {
 	mockChain := &mockGroupRegistrationInterface{
 		groupsToRemove: [][]byte{},
+		groupsCheckedIfStale: make(map[string]bool),
 	}
 
 	gr := NewGroupRegistry(mockChain, persistenceMock)
@@ -129,7 +130,7 @@ func TestUnregisterStaleGroups(t *testing.T) {
 
 	mockChain.markAsStale(signer2.GroupPublicKeyBytes())
 
-	gr.UnregisterStaleGroups()
+	gr.UnregisterStaleGroups(signer3.GroupPublicKeyBytes())
 
 	group1 := gr.GetGroup(signer1.GroupPublicKeyBytes())
 	if group1 == nil {
@@ -149,11 +150,41 @@ func TestUnregisterStaleGroups(t *testing.T) {
 	if group3 == nil {
 		t.Fatalf("Expecting a group, but nil was returned instead")
 	}
+}
+
+func TestUnregisterStaleGroupsSkipLastGroupCheck(t *testing.T) {
+	mockChain := &mockGroupRegistrationInterface{
+		groupsToRemove:       [][]byte{},
+		groupsCheckedIfStale: make(map[string]bool),
+	}
+
+	gr := NewGroupRegistry(mockChain, persistenceMock)
+
+	gr.RegisterGroup(signer1, channelName1)
+	gr.RegisterGroup(signer2, channelName1)
+	gr.RegisterGroup(signer3, channelName1)
+
+	gr.UnregisterStaleGroups(signer3.GroupPublicKeyBytes())
+
+	group1PublicKeyString := groupKeyToString(signer1.GroupPublicKeyBytes())
+	if mockChain.groupsCheckedIfStale[group1PublicKeyString] != true {
+		t.Fatalf("IsStaleGroup() was expected to be called for the first group")
+	}
+
+	group2PublicKeyString := groupKeyToString(signer2.GroupPublicKeyBytes())
+	if mockChain.groupsCheckedIfStale[group2PublicKeyString] != true {
+		t.Fatalf("IsStaleGroup() was expected to be called for the second group")
+	}
 
+	group3PublicKeyString := groupKeyToString(signer3.GroupPublicKeyBytes())
+	if mockChain.groupsCheckedIfStale[group3PublicKeyString] != false {
+		t.Fatalf("IsStaleGroup() was expected to not be called for the third group")
+	}
 }
 
 type mockGroupRegistrationInterface struct {
-	groupsToRemove [][]byte
+	groupsToRemove       [][]byte
+	groupsCheckedIfStale map[string]bool
 }
 
 func (mgri *mockGroupRegistrationInterface) markAsStale(publicKey []byte) {
@@ -167,6 +198,7 @@ func (mgri *mockGroupRegistrationInterface) OnGroupRegistered(
 }
 
 func (mgri *mockGroupRegistrationInterface) IsStaleGroup(groupPublicKey []byte) (bool, error) {
+	mgri.groupsCheckedIfStale[groupKeyToString(groupPublicKey)] = true
 	for _, groupToRemove := range mgri.groupsToRemove {
 		if bytes.Compare(groupToRemove, groupPublicKey) == 0 {
 			return true, nil