Remove redundant code and Add Safe check in bulding committed seals (#53)

* Remove redundant local variables in validateProposal

* Add safe check in ExtractCommittedSeals

* Fix lint error

core/ibft.go

@@ -658,7 +658,6 @@ func (i *IBFT) validateProposal(msg *proto.Message, view *proto.View) bool {
 		round  = view.Round
 
 		proposalHash = messages.ExtractProposalHash(msg)
-		certificate  = messages.ExtractRoundChangeCertificate(msg)
 		rcc          = messages.ExtractRoundChangeCertificate(msg)
 	)
 
@@ -668,12 +667,12 @@ func (i *IBFT) validateProposal(msg *proto.Message, view *proto.View) bool {
 	}
 
 	// Make sure there is a certificate
-	if certificate == nil {
+	if rcc == nil {
 		return false
 	}
 
 	// Make sure there are Quorum RCC
-	if !i.backend.HasQuorum(view.Height, certificate.RoundChangeMessages, proto.MessageType_ROUND_CHANGE) {
+	if !i.backend.HasQuorum(view.Height, rcc.RoundChangeMessages, proto.MessageType_ROUND_CHANGE) {
 		return false
 	}
 
@@ -682,12 +681,12 @@ func (i *IBFT) validateProposal(msg *proto.Message, view *proto.View) bool {
 		return false
 	}
 
-	if !messages.HasUniqueSenders(certificate.RoundChangeMessages) {
+	if !messages.HasUniqueSenders(rcc.RoundChangeMessages) {
 		return false
 	}
 
 	// Make sure all messages in the RCC are valid Round Change messages
-	for _, rc := range certificate.RoundChangeMessages {
+	for _, rc := range rcc.RoundChangeMessages {
 		// Make sure the message is a Round Change message
 		if rc.Type != proto.MessageType_ROUND_CHANGE {
 			return false
@@ -916,10 +915,16 @@ func (i *IBFT) handleCommit(view *proto.View) bool {
 		return false
 	}
 
+	commitSeals, err := messages.ExtractCommittedSeals(commitMessages)
+	if err != nil {
+		// safe check
+		i.log.Error("failed to extract committed seals from commit messages: %+v", err)
+
+		return false
+	}
+
 	// Set the committed seals
-	i.state.setCommittedSeals(
-		messages.ExtractCommittedSeals(commitMessages),
-	)
+	i.state.setCommittedSeals(commitSeals)
 
 	//	Move to the fin state
 	i.state.changeState(fin)

messages/helpers.go

@@ -2,10 +2,16 @@ package messages
 
 import (
 	"bytes"
+	"errors"
 
 	"github.com/0xPolygon/go-ibft/messages/proto"
 )
 
+var (
+	// ErrWrongCommitMessageType is an error indicating wrong type in commit messages
+	ErrWrongCommitMessageType = errors.New("wrong type message is included in COMMIT messages")
+)
+
 // CommittedSeal Validator proof of signing a committed block
 type CommittedSeal struct {
 	Signer    []byte
@@ -13,18 +19,19 @@ type CommittedSeal struct {
 }
 
 // ExtractCommittedSeals extracts the committed seals from the passed in messages
-func ExtractCommittedSeals(commitMessages []*proto.Message) []*CommittedSeal {
+func ExtractCommittedSeals(commitMessages []*proto.Message) ([]*CommittedSeal, error) {
 	committedSeals := make([]*CommittedSeal, 0)
 
 	for _, commitMessage := range commitMessages {
 		if commitMessage.Type != proto.MessageType_COMMIT {
-			continue
+			// safe check
+			return nil, ErrWrongCommitMessageType
 		}
 
 		committedSeals = append(committedSeals, ExtractCommittedSeal(commitMessage))
 	}
 
-	return committedSeals
+	return committedSeals, nil
 }
 
 // ExtractCommittedSeal extracts the committed seal from the passed in message
@@ -150,9 +157,7 @@ func HaveSameProposalHash(messages []*proto.Message) bool {
 			extractedHash = ExtractProposalHash(message)
 		case proto.MessageType_PREPARE:
 			extractedHash = ExtractPrepareHash(message)
-		case proto.MessageType_COMMIT:
-			return false
-		case proto.MessageType_ROUND_CHANGE:
+		case proto.MessageType_COMMIT, proto.MessageType_ROUND_CHANGE:
 			return false
 		default:
 			return false

messages/helpers_test.go

@@ -11,37 +11,76 @@ import (
 func TestMessages_ExtractCommittedSeals(t *testing.T) {
 	t.Parallel()
 
-	signer := []byte("signer")
-	committedSeal := []byte("committed seal")
+	var (
+		committedSeal = []byte("committed seal")
+	)
 
-	commitMessage := &proto.Message{
+	createCommitMessage := func(signer string) *proto.Message {
+		return &proto.Message{
 			Type: proto.MessageType_COMMIT,
 			Payload: &proto.Message_CommitData{
 				CommitData: &proto.CommitMessage{
 					CommittedSeal: committedSeal,
 				},
 			},
-		From: signer,
+			From: []byte(signer),
 		}
-	invalidMessage := &proto.Message{
-		Type: proto.MessageType_PREPARE,
 	}
 
-	seals := ExtractCommittedSeals([]*proto.Message{
-		commitMessage,
-		invalidMessage,
-	})
-
-	if len(seals) != 1 {
-		t.Fatalf("Seals not extracted")
+	createWrongMessage := func(signer string, msgType proto.MessageType) *proto.Message {
+		return &proto.Message{
+			Type: msgType,
+		}
 	}
 
-	expected := &CommittedSeal{
-		Signer:    signer,
+	createCommittedSeal := func(from string) *CommittedSeal {
+		return &CommittedSeal{
+			Signer:    []byte(from),
 			Signature: committedSeal,
 		}
+	}
+
+	tests := []struct {
+		name     string
+		messages []*proto.Message
+		expected []*CommittedSeal
+		err      error
+	}{
+		{
+			name: "contains only valid COMMIT messages",
+			messages: []*proto.Message{
+				createCommitMessage("signer1"),
+				createCommitMessage("signer2"),
+			},
+			expected: []*CommittedSeal{
+				createCommittedSeal("signer1"),
+				createCommittedSeal("signer2"),
+			},
+			err: nil,
+		},
+		{
+			name: "contains wrong type messages",
+			messages: []*proto.Message{
+				createCommitMessage("signer1"),
+				createWrongMessage("signer2", proto.MessageType_PREPREPARE),
+			},
+			expected: nil,
+			err:      ErrWrongCommitMessageType,
+		},
+	}
 
-	assert.Equal(t, expected, seals[0])
+	for _, test := range tests {
+		test := test
+
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			seals, err := ExtractCommittedSeals(test.messages)
+
+			assert.Equal(t, test.expected, seals)
+			assert.Equal(t, test.err, err)
+		})
+	}
 }
 
 func TestMessages_ExtractCommitHash(t *testing.T) {