fixes after review: renamings, change mutex to read when applicable

consensus/signing/errors.go

@@ -26,10 +26,7 @@ var ErrNilKeyGenerator = errors.New("key generator is nil")
 // ErrNilPublicKeys is raised when public keys are expected but received nil
 var ErrNilPublicKeys = errors.New("public keys are nil")
 
-// ErrNilSingleSigner singals that a nil single signer has been provided
-var ErrNilSingleSigner = errors.New("single signer is nil")
-
-// ErrNilMultiSignerContainer singals that a nil multi signer container has been provided
+// ErrNilMultiSignerContainer is raised when a nil multi signer container has been provided
 var ErrNilMultiSignerContainer = errors.New("multi signer container is nil")
 
 // ErrIndexOutOfBounds is raised when an out of bound index is used

consensus/signing/signing.go

@@ -40,13 +40,13 @@ func NewSignatureHolder(args ArgsSignatureHolder) (*signatureHolder, error) {
 	sigSharesSize := uint16(len(args.PubKeys))
 	sigShares := make([][]byte, sigSharesSize)
 
-	pybKeysBytes, err := convertStringsToPubKeysBytes(args.PubKeys)
+	pubKeysBytes, err := convertStringsToPubKeysBytes(args.PubKeys)
 	if err != nil {
 		return nil, err
 	}
 
 	data := &signatureHolderData{
-		pubKeys:   pybKeysBytes,
+		pubKeys:   pubKeysBytes,
 		privKey:   args.PrivKeyBytes,
 		sigShares: sigShares,
 	}
@@ -76,8 +76,8 @@ func checkArgs(args ArgsSignatureHolder) error {
 	return nil
 }
 
-// Create generated a signature holder component and initializes corresponding fields
-func (sh *signatureHolder) Create(pubKeys []string, index uint16) (*signatureHolder, error) {
+// Create generates a signature holder component and initializes corresponding fields
+func (sh *signatureHolder) Create(pubKeys []string) (*signatureHolder, error) {
 	sh.mutSigningData.RLock()
 	privKey := sh.data.privKey
 	sh.mutSigningData.RUnlock()
@@ -150,8 +150,8 @@ func (sh *signatureHolder) VerifySignatureShare(index uint16, sig []byte, messag
 		return ErrInvalidSignature
 	}
 
-	sh.mutSigningData.Lock()
-	defer sh.mutSigningData.Unlock()
+	sh.mutSigningData.RLock()
+	defer sh.mutSigningData.RUnlock()
 
 	indexOutOfBounds := index >= uint16(len(sh.data.pubKeys))
 	if indexOutOfBounds {
@@ -188,8 +188,8 @@ func (sh *signatureHolder) StoreSignatureShare(index uint16, sig []byte) error {
 
 // SignatureShare returns the partial signature set for given index
 func (sh *signatureHolder) SignatureShare(index uint16) ([]byte, error) {
-	sh.mutSigningData.Lock()
-	defer sh.mutSigningData.Unlock()
+	sh.mutSigningData.RLock()
+	defer sh.mutSigningData.RUnlock()
 
 	if int(index) >= len(sh.data.sigShares) {
 		return nil, ErrIndexOutOfBounds
@@ -203,18 +203,18 @@ func (sh *signatureHolder) SignatureShare(index uint16) ([]byte, error) {
 }
 
 // not concurrent safe, should be used under RLock mutex
-func (sh *signatureHolder) isIndexInBitmap(index uint16, bitmap []byte) error {
+func (sh *signatureHolder) isIndexInBitmap(index uint16, bitmap []byte) bool {
 	indexOutOfBounds := index >= uint16(len(sh.data.pubKeys))
 	if indexOutOfBounds {
-		return ErrIndexOutOfBounds
+		return false
 	}
 
 	indexNotInBitmap := bitmap[index/8]&(1<<uint8(index%8)) == 0
 	if indexNotInBitmap {
-		return ErrIndexNotSelected
+		return false
 	}
 
-	return nil
+	return true
 }
 
 // AggregateSigs aggregates all collected partial signatures
@@ -235,21 +235,20 @@ func (sh *signatureHolder) AggregateSigs(bitmap []byte, epoch uint32) ([]byte, e
 	signatures := make([][]byte, 0, len(sh.data.sigShares))
 	pubKeysSigners := make([][]byte, 0, len(sh.data.sigShares))
 
+	multiSigner, err := sh.multiSignerContainer.GetMultiSigner(epoch)
+	if err != nil {
+		return nil, err
+	}
+
 	for i := range sh.data.sigShares {
-		err := sh.isIndexInBitmap(uint16(i), bitmap)
-		if err != nil {
+		if !sh.isIndexInBitmap(uint16(i), bitmap) {
 			continue
 		}
 
 		signatures = append(signatures, sh.data.sigShares[i])
 		pubKeysSigners = append(pubKeysSigners, sh.data.pubKeys[i])
 	}
 
-	multiSigner, err := sh.multiSignerContainer.GetMultiSigner(epoch)
-	if err != nil {
-		return nil, err
-	}
-
 	return multiSigner.AggregateSigs(pubKeysSigners, signatures)
 }
 
@@ -270,28 +269,27 @@ func (sh *signatureHolder) Verify(message []byte, bitmap []byte, epoch uint32) e
 		return ErrNilBitmap
 	}
 
-	sh.mutSigningData.Lock()
-	defer sh.mutSigningData.Unlock()
+	sh.mutSigningData.RLock()
+	defer sh.mutSigningData.RUnlock()
 
 	maxFlags := len(bitmap) * 8
 	flagsMismatch := maxFlags < len(sh.data.pubKeys)
 	if flagsMismatch {
 		return ErrBitmapMismatch
 	}
 
+	multiSigner, err := sh.multiSignerContainer.GetMultiSigner(epoch)
+	if err != nil {
+		return err
+	}
+
 	pubKeys := make([][]byte, 0)
-	for i := range sh.data.pubKeys {
-		err := sh.isIndexInBitmap(uint16(i), bitmap)
-		if err != nil {
+	for i, pk := range sh.data.pubKeys {
+		if !sh.isIndexInBitmap(uint16(i), bitmap) {
 			continue
 		}
 
-		pubKeys = append(pubKeys, sh.data.pubKeys[i])
-	}
-
-	multiSigner, err := sh.multiSignerContainer.GetMultiSigner(epoch)
-	if err != nil {
-		return err
+		pubKeys = append(pubKeys, pk)
 	}
 
 	return multiSigner.VerifyAggregatedSig(pubKeys, message, sh.data.aggSig)

consensus/signing/signing_test.go

@@ -76,7 +76,7 @@ func TestNewSigner(t *testing.T) {
 	})
 }
 
-func TestCreate(t *testing.T) {
+func TestSignatureHolder_Create(t *testing.T) {
 	t.Parallel()
 
 	t.Run("empty pubkeys in list", func(t *testing.T) {
@@ -89,7 +89,7 @@ func TestCreate(t *testing.T) {
 		require.NotNil(t, signer)
 
 		pubKeys := []string{"pubKey1", ""}
-		createdSigner, err := signer.Create(pubKeys, uint16(2))
+		createdSigner, err := signer.Create(pubKeys)
 		require.Nil(t, createdSigner)
 		require.Equal(t, signing.ErrEmptyPubKeyString, err)
 	})
@@ -104,13 +104,13 @@ func TestCreate(t *testing.T) {
 		require.NotNil(t, signer)
 
 		pubKeys := []string{"pubKey1", "pubKey2"}
-		createdSigner, err := signer.Create(pubKeys, uint16(2))
+		createdSigner, err := signer.Create(pubKeys)
 		require.Nil(t, err)
 		require.NotNil(t, createdSigner)
 	})
 }
 
-func TestReset(t *testing.T) {
+func TestSignatureHolder_Reset(t *testing.T) {
 	t.Parallel()
 
 	t.Run("nil public keys", func(t *testing.T) {
@@ -144,7 +144,7 @@ func TestReset(t *testing.T) {
 	})
 }
 
-func TestCreateSignatureShare(t *testing.T) {
+func TestSignatureHolder_CreateSignatureShare(t *testing.T) {
 	t.Parallel()
 
 	selfIndex := uint16(0)
@@ -198,7 +198,7 @@ func TestCreateSignatureShare(t *testing.T) {
 	})
 }
 
-func TestVerifySignatureShare(t *testing.T) {
+func TestSignatureHolder_VerifySignatureShare(t *testing.T) {
 	t.Parallel()
 
 	ownIndex := uint16(1)
@@ -261,7 +261,7 @@ func TestVerifySignatureShare(t *testing.T) {
 	})
 }
 
-func TestStoreSignatureShare(t *testing.T) {
+func TestSignatureHolder_StoreSignatureShare(t *testing.T) {
 	t.Parallel()
 
 	ownIndex := uint16(2)
@@ -303,7 +303,7 @@ func TestStoreSignatureShare(t *testing.T) {
 	})
 }
 
-func TestSignatureShare(t *testing.T) {
+func TestSignatureHolder_SignatureShare(t *testing.T) {
 	t.Parallel()
 
 	t.Run("index out of bounds", func(t *testing.T) {
@@ -359,7 +359,7 @@ func TestSignatureShare(t *testing.T) {
 	})
 }
 
-func TestAggregateSigs(t *testing.T) {
+func TestSignatureHolder_AggregateSigs(t *testing.T) {
 	t.Parallel()
 
 	epoch := uint32(0)
@@ -453,7 +453,7 @@ func TestAggregateSigs(t *testing.T) {
 	})
 }
 
-func TestVerify(t *testing.T) {
+func TestSignatureHolder_Verify(t *testing.T) {
 	t.Parallel()
 
 	message := []byte("message")

consensus/spos/bls/subroundSignature.go

@@ -68,7 +68,7 @@ func (sr *subroundSignature) doSignatureJob(_ context.Context) bool {
 	if !sr.CanDoSubroundJob(sr.Current()) {
 		return false
 	}
-	if sr.Header == nil {
+	if check.IfNil(sr.Header) {
 		log.Error("doSignatureJob", "error", spos.ErrNilHeader)
 		return false
 	}
@@ -79,11 +79,6 @@ func (sr *subroundSignature) doSignatureJob(_ context.Context) bool {
 		return false
 	}
 
-	if check.IfNil(sr.Header) {
-		log.Error("doSignatureJob", "error", spos.ErrNilHeader)
-		return false
-	}
-
 	signatureShare, err := sr.SignatureHandler().CreateSignatureShare(sr.GetData(), uint16(selfIndex), sr.Header.GetEpoch())
 	if err != nil {
 		log.Debug("doSignatureJob.CreateSignatureShare", "error", err.Error())