Merge pull request ethereum#65 from maticnetwork/revert-active-seal

fix: revert cancel active seal / remove errRecentlySigned

consensus/bor/bor.go

@@ -121,18 +121,8 @@ 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")
 )
 
-// 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 +228,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
 }
@@ -566,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)
@@ -728,9 +718,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
@@ -755,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)
@@ -777,17 +765,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 +786,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.

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: 0, Signer: addr.Bytes()})
+}

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 {

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
@@ -69,17 +47,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 {
@@ -110,17 +77,31 @@ 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
 }
 
 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,
 	)
 }
 

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
 }

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.

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
 }