refactor slashing code and fix audit todos

consensus/checks.go

@@ -92,22 +92,6 @@ func (consensus *Consensus) isRightBlockNumAndViewID(recvMsg *FBFTMessage,
 	return true
 }
 
-func (consensus *Consensus) couldThisBeADoubleSigner(
-	recvMsg *FBFTMessage,
-) bool {
-	num, hash, now := consensus.blockNum, recvMsg.BlockHash, consensus.blockNum
-	suspicious := !consensus.FBFTLog.HasMatchingAnnounce(num, hash) ||
-		!consensus.FBFTLog.HasMatchingPrepared(num, hash)
-	if suspicious {
-		consensus.getLogger().Debug().
-			Str("message", recvMsg.String()).
-			Uint64("block-on-consensus", now).
-			Msg("possible double signer")
-		return true
-	}
-	return false
-}
-
 func (consensus *Consensus) onAnnounceSanityChecks(recvMsg *FBFTMessage) bool {
 	logMsgs := consensus.FBFTLog.GetMessagesByTypeSeqView(
 		msg_pb.MessageType_ANNOUNCE, recvMsg.BlockNum, recvMsg.ViewID,

consensus/double_sign.go

@@ -0,0 +1,119 @@
+package consensus
+
+import (
+	"math/big"
+	"time"
+
+	"github.com/ethereum/go-ethereum/common"
+	"github.com/harmony-one/bls/ffi/go/bls"
+	"github.com/harmony-one/harmony/consensus/quorum"
+	"github.com/harmony-one/harmony/consensus/votepower"
+	"github.com/harmony-one/harmony/shard"
+	"github.com/harmony-one/harmony/staking/slash"
+)
+
+// Check for double sign and if any, send it out to beacon chain for slashing.
+func (consensus *Consensus) checkDoubleSign(recvMsg *FBFTMessage) {
+	if consensus.couldThisBeADoubleSigner(recvMsg) {
+		if alreadyCastBallot := consensus.Decider.ReadBallot(
+			quorum.Commit, recvMsg.SenderPubkey,
+		); alreadyCastBallot != nil {
+			firstPubKey := bls.PublicKey{}
+			alreadyCastBallot.SignerPubKey.ToLibBLSPublicKey(&firstPubKey)
+			if recvMsg.SenderPubkey.IsEqual(&firstPubKey) {
+				for _, blk := range consensus.FBFTLog.GetBlocksByNumber(recvMsg.BlockNum) {
+					firstSignedBlock := blk.Header()
+					areHeightsEqual := firstSignedBlock.Number().Uint64() == recvMsg.BlockNum
+					areViewIDsEqual := firstSignedBlock.ViewID().Uint64() == recvMsg.ViewID
+					areHeadersEqual := firstSignedBlock.Hash() == recvMsg.BlockHash
+
+					// If signer already firstSignedBlock, and the block height is the same
+					// and the viewID is the same, then we need to verify the block
+					// hash, and if block hash is different, then that is a clear
+					// case of double signing
+					if areHeightsEqual && areViewIDsEqual && !areHeadersEqual {
+						var doubleSign bls.Sign
+						if err := doubleSign.Deserialize(recvMsg.Payload); err != nil {
+							consensus.getLogger().Err(err).Str("msg", recvMsg.String()).
+								Msg("could not deserialize potential double signer")
+							return
+						}
+
+						curHeader := consensus.ChainReader.CurrentHeader()
+						committee, err := consensus.ChainReader.ReadShardState(curHeader.Epoch())
+						if err != nil {
+							consensus.getLogger().Err(err).
+								Uint32("shard", consensus.ShardID).
+								Uint64("epoch", curHeader.Epoch().Uint64()).
+								Msg("could not read shard state")
+							return
+						}
+						offender := *shard.FromLibBLSPublicKeyUnsafe(recvMsg.SenderPubkey)
+						subComm, err := committee.FindCommitteeByID(
+							consensus.ShardID,
+						)
+						if err != nil {
+							consensus.getLogger().Err(err).
+								Str("msg", recvMsg.String()).
+								Msg("could not find subcommittee for bls key")
+							return
+						}
+
+						addr, err := subComm.AddressForBLSKey(offender)
+
+						if err != nil {
+							consensus.getLogger().Err(err).Str("msg", recvMsg.String()).
+								Msg("could not find address for bls key")
+							return
+						}
+
+						now := big.NewInt(time.Now().UnixNano())
+
+						go func(reporter common.Address) {
+							evid := slash.Evidence{
+								ConflictingBallots: slash.ConflictingBallots{
+									AlreadyCastBallot: *alreadyCastBallot,
+									DoubleSignedBallot: votepower.Ballot{
+										SignerPubKey:    offender,
+										BlockHeaderHash: recvMsg.BlockHash,
+										Signature:       common.Hex2Bytes(doubleSign.SerializeToHexStr()),
+										Height:          recvMsg.BlockNum,
+										ViewID:          recvMsg.ViewID,
+									}},
+								Moment: slash.Moment{
+									Epoch:        curHeader.Epoch(),
+									ShardID:      consensus.ShardID,
+									TimeUnixNano: now,
+								},
+							}
+							proof := slash.Record{
+								Evidence: evid,
+								Reporter: reporter,
+								Offender: *addr,
+							}
+							consensus.SlashChan <- proof
+						}(consensus.SelfAddresses[consensus.LeaderPubKey.SerializeToHexStr()])
+						return
+					}
+				}
+			}
+		}
+		return
+	}
+}
+
+func (consensus *Consensus) couldThisBeADoubleSigner(
+	recvMsg *FBFTMessage,
+) bool {
+	num, hash, now := consensus.blockNum, recvMsg.BlockHash, consensus.blockNum
+	suspicious := !consensus.FBFTLog.HasMatchingAnnounce(num, hash) ||
+		!consensus.FBFTLog.HasMatchingPrepared(num, hash)
+	if suspicious {
+		consensus.getLogger().Debug().
+			Str("message", recvMsg.String()).
+			Uint64("block-on-consensus", now).
+			Msg("possible double signer")
+		return true
+	}
+	return false
+}

consensus/leader.go

@@ -1,9 +1,7 @@
 package consensus
 
 import (
-	"bytes"
 	"encoding/binary"
-	"math/big"
 	"time"
 
 	"github.com/ethereum/go-ethereum/common"
@@ -11,12 +9,9 @@ import (
 	"github.com/harmony-one/bls/ffi/go/bls"
 	msg_pb "github.com/harmony-one/harmony/api/proto/message"
 	"github.com/harmony-one/harmony/consensus/quorum"
-	"github.com/harmony-one/harmony/consensus/votepower"
 	"github.com/harmony-one/harmony/core/types"
 	nodeconfig "github.com/harmony-one/harmony/internal/configs/node"
 	"github.com/harmony-one/harmony/p2p/host"
-	"github.com/harmony-one/harmony/shard"
-	"github.com/harmony-one/harmony/staking/slash"
 )
 
 func (consensus *Consensus) announce(block *types.Block) {
@@ -195,7 +190,6 @@ func (consensus *Consensus) onPrepare(msg *msg_pb.Message) {
 
 func (consensus *Consensus) onCommit(msg *msg_pb.Message) {
 	recvMsg, err := ParseFBFTMessage(msg)
-	log := consensus.getLogger()
 	if err != nil {
 		consensus.getLogger().Debug().Err(err).Msg("[OnCommit] Parse pbft message failed")
 		return
@@ -209,95 +203,6 @@ func (consensus *Consensus) onCommit(msg *msg_pb.Message) {
 	consensus.mutex.Lock()
 	defer consensus.mutex.Unlock()
 
-	// TODO(audit): refactor into a new func
-	if key := (bls.PublicKey{}); consensus.couldThisBeADoubleSigner(recvMsg) {
-		if alreadyCastBallot := consensus.Decider.ReadBallot(
-			quorum.Commit, recvMsg.SenderPubkey,
-		); alreadyCastBallot != nil {
-			for _, blk := range consensus.FBFTLog.GetBlocksByNumber(recvMsg.BlockNum) {
-				alreadyCastBallot.SignerPubKey.ToLibBLSPublicKey(&key)
-				if recvMsg.SenderPubkey.IsEqual(&key) {
-					signed := blk.Header()
-					areHeightsEqual := signed.Number().Uint64() == recvMsg.BlockNum
-					areViewIDsEqual := signed.ViewID().Uint64() == recvMsg.ViewID
-					areHeadersEqual := bytes.Compare(
-						signed.Hash().Bytes(), recvMsg.BlockHash.Bytes(),
-					) == 0
-					// If signer already signed, and the block height is the same
-					// and the viewID is the same, then we need to verify the block
-					// hash, and if block hash is different, then that is a clear
-					// case of double signing
-					if areHeightsEqual && areViewIDsEqual && !areHeadersEqual {
-						var doubleSign bls.Sign
-						if err := doubleSign.Deserialize(recvMsg.Payload); err != nil {
-							log.Err(err).Str("msg", recvMsg.String()).
-								Msg("could not deserialize potential double signer")
-							return
-						}
-
-						curHeader := consensus.ChainReader.CurrentHeader()
-						committee, err := consensus.ChainReader.ReadShardState(curHeader.Epoch())
-						if err != nil {
-							log.Err(err).
-								Uint32("shard", consensus.ShardID).
-								Uint64("epoch", curHeader.Epoch().Uint64()).
-								Msg("could not read shard state")
-							return
-						}
-						offender := *shard.FromLibBLSPublicKeyUnsafe(recvMsg.SenderPubkey)
-						subComm, err := committee.FindCommitteeByID(
-							consensus.ShardID,
-						)
-						if err != nil {
-							log.Err(err).
-								Str("msg", recvMsg.String()).
-								Msg("could not find subcommittee for bls key")
-							return
-						}
-
-						addr, err := subComm.AddressForBLSKey(offender)
-
-						if err != nil {
-							log.Err(err).Str("msg", recvMsg.String()).
-								Msg("could not find address for bls key")
-							return
-						}
-
-						now := big.NewInt(time.Now().UnixNano())
-
-						go func(reporter common.Address) {
-							evid := slash.Evidence{
-								ConflictingBallots: slash.ConflictingBallots{
-									*alreadyCastBallot,
-									votepower.Ballot{
-										SignerPubKey:    offender,
-										BlockHeaderHash: recvMsg.BlockHash,
-										Signature:       common.Hex2Bytes(doubleSign.SerializeToHexStr()),
-										Height:          recvMsg.BlockNum,
-										ViewID:          recvMsg.ViewID,
-									}},
-								Moment: slash.Moment{
-									Epoch:        curHeader.Epoch(),
-									ShardID:      consensus.ShardID,
-									TimeUnixNano: now,
-								},
-								ProposalHeader: signed,
-							}
-							proof := slash.Record{
-								Evidence: evid,
-								Reporter: reporter,
-								Offender: *addr,
-							}
-							consensus.SlashChan <- proof
-						}(consensus.SelfAddresses[consensus.LeaderPubKey.SerializeToHexStr()])
-						return
-					}
-				}
-			}
-		}
-		return
-	}
-
 	validatorPubKey, commitSig, commitBitmap :=
 		recvMsg.SenderPubkey, recvMsg.Payload, consensus.commitBitmap
 	logger := consensus.getLogger().With().
@@ -326,6 +231,9 @@ func (consensus *Consensus) onCommit(msg *msg_pb.Message) {
 		return
 	}
 
+	// Check for potential double signing
+	consensus.checkDoubleSign(recvMsg)
+
 	logger = logger.With().
 		Int64("numReceivedSoFar", consensus.Decider.SignersCount(quorum.Commit)).
 		Logger()

core/blockchain.go

@@ -2036,27 +2036,29 @@ func (bc *BlockChain) AddPendingSlashingCandidates(
 	bc.pendingSlashingCandidatesMU.Lock()
 	defer bc.pendingSlashingCandidatesMU.Unlock()
 	current := bc.ReadPendingSlashingCandidates()
-	pendingSlashes := append(
-		bc.pendingSlashes, current.SetDifference(candidates)...,
-	)
+
 	state, err := bc.State()
 	if err != nil {
 		return err
 	}
 
 	valid := slash.Records{}
-
-	for i := range pendingSlashes {
-		if err := slash.Verify(bc, state, &pendingSlashes[i]); err == nil {
-			valid = append(valid, pendingSlashes[i])
+	for i := range candidates {
+		if err := slash.Verify(bc, state, &candidates[i]); err == nil {
+			valid = append(valid, candidates[i])
 		}
 	}
-	if l, c := len(valid), len(current); l > maxPendingSlashes {
+
+	pendingSlashes := append(
+		bc.pendingSlashes, current.SetDifference(valid)...,
+	)
+
+	if l, c := len(pendingSlashes), len(current); l > maxPendingSlashes {
 		return errors.Wrapf(
 			errExceedMaxPendingSlashes, "current %d with-additional %d", c, l,
 		)
 	}
-	bc.pendingSlashes = valid
+	bc.pendingSlashes = pendingSlashes
 	return bc.writeSlashes(bc.pendingSlashes)
 }
 

internal/chain/engine.go

@@ -3,6 +3,8 @@ package chain
 import (
 	"bytes"
 	"encoding/binary"
+	"math/big"
+	"sort"
 
 	"github.com/harmony-one/harmony/staking/availability"
 
@@ -383,27 +385,76 @@ func applySlashes(
 	state *state.DB,
 	doubleSigners slash.Records,
 ) error {
-	// TODO(audit): should read from the epoch when the slash happened
-	superCommittee, err := chain.ReadShardState(chain.CurrentHeader().Epoch())
+	type keyStruct struct {
+		height  uint64
+		viewID  uint64
+		shardID uint32
+		epoch   uint64
+	}
+
+	groupedRecords := map[keyStruct]slash.Records{}
+
+	// First group slashes by same signed blocks
+	for i := range doubleSigners {
+		thisKey := keyStruct{
+			height:  doubleSigners[i].Evidence.AlreadyCastBallot.Height,
+			viewID:  doubleSigners[i].Evidence.AlreadyCastBallot.ViewID,
+			shardID: doubleSigners[i].Evidence.Moment.ShardID,
+			epoch:   doubleSigners[i].Evidence.Moment.Epoch.Uint64(),
+		}
+
+		if _, ok := groupedRecords[thisKey]; ok {
+			groupedRecords[thisKey] = append(groupedRecords[thisKey], doubleSigners[i])
+		} else {
+			groupedRecords[thisKey] = slash.Records{doubleSigners[i]}
+		}
+	}
+
+	sortedKeys := []keyStruct{}
+
+	for key := range groupedRecords {
+		sortedKeys = append(sortedKeys, key)
+	}
+
+	// Sort them so the slashes are always consistent
+	sort.SliceStable(sortedKeys, func(i, j int) bool {
+		if sortedKeys[i].shardID < sortedKeys[j].shardID {
+			return true
+		} else if sortedKeys[i].height < sortedKeys[j].height {
+			return true
+		} else if sortedKeys[i].viewID < sortedKeys[j].viewID {
+			return true
+		}
+		return false
+	})
+
+	// Do the slashing by groups in the sorted order
+	for _, key := range sortedKeys {
+		records := groupedRecords[key]
+		superCommittee, err := chain.ReadShardState(big.NewInt(int64(key.epoch)))
 
 		if err != nil {
 			return errors.New("could not read shard state")
 		}
 
-	staked := superCommittee.StakedValidators()
+		shardCommittee, err := superCommittee.FindCommitteeByID(key.shardID)
+
+		if err != nil {
+			return errors.New("could not find shard committee")
+		}
+
+		staked := shardCommittee.StakedValidators()
 		// Apply the slashes, invariant: assume been verified as legit slash by this point
 		var slashApplied *slash.Application
-	// TODO(audit): need to group doubleSigners by the target (block) and slash them separately
-	//              the rate of slash should be based on num_keys_signed_on_same_block/total_bls_key
-	rate := slash.Rate(len(doubleSigners), staked.CountStakedBLSKey)
+		rate := slash.Rate(len(records), staked.CountStakedBLSKey)
 		utils.Logger().Info().
 			Str("rate", rate.String()).
-		RawJSON("records", []byte(doubleSigners.String())).
+			RawJSON("records", []byte(records.String())).
 			Msg("now applying slash to state during block finalization")
 		if slashApplied, err = slash.Apply(
 			chain,
 			state,
-		doubleSigners,
+			records,
 			rate,
 		); err != nil {
 			return ctxerror.New("[Finalize] could not apply slash").WithCause(err)
@@ -411,9 +462,10 @@ func applySlashes(
 
 		utils.Logger().Info().
 			Str("rate", rate.String()).
-		RawJSON("records", []byte(doubleSigners.String())).
+			RawJSON("records", []byte(records.String())).
 			RawJSON("applied", []byte(slashApplied.String())).
 			Msg("slash applied successfully")
+	}
 	return nil
 }
 

shard/committee/assignment.go

@@ -292,7 +292,7 @@ func eposStakedCommittee(
 		}
 	}
 
-	// TODO: make sure external validator BLS key are also not duplicate to Harmony's keys
+	// TODO(audit): make sure external validator BLS key are also not duplicate to Harmony's keys
 	completedEPoSRound, err := NewEPoSRound(stakerReader)
 
 	if err != nil {

staking/slash/double-sign.go

@@ -9,7 +9,6 @@ import (
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/rlp"
 	"github.com/harmony-one/bls/ffi/go/bls"
-	"github.com/harmony-one/harmony/block"
 	"github.com/harmony-one/harmony/consensus/votepower"
 	"github.com/harmony-one/harmony/core/state"
 	"github.com/harmony-one/harmony/core/types"
@@ -68,7 +67,6 @@ type Moment struct {
 type Evidence struct {
 	Moment
 	ConflictingBallots
-	ProposalHeader *block.Header `json:"header"`
 }
 
 // ConflictingBallots ..
@@ -101,8 +99,7 @@ func (e Evidence) MarshalJSON() ([]byte, error) {
 	return json.Marshal(struct {
 		Moment
 		ConflictingBallots
-		ProposalHeader *block.Header `json:"header"`
-	}{e.Moment, e.ConflictingBallots, e.ProposalHeader})
+	}{e.Moment, e.ConflictingBallots})
 }
 
 // Records ..

staking/slash/double-sign_test.go

@@ -397,7 +397,6 @@ func defaultSlashRecord() Record {
 				TimeUnixNano: big.NewInt(doubleSignUnixNano),
 				ShardID:      doubleSignShardID,
 			},
-			ProposalHeader: &header,
 		},
 		Reporter: reporterAddr,
 		Offender: offenderAddr,