Use block hash as cache key in ExecutionLayerBlockProductionManager

beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java

@@ -38,6 +38,7 @@
 import tech.pegasys.teku.spec.datastructures.blocks.SignedBeaconBlock;
 import tech.pegasys.teku.spec.datastructures.blocks.SignedBeaconBlockUnblinder;
 import tech.pegasys.teku.spec.datastructures.blocks.SignedBlockContainer;
+import tech.pegasys.teku.spec.datastructures.blocks.SlotAndBlockRoot;
 import tech.pegasys.teku.spec.datastructures.blocks.blockbody.BeaconBlockBodyBuilder;
 import tech.pegasys.teku.spec.datastructures.blocks.versions.deneb.BlockContentsSchema;
 import tech.pegasys.teku.spec.datastructures.builder.BuilderBid;
@@ -60,6 +61,7 @@
 import tech.pegasys.teku.spec.datastructures.type.SszKZGCommitment;
 import tech.pegasys.teku.spec.datastructures.type.SszKZGProof;
 import tech.pegasys.teku.spec.executionlayer.ExecutionLayerBlockProductionManager;
+import tech.pegasys.teku.spec.executionlayer.ExecutionLayerBlockProductionManager.SlotAndExecutionBlockHash;
 import tech.pegasys.teku.spec.logic.versions.deneb.helpers.MiscHelpersDeneb;
 import tech.pegasys.teku.spec.schemas.SchemaDefinitions;
 import tech.pegasys.teku.spec.schemas.SchemaDefinitionsBellatrix;
@@ -240,12 +242,26 @@ private SafeFuture<Void> setExecutionData(
             blockProductionPerformance);
 
     return SafeFuture.allOf(
+        cacheExecutionPayloadResult(executionPayloadResult, blockSlotState),
         cacheExecutionPayloadValue(executionPayloadResult, blockSlotState),
         setPayloadOrPayloadHeader(bodyBuilder, executionPayloadResult),
         setKzgCommitments(bodyBuilder, schemaDefinitions, executionPayloadResult),
         setExecutionRequests(bodyBuilder, executionPayloadResult));
   }
 
