Blob verification spectest (#13707)

* use real blob verifier in forkchoice spectest

* wip

* Use real blob sidecar for test

* Set file db correctly

* correctly handle blob cases where valid=false

* work-around spectest's weird Fork in genesis state

* gaz

* revert T-money's log level change

* rm whitespace

* unskip minimal test

* Preston's feedback

---------

Co-authored-by: Kasey Kirkham <kasey@users.noreply.github.com>
Co-authored-by: terence tsao <terence@prysmaticlabs.com>

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 allSidecarRequirements = []Requirement{
 	RequireBlobIndexInBounds,
 	RequireNotFromFutureSlot,
 	RequireSlotAboveFinalized,
@@ -45,26 +43,32 @@ var GossipSidecarRequirements = []Requirement{
 	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 = requirementList(allSidecarRequirements).excluding()
+
+// SpectestSidecarRequirements is used by the forkchoice spectests when verifying blobs used in the on_block tests.
+// The only requirements we exclude for these tests 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, it is safe to skip the following verifications.
-// RequireSidecarProposerExpected
-// RequireNotFromFutureSlot,
-// RequireSlotAboveFinalized,
-// RequireSidecarParentSeen,
-// RequireSidecarParentValid,
-// RequireSidecarParentSlotLower,
-// RequireSidecarDescendsFromFinalized,
-var InitsyncSidecarRequirements = []Requirement{
-	RequireValidProposerSignature,
-	RequireSidecarKzgProofVerified,
-	RequireBlobIndexInBounds,
-	RequireSidecarInclusionProven,
-}
+// block root, the list of required verifications is much shorter than gossip.
+var InitsyncSidecarRequirements = requirementList(GossipSidecarRequirements).excluding(
+	RequireNotFromFutureSlot,
+	RequireSlotAboveFinalized,
+	RequireSidecarParentSeen,
+	RequireSidecarParentValid,
+	RequireSidecarParentSlotLower,
+	RequireSidecarDescendsFromFinalized,
+	RequireSidecarProposerExpected,
+)
 
-// 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/cache.go

@@ -61,13 +61,17 @@ func (d SignatureData) logFields() log.Fields {
 	}
 }
 
-func newSigCache(vr []byte, size int) *sigCache {
-	return &sigCache{Cache: lruwrpr.New(size), valRoot: vr}
+func newSigCache(vr []byte, size int, gf forkLookup) *sigCache {
+	if gf == nil {
+		gf = forks.Fork
+	}
+	return &sigCache{Cache: lruwrpr.New(size), valRoot: vr, getFork: gf}
 }
 
 type sigCache struct {
 	*lru.Cache
 	valRoot []byte
+	getFork forkLookup
 }
 
 // VerifySignature verifies the given signature data against the key obtained via ValidatorAtIndexer.
@@ -81,7 +85,7 @@ func (c *sigCache) VerifySignature(sig SignatureData, v ValidatorAtIndexer) (err
 		}
 	}()
 	e := slots.ToEpoch(sig.Slot)
-	fork, err := forks.Fork(e)
+	fork, err := c.getFork(e)
 	if err != nil {
 		return err
 	}

beacon-chain/verification/cache_test.go

@@ -27,7 +27,7 @@ func TestVerifySignature(t *testing.T) {
 	_, blobs, _, pk := testSignedBlockBlobKeys(t, valRoot[:], 0, 1)
 	b := blobs[0]
 
-	sc := newSigCache(valRoot[:], 1)
+	sc := newSigCache(valRoot[:], 1, nil)
 	cb := func(idx primitives.ValidatorIndex) (*eth.Validator, error) {
 		return &eth.Validator{PublicKey: pk.Marshal()}, nil
 	}
@@ -42,7 +42,7 @@ func TestSignatureCacheMissThenHit(t *testing.T) {
 	_, blobs, _, pk := testSignedBlockBlobKeys(t, valRoot[:], 0, 1)
 	b := blobs[0]
 
-	sc := newSigCache(valRoot[:], 1)
+	sc := newSigCache(valRoot[:], 1, nil)
 	cb := func(idx primitives.ValidatorIndex) (*eth.Validator, error) {
 		return &eth.Validator{PublicKey: pk.Marshal()}, nil
 	}

beacon-chain/verification/error.go

@@ -13,11 +13,17 @@ type VerificationMultiError struct {
 
 // Unwrap is used by errors.Is to unwrap errors.
 func (ve VerificationMultiError) Unwrap() error {
+	if ve.err == nil {
+		return nil
+	}
 	return ve.err
 }
 
 // Error satisfies the standard error interface.
 func (ve VerificationMultiError) Error() string {
+	if ve.err == nil {
+		return ""
+	}
 	return ve.err.Error()
 }
 

beacon-chain/verification/initializer.go

@@ -10,6 +10,8 @@ import (
 	"github.com/prysmaticlabs/prysm/v5/beacon-chain/state"
 	"github.com/prysmaticlabs/prysm/v5/consensus-types/blocks"
 	"github.com/prysmaticlabs/prysm/v5/consensus-types/primitives"
+	"github.com/prysmaticlabs/prysm/v5/network/forks"
+	ethpb "github.com/prysmaticlabs/prysm/v5/proto/prysm/v1alpha1"
 )
 
 // Forkchoicer represents the forkchoice methods that the verifiers need.
@@ -59,21 +61,40 @@ func (ini *Initializer) NewBlobVerifier(b blocks.ROBlob, reqs []Requirement) *RO
 // via the WaitForInitializer method.
 type InitializerWaiter struct {
 	sync.RWMutex
-	ready bool
-	cw    startup.ClockWaiter
-	ini   *Initializer
+	ready   bool
+	cw      startup.ClockWaiter
+	ini     *Initializer
+	getFork forkLookup
+}
+
+type forkLookup func(targetEpoch primitives.Epoch) (*ethpb.Fork, error)
+
+type InitializerOption func(waiter *InitializerWaiter)
+
+// WithForkLookup allows tests to modify how Fork consensus type lookup works. Needed for spectests with weird Forks.
+func WithForkLookup(fl forkLookup) InitializerOption {
+	return func(iw *InitializerWaiter) {
+		iw.getFork = fl
+	}
 }
 
 // NewInitializerWaiter creates an InitializerWaiter which can be used to obtain an Initializer once async dependencies are ready.
-func NewInitializerWaiter(cw startup.ClockWaiter, fc Forkchoicer, sr StateByRooter) *InitializerWaiter {
+func NewInitializerWaiter(cw startup.ClockWaiter, fc Forkchoicer, sr StateByRooter, opts ...InitializerOption) *InitializerWaiter {
 	pc := newPropCache()
 	// signature cache is initialized in WaitForInitializer, since we need the genesis validators root, which can be obtained from startup.Clock.
 	shared := &sharedResources{
 		fc: fc,
 		pc: pc,
 		sr: sr,
 	}
-	return &InitializerWaiter{cw: cw, ini: &Initializer{shared: shared}}
+	iw := &InitializerWaiter{cw: cw, ini: &Initializer{shared: shared}}
+	for _, o := range opts {
+		o(iw)
+	}
+	if iw.getFork == nil {
+		iw.getFork = forks.Fork
+	}
+	return iw
 }
 
 // WaitForInitializer ensures that asynchronous initialization of the shared resources the initializer
@@ -84,7 +105,7 @@ func (w *InitializerWaiter) WaitForInitializer(ctx context.Context) (*Initialize
 	}
 	// We wait until this point to initialize the signature cache because here we have access to the genesis validator root.
 	vr := w.ini.shared.clock.GenesisValidatorsRoot()
-	sc := newSigCache(vr[:], DefaultSignatureCacheSize)
+	sc := newSigCache(vr[:], DefaultSignatureCacheSize, w.getFork)
 	w.ini.shared.sc = sc
 	return w.ini, nil
 }

beacon-chain/verification/result.go

@@ -3,6 +3,54 @@ 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:
+		return "RequireBlobIndexInBounds"
+	case RequireNotFromFutureSlot:
+		return "RequireNotFromFutureSlot"
+	case RequireSlotAboveFinalized:
+		return "RequireSlotAboveFinalized"
+	case RequireValidProposerSignature:
+		return "RequireValidProposerSignature"
+	case RequireSidecarParentSeen:
+		return "RequireSidecarParentSeen"
+	case RequireSidecarParentValid:
+		return "RequireSidecarParentValid"
+	case RequireSidecarParentSlotLower:
+		return "RequireSidecarParentSlotLower"
+	case RequireSidecarDescendsFromFinalized:
+		return "RequireSidecarDescendsFromFinalized"
+	case RequireSidecarInclusionProven:
+		return "RequireSidecarInclusionProven"
+	case RequireSidecarKzgProofVerified:
+		return "RequireSidecarKzgProofVerified"
+	case RequireSidecarProposerExpected:
+		return "RequireSidecarProposerExpected"
+	default:
+		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.
 // This bitmap can be used to test which verifications have been successfully completed in order to
 // decide whether it is safe to issue a "Verified" type variant.

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, allSidecarRequirements, GossipSidecarRequirements)
+}
+
+func TestAllBlobRequirementsHaveStrings(t *testing.T) {
+	var derp Requirement = math.MaxInt
+	require.Equal(t, unknownRequirementName, derp.String())
+	for i := range allSidecarRequirements {
+		require.NotEqual(t, unknownRequirementName, allSidecarRequirements[i].String())
+	}
+}

testing/spectest/mainnet/deneb/forkchoice/forkchoice_test.go

@@ -8,6 +8,5 @@ import (
 )
 
 func TestMainnet_Deneb_Forkchoice(t *testing.T) {
-	t.Skip("This will fail until we re-integrate proof verification")
 	forkchoice.Run(t, "mainnet", version.Deneb)
 }

testing/spectest/minimal/deneb/forkchoice/forkchoice_test.go

@@ -8,6 +8,5 @@ import (
 )
 
 func TestMinimal_Deneb_Forkchoice(t *testing.T) {
-	t.Skip("blocked by go-kzg-4844 minimal trusted setup")
 	forkchoice.Run(t, "minimal", version.Deneb)
 }

testing/spectest/shared/common/forkchoice/BUILD.bazel

@@ -22,6 +22,7 @@ go_library(
         "//beacon-chain/db/filesystem:go_default_library",
         "//beacon-chain/db/testing:go_default_library",
         "//beacon-chain/execution:go_default_library",
+        "//beacon-chain/forkchoice:go_default_library",
         "//beacon-chain/forkchoice/doubly-linked-tree:go_default_library",
         "//beacon-chain/operations/attestations:go_default_library",
         "//beacon-chain/startup:go_default_library",

testing/spectest/shared/common/forkchoice/builder.go

@@ -10,7 +10,9 @@ import (
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/prysmaticlabs/prysm/v5/beacon-chain/blockchain"
 	"github.com/prysmaticlabs/prysm/v5/beacon-chain/execution"
+	"github.com/prysmaticlabs/prysm/v5/beacon-chain/startup"
 	"github.com/prysmaticlabs/prysm/v5/beacon-chain/state"
+	"github.com/prysmaticlabs/prysm/v5/beacon-chain/verification"
 	"github.com/prysmaticlabs/prysm/v5/config/params"
 	"github.com/prysmaticlabs/prysm/v5/consensus-types/interfaces"
 	"github.com/prysmaticlabs/prysm/v5/consensus-types/primitives"
@@ -23,16 +25,27 @@ type Builder struct {
 	service  *blockchain.Service
 	lastTick int64
 	execMock *engineMock
+	vwait    *verification.InitializerWaiter
 }
 
 func NewBuilder(t testing.TB, initialState state.BeaconState, initialBlock interfaces.ReadOnlySignedBeaconBlock) *Builder {
 	execMock := &engineMock{
 		powBlocks: make(map[[32]byte]*ethpb.PowBlock),
 	}
-	service := startChainService(t, initialState, initialBlock, execMock)
+	cw := startup.NewClockSynchronizer()
+	service, sg, fc := startChainService(t, initialState, initialBlock, execMock, cw)
+	// blob spectests use a weird Fork in the genesis beacon state that has different previous and current versions.
+	// This trips up the lite fork lookup code in the blob verifier that figures out the fork
+	// based on the slot of the block. So just for spectests we override that behavior and get the fork from the state
+	// which matches the behavior of block verification.
+	getFork := func(targetEpoch primitives.Epoch) (*ethpb.Fork, error) {
+		return initialState.Fork(), nil
+	}
+	bvw := verification.NewInitializerWaiter(cw, fc, sg, verification.WithForkLookup(getFork))
 	return &Builder{
 		service:  service,
 		execMock: execMock,
+		vwait:    bvw,
 	}
 }
 
@@ -88,15 +101,15 @@ func (bb *Builder) block(t testing.TB, b interfaces.ReadOnlySignedBeaconBlock) [
 // InvalidBlock receives the invalid block and notifies forkchoice.
 func (bb *Builder) InvalidBlock(t testing.TB, b interfaces.ReadOnlySignedBeaconBlock) {
 	r := bb.block(t, b)
-	ctx, cancel := context.WithTimeout(context.TODO(), time.Duration(params.BeaconConfig().SecondsPerSlot)*time.Second)
+	ctx, cancel := context.WithTimeout(context.TODO(), time.Second)
 	defer cancel()
 	require.Equal(t, true, bb.service.ReceiveBlock(ctx, b, r, nil) != nil)
 }
 
 // ValidBlock receives the valid block and notifies forkchoice.
 func (bb *Builder) ValidBlock(t testing.TB, b interfaces.ReadOnlySignedBeaconBlock) {
 	r := bb.block(t, b)
-	ctx, cancel := context.WithTimeout(context.TODO(), time.Duration(params.BeaconConfig().SecondsPerSlot)*time.Second)
+	ctx, cancel := context.WithTimeout(context.TODO(), time.Second)
 	defer cancel()
 	require.NoError(t, bb.service.ReceiveBlock(ctx, b, r, nil))
 }