Assume no validator set diff when getValidators fails in Istanbul Ver…

…ify (ethereum#268)

build/ci.go

@@ -349,7 +349,6 @@ func doTest(cmdline []string) {
 	// Test a single package at a time. CI builders are slow
 	// and some tests run into timeouts under load.
 	gotest := goTool("test", buildFlags(env)...)
-	// failfast -> fail ont the first failure, don't run all the tests.
 	gotest.Args = append(gotest.Args, "-p", "1", "-timeout", "5m")
 	if *coverage {
 		gotest.Args = append(gotest.Args, "-covermode=atomic", "-cover")

consensus/istanbul/backend/backend.go

@@ -35,7 +35,6 @@ import (
 	"github.com/ethereum/go-ethereum/ethdb"
 	"github.com/ethereum/go-ethereum/event"
 	"github.com/ethereum/go-ethereum/log"
-	"github.com/ethereum/go-ethereum/params"
 	lru "github.com/hashicorp/golang-lru"
 )
 
@@ -307,15 +306,14 @@ func (sb *Backend) verifyValSetDiff(proposal istanbul.Proposal, block *types.Blo
 		return err
 	}
 
-	validatorElectionAddress := sb.regAdd.GetRegisteredAddress(params.ValidatorsRegistryId)
-
-	if validatorElectionAddress != nil {
-		var newValSet []common.Address
-		if _, err := sb.iEvmH.MakeStaticCall(*validatorElectionAddress, getValidatorsFuncABI, "getValidators", []interface{}{}, &newValSet, 20000, header, state); err != nil {
-			log.Error("verifyValSetDiff - Error in getting the validator set from the validators smart contract")
-			return err
+	newValSet, err := sb.getValSet(block.Header(), state)
+	if err != nil {
+		log.Error("Istanbul.verifyValSetDiff - Error in retrieving the validator set. Verifying val set diff empty.", "err", err)
+		if len(istExtra.AddedValidators) != 0 || len(istExtra.RemovedValidators) != 0 {
+			log.Warn("verifyValSetDiff - Invalid val set diff.  Non empty diff when it should be empty.", "addedValidators", common.ConvertToStringSlice(istExtra.AddedValidators), "removedValidators", common.ConvertToStringSlice(istExtra.RemovedValidators))
+			return errInvalidValidatorSetDiff
 		}
-
+	} else {
 		parentValidators := sb.ParentValidators(proposal)
 		oldValSet := make([]common.Address, 0, parentValidators.Size())
 
@@ -328,13 +326,6 @@ func (sb *Backend) verifyValSetDiff(proposal istanbul.Proposal, block *types.Blo
 		if !istanbul.CompareValidatorSlices(addedValidators, istExtra.AddedValidators) || !istanbul.CompareValidatorSlices(removedValidators, istExtra.RemovedValidators) {
 			return errInvalidValidatorSetDiff
 		}
-	} else {
-		// The validator election smart contract is not registered yet, so the validator set diff should be empty
-
-		if len(istExtra.AddedValidators) != 0 || len(istExtra.RemovedValidators) != 0 {
-			log.Warn("verifyValSetDiff - Invalid val set diff.  Non empty diff when it should be empty.", "addedValidators", istExtra.AddedValidators, "removedValidators", istExtra.RemovedValidators)
-			return errInvalidValidatorSetDiff
-		}
 	}
 
 	return nil

consensus/istanbul/backend/engine.go

@@ -81,6 +81,8 @@ var (
 	errEmptyCommittedSeals = errors.New("zero committed seals")
 	// errMismatchTxhashes is returned if the TxHash in header is mismatch.
 	errMismatchTxhashes = errors.New("mismatch transactions hashes")
+	// errValidatorsContractNotRegistered is returned if there is no registered "Validators" address.
+	errValidatorsContractNotRegistered = errors.New("no registered `Validators` address")
 	// errInvalidValidatorSetDiff is returned if the header contains invalid validator set diff
 	errInvalidValidatorSetDiff = errors.New("invalid validator set diff")
 
@@ -381,38 +383,42 @@ func (sb *Backend) Prepare(chain consensus.ChainReader, header *types.Header) er
 	return nil
 }
 
+func (sb *Backend) getValSet(header *types.Header, state *state.StateDB) ([]common.Address, error) {
+	var newValSet []common.Address
+	validatorAddress := sb.regAdd.GetRegisteredAddress(params.ValidatorsRegistryId)
+	if validatorAddress == nil {
+		return newValSet, errValidatorsContractNotRegistered
+	} else {
+		// Get the new epoch's validator set
+		maxGasForGetValidators := uint64(1000000)
+		// TODO(asa) - Once the validator election smart contract is completed, then a more accurate gas value should be used.
+		_, err := sb.iEvmH.MakeStaticCall(*validatorAddress, getValidatorsFuncABI, "getValidators", []interface{}{}, &newValSet, maxGasForGetValidators, header, state)
+		return newValSet, err
+	}
+}
+
 // UpdateValSetDiff will update the validator set diff in the header, if the mined header is the last block of the epoch
 func (sb *Backend) UpdateValSetDiff(chain consensus.ChainReader, header *types.Header, state *state.StateDB) error {
 	// If this is the last block of the epoch, then get the validator set diff, to save into the header
 	log.Trace("Called UpdateValSetDiff", "number", header.Number.Uint64(), "epoch", sb.config.Epoch)
 	if istanbul.IsLastBlockOfEpoch(header.Number.Uint64(), sb.config.Epoch) {
-		validatorAddress := sb.regAdd.GetRegisteredAddress(params.ValidatorsRegistryId)
-		if validatorAddress == nil {
-			log.Warn("Finalizing last block of an epoch, and the validator smart contract is not deployed.  Using the previous epoch's validator set")
+		newValSet, err := sb.getValSet(header, state)
+		if err != nil {
+			log.Error("Istanbul.Finalize - Error in retrieving the validator set. Using the previous epoch's validator set", "err", err)
 		} else {
 			// Get the last epoch's validator set
 			snap, err := sb.snapshot(chain, header.Number.Uint64()-1, header.ParentHash, nil)
 			if err != nil {
 				return err
 			}
 
-			// Get the new epoch's validator set
-			var newValSet []common.Address
-
-			maxGasForGetValidators := uint64(1000000)
-			// TODO(kevjue) - Once the validator election smart contract is completed, then a more accurate gas value should be used.
-			leftoverGas, err := sb.iEvmH.MakeStaticCall(*validatorAddress, getValidatorsFuncABI, "getValidators", []interface{}{}, &newValSet, maxGasForGetValidators, header, state)
+			// add validators in snapshot to extraData's validators section
+			extra, err := assembleExtra(header, snap.validators(), newValSet)
 			if err != nil {
-				log.Error("Istanbul.Finalize - Error in retrieving the validator set. Using the previous epoch's validator set", "leftoverGas", leftoverGas, "err", err)
-			} else {
-				// add validators in snapshot to extraData's validators section
-				extra, err := assembleExtra(header, snap.validators(), newValSet)
-				if err != nil {
-					return err
-				}
-				header.Extra = extra
-				return nil
+				return err
 			}
+			header.Extra = extra
+			return nil
 		}
 	}
 	// If it's not the last block or we were unable to pull the new validator set, then the validator set diff should be empty