Fix issues with changing blockperiod down and up (#1505)

* Fix issues with changing blockperiod down and up

cmd/geth/config_test.go

@@ -394,7 +394,7 @@ func TestFlagsConfig(t *testing.T) {
 	quorumIstanbul := eth.Istanbul
 	config := quorumIstanbul.GetConfig(nil)
 	assert.Equal(t, uint64(10000), config.RequestTimeout)
-	assert.Equal(t, uint64(1), config.BlockPeriod)
+	assert.Equal(t, uint64(5), config.BlockPeriod)
 	assert.Equal(t, uint64(30000), config.Epoch)
 	assert.Equal(t, big.NewInt(0), quorumIstanbul.Ceil2Nby3Block)
 	assert.Equal(t, istanbul.RoundRobin, quorumIstanbul.ProposerPolicy.Id) // conflict with genesis?

cmd/utils/flags.go

@@ -2605,11 +2605,8 @@ func setBFTConfig(bftConfig *params.BFTConfig) *istanbul.Config {
 	if bftConfig.BlockPeriodSeconds != 0 {
 		istanbulConfig.BlockPeriod = bftConfig.BlockPeriodSeconds
 	}
-	if bftConfig.EmptyBlockPeriodSeconds != 0 {
-		istanbulConfig.EmptyBlockPeriod = bftConfig.EmptyBlockPeriodSeconds
-	}
-	if istanbulConfig.EmptyBlockPeriod < istanbulConfig.BlockPeriod {
-		istanbulConfig.EmptyBlockPeriod = istanbulConfig.BlockPeriod
+	if bftConfig.EmptyBlockPeriodSeconds != nil {
+		istanbulConfig.EmptyBlockPeriod = *bftConfig.EmptyBlockPeriodSeconds
 	}
 	if bftConfig.RequestTimeoutSeconds != 0 {
 		istanbulConfig.RequestTimeout = bftConfig.RequestTimeoutSeconds * 1000

cmd/utils/flags_test.go

@@ -204,6 +204,6 @@ func TestQuorumConfigFlags(t *testing.T) {
 	config := arbitraryEthConfig.Istanbul.GetConfig(nil)
 	assert.Equal(t, uint64(23), config.RequestTimeout, "IstanbulRequestTimeoutFlag value is incorrect")
 	assert.Equal(t, uint64(34), config.BlockPeriod, "IstanbulBlockPeriodFlag value is incorrect")
-	assert.Equal(t, uint64(34), config.EmptyBlockPeriod, "IstanbulEmptyBlockPeriodFlag value is incorrect")
+	assert.Equal(t, uint64(0), config.EmptyBlockPeriod, "IstanbulEmptyBlockPeriodFlag value is incorrect")
 	assert.Equal(t, true, arbitraryEthConfig.RaftMode, "RaftModeFlag value is incorrect")
 }

consensus/istanbul/config.go

@@ -137,8 +137,8 @@ type Config struct {
 
 var DefaultConfig = &Config{
 	RequestTimeout:         10000,
-	BlockPeriod:            1,
-	EmptyBlockPeriod:       10,
+	BlockPeriod:            5,
+	EmptyBlockPeriod:       0,
 	ProposerPolicy:         NewRoundRobinProposerPolicy(),
 	Epoch:                  30000,
 	Ceil2Nby3Block:         big.NewInt(0),
@@ -193,13 +193,11 @@ func (c Config) GetConfig(blockNumber *big.Int) Config {
 		if transition.BlockPeriodSeconds != 0 {
 			newConfig.BlockPeriod = transition.BlockPeriodSeconds
 		}
-		if transition.EmptyBlockPeriodSeconds != 0 {
-			newConfig.EmptyBlockPeriod = transition.EmptyBlockPeriodSeconds
+		if transition.EmptyBlockPeriodSeconds != nil {
+			newConfig.EmptyBlockPeriod = *transition.EmptyBlockPeriodSeconds
 		}
 	})
-	if newConfig.EmptyBlockPeriod < newConfig.BlockPeriod {
-		newConfig.EmptyBlockPeriod = newConfig.BlockPeriod
-	}
+
 	return newConfig
 }
 

consensus/istanbul/config_test.go

@@ -62,7 +62,7 @@ func TestGetConfig(t *testing.T) {
 	}, {
 		Block:                   big.NewInt(5),
 		RequestTimeoutSeconds:   15000,
-		EmptyBlockPeriodSeconds: 1,
+		EmptyBlockPeriodSeconds: nil,
 	}}
 	config1 := *DefaultConfig
 	config1.Epoch = 40000

consensus/istanbul/qbft/core/request.go

@@ -21,6 +21,7 @@ import (
 
 	"github.com/ethereum/go-ethereum/consensus/istanbul"
 	"github.com/ethereum/go-ethereum/core/types"
+	"github.com/ethereum/go-ethereum/log"
 )
 
 // handleRequest is called by proposer in reaction to `miner.Seal()`
@@ -58,7 +59,10 @@ func (c *core) handleRequest(request *Request) error {
 		block, ok := request.Proposal.(*types.Block)
 		if ok && len(block.Transactions()) == 0 { // if empty block
 			config := c.config.GetConfig(c.current.Sequence())
-			delay = time.Duration(config.EmptyBlockPeriod-config.BlockPeriod) * time.Second
+			if config.EmptyBlockPeriod > config.BlockPeriod {
+				log.Info("EmptyBlockPeriod detected adding delay to request", "EmptyBlockPeriod", config.EmptyBlockPeriod, "BlockTime", block.Time())
+				delay = time.Duration(config.EmptyBlockPeriod) * time.Second
+			}
 		}
 
 		c.newRoundTimer = time.AfterFunc(delay, func() {

consensus/istanbul/qbft/engine/engine.go

@@ -101,11 +101,11 @@ func (e *Engine) VerifyBlockProposal(chain consensus.ChainHeaderReader, block *t
 		return time.Until(time.Unix(int64(block.Header().Time), 0)), consensus.ErrFutureBlock
 	}
 
-	config := e.cfg.GetConfig(block.Number())
+	parentHeader := chain.GetHeaderByHash(block.ParentHash())
+	config := e.cfg.GetConfig(parentHeader.Number)
+
 	if config.EmptyBlockPeriod > config.BlockPeriod && len(block.Transactions()) == 0 {
-		// empty block verification
-		parentHeader := chain.GetHeaderByHash(block.ParentHash())
-		if parentHeader != nil && block.Header().Time < parentHeader.Time+config.EmptyBlockPeriod {
+		if block.Header().Time < parentHeader.Time+config.EmptyBlockPeriod {
 			return 0, fmt.Errorf("empty block verification fail")
 		}
 	}
@@ -205,10 +205,8 @@ func (e *Engine) verifyCascadingFields(chain consensus.ChainHeaderReader, header
 		return consensus.ErrUnknownAncestor
 	}
 
-	blockPeriod := e.cfg.GetConfig(parent.Number).BlockPeriod
-
 	// Ensure that the block's timestamp isn't too close to it's parent
-	if parent.Time+blockPeriod > header.Time {
+	if parent.Time+e.cfg.GetConfig(parent.Number).BlockPeriod > header.Time {
 		return istanbulcommon.ErrInvalidTimestamp
 	}
 

eth/ethconfig/config.go

@@ -299,11 +299,8 @@ func setBFTConfig(istanbulConfig *istanbul.Config, bftConfig *params.BFTConfig)
 	if bftConfig.BlockPeriodSeconds != 0 {
 		istanbulConfig.BlockPeriod = bftConfig.BlockPeriodSeconds
 	}
-	if bftConfig.EmptyBlockPeriodSeconds != 0 {
-		istanbulConfig.EmptyBlockPeriod = bftConfig.EmptyBlockPeriodSeconds
-	}
-	if bftConfig.EmptyBlockPeriodSeconds < bftConfig.BlockPeriodSeconds {
-		istanbulConfig.EmptyBlockPeriod = bftConfig.BlockPeriodSeconds
+	if bftConfig.EmptyBlockPeriodSeconds != nil {
+		istanbulConfig.EmptyBlockPeriod = *bftConfig.EmptyBlockPeriodSeconds
 	}
 	if bftConfig.RequestTimeoutSeconds != 0 {
 		istanbulConfig.RequestTimeout = bftConfig.RequestTimeoutSeconds * 1000

eth/ethconfig/config_test.go

@@ -25,31 +25,29 @@ func TestSetBFT(t *testing.T) {
 	assert.Equal(t, config.ProposerPolicy, istanbul.DefaultConfig.ProposerPolicy)
 
 	bftConfig = &params.BFTConfig{
-		EpochLength:             10000,
-		Ceil2Nby3Block:          big.NewInt(10),
-		RequestTimeoutSeconds:   100,
-		EmptyBlockPeriodSeconds: 1, // less than block period is making this disable, will be replace by block period
-		BlockPeriodSeconds:      5,
+		EpochLength:           10000,
+		Ceil2Nby3Block:        big.NewInt(10),
+		RequestTimeoutSeconds: 100,
+		BlockPeriodSeconds:    5,
 	}
 	setBFTConfig(config, bftConfig)
 	assert.Equal(t, config.Ceil2Nby3Block, bftConfig.Ceil2Nby3Block)
 	assert.Equal(t, config.Epoch, bftConfig.EpochLength)
 	assert.Equal(t, config.RequestTimeout, bftConfig.RequestTimeoutSeconds*1000)
 	assert.Equal(t, config.BlockPeriod, uint64(5))
-	assert.Equal(t, config.EmptyBlockPeriod, uint64(5))
+	assert.Equal(t, config.EmptyBlockPeriod, uint64(0))
 	assert.Equal(t, config.ProposerPolicy, istanbul.DefaultConfig.ProposerPolicy)
 
 	bftConfig = &params.BFTConfig{
-		EpochLength:             10000,
-		Ceil2Nby3Block:          big.NewInt(10),
-		RequestTimeoutSeconds:   100,
-		EmptyBlockPeriodSeconds: 5,
+		EpochLength:           10000,
+		Ceil2Nby3Block:        big.NewInt(10),
+		RequestTimeoutSeconds: 100,
 	}
 	setBFTConfig(config, bftConfig)
 	assert.Equal(t, config.Ceil2Nby3Block, bftConfig.Ceil2Nby3Block)
 	assert.Equal(t, config.Epoch, bftConfig.EpochLength)
 	assert.Equal(t, config.RequestTimeout, bftConfig.RequestTimeoutSeconds*1000)
 	assert.Equal(t, config.BlockPeriod, istanbul.DefaultConfig.BlockPeriod)
-	assert.Equal(t, config.EmptyBlockPeriod, uint64(5))
+	assert.Equal(t, config.EmptyBlockPeriod, uint64(0))
 	assert.Equal(t, config.ProposerPolicy, istanbul.DefaultConfig.ProposerPolicy)
 }

params/config.go

@@ -409,13 +409,13 @@ func (c *IstanbulConfig) String() string {
 }
 
 type BFTConfig struct {
-	EpochLength              uint64         `json:"epochlength"`              // Number of blocks that should pass before pending validator votes are reset
-	BlockPeriodSeconds       uint64         `json:"blockperiodseconds"`       // Minimum time between two consecutive IBFT or QBFT blocks’ timestamps in seconds
-	EmptyBlockPeriodSeconds  uint64         `json:"emptyblockperiodseconds"`  // Minimum time between two consecutive IBFT or QBFT a block and empty block’ timestamps in seconds
-	RequestTimeoutSeconds    uint64         `json:"requesttimeoutseconds"`    // Minimum request timeout for each IBFT or QBFT round in milliseconds
-	ProposerPolicy           uint64         `json:"policy"`                   // The policy for proposer selection
-	Ceil2Nby3Block           *big.Int       `json:"ceil2Nby3Block,omitempty"` // Number of confirmations required to move from one state to next [2F + 1 to Ceil(2N/3)]
-	ValidatorContractAddress common.Address `json:"validatorcontractaddress"` // Smart contract address for list of validators
+	EpochLength              uint64         `json:"epochlength"`                       // Number of blocks that should pass before pending validator votes are reset
+	BlockPeriodSeconds       uint64         `json:"blockperiodseconds"`                // Minimum time between two consecutive IBFT or QBFT blocks’ timestamps in seconds
+	EmptyBlockPeriodSeconds  *uint64        `json:"emptyblockperiodseconds,omitempty"` // Minimum time between two consecutive IBFT or QBFT a block and empty block’ timestamps in seconds
+	RequestTimeoutSeconds    uint64         `json:"requesttimeoutseconds"`             // Minimum request timeout for each IBFT or QBFT round in milliseconds
+	ProposerPolicy           uint64         `json:"policy"`                            // The policy for proposer selection
+	Ceil2Nby3Block           *big.Int       `json:"ceil2Nby3Block,omitempty"`          // Number of confirmations required to move from one state to next [2F + 1 to Ceil(2N/3)]
+	ValidatorContractAddress common.Address `json:"validatorcontractaddress"`          // Smart contract address for list of validators
 }
 
 type IBFTConfig struct {
@@ -447,7 +447,7 @@ type Transition struct {
 	Algorithm                    string         `json:"algorithm,omitempty"`
 	EpochLength                  uint64         `json:"epochlength,omitempty"`                  // Number of blocks that should pass before pending validator votes are reset
 	BlockPeriodSeconds           uint64         `json:"blockperiodseconds,omitempty"`           // Minimum time between two consecutive IBFT or QBFT blocks’ timestamps in seconds
-	EmptyBlockPeriodSeconds      uint64         `json:"emptyblockperiodseconds,omitempty"`      // Minimum time between two consecutive IBFT or QBFT a block and empty block’ timestamps in seconds
+	EmptyBlockPeriodSeconds      *uint64        `json:"emptyblockperiodseconds,omitempty"`      // Minimum time between two consecutive IBFT or QBFT a block and empty block’ timestamps in seconds
 	RequestTimeoutSeconds        uint64         `json:"requesttimeoutseconds,omitempty"`        // Minimum request timeout for each IBFT or QBFT round in milliseconds
 	ContractSizeLimit            uint64         `json:"contractsizelimit,omitempty"`            // Maximum smart contract code size
 	ValidatorContractAddress     common.Address `json:"validatorcontractaddress"`               // Smart contract address for list of validators

params/config_test.go

@@ -330,11 +330,12 @@ func TestCheckTransitionsData(t *testing.T) {
 		wantErr error
 	}
 	var ibftTransitionsConfig, qbftTransitionsConfig, invalidTransition, invalidBlockOrder []Transition
+	var emptyBlockPeriodSeconds uint64 = 10
 
-	tranI0 := Transition{big.NewInt(0), IBFT, 30000, 5, 5, 10, 50, common.Address{}, "", nil, nil, nil, nil, 0, nil, 0}
-	tranQ5 := Transition{big.NewInt(5), QBFT, 30000, 5, 10, 10, 50, common.Address{}, "", nil, nil, nil, nil, 0, nil, 0}
-	tranI10 := Transition{big.NewInt(10), IBFT, 30000, 5, 5, 10, 50, common.Address{}, "", nil, nil, nil, nil, 0, nil, 0}
-	tranQ8 := Transition{big.NewInt(8), QBFT, 30000, 5, 10, 10, 50, common.Address{}, "", nil, nil, nil, nil, 0, nil, 0}
+	tranI0 := Transition{big.NewInt(0), IBFT, 30000, 5, nil, 10, 50, common.Address{}, "", nil, nil, nil, nil, 0, nil, 0}
+	tranQ5 := Transition{big.NewInt(5), QBFT, 30000, 5, &emptyBlockPeriodSeconds, 10, 50, common.Address{}, "", nil, nil, nil, nil, 0, nil, 0}
+	tranI10 := Transition{big.NewInt(10), IBFT, 30000, 5, nil, 10, 50, common.Address{}, "", nil, nil, nil, nil, 0, nil, 0}
+	tranQ8 := Transition{big.NewInt(8), QBFT, 30000, 5, &emptyBlockPeriodSeconds, 10, 50, common.Address{}, "", nil, nil, nil, nil, 0, nil, 0}
 
 	ibftTransitionsConfig = append(ibftTransitionsConfig, tranI0, tranI10)
 	qbftTransitionsConfig = append(qbftTransitionsConfig, tranQ5, tranQ8)
@@ -394,7 +395,7 @@ func TestCheckTransitionsData(t *testing.T) {
 			wantErr: ErrBlockOrder,
 		},
 		{
-			stored:  &ChainConfig{Transitions: []Transition{{nil, IBFT, 30000, 5, 10, 10, 50, common.Address{}, "", nil, nil, nil, nil, 0, nil, 0}}},
+			stored:  &ChainConfig{Transitions: []Transition{{nil, IBFT, 30000, 5, &emptyBlockPeriodSeconds, 10, 50, common.Address{}, "", nil, nil, nil, nil, 0, nil, 0}}},
 			wantErr: ErrBlockNumberMissing,
 		},
 		{