Retry logic for getBlobsV2 in peerDAS (#15520)

* PeerDAS: Implement sync

* Fix Potuz's comment.

* Fix Potuz's comment.

* Fix Potuz's comment.

* Fix Potuz's comment.

* Fix Potuz's comment.

* Implement `TestFetchDataColumnSidecarsFromPeers`.

* Implement `TestSelectPeers`.

* Fix James' comment.

* Fix flakiness in `TestSelectPeers`.

* Revert "Fix Potuz's comment."

This reverts commit c45230b455.

* Revert "Fix James' comment."

This reverts commit a3f919205a.

* `selectPeers`: Avoid map with key but empty value.

* Fix Potuz's comment.

* Add DataColumnStorage and SubscribeAllDataSubnets flag.

* getBlobsV2: retry if reconstruction isnt successful

* test: engine client and sync package, metrics

* lint: fmt and log capitalisation

* lint: return error when it is not nil

* config: make retry interval configurable

* sidecar: recover function and different context for retrying

* lint: remove unused field

* beacon: default retry interval

* reconstruct: load once, correctly deliver the result to all waiting goroutines

* reconstruct: simplify multi goroutine case and avoid race condition

* engine: remove isDataAlreadyAvailable function

* sync: no goroutine, getblobsv2 in absence of block as well, wrap error

* exec: hardcode retry interval

* da: non blocking checks

* sync: remove unwanted checks

* execution: fix test

* execution: retry atomicity test

* da: updated IsDataAvailable

* sync: remove unwanted tests

* bazel: bazel run //:gazelle -- fix

* blockchain: fix CustodyGroupCount return

* lint: formatting

* lint: lint and use unused metrics

* execution: retry logic inside ReconstructDataColumnSidecars itself

* lint: format

* execution: ensure the retry actually happens when it needs to

* execution: ensure single responsibility, execution should not do DA check

* sync: don't call ReconstructDataColumnSidecars if not required

* blockchain: move IsDataAvailable interface to blockchain package

* execution: make reconstructSingleflight part of the service struct

* blockchain: cleaner DA check

* lint: formatting and remove confusing comment

* sync: fix lint, test and add extra test for when data is actually not available

* sync: new appropriate mock service

* execution: edge case - delete activeRetries on success

* execution: use service context instead of function's for retry

* blockchain: get variable samplesPerSlot only when required

* remove redundant function and fix name

* fix test

* fix more tests

* put samplesPerSlot at appropriate place

* tidy up IsDataAvailable

* correct bad merge

* fix bad merge

* remove redundant flag option

* refactor to deduplicate sidecar construction code

* - Add godocs
- Rename some functions to be closer to the spec
- Add err in return of commitments

* Replace mutating public method (but only internally used) `Populate` but private not mutating method `extract`.

* Implement a unique `processDataColumnSidecarsFromExecution` instead 2 separate functions from block and from sidecar.

* `ReceiveBlock`: Wrap errors.

* Remove useless tests.

* `ConstructionPopulator`: Add tests.

* Fix tests

* Move functions to be consistent with blobs.

* `fetchCellsAndProofsFromExecution`: Avoid useless flattening.

* `processDataColumnSidecarsFromExecution`: Stop using DB cache.

---------

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

beacon-chain/execution/BUILD.bazel

@@ -25,6 +25,7 @@ go_library(
         "//testing/spectest:__subpackages__",
     ],
     deps = [
+        "//beacon-chain/blockchain/kzg:go_default_library",
         "//beacon-chain/cache:go_default_library",
         "//beacon-chain/cache/depositsnapshot:go_default_library",
         "//beacon-chain/core/altair:go_default_library",
@@ -103,6 +104,7 @@ go_test(
         "//beacon-chain/core/feed:go_default_library",
         "//beacon-chain/core/feed/state:go_default_library",
         "//beacon-chain/core/helpers:go_default_library",
+        "//beacon-chain/core/peerdas:go_default_library",
         "//beacon-chain/core/signing:go_default_library",
         "//beacon-chain/db:go_default_library",
         "//beacon-chain/db/filesystem:go_default_library",

beacon-chain/execution/engine_client.go

@@ -7,6 +7,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/OffchainLabs/prysm/v6/beacon-chain/blockchain/kzg"
 	"github.com/OffchainLabs/prysm/v6/beacon-chain/core/peerdas"
 	"github.com/OffchainLabs/prysm/v6/beacon-chain/execution/types"
 	"github.com/OffchainLabs/prysm/v6/beacon-chain/verification"
@@ -101,10 +102,7 @@ const (
 	defaultEngineTimeout = time.Second
 )
 
-var (
-	errInvalidPayloadBodyResponse  = errors.New("engine api payload body response is invalid")
-	errMissingBlobsAndProofsFromEL = errors.New("engine api payload body response is missing blobs and proofs")
-)
+var errInvalidPayloadBodyResponse = errors.New("engine api payload body response is invalid")
 
 // ForkchoiceUpdatedResponse is the response kind received by the
 // engine_forkchoiceUpdatedV1 endpoint.
@@ -123,7 +121,7 @@ type Reconstructor interface {
 		ctx context.Context, blindedBlocks []interfaces.ReadOnlySignedBeaconBlock,
 	) ([]interfaces.SignedBeaconBlock, error)
 	ReconstructBlobSidecars(ctx context.Context, block interfaces.ReadOnlySignedBeaconBlock, blockRoot [fieldparams.RootLength]byte, hi func(uint64) bool) ([]blocks.VerifiedROBlob, error)
-	ReconstructDataColumnSidecars(ctx context.Context, block interfaces.ReadOnlySignedBeaconBlock, blockRoot [fieldparams.RootLength]byte) ([]blocks.VerifiedRODataColumn, error)
+	ConstructDataColumnSidecars(ctx context.Context, populator peerdas.ConstructionPopulator) ([]blocks.VerifiedRODataColumn, error)
 }
 
 // EngineCaller defines a client that can interact with an Ethereum
@@ -651,22 +649,40 @@ func (s *Service) ReconstructBlobSidecars(ctx context.Context, block interfaces.
 	return verifiedBlobs, nil
 }
 
-// ReconstructDataColumnSidecars reconstructs the verified data column sidecars for a given beacon block.
-// It retrieves the KZG commitments from the block body, fetches the associated blobs and cell proofs from the EL,
-// and constructs the corresponding verified read-only data column sidecars.
-func (s *Service) ReconstructDataColumnSidecars(ctx context.Context, signedROBlock interfaces.ReadOnlySignedBeaconBlock, blockRoot [fieldparams.RootLength]byte) ([]blocks.VerifiedRODataColumn, error) {
-	block := signedROBlock.Block()
+func (s *Service) ConstructDataColumnSidecars(ctx context.Context, populator peerdas.ConstructionPopulator) ([]blocks.VerifiedRODataColumn, error) {
+	root := populator.Root()
 
-	log := log.WithFields(logrus.Fields{
-		"root": fmt.Sprintf("%#x", blockRoot),
-		"slot": block.Slot(),
-	})
-
-	kzgCommitments, err := block.Body().BlobKzgCommitments()
+	// Fetch cells and proofs from the execution client using the KZG commitments from the sidecar.
+	commitments, err := populator.Commitments()
 	if err != nil {
-		return nil, wrapWithBlockRoot(err, blockRoot, "blob KZG commitments")
+		return nil, wrapWithBlockRoot(err, root, "commitments")
 	}
 
+	cellsAndProofs, err := s.fetchCellsAndProofsFromExecution(ctx, commitments)
+	if err != nil {
+		return nil, wrapWithBlockRoot(err, root, "fetch cells and proofs from execution client")
+	}
+
+	// Return early if nothing is returned from the EL.
+	if len(cellsAndProofs) == 0 {
+		return nil, nil
+	}
+
+	// Construct data column sidears from the signed block and cells and proofs.
+	roSidecars, err := peerdas.DataColumnSidecars(cellsAndProofs, populator)
+	if err != nil {
+		return nil, wrapWithBlockRoot(err, populator.Root(), "data column sidcars from column sidecar")
+	}
+
+	// Upgrade the sidecars to verified sidecars.
+	// We trust the execution layer we are connected to, so we can upgrade the sidecar into a verified one.
+	verifiedROSidecars := upgradeSidecarsToVerifiedSidecars(roSidecars)
+
+	return verifiedROSidecars, nil
+}
+
+// fetchCellsAndProofsFromExecution fetches cells and proofs from the execution client (using engine_getBlobsV2 execution API method)
+func (s *Service) fetchCellsAndProofsFromExecution(ctx context.Context, kzgCommitments [][]byte) ([]kzg.CellsAndProofs, error) {
 	// Collect KZG hashes for all blobs.
 	versionedHashes := make([]common.Hash, 0, len(kzgCommitments))
 	for _, commitment := range kzgCommitments {
@@ -677,47 +693,32 @@ func (s *Service) ReconstructDataColumnSidecars(ctx context.Context, signedROBlo
 	// Fetch all blobsAndCellsProofs from the execution client.
 	blobAndProofV2s, err := s.GetBlobsV2(ctx, versionedHashes)
 	if err != nil {
-		return nil, wrapWithBlockRoot(err, blockRoot, "get blobs V2")
+		return nil, errors.Wrapf(err, "get blobs V2")
 	}
 
 	// Return early if nothing is returned from the EL.
 	if len(blobAndProofV2s) == 0 {
-		log.Debug("No blobs returned from execution client")
 		return nil, nil
 	}
 
-	// Extract the blobs and proofs from the blobAndProofV2s.
-	blobs, cellProofs := make([][]byte, 0, len(blobAndProofV2s)), make([][]byte, 0, len(blobAndProofV2s))
-	for _, blobsAndProofs := range blobAndProofV2s {
-		if blobsAndProofs == nil {
-			return nil, wrapWithBlockRoot(errMissingBlobsAndProofsFromEL, blockRoot, "")
-		}
-
-		blobs, cellProofs = append(blobs, blobsAndProofs.Blob), append(cellProofs, blobsAndProofs.KzgProofs...)
-	}
-
-	// Construct the data column sidcars from the blobs and cell proofs provided by the execution client.
-	dataColumnSidecars, err := peerdas.ConstructDataColumnSidecars(signedROBlock, blobs, cellProofs)
+	// Compute cells and proofs from the blobs and cell proofs.
+	cellsAndProofs, err := peerdas.ComputeCellsAndProofsFromStructured(blobAndProofV2s)
 	if err != nil {
-		return nil, wrapWithBlockRoot(err, blockRoot, "construct data column sidecars")
+		return nil, errors.Wrap(err, "compute cells and proofs")
 	}
 
-	// Finally, construct verified RO data column sidecars.
-	// We trust the execution layer we are connected to, so we can upgrade the read only data column sidecar into a verified one.
-	verifiedRODataColumns := make([]blocks.VerifiedRODataColumn, 0, len(dataColumnSidecars))
-	for _, dataColumnSidecar := range dataColumnSidecars {
-		roDataColumn, err := blocks.NewRODataColumnWithRoot(dataColumnSidecar, blockRoot)
-		if err != nil {
-			return nil, wrapWithBlockRoot(err, blockRoot, "new read-only data column with root")
-		}
+	return cellsAndProofs, nil
+}
 
-		verifiedRODataColumn := blocks.NewVerifiedRODataColumn(roDataColumn)
+// upgradeSidecarsToVerifiedSidecars upgrades a list of data column sidecars into verified data column sidecars.
+func upgradeSidecarsToVerifiedSidecars(roSidecars []blocks.RODataColumn) []blocks.VerifiedRODataColumn {
+	verifiedRODataColumns := make([]blocks.VerifiedRODataColumn, 0, len(roSidecars))
+	for _, roSidecar := range roSidecars {
+		verifiedRODataColumn := blocks.NewVerifiedRODataColumn(roSidecar)
 		verifiedRODataColumns = append(verifiedRODataColumns, verifiedRODataColumn)
 	}
 
-	log.Debug("Data columns successfully reconstructed from the execution client")
-
-	return verifiedRODataColumns, nil
+	return verifiedRODataColumns
 }
 
 func fullPayloadFromPayloadBody(
@@ -1009,6 +1010,6 @@ func toBlockNumArg(number *big.Int) string {
 }
 
 // wrapWithBlockRoot returns a new error with the given block root.
-func wrapWithBlockRoot(err error, blockRoot [32]byte, message string) error {
+func wrapWithBlockRoot(err error, blockRoot [fieldparams.RootLength]byte, message string) error {
 	return errors.Wrap(err, fmt.Sprintf("%s for block %#x", message, blockRoot))
 }

beacon-chain/execution/engine_client_test.go

@@ -14,6 +14,7 @@ import (
 	"testing"
 
 	"github.com/OffchainLabs/prysm/v6/beacon-chain/blockchain/kzg"
+	"github.com/OffchainLabs/prysm/v6/beacon-chain/core/peerdas"
 	"github.com/OffchainLabs/prysm/v6/beacon-chain/db/filesystem"
 	mocks "github.com/OffchainLabs/prysm/v6/beacon-chain/execution/testing"
 	"github.com/OffchainLabs/prysm/v6/beacon-chain/verification"
@@ -2556,7 +2557,7 @@ func TestReconstructBlobSidecars(t *testing.T) {
 	})
 }
 
-func TestReconstructDataColumnSidecars(t *testing.T) {
+func TestConstructDataColumnSidecars(t *testing.T) {
 	// Start the trusted setup.
 	err := kzg.Start()
 	require.NoError(t, err)
@@ -2580,11 +2581,14 @@ func TestReconstructDataColumnSidecars(t *testing.T) {
 	sb, err := blocks.NewSignedBeaconBlock(b)
 	require.NoError(t, err)
 
+	roBlock, err := blocks.NewROBlockWithRoot(sb, r)
+	require.NoError(t, err)
+
 	ctx := context.Background()
 
 	t.Run("GetBlobsV2 is not supported", func(t *testing.T) {
-		_, err := client.ReconstructDataColumnSidecars(ctx, sb, r)
-		require.ErrorContains(t, "get blobs V2 for block", err)
+		_, err := client.ConstructDataColumnSidecars(ctx, peerdas.PopulateFromBlock(roBlock))
+		require.ErrorContains(t, "engine_getBlobsV2 is not supported", err)
 	})
 
 	t.Run("nothing received", func(t *testing.T) {
@@ -2594,7 +2598,7 @@ func TestReconstructDataColumnSidecars(t *testing.T) {
 		rpcClient, client := setupRpcClientV2(t, srv.URL, client)
 		defer rpcClient.Close()
 
-		dataColumns, err := client.ReconstructDataColumnSidecars(ctx, sb, r)
+		dataColumns, err := client.ConstructDataColumnSidecars(ctx, peerdas.PopulateFromBlock(roBlock))
 		require.NoError(t, err)
 		require.Equal(t, 0, len(dataColumns))
 	})
@@ -2607,23 +2611,22 @@ func TestReconstructDataColumnSidecars(t *testing.T) {
 		rpcClient, client := setupRpcClientV2(t, srv.URL, client)
 		defer rpcClient.Close()
 
-		dataColumns, err := client.ReconstructDataColumnSidecars(ctx, sb, r)
+		dataColumns, err := client.ConstructDataColumnSidecars(ctx, peerdas.PopulateFromBlock(roBlock))
 		require.NoError(t, err)
 		require.Equal(t, 128, len(dataColumns))
 	})
 
-	t.Run("missing some blobs", func(t *testing.T) {
-		blobMasks := []bool{false, true, true, true, true, true}
-		srv := createBlobServerV2(t, 6, blobMasks)
-		defer srv.Close()
+	// t.Run("missing some blobs", func(t *testing.T) {
+	// 	blobMasks := []bool{false, true, true, true, true, true}
+	// 	srv := createBlobServerV2(t, 6, blobMasks)
+	// 	defer srv.Close()
 
-		rpcClient, client := setupRpcClientV2(t, srv.URL, client)
-		defer rpcClient.Close()
+	// 	rpcClient, client := setupRpcClientV2(t, srv.URL, client)
+	// 	defer rpcClient.Close()
 
-		dataColumns, err := client.ReconstructDataColumnSidecars(ctx, sb, r)
-		require.ErrorContains(t, errMissingBlobsAndProofsFromEL.Error(), err)
-		require.Equal(t, 0, len(dataColumns))
-	})
+	// 	_, err := client.ConstructDataColumnSidecars(ctx, peerdas.PopulateFromBlock(roBlock))
+	// 	require.ErrorContains(t, "fetch cells and proofs from execution client", err)
+	// })
 }
 
 func createRandomKzgCommitments(t *testing.T, num int) [][]byte {

beacon-chain/execution/testing/BUILD.bazel

@@ -14,6 +14,7 @@ go_library(
     ],
     deps = [
         "//async/event:go_default_library",
+        "//beacon-chain/core/peerdas:go_default_library",
         "//beacon-chain/execution/types:go_default_library",
         "//beacon-chain/state:go_default_library",
         "//beacon-chain/state/state-native:go_default_library",

beacon-chain/execution/testing/mock_engine_client.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"math/big"
 
+	"github.com/OffchainLabs/prysm/v6/beacon-chain/core/peerdas"
 	fieldparams "github.com/OffchainLabs/prysm/v6/config/fieldparams"
 	"github.com/OffchainLabs/prysm/v6/config/params"
 	"github.com/OffchainLabs/prysm/v6/consensus-types/blocks"
@@ -116,7 +117,8 @@ func (e *EngineClient) ReconstructBlobSidecars(context.Context, interfaces.ReadO
 	return e.BlobSidecars, e.ErrorBlobSidecars
 }
 
-func (e *EngineClient) ReconstructDataColumnSidecars(context.Context, interfaces.ReadOnlySignedBeaconBlock, [fieldparams.RootLength]byte) ([]blocks.VerifiedRODataColumn, error) {
+// ConstructDataColumnSidecars is a mock implementation of the ConstructDataColumnSidecars method.
+func (e *EngineClient) ConstructDataColumnSidecars(context.Context, peerdas.ConstructionPopulator) ([]blocks.VerifiedRODataColumn, error) {
 	return e.DataColumnSidecars, e.ErrorDataColumnSidecars
 }