Add consistency check for the builder BlobsBundle (Consensys#7736)

sicw · Nov 21, 2023 · 6f83c43 · 6f83c43
1 parent 982c442
commit 6f83c43
Show file tree

Hide file tree

Showing 10 changed files with 58 additions and 73 deletions.
diff --git a/.../src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java b/.../src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java
@@ -13,6 +13,7 @@
 
 package tech.pegasys.teku.validator.coordinator;
 
+import com.google.common.base.Preconditions;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Optional;
@@ -339,6 +340,8 @@ public Function<BeaconBlock, SafeFuture<BlobsBundle>> createBlobsBundleSelector(
   public Function<SignedBlockContainer, List<BlobSidecar>> createBlobSidecarsSelector() {
     return blockContainer -> {
       final UInt64 slot = blockContainer.getSlot();
+      final SignedBeaconBlock block = blockContainer.getSignedBlock();
+
       final MiscHelpersDeneb miscHelpersDeneb =
           MiscHelpersDeneb.required(spec.atSlot(slot).miscHelpers());
 
@@ -350,31 +353,24 @@ public Function<SignedBlockContainer, List<BlobSidecar>> createBlobSidecarsSelec
         // blobs and the proofs wouldn't be part of the BlockContainer
         final tech.pegasys.teku.spec.datastructures.builder.BlobsBundle blobsBundle =
             getCachedBuilderBlobsBundle(slot);
+        // consistency check because the BlobsBundle comes from an external source (a builder)
+        final SszList<SszKZGCommitment> blockCommitments =
+            block.getMessage().getBody().getOptionalBlobKzgCommitments().orElseThrow();
+        Preconditions.checkState(
+            blobsBundle.getCommitments().hashTreeRoot().equals(blockCommitments.hashTreeRoot()),
+            "Commitments in the builder BlobsBundle don't match the commitments in the block");
         blobs = blobsBundle.getBlobs();
         proofs = blobsBundle.getProofs();
       } else {
-        blobs =
-            blockContainer
-                .getBlobs()
-                .orElseThrow(
-                    () ->
-                        new IllegalStateException("Blobs are not available in " + blockContainer));
-        proofs =
-            blockContainer
-                .getKzgProofs()
-                .orElseThrow(
-                    () ->
-                        new IllegalStateException("Proofs are not available in " + blockContainer));
+        blobs = blockContainer.getBlobs().orElseThrow();
+        proofs = blockContainer.getKzgProofs().orElseThrow();
       }
 
       return IntStream.range(0, blobs.size())
           .mapToObj(
               index ->
                   miscHelpersDeneb.constructBlobSidecar(
-                      blockContainer.getSignedBlock(),
-                      UInt64.valueOf(index),
-                      blobs.get(index),
-                      proofs.get(index)))
+                      block, UInt64.valueOf(index), blobs.get(index), proofs.get(index)))
           .toList();
     };
   }

diff --git a/.../test/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactoryTest.java b/.../test/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactoryTest.java
@@ -14,6 +14,7 @@
 package tech.pegasys.teku.validator.coordinator;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
@@ -546,19 +547,37 @@ void shouldCreateBlobSidecarsForBlockContents() {
             });
   }
 
