valset diff bug fixes (ethereum#199)

* fixed empty headers for non final epoch headers

* fixed lint issues

* multiple changes.  fixed bug of nil iEvmH and regAdd in istanbul backend, more elegent handling of non registered addresses

common/types.go

@@ -268,6 +268,17 @@ func (a Address) Value() (driver.Value, error) {
 	return a[:], nil
 }
 
+// Converts an array of addresses to an array of their hex strings.  Used for printing out an array of addresses
+func ConvertToStringSlice(addresses []Address) []string {
+	returnList := make([]string, len(addresses))
+
+	for i, address := range addresses {
+		returnList[i] = address.Hex()
+	}
+
+	return returnList
+}
+
 // UnprefixedAddress allows marshaling an Address without 0x prefix.
 type UnprefixedAddress Address
 

consensus/consensus.go

@@ -20,6 +20,7 @@ package consensus
 import (
 	"math/big"
 
+	"github.com/ethereum/go-ethereum/accounts/abi"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/core/state"
 	"github.com/ethereum/go-ethereum/core/types"
@@ -50,6 +51,14 @@ type ChainReader interface {
 	GetBlock(hash common.Hash, number uint64) *types.Block
 }
 
+type ConsensusIEvmH interface {
+	MakeCall(scAddress common.Address, abi abi.ABI, funcName string, args []interface{}, returnObj interface{}, gas uint64, header *types.Header, state *state.StateDB) (uint64, error)
+}
+
+type ConsensusRegAdd interface {
+	GetRegisteredAddress(registryId string) *common.Address
+}
+
 // Engine is an algorithm agnostic consensus engine.
 type Engine interface {
 	// Author retrieves the Ethereum address of the account that minted the given
@@ -137,7 +146,9 @@ type Istanbul interface {
 	// Start starts the engine
 	Start(chain ChainReader, currentBlock func() *types.Block, hasBadBlock func(common.Hash) bool,
 		stateAt func(common.Hash) (*state.StateDB, error), processBlock func(*types.Block, *state.StateDB) (types.Receipts, []*types.Log, uint64, error),
-		validateState func(*types.Block, *state.StateDB, types.Receipts, uint64) error) error
+		validateState func(*types.Block, *state.StateDB, types.Receipts, uint64) error,
+		iEvmH ConsensusIEvmH,
+		regAdd ConsensusRegAdd) error
 
 	// Stop stops the engine
 	Stop() error

consensus/istanbul/backend/backend.go

@@ -100,8 +100,8 @@ type Backend struct {
 	recentMessages *lru.ARCCache // the cache of peer's messages
 	knownMessages  *lru.ARCCache // the cache of self messages
 
-	iEvmH  *core.InternalEVMHandler
-	regAdd *core.RegisteredAddresses
+	iEvmH  consensus.ConsensusIEvmH
+	regAdd consensus.ConsensusRegAdd
 }
 
 // Address implements istanbul.Backend.Address

consensus/istanbul/backend/engine.go

@@ -360,10 +360,18 @@ func (sb *Backend) Prepare(chain consensus.ChainReader, header *types.Header) er
 // 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 {
+		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")
+
+			extra, err := assembleExtra(header, []common.Address{}, []common.Address{})
+			if err != nil {
+				return err
+			}
+			header.Extra = extra
+
 		} else {
 			// Get the last epoch's validator set
 			snap, err := sb.snapshot(chain, header.Number.Uint64()-1, header.ParentHash)
@@ -388,6 +396,13 @@ func (sb *Backend) UpdateValSetDiff(chain consensus.ChainReader, header *types.H
 			}
 			header.Extra = extra
 		}
+	} else {
+		// If it's not the last block, then the validator set diff should be empty
+		extra, err := assembleExtra(header, []common.Address{}, []common.Address{})
+		if err != nil {
+			return err
+		}
+		header.Extra = extra
 	}
 
 	return nil
@@ -521,7 +536,8 @@ func (sb *Backend) APIs(chain consensus.ChainReader) []rpc.API {
 // Start implements consensus.Istanbul.Start
 func (sb *Backend) Start(chain consensus.ChainReader, currentBlock func() *types.Block, hasBadBlock func(common.Hash) bool,
 	stateAt func(common.Hash) (*state.StateDB, error), processBlock func(*types.Block, *state.StateDB) (types.Receipts, []*types.Log, uint64, error),
-	validateState func(*types.Block, *state.StateDB, types.Receipts, uint64) error) error {
+	validateState func(*types.Block, *state.StateDB, types.Receipts, uint64) error,
+	iEvmH consensus.ConsensusIEvmH, regAdd consensus.ConsensusRegAdd) error {
 	sb.coreMu.Lock()
 	defer sb.coreMu.Unlock()
 	if sb.coreStarted {
@@ -541,6 +557,8 @@ func (sb *Backend) Start(chain consensus.ChainReader, currentBlock func() *types
 	sb.stateAt = stateAt
 	sb.processBlock = processBlock
 	sb.validateState = validateState
+	sb.iEvmH = iEvmH
+	sb.regAdd = regAdd
 
 	if err := sb.core.Start(); err != nil {
 		return err
@@ -732,6 +750,9 @@ func assembleExtra(header *types.Header, oldValSet []common.Address, newValSet [
 
 	addedValidators, removedValidators := istanbul.ValidatorSetDiff(oldValSet, newValSet)
 
+	log.Trace("Setting istanbul header validator fields", "oldValSet", common.ConvertToStringSlice(oldValSet), "newValSet", common.ConvertToStringSlice(newValSet),
+		"addedValidators", common.ConvertToStringSlice(addedValidators), "removedValidators", common.ConvertToStringSlice(removedValidators))
+
 	ist := &types.IstanbulExtra{
 		AddedValidators:   addedValidators,
 		RemovedValidators: removedValidators,

consensus/istanbul/backend/engine_test.go

@@ -51,7 +51,7 @@ func newBlockChain(n int) (*core.BlockChain, *Backend) {
 	if err != nil {
 		panic(err)
 	}
-	b.Start(blockchain, blockchain.CurrentBlock, blockchain.HasBadBlock, nil, nil, nil)
+	b.Start(blockchain, blockchain.CurrentBlock, blockchain.HasBadBlock, nil, nil, nil, nil, nil)
 	snap, err := b.snapshot(blockchain, 0, common.Hash{})
 	if err != nil {
 		panic(err)

consensus/istanbul/utils.go

@@ -66,10 +66,11 @@ func CheckValidatorSignature(valSet ValidatorSet, data []byte, sig []byte) (comm
 // Retrieves the block number within an epoch.  The return value will be 1-based.
 // There is a special case if the number == 0.  It is basically the last block of the 0th epoch, and should have a value of epochSize
 func getNumberWithinEpoch(number uint64, epochSize uint64) uint64 {
+	number = number % epochSize
 	if number == 0 {
 		return epochSize
 	} else {
-		return (number % epochSize)
+		return number
 	}
 }
 

core/registeredAddresses.go

@@ -49,10 +49,10 @@ const (
 )
 
 var (
-	// TODO(kevjue) - Replace with the actual registered address for the registry smart contract
 	registrySmartContractAddress = common.HexToAddress("0x000000000000000000000000000000000000ce10")
-	registeredContractIds        = []string{params.GoldTokenRegistryId, params.AddressBasedEncryptionRegistryId, params.ReserveRegistryId, params.SortedOraclesRegistryId, params.GasCurrencyWhitelistRegistryId}
+	registeredContractIds        = []string{params.GoldTokenRegistryId, params.AddressBasedEncryptionRegistryId, params.ReserveRegistryId, params.SortedOraclesRegistryId, params.GasCurrencyWhitelistRegistryId, params.ValidatorsRegistryId}
 	getAddressForFuncABI, _      = abi.JSON(strings.NewReader(getAddressForABI))
+	zeroAddress                  = common.Address{}
 )
 
 type RegisteredAddresses struct {
@@ -74,7 +74,10 @@ func (ra *RegisteredAddresses) retrieveRegisteredAddresses() map[string]common.A
 			continue
 		} else {
 			log.Trace("RegisteredAddresses.retrieveRegisteredAddresses - Registry.getAddressFor invocation success", "contractAddress", contractAddress.Hex(), "leftoverGas", leftoverGas)
-			returnMap[contractRegistryId] = contractAddress
+
+			if contractAddress != zeroAddress {
+				returnMap[contractRegistryId] = contractAddress
+			}
 		}
 	}
 

eth/backend.go

@@ -96,6 +96,7 @@ type Ethereum struct {
 
 	gcWl   *core.GasCurrencyWhitelist
 	regAdd *core.RegisteredAddresses
+	iEvmH  *core.InternalEVMHandler
 
 	lock sync.RWMutex // Protects the variadic fields (e.g. gas price and etherbase)
 }
@@ -187,19 +188,19 @@ func New(ctx *node.ServiceContext, config *Config) (*Ethereum, error) {
 
 	// Create an internalEVMHandler handler object that geth can use to make calls to smart contracts.  Note
 	// that this should NOT be used when executing smart contract calls done via end user transactions.
-	iEvmH := core.NewInternalEVMHandler(eth.chainConfig, eth.blockchain)
+	eth.iEvmH = core.NewInternalEVMHandler(eth.chainConfig, eth.blockchain)
 
 	// Object used to retrieve and cache registered addresses from the Registry smart contract.
-	eth.regAdd = core.NewRegisteredAddresses(iEvmH)
-	iEvmH.SetRegisteredAddresses(eth.regAdd)
+	eth.regAdd = core.NewRegisteredAddresses(eth.iEvmH)
+	eth.iEvmH.SetRegisteredAddresses(eth.regAdd)
 
 	// Object used to retrieve and cache the gas currency whitelist from the GasCurrencyWhiteList smart contract
-	eth.gcWl = core.NewGasCurrencyWhitelist(eth.regAdd, iEvmH)
+	eth.gcWl = core.NewGasCurrencyWhitelist(eth.regAdd, eth.iEvmH)
 
 	// Object used to compare two different prices using any of the whitelisted gas currencies.
-	pc := core.NewPriceComparator(eth.gcWl, eth.regAdd, iEvmH)
+	pc := core.NewPriceComparator(eth.gcWl, eth.regAdd, eth.iEvmH)
 
-	eth.txPool = core.NewTxPool(config.TxPool, eth.chainConfig, eth.blockchain, pc, eth.gcWl, iEvmH)
+	eth.txPool = core.NewTxPool(config.TxPool, eth.chainConfig, eth.blockchain, pc, eth.gcWl, eth.iEvmH)
 	eth.blockchain.Processor().SetGasCurrencyWhitelist(eth.gcWl)
 	eth.blockchain.Processor().SetRegisteredAddresses(eth.regAdd)
 
@@ -510,6 +511,7 @@ func (s *Ethereum) NetVersion() uint64                               { return s.
 func (s *Ethereum) Downloader() *downloader.Downloader               { return s.protocolManager.downloader }
 func (s *Ethereum) GasCurrencyWhitelist() *core.GasCurrencyWhitelist { return s.gcWl }
 func (s *Ethereum) RegisteredAddresses() *core.RegisteredAddresses   { return s.regAdd }
+func (s *Ethereum) InternalEVMHandler() *core.InternalEVMHandler     { return s.iEvmH }
 
 // Protocols implements node.Service, returning all the currently configured
 // network protocols to start.

miner/miner.go

@@ -41,6 +41,7 @@ type Backend interface {
 	TxPool() *core.TxPool
 	GasCurrencyWhitelist() *core.GasCurrencyWhitelist
 	RegisteredAddresses() *core.RegisteredAddresses
+	InternalEVMHandler() *core.InternalEVMHandler
 }
 
 // Miner creates blocks and searches for proof-of-work values.

miner/worker.go

@@ -291,7 +291,8 @@ func (w *worker) start() {
 			},
 			func(block *types.Block, state *state.StateDB, receipts types.Receipts, usedGas uint64) error {
 				return w.chain.Validator().ValidateState(block, nil, state, receipts, usedGas)
-			})
+			},
+			w.eth.InternalEVMHandler(), w.eth.RegisteredAddresses())
 	}
 }
 
@@ -1052,10 +1053,8 @@ func (w *worker) commit(uncles []*types.Header, interval func(), update bool, st
 
 	// Set the validator set diff in the new header if we're using Istanbul and it's the last block of the epoch
 	if istanbul, ok := w.engine.(consensus.Istanbul); ok {
-		if istanbul.IsLastBlockOfEpoch(w.current.header) {
-			if err := istanbul.UpdateValSetDiff(w.chain, w.current.header, s); err != nil {
-				return err
-			}
+		if err := istanbul.UpdateValSetDiff(w.chain, w.current.header, s); err != nil {
+			return err
 		}
 	}
 

miner/worker_test.go

@@ -159,6 +159,7 @@ func (b *testWorkerBackend) PostChainEvents(events []interface{}) {
 }
 func (b *testWorkerBackend) GasCurrencyWhitelist() *core.GasCurrencyWhitelist { return nil }
 func (b *testWorkerBackend) RegisteredAddresses() *core.RegisteredAddresses   { return nil }
+func (b *testWorkerBackend) InternalEVMHandler() *core.InternalEVMHandler     { return nil }
 
 func newTestWorker(t *testing.T, chainConfig *params.ChainConfig, engine consensus.Engine, blocks int, shouldAddPendingTxs bool) (*worker, *testWorkerBackend) {
 	backend := newTestWorkerBackend(t, chainConfig, engine, blocks)