Handle skipped blobs side car for empty blobs in blobsSidecarsByRange (…

…#4936)

* Handle skipped blobs side car for empty blobs in blobsSidecarsByRange

* add another quick check

packages/beacon-node/src/network/network.ts

@@ -12,9 +12,10 @@ import {altair, eip4844, Epoch, phase0} from "@lodestar/types";
 import {IMetrics} from "../metrics/index.js";
 import {ChainEvent, IBeaconChain, IBeaconClock} from "../chain/index.js";
 import {BlockInput, BlockInputType, getBlockInput} from "../chain/blocks/types.js";
+import {ckzg} from "../util/kzg.js";
 import {INetworkOptions} from "./options.js";
 import {INetwork} from "./interface.js";
-import {ReqRespBeaconNode, ReqRespHandlers} from "./reqresp/index.js";
+import {ReqRespBeaconNode, ReqRespHandlers, doBeaconBlocksMaybeBlobsByRange} from "./reqresp/index.js";
 import {Eth2Gossipsub, getGossipHandlers, GossipHandlers, GossipTopicTypeMap, GossipType} from "./gossip/index.js";
 import {MetadataController} from "./metadata.js";
 import {FORK_EPOCH_LOOKAHEAD, getActiveForks} from "./forks.js";
@@ -227,46 +228,14 @@ export class Network implements INetwork {
     peerId: PeerId,
     request: phase0.BeaconBlocksByRangeRequest
   ): Promise<BlockInput[]> {
-    // TODO EIP-4844: Assumes all blocks in the same epoch
-    // TODO EIP-4844: Ensure all blocks are in the same epoch
-    if (this.config.getForkSeq(request.startSlot) < ForkSeq.eip4844) {
-      const blocks = await this.reqResp.beaconBlocksByRange(peerId, request);
-      return blocks.map((block) => getBlockInput.preEIP4844(this.config, block));
-    }
-
-    // Only request blobs if they are recent enough
-    else if (
-      computeEpochAtSlot(request.startSlot) >=
-      this.chain.clock.currentEpoch - this.config.MIN_EPOCHS_FOR_BLOBS_SIDECARS_REQUESTS
-    ) {
-      // TODO EIP-4844: Do two requests at once for blocks and blobs
-      const blocks = await this.reqResp.beaconBlocksByRange(peerId, request);
-      const blobsSidecars = await this.reqResp.blobsSidecarsByRange(peerId, request);
-
-      if (blocks.length !== blobsSidecars.length) {
-        throw Error(`blocks.length ${blocks.length} != blobsSidecars.length ${blobsSidecars.length}`);
-      }
-
-      const blockInputs: BlockInput[] = [];
-      for (let i = 0; i < blocks.length; i++) {
-        const block = blocks[i];
-        const blobsSidecar = blobsSidecars[i];
-
-        // TODO EIP-4844: Do more verification blob is for block
-        if (block.message.slot !== blobsSidecar.beaconBlockSlot) {
-          throw Error(`blob does not match block slot ${block.message.slot} != ${blobsSidecar.beaconBlockSlot}`);
-        }
-
-        blockInputs.push(getBlockInput.postEIP4844(this.config, block, blobsSidecar));
-      }
-      return blockInputs;
-    }
-
-    // Post EIP-4844 but old blobs
-    else {
-      const blocks = await this.reqResp.beaconBlocksByRange(peerId, request);
-      return blocks.map((block) => getBlockInput.postEIP4844OldBlobs(this.config, block));
-    }
+    return doBeaconBlocksMaybeBlobsByRange(
+      this.config,
+      this.reqResp,
+      peerId,
+      request,
+      this.clock.currentEpoch,
+      ckzg.computeAggregateKzgProof([])
+    );
   }
 
   async beaconBlocksMaybeBlobsByRoot(peerId: PeerId, request: phase0.BeaconBlocksByRootRequest): Promise<BlockInput[]> {

packages/beacon-node/src/network/reqresp/doBeaconBlocksMaybeBlobsByRange.ts

@@ -0,0 +1,87 @@
+import {PeerId} from "@libp2p/interface-peer-id";
+import {IBeaconConfig} from "@lodestar/config";
+import {eip4844, Epoch, phase0} from "@lodestar/types";
+import {ForkSeq} from "@lodestar/params";
+import {computeEpochAtSlot} from "@lodestar/state-transition";
+
+import {BlockInput, getBlockInput} from "../../chain/blocks/types.js";
+import {IReqRespBeaconNode} from "./interface.js";
+
+export async function doBeaconBlocksMaybeBlobsByRange(
+  config: IBeaconConfig,
+  reqResp: IReqRespBeaconNode,
+  peerId: PeerId,
+  request: phase0.BeaconBlocksByRangeRequest,
+  currentEpoch: Epoch,
+  // To make test life easy without loading ckzg
+  emptyKzgAggregatedProof: eip4844.BlobsSidecar["kzgAggregatedProof"]
+): Promise<BlockInput[]> {
+  // TODO EIP-4844: Assumes all blocks in the same epoch
+  // TODO EIP-4844: Ensure all blocks are in the same epoch
+  if (config.getForkSeq(request.startSlot) < ForkSeq.eip4844) {
+    const blocks = await reqResp.beaconBlocksByRange(peerId, request);
+    return blocks.map((block) => getBlockInput.preEIP4844(config, block));
+  }
+
+  // Only request blobs if they are recent enough
+  else if (computeEpochAtSlot(request.startSlot) >= currentEpoch - config.MIN_EPOCHS_FOR_BLOBS_SIDECARS_REQUESTS) {
+    // TODO EIP-4844: Do two requests at once for blocks and blobs
+    const blocks = await reqResp.beaconBlocksByRange(peerId, request);
+    const blobsSidecars = await reqResp.blobsSidecarsByRange(peerId, request);
+
+    const blockInputs: BlockInput[] = [];
+    let blobSideCarIndex = 0;
+    let lastMatchedSlot = -1;
+
+    // Match blobSideCar with the block as some blocks would have no blobs and hence
+    // would be omitted from the response. If there are any inconsitencies in the
+    // response, the validations during import will reject the block and hence this
+    // entire segment.
+    //
+    // Assuming that the blocks and blobs will come in same sorted order
+    for (let i = 0; i < blocks.length; i++) {
+      const block = blocks[i];
+      let blobsSidecar: eip4844.BlobsSidecar;
+
+      if (blobsSidecars[blobSideCarIndex]?.beaconBlockSlot === block.message.slot) {
+        blobsSidecar = blobsSidecars[blobSideCarIndex];
+        lastMatchedSlot = block.message.slot;
+        blobSideCarIndex++;
+      } else {
+        // Quick inspect if the blobsSidecar was expected
+        const blobKzgCommitmentsLen = (block.message.body as eip4844.BeaconBlockBody).blobKzgCommitments.length;
+        if (blobKzgCommitmentsLen !== 0) {
+          throw Error(
+            `Missing blobsSidecar for blockSlot=${block.message.slot} with blobKzgCommitmentsLen=${blobKzgCommitmentsLen}`
+          );
+        }
+        blobsSidecar = {
+          beaconBlockRoot: config.getForkTypes(block.message.slot).BeaconBlock.hashTreeRoot(block.message),
+          beaconBlockSlot: block.message.slot,
+          blobs: [],
+          kzgAggregatedProof: emptyKzgAggregatedProof,
+        };
+      }
+      blockInputs.push(getBlockInput.postEIP4844(config, block, blobsSidecar));
+    }
+
+    // If there are still unconsumed blobs this means that the response was inconsistent
+    // and matching was wrong and hence we should throw error
+    if (blobsSidecars[blobSideCarIndex] !== undefined) {
+      throw Error(
+        `Unmatched blobsSidecars, blocks=${blocks.length}, blobs=${
+          blobsSidecars.length
+        } lastMatchedSlot=${lastMatchedSlot}, pending blobsSidecars slots=${blobsSidecars
+          .slice(blobSideCarIndex)
+          .map((blb) => blb.beaconBlockSlot)}`
+      );
+    }
+    return blockInputs;
+  }
+
+  // Post EIP-4844 but old blobs
+  else {
+    const blocks = await reqResp.beaconBlocksByRange(peerId, request);
+    return blocks.map((block) => getBlockInput.postEIP4844OldBlobs(config, block));
+  }
+}

packages/beacon-node/src/network/reqresp/index.ts

@@ -1,2 +1,3 @@
 export * from "./ReqRespBeaconNode.js";
 export * from "./interface.js";
+export * from "./doBeaconBlocksMaybeBlobsByRange.js";

packages/beacon-node/test/unit/network/doBeaconBlocksMaybeBlobsByRange.ts

@@ -0,0 +1,88 @@
+import sinon, {SinonStubbedInstance} from "sinon";
+import {expect} from "chai";
+import {peerIdFromString} from "@libp2p/peer-id";
+import {ssz, eip4844} from "@lodestar/types";
+import {createIBeaconConfig, createIChainForkConfig, defaultChainConfig} from "@lodestar/config";
+
+import {doBeaconBlocksMaybeBlobsByRange, ReqRespBeaconNode} from "../../../src/network/reqresp/index.js";
+import {BlockInputType} from "../../../src/chain/blocks/types.js";
+
+describe("doBeaconBlocksMaybeBlobsByRange", function () {
+  const sandbox = sinon.createSandbox();
+  const reqResp = sandbox.createStubInstance(ReqRespBeaconNode) as SinonStubbedInstance<ReqRespBeaconNode> &
+    ReqRespBeaconNode;
+  const peerId = peerIdFromString("Qma9T5YraSnpRDZqRR4krcSJabThc8nwZuJV3LercPHufi");
+
+  /* eslint-disable @typescript-eslint/naming-convention */
+  const chainConfig = createIChainForkConfig({
+    ...defaultChainConfig,
+    ALTAIR_FORK_EPOCH: 0,
+    BELLATRIX_FORK_EPOCH: 0,
+    CAPELLA_FORK_EPOCH: 0,
+    EIP4844_FORK_EPOCH: 0,
+  });
+  const genesisValidatorsRoot = Buffer.alloc(32, 0xaa);
+  const config = createIBeaconConfig(chainConfig, genesisValidatorsRoot);
+  const rangeRequest = ssz.phase0.BeaconBlocksByRangeRequest.defaultValue();
+  const emptyKzgAggregatedProof = ssz.eip4844.BlobsSidecar.defaultValue().kzgAggregatedProof;
+
+  const block1 = ssz.eip4844.SignedBeaconBlock.defaultValue();
+  block1.message.slot = 1;
+  const block2 = ssz.eip4844.SignedBeaconBlock.defaultValue();
+  block2.message.slot = 2;
+
+  const blobsSidecar1 = ssz.eip4844.BlobsSidecar.defaultValue();
+  blobsSidecar1.beaconBlockSlot = 1;
+  const blobsSidecar2 = ssz.eip4844.BlobsSidecar.defaultValue();
+  blobsSidecar2.beaconBlockSlot = 2;
+
+  // Array of testcases which are array of matched blocks with/without (if empty) sidecars
+  const testCases: [string, [eip4844.SignedBeaconBlock, eip4844.BlobsSidecar | undefined][]][] = [
+    ["one block with sidecar", [[block1, blobsSidecar1]]],
+    [
+      "two blocks with sidecar",
+      [
+        [block1, blobsSidecar1],
+        [block2, blobsSidecar2],
+      ],
+    ],
+    ["block with skipped sidecar", [[block1, undefined]]],
+  ];
+  testCases.map(([testName, blocksWithBlobs]) => {
+    it(testName, async () => {
+      const blocks = blocksWithBlobs.map(([block, _blobs]) => block as eip4844.SignedBeaconBlock);
+      const blobsSidecars = blocksWithBlobs
+        .map(([_block, blobs]) => blobs as eip4844.BlobsSidecar)
+        .filter((blobs) => blobs !== undefined);
+
+      const expectedResponse = blocksWithBlobs.map(([block, blobsSidecar]) => {
+        const blobs =
+          blobsSidecar !== undefined
+            ? blobsSidecar
+            : {
+                beaconBlockRoot: ssz.eip4844.BeaconBlock.hashTreeRoot(block.message),
+                beaconBlockSlot: block.message.slot,
+                blobs: [],
+                kzgAggregatedProof: emptyKzgAggregatedProof,
+              };
+        return {
+          type: BlockInputType.postEIP4844,
+          block,
+          blobs,
+        };
+      });
+      reqResp.beaconBlocksByRange.resolves(blocks);
+      reqResp.blobsSidecarsByRange.resolves(blobsSidecars);
+
+      const response = await doBeaconBlocksMaybeBlobsByRange(
+        config,
+        reqResp,
+        peerId,
+        rangeRequest,
+        0,
+        emptyKzgAggregatedProof
+      );
+      expect(response).to.be.deep.equal(expectedResponse);
+    });
+  });
+});