Fix issues with changing blockperiod down and up

- fix validation of empty blocks
Consensys · Sep 16, 2022 · a4ef457 · a4ef457
1 parent 957349b
commit a4ef457
Show file tree

Hide file tree

Showing 7 changed files with 23 additions and 28 deletions.
diff --git a/cmd/utils/flags.go b/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

diff --git a/consensus/istanbul/config.go b/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
 }
 

diff --git a/consensus/istanbul/ibft/engine/engine.go b/consensus/istanbul/ibft/engine/engine.go
@@ -156,6 +156,7 @@ func (e *Engine) verifyCascadingFields(chain consensus.ChainHeaderReader, header
 		return consensus.ErrUnknownAncestor
 	}
 
+
 	// Ensure that the block's timestamp isn't too close to it's parent
 	if parent.Time+e.cfg.GetConfig(parent.Number).BlockPeriod > header.Time {
 		return istanbulcommon.ErrInvalidTimestamp

diff --git a/consensus/istanbul/qbft/core/request.go b/consensus/istanbul/qbft/core/request.go
@@ -17,10 +17,11 @@
 package core
 
 import (
+	"github.com/ethereum/go-ethereum/core/types"
+	"github.com/ethereum/go-ethereum/log"
 	"time"
 
 	"github.com/ethereum/go-ethereum/consensus/istanbul"
-	"github.com/ethereum/go-ethereum/core/types"
 )
 
 // 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() {

diff --git a/consensus/istanbul/qbft/engine/engine.go b/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
 	}
 

diff --git a/eth/ethconfig/config.go b/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

diff --git a/params/config.go b/params/config.go
@@ -411,7 +411,7 @@ 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
+	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)]
@@ -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