fix penalty calculation bug (#189)

consensus/XDPoS/engines/engine_v2/timeout.go

@@ -68,7 +68,7 @@ func (x *XDPoS_v2) onTimeoutPoolThresholdReached(blockChainReader consensus.Chai
 	syncInfo := x.getSyncInfo()
 	x.broadcastToBftChannel(syncInfo)
 
-	log.Info("Successfully processed the timeout message and produced TC & SyncInfo!", "TcRound", timeoutCert.Round, "NumberOfTcSig", len(timeoutCert.Signatures))
+	log.Info("Successfully processed the timeout message and produced TC & SyncInfo!", "QcRound", syncInfo.HighestQuorumCert.ProposedBlockInfo.Round, "QcBlockNum", syncInfo.HighestQuorumCert.ProposedBlockInfo.Number, "TcRound", timeoutCert.Round, "NumberOfTcSig", len(timeoutCert.Signatures))
 	return nil
 }
 

consensus/tests/engine_v2_tests/penalty_test.go

@@ -53,9 +53,10 @@ func TestHookPenaltyV2Mining(t *testing.T) {
 		Coinbase:   acc1Addr,
 	}
 	// Force to make the node to be at its round to mine, otherwise won't pass the yourturn masternodes check
-	// We have 19 nodes in total (20 candidates in snapshot - 1 penalty) and the fake signer is always at the 18th(last) in the list. Hence int(config.XDPoS.Epoch)*3+18-900, the +18 means is to force to next 18 round and -900 is the relative round number to block number int(config.XDPoS.Epoch)*3
+	// We have 19 nodes in total (20 candidates in snapshot - 1 penalty) and the fake signer is always at the 18th(last) in the list.
+	// Hence int(config.XDPoS.Epoch)*3+18-900, the +18 means is to force to next 18 round and -900 is the relative round number to block number int(config.XDPoS.Epoch)*3
 	adaptor.EngineV2.SetNewRoundFaker(blockchain, types.Round(int(config.XDPoS.Epoch)*3+18-900), false)
-	// The test default signer is not in the msaternodes, so we set the faker signer
+	// The test default signer is not in the masternodes, so we set the faker signer
 	adaptor.EngineV2.AuthorizeFaker(acc1Addr)
 	err = adaptor.Prepare(blockchain, headerMining)
 	assert.Nil(t, err)
@@ -106,10 +107,32 @@ func TestHookPenaltyV2Jump(t *testing.T) {
 	assert.Nil(t, err)
 	masternodes := adaptor.GetMasternodesFromCheckpointHeader(header901)
 	assert.Equal(t, 5, len(masternodes))
-	header2085 := blockchain.GetHeaderByNumber(uint64(end))
+	header2685 := blockchain.GetHeaderByNumber(uint64(end))
 	adaptor.EngineV2.SetNewRoundFaker(blockchain, types.Round(config.XDPoS.Epoch*3), false)
-	// round 2085-2100 miss blocks, penalty should work as usual
+	// round 2685-2700 miss blocks, penalty should work as usual
-	penalty, err := adaptor.EngineV2.HookPenalty(blockchain, header2085.Number, header2085.ParentHash, masternodes)
+	penalty, err := adaptor.EngineV2.HookPenalty(blockchain, header2685.Number, header2685.ParentHash, masternodes)
+	assert.Nil(t, err)
+	assert.Equal(t, 2, len(penalty))
+}
+
+// Test calculate penalty under startRange blocks, currently is 150
+func TestHookPenaltyV2LessThen150Blocks(t *testing.T) {
+	config := params.TestXDPoSMockChainConfig
+	blockchain, _, _, _, _ := PrepareXDCTestBlockChainWithPenaltyForV2Engine(t, int(config.XDPoS.Epoch)*3, config)
+	adaptor := blockchain.Engine().(*XDPoS.XDPoS)
+	hooks.AttachConsensusV2Hooks(adaptor, blockchain, config)
+	assert.NotNil(t, adaptor.EngineV2.HookPenalty)
+	var extraField types.ExtraFields_v2
+	// 901 is the first v2 block
+	header901 := blockchain.GetHeaderByNumber(config.XDPoS.Epoch + 1)
+	err := utils.DecodeBytesExtraFields(header901.Extra, &extraField)
+	assert.Nil(t, err)
+	masternodes := adaptor.GetMasternodesFromCheckpointHeader(header901)
+	assert.Equal(t, 5, len(masternodes))
+	header1900 := blockchain.GetHeaderByNumber(1900)
+	adaptor.EngineV2.SetNewRoundFaker(blockchain, types.Round(config.XDPoS.Epoch*3), false)
+	// penalty count from 1900
+	penalty, err := adaptor.EngineV2.HookPenalty(blockchain, header1900.Number, header1900.ParentHash, masternodes)
 	assert.Nil(t, err)
 	assert.Equal(t, 2, len(penalty))
 }

eth/bft/bft_handler.go

@@ -174,9 +174,11 @@ func (b *Bfter) Stop() {
 	close(b.quit)
 }
 func (b *Bfter) loop() {
+	log.Info("BFT Loop Start")
 	for {
 		select {
 		case <-b.quit:
+			log.Warn("BFT Loop Close")
 			return
 		case obj := <-b.broadcastCh:
 			switch v := obj.(type) {

eth/hooks/engine_v2_hooks.go

@@ -22,11 +22,10 @@ func AttachConsensusV2Hooks(adaptor *XDPoS.XDPoS, bc *core.BlockChain, chainConf
 	// Hook scans for bad masternodes and decide to penalty them
 	adaptor.EngineV2.HookPenalty = func(chain consensus.ChainReader, number *big.Int, currentHash common.Hash, candidates []common.Address) ([]common.Address, error) {
 		start := time.Now()
-		listBlockHash := make([]common.Hash, chain.Config().XDPoS.Epoch)
+		listBlockHash := []common.Hash{}
-
 		// get list block hash & stats total created block
 		statMiners := make(map[common.Address]int)
-		listBlockHash[0] = currentHash
+		listBlockHash = append(listBlockHash, currentHash)
 		parentNumber := number.Uint64() - 1
 		parentHash := currentHash
 
@@ -63,9 +62,10 @@ func AttachConsensusV2Hooks(adaptor *XDPoS.XDPoS, bc *core.BlockChain, chainConf
 			} else {
 				statMiners[miner] = 1
 			}
-			parentHash = parentHeader.ParentHash
-			listBlockHash[i] = parentHash
 			parentNumber--
+			parentHash = parentHeader.ParentHash
+			listBlockHash = append(listBlockHash, parentHash)
+			log.Debug("[HookPenalty] listBlockHash", "i", i, "len", len(listBlockHash), "parentHash", parentHash, "parentNumber", parentNumber)
 		}
 
 		// add list not miner to penalties
@@ -109,35 +109,34 @@ func AttachConsensusV2Hooks(adaptor *XDPoS.XDPoS, bc *core.BlockChain, chainConf
 			startRange = len(listBlockHash) - 1
 		}
 		for i := startRange; i >= 0; i-- {
-			if len(penComebacks) > 0 {
+			if len(penComebacks) == 0 {
-				blockNumber := number.Uint64() - uint64(i) - 1
+				break
-				bhash := listBlockHash[i]
+			}
-				if blockNumber%common.MergeSignRange == 0 {
+			blockNumber := number.Uint64() - uint64(i) - 1
-					mapBlockHash[bhash] = true
+			bhash := listBlockHash[i]
-				}
+			if blockNumber%common.MergeSignRange == 0 {
-				signData, ok := adaptor.GetCachedSigningTxs(bhash)
+				mapBlockHash[bhash] = true
-				if !ok {
+			}
-					block := chain.GetBlock(bhash, blockNumber)
+			signData, ok := adaptor.GetCachedSigningTxs(bhash)
-					txs := block.Transactions()
+			if !ok {
-					signData = adaptor.CacheSigningTxs(bhash, txs)
+				block := chain.GetBlock(bhash, blockNumber)
-				}
+				txs := block.Transactions()
-				txs := signData.([]*types.Transaction)
+				signData = adaptor.CacheSigningTxs(bhash, txs)
-				// Check signer signed?
+			}
-				for _, tx := range txs {
+			txs := signData.([]*types.Transaction)
-					blkHash := common.BytesToHash(tx.Data()[len(tx.Data())-32:])
+			// Check signer signed?
-					from := *tx.From()
+			for _, tx := range txs {
-					if mapBlockHash[blkHash] {
+				blkHash := common.BytesToHash(tx.Data()[len(tx.Data())-32:])
-						for j, addr := range penComebacks {
+				from := *tx.From()
-							if from == addr {
+				if mapBlockHash[blkHash] {
-								// Remove it from dupSigners.
+					for j, addr := range penComebacks {
-								penComebacks = append(penComebacks[:j], penComebacks[j+1:]...)
+						if from == addr {
-								break
+							// Remove it from dupSigners.
-							}
+							penComebacks = append(penComebacks[:j], penComebacks[j+1:]...)
+							break
 						}
 					}
 				}
-			} else {
-				break
 			}
 		}