Avoid mutating commit bitmap during certificate comparison

consensus/commit_bitmap.go

@@ -0,0 +1,55 @@
+package consensus
+
+import (
+	"math/bits"
+
+	"github.com/harmony-one/harmony/crypto/bls"
+)
+
+// isMoreCompleteCommitPayload reports whether candidate has strictly more
+// participating committee slots than current. Both payloads must use the same
+// canonical signature-and-bitmap encoding; equal signer counts keep current.
+func isMoreCompleteCommitPayload(current, candidate []byte, participantCount int) bool {
+	if !hasCanonicalCommitBitmap(current, participantCount) ||
+		!hasCanonicalCommitBitmap(candidate, participantCount) {
+		return false
+	}
+	return commitPayloadSignerCount(candidate, participantCount) >
+		commitPayloadSignerCount(current, participantCount)
+}
+
+// hasCanonicalCommitBitmap validates only the payload's structural encoding:
+// its length must match one BLS signature followed by one bit per committee
+// slot, and any unused high bits in the last bitmap byte must be zero. It does
+// not verify the BLS signature or weighted quorum.
+func hasCanonicalCommitBitmap(payload []byte, participantCount int) bool {
+	bitmapLen := (participantCount + 7) / 8
+	if participantCount <= 0 || len(payload) != bls.BLSSignatureSizeInBytes+bitmapLen {
+		return false
+	}
+	if remainingBits := participantCount % 8; remainingBits != 0 {
+		validBits := byte(1<<remainingBits) - 1
+		bitmap := payload[bls.BLSSignatureSizeInBytes:]
+		if bitmap[len(bitmap)-1]&^validBits != 0 {
+			return false
+		}
+	}
+	return true
+}
+
+// commitPayloadSignerCount counts enabled committee slots while ignoring the
+// unused high bits in the last bitmap byte. The caller must first validate the
+// payload with hasCanonicalCommitBitmap.
+func commitPayloadSignerCount(payload []byte, participantCount int) int {
+	bitmap := payload[bls.BLSSignatureSizeInBytes:]
+	fullBytes := participantCount / 8
+	count := 0
+	for _, bitmapByte := range bitmap[:fullBytes] {
+		count += bits.OnesCount8(bitmapByte)
+	}
+	if remainingBits := participantCount % 8; remainingBits != 0 {
+		validBits := byte(1<<remainingBits) - 1
+		count += bits.OnesCount8(bitmap[fullBytes] & validBits)
+	}
+	return count
+}

consensus/commit_bitmap_test.go

@@ -0,0 +1,63 @@
+package consensus
+
+import (
+	"testing"
+
+	"github.com/harmony-one/harmony/crypto/bls"
+)
+
+func testCommitPayload(bitmap ...byte) []byte {
+	payload := make([]byte, bls.BLSSignatureSizeInBytes+len(bitmap))
+	copy(payload[bls.BLSSignatureSizeInBytes:], bitmap)
+	return payload
+}
+
+func TestIsMoreCompleteCommitPayload(t *testing.T) {
+	tests := []struct {
+		name             string
+		current          []byte
+		candidate        []byte
+		participantCount int
+		want             bool
+	}{
+		{
+			name:             "more signers wins",
+			current:          testCommitPayload(0b00000111),
+			candidate:        testCommitPayload(0b00001111),
+			participantCount: 8,
+			want:             true,
+		},
+		{
+			name:             "fewer signers cannot downgrade",
+			current:          testCommitPayload(0b00001111),
+			candidate:        testCommitPayload(0b00000111),
+			participantCount: 8,
+		},
+		{
+			name:             "equal signer count keeps current",
+			current:          testCommitPayload(0b00001111),
+			candidate:        testCommitPayload(0b11110000),
+			participantCount: 8,
+		},
+		{
+			name:             "different bitmap size is incompatible",
+			current:          testCommitPayload(0b00000001),
+			candidate:        testCommitPayload(0b11111111, 0b00000001),
+			participantCount: 8,
+		},
+		{
+			name:             "noncanonical padding is rejected",
+			current:          testCommitPayload(0b00000011, 0),
+			candidate:        testCommitPayload(0b00000111, 0b11111110),
+			participantCount: 9,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := isMoreCompleteCommitPayload(tt.current, tt.candidate, tt.participantCount); got != tt.want {
+				t.Fatalf("isMoreCompleteCommitPayload() = %t, want %t", got, tt.want)
+			}
+		})
+	}
+}

consensus/validator.go

@@ -358,7 +358,7 @@ 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.
+	// Compare against the verified incoming COMMITTED bitmap.
 	consensus.checkOwnCommitInclusion(recvMsg.BlockNum, recvMsg.BlockHash, mask)
 	consensus.fBFTLog.AddVerifiedMessage(recvMsg)
 	consensus.aggregatedCommitSig = aggSig
@@ -371,13 +371,11 @@ func (consensus *Consensus) onCommitted(recvMsg *FBFTMessage) {
 	// Need to check whether this block actually was committed, because it could be another block
 	// with the same number that's committed and overriding its commit sigBytes is wrong.
 	blk := consensus.Blockchain().GetBlockByHash(blockObj.Hash())
-	if err == nil && len(commitSigBitmap) == len(recvMsg.Payload) && blk != nil {
-		new := mask.CountEnabled()
-		mask.SetMask(commitSigBitmap[bls.BLSSignatureSizeInBytes:])
-		cur := mask.CountEnabled()
-		if new > cur {
-			consensus.getLogger().Info().Hex("old", commitSigBitmap).Hex("new", recvMsg.Payload).Msg("[OnCommitted] Overriding commit signatures!!")
-			consensus.Blockchain().WriteCommitSig(blockObj.NumberU64(), recvMsg.Payload)
+	participantCount := len(consensus.decider().Participants())
+	if err == nil && blk != nil && isMoreCompleteCommitPayload(commitSigBitmap, recvMsg.Payload, participantCount) {
+		consensus.getLogger().Info().Hex("old", commitSigBitmap).Hex("new", recvMsg.Payload).Msg("[OnCommitted] Overriding commit signatures!!")
+		if err := consensus.Blockchain().WriteCommitSig(blockObj.NumberU64(), recvMsg.Payload); err != nil {
+			consensus.getLogger().Warn().Err(err).Msg("[OnCommitted] failed writing richer commit sig")
 		}
 	}