Fix bug from PR 13827 (#13871)

* fix AS cache bug, tighten ro constructors

* additional coverage on AS cache filter

---------

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

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 {

consensus-types/blocks/roblob_test.go

@@ -5,50 +5,94 @@ import (
 
 	fieldparams "github.com/prysmaticlabs/prysm/v5/config/fieldparams"
 	"github.com/prysmaticlabs/prysm/v5/consensus-types/primitives"
+	"github.com/prysmaticlabs/prysm/v5/encoding/bytesutil"
 	ethpb "github.com/prysmaticlabs/prysm/v5/proto/prysm/v1alpha1"
 	"github.com/prysmaticlabs/prysm/v5/testing/assert"
+	"github.com/prysmaticlabs/prysm/v5/testing/require"
 )
 
-func TestNewROBlobWithRoot(t *testing.T) {
-	sidecar := &ethpb.BlobSidecar{}
-	root := [32]byte{}
-
-	blob, err := NewROBlobWithRoot(sidecar, root)
-	assert.NoError(t, err)
-	assert.Equal(t, root, blob.BlockRoot())
-
-	blob, err = NewROBlobWithRoot(nil, root)
-	assert.Equal(t, errNilBlock, err)
-}
-
-// TestNewROBlob tests the NewROBlob function.
-func TestNewROBlob(t *testing.T) {
-	h := &ethpb.SignedBeaconBlockHeader{
-		Header: &ethpb.BeaconBlockHeader{
-			ParentRoot: make([]byte, fieldparams.RootLength),
-			StateRoot:  make([]byte, fieldparams.RootLength),
-			BodyRoot:   make([]byte, fieldparams.RootLength),
+func TestROBlobNilChecks(t *testing.T) {
+	cases := []struct {
+		name  string
+		bfunc func(t *testing.T) *ethpb.BlobSidecar
+		err   error
+		root  []byte
+	}{
+		{
+			name: "nil signed blob",
+			bfunc: func(t *testing.T) *ethpb.BlobSidecar {
+				return nil
+			},
+			err:  errNilBlob,
+			root: bytesutil.PadTo([]byte("sup"), 32),
+		},
+		{
+			name: "nil signed block header",
+			bfunc: func(t *testing.T) *ethpb.BlobSidecar {
+				return &ethpb.BlobSidecar{
+					SignedBlockHeader: nil,
+				}
+			},
+			err:  errNilBlockHeader,
+			root: bytesutil.PadTo([]byte("sup"), 32),
+		},
+		{
+			name: "nil inner header",
+			bfunc: func(t *testing.T) *ethpb.BlobSidecar {
+				return &ethpb.BlobSidecar{
+					SignedBlockHeader: &ethpb.SignedBeaconBlockHeader{
+						Header: nil,
+					},
+				}
+			},
+			err:  errNilBlockHeader,
+			root: bytesutil.PadTo([]byte("sup"), 32),
+		},
+		{
+			name: "nil signature",
+			bfunc: func(t *testing.T) *ethpb.BlobSidecar {
+				return &ethpb.BlobSidecar{
+					SignedBlockHeader: &ethpb.SignedBeaconBlockHeader{
+						Header: &ethpb.BeaconBlockHeader{
+							ParentRoot: make([]byte, fieldparams.RootLength),
+							StateRoot:  make([]byte, fieldparams.RootLength),
+							BodyRoot:   make([]byte, fieldparams.RootLength),
+						},
+						Signature: nil,
+					},
+				}
+			},
+			err:  errMissingBlockSignature,
+			root: bytesutil.PadTo([]byte("sup"), 32),
 		},
-		Signature: make([]byte, fieldparams.BLSSignatureLength),
 	}
-	sidecar := &ethpb.BlobSidecar{
-		SignedBlockHeader: h,
+	for _, c := range cases {
+		t.Run(c.name+" NewROBlob", func(t *testing.T) {
+			b := c.bfunc(t)
+			bl, err := NewROBlob(b)
+			if c.err != nil {
+				require.ErrorIs(t, err, c.err)
+			} else {
+				require.NoError(t, err)
+				hr, err := b.SignedBlockHeader.HashTreeRoot()
+				require.NoError(t, err)
+				require.Equal(t, hr, bl.BlockRoot())
+			}
+		})
+		if len(c.root) == 0 {
+			continue
+		}
+		t.Run(c.name+" NewROBlobWithRoot", func(t *testing.T) {
+			b := c.bfunc(t)
+			// We want the same validation when specifying a root.
+			bl, err := NewROBlobWithRoot(b, bytesutil.ToBytes32(c.root))
+			if c.err != nil {
+				require.ErrorIs(t, err, c.err)
+			} else {
+				assert.Equal(t, bytesutil.ToBytes32(c.root), bl.BlockRoot())
+			}
+		})
 	}
-
-	blob, err := NewROBlob(sidecar)
-	assert.NoError(t, err)
-	assert.NotNil(t, blob)
-
-	_, err = NewROBlob(nil)
-	assert.Equal(t, errNilBlock, err)
-
-	sidecar.SignedBlockHeader = nil
-	_, err = NewROBlob(sidecar)
-	assert.Equal(t, errNilBlockHeader, err)
-
-	sidecar.SignedBlockHeader = &ethpb.SignedBeaconBlockHeader{}
-	_, err = NewROBlob(sidecar)
-	assert.Equal(t, errNilBlockHeader, err)
 }
 
 func TestBlockRoot(t *testing.T) {

consensus-types/blocks/roblock_test.go

@@ -4,9 +4,11 @@ import (
 	"sort"
 	"testing"
 
+	"github.com/prysmaticlabs/prysm/v5/consensus-types/interfaces"
 	"github.com/prysmaticlabs/prysm/v5/consensus-types/primitives"
 	"github.com/prysmaticlabs/prysm/v5/encoding/bytesutil"
 	eth "github.com/prysmaticlabs/prysm/v5/proto/prysm/v1alpha1"
+	"github.com/prysmaticlabs/prysm/v5/runtime/version"
 	"github.com/prysmaticlabs/prysm/v5/testing/require"
 )
 
@@ -88,3 +90,103 @@ func testROBlock(t *testing.T, slot primitives.Slot, root [32]byte) ROBlock {
 		root:                      root,
 	}
 }
+
+func TestROBlockNilChecks(t *testing.T) {
+	cases := []struct {
+		name  string
+		bfunc func(t *testing.T) interfaces.SignedBeaconBlock
+		err   error
+		root  []byte
+	}{
+		{
+			name: "happy path",
+			bfunc: func(t *testing.T) interfaces.SignedBeaconBlock {
+				b, err := NewSignedBeaconBlock(hydrateSignedBeaconBlock())
+				require.NoError(t, err)
+				return b
+			},
+		},
+		{
+			name: "happy path - with root",
+			bfunc: func(t *testing.T) interfaces.SignedBeaconBlock {
+				b, err := NewSignedBeaconBlock(hydrateSignedBeaconBlock())
+				require.NoError(t, err)
+				return b
+			},
+			root: bytesutil.PadTo([]byte("sup"), 32),
+		},
+		{
+			name: "nil signed block",
+			bfunc: func(t *testing.T) interfaces.SignedBeaconBlock {
+				return nil
+			},
+			err: ErrNilSignedBeaconBlock,
+		},
+		{
+			name: "nil signed block - with root",
+			bfunc: func(t *testing.T) interfaces.SignedBeaconBlock {
+				return nil
+			},
+			err:  ErrNilSignedBeaconBlock,
+			root: bytesutil.PadTo([]byte("sup"), 32),
+		},
+		{
+			name: "nil inner block",
+			bfunc: func(t *testing.T) interfaces.SignedBeaconBlock {
+				return &SignedBeaconBlock{
+					version:   version.Deneb,
+					block:     nil,
+					signature: bytesutil.ToBytes96(nil),
+				}
+			},
+			err: ErrNilSignedBeaconBlock,
+		},
+		{
+			name: "nil inner block",
+			bfunc: func(t *testing.T) interfaces.SignedBeaconBlock {
+				return &SignedBeaconBlock{
+					version:   version.Deneb,
+					block:     nil,
+					signature: bytesutil.ToBytes96(nil),
+				}
+			},
+			err: ErrNilSignedBeaconBlock,
+		},
+		{
+			name: "nil block body",
+			bfunc: func(t *testing.T) interfaces.SignedBeaconBlock {
+				bb := &BeaconBlock{
+					version:       version.Deneb,
+					slot:          0,
+					proposerIndex: 0,
+					parentRoot:    bytesutil.ToBytes32(nil),
+					stateRoot:     bytesutil.ToBytes32(nil),
+					body:          nil,
+				}
+				return &SignedBeaconBlock{
+					version:   version.Deneb,
+					block:     bb,
+					signature: bytesutil.ToBytes96(nil),
+				}
+			},
+			err: ErrNilSignedBeaconBlock,
+		},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			b := c.bfunc(t)
+			var err error
+			if len(c.root) == 0 {
+				_, err = NewROBlock(b)
+			} else {
+				_, err = NewROBlockWithRoot(b, bytesutil.ToBytes32(c.root))
+			}
+			if c.err != nil {
+				require.ErrorIs(t, err, c.err)
+				return
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}

consensus-types/blocks/types.go

@@ -27,10 +27,13 @@ const (
 var (
 	// ErrUnsupportedVersion for beacon block methods.
 	ErrUnsupportedVersion    = errors.New("unsupported beacon block version")
+	errNilBlob               = errors.New("received nil blob sidecar")
 	errNilBlock              = errors.New("received nil beacon block")
 	errNilBlockBody          = errors.New("received nil beacon block body")
 	errIncorrectBlockVersion = errors.New(incorrectBlockVersion)
 	errIncorrectBodyVersion  = errors.New(incorrectBodyVersion)
+	errNilBlockHeader        = errors.New("received nil beacon block header")
+	errMissingBlockSignature = errors.New("received nil beacon block signature")
 )
 
 // BeaconBlockBody is the main beacon block body structure. It can represent any block type.