op-service: improve beacon client robustness (#17866)

* op-service: always fall back to fetching blob sidecars

Previously, we only reverted to fetching sidecars when an error was
received from the beacon server. In an attempt to be more robust,
we also fetch sidecars when the beacon server gives us a semantically
invalid response.

* op-service: rearrange blobs based on provided hashes

The behavior in the post-Fulu /blobs/ happy path and the
/blob_sidecars/ fallback slightly differed: in the former case,
returned blobs were ordered based their index in the blob, while in the
latter case they were ordered based on the provided hashes.

This bug was not caught in testing because all calls to BeaconBlobs
provide a slice of hashes sorted by index.

Author: joshklop

op-service/sources/l1_beacon_client.go

@@ -9,6 +9,7 @@ import (
 	"net/http"
 	"net/url"
 	"path"
+	"slices"
 	"strconv"
 	"sync"
 
@@ -303,30 +304,53 @@ func (cl *L1BeaconClient) GetBlobs(ctx context.Context, ref eth.L1BlockRef, hash
 	if err != nil {
 		return nil, err
 	}
+	blobs, errBeaconBlobs := cl.beaconBlobs(ctx, slot, hashes)
+	if errBeaconBlobs == nil {
+		return blobs, nil
+	}
+	// If fetching from the post-Fulu /blobs/ endpoint fails, fall back to /blob_sidecars/.
+	errBeaconBlobs = fmt.Errorf("failed to get blobs: %w", errBeaconBlobs)
+	blobSidecars, err := cl.getBlobSidecars(ctx, slot, hashes)
+	if err != nil {
+		return nil, fmt.Errorf("%w; failed to get blob sidecars for L1BlockRef %s after falling back: %w", errBeaconBlobs, ref, err)
+	}
+	blobs, err = blobsFromSidecars(blobSidecars, hashes)
+	if err != nil {
+		return nil, fmt.Errorf("%w; failed to get blobs from sidecars for L1BlockRef %s after falling back: %w", errBeaconBlobs, ref, err)
+	}
+	return blobs, nil
+}
+
+func (cl *L1BeaconClient) beaconBlobs(ctx context.Context, slot uint64, hashes []eth.IndexedBlobHash) ([]*eth.Blob, error) {
 	resp, err := cl.cl.BeaconBlobs(ctx, slot, hashes)
 	if err != nil {
-		// We would normally check for an explicit error like "method not found", but the Beacon
-		// API doesn't standardize such a response. Thus, we interpret all errors as
-		// "method not found" and fall back to fetching sidecars.
-		blobSidecars, err := cl.getBlobSidecars(ctx, slot, hashes)
-		if err != nil {
-			return nil, fmt.Errorf("failed to get blob sidecars for L1BlockRef %s: %w", ref, err)
-		}
-		blobs, err := blobsFromSidecars(blobSidecars, hashes)
-		if err != nil {
-			return nil, fmt.Errorf("failed to get blobs from sidecars for L1BlockRef %s: %w", ref, err)
-		}
-		return blobs, nil
+		return nil, fmt.Errorf("get blobs from beacon client: %w", err)
 	}
 	if len(resp.Data) != len(hashes) {
 		return nil, fmt.Errorf("expected %d blobs but got %d", len(hashes), len(resp.Data))
 	}
-	var blobs []*eth.Blob
-	for i, blob := range resp.Data {
-		if err := verifyBlob(blob, hashes[i].Hash); err != nil {
-			return nil, fmt.Errorf("blob %d failed verification: %w", i, err)
+	// This function guarantees that the returned blobs will be ordered according to the provided
+	// hashes. The BeaconBlobs call above has a different ordering. From the getBlobs spec:
+	//   The returned blobs are ordered based on their kzg commitments in the block.
+	// https://ethereum.github.io/beacon-APIs/beacon-node-oapi.yaml
+	//
+	// This loop
+	//   1. verifies the integrity of each blob, and
+	//   2. rearranges the blobs to match the order of the provided hashes.
+	blobs := make([]*eth.Blob, len(hashes))
+	for _, blob := range resp.Data {
+		commitment, err := blob.ComputeKZGCommitment()
+		if err != nil {
+			return nil, fmt.Errorf("compute blob kzg commitment: %w", err)
+		}
+		got := eth.KZGToVersionedHash(commitment)
+		idx := slices.IndexFunc(hashes, func(indexedHash eth.IndexedBlobHash) bool {
+			return got == indexedHash.Hash
+		})
+		if idx == -1 {
+			return nil, fmt.Errorf("received a blob hash that does not match any expected hash: %s", got)
 		}
-		blobs = append(blobs, blob)
+		blobs[idx] = blob
 	}
 	return blobs, nil
 }

op-service/sources/l1_beacon_client_test.go

@@ -321,3 +321,75 @@ func TestVerifyBlob(t *testing.T) {
 	differentBlob[0] = byte(8)
 	require.Error(t, verifyBlob(&differentBlob, versionedHash))
 }
+
+func TestGetBlobs(t *testing.T) {
+	hash0, sidecar0 := makeTestBlobSidecar(0)
+	hash1, sidecar1 := makeTestBlobSidecar(1)
+	hash2, sidecar2 := makeTestBlobSidecar(2)
+
+	hashes := []eth.IndexedBlobHash{hash0, hash2, hash1} // Mix up the order.
+
+	invalidBlob0 := sidecar0.Blob
+	invalidBlob0[10]++
+
+	cases := []struct {
+		name           string
+		beaconBlobs    []*eth.Blob
+		expectFallback bool
+	}{
+		{
+			name: "happy path",
+			// From the /blobs/ spec:
+			//   Blobs are returned as an ordered list matching the order of their corresponding
+			//   KZG commitments in the block.
+			beaconBlobs:    []*eth.Blob{&sidecar0.Blob, &sidecar1.Blob, &sidecar2.Blob},
+			expectFallback: false,
+		},
+		{
+			name:           "fallback on client error",
+			beaconBlobs:    nil,
+			expectFallback: true,
+		},
+		{
+			name:           "fallback on invalid number of blobs",
+			beaconBlobs:    []*eth.Blob{&sidecar0.Blob},
+			expectFallback: true,
+		},
+		{
+			name:           "fallback on invalid blob",
+			beaconBlobs:    []*eth.Blob{&invalidBlob0, &sidecar1.Blob, &sidecar2.Blob},
+			expectFallback: true,
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			ctx := context.Background()
+			p := mocks.NewBeaconClient(t)
+			p.EXPECT().BeaconGenesis(ctx).Return(eth.APIGenesisResponse{Data: eth.ReducedGenesisData{GenesisTime: 10}}, nil)
+			p.EXPECT().ConfigSpec(ctx).Return(eth.APIConfigResponse{Data: eth.ReducedConfigData{SecondsPerSlot: 2}}, nil)
+			client := NewL1BeaconClient(p, L1BeaconClientConfig{})
+			ref := eth.L1BlockRef{Time: 12}
+
+			// construct the mock response for the beacon blobs call
+			var beaconBlobsResponse eth.APIBeaconBlobsResponse
+			var err error
+			if c.beaconBlobs == nil {
+				err = errors.New("client error")
+			} else {
+				beaconBlobsResponse = eth.APIBeaconBlobsResponse{Data: c.beaconBlobs}
+			}
+			p.EXPECT().BeaconBlobs(ctx, uint64(1), hashes).Return(beaconBlobsResponse, err)
+
+			if c.expectFallback {
+				p.EXPECT().BeaconBlobSideCars(ctx, false, uint64(1), hashes).Return(eth.APIGetBlobSidecarsResponse{
+					Data: toAPISideCars([]*eth.BlobSidecar{sidecar0, sidecar1, sidecar2}),
+				}, nil)
+			}
+
+			resp, err := client.GetBlobs(ctx, ref, hashes)
+			require.NoError(t, err)
+			require.Equal(t, []*eth.Blob{&sidecar0.Blob, &sidecar2.Blob, &sidecar1.Blob}, resp)
+		})
+	}
+}