istanbul: Fail fast VerifyHeaders, Fix handlerWg

consensus/istanbul/backend/engine.go

@@ -132,8 +132,18 @@ func (sb *backend) PreprocessHeaderVerification(headers []*types.Header) (chan<-
 	abort := make(chan struct{})
 	results := make(chan error, inmemoryBlocks)
 	go func() {
+		errored := false
 		for _, header := range headers {
-			err := sb.computeSignatureAddrs(header)
+			var err error
+			if errored { // If errored once in the batch, skip the rest
+				err = consensus.ErrUnknownAncestor
+			} else {
+				err = sb.computeSignatureAddrs(header)
+			}
+
+			if err != nil {
+				errored = true
+			}
 
 			select {
 			case <-abort:
@@ -284,8 +294,18 @@ func (sb *backend) VerifyHeaders(chain consensus.ChainReader, headers []*types.H
 	abort := make(chan struct{})
 	results := make(chan error, len(headers))
 	go func() {
+		errored := false
 		for i, header := range headers {
-			err := sb.verifyHeader(chain, header, headers[:i])
+			var err error
+			if errored { // If errored once in the batch, skip the rest
+				err = consensus.ErrUnknownAncestor
+			} else {
+				err = sb.verifyHeader(chain, header, headers[:i])
+			}
+
+			if err != nil {
+				errored = true
+			}
 
 			select {
 			case <-abort:

consensus/istanbul/backend/engine_test.go

@@ -453,6 +453,7 @@ func TestVerifyHeaders(t *testing.T) {
 
 	for i := 0; i < size; i++ {
 		var b *types.Block
+		// 100 headers with 50 of them empty committed seals, 50 of them invalid committed seals.
 		if i == 0 {
 			b = makeBlockWithoutSeal(chain, engine, genesis)
 			b, _ = engine.updateBlock(b)
@@ -483,8 +484,8 @@ OUT1:
 		select {
 		case err := <-results:
 			if err != nil {
-				if err != errEmptyCommittedSeals && err != errInvalidCommittedSeals {
-					t.Errorf("error mismatch: have %v, want errEmptyCommittedSeals|errInvalidCommittedSeals", err)
+				if err != errEmptyCommittedSeals && err != errInvalidCommittedSeals && err != consensus.ErrUnknownAncestor {
+					t.Errorf("error mismatch: have %v, want errEmptyCommittedSeals|errInvalidCommittedSeals|ErrUnknownAncestor", err)
 					break OUT1
 				}
 			}
@@ -496,52 +497,43 @@ OUT1:
 			break OUT1
 		}
 	}
-	// abort cases
-	abort, results := engine.VerifyHeaders(chain, headers, nil)
+	_, results = engine.VerifyHeaders(chain, headers, nil)
 	timeout = time.NewTimer(timeoutDura)
 	index = 0
 OUT2:
 	for {
 		select {
 		case err := <-results:
 			if err != nil {
-				if err != errEmptyCommittedSeals && err != errInvalidCommittedSeals {
-					t.Errorf("error mismatch: have %v, want errEmptyCommittedSeals|errInvalidCommittedSeals", err)
+				if err != errEmptyCommittedSeals && err != errInvalidCommittedSeals && err != consensus.ErrUnknownAncestor {
+					t.Errorf("error mismatch: have %v, want errEmptyCommittedSeals|errInvalidCommittedSeals|ErrUnknownAncestor", err)
 					break OUT2
 				}
 			}
-			index++
-			if index == 5 {
-				abort <- struct{}{}
-			}
-			if index >= size {
-				t.Errorf("verifyheaders should be aborted")
-				break OUT2
-			}
 		case <-timeout.C:
 			break OUT2
 		}
 	}
 	// error header cases
 	headers[2].Number = big.NewInt(100)
-	abort, results = engine.VerifyHeaders(chain, headers, nil)
+	_, results = engine.VerifyHeaders(chain, headers, nil)
 	timeout = time.NewTimer(timeoutDura)
 	index = 0
 	errors := 0
-	expectedErrors := 2
+	expectedErrors := 0
 OUT3:
 	for {
 		select {
 		case err := <-results:
 			if err != nil {
-				if err != errEmptyCommittedSeals && err != errInvalidCommittedSeals {
+				if err != errEmptyCommittedSeals && err != errInvalidCommittedSeals && err != consensus.ErrUnknownAncestor {
 					errors++
 				}
 			}
 			index++
 			if index == size {
 				if errors != expectedErrors {
-					t.Errorf("error mismatch: have %v, want %v", err, expectedErrors)
+					t.Errorf("error mismatch: have %v, want %v", errors, expectedErrors)
 				}
 				break OUT3
 			}

consensus/istanbul/core/handler.go

@@ -33,6 +33,7 @@ func (c *core) Start() error {
 	// Tests will handle events itself, so we have to make subscribeEvents()
 	// be able to call in test.
 	c.subscribeEvents()
+	c.handlerWg.Add(1)
 	go c.handleEvents()
 
 	return nil
@@ -81,8 +82,6 @@ func (c *core) handleEvents() {
 		c.handlerWg.Done()
 	}()
 
-	c.handlerWg.Add(1)
-
 	for {
 		select {
 		case event, ok := <-c.events.Chan():

consensus/istanbul/validator/weighted.go

@@ -602,10 +602,6 @@ func (valSet *weightedCouncil) ReplaceValidators(vals []istanbul.Validator) bool
 	return true
 }
 
-func (valSet *weightedCouncil) GetValidators() []istanbul.Validator {
-	return valSet.validators
-}
-
 func (valSet *weightedCouncil) Copy() istanbul.ValidatorSet {
 	valSet.validatorMu.RLock()
 	defer valSet.validatorMu.RUnlock()
@@ -783,8 +779,8 @@ func filterValidators(isSingleMode bool, govNodeAddr common.Address, weightedVal
 
 // getStakingAmountsOfValidators calculates stakingAmounts of validators.
 // If validators have multiple staking contracts, stakingAmounts will be a sum of stakingAmounts with the same rewardAddress.
-//  - []*weightedValidator : a list of validators which type is converted to weightedValidator
-//  - []float64 : a list of stakingAmounts.
+//   - []*weightedValidator : a list of validators which type is converted to weightedValidator
+//   - []float64 : a list of stakingAmounts.
 func getStakingAmountsOfValidators(validators istanbul.Validators, stakingInfo *reward.StakingInfo) ([]*weightedValidator, []float64, error) {
 	nVals := len(validators)
 	weightedVals := make([]*weightedValidator, nVals)