From 767fe584b596ce45117e51b0de6514840ff99a09 Mon Sep 17 00:00:00 2001 From: atvanguard <93arpit@gmail.com> Date: Mon, 18 May 2020 12:37:20 +0530 Subject: [PATCH 1/3] fix: revert cancel active seal logic --- consensus/bor/bor.go | 52 ++++++----------------------------------- consensus/bor/errors.go | 11 --------- consensus/consensus.go | 1 - core/blockchain.go | 1 - 4 files changed, 7 insertions(+), 58 deletions(-) diff --git a/consensus/bor/bor.go b/consensus/bor/bor.go index 7ee2f52be1..0d244548eb 100644 --- a/consensus/bor/bor.go +++ b/consensus/bor/bor.go @@ -127,12 +127,6 @@ var ( errRecentlySigned = errors.New("recently signed") ) -// ActiveSealingOp keeps the context of the active sealing operation -type ActiveSealingOp struct { - number uint64 - cancel context.CancelFunc -} - // SignerFn is a signer callback function to request a header to be signed by a // backing account. type SignerFn func(accounts.Account, string, []byte) ([]byte, error) @@ -238,9 +232,8 @@ type Bor struct { stateReceiverABI abi.ABI HeimdallClient IHeimdallClient - stateDataFeed event.Feed - scope event.SubscriptionScope - activeSealingOp *ActiveSealingOp + stateDataFeed event.Feed + scope event.SubscriptionScope // The fields below are for testing only fakeDiff bool // Skip difficulty verifications } @@ -728,9 +721,6 @@ func (c *Bor) Authorize(signer common.Address, signFn SignerFn) { // Seal implements consensus.Engine, attempting to create a sealed block using // the local signing credentials. func (c *Bor) Seal(chain consensus.ChainReader, block *types.Block, results chan<- *types.Block, stop <-chan struct{}) error { - // if c.activeSealingOp != nil { - // return &SealingInFlightError{c.activeSealingOp.number} - // } header := block.Header() // Sealing the genesis block is not supported @@ -777,17 +767,12 @@ func (c *Bor) Seal(chain consensus.ChainReader, block *types.Block, results chan // Wait until sealing is terminated or delay timeout. log.Trace("Waiting for slot to sign and propagate", "delay", common.PrettyDuration(delay)) - shouldSeal := make(chan bool) - go c.WaitForSealingOp(number, shouldSeal, delay, stop) go func() { - defer func() { - close(shouldSeal) - c.activeSealingOp = nil - }() - switch <-shouldSeal { - case false: + select { + case <-stop: + log.Debug("Discarding sealing operation for block", "number", number) return - case true: + case <-time.After(delay): if wiggle > 0 { log.Info( "Sealing out-of-turn", @@ -803,38 +788,15 @@ func (c *Bor) Seal(chain consensus.ChainReader, block *types.Block, results chan "headerDifficulty", header.Difficulty, ) } - select { case results <- block.WithSeal(header): default: - log.Warn("Sealing result was not read by miner", "sealhash", SealHash(header)) + log.Warn("Sealing result was not read by miner", "number", number, "sealhash", SealHash(header)) } }() return nil } -// WaitForSealingOp blocks until delay elapses or stop signal is received -func (c *Bor) WaitForSealingOp(number uint64, shouldSeal chan bool, delay time.Duration, stop <-chan struct{}) { - ctx, cancel := context.WithCancel(context.Background()) - c.activeSealingOp = &ActiveSealingOp{number, cancel} - select { - case <-stop: - shouldSeal <- false - case <-ctx.Done(): - shouldSeal <- false - case <-time.After(delay): - shouldSeal <- true - } -} - -// CancelActiveSealingOp cancels in-flight sealing process -func (c *Bor) CancelActiveSealingOp() { - if c.activeSealingOp != nil { - log.Debug("Discarding active sealing operation", "number", c.activeSealingOp.number) - c.activeSealingOp.cancel() - } -} - // CalcDifficulty is the difficulty adjustment algorithm. It returns the difficulty // that a new block should have based on the previous blocks in the chain and the // current signer. diff --git a/consensus/bor/errors.go b/consensus/bor/errors.go index f357705db4..7e0b90941c 100644 --- a/consensus/bor/errors.go +++ b/consensus/bor/errors.go @@ -69,17 +69,6 @@ func (e *MaxCheckpointLengthExceededError) Error() string { ) } -type SealingInFlightError struct { - Number uint64 -} - -func (e *SealingInFlightError) Error() string { - return fmt.Sprintf( - "Requested concurrent block sealing. Sealing for block %d is already in progress", - e.Number, - ) -} - // MismatchingValidatorsError is returned if a last block in sprint contains a // list of validators different from the one that local node calculated type MismatchingValidatorsError struct { diff --git a/consensus/consensus.go b/consensus/consensus.go index 0620cff059..0372391138 100644 --- a/consensus/consensus.go +++ b/consensus/consensus.go @@ -120,7 +120,6 @@ type Engine interface { type Bor interface { Engine IsValidatorAction(chain ChainReader, from common.Address, tx *types.Transaction) bool - CancelActiveSealingOp() } // PoW is a consensus engine based on proof-of-work. diff --git a/core/blockchain.go b/core/blockchain.go index 3ce4356929..e90ffbb30e 100644 --- a/core/blockchain.go +++ b/core/blockchain.go @@ -1718,7 +1718,6 @@ func (bc *BlockChain) insertChain(chain types.Blocks, verifySeals bool) (int, [] if lastCanon != nil && bc.CurrentBlock().Hash() == lastCanon.Hash() { events = append(events, ChainHeadEvent{lastCanon}) } - bc.engine.(consensus.Bor).CancelActiveSealingOp() return it.index, events, coalescedLogs, err } From c7ea09a019c1fc9eaff6ed6a35e42935e504acda Mon Sep 17 00:00:00 2001 From: atvanguard <93arpit@gmail.com> Date: Mon, 18 May 2020 13:33:27 +0530 Subject: [PATCH 2/3] dev: Change wording for UnauthorizedSignerError, remove errRecentlySigned --- consensus/bor/bor.go | 4 --- consensus/bor/bor_test/bor_test.go | 27 ++++++++++++++++ consensus/bor/bor_test/snapshot_test.go | 8 ++--- consensus/bor/errors.go | 42 ++++++++++--------------- consensus/bor/snapshot.go | 13 ++------ 5 files changed, 51 insertions(+), 43 deletions(-) diff --git a/consensus/bor/bor.go b/consensus/bor/bor.go index 0d244548eb..a8f90dfe66 100644 --- a/consensus/bor/bor.go +++ b/consensus/bor/bor.go @@ -121,10 +121,6 @@ var ( // errOutOfRangeChain is returned if an authorization list is attempted to // be modified via out-of-range or non-contiguous headers. errOutOfRangeChain = errors.New("out of range or non-contiguous chain") - - // errRecentlySigned is returned if a header is signed by an authorized entity - // that already signed a header recently, thus is temporarily not allowed to. - errRecentlySigned = errors.New("recently signed") ) // SignerFn is a signer callback function to request a header to be signed by a diff --git a/consensus/bor/bor_test/bor_test.go b/consensus/bor/bor_test/bor_test.go index 62d929220a..1c6c530e82 100644 --- a/consensus/bor/bor_test/bor_test.go +++ b/consensus/bor/bor_test/bor_test.go @@ -160,3 +160,30 @@ func TestOutOfTurnSigning(t *testing.T) { _, err = chain.InsertChain([]*types.Block{block}) assert.Nil(t, err) } + +func TestSignerNotFound(t *testing.T) { + init := buildEthereumInstance(t, rawdb.NewMemoryDatabase()) + chain := init.ethereum.BlockChain() + engine := init.ethereum.Engine() + _bor := engine.(*bor.Bor) + + res, _ := loadSpanFromFile(t) + h := &mocks.IHeimdallClient{} + h.On("FetchWithRetry", "bor", "span", "1").Return(res, nil) + _bor.SetHeimdallClient(h) + + db := init.ethereum.ChainDb() + block := init.genesis.ToBlock(db) + + // random signer account that is not a part of the validator set + signer := "3714d99058cd64541433d59c6b391555b2fd9b54629c2b717a6c9c00d1127b6b" + signerKey, _ := hex.DecodeString(signer) + key, _ = crypto.HexToECDSA(signer) + addr = crypto.PubkeyToAddress(key.PublicKey) + + block = buildNextBlock(t, _bor, chain, block, signerKey, init.genesis.Config.Bor) + _, err := chain.InsertChain([]*types.Block{block}) + assert.Equal(t, + *err.(*bor.UnauthorizedSignerError), + bor.UnauthorizedSignerError{Number: 1, Signer: addr.Bytes()}) +} diff --git a/consensus/bor/bor_test/snapshot_test.go b/consensus/bor/bor_test/snapshot_test.go index 264c632952..6eb43b3e60 100644 --- a/consensus/bor/bor_test/snapshot_test.go +++ b/consensus/bor/bor_test/snapshot_test.go @@ -84,9 +84,9 @@ func TestGetSignerSuccessionNumber_ProposerNotFound(t *testing.T) { signer := snap.ValidatorSet.Validators[3].Address _, err := snap.GetSignerSuccessionNumber(signer) assert.NotNil(t, err) - e, ok := err.(*bor.ProposerNotFoundError) + e, ok := err.(*bor.UnauthorizedProposerError) assert.True(t, ok) - assert.Equal(t, dummyProposerAddress, e.Address) + assert.Equal(t, dummyProposerAddress.Bytes(), e.Proposer) } func TestGetSignerSuccessionNumber_SignerNotFound(t *testing.T) { @@ -97,9 +97,9 @@ func TestGetSignerSuccessionNumber_SignerNotFound(t *testing.T) { dummySignerAddress := randomAddress() _, err := snap.GetSignerSuccessionNumber(dummySignerAddress) assert.NotNil(t, err) - e, ok := err.(*bor.SignerNotFoundError) + e, ok := err.(*bor.UnauthorizedSignerError) assert.True(t, ok) - assert.Equal(t, dummySignerAddress, e.Address) + assert.Equal(t, dummySignerAddress.Bytes(), e.Signer) } func buildRandomValidatorSet(numVals int) []*bor.Validator { diff --git a/consensus/bor/errors.go b/consensus/bor/errors.go index 7e0b90941c..2153c8c4a5 100644 --- a/consensus/bor/errors.go +++ b/consensus/bor/errors.go @@ -2,30 +2,8 @@ package bor import ( "fmt" - - "github.com/maticnetwork/bor/common" ) -// Will include any new bor consensus errors here in an attempt to make error messages more descriptive - -// ProposerNotFoundError is returned if the given proposer address is not present in the validator set -type ProposerNotFoundError struct { - Address common.Address -} - -func (e *ProposerNotFoundError) Error() string { - return fmt.Sprintf("Proposer: %s not found", e.Address.Hex()) -} - -// SignerNotFoundError is returned when the signer address is not present in the validator set -type SignerNotFoundError struct { - Address common.Address -} - -func (e *SignerNotFoundError) Error() string { - return fmt.Sprintf("Signer: %s not found", e.Address.Hex()) -} - // TotalVotingPowerExceededError is returned when the maximum allowed total voting power is exceeded type TotalVotingPowerExceededError struct { Sum int64 @@ -99,7 +77,21 @@ func (e *BlockTooSoonError) Error() string { ) } -// UnauthorizedSignerError is returned if a header is signed by a non-authorized entity. +// UnauthorizedProposerError is returned if a header is [being] signed by an unauthorized entity. +type UnauthorizedProposerError struct { + Number uint64 + Proposer []byte +} + +func (e *UnauthorizedProposerError) Error() string { + return fmt.Sprintf( + "Proposer 0x%x is not a part of the producer set at block %d", + e.Proposer, + e.Number, + ) +} + +// UnauthorizedSignerError is returned if a header is [being] signed by an unauthorized entity. type UnauthorizedSignerError struct { Number uint64 Signer []byte @@ -107,9 +99,9 @@ type UnauthorizedSignerError struct { func (e *UnauthorizedSignerError) Error() string { return fmt.Sprintf( - "Validator set for block %d doesn't contain the signer 0x%x\n", - e.Number, + "Signer 0x%x is not a part of the producer set at block %d", e.Signer, + e.Number, ) } diff --git a/consensus/bor/snapshot.go b/consensus/bor/snapshot.go index a62c2014b9..2ff7dec0ce 100644 --- a/consensus/bor/snapshot.go +++ b/consensus/bor/snapshot.go @@ -26,7 +26,6 @@ import ( "github.com/maticnetwork/bor/core/types" "github.com/maticnetwork/bor/ethdb" "github.com/maticnetwork/bor/internal/ethapi" - "github.com/maticnetwork/bor/log" "github.com/maticnetwork/bor/params" ) @@ -191,24 +190,18 @@ func (s *Snapshot) GetSignerSuccessionNumber(signer common.Address) (int, error) proposer := s.ValidatorSet.GetProposer().Address proposerIndex, _ := s.ValidatorSet.GetByAddress(proposer) if proposerIndex == -1 { - return -1, &ProposerNotFoundError{proposer} + return -1, &UnauthorizedProposerError{s.Number, proposer.Bytes()} } signerIndex, _ := s.ValidatorSet.GetByAddress(signer) if signerIndex == -1 { - return -1, &SignerNotFoundError{signer} + return -1, &UnauthorizedSignerError{s.Number, signer.Bytes()} } - limit := len(validators)/2 + 1 tempIndex := signerIndex - if proposerIndex != tempIndex && limit > 0 { + if proposerIndex != tempIndex { if tempIndex < proposerIndex { tempIndex = tempIndex + len(validators) } - - if tempIndex-proposerIndex > limit { - log.Info("errRecentlySigned", "proposerIndex", validators[proposerIndex].Address.Hex(), "signerIndex", validators[signerIndex].Address.Hex()) - return -1, errRecentlySigned - } } return tempIndex - proposerIndex, nil } From 1be0c4894afee14c7aae7ebab3074b47cdab9389 Mon Sep 17 00:00:00 2001 From: atvanguard <93arpit@gmail.com> Date: Mon, 18 May 2020 14:17:13 +0530 Subject: [PATCH 3/3] dev: provide previous block number to UnauthorizedSignerError --- consensus/bor/bor.go | 6 ++++-- consensus/bor/bor_test/bor_test.go | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/consensus/bor/bor.go b/consensus/bor/bor.go index a8f90dfe66..fac5500b76 100644 --- a/consensus/bor/bor.go +++ b/consensus/bor/bor.go @@ -555,7 +555,8 @@ func (c *Bor) verifySeal(chain consensus.ChainReader, header *types.Header, pare return err } if !snap.ValidatorSet.HasAddress(signer.Bytes()) { - return &UnauthorizedSignerError{number, signer.Bytes()} + // Check the UnauthorizedSignerError.Error() msg to see why we pass number-1 + return &UnauthorizedSignerError{number - 1, signer.Bytes()} } succession, err := snap.GetSignerSuccessionNumber(signer) @@ -741,7 +742,8 @@ func (c *Bor) Seal(chain consensus.ChainReader, block *types.Block, results chan // Bail out if we're unauthorized to sign a block if !snap.ValidatorSet.HasAddress(signer.Bytes()) { - return &UnauthorizedSignerError{number, signer.Bytes()} + // Check the UnauthorizedSignerError.Error() msg to see why we pass number-1 + return &UnauthorizedSignerError{number - 1, signer.Bytes()} } successionNumber, err := snap.GetSignerSuccessionNumber(signer) diff --git a/consensus/bor/bor_test/bor_test.go b/consensus/bor/bor_test/bor_test.go index 1c6c530e82..98331996f9 100644 --- a/consensus/bor/bor_test/bor_test.go +++ b/consensus/bor/bor_test/bor_test.go @@ -185,5 +185,5 @@ func TestSignerNotFound(t *testing.T) { _, err := chain.InsertChain([]*types.Block{block}) assert.Equal(t, *err.(*bor.UnauthorizedSignerError), - bor.UnauthorizedSignerError{Number: 1, Signer: addr.Bytes()}) + bor.UnauthorizedSignerError{Number: 0, Signer: addr.Bytes()}) }