From e1278520d064cd7bf64997cd90686efc23dedc63 Mon Sep 17 00:00:00 2001 From: Amit Panghal Date: Mon, 30 Sep 2019 15:26:13 -0400 Subject: [PATCH] Chain stalls while scaling out from 1 to 4 nodes, changing quorum size fixes things (#796) * Adding Ceil2Nby3Block genesis config option to specify the number of blocks required to transition from 2f+1 to Ceil(2n/3) in IBFT * Add support for using ceil(2N/3) in IBFT --- consensus/istanbul/backend/engine.go | 7 +++---- consensus/istanbul/config.go | 4 ++++ consensus/istanbul/core/commit.go | 2 +- consensus/istanbul/core/commit_test.go | 16 ++++++++-------- consensus/istanbul/core/core.go | 9 +++++++++ consensus/istanbul/core/core_test.go | 18 ++++++++++++++++++ consensus/istanbul/core/prepare.go | 2 +- consensus/istanbul/core/prepare_test.go | 18 +++++++++--------- consensus/istanbul/core/roundchange.go | 4 ++-- eth/backend.go | 2 ++ eth/handler_test.go | 2 +- params/config.go | 9 +++++++-- params/config_test.go | 17 +++++++++++++++++ 13 files changed, 82 insertions(+), 28 deletions(-) diff --git a/consensus/istanbul/backend/engine.go b/consensus/istanbul/backend/engine.go index 3fd681bd2..9349039f2 100644 --- a/consensus/istanbul/backend/engine.go +++ b/consensus/istanbul/backend/engine.go @@ -27,8 +27,8 @@ import ( "github.com/ethereum/go-ethereum/common/hexutil" "github.com/ethereum/go-ethereum/consensus" "github.com/ethereum/go-ethereum/consensus/istanbul" - istanbulCore "github.com/ethereum/go-ethereum/consensus/istanbul/core" "github.com/ethereum/go-ethereum/consensus/istanbul/validator" + istanbulCore "github.com/ethereum/go-ethereum/consensus/istanbul/core" "github.com/ethereum/go-ethereum/core/state" "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/crypto/sha3" @@ -289,8 +289,8 @@ func (sb *backend) verifyCommittedSeals(chain consensus.ChainReader, header *typ } } - // The length of validSeal should be larger than number of faulty node + 1 - if validSeal <= 2*snap.ValSet.F() { + // The length of validSeal should be larger than number of faulty node + 1 + if validSeal <= snap.ValSet.F() { return errInvalidCommittedSeals } @@ -616,7 +616,6 @@ func sigHash(header *types.Header) (hash common.Hash) { return hash } - // SealHash returns the hash of a block prior to it being sealed. func (sb *backend) SealHash(header *types.Header) common.Hash { return sigHash(header) diff --git a/consensus/istanbul/config.go b/consensus/istanbul/config.go index d2d44bf2e..2a223a1a5 100644 --- a/consensus/istanbul/config.go +++ b/consensus/istanbul/config.go @@ -16,6 +16,8 @@ package istanbul +import "math/big" + type ProposerPolicy uint64 const ( @@ -28,6 +30,7 @@ type Config struct { BlockPeriod uint64 `toml:",omitempty"` // Default minimum difference between two consecutive 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)] } var DefaultConfig = &Config{ @@ -35,4 +38,5 @@ var DefaultConfig = &Config{ BlockPeriod: 1, ProposerPolicy: RoundRobin, Epoch: 30000, + Ceil2Nby3Block: big.NewInt(0), } diff --git a/consensus/istanbul/core/commit.go b/consensus/istanbul/core/commit.go index fa5810225..166d1925e 100644 --- a/consensus/istanbul/core/commit.go +++ b/consensus/istanbul/core/commit.go @@ -72,7 +72,7 @@ func (c *core) handleCommit(msg *message, src istanbul.Validator) error { // // If we already have a proposal, we may have chance to speed up the consensus process // by committing the proposal without PREPARE messages. - if c.current.Commits.Size() > 2*c.valSet.F() && c.state.Cmp(StateCommitted) < 0 { + if c.current.Commits.Size() >= c.QuorumSize() && c.state.Cmp(StateCommitted) < 0 { // Still need to call LockHash here since state can skip Prepared state and jump directly to the Committed state. c.current.LockHash() c.commit() diff --git a/consensus/istanbul/core/commit_test.go b/consensus/istanbul/core/commit_test.go index cca5cbbac..97be7b062 100644 --- a/consensus/istanbul/core/commit_test.go +++ b/consensus/istanbul/core/commit_test.go @@ -191,8 +191,8 @@ OUTER: if r0.state != StatePrepared { t.Errorf("state mismatch: have %v, want %v", r0.state, StatePrepared) } - if r0.current.Commits.Size() > 2*r0.valSet.F() { - t.Errorf("the size of commit messages should be less than %v", 2*r0.valSet.F()+1) + if r0.current.Commits.Size() >= r0.QuorumSize() { + t.Errorf("the size of commit messages should be less than %v", r0.QuorumSize()) } if r0.current.IsHashLocked() { t.Errorf("block should not be locked") @@ -200,12 +200,12 @@ OUTER: continue } - // core should have 2F+1 prepare messages - if r0.current.Commits.Size() <= 2*r0.valSet.F() { - t.Errorf("the size of commit messages should be larger than 2F+1: size %v", r0.current.Commits.Size()) + // core should have 2F+1 before Ceil2Nby3Block or Ceil(2N/3) prepare messages + if r0.current.Commits.Size() < r0.QuorumSize() { + t.Errorf("the size of commit messages should be larger than 2F+1 or Ceil(2N/3): size %v", r0.QuorumSize()) } - // check signatures large than 2F+1 + // check signatures large than F signedCount := 0 committedSeals := v0.committedMsgs[0].committedSeals for _, validator := range r0.valSet.List() { @@ -216,8 +216,8 @@ OUTER: } } } - if signedCount <= 2*r0.valSet.F() { - t.Errorf("the expected signed count should be larger than %v, but got %v", 2*r0.valSet.F(), signedCount) + if signedCount <= r0.valSet.F() { + t.Errorf("the expected signed count should be larger than %v, but got %v", r0.valSet.F(), signedCount) } if !r0.current.IsHashLocked() { t.Errorf("block should be locked") diff --git a/consensus/istanbul/core/core.go b/consensus/istanbul/core/core.go index c4fd5501b..a10b5b8f8 100644 --- a/consensus/istanbul/core/core.go +++ b/consensus/istanbul/core/core.go @@ -342,6 +342,15 @@ func (c *core) checkValidatorSignature(data []byte, sig []byte) (common.Address, return istanbul.CheckValidatorSignature(c.valSet, data, sig) } +func (c *core) QuorumSize() int { + if c.config.Ceil2Nby3Block == nil || (c.current != nil && c.current.sequence.Cmp(c.config.Ceil2Nby3Block) < 0) { + c.logger.Trace("Confirmation Formula used 2F+ 1") + return (2 * c.valSet.F()) + 1 + } + c.logger.Trace("Confirmation Formula used ceil(2N/3)") + return int(math.Ceil(float64(2*c.valSet.Size()) / 3)) +} + // PrepareCommittedSeal returns a committed seal for the given hash func PrepareCommittedSeal(hash common.Hash) []byte { var buf bytes.Buffer diff --git a/consensus/istanbul/core/core_test.go b/consensus/istanbul/core/core_test.go index a293ea9ee..e5c753485 100644 --- a/consensus/istanbul/core/core_test.go +++ b/consensus/istanbul/core/core_test.go @@ -17,6 +17,7 @@ package core import ( + "github.com/ethereum/go-ethereum/common" "math/big" "reflect" "testing" @@ -80,3 +81,20 @@ func TestNewRequest(t *testing.T) { } } } + +func TestQuorumSize(t *testing.T) { + N := uint64(4) + F := uint64(1) + + sys := NewTestSystemWithBackend(N, F) + backend := sys.backends[0] + c := backend.engine.(*core) + + valSet := c.valSet + for i := 1; i <= 1000; i++ { + valSet.AddValidator(common.StringToAddress(string(i))) + if 2*c.QuorumSize() <= (valSet.Size()+valSet.F()) || 2*c.QuorumSize() > (valSet.Size()+valSet.F()+2) { + t.Errorf("quorumSize constraint failed, expected value (2*QuorumSize > Size+F && 2*QuorumSize <= Size+F+2) to be:%v, got: %v, for size: %v", true, false, valSet.Size()) + } + } +} diff --git a/consensus/istanbul/core/prepare.go b/consensus/istanbul/core/prepare.go index f4ea25ae1..4a1154c06 100644 --- a/consensus/istanbul/core/prepare.go +++ b/consensus/istanbul/core/prepare.go @@ -59,7 +59,7 @@ func (c *core) handlePrepare(msg *message, src istanbul.Validator) error { // Change to Prepared state if we've received enough PREPARE messages or it is locked // and we are in earlier state before Prepared state. - if ((c.current.IsHashLocked() && prepare.Digest == c.current.GetLockedHash()) || c.current.GetPrepareOrCommitSize() > 2*c.valSet.F()) && + if ((c.current.IsHashLocked() && prepare.Digest == c.current.GetLockedHash()) || c.current.GetPrepareOrCommitSize() >= c.QuorumSize()) && c.state.Cmp(StatePrepared) < 0 { c.current.LockHash() c.setState(StatePrepared) diff --git a/consensus/istanbul/core/prepare_test.go b/consensus/istanbul/core/prepare_test.go index a2b495d85..ac6ef475c 100644 --- a/consensus/istanbul/core/prepare_test.go +++ b/consensus/istanbul/core/prepare_test.go @@ -17,6 +17,7 @@ package core import ( + "math" "math/big" "reflect" "testing" @@ -156,12 +157,11 @@ func TestHandlePrepare(t *testing.T) { errInconsistentSubject, }, { - // less than 2F+1 func() *testSystem { sys := NewTestSystemWithBackend(N, F) - // save less than 2*F+1 replica - sys.backends = sys.backends[2*int(F)+1:] + // save less than Ceil(2*N/3) replica + sys.backends = sys.backends[int(math.Ceil(float64(2*N)/3)):] for i, backend := range sys.backends { c := backend.engine.(*core) @@ -214,8 +214,8 @@ OUTER: if r0.state != StatePreprepared { t.Errorf("state mismatch: have %v, want %v", r0.state, StatePreprepared) } - if r0.current.Prepares.Size() > 2*r0.valSet.F() { - t.Errorf("the size of PREPARE messages should be less than %v", 2*r0.valSet.F()+1) + if r0.current.Prepares.Size() >= r0.QuorumSize() { + t.Errorf("the size of PREPARE messages should be less than %v", r0.QuorumSize()) } if r0.current.IsHashLocked() { t.Errorf("block should not be locked") @@ -224,12 +224,12 @@ OUTER: continue } - // core should have 2F+1 PREPARE messages - if r0.current.Prepares.Size() <= 2*r0.valSet.F() { - t.Errorf("the size of PREPARE messages should be larger than 2F+1: size %v", r0.current.Commits.Size()) + // core should have 2F+1 before Ceil2Nby3Block and Ceil(2N/3) after Ceil2Nby3Block PREPARE messages + if r0.current.Prepares.Size() < r0.QuorumSize() { + t.Errorf("the size of PREPARE messages should be larger than 2F+1 or ceil(2N/3): size %v", r0.current.Commits.Size()) } - // a message will be delivered to backend if 2F+1 + // a message will be delivered to backend if ceil(2N/3) if int64(len(v0.sentMsgs)) != 1 { t.Errorf("the Send() should be called once: times %v", len(test.system.backends[0].sentMsgs)) } diff --git a/consensus/istanbul/core/roundchange.go b/consensus/istanbul/core/roundchange.go index 89ffc41be..0289ef041 100644 --- a/consensus/istanbul/core/roundchange.go +++ b/consensus/istanbul/core/roundchange.go @@ -98,8 +98,8 @@ func (c *core) handleRoundChange(msg *message, src istanbul.Validator) error { c.sendRoundChange(roundView.Round) } return nil - } else if num == int(2*c.valSet.F()+1) && (c.waitingForRoundChange || cv.Round.Cmp(roundView.Round) < 0) { - // We've received 2f+1 ROUND CHANGE messages, start a new round immediately. + } else if num == c.QuorumSize() && (c.waitingForRoundChange || cv.Round.Cmp(roundView.Round) < 0) { + // We've received 2f+1/Ceil(2N/3) ROUND CHANGE messages, start a new round immediately. c.startNewRound(roundView.Round) return nil } else if cv.Round.Cmp(roundView.Round) < 0 { diff --git a/eth/backend.go b/eth/backend.go index 2483c43a0..ab213d875 100644 --- a/eth/backend.go +++ b/eth/backend.go @@ -257,6 +257,8 @@ func CreateConsensusEngine(ctx *node.ServiceContext, chainConfig *params.ChainCo config.Istanbul.Epoch = chainConfig.Istanbul.Epoch } config.Istanbul.ProposerPolicy = istanbul.ProposerPolicy(chainConfig.Istanbul.ProposerPolicy) + config.Istanbul.Ceil2Nby3Block = chainConfig.Istanbul.Ceil2Nby3Block + return istanbulBackend.New(&config.Istanbul, ctx.NodeKey(), db) } diff --git a/eth/handler_test.go b/eth/handler_test.go index 9583b3cde..d163dbb91 100644 --- a/eth/handler_test.go +++ b/eth/handler_test.go @@ -82,7 +82,7 @@ func TestNodeInfo(t *testing.T) { }{ {"ethash", nil, nil, false}, {"raft", nil, nil, true}, - {"istanbul", nil, ¶ms.IstanbulConfig{1, 1}, false}, + {"istanbul", nil, ¶ms.IstanbulConfig{1, 1, big.NewInt(0)}, false}, {"clique", ¶ms.CliqueConfig{1, 1}, nil, false}, } diff --git a/params/config.go b/params/config.go index 436a383a0..e837fc7ac 100644 --- a/params/config.go +++ b/params/config.go @@ -124,6 +124,7 @@ var ( Istanbul: &IstanbulConfig{ Epoch: 30000, ProposerPolicy: 0, + Ceil2Nby3Block: big.NewInt(0), }, } @@ -214,8 +215,9 @@ func (c *CliqueConfig) String() string { // IstanbulConfig is the consensus engine configs for Istanbul based sealing. type IstanbulConfig struct { - Epoch uint64 `json:"epoch"` // Epoch length to reset votes and checkpoint - ProposerPolicy uint64 `json:"policy"` // The policy for proposer selection + Epoch uint64 `json:"epoch"` // Epoch length to reset votes and checkpoint + 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)] } // String implements the stringer interface, returning the consensus engine details. @@ -375,6 +377,9 @@ func (c *ChainConfig) checkCompatible(newcfg *ChainConfig, head *big.Int, isQuor if isForkIncompatible(c.EWASMBlock, newcfg.EWASMBlock, head) { return newCompatError("ewasm fork block", c.EWASMBlock, newcfg.EWASMBlock) } + if c.Istanbul != nil && newcfg.Istanbul != nil && isForkIncompatible(c.Istanbul.Ceil2Nby3Block, newcfg.Istanbul.Ceil2Nby3Block, head) { + return newCompatError("Ceil 2N/3 fork block", c.Istanbul.Ceil2Nby3Block, newcfg.Istanbul.Ceil2Nby3Block) + } return nil } diff --git a/params/config_test.go b/params/config_test.go index 61a8bbab7..259205b85 100644 --- a/params/config_test.go +++ b/params/config_test.go @@ -70,6 +70,23 @@ func TestCheckCompatible(t *testing.T) { RewindTo: 9, }, }, + { + stored: &ChainConfig{Istanbul: &IstanbulConfig{Ceil2Nby3Block: big.NewInt(10)}}, + new: &ChainConfig{Istanbul: &IstanbulConfig{Ceil2Nby3Block: big.NewInt(20)}}, + head: 4, + wantErr: nil, + }, + { + stored: &ChainConfig{Istanbul: &IstanbulConfig{Ceil2Nby3Block: big.NewInt(10)}}, + new: &ChainConfig{Istanbul: &IstanbulConfig{Ceil2Nby3Block: big.NewInt(20)}}, + head: 30, + wantErr: &ConfigCompatError{ + What: "Ceil 2N/3 fork block", + StoredConfig: big.NewInt(10), + NewConfig: big.NewInt(20), + RewindTo: 9, + }, + }, } for _, test := range tests {