Fix bug from PR 13827 (#13871)

* fix AS cache bug, tighten ro constructors

* additional coverage on AS cache filter

---------

Co-authored-by: Kasey Kirkham <[email protected]>

beacon-chain/das/cache.go

@@ -92,7 +92,7 @@ func (e *cacheEntry) filter(root [32]byte, kc safeCommitmentArray) ([]blocks.ROB
 	if e.diskSummary.AllAvailable(kc.count()) {
 		return nil, nil
 	}
-	scs := make([]blocks.ROBlob, kc.count())
+	scs := make([]blocks.ROBlob, 0, kc.count())
 	for i := uint64(0); i < fieldparams.MaxBlobsPerBlock; i++ {
 		// We already have this blob, we don't need to write it or validate it.
 		if e.diskSummary.HasIndex(i) {
@@ -111,7 +111,7 @@ func (e *cacheEntry) filter(root [32]byte, kc safeCommitmentArray) ([]blocks.ROB
 		if !bytes.Equal(kc[i], e.scs[i].KzgCommitment) {
 			return nil, errors.Wrapf(errCommitmentMismatch, "root=%#x, index=%#x, commitment=%#x, block commitment=%#x", root, i, e.scs[i].KzgCommitment, kc[i])
 		}
-		scs[i] = *e.scs[i]
+		scs = append(scs, *e.scs[i])
 	}
 
 	return scs, nil

beacon-chain/das/cache_test.go

@@ -3,9 +3,14 @@ package das
 import (
 	"testing"
 
+	"github.com/prysmaticlabs/prysm/v5/beacon-chain/db/filesystem"
+	"github.com/prysmaticlabs/prysm/v5/config/params"
+	"github.com/prysmaticlabs/prysm/v5/consensus-types/blocks"
 	"github.com/prysmaticlabs/prysm/v5/consensus-types/primitives"
 	"github.com/prysmaticlabs/prysm/v5/encoding/bytesutil"
 	"github.com/prysmaticlabs/prysm/v5/testing/require"
+	"github.com/prysmaticlabs/prysm/v5/testing/util"
+	"github.com/prysmaticlabs/prysm/v5/time/slots"
 )
 
 func TestCacheEnsureDelete(t *testing.T) {
@@ -23,3 +28,145 @@ func TestCacheEnsureDelete(t *testing.T) {
 	var nilEntry *cacheEntry
 	require.Equal(t, nilEntry, c.entries[k])
 }
+
+type filterTestCaseSetupFunc func(t *testing.T) (*cacheEntry, safeCommitmentArray, []blocks.ROBlob)
+
+func filterTestCaseSetup(slot primitives.Slot, nBlobs int, onDisk []int, numExpected int) filterTestCaseSetupFunc {
+	return func(t *testing.T) (*cacheEntry, safeCommitmentArray, []blocks.ROBlob) {
+		blk, blobs := util.GenerateTestDenebBlockWithSidecar(t, [32]byte{}, slot, nBlobs)
+		commits, err := commitmentsToCheck(blk, blk.Block().Slot())
+		require.NoError(t, err)
+		entry := &cacheEntry{}
+		if len(onDisk) > 0 {
+			od := map[[32]byte][]int{blk.Root(): onDisk}
+			sumz := filesystem.NewMockBlobStorageSummarizer(t, od)
+			sum := sumz.Summary(blk.Root())
+			entry.setDiskSummary(sum)
+		}
+		expected := make([]blocks.ROBlob, 0, nBlobs)
+		for i := 0; i < commits.count(); i++ {
+			if entry.diskSummary.HasIndex(uint64(i)) {
+				continue
+			}
+			// If we aren't telling the cache a blob is on disk, add it to the expected list and stash.
+			expected = append(expected, blobs[i])
+			require.NoError(t, entry.stash(&blobs[i]))
+		}
+		require.Equal(t, numExpected, len(expected))
+		return entry, commits, expected
+	}
+}
+
+func TestFilterDiskSummary(t *testing.T) {
+	denebSlot, err := slots.EpochStart(params.BeaconConfig().DenebForkEpoch)
+	require.NoError(t, err)
+	cases := []struct {
+		name  string
+		setup filterTestCaseSetupFunc
+	}{
+		{
+			name:  "full blobs, all on disk",
+			setup: filterTestCaseSetup(denebSlot, 6, []int{0, 1, 2, 3, 4, 5}, 0),
+		},
+		{
+			name:  "full blobs, first on disk",
+			setup: filterTestCaseSetup(denebSlot, 6, []int{0}, 5),
+		},
+		{
+			name:  "full blobs, middle on disk",
+			setup: filterTestCaseSetup(denebSlot, 6, []int{2}, 5),
+		},
+		{
+			name:  "full blobs, last on disk",
+			setup: filterTestCaseSetup(denebSlot, 6, []int{5}, 5),
+		},
+		{
+			name:  "full blobs, none on disk",
+			setup: filterTestCaseSetup(denebSlot, 6, []int{}, 6),
+		},
+		{
+			name:  "one commitment, on disk",
+			setup: filterTestCaseSetup(denebSlot, 1, []int{0}, 0),
+		},
+		{
+			name:  "one commitment, not on disk",
+			setup: filterTestCaseSetup(denebSlot, 1, []int{}, 1),
+		},
+		{
+			name:  "two commitments, first on disk",
+			setup: filterTestCaseSetup(denebSlot, 2, []int{0}, 1),
+		},
+		{
+			name:  "two commitments, last on disk",
+			setup: filterTestCaseSetup(denebSlot, 2, []int{1}, 1),
+		},
+		{
+			name:  "two commitments, none on disk",
+			setup: filterTestCaseSetup(denebSlot, 2, []int{}, 2),
+		},
+		{
+			name:  "two commitments, all on disk",
+			setup: filterTestCaseSetup(denebSlot, 2, []int{0, 1}, 0),
+		},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			entry, commits, expected := c.setup(t)
+			// first (root) argument doesn't matter, it is just for logs
+			got, err := entry.filter([32]byte{}, commits)
+			require.NoError(t, err)
+			require.Equal(t, len(expected), len(got))
+		})
+	}
+}
+
+func TestFilter(t *testing.T) {
+	denebSlot, err := slots.EpochStart(params.BeaconConfig().DenebForkEpoch)
+	require.NoError(t, err)
+	cases := []struct {
+		name  string
+		setup func(t *testing.T) (*cacheEntry, safeCommitmentArray, []blocks.ROBlob)
+		err   error
+	}{
+		{
+			name: "commitments mismatch - extra sidecar",
+			setup: func(t *testing.T) (*cacheEntry, safeCommitmentArray, []blocks.ROBlob) {
+				entry, commits, expected := filterTestCaseSetup(denebSlot, 6, []int{0, 1}, 4)(t)
+				commits[5] = nil
+				return entry, commits, expected
+			},
+			err: errCommitmentMismatch,
+		},
+		{
+			name: "sidecar missing",
+			setup: func(t *testing.T) (*cacheEntry, safeCommitmentArray, []blocks.ROBlob) {
+				entry, commits, expected := filterTestCaseSetup(denebSlot, 6, []int{0, 1}, 4)(t)
+				entry.scs[5] = nil
+				return entry, commits, expected
+			},
+			err: errMissingSidecar,
+		},
+		{
+			name: "commitments mismatch - different bytes",
+			setup: func(t *testing.T) (*cacheEntry, safeCommitmentArray, []blocks.ROBlob) {
+				entry, commits, expected := filterTestCaseSetup(denebSlot, 6, []int{0, 1}, 4)(t)
+				entry.scs[5].KzgCommitment = []byte("nope")
+				return entry, commits, expected
+			},
+			err: errCommitmentMismatch,
+		},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			entry, commits, expected := c.setup(t)
+			// first (root) argument doesn't matter, it is just for logs
+			got, err := entry.filter([32]byte{}, commits)
+			if c.err != nil {
+				require.ErrorIs(t, err, c.err)
+				return
+			}
+			require.NoError(t, err)
+			require.Equal(t, len(expected), len(got))
+		})
+	}
+}

beacon-chain/sync/rpc_send_request_test.go

@@ -482,7 +482,10 @@ func TestSendRequest_SendBeaconBlocksByRootRequest(t *testing.T) {
 func TestBlobValidatorFromRootReq(t *testing.T) {
 	rootA := bytesutil.PadTo([]byte("valid"), 32)
 	rootB := bytesutil.PadTo([]byte("invalid"), 32)
-	header := &ethpb.SignedBeaconBlockHeader{}
+	header := &ethpb.SignedBeaconBlockHeader{
+		Header:    &ethpb.BeaconBlockHeader{Slot: 0},
+		Signature: make([]byte, fieldparams.BLSSignatureLength),
+	}
 	blobSidecarA0 := util.GenerateTestDenebBlobSidecar(t, bytesutil.ToBytes32(rootA), header, 0, []byte{}, make([][]byte, 0))
 	blobSidecarA1 := util.GenerateTestDenebBlobSidecar(t, bytesutil.ToBytes32(rootA), header, 1, []byte{}, make([][]byte, 0))
 	blobSidecarB0 := util.GenerateTestDenebBlobSidecar(t, bytesutil.ToBytes32(rootB), header, 0, []byte{}, make([][]byte, 0))
@@ -590,7 +593,8 @@ func TestBlobValidatorFromRangeReq(t *testing.T) {
 		t.Run(c.name, func(t *testing.T) {
 			vf := blobValidatorFromRangeReq(c.req)
 			header := &ethpb.SignedBeaconBlockHeader{
-				Header: &ethpb.BeaconBlockHeader{Slot: c.responseSlot},
+				Header:    &ethpb.BeaconBlockHeader{Slot: c.responseSlot},
+				Signature: make([]byte, fieldparams.BLSSignatureLength),
 			}
 			sc := util.GenerateTestDenebBlobSidecar(t, [32]byte{}, header, 0, []byte{}, make([][]byte, 0))
 			err := vf(sc)

beacon-chain/verification/batch.go

@@ -46,10 +46,12 @@ func (batch *BlobBatchVerifier) VerifiedROBlobs(ctx context.Context, blk blocks.
 	if len(scs) == 0 {
 		return nil, nil
 	}
+	blkSig := blk.Signature()
 	// We assume the proposer is validated wrt the block in batch block processing before performing the DA check.
 	// So at this stage we just need to make sure the value being signed and signature bytes match the block.
 	for i := range scs {
-		if blk.Signature() != bytesutil.ToBytes96(scs[i].SignedBlockHeader.Signature) {
+		blobSig := bytesutil.ToBytes96(scs[i].SignedBlockHeader.Signature)
+		if blkSig != blobSig {
 			return nil, ErrBatchSignatureMismatch
 		}
 		// Extra defensive check to make sure the roots match. This should be unnecessary in practice since the root from

consensus-types/blocks/kzg_test.go

@@ -244,7 +244,8 @@ func Test_VerifyKZGInclusionProof(t *testing.T) {
 		StateRoot:  make([]byte, 32),
 	}
 	signedHeader := &ethpb.SignedBeaconBlockHeader{
-		Header: header,
+		Header:    header,
+		Signature: make([]byte, fieldparams.BLSSignatureLength),
 	}
 	sidecar := &ethpb.BlobSidecar{
 		Index:                    uint64(index),

consensus-types/blocks/roblob.go

@@ -1,35 +1,42 @@
 package blocks
 
 import (
-	"github.com/pkg/errors"
 	"github.com/prysmaticlabs/prysm/v5/consensus-types/primitives"
 	"github.com/prysmaticlabs/prysm/v5/encoding/bytesutil"
 	ethpb "github.com/prysmaticlabs/prysm/v5/proto/prysm/v1alpha1"
 )
 
-var errNilBlockHeader = errors.New("received nil beacon block header")
-
 // ROBlob represents a read-only blob sidecar with its block root.
 type ROBlob struct {
 	*ethpb.BlobSidecar
 	root [32]byte
 }
 
+func roblobNilCheck(b *ethpb.BlobSidecar) error {
+	if b == nil {
+		return errNilBlob
+	}
+	if b.SignedBlockHeader == nil || b.SignedBlockHeader.Header == nil {
+		return errNilBlockHeader
+	}
+	if len(b.SignedBlockHeader.Signature) == 0 {
+		return errMissingBlockSignature
+	}
+	return nil
+}
+
 // NewROBlobWithRoot creates a new ROBlob with a given root.
 func NewROBlobWithRoot(b *ethpb.BlobSidecar, root [32]byte) (ROBlob, error) {
-	if b == nil {
-		return ROBlob{}, errNilBlock
+	if err := roblobNilCheck(b); err != nil {
+		return ROBlob{}, err
 	}
 	return ROBlob{BlobSidecar: b, root: root}, nil
 }
 
 // NewROBlob creates a new ROBlob by computing the HashTreeRoot of the header.
 func NewROBlob(b *ethpb.BlobSidecar) (ROBlob, error) {
-	if b == nil {
-		return ROBlob{}, errNilBlock
-	}
-	if b.SignedBlockHeader == nil || b.SignedBlockHeader.Header == nil {
-		return ROBlob{}, errNilBlockHeader
+	if err := roblobNilCheck(b); err != nil {
+		return ROBlob{}, err
 	}
 	root, err := b.SignedBlockHeader.Header.HashTreeRoot()
 	if err != nil {