Preston's feedback

beacon-chain/verification/BUILD.bazel

@@ -45,6 +45,7 @@ go_test(
         "blob_test.go",
         "cache_test.go",
         "initializer_test.go",
+        "result_test.go",
     ],
     embed = [":go_default_library"],
     deps = [

beacon-chain/verification/blob.go

@@ -29,9 +29,7 @@ const (
 	RequireSidecarProposerExpected
 )
 
-// GossipSidecarRequirements defines the set of requirements that BlobSidecars received on gossip
-// must satisfy in order to upgrade an ROBlob to a VerifiedROBlob.
-var GossipSidecarRequirements = []Requirement{
+var allBlobRequirements = []Requirement{
 	RequireBlobIndexInBounds,
 	RequireNotFromFutureSlot,
 	RequireSlotAboveFinalized,
@@ -47,40 +45,30 @@ var GossipSidecarRequirements = []Requirement{
 
 // GossipSidecarRequirements defines the set of requirements that BlobSidecars received on gossip
 // must satisfy in order to upgrade an ROBlob to a VerifiedROBlob.
-var SpectestSidecarRequirements = []Requirement{
-	RequireBlobIndexInBounds,
+var GossipSidecarRequirements = requirementList(allBlobRequirements).excluding()
+
+// SpectestSidecarRequirements is used by the forkchoice spectests when verifying blobs used in the on_block tests.
+// The only requirements we exclude for these test are the parent validity and seen tests, as these are specific to
+// gossip processing and require the bad block cache that we only use there.
+var SpectestSidecarRequirements = requirementList(GossipSidecarRequirements).excluding(
+	RequireSidecarParentSeen, RequireSidecarParentValid)
+
+// InitsyncSidecarRequirements is the list of verification requirements to be used by the init-sync service
+// for batch-mode syncing. Because we only perform batch verification as part of the IsDataAvailable method
+// for blobs after the block has been verified, and the blobs to be verified are keyed in the cache by the
+// block root, the list of required verifications is much shorter than gossip.
+var InitsyncSidecarRequirements = requirementList(allBlobRequirements).excluding(
 	RequireNotFromFutureSlot,
 	RequireSlotAboveFinalized,
-	RequireValidProposerSignature,
+	RequireSidecarParentSeen,
+	RequireSidecarParentValid,
 	RequireSidecarParentSlotLower,
-	//RequireSidecarParentSeen,
-	//RequireSidecarParentValid,
 	RequireSidecarDescendsFromFinalized,
-	RequireSidecarInclusionProven,
-	RequireSidecarKzgProofVerified,
 	RequireSidecarProposerExpected,
-}
-
-// InitsyncSidecarRequirements is the list of verification requirements to be used by the init-sync service
-// for batch-mode syncing. Because we only perform batch verification as part of the IsDataAvailable method
-// for blobs after the block has been verified, and the blobs to be verified are keyed in the cache by the
-// block root, it is safe to skip the following verifications.
-// RequireSidecarProposerExpected
-// RequireNotFromFutureSlot,
-// RequireSlotAboveFinalized,
-// RequireSidecarParentSeen,
-// RequireSidecarParentValid,
-// RequireSidecarParentSlotLower,
-// RequireSidecarDescendsFromFinalized,
-var InitsyncSidecarRequirements = []Requirement{
-	RequireValidProposerSignature,
-	RequireSidecarKzgProofVerified,
-	RequireBlobIndexInBounds,
-	RequireSidecarInclusionProven,
-}
+)
 
-// BackfillSidecarRequirements is the same as InitsyncSidecarRequirements
-var BackfillSidecarRequirements = InitsyncSidecarRequirements
+// BackfillSidecarRequirements is the same as InitsyncSidecarRequirements.
+var BackfillSidecarRequirements = requirementList(InitsyncSidecarRequirements).excluding()
 
 var (
 	ErrBlobInvalid = errors.New("blob failed verification")

beacon-chain/verification/result.go

@@ -3,6 +3,8 @@ package verification
 // Requirement represents a validation check that needs to pass in order for a Verified form a consensus type to be issued.
 type Requirement int
 
+var unknownRequirementName = "unknown"
+
 func (r Requirement) String() string {
 	switch r {
 	case RequireBlobIndexInBounds:
@@ -28,8 +30,25 @@ func (r Requirement) String() string {
 	case RequireSidecarProposerExpected:
 		return "RequireSidecarProposerExpected"
 	default:
-		return "unknown"
+		return unknownRequirementName
+	}
+}
+
+type requirementList []Requirement
+
+func (rl requirementList) excluding(minus ...Requirement) []Requirement {
+	rm := make(map[Requirement]struct{})
+	nl := make([]Requirement, 0, len(rl)-len(minus))
+	for i := range minus {
+		rm[minus[i]] = struct{}{}
+	}
+	for i := range rl {
+		if _, excluded := rm[rl[i]]; excluded {
+			continue
+		}
+		nl = append(nl, rl[i])
 	}
+	return nl
 }
 
 // results collects positive verification results.

beacon-chain/verification/result_test.go

@@ -0,0 +1,63 @@
+package verification
+
+import (
+	"math"
+	"testing"
+
+	"github.com/prysmaticlabs/prysm/v5/testing/require"
+)
+
+func TestResultList(t *testing.T) {
+	const (
+		a Requirement = iota
+		b
+		c
+		d
+		e
+		f
+		g
+		h
+	)
+	// leave out h to test excluding non-existent item
+	all := []Requirement{a, b, c, d, e, f, g}
+	alsoAll := requirementList(all).excluding()
+	require.DeepEqual(t, all, alsoAll)
+	missingFirst := requirementList(all).excluding(a)
+	require.Equal(t, len(all)-1, len(missingFirst))
+	require.DeepEqual(t, all[1:], missingFirst)
+	missingLast := requirementList(all).excluding(g)
+	require.Equal(t, len(all)-1, len(missingLast))
+	require.DeepEqual(t, all[0:len(all)-1], missingLast)
+	missingEnds := requirementList(missingLast).excluding(a)
+	require.Equal(t, len(missingLast)-1, len(missingEnds))
+	require.DeepEqual(t, all[1:len(all)-1], missingEnds)
+	excludeNonexist := requirementList(missingEnds).excluding(h)
+	require.Equal(t, len(missingEnds), len(excludeNonexist))
+	require.DeepEqual(t, missingEnds, excludeNonexist)
+}
+
+func TestExportedBlobSanityCheck(t *testing.T) {
+	// make sure all requirement lists contain the bare minimum checks
+	sanity := []Requirement{RequireValidProposerSignature, RequireSidecarKzgProofVerified, RequireBlobIndexInBounds, RequireSidecarInclusionProven}
+	reqs := [][]Requirement{GossipSidecarRequirements, SpectestSidecarRequirements, InitsyncSidecarRequirements, BackfillSidecarRequirements}
+	for i := range reqs {
+		r := reqs[i]
+		reqMap := make(map[Requirement]struct{})
+		for ii := range r {
+			reqMap[r[ii]] = struct{}{}
+		}
+		for ii := range sanity {
+			_, ok := reqMap[sanity[ii]]
+			require.Equal(t, true, ok)
+		}
+	}
+	require.DeepEqual(t, allBlobRequirements, GossipSidecarRequirements)
+}
+
+func TestAllBlobRequirementsHaveStrings(t *testing.T) {
+	var derp Requirement = math.MaxInt
+	require.Equal(t, unknownRequirementName, derp.String())
+	for i := range allBlobRequirements {
+		require.NotEqual(t, unknownRequirementName, allBlobRequirements[i].String())
+	}
+}