From 71361da6bd5857d8dac2417186f01333da87f2e9 Mon Sep 17 00:00:00 2001 From: Rongjian Lan Date: Thu, 12 Mar 2020 10:08:37 -0700 Subject: [PATCH] more audit todos and fixes (#2489) --- consensus/consensus.go | 5 +++-- consensus/leader.go | 1 + core/rawdb/accessors_offchain.go | 1 + internal/chain/engine.go | 5 +++++ node/worker/worker.go | 6 +++++- shard/committee/assignment.go | 29 +++++++++++------------------ staking/effective/calculate.go | 6 +----- staking/slash/double-sign.go | 8 ++++---- 8 files changed, 31 insertions(+), 30 deletions(-) diff --git a/consensus/consensus.go b/consensus/consensus.go index 9a7b4bb95..84cb6c766 100644 --- a/consensus/consensus.go +++ b/consensus/consensus.go @@ -79,8 +79,9 @@ type Consensus struct { MinPeers int pubKeyLock sync.Mutex // private/public keys of current node - priKey *multibls.PrivateKey - PubKey *multibls.PublicKey + priKey *multibls.PrivateKey + PubKey *multibls.PublicKey + // TODO(audit): SelfAddresses doesn't have the ECDSA address for external validators. Don't use it that way. SelfAddresses map[string]common.Address // the publickey of leader LeaderPubKey *bls.PublicKey diff --git a/consensus/leader.go b/consensus/leader.go index a704ef8b2..9a3827ab2 100644 --- a/consensus/leader.go +++ b/consensus/leader.go @@ -209,6 +209,7 @@ 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, diff --git a/core/rawdb/accessors_offchain.go b/core/rawdb/accessors_offchain.go index 9cca69353..20d94dfad 100644 --- a/core/rawdb/accessors_offchain.go +++ b/core/rawdb/accessors_offchain.go @@ -106,6 +106,7 @@ func DeletePendingCrossLinks(db DatabaseDeleter) error { } // ReadPendingSlashingCandidates retrieves last pending slashing candidates +// TODO(audit): the pending slashes written in DB is never used. func ReadPendingSlashingCandidates(db DatabaseReader) ([]byte, error) { return db.Get(pendingSlashingKey) } diff --git a/internal/chain/engine.go b/internal/chain/engine.go index f1b84eb60..af4cac0ef 100644 --- a/internal/chain/engine.go +++ b/internal/chain/engine.go @@ -290,6 +290,8 @@ func (e *engineImpl) Finalize( if err := applySlashes(chain, header, state, doubleSigners); err != nil { return nil, nil, err } + } else if len(doubleSigners) > 0 { + return nil, nil, errors.New("slashes proposed in non-beacon chain or non-staking epoch") } // Finalize the state root @@ -370,6 +372,7 @@ 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()) if err != nil { @@ -379,6 +382,8 @@ func applySlashes( 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()). diff --git a/node/worker/worker.go b/node/worker/worker.go index 0538f870d..fe83718bd 100644 --- a/node/worker/worker.go +++ b/node/worker/worker.go @@ -359,7 +359,11 @@ func (w *Worker) verifySlashes( }, ) - workingState := w.GetCurrentState() + // Always base the state on current tip of the chain + workingState, err := w.chain.State() + if err != nil { + return successes, failures + } for i := range d { if err := slash.Verify( diff --git a/shard/committee/assignment.go b/shard/committee/assignment.go index 811a58c13..ea05c43b2 100644 --- a/shard/committee/assignment.go +++ b/shard/committee/assignment.go @@ -171,6 +171,7 @@ func eposStakedCommittee( if validator.EPOSStatus != effective.Active { continue } + // TODO(audit): remove the sanity check here; do the sanity check with maxBLSKey before validator change if err := validator.SanityCheck(maxBLSKey); err != nil { utils.Logger().Info(). Int("staked-candidates", len(candidates)). @@ -178,10 +179,7 @@ func eposStakedCommittee( Msg("validator sanity check failed") continue } - validatorStake := big.NewInt(0) - for _, delegation := range validator.Delegations { - validatorStake.Add(validatorStake, delegation.Amount) - } + totalStake := validator.TotalDelegation() found := false dupKey := shard.BlsPublicKey{} @@ -202,28 +200,23 @@ func eposStakedCommittee( } essentials[validator.Address] = effective.SlotOrder{ - validatorStake, + totalStake, validator.SlotPubKeys, } } - staked := effective.Apply(essentials, stakedSlotsCount) + electedSlots := effective.Apply(essentials, stakedSlotsCount) shardBig := big.NewInt(int64(shardCount)) - if l := len(staked); l < stakedSlotsCount { - // WARN unlikely to happen in production but will happen as we are developing - stakedSlotsCount = l - } - - totalStake := numeric.ZeroDec() + totalEffectiveStake := numeric.ZeroDec() - for i := 0; i < stakedSlotsCount; i++ { - shardID := int(new(big.Int).Mod(staked[i].BlsPublicKey.Big(), shardBig).Int64()) - slot := staked[i] - totalStake = totalStake.Add(slot.Dec) + for i := 0; i < len(electedSlots); i++ { + slot := electedSlots[i] + shardID := int(new(big.Int).Mod(slot.BlsPublicKey.Big(), shardBig).Int64()) + totalEffectiveStake = totalEffectiveStake.Add(slot.Dec) shardState.Shards[shardID].Slots = append(shardState.Shards[shardID].Slots, shard.Slot{ slot.Address, - staked[i].BlsPublicKey, + slot.BlsPublicKey, &slot.Dec, }) } @@ -231,7 +224,7 @@ func eposStakedCommittee( if c := len(candidates); c != 0 { utils.Logger().Info(). Int("staked-candidates", c). - Str("total-staked-by-validators", totalStake.String()). + Str("sum-all-effective-stake-by-validators", totalEffectiveStake.String()). Msg("epos based super-committe") } diff --git a/staking/effective/calculate.go b/staking/effective/calculate.go index 7b0894921..c0f72334d 100644 --- a/staking/effective/calculate.go +++ b/staking/effective/calculate.go @@ -68,7 +68,6 @@ func (s Slots) JSON() string { // Median .. func Median(stakes []SlotPurchase) numeric.Dec { - if len(stakes) == 0 { utils.Logger().Error().Int("non-zero", len(stakes)). Msg("Input to median has len 0, check caller") @@ -76,7 +75,7 @@ func Median(stakes []SlotPurchase) numeric.Dec { sort.SliceStable( stakes, - func(i, j int) bool { return stakes[i].Dec.LT(stakes[j].Dec) }, + func(i, j int) bool { return stakes[i].Dec.GT(stakes[j].Dec) }, ) const isEven = 0 switch l := len(stakes); l % 2 { @@ -132,9 +131,6 @@ func Compute( }) } } - if len(eposedSlots) < len(shortHand) { - // WARN Should never happen - } sort.SliceStable( eposedSlots, diff --git a/staking/slash/double-sign.go b/staking/slash/double-sign.go index 271761acb..dfa038fff 100644 --- a/staking/slash/double-sign.go +++ b/staking/slash/double-sign.go @@ -186,9 +186,8 @@ func Verify( ) } currentEpoch := chain.CurrentBlock().Epoch() - // on beaconchain committee, the slash can't come from the future - if candidate.Evidence.ShardID == shard.BeaconChainShardID && - candidate.Evidence.Epoch.Cmp(currentEpoch) == 1 { + // the slash can't come from the future (shard chain's epoch can't be larger than beacon chain's) + if candidate.Evidence.Epoch.Cmp(currentEpoch) == 1 { return errors.Wrapf( errSlashFromFutureEpoch, "current-epoch %v", currentEpoch, ) @@ -232,6 +231,7 @@ func Verify( } blockNumBytes := make([]byte, 8) + // TODO(audit): add view ID into signature payload binary.LittleEndian.PutUint64(blockNumBytes, ballot.Height) commitPayload := append(blockNumBytes, ballot.BlockHeaderHash[:]...) if !signature.VerifyHash(publicKey, commitPayload) { @@ -362,7 +362,7 @@ func delegatorSlashApply( } if nowAmt.Cmp(common.Big0) == 0 { - // TODO need to remove the undelegate + // TODO(audit): need to remove the undelegate utils.Logger().Info(). RawJSON("delegation-snapshot", []byte(delegationSnapshot.String())). RawJSON("delegation-current", []byte(delegationNow.String())).