fix(blobs): Validate sidecar KzgCommitments and disallow sidecar om…

…ission (#2291)

beacon/blockchain/errors.go

@@ -25,6 +25,8 @@ import "github.com/berachain/beacon-kit/errors"
 var (
 	// ErrNilBlk is an error for when the beacon block is nil.
 	ErrNilBlk = errors.New("nil beacon block")
+	// ErrNilBlob is an error for when the BlobSidecars is nil.
+	ErrNilBlob = errors.New("nil blob")
 	// ErrDataNotAvailable indicates that the required data is not available.
 	ErrDataNotAvailable = errors.New("data not available")
 )

beacon/blockchain/process_proposal.go

@@ -22,6 +22,7 @@ package blockchain
 
 import (
 	"context"
+	"fmt"
 	"time"
 
 	payloadtime "github.com/berachain/beacon-kit/beacon/payload-time"
@@ -62,6 +63,11 @@ func (s *Service[
 		)
 	if err != nil {
 		return createProcessProposalResponse(errors.WrapNonFatal(err))
+	} else if blk.IsNil() {
+		s.logger.Warn(
+			"Aborting block verification - beacon block not found in proposal",
+		)
+		return createProcessProposalResponse(errors.WrapNonFatal(ErrNilBlk))
 	}
 
 	// Decode the blob sidecars.
@@ -72,56 +78,47 @@ func (s *Service[
 		)
 	if err != nil {
 		return createProcessProposalResponse(errors.WrapNonFatal(err))
+	} else if sidecars.IsNil() {
+		s.logger.Warn(
+			"Aborting block verification - blob sidecars not found in proposal",
+		)
+		return createProcessProposalResponse(errors.WrapNonFatal(ErrNilBlob))
 	}
 
-	// In theory, swapping the order of verification between the sidecars
-	// and the incoming block should not introduce any inconsistencies
-	// in the state on which the sidecar verification depends on (notably
-	// the currently active fork). ProcessProposal should only
-	// keep the state changes as candidates (which is what we do in
-	// VerifyIncomingBlock).
+	// Make sure we have the right number of BlobSidecars
+	numCommitments := len(blk.GetBody().GetBlobKzgCommitments())
+	if numCommitments != sidecars.Len() {
+		err = fmt.Errorf("expected %d sidecars, got %d",
+			numCommitments, sidecars.Len(),
+		)
+		return createProcessProposalResponse(errors.WrapNonFatal(err))
+	}
 
-	// Process the blob sidecars, if any
-	if !sidecars.IsNil() && sidecars.Len() > 0 {
+	if numCommitments > 0 {
+		// Process the blob sidecars
+		//
+		// In theory, swapping the order of verification between the sidecars
+		// and the incoming block should not introduce any inconsistencies
+		// in the state on which the sidecar verification depends on (notably
+		// the currently active fork). ProcessProposal should only
+		// keep the state changes as candidates (which is what we do in
+		// VerifyIncomingBlock).
 		var consensusSidecars *types.ConsensusSidecars
 		consensusSidecars = consensusSidecars.New(
 			sidecars,
 			blk.GetHeader(),
 		)
-
-		s.logger.Info("Received incoming blob sidecars")
-
-		// TODO: Clean this up once we remove generics.
-		cs := convertConsensusSidecars[ConsensusSidecarsT](consensusSidecars)
-
-		// Get the sidecar verification function from the state processor
-		//nolint:govet	// err shadowing
-		sidecarVerifierFn, err := s.stateProcessor.GetSidecarVerifierFn(
-			s.storageBackend.StateFromContext(ctx),
+		err = s.VerifyIncomingBlobSidecars(
+			ctx,
+			consensusSidecars,
 		)
 		if err != nil {
 			s.logger.Error(
-				"an error incurred while calculating the sidecar verifier",
-				"reason", err,
+				"failed to verify incoming blob sidecars",
+				"error", err,
 			)
 			return createProcessProposalResponse(errors.WrapNonFatal(err))
 		}
-
-		// Verify the blobs and ensure they match the local state.
-		err = s.blobProcessor.VerifySidecars(cs, sidecarVerifierFn)
-		if err != nil {
-			s.logger.Error(
-				"rejecting incoming blob sidecars",
-				"reason", err,
-			)
-			return createProcessProposalResponse(errors.WrapNonFatal(err))
-		}
-
-		s.logger.Info(
-			"Blob sidecars verification succeeded - accepting incoming blob sidecars",
-			"num_blobs",
-			sidecars.Len(),
-		)
 	}
 
 	// Process the block
@@ -145,6 +142,52 @@ func (s *Service[
 	return createProcessProposalResponse(nil)
 }
 
+// VerifyIncomingBlobSidecars verifies the BlobSidecars of an incoming
+// proposal and logs the process.
+func (s *Service[
+	_, _, ConsensusBlockT, _,
+	GenesisT, ConsensusSidecarsT,
+]) VerifyIncomingBlobSidecars(
+	ctx context.Context,
+	cSidecars *types.ConsensusSidecars,
+) error {
+	sidecars := cSidecars.GetSidecars()
+
+	s.logger.Info("Received incoming blob sidecars")
+
+	// TODO: Clean this up once we remove generics.
+	cs := convertConsensusSidecars[ConsensusSidecarsT](cSidecars)
+
+	// Get the sidecar verification function from the state processor
+	sidecarVerifierFn, err := s.stateProcessor.GetSidecarVerifierFn(
+		s.storageBackend.StateFromContext(ctx),
+	)
+	if err != nil {
+		s.logger.Error(
+			"an error incurred while calculating the sidecar verifier",
+			"reason", err,
+		)
+		return err
+	}
+
+	// Verify the blobs and ensure they match the local state.
+	err = s.blobProcessor.VerifySidecars(cs, sidecarVerifierFn)
+	if err != nil {
+		s.logger.Error(
+			"rejecting incoming blob sidecars",
+			"reason", err,
+		)
+		return err
+	}
+
+	s.logger.Info(
+		"Blob sidecars verification succeeded - accepting incoming blob sidecars",
+		"num_blobs",
+		sidecars.Len(),
+	)
+	return nil
+}
+
 // VerifyIncomingBlock verifies the state root of an incoming block
 // and logs the process.
 func (s *Service[
@@ -165,14 +208,6 @@ func (s *Service[
 	// ideally via some broader sync service.
 	s.forceStartupSyncOnce.Do(func() { s.forceStartupHead(ctx, preState) })
 
-	// If the block is nil or a nil pointer, exit early.
-	if beaconBlk.IsNil() {
-		s.logger.Warn(
-			"Aborting block verification - beacon block not found in proposal",
-		)
-		return errors.WrapNonFatal(ErrNilBlk)
-	}
-
 	s.logger.Info(
 		"Received incoming beacon block",
 		"state_root", beaconBlk.GetStateRoot(), "slot", beaconBlk.GetSlot(),

da/blob/verifier.go

@@ -29,7 +29,9 @@ import (
 	ctypes "github.com/berachain/beacon-kit/consensus-types/types"
 	"github.com/berachain/beacon-kit/da/kzg"
 	datypes "github.com/berachain/beacon-kit/da/types"
+	"github.com/berachain/beacon-kit/errors"
 	"github.com/berachain/beacon-kit/primitives/crypto"
+	"github.com/berachain/beacon-kit/primitives/eip4844"
 	"github.com/berachain/beacon-kit/primitives/math"
 	"golang.org/x/sync/errgroup"
 )
@@ -75,15 +77,29 @@ func (bv *verifier) verifySidecars(
 
 	g, _ := errgroup.WithContext(context.Background())
 
-	// Verifying that sidecars block headers match with header of the
-	// corresponding block concurrently.
+	// create lookup table to check for duplicate commitments
+	duplicateCommitment := make(map[eip4844.KZGCommitment]struct{})
+
+	// Validate sidecar fields against data from the BeaconBlock.
 	for i, s := range sidecars.GetSidecars() {
+		// Check if sidecar's kzgCommitment is duplicate. Along with the
+		// length check and the inclusion proof, this fully verifies that
+		// the KzgCommitments in the BlobSidecar are the exact same as the
+		// ones in the BeaconBlockBody without having to explicitly compare.
+		if _, exists := duplicateCommitment[s.GetKzgCommitment()]; exists {
+			return errors.New(
+				"found duplicate KzgCommitments in BlobSidecars",
+			)
+		}
+		duplicateCommitment[s.GetKzgCommitment()] = struct{}{}
+
 		// This check happens outside the goroutines so that we do not
 		// process the inclusion proofs before validating the index.
 		if s.GetIndex() >= bv.chainSpec.MaxBlobsPerBlock() {
 			return fmt.Errorf("invalid sidecar Index: %d", i)
 		}
 		g.Go(func() error {
+			// Verify the signature.
 			var sigHeader = s.GetSignedBeaconBlockHeader()
 
 			// Check BlobSidecar.Header equality with BeaconBlockHeader

da/types/sidecars.go

@@ -30,7 +30,7 @@ import (
 // Sidecars is a slice of blob side cars to be included in the block.
 type BlobSidecars []*BlobSidecar
 
-// NewBlobSidecars creates a new BlobSidecars object.
+// Empty creates a new empty BlobSidecars object.
 func (bs *BlobSidecars) Empty() *BlobSidecars {
 	return &BlobSidecars{}
 }

da/types/sidecars_test.go

@@ -150,3 +150,10 @@ func TestValidateBlockRoots(t *testing.T) {
 		"Validating sidecar with invalid roots should produce an error",
 	)
 }
+
+func TestZeroSidecarsInBlobSidecarsIsNotNil(t *testing.T) {
+	// This test exists to ensure that proposing a BlobSidecars with 0
+	// Sidecars is not considered IsNil().
+	sidecars := &types.BlobSidecars{}
+	require.False(t, sidecars.IsNil())
+}