refactor: add IsValidKeyID to CommonMessageSignatureProofScheme

This circumvents tracking a dedicated nil proof for key ID verification.
It seems reasonable to push this reponsibility to the scheme.
Non-aggregating schemes will have some way of representing the ID by a
singular index, while aggregating schemes will likely do some kind of
verification that the ID is following scheme-specific aggregation rules,
such as needing to fill leftmost first, needing to fill pairwise in a
binary tree shape, or so on.

Next up is removing all notions of the nil proof being required.

gcrypto/commonmessagesignatureproof.go

@@ -118,11 +118,16 @@ type SparseSignature struct {
 	Sig []byte
 }
 
-// CommonMessageSignatureProofScheme indicates how to create and restore
+// CommonMessageSignatureProofScheme indicates how to create
 // CommonMessageSignatureProof instances.
+//
+// It also contains methods that have no relation to a particular proof instance.
 type CommonMessageSignatureProofScheme interface {
 	// New creates a new, empty proof.
 	New(msg []byte, candidateKeys []PubKey, pubKeyHash string) (CommonMessageSignatureProof, error)
+
+	// IsValidKeyID reports whether the given ID is valid given the set of public keys.
+	IsValidKeyID(id []byte, keys []PubKey) bool
 }
 
 // LiteralCommonMessageSignatureProofScheme returns a CommonMessageSignatureProofScheme