+  private SafeFuture<Void> cacheExecutionPayloadResult(
+      final ExecutionPayloadResult executionPayloadResult, final BeaconState blockSlotState) {
+    return executionPayloadResult
+        .getExecutionBlockHashFuture()
+        .thenAccept(
+            executionBlockHash -> {
+              final SlotAndExecutionBlockHash key =
+                  new SlotAndExecutionBlockHash(blockSlotState.getSlot(), executionBlockHash);
+              executionLayerBlockProductionManager.cacheExecutionPayloadResult(
+                  key, executionPayloadResult);
+            });
+  }
+
   private SafeFuture<Void> cacheExecutionPayloadValue(
       final ExecutionPayloadResult executionPayloadResult, final BeaconState blockSlotState) {
     return executionPayloadResult
@@ -422,13 +438,19 @@ private ExecutionPayload getExecutionPayloadFromBuilderFlow(
   public Function<BeaconBlock, SafeFuture<BlobsBundle>> createBlobsBundleSelector() {
     return block -> {
       final UInt64 slot = block.getSlot();
+      final Bytes32 executionBlockHash =
+          block.getBody().getOptionalExecutionPayload().orElseThrow().getBlockHash();
+      final SlotAndExecutionBlockHash slotAndExecutionBlockHash =
+          new SlotAndExecutionBlockHash(slot, executionBlockHash);
       final ExecutionPayloadResult executionPayloadResult =
           executionLayerBlockProductionManager
-              .getCachedPayloadResult(slot)
+              .getCachedPayloadResult(slotAndExecutionBlockHash)
               .orElseThrow(
                   () ->
                       new IllegalStateException(
-                          "ExecutionPayloadResult hasn't been cached for slot " + slot));
+                          String.format(
+                              "ExecutionPayloadResult hasn't been cached for slot %s and execution block hash %s",
+                              slot, executionBlockHash)));
 
       if (executionPayloadResult.isFromLocalFlow()) {
         // we performed a non-blinded flow, so the bundle must be in
@@ -463,13 +485,16 @@ public Function<SignedBlockContainer, List<BlobSidecar>> createBlobSidecarsSelec
       if (blockContainer.isBlinded()) {
         // need to use the builder BlobsBundle or the local fallback for the blinded flow, because
         // the blobs and the proofs wouldn't be part of the BlockContainer.
+        final Bytes32 blockRoot = block.getRoot();
         final BuilderPayloadOrFallbackData builderPayloadOrFallbackData =
             executionLayerBlockProductionManager
-                .getCachedUnblindedPayload(block.getSlotAndBlockRoot())
+                .getCachedUnblindedPayload(new SlotAndBlockRoot(slot, blockRoot))
                 .orElseThrow(
                     () ->
                         new IllegalStateException(
-                            "BuilderPayloadOrFallbackData hasn't been cached for slot " + slot));
+                            String.format(
+                                "BuilderPayloadOrFallbackData hasn't been cached for slot %s and block root %s",
+                                slot, blockRoot)));
 
         final Optional<BuilderPayload> maybeBuilderPayload =
             builderPayloadOrFallbackData.getBuilderPayload();

beacon/validator/src/test/java/tech/pegasys/teku/validator/coordinator/BlockFactoryDenebTest.java

@@ -29,7 +29,6 @@
 import tech.pegasys.teku.spec.datastructures.blocks.BeaconBlock;
 import tech.pegasys.teku.spec.datastructures.blocks.BlockContainer;
 import tech.pegasys.teku.spec.datastructures.blocks.SignedBeaconBlock;
-import tech.pegasys.teku.spec.datastructures.blocks.SignedBlockContainer;
 import tech.pegasys.teku.spec.datastructures.blocks.versions.deneb.BlockContents;
 import tech.pegasys.teku.spec.datastructures.execution.BlobsBundle;
 import tech.pegasys.teku.spec.datastructures.type.SszKZGCommitment;
@@ -153,13 +152,13 @@ void shouldCreateValidBlobSidecarsForBlindedBlock() {
 
     final BlockAndBlobSidecars blockAndBlobSidecars = createBlockAndBlobSidecars(true, spec);
 
-    final SignedBlockContainer block = blockAndBlobSidecars.block();
+    final SignedBeaconBlock block = blockAndBlobSidecars.block().getSignedBlock();
     final List<BlobSidecar> blobSidecars = blockAndBlobSidecars.blobSidecars();
 
     verify(executionLayer).getCachedUnblindedPayload(block.getSlotAndBlockRoot());
 
     final SszList<SszKZGCommitment> expectedCommitments =
-        block.getSignedBlock().getMessage().getBody().getOptionalBlobKzgCommitments().orElseThrow();
+        block.getMessage().getBody().getOptionalBlobKzgCommitments().orElseThrow();
 
     assertThat(blobSidecars).hasSize(blobsCount).hasSameSizeAs(expectedCommitments);
 

beacon/validator/src/test/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactoryTest.java

@@ -82,6 +82,7 @@
 import tech.pegasys.teku.spec.datastructures.type.SszKZGCommitment;
 import tech.pegasys.teku.spec.datastructures.type.SszKZGProof;
 import tech.pegasys.teku.spec.executionlayer.ExecutionLayerBlockProductionManager;
+import tech.pegasys.teku.spec.executionlayer.ExecutionLayerBlockProductionManager.SlotAndExecutionBlockHash;
 import tech.pegasys.teku.spec.logic.versions.capella.operations.validation.BlsToExecutionChangesValidator.BlsToExecutionChangeInvalidReason;
 import tech.pegasys.teku.spec.logic.versions.deneb.helpers.MiscHelpersDeneb;
 import tech.pegasys.teku.spec.logic.versions.phase0.operations.validation.AttesterSlashingValidator.AttesterSlashingInvalidReason;
@@ -741,7 +742,7 @@ void shouldGetBlobsBundleForLocallyProducedBlocks() {
     // the BlobsBundle is stored in the ExecutionPayloadResult
     prepareCachedPayloadResult(
         block.getSlot(),
-        dataStructureUtil.randomExecutionPayload(),
+        block.getBody().getOptionalExecutionPayload().orElseThrow(),
         dataStructureUtil.randomPayloadExecutionContext(false),
         expectedBlobsBundle);
 
@@ -759,7 +760,7 @@ void shouldGetBlobsBundleForLocallyProducedBlocksViaFallback() {
     // the BlobsBundle is stored in the header with fallback
     prepareCachedPayloadHeaderWithFallbackResult(
         block.getSlot(),
-        dataStructureUtil.randomExecutionPayload(),
+        block.getBody().getOptionalExecutionPayload().orElseThrow(),
         dataStructureUtil.randomPayloadExecutionContext(false),
         expectedBlobsBundle);
 
@@ -1257,7 +1258,8 @@ private void prepareCachedPayloadResult(
       final ExecutionPayload executionPayload,
       final ExecutionPayloadContext executionPayloadContext,
       final BlobsBundle blobsBundle) {
-    when(executionLayer.getCachedPayloadResult(slot))
+    when(executionLayer.getCachedPayloadResult(
+            new SlotAndExecutionBlockHash(slot, executionPayload.getBlockHash())))
         .thenReturn(
             Optional.of(
                 ExecutionPayloadResult.createForLocalFlow(
@@ -1277,7 +1279,8 @@ private void prepareCachedPayloadHeaderWithFallbackResult(
             new FallbackData(
                 new GetPayloadResponse(executionPayload, UInt256.ZERO, blobsBundle, false),
                 FallbackReason.SHOULD_OVERRIDE_BUILDER_FLAG_IS_TRUE));
-    when(executionLayer.getCachedPayloadResult(slot))
+    when(executionLayer.getCachedPayloadResult(
+            new SlotAndExecutionBlockHash(slot, executionPayload.getBlockHash())))
         .thenReturn(
             Optional.of(
                 ExecutionPayloadResult.createForBuilderFlow(

ethereum/executionlayer/src/main/java/tech/pegasys/teku/ethereum/executionlayer/ExecutionBuilderModule.java

@@ -24,6 +24,7 @@
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.apache.tuweni.bytes.Bytes32;
 import org.apache.tuweni.units.bigints.UInt256;
 import tech.pegasys.teku.bls.BLSPublicKey;
 import tech.pegasys.teku.ethereum.executionclient.BuilderClient;
@@ -49,6 +50,7 @@
 import tech.pegasys.teku.spec.datastructures.execution.FallbackReason;
 import tech.pegasys.teku.spec.datastructures.execution.GetPayloadResponse;
 import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconState;
+import tech.pegasys.teku.spec.executionlayer.ExecutionLayerBlockProductionManager.SlotAndExecutionBlockHash;
 
 public class ExecutionBuilderModule {
 
@@ -329,27 +331,37 @@ public SafeFuture<Void> builderRegisterValidators(
 
   public SafeFuture<BuilderPayloadOrFallbackData> builderGetPayload(
       final SignedBeaconBlock signedBeaconBlock,
-      final Function<UInt64, Optional<ExecutionPayloadResult>> getPayloadResultFunction) {
+      final Function<SlotAndExecutionBlockHash, Optional<ExecutionPayloadResult>>
+          getPayloadResultFunction) {
 
     Preconditions.checkArgument(signedBeaconBlock.isBlinded(), "SignedBeaconBlock must be blind");
 
     final UInt64 slot = signedBeaconBlock.getSlot();
 
-    final Optional<SafeFuture<BuilderBidOrFallbackData>> maybeProcessedSlot =
+    final Bytes32 executionBlockHash =
+        signedBeaconBlock
+            .getMessage()
+            .getBody()
+            .getOptionalExecutionPayloadHeader()
+            .orElseThrow()
+            .getBlockHash();
+
+    final Optional<SafeFuture<BuilderBidOrFallbackData>> maybeProcessed =
         getPayloadResultFunction
-            .apply(slot)
+            .apply(new SlotAndExecutionBlockHash(slot, executionBlockHash))
             .flatMap(ExecutionPayloadResult::getBuilderBidOrFallbackDataFuture);
 
-    if (maybeProcessedSlot.isEmpty()) {
-      LOG.warn(
-          "Blinded block seems to not be built via either builder or local EL. Trying to unblind it via builder endpoint anyway.");
-      return getPayloadFromBuilder(signedBeaconBlock);
-    }
-
-    final SafeFuture<BuilderBidOrFallbackData> builderBidOrFallbackDataFuture =
-        maybeProcessedSlot.get();
-
-    return getPayloadFromBuilderOrFallbackData(signedBeaconBlock, builderBidOrFallbackDataFuture);
+    return maybeProcessed
+        .map(
+            builderBidOrFallbackDataFuture ->
+                getPayloadFromBuilderOrFallbackData(
+                    signedBeaconBlock, builderBidOrFallbackDataFuture))
+        .orElseGet(
+            () -> {
+              LOG.warn(
+                  "Blinded block seems to not be built via either builder or local EL. Trying to unblind it via builder endpoint anyway.");
+              return getPayloadFromBuilder(signedBeaconBlock);
+            });
   }
 
   private boolean isTransitionNotFinalized(final ExecutionPayloadContext executionPayloadContext) {

ethereum/executionlayer/src/main/java/tech/pegasys/teku/ethereum/executionlayer/ExecutionLayerBlockProductionManagerImpl.java

@@ -13,9 +13,9 @@
 
 package tech.pegasys.teku.ethereum.executionlayer;
 
-import java.util.NavigableMap;
+import java.util.Map;
 import java.util.Optional;
-import java.util.concurrent.ConcurrentSkipListMap;
+import java.util.concurrent.ConcurrentHashMap;
 import tech.pegasys.teku.ethereum.events.SlotEventsChannel;
 import tech.pegasys.teku.ethereum.performance.trackers.BlockProductionPerformance;
 import tech.pegasys.teku.ethereum.performance.trackers.BlockPublishingPerformance;
@@ -38,11 +38,11 @@ public class ExecutionLayerBlockProductionManagerImpl
   private static final UInt64 EXECUTION_RESULT_CACHE_RETENTION_SLOTS = UInt64.valueOf(2);
   private static final UInt64 BUILDER_RESULT_CACHE_RETENTION_SLOTS = UInt64.valueOf(2);
 
-  private final NavigableMap<UInt64, ExecutionPayloadResult> executionResultCache =
-      new ConcurrentSkipListMap<>();
+  private final Map<SlotAndExecutionBlockHash, ExecutionPayloadResult> executionResultCache =
+      new ConcurrentHashMap<>();
 
-  private final NavigableMap<SlotAndBlockRoot, BuilderPayloadOrFallbackData> builderResultCache =
-      new ConcurrentSkipListMap<>();
+  private final Map<SlotAndBlockRoot, BuilderPayloadOrFallbackData> builderResultCache =
+      new ConcurrentHashMap<>();
 
   private final ExecutionLayerChannel executionLayerChannel;
 
@@ -54,15 +54,21 @@ public ExecutionLayerBlockProductionManagerImpl(
   @Override
   public void onSlot(final UInt64 slot) {
     executionResultCache
-        .headMap(slot.minusMinZero(EXECUTION_RESULT_CACHE_RETENTION_SLOTS), false)
-        .clear();
-    final UInt64 slotMax = slot.minusMinZero(BUILDER_RESULT_CACHE_RETENTION_SLOTS);
-    builderResultCache.keySet().removeIf(key -> key.getSlot().isLessThan(slotMax));
+        .keySet()
+        .removeIf(
+            key ->
+                key.slot().isLessThan(slot.minusMinZero(EXECUTION_RESULT_CACHE_RETENTION_SLOTS)));
+    builderResultCache
+        .keySet()
+        .removeIf(
+            key ->
+                key.getSlot().isLessThan(slot.minusMinZero(BUILDER_RESULT_CACHE_RETENTION_SLOTS)));
   }
 
   @Override
-  public Optional<ExecutionPayloadResult> getCachedPayloadResult(final UInt64 slot) {
-    return Optional.ofNullable(executionResultCache.get(slot));
+  public Optional<ExecutionPayloadResult> getCachedPayloadResult(
+      final SlotAndExecutionBlockHash slotAndExecutionBlockHash) {
+    return Optional.ofNullable(executionResultCache.get(slotAndExecutionBlockHash));
   }
 
   @Override
@@ -72,16 +78,19 @@ public ExecutionPayloadResult initiateBlockProduction(
       final boolean attemptBuilderFlow,
       final Optional<UInt64> requestedBuilderBoostFactor,
       final BlockProductionPerformance blockProductionPerformance) {
-    final ExecutionPayloadResult result;
     if (attemptBuilderFlow) {
-      result =
-          executeBuilderFlow(
-              context, blockSlotState, requestedBuilderBoostFactor, blockProductionPerformance);
+      return executeBuilderFlow(
+          context, blockSlotState, requestedBuilderBoostFactor, blockProductionPerformance);
     } else {
-      result = executeLocalFlow(context, blockSlotState, blockProductionPerformance);
+      return executeLocalFlow(context, blockSlotState, blockProductionPerformance);
     }
-    executionResultCache.put(blockSlotState.getSlot(), result);
-    return result;
+  }
+
+  @Override
+  public void cacheExecutionPayloadResult(
+      final SlotAndExecutionBlockHash slotAndExecutionBlockHash,
+      final ExecutionPayloadResult executionPayloadResult) {
+    executionResultCache.put(slotAndExecutionBlockHash, executionPayloadResult);
   }
 
   @Override

ethereum/executionlayer/src/main/java/tech/pegasys/teku/ethereum/executionlayer/ExecutionLayerManagerImpl.java

@@ -58,6 +58,7 @@
 import tech.pegasys.teku.spec.datastructures.execution.NewPayloadRequest;
 import tech.pegasys.teku.spec.datastructures.execution.PowBlock;
 import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconState;
+import tech.pegasys.teku.spec.executionlayer.ExecutionLayerBlockProductionManager.SlotAndExecutionBlockHash;
 import tech.pegasys.teku.spec.executionlayer.ForkChoiceState;
 import tech.pegasys.teku.spec.executionlayer.ForkChoiceUpdatedResult;
 import tech.pegasys.teku.spec.executionlayer.PayloadBuildingAttributes;
@@ -239,7 +240,8 @@ public SafeFuture<Void> builderRegisterValidators(
   @Override
   public SafeFuture<BuilderPayloadOrFallbackData> builderGetPayload(
       final SignedBeaconBlock signedBeaconBlock,
-      final Function<UInt64, Optional<ExecutionPayloadResult>> getCachedPayloadResultFunction) {
+      final Function<SlotAndExecutionBlockHash, Optional<ExecutionPayloadResult>>
+          getCachedPayloadResultFunction) {
     return executionBuilderModule.builderGetPayload(
         signedBeaconBlock, getCachedPayloadResultFunction);
   }