diff --git a/consensus/checks.go b/consensus/checks.go index d927653bf4..ebd5c49f50 100644 --- a/consensus/checks.go +++ b/consensus/checks.go @@ -58,6 +58,7 @@ func (consensus *Consensus) senderKeySanityChecks(msg *msg_pb.Message, senderKey func (consensus *Consensus) isRightBlockNumAndViewID(recvMsg *FBFTMessage) bool { blockNum := consensus.getBlockNum() if recvMsg.ViewID != consensus.getCurBlockViewID() || recvMsg.BlockNum != blockNum { + consensus.reportLateVoteIfPastFinalized(recvMsg, blockNum) consensus.getLogger().Debug(). Uint64("blockNum", blockNum). Str("recvMsg", recvMsg.String()). diff --git a/consensus/consensus.go b/consensus/consensus.go index 251f5acdc1..b03dbf47d9 100644 --- a/consensus/consensus.go +++ b/consensus/consensus.go @@ -136,6 +136,10 @@ type Consensus struct { lastKnownSignPower int64 lastKnowViewChange int64 + // last COMMIT this node successfully broadcast + lastCommitSentBlockNum uint64 + lastCommitSentHash atomic.Value // common.Hash + transitions struct { finalCommit bool } diff --git a/consensus/late_signature.go b/consensus/late_signature.go new file mode 100644 index 0000000000..796f0824c1 --- /dev/null +++ b/consensus/late_signature.go @@ -0,0 +1,93 @@ +package consensus + +import ( + "sync/atomic" + + "github.com/ethereum/go-ethereum/common" + "github.com/harmony-one/harmony/crypto/bls" + "github.com/prometheus/client_golang/prometheus" +) + +// recordLastCommitSent stores the block number and hash of the last COMMIT +// this node broadcast successfully. +func (consensus *Consensus) recordLastCommitSent(blockNum uint64, blockHash common.Hash) { + atomic.StoreUint64(&consensus.lastCommitSentBlockNum, blockNum) + consensus.lastCommitSentHash.Store(blockHash) +} + +// checkOwnCommitInclusion logs and counts when a local COMMIT key is absent +// from the final COMMITTED bitmap for a block this node previously signed. +func (consensus *Consensus) checkOwnCommitInclusion(blockNum uint64, blockHash common.Hash, mask *bls.Mask) { + if mask == nil { + return + } + if atomic.LoadUint64(&consensus.lastCommitSentBlockNum) != blockNum { + return + } + v := consensus.lastCommitSentHash.Load() + if v == nil { + return + } + sentHash, ok := v.(common.Hash) + if !ok || sentHash != blockHash { + return + } + + priKeys, err := consensus.getPriKeysInCommittee() + if err != nil || len(priKeys) == 0 { + return + } + localPubs := make([]bls.SerializedPublicKey, 0, len(priKeys)) + for _, key := range priKeys { + localPubs = append(localPubs, key.Pub.Bytes) + } + + for _, pub := range excludedLocalCommitKeys(mask, localPubs) { + consensus.getLogger().Warn(). + Uint64("blockNum", blockNum). + Str("blockHash", blockHash.Hex()). + Str("blsPubKey", pub.Hex()). + Msg("[OnCommitted] local commit signature not included in final commit bitmap") + consensusLateSignatureCounterVec.With(prometheus.Labels{ + "role": "validator", + "phase": "committed", + }).Inc() + } +} + +// excludedLocalCommitKeys returns local committee keys that are present in the +// participant set but disabled in the commit bitmap. +func excludedLocalCommitKeys(mask *bls.Mask, localPubs []bls.SerializedPublicKey) []bls.SerializedPublicKey { + excluded := make([]bls.SerializedPublicKey, 0) + for _, pub := range localPubs { + ok, err := mask.KeyEnabled(pub) + if err != nil { + continue + } + if !ok { + excluded = append(excluded, pub) + } + } + return excluded +} + +// reportLateVoteIfPastFinalized logs and counts a prepare/commit vote whose +// block number is exactly one behind the leader's current block number. +func (consensus *Consensus) reportLateVoteIfPastFinalized(recvMsg *FBFTMessage, myBlockNum uint64) { + if recvMsg == nil || recvMsg.BlockNum+1 != myBlockNum { + return + } + phase := recvMsg.MessageType.String() + consensusLateSignatureCounterVec.With(prometheus.Labels{ + "role": "leader", + "phase": phase, + }).Inc() + + consensus.getLogger().Info(). + Uint64("msgBlockNum", recvMsg.BlockNum). + Uint64("myBlockNum", myBlockNum). + Uint64("msgViewID", recvMsg.ViewID). + Str("phase", phase). + Str("recvMsg", recvMsg.String()). + Msg("[Consensus] late vote received after block finalized") +} diff --git a/consensus/late_signature_test.go b/consensus/late_signature_test.go new file mode 100644 index 0000000000..6c88dde077 --- /dev/null +++ b/consensus/late_signature_test.go @@ -0,0 +1,92 @@ +package consensus + +import ( + "testing" + + "github.com/ethereum/go-ethereum/common" + msg_pb "github.com/harmony-one/harmony/api/proto/message" + "github.com/harmony-one/harmony/crypto/bls" + dto "github.com/prometheus/client_model/go" + "github.com/stretchr/testify/require" +) + +// TestExcludedLocalCommitKeys checks bitmap exclusion for local committee keys. +func TestExcludedLocalCommitKeys(t *testing.T) { + pub1 := bls.PublicKeyWrapper{Object: bls.RandPrivateKey().GetPublicKey()} + pub2 := bls.PublicKeyWrapper{Object: bls.RandPrivateKey().GetPublicKey()} + pub3 := bls.PublicKeyWrapper{Object: bls.RandPrivateKey().GetPublicKey()} + pub1.Bytes.FromLibBLSPublicKey(pub1.Object) + pub2.Bytes.FromLibBLSPublicKey(pub2.Object) + pub3.Bytes.FromLibBLSPublicKey(pub3.Object) + + mask := bls.NewMask([]bls.PublicKeyWrapper{pub1, pub2, pub3}) + require.NoError(t, mask.SetKey(pub1.Bytes, true)) + require.NoError(t, mask.SetKey(pub3.Bytes, true)) + + excluded := excludedLocalCommitKeys(mask, []bls.SerializedPublicKey{pub1.Bytes, pub2.Bytes}) + require.Equal(t, []bls.SerializedPublicKey{pub2.Bytes}, excluded) + + excluded = excludedLocalCommitKeys(mask, []bls.SerializedPublicKey{pub1.Bytes}) + require.Empty(t, excluded) + + // Key not in participants is ignored (err from KeyEnabled). + outsider := bls.PublicKeyWrapper{Object: bls.RandPrivateKey().GetPublicKey()} + outsider.Bytes.FromLibBLSPublicKey(outsider.Object) + excluded = excludedLocalCommitKeys(mask, []bls.SerializedPublicKey{outsider.Bytes}) + require.Empty(t, excluded) +} + +// TestReportLateVoteIfPastFinalized increments the metric only for the prior block. +func TestReportLateVoteIfPastFinalized(t *testing.T) { + c := &Consensus{current: NewState(Normal, 0)} + initMetrics() + + recvMsg := &FBFTMessage{ + MessageType: msg_pb.MessageType_COMMIT, + BlockNum: 10, + ViewID: 1, + } + // Not the immediately previous block — no-op. + c.reportLateVoteIfPastFinalized(recvMsg, 12) + c.reportLateVoteIfPastFinalized(nil, 11) + + before := lateSignatureCount(t, "leader", msg_pb.MessageType_COMMIT.String()) + c.reportLateVoteIfPastFinalized(recvMsg, 11) + after := lateSignatureCount(t, "leader", msg_pb.MessageType_COMMIT.String()) + require.Equal(t, before+1, after) +} + +// TestRecordLastCommitSentGatesInclusionCheck skips checks without a matching sent COMMIT. +func TestRecordLastCommitSentGatesInclusionCheck(t *testing.T) { + pub1 := bls.PublicKeyWrapper{Object: bls.RandPrivateKey().GetPublicKey()} + pub2 := bls.PublicKeyWrapper{Object: bls.RandPrivateKey().GetPublicKey()} + pub1.Bytes.FromLibBLSPublicKey(pub1.Object) + pub2.Bytes.FromLibBLSPublicKey(pub2.Object) + + mask := bls.NewMask([]bls.PublicKeyWrapper{pub1, pub2}) + require.NoError(t, mask.SetKey(pub2.Bytes, true)) // pub1 excluded + + hash := common.HexToHash("0xabc") + c := &Consensus{current: NewState(Normal, 0)} + initMetrics() + + // Without a matching last-sent commit, check is a no-op even if key excluded. + c.checkOwnCommitInclusion(5, hash, mask) + + c.recordLastCommitSent(5, hash) + // Still no-op: getPriKeysInCommittee fails with empty priKey. + c.checkOwnCommitInclusion(5, hash, mask) + + // Wrong hash — no-op. + c.recordLastCommitSent(5, common.HexToHash("0xdef")) + c.checkOwnCommitInclusion(5, hash, mask) +} + +func lateSignatureCount(t *testing.T, role, phase string) float64 { + t.Helper() + metric, err := consensusLateSignatureCounterVec.GetMetricWithLabelValues(role, phase) + require.NoError(t, err) + var m dto.Metric + require.NoError(t, metric.Write(&m)) + return m.GetCounter().GetValue() +} diff --git a/consensus/metrics.go b/consensus/metrics.go index fa460f4b7e..1f8ba2758d 100644 --- a/consensus/metrics.go +++ b/consensus/metrics.go @@ -127,6 +127,20 @@ var ( Buckets: prometheus.ExponentialBuckets(800, 1.25, 10), }, ) + // consensusLateSignatureCounterVec counts late prepare/commit votes and + // local COMMIT keys missing from the COMMITTED bitmap. + consensusLateSignatureCounterVec = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Namespace: "hmy", + Subsystem: "consensus", + Name: "late_signature", + Help: "votes arriving after finalize, or local commits missing from the COMMITTED bitmap", + }, + []string{ + "role", + "phase", + }, + ) onceMetrics sync.Once @@ -188,6 +202,7 @@ func initMetrics() { consensusGaugeVec, consensusPubkeyVec, consensusFinalityHistogram, + consensusLateSignatureCounterVec, lastPreimageImportGauge, preimageEndGauge, preimageStartGauge, diff --git a/consensus/validator.go b/consensus/validator.go index 874edb0ef8..6da2d16cf2 100644 --- a/consensus/validator.go +++ b/consensus/validator.go @@ -184,6 +184,7 @@ func (consensus *Consensus) sendCommitMessages(blockObj *types.Block) { if err := consensus.broadcastConsensusP2pMessages(p2pMsgs); err != nil { consensus.getLogger().Warn().Err(err).Msg("[sendCommitMessages] Cannot send commit message!!") } else { + consensus.recordLastCommitSent(blockObj.NumberU64(), blockObj.Hash()) consensus.getLogger().Info(). Uint64("blockNum", consensus.BlockNum()). Hex("blockHash", consensus.current.blockHash[:]). @@ -357,6 +358,8 @@ func (consensus *Consensus) onCommitted(recvMsg *FBFTMessage) { consensus.getLogger().Error().Err(err).Msg("[OnCommitted] readSignatureBitmapPayload failed") return } + // Compare against the COMMITTED bitmap before any later SetMask mutation. + consensus.checkOwnCommitInclusion(recvMsg.BlockNum, recvMsg.BlockHash, mask) consensus.fBFTLog.AddVerifiedMessage(recvMsg) consensus.aggregatedCommitSig = aggSig consensus.commitBitmap = mask