Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

qbft: add new block period setting used when the block is empty #1433

Merged
merged 14 commits into from
Jul 28, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions cmd/utils/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -2603,6 +2603,12 @@ 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.RequestTimeoutSeconds != 0 {
istanbulConfig.RequestTimeout = bftConfig.RequestTimeoutSeconds * 1000
}
Expand Down
1 change: 1 addition & 0 deletions cmd/utils/flags_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,5 +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, true, arbitraryEthConfig.RaftMode, "RaftModeFlag value is incorrect")
}
8 changes: 8 additions & 0 deletions consensus/istanbul/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ func (p *ProposerPolicy) ClearRegistry() {
type Config struct {
RequestTimeout uint64 `toml:",omitempty"` // The timeout for each Istanbul round in milliseconds.
BlockPeriod uint64 `toml:",omitempty"` // Default minimum difference between two consecutive block's timestamps in second
EmptyBlockPeriod uint64 `toml:",omitempty"` // Default minimum difference between a block and empty block's timestamps in second
ProposerPolicy *ProposerPolicy `toml:",omitempty"` // The policy for proposer selection
Epoch uint64 `toml:",omitempty"` // The number of blocks after which to checkpoint and reset the pending votes
Ceil2Nby3Block *big.Int `toml:",omitempty"` // Number of confirmations required to move from one state to next [2F + 1 to Ceil(2N/3)]
Expand All @@ -136,6 +137,7 @@ type Config struct {
var DefaultConfig = &Config{
RequestTimeout: 10000,
BlockPeriod: 1,
EmptyBlockPeriod: 10,
ProposerPolicy: NewRoundRobinProposerPolicy(),
Epoch: 30000,
Ceil2Nby3Block: big.NewInt(0),
Expand Down Expand Up @@ -182,6 +184,12 @@ func (c Config) GetConfig(blockNumber *big.Int) Config {
if c.Transitions[i].BlockPeriodSeconds != 0 {
newConfig.BlockPeriod = c.Transitions[i].BlockPeriodSeconds
}
if c.Transitions[i].EmptyBlockPeriodSeconds != 0 {
newConfig.EmptyBlockPeriod = c.Transitions[i].EmptyBlockPeriodSeconds
}
}
if newConfig.EmptyBlockPeriod < newConfig.BlockPeriod {
newConfig.EmptyBlockPeriod = newConfig.BlockPeriod
}
return newConfig
}
Expand Down
9 changes: 7 additions & 2 deletions consensus/istanbul/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,19 @@ func TestGetConfig(t *testing.T) {
Block: big.NewInt(3),
BlockPeriodSeconds: 5,
}, {
Block: big.NewInt(5),
RequestTimeoutSeconds: 15000,
Block: big.NewInt(5),
RequestTimeoutSeconds: 15000,
EmptyBlockPeriodSeconds: 1,
}}
config1 := *DefaultConfig
config1.Epoch = 40000
config1.BlockPeriod = 1 // default value
config1.EmptyBlockPeriod = 10 // default value
config3 := config1
config3.BlockPeriod = 5
config3.EmptyBlockPeriod = 10
config5 := config3
config5.EmptyBlockPeriod = 5 // equals to block period because it can be lower than this active value (5) from block 3
config5.RequestTimeout = 15000

type test struct {
Expand Down
7 changes: 7 additions & 0 deletions consensus/istanbul/qbft/core/core.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ type core struct {
pendingRequestsMu *sync.Mutex

consensusTimestamp time.Time

newRoundMutex sync.Mutex
newRoundTimer *time.Timer
}

func (c *core) currentView() *istanbul.View {
Expand Down Expand Up @@ -251,6 +254,10 @@ func (c *core) stopTimer() {
func (c *core) newRoundChangeTimer() {
c.stopTimer()

for c.current == nil { // wait because it is asynchronous in handleRequest
time.Sleep(10 * time.Millisecond)
}

// set timeout based on the round number
baseTimeout := time.Duration(c.config.GetConfig(c.current.Sequence()).RequestTimeout) * time.Millisecond
round := c.current.Round().Uint64()
Expand Down
30 changes: 26 additions & 4 deletions consensus/istanbul/qbft/core/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@
package core

import (
"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()`
Expand All @@ -42,11 +45,30 @@ func (c *core) handleRequest(request *Request) error {

c.current.pendingRequest = request
if c.state == StateAcceptRequest {
// Start ROUND-CHANGE timer
c.newRoundChangeTimer()
c.newRoundMutex.Lock()
defer c.newRoundMutex.Unlock()

if c.newRoundTimer != nil {
c.newRoundTimer.Stop()
c.newRoundTimer = nil
}

// Send PRE-PREPARE message to other validators
c.sendPreprepareMsg(request)
delay := time.Duration(0)

block, ok := request.Proposal.(*types.Block)
if ok && len(block.Transactions()) == 0 { // if empty block
delay = -time.Since(time.Unix(int64(block.Time()), 0)) + time.Duration(c.config.EmptyBlockPeriod-c.config.BlockPeriod)*time.Second
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block.Time() should be replaced with the timestamp in the header of the parent of block

Also, is delay going negative going to create any issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have the parent block information at this place, block time should be the parent block time + block period as value.
I am checking for the negative value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no problem for negative value or 0, it is executed immediately
https://go.dev/play/p/QB80T3p4KRa

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delta between block's time and now should be 0, I will check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quorum-examples-node1-1  | INFO [07-08|14:58:32.176] qbft block                               time=1657292312 since=176.173958ms
quorum-examples-node1-1  | INFO [07-08|14:58:44.035] qbft block                               time=1657292324 since=35.466547ms
quorum-examples-node1-1  | INFO [07-08|14:58:53.048] qbft block                               time=1657292333 since=48.515009ms
quorum-examples-node1-1  | INFO [07-08|14:59:02.044] qbft block                               time=1657292342 since=44.105264ms
quorum-examples-node1-1  | INFO [07-08|14:59:11.016] qbft block                               time=1657292351 since=16.593976ms
quorum-examples-node1-1  | INFO [07-08|14:59:20.078] qbft block                               time=1657292360 since=78.740772ms
quorum-examples-node1-1  | INFO [07-08|14:59:29.062] qbft block                               time=1657292369 since=62.659776ms
quorum-examples-node1-1  | INFO [07-08|14:59:38.058] qbft block                               time=1657292378 since=58.693822ms
quorum-examples-node1-1  | INFO [07-08|14:59:47.045] qbft block                               time=1657292387 since=45.062118ms
quorum-examples-node1-1  | INFO [07-08|14:59:56.047] qbft block                               time=1657292396 since=47.224956ms
quorum-examples-node1-1  | INFO [07-08|15:00:05.019] qbft block                               time=1657292405 since=19.969544ms
quorum-examples-node1-1  | INFO [07-08|15:00:14.035] qbft block                               time=1657292414 since=35.883881ms
quorum-examples-node1-1  | INFO [07-08|15:00:23.028] qbft block                               time=1657292423 since=28.577886ms
quorum-examples-node1-1  | INFO [07-08|15:00:32.038] qbft block                               time=1657292432 since=38.90014ms

so as it is in seconds, there is no need to add time.Since(time.Unix(int64(block.Time()), 0)), it is negligeable, and we are back to my original formula

}

c.newRoundTimer = time.AfterFunc(delay, func() {
c.newRoundTimer = nil

// Start ROUND-CHANGE timer
c.newRoundChangeTimer()

// Send PRE-PREPARE message to other validators
c.sendPreprepareMsg(request)
})
}

return nil
Expand Down
15 changes: 13 additions & 2 deletions consensus/istanbul/qbft/engine/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package qbftengine

import (
"bytes"
"fmt"
"math/big"
"time"

Expand Down Expand Up @@ -100,6 +101,14 @@ func (e *Engine) VerifyBlockProposal(chain consensus.ChainHeaderReader, block *t
return time.Until(time.Unix(int64(block.Header().Time), 0)), consensus.ErrFutureBlock
}

if e.cfg.EmptyBlockPeriod > e.cfg.BlockPeriod && len(block.Transactions()) == 0 {
// empty block verification
parentHeader := chain.GetHeaderByHash(block.ParentHash())
if parentHeader != nil && block.Header().Time < parentHeader.Time+e.cfg.EmptyBlockPeriod {
return 0, fmt.Errorf("empty block verification fail")
}
}

return 0, err
}

Expand Down Expand Up @@ -195,8 +204,10 @@ 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+e.cfg.GetConfig(parent.Number).BlockPeriod > header.Time {
if parent.Time+blockPeriod > header.Time {
return istanbulcommon.ErrInvalidTimestamp
}

Expand Down Expand Up @@ -317,7 +328,7 @@ func (e *Engine) Prepare(chain consensus.ChainHeaderReader, header *types.Header
header.Difficulty = istanbulcommon.DefaultDifficulty

// set header's timestamp
header.Time = parent.Time + e.cfg.GetConfig(header.Number).BlockPeriod
header.Time = parent.Time + e.cfg.GetConfig(header.Number).BlockPeriod // TODO use block period is block is not empty
baptiste-b-pegasys marked this conversation as resolved.
Show resolved Hide resolved
if header.Time < uint64(time.Now().Unix()) {
header.Time = uint64(time.Now().Unix())
}
Expand Down
6 changes: 6 additions & 0 deletions eth/ethconfig/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,12 @@ 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.RequestTimeoutSeconds != 0 {
istanbulConfig.RequestTimeout = bftConfig.RequestTimeoutSeconds * 1000
}
Expand Down
30 changes: 30 additions & 0 deletions eth/ethconfig/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,35 @@ func TestSetBFT(t *testing.T) {
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, istanbul.DefaultConfig.EmptyBlockPeriod)
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,
}
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.ProposerPolicy, istanbul.DefaultConfig.ProposerPolicy)

bftConfig = &params.BFTConfig{
EpochLength: 10000,
Ceil2Nby3Block: big.NewInt(10),
RequestTimeoutSeconds: 100,
EmptyBlockPeriodSeconds: 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, istanbul.DefaultConfig.BlockPeriod)
assert.Equal(t, config.EmptyBlockPeriod, uint64(5))
assert.Equal(t, config.ProposerPolicy, istanbul.DefaultConfig.ProposerPolicy)
}
14 changes: 8 additions & 6 deletions params/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,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
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)]
Expand Down Expand Up @@ -436,12 +437,13 @@ const (
type Transition struct {
Block *big.Int `json:"block"`
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
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
ValidatorSelectionMode string `json:"validatorselectionmode"` // Validator selection mode to switch to
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
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
ValidatorSelectionMode string `json:"validatorselectionmode"` // Validator selection mode to switch to
}

// String implements the fmt.Stringer interface.
Expand Down
10 changes: 5 additions & 5 deletions params/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -330,10 +330,10 @@ func TestCheckTransitionsData(t *testing.T) {
wantErr error
}
var ibftTransitionsConfig, qbftTransitionsConfig, invalidTransition, invalidBlockOrder []Transition
tranI0 := Transition{big.NewInt(0), IBFT, 30000, 5, 10, 50, common.Address{}, ""}
tranQ5 := Transition{big.NewInt(5), QBFT, 30000, 5, 10, 50, common.Address{}, ""}
tranI10 := Transition{big.NewInt(10), IBFT, 30000, 5, 10, 50, common.Address{}, ""}
tranQ8 := Transition{big.NewInt(8), QBFT, 30000, 5, 10, 50, common.Address{}, ""}
tranI0 := Transition{big.NewInt(0), IBFT, 30000, 5, 10, 10, 50, common.Address{}, ""}
tranQ5 := Transition{big.NewInt(5), QBFT, 30000, 5, 10, 10, 50, common.Address{}, ""}
tranI10 := Transition{big.NewInt(10), IBFT, 30000, 5, 10, 10, 50, common.Address{}, ""}
tranQ8 := Transition{big.NewInt(8), QBFT, 30000, 5, 10, 10, 50, common.Address{}, ""}

ibftTransitionsConfig = append(ibftTransitionsConfig, tranI0, tranI10)
qbftTransitionsConfig = append(qbftTransitionsConfig, tranQ5, tranQ8)
Expand Down Expand Up @@ -393,7 +393,7 @@ func TestCheckTransitionsData(t *testing.T) {
wantErr: ErrBlockOrder,
},
{
stored: &ChainConfig{Transitions: []Transition{{nil, IBFT, 30000, 5, 10, 50, common.Address{}, ""}}},
stored: &ChainConfig{Transitions: []Transition{{nil, IBFT, 30000, 5, 10, 10, 50, common.Address{}, ""}}},
wantErr: ErrBlockNumberMissing,
},
{
Expand Down