eth/catalyst: implement getBlobsV3

[MarcoPolo]

This is used by cell-level dissemination (aka partial messages) to give the CL all blobs the EL knows about and let CL communicate efficiently about any other missing blobs. In other words, partial responses from the EL is useful now.

See the related (closed) PR: ethereum/execution-apis#674 and the new PR: ethereum/execution-apis#719

[raulk]

Previous version of this PR: #32170

[raulk]

This one includes metrics accounting, so it seems like an improvement over the old one.

[MariusVanDerWijden]

How do you feel about:

diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go
index 109581e240..2be050f084 100644
--- a/eth/catalyst/api.go
+++ b/eth/catalyst/api.go
@@ -571,51 +571,19 @@ func (api *ConsensusAPI) GetBlobsV2(hashes []common.Hash) ([]*engine.BlobAndProo
 	if api.config().LatestFork(head.Time) < forks.Osaka {
 		return nil, unsupportedForkErr("engine_getBlobsV2 is not available before Osaka fork")
 	}
-	if len(hashes) > 128 {
-		return nil, engine.TooLargeRequest.With(fmt.Errorf("requested blob count too large: %v", len(hashes)))
-	}
-	available := api.eth.BlobTxPool().AvailableBlobs(hashes)
-	getBlobsRequestedCounter.Inc(int64(len(hashes)))
-	getBlobsAvailableCounter.Inc(int64(available))
-
-	// Optimization: check first if all blobs are available, if not, return empty response
-	if available != len(hashes) {
-		getBlobsV2RequestMiss.Inc(1)
-		return nil, nil
-	}
-
-	blobs, _, proofs, err := api.eth.BlobTxPool().GetBlobs(hashes, types.BlobSidecarVersion1, false)
-	if err != nil {
-		return nil, engine.InvalidParams.With(err)
-	}
-
-	// To comply with API spec, check again that we really got all data needed
-	for _, blob := range blobs {
-		if blob == nil {
-			getBlobsV2RequestMiss.Inc(1)
-			return nil, nil
-		}
-	}
-	getBlobsV2RequestHit.Inc(1)
-
-	res := make([]*engine.BlobAndProofV2, len(hashes))
-	for i := 0; i < len(blobs); i++ {
-		var cellProofs []hexutil.Bytes
-		for _, proof := range proofs[i] {
-			cellProofs = append(cellProofs, proof[:])
-		}
-		res[i] = &engine.BlobAndProofV2{
-			Blob:       blobs[i][:],
-			CellProofs: cellProofs,
-		}
-	}
-	return res, nil
+	return api.getBlobs(hashes, false)
 }
 
 // GetBlobsV3 returns a set of blobs from the transaction pool. Same as
 // GetBlobsV2, except will return partial responses in case there is a missing
 // blob.
 func (api *ConsensusAPI) GetBlobsV3(hashes []common.Hash) ([]*engine.BlobAndProofV2, error) {
+	return api.getBlobs(hashes, true)
+}
+
+// getBlobs returns all available blobs.
+// if allowPartials is not set, either all or no blobs are returned.
+func (api *ConsensusAPI) getBlobs(hashes []common.Hash, allowPartials bool) ([]*engine.BlobAndProofV2, error) {
 	if len(hashes) > 128 {
 		return nil, engine.TooLargeRequest.With(fmt.Errorf("requested blob count too large: %v", len(hashes)))
 	}
@@ -631,10 +599,14 @@ func (api *ConsensusAPI) GetBlobsV3(hashes []common.Hash) ([]*engine.BlobAndProo
 	res := make([]*engine.BlobAndProofV2, len(hashes))
 	for i := range blobs {
 		if blobs[i] == nil {
-			getBlobsV3RequestMiss.Inc(1)
-			continue
+			if allowPartials {
+				getBlobsV3RequestMiss.Inc(1)
+				continue
+			} else {
+				getBlobsV2RequestMiss.Inc(1)
+				return nil, nil
+			}
 		}
-		getBlobsV3RequestHit.Inc(1)
 		var cellProofs []hexutil.Bytes
 		for _, proof := range proofs[i] {
 			cellProofs = append(cellProofs, proof[:])
@@ -644,6 +616,11 @@ func (api *ConsensusAPI) GetBlobsV3(hashes []common.Hash) ([]*engine.BlobAndProo
 			CellProofs: cellProofs,
 		}
 	}
+	if allowPartials {
+		getBlobsV3RequestHit.Inc(int64(len(blobs)))
+	} else {
+		getBlobsV2RequestHit.Inc(1)
+	}
 	return res, nil
 }
 

[MarcoPolo]

@MariusVanDerWijden done. Updated in the latest version of the commit git range-diff c75be1f61a18...5cbe6bd60c4d

[MariusVanDerWijden]

LGTM

[fjl]

I don't understand why it's a separate metric for v3. We can just use the existing getblobs metrics. If we add the V3 in metrics names, all the dashboards have to be changed.

[MariusVanDerWijden]

The existing metric only counts if all blobs are available or unavailable. The new one counts how many were available. We could change the old one and integrate both

[raulk]

Changing the old metric would change the semantics though. The old metric implicitly counts "number of requests that returned data". I'd keep v2 as is (and potentially add the blob-counting version too to v2, as it's not easy to get down to blob hits/misses today AFAIK -- multiplying the metric by blobs included is not an option).

[rjl493456442]

Can't we just use these two metrics for counting how many blobs are available and how many are not?

	// Number of blobs requested via getBlobsV2
	getBlobsRequestedCounter = metrics.NewRegisteredCounter("engine/getblobs/requested", nil)

	// Number of blobs requested via getBlobsV2 that are present in the blobpool
	getBlobsAvailableCounter = metrics.NewRegisteredCounter("engine/getblobs/available", nil)

They are duplicated with the engine/getblobsV3/hit and engine/getblobsV3/miss right?

[rjl493456442]

something like this?

diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go
index cd07465d99..073da9fba3 100644
--- a/eth/catalyst/api.go
+++ b/eth/catalyst/api.go
@@ -93,12 +93,6 @@ var (
 
 	// Number of times getBlobsV2 responded with “miss”
 	getBlobsV2RequestMiss = metrics.NewRegisteredCounter("engine/getblobs/miss", nil)
-
-	// Number of blobs getBlobsV3 could return
-	getBlobsV3RequestHit = metrics.NewRegisteredCounter("engine/getblobsV3/hit", nil)
-
-	// Number of blobs getBlobsV3 could not return
-	getBlobsV3RequestMiss = metrics.NewRegisteredCounter("engine/getblobsV3/miss", nil)
 )
 
 type ConsensusAPI struct {
@@ -571,7 +565,7 @@ func (api *ConsensusAPI) GetBlobsV2(hashes []common.Hash) ([]*engine.BlobAndProo
 	if api.config().LatestFork(head.Time) < forks.Osaka {
 		return nil, nil
 	}
-	return api.getBlobs(hashes, false)
+	return api.getBlobs(hashes, true)
 }
 
 // GetBlobsV3 returns a set of blobs from the transaction pool. Same as
@@ -582,12 +576,12 @@ func (api *ConsensusAPI) GetBlobsV3(hashes []common.Hash) ([]*engine.BlobAndProo
 	if api.config().LatestFork(head.Time) < forks.Osaka {
 		return nil, nil
 	}
-	return api.getBlobs(hashes, true)
+	return api.getBlobs(hashes, false)
 }
 
-// getBlobs returns all available blobs.
-// if allowPartials is not set, either all or no blobs are returned.
-func (api *ConsensusAPI) getBlobs(hashes []common.Hash, allowPartials bool) ([]*engine.BlobAndProofV2, error) {
+// getBlobs returns all available blobs. In v2, partial responses are not allowed,
+// while v3 supports partial responses.
+func (api *ConsensusAPI) getBlobs(hashes []common.Hash, v2 bool) ([]*engine.BlobAndProofV2, error) {
 	if len(hashes) > 128 {
 		return nil, engine.TooLargeRequest.With(fmt.Errorf("requested blob count too large: %v", len(hashes)))
 	}
@@ -596,7 +590,7 @@ func (api *ConsensusAPI) getBlobs(hashes []common.Hash, allowPartials bool) ([]*
 	getBlobsAvailableCounter.Inc(int64(available))
 
 	// Short circuit if partial response is not allowed
-	if !allowPartials && available != len(hashes) {
+	if v2 && available != len(hashes) {
 		getBlobsV2RequestMiss.Inc(1)
 		return nil, nil
 	}
@@ -609,9 +603,10 @@ func (api *ConsensusAPI) getBlobs(hashes []common.Hash, allowPartials bool) ([]*
 	// Validate the blobs from the pool and assemble the response
 	res := make([]*engine.BlobAndProofV2, len(hashes))
 	for i := range blobs {
+		// The blob has been evicted since the last AvailableBlobs call.
+		// Return null if partial response is not allowed.
 		if blobs[i] == nil {
-			if allowPartials {
-				getBlobsV3RequestMiss.Inc(1)
+			if !v2 {
 				continue
 			} else {
 				getBlobsV2RequestMiss.Inc(1)
@@ -627,9 +622,7 @@ func (api *ConsensusAPI) getBlobs(hashes []common.Hash, allowPartials bool) ([]*
 			CellProofs: cellProofs,
 		}
 	}
-	if allowPartials {
-		getBlobsV3RequestHit.Inc(int64(len(blobs)))
-	} else {
+	if v2 {
 		getBlobsV2RequestHit.Inc(1)
 	}
 	return res, nil

[MarcoPolo]

Can't we just use these two metrics for counting how many blobs are available and how many are not?

You're right. I'll make that change.

I think we might also want a metric that tracks the number of times we returned a partial response, as I don't think we can get that with the existing metrics.

[MarcoPolo]

• I've added @rjl493456442 's diff above as commit 5aef119.
• I've renamed the metric variables to drop the "v2" qualifier 45a9f25
• I've added a partial hit metric efdb0d7. This gives us the metric of how many times getBlobs returned some, but not all, blobs.

[rjl493456442]

LGTM