From eb54d32e2cae851b48ac69b35883b83658eece25 Mon Sep 17 00:00:00 2001 From: Rongjian Lan Date: Tue, 24 Mar 2020 16:31:36 -0700 Subject: [PATCH] refactor slashing code and fix audit todos --- consensus/checks.go | 16 ---- consensus/double_sign.go | 119 ++++++++++++++++++++++++++++++ consensus/leader.go | 98 +----------------------- core/blockchain.go | 20 ++--- internal/chain/engine.go | 104 +++++++++++++++++++------- shard/committee/assignment.go | 2 +- staking/slash/double-sign.go | 5 +- staking/slash/double-sign_test.go | 1 - 8 files changed, 213 insertions(+), 152 deletions(-) create mode 100644 consensus/double_sign.go diff --git a/consensus/checks.go b/consensus/checks.go index 551838ce0..24de07851 100644 --- a/consensus/checks.go +++ b/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, diff --git a/consensus/double_sign.go b/consensus/double_sign.go new file mode 100644 index 000000000..811ef57a8 --- /dev/null +++ b/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 +} diff --git a/consensus/leader.go b/consensus/leader.go index 9a3827ab2..21de0b5fc 100644 --- a/consensus/leader.go +++ b/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() diff --git a/core/blockchain.go b/core/blockchain.go index 309abc660..beef24577 100644 --- a/core/blockchain.go +++ b/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) } diff --git a/internal/chain/engine.go b/internal/chain/engine.go index b730e7e8e..1800f1389 100644 --- a/internal/chain/engine.go +++ b/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,37 +385,87 @@ 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 err != nil { - return errors.New("could not read shard state") + if _, ok := groupedRecords[thisKey]; ok { + groupedRecords[thisKey] = append(groupedRecords[thisKey], doubleSigners[i]) + } else { + groupedRecords[thisKey] = slash.Records{doubleSigners[i]} + } } - staked := superCommittee.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) - utils.Logger().Info(). - Str("rate", rate.String()). - RawJSON("records", []byte(doubleSigners.String())). - Msg("now applying slash to state during block finalization") - if slashApplied, err = slash.Apply( - chain, - state, - doubleSigners, - rate, - ); err != nil { - return ctxerror.New("[Finalize] could not apply slash").WithCause(err) + sortedKeys := []keyStruct{} + + for key := range groupedRecords { + sortedKeys = append(sortedKeys, key) } - utils.Logger().Info(). - Str("rate", rate.String()). - RawJSON("records", []byte(doubleSigners.String())). - RawJSON("applied", []byte(slashApplied.String())). - Msg("slash applied successfully") + // 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") + } + + 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 + rate := slash.Rate(len(records), staked.CountStakedBLSKey) + utils.Logger().Info(). + Str("rate", rate.String()). + RawJSON("records", []byte(records.String())). + Msg("now applying slash to state during block finalization") + if slashApplied, err = slash.Apply( + chain, + state, + records, + rate, + ); err != nil { + return ctxerror.New("[Finalize] could not apply slash").WithCause(err) + } + + utils.Logger().Info(). + Str("rate", rate.String()). + RawJSON("records", []byte(records.String())). + RawJSON("applied", []byte(slashApplied.String())). + Msg("slash applied successfully") + } return nil } diff --git a/shard/committee/assignment.go b/shard/committee/assignment.go index e442fb566..233a1a2e6 100644 --- a/shard/committee/assignment.go +++ b/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 { diff --git a/staking/slash/double-sign.go b/staking/slash/double-sign.go index dbe2332af..667f13aa5 100644 --- a/staking/slash/double-sign.go +++ b/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 .. diff --git a/staking/slash/double-sign_test.go b/staking/slash/double-sign_test.go index b07a1fa2e..5b4b36e54 100644 --- a/staking/slash/double-sign_test.go +++ b/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,