Resolved vote related comments of fast finality review

core/types/vote.go

@@ -3,6 +3,9 @@ package types
 import (
 	"sync/atomic"
 
+	"github.com/pkg/errors"
+	"github.com/prysmaticlabs/prysm/crypto/bls"
+
 	"github.com/ethereum/go-ethereum/common"
 )
 
@@ -61,3 +64,22 @@ func (v *VoteEnvelope) calcVoteHash() common.Hash {
 }
 
 func (b BLSPublicKey) Bytes() []byte { return b[:] }
+
+// Verify vote using BLS.
+func (vote *VoteEnvelope) Verify() error {
+	blsPubKey, err := bls.PublicKeyFromBytes(vote.VoteAddress[:])
+	if err != nil {
+		return errors.Wrap(err, "convert public key from bytes to bls failed")
+	}
+
+	sig, err := bls.SignatureFromBytes(vote.Signature[:])
+	if err != nil {
+		return errors.Wrap(err, "invalid signature")
+	}
+
+	voteDataHash := vote.Data.Hash()
+	if !sig.Verify(blsPubKey, voteDataHash[:]) {
+		return errors.New("verify bls signature failed.")
+	}
+	return nil
+}

core/vote/vote_manager.go

@@ -1,12 +1,7 @@
 package vote
 
 import (
-	"context"
 	"fmt"
-	"io/ioutil"
-
-	"github.com/prysmaticlabs/prysm/validator/accounts/iface"
-	"github.com/prysmaticlabs/prysm/validator/accounts/wallet"
 
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/consensus"
@@ -20,7 +15,7 @@ import (
 )
 
 type (
-	getHighestJustifiedHeader func(chain consensus.ChainHeaderReader, header *types.Header) *types.Header
+	getHighestJustifiedHeaderFunc func(chain consensus.ChainHeaderReader, header *types.Header) *types.Header
 )
 
 // VoteManager will handle the vote produced by self.
@@ -39,10 +34,10 @@ type VoteManager struct {
 
 	engine consensus.PoSA
 
-	get getHighestJustifiedHeader
+	getHighestJustifiedHeader getHighestJustifiedHeaderFunc
 }
 
-func NewVoteManager(mux *event.TypeMux, chainconfig *params.ChainConfig, chain *core.BlockChain, pool *VotePool, journalPath, blsPasswordPath, blsWalletPath string, engine consensus.PoSA, get getHighestJustifiedHeader) (*VoteManager, error) {
+func NewVoteManager(mux *event.TypeMux, chainconfig *params.ChainConfig, chain *core.BlockChain, pool *VotePool, journalPath, blsPasswordPath, blsWalletPath string, engine consensus.PoSA, getHighestJustifiedHeader getHighestJustifiedHeaderFunc) (*VoteManager, error) {
 	voteManager := &VoteManager{
 		mux: mux,
 
@@ -53,45 +48,11 @@ func NewVoteManager(mux *event.TypeMux, chainconfig *params.ChainConfig, chain *
 		pool:   pool,
 		engine: engine,
 
-		get: get,
-	}
-
-	dirExists, err := wallet.Exists(blsWalletPath)
-	if err != nil {
-		log.Error("Check BLS wallet exists error: %v.", err)
-		return nil, err
-	}
-	if !dirExists {
-		log.Error("BLS wallet did not exists.")
-		return nil, fmt.Errorf("BLS wallet did not exists.")
-	}
-
-	walletPassword, err := ioutil.ReadFile(blsPasswordPath)
-	if err != nil {
-		log.Error("Read BLS wallet password error: %v.", err)
-		return nil, err
-	}
-	log.Info("Read BLS wallet password successfully")
-
-	w, err := wallet.OpenWallet(context.Background(), &wallet.Config{
-		WalletDir:      blsWalletPath,
-		WalletPassword: string(walletPassword),
-	})
-	if err != nil {
-		log.Error("Open BLS wallet failed: %v.", err)
-		return nil, err
-	}
-	log.Info("Open BLS wallet successfully")
-
-	km, err := w.InitializeKeymanager(context.Background(), iface.InitKeymanagerConfig{ListenForChanges: false})
-	if err != nil {
-		log.Error("Initialize key manager failed: %v.", err)
-		return nil, err
+		getHighestJustifiedHeader: getHighestJustifiedHeader,
 	}
-	log.Info("Initialized keymanager successfully")
 
-	// Create voteSigner
-	voteSigner, err := NewVoteSigner(&km)
+	// Create voteSigner.
+	voteSigner, err := NewVoteSigner(blsPasswordPath, blsWalletPath)
 	if err != nil {
 		return nil, err
 	}
@@ -194,7 +155,7 @@ func (voteManager *VoteManager) loop() {
 // A validator must not vote within the span of its other votes . (Rule 2)
 // Validators always vote for their canonical chain’s latest block. (Rule 3)
 func (voteManager *VoteManager) UnderRules(header *types.Header) (bool, uint64, common.Hash) {
-	justifiedHeader := voteManager.get(voteManager.chain, header)
+	justifiedHeader := voteManager.getHighestJustifiedHeader(voteManager.chain, header)
 	if justifiedHeader == nil {
 		log.Error("highestJustifiedHeader is nil")
 		return false, 0, common.Hash{}
@@ -219,21 +180,16 @@ func (voteManager *VoteManager) UnderRules(header *types.Header) (bool, uint64,
 		return false, 0, common.Hash{}
 	}
 
-	journalLatestVote, err := journal.ReadVote(lastIndex)
-	if err != nil {
-		return false, 0, common.Hash{}
-	}
-	if journalLatestVote == nil {
-		// Indicate there's no vote before in local node, so it must be under rules.
-		return true, sourceNumber, sourceHash
-	}
-
 	for index := lastIndex; index >= firstIndex; index-- {
 		vote, err := journal.ReadVote(index)
 		if err != nil {
 			return false, 0, common.Hash{}
 		}
 		if vote == nil {
+			// Indicate there's no vote in local journal, so it must be under rules.
+			if index == 0 {
+				return true, sourceNumber, sourceHash
+			}
 			log.Error("vote is nil")
 			return false, 0, common.Hash{}
 		}

core/vote/vote_pool.go

@@ -139,17 +139,6 @@ func (pool *VotePool) putIntoVotePool(vote *types.VoteEnvelope) bool {
 		votesPq = pool.futureVotesPq
 		isFutureVote = true
 	} else {
-		// Verify if the vote comes from valid validators based on voteAddress (BLSPublicKey).
-		if posa, ok := pool.engine.(consensus.PoSA); ok {
-			if !posa.VerifyVote(pool.chain, vote) {
-				return false
-			}
-		}
-		// Verify bls signature.
-		if err := VerifyVoteWithBLS(vote); err != nil {
-			log.Error("Failed to verify voteMessage", "err", err)
-			return false
-		}
 		votes = pool.curVotes
 		votesPq = pool.curVotesPq
 	}
@@ -159,14 +148,20 @@ func (pool *VotePool) putIntoVotePool(vote *types.VoteEnvelope) bool {
 		return false
 	}
 
-	pool.putVote(votes, votesPq, vote, voteData, voteHash, isFutureVote)
-
 	if !isFutureVote {
+		// Verify if the vote comes from valid validators based on voteAddress (BLSPublicKey), only verify curVotes here, will verify futureVotes in transfer process.
+		if posa, ok := pool.engine.(consensus.PoSA); ok {
+			if !posa.VerifyVote(pool.chain, vote) {
+				return false
+			}
+		}
 		// Send vote for handler usage of broadcasting to peers.
 		voteEv := core.NewVoteEvent{Vote: vote}
 		pool.votesFeed.Send(voteEv)
 	}
 
+	pool.putVote(votes, votesPq, vote, voteData, voteHash, isFutureVote)
+
 	return true
 }
 
@@ -259,13 +254,11 @@ func (pool *VotePool) transfer(blockHash common.Hash) {
 				continue
 			}
 		}
-		// Verify the vote from futureVotes.
-		if err := VerifyVoteWithBLS(vote); err == nil {
-			// In the process of transfer, send valid vote to votes channel for handler usage
-			voteEv := core.NewVoteEvent{Vote: vote}
-			pool.votesFeed.Send(voteEv)
-			validVotes = append(validVotes, vote)
-		}
+
+		// In the process of transfer, send valid vote to votes channel for handler usage
+		voteEv := core.NewVoteEvent{Vote: vote}
+		pool.votesFeed.Send(voteEv)
+		validVotes = append(validVotes, vote)
 	}
 
 	voteData := heap.Pop(futurePq)
@@ -349,15 +342,25 @@ func (pool *VotePool) basicVerify(vote *types.VoteEnvelope, headNumber uint64, m
 		log.Warn("BlockNumber of vote is outside the range of header-256~header+13")
 		return false
 	}
-	// To prevent DOS attacks, make sure no more than 50 votes for the same blockHash if it's futureVotes
-	// No more than 21 votes per blockHash if not futureVotes and no more than 50 votes per blockHash if futureVotes
+
+	// To prevent DOS attacks, make sure no more than 21 votes per blockHash if not futureVotes
+	// and no more than 50 votes per blockHash if futureVotes.
 	maxVoteAmountPerBlock := maxCurVoteAmountPerBlock
 	if isFutureVote {
 		maxVoteAmountPerBlock = maxFutureVoteAmountPerBlock
 	}
 	if voteBox, ok := m[targetHash]; ok {
-		return len(voteBox.voteMessages) <= maxVoteAmountPerBlock
+		if len(voteBox.voteMessages) > maxVoteAmountPerBlock {
+			return false
+		}
 	}
+
+	// Verify bls signature.
+	if err := vote.Verify(); err != nil {
+		log.Error("Failed to verify voteMessage", "err", err)
+		return false
+	}
+
 	return true
 }
 

core/vote/vote_pool_test.go

@@ -170,7 +170,7 @@ func testVotePool(t *testing.T, inValidRules bool) {
 	file.Close()
 	os.Remove(journal)
 
-	var ruleFunc getHighestJustifiedHeader
+	var ruleFunc getHighestJustifiedHeaderFunc
 	if inValidRules {
 		ruleFunc = getHighestJustifiedHeaderForValid
 	} else {

core/vote/vote_signer.go

@@ -3,16 +3,20 @@ package vote
 import (
 	"context"
 	"fmt"
+	"io/ioutil"
 	"time"
 
 	"github.com/pkg/errors"
 
 	"github.com/prysmaticlabs/prysm/crypto/bls"
 	validatorpb "github.com/prysmaticlabs/prysm/proto/prysm/v1alpha1/validator-client"
+	"github.com/prysmaticlabs/prysm/validator/accounts/iface"
+	"github.com/prysmaticlabs/prysm/validator/accounts/wallet"
 	"github.com/prysmaticlabs/prysm/validator/keymanager"
 
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/core/types"
+	"github.com/ethereum/go-ethereum/log"
 	"github.com/ethereum/go-ethereum/metrics"
 )
 
@@ -25,16 +29,51 @@ type VoteSigner struct {
 	pubKey [48]byte
 }
 
-func NewVoteSigner(km *keymanager.IKeymanager) (*VoteSigner, error) {
+func NewVoteSigner(blsPasswordPath, blsWalletPath string) (*VoteSigner, error) {
+	dirExists, err := wallet.Exists(blsWalletPath)
+	if err != nil {
+		log.Error("Check BLS wallet exists error: %v.", err)
+		return nil, err
+	}
+	if !dirExists {
+		log.Error("BLS wallet did not exists.")
+		return nil, fmt.Errorf("BLS wallet did not exists.")
+	}
+
+	walletPassword, err := ioutil.ReadFile(blsPasswordPath)
+	if err != nil {
+		log.Error("Read BLS wallet password error: %v.", err)
+		return nil, err
+	}
+	log.Info("Read BLS wallet password successfully")
+
+	w, err := wallet.OpenWallet(context.Background(), &wallet.Config{
+		WalletDir:      blsWalletPath,
+		WalletPassword: string(walletPassword),
+	})
+	if err != nil {
+		log.Error("Open BLS wallet failed: %v.", err)
+		return nil, err
+	}
+	log.Info("Open BLS wallet successfully")
+
+	km, err := w.InitializeKeymanager(context.Background(), iface.InitKeymanagerConfig{ListenForChanges: false})
+	if err != nil {
+		log.Error("Initialize key manager failed: %v.", err)
+		return nil, err
+	}
+	log.Info("Initialized keymanager successfully")
+
 	ctx, cancel := context.WithTimeout(context.Background(), voteSignerTimeout)
 	defer cancel()
 
-	pubKeys, err := (*km).FetchValidatingPublicKeys(ctx)
+	pubKeys, err := km.FetchValidatingPublicKeys(ctx)
 	if err != nil {
 		return nil, errors.Wrap(err, "could not fetch validating public keys")
 	}
+
 	return &VoteSigner{
-		km:     km,
+		km:     &km,
 		pubKey: pubKeys[0],
 	}, nil
 }
@@ -65,25 +104,6 @@ func (signer *VoteSigner) SignVote(vote *types.VoteEnvelope) error {
 	return nil
 }
 
-// VerifyVoteWithBLS using BLS.
-func VerifyVoteWithBLS(vote *types.VoteEnvelope) error {
-	blsPubKey, err := bls.PublicKeyFromBytes(vote.VoteAddress[:])
-	if err != nil {
-		return errors.Wrap(err, "convert public key from bytes to bls failed")
-	}
-
-	sig, err := bls.SignatureFromBytes(vote.Signature[:])
-	if err != nil {
-		return errors.Wrap(err, "invalid signature")
-	}
-
-	voteDataHash := vote.Data.Hash()
-	if !sig.Verify(blsPubKey, voteDataHash[:]) {
-		return errors.New("verify bls signature failed.")
-	}
-	return nil
-}
-
 // Metrics to indicate if there's any failed signing.
 func votesSigningErrorMetric(blockNumber uint64, blockHash common.Hash) metrics.Gauge {
 	return metrics.GetOrRegisterGauge(fmt.Sprintf("voteSigning/blockNumber/%d/blockHash/%s", blockNumber, blockHash), nil)