+  @Test
+  void shouldFailCreatingBlobSidecarsIfBuilderBlobsBundleIsNotConsistent() {
+    final SszList<SszKZGCommitment> commitments = dataStructureUtil.randomBlobKzgCommitments(3);
+    final SignedBeaconBlock signedBlindedBeaconBlock =
+        dataStructureUtil.randomSignedBlindedBeaconBlockWithCommitments(commitments);
+
+    final tech.pegasys.teku.spec.datastructures.builder.BlobsBundle blobsBundle =
+        dataStructureUtil.randomBuilderBlobsBundle(3);
+
+    prepareCachedBuilderPayload(
+        signedBlindedBeaconBlock.getSlot(),
+        dataStructureUtil.randomExecutionPayload(),
+        Optional.of(blobsBundle));
+
+    assertThatThrownBy(() -> factory.createBlobSidecarsSelector().apply(signedBlindedBeaconBlock))
+        .isInstanceOf(IllegalStateException.class)
+        .hasMessage(
+            "Commitments in the builder BlobsBundle don't match the commitments in the block");
+  }
+
   @Test
   void shouldCreateBlobSidecarsForBlindedBlock() {
+    final SszList<SszKZGCommitment> commitments = dataStructureUtil.randomBlobKzgCommitments(3);
     final SignedBeaconBlock signedBlindedBeaconBlock =
-        dataStructureUtil.randomSignedBlindedBeaconBlock();
+        dataStructureUtil.randomSignedBlindedBeaconBlockWithCommitments(commitments);
 
     final MiscHelpersDeneb miscHelpersDeneb =
         MiscHelpersDeneb.required(spec.atSlot(signedBlindedBeaconBlock.getSlot()).miscHelpers());
 
-    final int commitmentsCount =
-        miscHelpersDeneb.getBlobKzgCommitmentsCount(signedBlindedBeaconBlock);
-
     final tech.pegasys.teku.spec.datastructures.builder.BlobsBundle blobsBundle =
-        dataStructureUtil.randomBuilderBlobsBundle(commitmentsCount);
+        dataStructureUtil.randomBuilderBlobsBundle(commitments);
 
     prepareCachedBuilderPayload(
         signedBlindedBeaconBlock.getSlot(),

diff --git a/...erenceTest/java/tech/pegasys/teku/reference/phase0/forkchoice/StubBlobSidecarManager.java b/...erenceTest/java/tech/pegasys/teku/reference/phase0/forkchoice/StubBlobSidecarManager.java
@@ -27,7 +27,6 @@
 import tech.pegasys.teku.kzg.KZGProof;
 import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.Blob;
 import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar;
-import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecarOld;
 import tech.pegasys.teku.spec.datastructures.blocks.SignedBeaconBlock;
 import tech.pegasys.teku.spec.datastructures.blocks.blockbody.versions.deneb.BeaconBlockBodyDeneb;
 import tech.pegasys.teku.spec.datastructures.type.SszKZGCommitment;
@@ -60,11 +59,6 @@ public SafeFuture<InternalValidationResult> validateAndPrepareForBlockImport(
         new UnsupportedOperationException("Not available in fork choice reference tests"));
   }
 
-  @Override
-  public void prepareForBlockImport(final BlobSidecarOld blobSidecar) {
-    // NOOP
-  }
-
   @Override
   public void prepareForBlockImport(final BlobSidecar blobSidecar) {
     // NOOP

diff --git a/...layer/src/main/java/tech/pegasys/teku/ethereum/executionlayer/ExecutionBuilderModule.java b/...layer/src/main/java/tech/pegasys/teku/ethereum/executionlayer/ExecutionBuilderModule.java
@@ -392,8 +392,8 @@ private SafeFuture<BuilderPayload> getPayloadFromBuilderOrFallbackData(
       final SafeFuture<HeaderWithFallbackData> headerWithFallbackDataFuture) {
     // note: we don't do any particular consistency check here.
     // the header/payload compatibility check is done by SignedBeaconBlockUnblinder
-    // TODO: clarify where is blobs bundle compatibility check done
-    // the blobs bundle compatibility is done by ...
+    // the blobs bundle compatibility check is done by
+    // BlockOperationSelectorFactory#createBlobSidecarsSelector
     return headerWithFallbackDataFuture.thenCompose(
         headerWithFallbackData -> {
           if (headerWithFallbackData.getFallbackData().isEmpty()) {

diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/execution/BlobsBundle.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/execution/BlobsBundle.java
@@ -13,6 +13,8 @@
 
 package tech.pegasys.teku.spec.datastructures.execution;
 
+import static com.google.common.base.Preconditions.checkArgument;
+
 import com.google.common.base.MoreObjects;
 import java.util.List;
 import java.util.Objects;
@@ -28,6 +30,16 @@ public class BlobsBundle {
 
   public BlobsBundle(
       final List<KZGCommitment> commitments, final List<KZGProof> proofs, final List<Blob> blobs) {
+    checkArgument(
+        commitments.size() == blobs.size(),
+        "Expected %s commitments but got %s",
+        blobs.size(),
+        commitments.size());
+    checkArgument(
+        proofs.size() == blobs.size(),
+        "Expected %s proofs but got %s",
+        blobs.size(),
+        proofs.size());
     this.commitments = commitments;
     this.proofs = proofs;
     this.blobs = blobs;

diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/helpers/MiscHelpers.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/helpers/MiscHelpers.java
@@ -39,7 +39,6 @@
 import tech.pegasys.teku.spec.config.SpecConfig;
 import tech.pegasys.teku.spec.constants.NetworkConstants;
 import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar;
-import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecarOld;
 import tech.pegasys.teku.spec.datastructures.blocks.BeaconBlock;
 import tech.pegasys.teku.spec.datastructures.state.ForkData;
 import tech.pegasys.teku.spec.datastructures.state.SigningData;
@@ -345,11 +344,6 @@ public boolean isExecutionEnabled(final BeaconState genericState, final BeaconBl
     return false;
   }
 
-  @Deprecated
-  public boolean verifyBlobKzgProof(final KZG kzg, final BlobSidecarOld blobSidecar) {
-    return false;
-  }
-
   public boolean verifyBlobKzgProof(final KZG kzg, final BlobSidecar blobSidecar) {
     return false;
   }

diff --git a/...c/src/main/java/tech/pegasys/teku/spec/logic/versions/deneb/helpers/MiscHelpersDeneb.java b/...c/src/main/java/tech/pegasys/teku/spec/logic/versions/deneb/helpers/MiscHelpersDeneb.java
@@ -34,7 +34,6 @@
 import tech.pegasys.teku.spec.config.SpecConfigDeneb;
 import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.Blob;
 import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar;
-import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecarOld;
 import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecarSchema;
 import tech.pegasys.teku.spec.datastructures.blocks.BeaconBlock;
 import tech.pegasys.teku.spec.datastructures.blocks.SignedBeaconBlock;
@@ -74,23 +73,6 @@ public MiscHelpersDeneb(
     this.blobSidecarSchema = schemaDefinitions.getBlobSidecarSchema();
   }
 
-  /**
-   * Performs <a
-   * href="https://github.com/ethereum/consensus-specs/blob/dev/specs/deneb/polynomial-commitments.md#verify_blob_kzg_proof">verify_blob_kzg_proof</a>
-   * on the given blob sidecar
-   *
-   * @param kzg the kzg implementation which will be used for verification
-   * @param blobSidecar blob sidecar to verify
-   * @return true if blob sidecar is valid
-   */
-  @Override
-  public boolean verifyBlobKzgProof(final KZG kzg, final BlobSidecarOld blobSidecar) {
-    return kzg.verifyBlobKzgProof(
-        blobSidecar.getBlob().getBytes(),
-        blobSidecar.getKZGCommitment(),
-        blobSidecar.getKZGProof());
-  }
-
   /**
    * Performs <a
    * href="https://github.com/ethereum/consensus-specs/blob/dev/specs/deneb/polynomial-commitments.md#verify_blob_kzg_proof">verify_blob_kzg_proof</a>

diff --git a/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java b/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java
@@ -2226,10 +2226,6 @@ public BlobSidecarOld randomBlobSidecarOld() {
     return new RandomBlobSidecarOldBuilder().build();
   }
 
-  public BlobSidecar randomBlobSidecar(final long index) {
-    return new RandomBlobSidecarBuilder().index(UInt64.valueOf(index)).build();
-  }
-
   public BlobSidecar randomBlobSidecarForBlock(
       final SignedBeaconBlock signedBeaconBlock, final long index) {
     return new RandomBlobSidecarBuilder()
@@ -2287,15 +2283,20 @@ public List<BlobIdentifier> randomBlobIdentifiers(final int count) {
 
   public tech.pegasys.teku.spec.datastructures.builder.BlobsBundle randomBuilderBlobsBundle(
       final int count) {
+    return randomBuilderBlobsBundle(randomBlobKzgCommitments(count));
+  }
+
+  public tech.pegasys.teku.spec.datastructures.builder.BlobsBundle randomBuilderBlobsBundle(
+      final SszList<SszKZGCommitment> commitments) {
     final UInt64 slot = randomSlot();
     final SchemaDefinitionsDeneb schemaDefinitions = getDenebSchemaDefinitions(slot);
     final BlobsBundleSchema schema = schemaDefinitions.getBlobsBundleSchema();
 
     return new tech.pegasys.teku.spec.datastructures.builder.BlobsBundle(
         schema,
-        randomSszList(schema.getCommitmentsSchema(), this::randomSszKZGCommitment, count),
-        randomSszList(schema.getProofsSchema(), this::randomSszKZGProof, count),
-        randomSszList(schema.getBlobsSchema(), this::randomBlob, count));
+        commitments,
+        randomSszList(schema.getProofsSchema(), this::randomSszKZGProof, commitments.size()),
+        randomSszList(schema.getBlobsSchema(), this::randomBlob, commitments.size()));
   }
 
   public tech.pegasys.teku.spec.datastructures.builder.BlobsBundle randomBuilderBlobsBundle() {

diff --git a/...etransition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobSidecarManager.java b/...etransition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobSidecarManager.java
@@ -18,7 +18,6 @@
 import tech.pegasys.teku.infrastructure.async.SafeFuture;
 import tech.pegasys.teku.infrastructure.unsigned.UInt64;
 import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar;
-import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecarOld;
 import tech.pegasys.teku.spec.datastructures.blocks.SignedBeaconBlock;
 import tech.pegasys.teku.spec.logic.versions.deneb.blobs.BlobSidecarsAndValidationResult;
 import tech.pegasys.teku.spec.logic.versions.deneb.blobs.BlobSidecarsAvailabilityChecker;
@@ -37,9 +36,6 @@ public SafeFuture<InternalValidationResult> validateAndPrepareForBlockImport(
         @Override
         public void prepareForBlockImport(final BlobSidecar blobSidecar) {}
 
-        @Override
-        public void prepareForBlockImport(final BlobSidecarOld blobSidecar) {}
-
         @Override
         public void subscribeToReceivedBlobSidecar(
             final ReceivedBlobSidecarListener receivedBlobSidecarListener) {}
@@ -67,9 +63,6 @@ SafeFuture<InternalValidationResult> validateAndPrepareForBlockImport(
 
   void prepareForBlockImport(BlobSidecar blobSidecar);
 
-  @Deprecated
-  void prepareForBlockImport(BlobSidecarOld blobSidecar);
-
   void subscribeToReceivedBlobSidecar(ReceivedBlobSidecarListener receivedBlobSidecarListener);
 
   boolean isAvailabilityRequiredAtSlot(UInt64 slot);

diff --git a/...nsition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobSidecarManagerImpl.java b/...nsition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobSidecarManagerImpl.java
@@ -25,7 +25,6 @@
 import tech.pegasys.teku.kzg.KZG;
 import tech.pegasys.teku.spec.Spec;
 import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar;
-import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecarOld;
 import tech.pegasys.teku.spec.datastructures.blocks.SignedBeaconBlock;
 import tech.pegasys.teku.spec.logic.versions.deneb.blobs.BlobSidecarsAndValidationResult;
 import tech.pegasys.teku.spec.logic.versions.deneb.blobs.BlobSidecarsAvailabilityChecker;
@@ -108,11 +107,6 @@ public void prepareForBlockImport(final BlobSidecar blobSidecar) {
     receivedBlobSidecarSubscribers.forEach(s -> s.onBlobSidecarReceived(blobSidecar));
   }
 
-  @Override
-  public void prepareForBlockImport(final BlobSidecarOld blobSidecar) {
-    throw new RuntimeException("Deprecated");
-  }
-
   @Override
   public void subscribeToReceivedBlobSidecar(
       final ReceivedBlobSidecarListener receivedBlobSidecarListener) {