@@ -133,18 +138,26 @@ type CommonMessageSignatureProofScheme interface {
 // without writing extra boilerplate to produce a corresponding scheme.
 func LiteralCommonMessageSignatureProofScheme[P CommonMessageSignatureProof](
 	newFn func([]byte, []PubKey, string) (P, error),
+	isValidKeyIDFn func([]byte, []PubKey) bool,
 ) CommonMessageSignatureProofScheme {
 	return literalCommonMessageSignatureProofScheme{
 		newFn: func(msg []byte, candidateKeys []PubKey, pubKeyHash string) (CommonMessageSignatureProof, error) {
 			return newFn(msg, candidateKeys, pubKeyHash)
 		},
+		isValidKeyIDFn: isValidKeyIDFn,
 	}
 }
 
 type literalCommonMessageSignatureProofScheme struct {
 	newFn func([]byte, []PubKey, string) (CommonMessageSignatureProof, error)
+
+	isValidKeyIDFn func([]byte, []PubKey) bool
 }
 
 func (s literalCommonMessageSignatureProofScheme) New(msg []byte, candidateKeys []PubKey, pubKeyHash string) (CommonMessageSignatureProof, error) {
 	return s.newFn(msg, candidateKeys, pubKeyHash)
 }
+
+func (s literalCommonMessageSignatureProofScheme) IsValidKeyID(id []byte, keys []PubKey) bool {
+	return s.isValidKeyIDFn(id, keys)
+}

gcrypto/simplecommonmessagesignatureproof.go

@@ -13,6 +13,7 @@ import (
 // SimpleCommonMessageSignatureProofScheme is the scheme for a SimpleCommonMessageSignatureProof.
 var SimpleCommonMessageSignatureProofScheme CommonMessageSignatureProofScheme = LiteralCommonMessageSignatureProofScheme(
 	NewSimpleCommonMessageSignatureProof,
+	isValidSimpleCommonSignatureKeyID,
 )
 
 // SimpleCommonMessageSignatureProof is the simplest signature proof,
@@ -266,3 +267,20 @@ func (p SimpleCommonMessageSignatureProof) HasSparseKeyID(keyID []byte) (has, va
 	has = p.bitset.Test(uint(u))
 	return has, true
 }
+
+func isValidSimpleCommonSignatureKeyID(id []byte, keys []PubKey) bool {
+	if len(id) != 2 {
+		// Invalid because the key IDs must be a big endian uint16.
+		return false
+	}
+
+	u := binary.BigEndian.Uint16(id)
+	idx := int(u)
+	if idx < 0 || idx >= len(keys) {
+		// Key ID must be in range to be valid.
+		return false
+	}
+
+	// ID is in range so it is valid.
+	return true
+}

tm/tmconsensus/precommit.go

@@ -41,7 +41,7 @@ func (p PrecommitProof) AsSparse() (PrecommitSparseProof, error) {
 	return out, nil
 }
 
-// PrecommitSparseProof is the sparse proof representation for precommits for v0.2.
+// PrecommitSparseProof is the representation of sparse proofs for precommits arriving across the network.
 // It is currently identical to PrevoteSparseProof, but that may change with vote extensions.
 type PrecommitSparseProof struct {
 	Height uint64

tm/tmconsensus/prevote.go

@@ -41,7 +41,7 @@ func (p PrevoteProof) AsSparse() (PrevoteSparseProof, error) {
 	return out, nil
 }
 
-// PrevoteSparseProof is the sparse proof representation for prevotes for v0.2.
+// PrevoteSparseProof is the representation of sparse proofs for prevotes arriving across the network.
 type PrevoteSparseProof struct {
 	Height uint64
 	Round  uint32

tm/tmengine/internal/tmmirror/mirror.go

@@ -429,7 +429,7 @@ RETRY:
 	}
 
 	curProofs := curPrevoteState.PrevoteProofs
-	sigsToAdd := m.getSignaturesToAdd(curProofs, p.Proofs)
+	sigsToAdd := m.getSignaturesToAdd(curProofs, p.Proofs, vlReq.VRV.ValidatorSet)
 
 	if len(sigsToAdd) == 0 {
 		// Maybe the message had some valid signatures.
@@ -594,7 +594,7 @@ RETRY:
 	}
 
 	curProofs := curPrecommitState.PrecommitProofs
-	sigsToAdd := m.getSignaturesToAdd(curProofs, p.Proofs)
+	sigsToAdd := m.getSignaturesToAdd(curProofs, p.Proofs, vlReq.VRV.ValidatorSet)
 
 	if len(sigsToAdd) == 0 {
 		// Maybe the message had some valid signatures.
@@ -702,17 +702,26 @@ RETRY:
 func (m *Mirror) getSignaturesToAdd(
 	curProofs map[string]gcrypto.CommonMessageSignatureProof,
 	incomingSparseProofs map[string][]gcrypto.SparseSignature,
+	valSet tmconsensus.ValidatorSet,
 ) map[string][]gcrypto.SparseSignature {
-	// The Mirror always prepopulates the nil block signature proof.
-	// And we need it for a fallback if we see a signature for an unknown block,
-	// to confirm valid signature key IDs.
-	nilProof := curProofs[""]
-
 	var toAdd map[string][]gcrypto.SparseSignature
 
+	var pubKeys []gcrypto.PubKey
+
 	for blockHash, signatures := range incomingSparseProofs {
 		fullProof := curProofs[blockHash]
-		needToAdd := m.getNewSignatures(nilProof, fullProof, signatures)
+		if fullProof == nil && pubKeys == nil {
+			// We only consult pubKeys when fullProof is nil,
+			// and in a properly behaving honest network,
+			// fullProof will usually not be nil.
+			//
+			// This is going to allocate a bit every time we call it;
+			// possible optimization to actually only store these public keys once per validator set
+			// (even though we still limit it to at most one allocation
+			// per call to HandlePrevoteProofs or HandlePrecommitProofs).
+			pubKeys = tmconsensus.ValidatorsToPubKeys(valSet.Validators)
+		}
+		needToAdd := m.getNewSignatures(fullProof, signatures, pubKeys)
 		if len(needToAdd) == 0 {
 			// We already had those signatures.
 			continue
@@ -730,22 +739,19 @@ func (m *Mirror) getSignaturesToAdd(
 
 // getNewSignatures filters incomingProofs against the given fullProof,
 // returning only the signatures whose key ID is not present in fullProof.
-// The nilProof argument is assumed to always be available,
-// and is used as a fallback to check if the key IDs are valid,
-// in the event fullProof is nil.
-//
-// NOTE: this could potentially change to a standalone function instead of a method.
+// pubKeys is only used to determine if the incoming key IDs are valid,
+// only when fullProof is nil.
 func (m *Mirror) getNewSignatures(
-	nilProof gcrypto.CommonMessageSignatureProof,
 	fullProof gcrypto.CommonMessageSignatureProof,
 	incomingProofs []gcrypto.SparseSignature,
+	pubKeys []gcrypto.PubKey,
 ) []gcrypto.SparseSignature {
 	out := make([]gcrypto.SparseSignature, 0, len(incomingProofs))
 
 	if fullProof == nil {
-		// Falling back to only checking key ID validity via nilProof.
+		// Falling back to only checking key ID validity via the scheme.
 		for _, p := range incomingProofs {
-			if _, valid := nilProof.HasSparseKeyID(p.KeyID); !valid {
+			if valid := m.cmspScheme.IsValidKeyID(p.KeyID, pubKeys); !valid {
 				continue
 			}