core: Fix stuffs (#422)

* core: reduce syncing ba msg

* core: fix checking period of agreement result

6 files changed, 107 insertions, 37 deletions

diff --git a/core/agreement-mgr.go b/core/agreement-mgr.go
index d3cf533..9e86369 100644
--- a/core/agreement-mgr.go
+++ b/core/agreement-mgr.go
@@ -258,7 +258,7 @@ func (mgr *agreementMgr) processAgreementResult(
 	if isStop(aID) {
 		return nil
 	}
-	if result.Position == aID {
+	if result.Position == aID && !agreement.confirmed() {
 		mgr.logger.Info("Syncing BA", "position", &result.Position)
 		for key := range result.Votes {
 			if err := agreement.processVote(&result.Votes[key]); err != nil {
diff --git a/core/agreement.go b/core/agreement.go
index c17c59f..c1f514d 100644
--- a/core/agreement.go
+++ b/core/agreement.go
@@ -466,6 +466,12 @@ func (a *agreement) done() <-chan struct{} {
 	return ch
 }
 
+func (a *agreement) confirmed() bool {
+	a.lock.RLock()
+	defer a.lock.RUnlock()
+	return a.hasOutput
+}
+
 // processBlock is the entry point for processing Block.
 func (a *agreement) processBlock(block *types.Block) error {
 	a.lock.Lock()
diff --git a/core/consensus.go b/core/consensus.go
index b84947a..2cef6bc 100644
--- a/core/consensus.go
+++ b/core/consensus.go
@@ -49,14 +49,6 @@ var (
 		"incorrect agreement result position")
 	ErrNotEnoughVotes = fmt.Errorf(
 		"not enought votes")
-	ErrIncorrectVoteBlockHash = fmt.Errorf(
-		"incorrect vote block hash")
-	ErrIncorrectVoteType = fmt.Errorf(
-		"incorrect vote type")
-	ErrIncorrectVotePosition = fmt.Errorf(
-		"incorrect vote position")
-	ErrIncorrectVoteProposer = fmt.Errorf(
-		"incorrect vote proposer")
 	ErrCRSNotReady = fmt.Errorf(
 		"CRS not ready")
 	ErrConfigurationNotReady = fmt.Errorf(
diff --git a/core/consensus_test.go b/core/consensus_test.go
index b53af8a..d340c4e 100644
--- a/core/consensus_test.go
+++ b/core/consensus_test.go
@@ -623,34 +623,7 @@ func (s *ConsensusTestSuite) TestSyncBA() {
 	aID := con.baMgr.baModules[0].agreementID()
 	s.Equal(pos, aID)
 
-	// Test negative case.
-	baResult.BlockHash = common.NewRandomHash()
-	s.Equal(ErrIncorrectVoteBlockHash, con.ProcessAgreementResult(baResult))
-	baResult.BlockHash = hash
-
-	baResult.Position.Height++
-	s.Equal(ErrIncorrectVotePosition, con.ProcessAgreementResult(baResult))
-	baResult.Position = pos
-
-	baResult.Votes[0].Type = types.VotePreCom
-	s.Equal(ErrIncorrectVoteType, con.ProcessAgreementResult(baResult))
-	baResult.Votes[0].Type = types.VoteCom
-
-	baResult.Votes[0].ProposerID = types.NodeID{Hash: common.NewRandomHash()}
-	s.Equal(ErrIncorrectVoteProposer, con.ProcessAgreementResult(baResult))
-	baResult.Votes[0].ProposerID = types.NewNodeID(pubKeys[0])
-
-	baResult.Votes[0].Signature, err = prvKeys[0].Sign(common.NewRandomHash())
-	s.Require().NoError(err)
-	s.Equal(ErrIncorrectVoteSignature, con.ProcessAgreementResult(baResult))
-	s.Require().NoError(signers[0].SignVote(&baResult.Votes[0]))
-
-	baResult.Votes = baResult.Votes[:1]
-	s.Equal(ErrNotEnoughVotes, con.ProcessAgreementResult(baResult))
-	for range signers {
-		baResult.Votes = append(baResult.Votes, baResult.Votes[0])
-	}
-	s.Equal(ErrNotEnoughVotes, con.ProcessAgreementResult(baResult))
+	// Negative cases are moved to TestVerifyAgreementResult in utils_test.go.
 }
 
 func TestConsensus(t *testing.T) {
diff --git a/core/utils.go b/core/utils.go
index 3b1069e..8579e6e 100644
--- a/core/utils.go
+++ b/core/utils.go
@@ -32,6 +32,20 @@ import (
 	"github.com/dexon-foundation/dexon-consensus/core/utils"
 )
 
+// Errors for utils.
+var (
+	ErrIncorrectVoteBlockHash = fmt.Errorf(
+		"incorrect vote block hash")
+	ErrIncorrectVoteType = fmt.Errorf(
+		"incorrect vote type")
+	ErrIncorrectVotePosition = fmt.Errorf(
+		"incorrect vote position")
+	ErrIncorrectVoteProposer = fmt.Errorf(
+		"incorrect vote proposer")
+	ErrIncorrectVotePeriod = fmt.Errorf(
+		"incorrect vote period")
+)
+
 // NodeSetCache is type alias to avoid fullnode compile error when moving
 // it to core/utils package.
 type NodeSetCache = utils.NodeSetCache
@@ -161,10 +175,14 @@ func VerifyAgreementResult(
 	}
 	voted := make(map[types.NodeID]struct{}, len(notarySet))
 	voteType := res.Votes[0].Type
+	votePeriod := res.Votes[0].Period
 	if voteType != types.VoteFast && voteType != types.VoteCom {
 		return ErrIncorrectVoteType
 	}
 	for _, vote := range res.Votes {
+		if vote.Period != votePeriod {
+			return ErrIncorrectVotePeriod
+		}
 		if res.IsEmptyBlock {
 			if (vote.BlockHash != common.Hash{}) {
 				return ErrIncorrectVoteBlockHash
diff --git a/core/utils_test.go b/core/utils_test.go
index dd15607..5d61a48 100644
--- a/core/utils_test.go
+++ b/core/utils_test.go
@@ -19,8 +19,14 @@ package core
 
 import (
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/suite"
+
+	"github.com/dexon-foundation/dexon-consensus/common"
+	"github.com/dexon-foundation/dexon-consensus/core/test"
+	"github.com/dexon-foundation/dexon-consensus/core/types"
+	"github.com/dexon-foundation/dexon-consensus/core/utils"
 )
 
 type UtilsTestSuite struct {
@@ -39,6 +45,81 @@ func (s *UtilsTestSuite) TestRemoveFromSortedUint32Slice() {
 	s.Equal([]uint32{}, removeFromSortedUint32Slice([]uint32{}, 1))
 }
 
+func (s *UtilsTestSuite) TestVerifyAgreementResult() {
+	prvKeys, pubKeys, err := test.NewKeys(4)
+	s.Require().NoError(err)
+	gov, err := test.NewGovernance(test.NewState(
+		pubKeys, time.Second, &common.NullLogger{}, true), ConfigRoundShift)
+	s.Require().NoError(err)
+	cache := utils.NewNodeSetCache(gov)
+	hash := common.NewRandomHash()
+	signers := make([]*utils.Signer, 0, len(prvKeys))
+	for _, prvKey := range prvKeys {
+		signers = append(signers, utils.NewSigner(prvKey))
+	}
+	pos := types.Position{
+		Round:   0,
+		ChainID: 0,
+		Height:  20,
+	}
+	baResult := &types.AgreementResult{
+		BlockHash: hash,
+		Position:  pos,
+	}
+	for _, signer := range signers {
+		vote := types.NewVote(types.VoteCom, hash, 0)
+		vote.Position = pos
+		s.Require().NoError(signer.SignVote(vote))
+		baResult.Votes = append(baResult.Votes, *vote)
+	}
+	s.Require().NoError(VerifyAgreementResult(baResult, cache))
+
+	// Test negative case.
+	// All period should be the same.
+	baResult.Votes[1].Period++
+	s.Equal(ErrIncorrectVotePeriod, VerifyAgreementResult(baResult, cache))
+	baResult.Votes[1].Period--
+
+	// Blockhash should match the one in votes.
+	baResult.BlockHash = common.NewRandomHash()
+	s.Equal(ErrIncorrectVoteBlockHash, VerifyAgreementResult(baResult, cache))
+	baResult.BlockHash = hash
+
+	// Position should match.
+	baResult.Position.Height++
+	s.Equal(ErrIncorrectVotePosition, VerifyAgreementResult(baResult, cache))
+	baResult.Position = pos
+
+	// types.VotePreCom is not accepted in agreement result.
+	baResult.Votes[0].Type = types.VotePreCom
+	s.Equal(ErrIncorrectVoteType, VerifyAgreementResult(baResult, cache))
+	baResult.Votes[0].Type = types.VoteCom
+
+	// Vote type should be the same.
+	baResult.Votes[1].Type = types.VoteFast
+	s.Equal(ErrIncorrectVoteType, VerifyAgreementResult(baResult, cache))
+	baResult.Votes[1].Type = types.VoteCom
+
+	// Only vote proposed by notarySet is valid.
+	baResult.Votes[0].ProposerID = types.NodeID{Hash: common.NewRandomHash()}
+	s.Equal(ErrIncorrectVoteProposer, VerifyAgreementResult(baResult, cache))
+	baResult.Votes[0].ProposerID = types.NewNodeID(pubKeys[0])
+
+	// Vote shuold have valid signature.
+	baResult.Votes[0].Signature, err = prvKeys[0].Sign(common.NewRandomHash())
+	s.Require().NoError(err)
+	s.Equal(ErrIncorrectVoteSignature, VerifyAgreementResult(baResult, cache))
+	s.Require().NoError(signers[0].SignVote(&baResult.Votes[0]))
+
+	// Unique votes shuold be more than threshold.
+	baResult.Votes = baResult.Votes[:1]
+	s.Equal(ErrNotEnoughVotes, VerifyAgreementResult(baResult, cache))
+	for range signers {
+		baResult.Votes = append(baResult.Votes, baResult.Votes[0])
+	}
+	s.Equal(ErrNotEnoughVotes, VerifyAgreementResult(baResult, cache))
+}
+
 func TestUtils(t *testing.T) {
 	suite.Run(t, new(UtilsTestSuite))
 }