Unhide builder bid compare factor and fix win logging (#6974)

---------

Co-authored-by: Enrico Del Fante <[email protected]>

CHANGELOG.md

@@ -22,6 +22,7 @@ For information on changes in released versions of Teku, see the [releases page]
 - Added mainnet configuration for CAPELLA network fork due epoch 194048, April 12, 2023; 10:27:35pm UTC
 - Added experimental feature `--Xdeposit-snapshot-enabled` to use bundled deposit contract tree snapshot and persist it after finalization to decrease EL pressure and speed up node startup
 - Added `--p2p-discovery-site-local-addresses-enabled` option to allow discovery connections to local (RFC1918) addresses.
+- Added `--builder-bid-compare-factor` to be applied to the builder bid value when comparing it with locally produced payload.
 
 
 ### Bug Fixes

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

@@ -51,6 +51,7 @@ public class ExecutionBuilderModule {
 
   private static final Logger LOG = LogManager.getLogger();
   private static final int HUNDRED_PERCENT = 100;
+  private static final UInt256 WEI_TO_GWEI = UInt256.valueOf(10).pow(9);
 
   private final Spec spec;
   private final AtomicBoolean latestBuilderAvailability;
@@ -59,7 +60,7 @@ public class ExecutionBuilderModule {
   private final BuilderCircuitBreaker builderCircuitBreaker;
   private final Optional<BuilderClient> builderClient;
   private final EventLogger eventLogger;
-  private final Optional<Integer> builderBidChallengePercentage;
+  private final Optional<Integer> builderBidCompareFactor;
 
   public ExecutionBuilderModule(
       final Spec spec,
@@ -68,15 +69,15 @@ public ExecutionBuilderModule(
       final BuilderCircuitBreaker builderCircuitBreaker,
       final Optional<BuilderClient> builderClient,
       final EventLogger eventLogger,
-      final Optional<Integer> builderBidChallengePercentage) {
+      final Optional<Integer> builderBidCompareFactor) {
     this.spec = spec;
     this.latestBuilderAvailability = new AtomicBoolean(builderClient.isPresent());
     this.executionLayerManager = executionLayerManager;
     this.builderBidValidator = builderBidValidator;
     this.builderCircuitBreaker = builderCircuitBreaker;
     this.builderClient = builderClient;
     this.eventLogger = eventLogger;
-    this.builderBidChallengePercentage = builderBidChallengePercentage;
+    this.builderBidCompareFactor = builderBidCompareFactor;
   }
 
   private Optional<SafeFuture<HeaderWithFallbackData>> validateBuilderGetHeader(
@@ -167,15 +168,12 @@ public SafeFuture<HeaderWithFallbackData> builderGetHeader(
                     maybeLocalExecutionPayload
                         .map(ExecutionPayloadWithValue::getValue)
                         .orElse(UInt256.ZERO);
+                final UInt256 builderBidValue = signedBuilderBid.getMessage().getValue();
+
                 logReceivedBuilderBid(signedBuilderBid.getMessage());
 
-                if (isLocalPayloadValueWinning(signedBuilderBid, localPayloadValue)) {
-                  LOG.info(
-                      "The local execution payload value ({}) was awarded the block over the builder payload ({}), "
-                          + "{}% challenge configured.",
-                      signedBuilderBid.getMessage().getValue().toDecimalString(),
-                      localPayloadValue.toDecimalString(),
-                      builderBidChallengePercentage);
+                if (isLocalPayloadValueWinning(builderBidValue, localPayloadValue)) {
+                  logLocalPayloadWin(builderBidValue, localPayloadValue);
                   return getResultFromLocalExecutionPayload(
                       localExecutionPayload, slot, FallbackReason.LOCAL_BLOCK_VALUE_WON);
                 }
@@ -198,12 +196,10 @@ public SafeFuture<HeaderWithFallbackData> builderGetHeader(
 
   /** 1 ETH is 10^18 wei, Uint256 max is more than 10^77 */
   private boolean isLocalPayloadValueWinning(
-      final SignedBuilderBid signedBuilderBid, final UInt256 localPayloadValue) {
-    return builderBidChallengePercentage.isPresent()
-        && signedBuilderBid
-            .getMessage()
-            .getValue()
-            .multiply(builderBidChallengePercentage.get())
+      final UInt256 builderBidValue, final UInt256 localPayloadValue) {
+    return builderBidCompareFactor.isPresent()
+        && builderBidValue
+            .multiply(builderBidCompareFactor.get())
             .lessOrEqualThan(localPayloadValue.multiply(HUNDRED_PERCENT));
   }
 
@@ -412,4 +408,21 @@ private void logReceivedBuilderBid(final BuilderBid builderBid) {
         payloadHeader.getGasLimit(),
         payloadHeader.getGasUsed());
   }
+
+  private void logLocalPayloadWin(
+      final UInt256 builderPayloadValue, final UInt256 localPayloadValue) {
+    final Integer compareFactor = builderBidCompareFactor.orElseThrow();
+    if (compareFactor == HUNDRED_PERCENT) {
+      LOG.info(
+          "Local execution payload with value ({} Gwei) is chosen over builder payload ({} Gwei).",
+          localPayloadValue.divide(WEI_TO_GWEI).toDecimalString(),
+          builderPayloadValue.divide(WEI_TO_GWEI).toDecimalString());
+    } else {
+      LOG.info(
+          "Local execution payload with value ({} Gwei) is chosen over builder payload ({} Gwei, compare factor {}%).",
+          localPayloadValue.divide(WEI_TO_GWEI).toDecimalString(),
+          builderPayloadValue.divide(WEI_TO_GWEI).toDecimalString(),
+          compareFactor);
+    }
+  }
 }

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

@@ -83,7 +83,7 @@ public static ExecutionLayerManagerImpl create(
       final BuilderBidValidator builderBidValidator,
       final BuilderCircuitBreaker builderCircuitBreaker,
       final BlobsBundleValidator blobsBundleValidator,
-      final Optional<Integer> builderBidChallengePercentage) {
+      final Optional<Integer> builderBidCompareFactor) {
     final LabelledMetric<Counter> executionPayloadSourceCounter =
         metricsSystem.createLabelledCounter(
             TekuMetricCategory.BEACON,
@@ -112,7 +112,7 @@ public static ExecutionLayerManagerImpl create(
         builderCircuitBreaker,
         executionPayloadSourceCounter,
         blobsBundleValidator,
-        builderBidChallengePercentage);
+        builderBidCompareFactor);
   }
 
   public static ExecutionEngineClient createEngineClient(
@@ -154,7 +154,7 @@ private ExecutionLayerManagerImpl(
       final BuilderCircuitBreaker builderCircuitBreaker,
       final LabelledMetric<Counter> executionPayloadSourceCounter,
       final BlobsBundleValidator blobsBundleValidator,
-      final Optional<Integer> builderBidChallengePercentage) {
+      final Optional<Integer> builderBidCompareFactor) {
     this.executionClientHandler = executionClientHandler;
     this.spec = spec;
     this.blobsBundleValidator = blobsBundleValidator;
@@ -167,7 +167,7 @@ private ExecutionLayerManagerImpl(
             builderCircuitBreaker,
             builderClient,
             eventLogger,
-            builderBidChallengePercentage);
+            builderBidCompareFactor);
   }
 
   @Override

ethereum/executionlayer/src/test/java/tech/pegasys/teku/ethereum/executionlayer/ExecutionLayerManagerImplTest.java

@@ -260,7 +260,8 @@ public void builderGetHeaderGetPayload_shouldReturnHeaderAndPayloadViaEngineWhen
   }
 
   @Test
-  public void builderGetHeaderGetPayload_shouldReturnEnginePayloadWhenValueLowerButChallengeWon() {
+  public void
+      builderGetHeaderGetPayload_shouldReturnEnginePayloadWhenValueLowerButBetterWithFactor() {
     // Setup requires local payload to have at lest 50% value of builder's to win
     executionLayerManager = createExecutionLayerChannelImpl(true, false, Optional.of(50));
     setBuilderOnline();
@@ -296,7 +297,7 @@ public void builderGetHeaderGetPayload_shouldReturnEnginePayloadWhenValueLowerBu
   }
 
   @Test
-  public void builderGetHeaderGetPayload_shouldReturnBuilderPayloadWhenBuilderWonChallenge() {
+  public void builderGetHeaderGetPayload_shouldReturnBuilderPayloadWhenBuilderWonLocal() {
     // Setup requires local payload to have at lest 50% value of builder's to win
     executionLayerManager = createExecutionLayerChannelImpl(true, false, Optional.of(50));
     setBuilderOnline();
@@ -318,7 +319,8 @@ public void builderGetHeaderGetPayload_shouldReturnBuilderPayloadWhenBuilderWonC
   }
 
   @Test
-  public void builderGetHeaderGetPayload_shouldReturnBuilderPayloadWhenBuilderChallengeIsNever() {
+  public void
+      builderGetHeaderGetPayload_shouldReturnBuilderPayloadWhenBuilderFactorIsAlwaysBuilder() {
     // Setup will always ignore local payload in favor of Builder bid
     executionLayerManager = createExecutionLayerChannelImpl(true, false, Optional.empty());
     setBuilderOnline();
@@ -340,6 +342,40 @@ public void builderGetHeaderGetPayload_shouldReturnBuilderPayloadWhenBuilderChal
         .isCompletedWithValue(expectedResult);
   }
 
+  @Test
+  public void
+      builderGetHeaderGetPayload_shouldReturnLocalPayloadWhenBuilderFactorIsAlwaysBuilderAndBidValidationFails() {
+    // Setup will always ignore local payload in favor of Builder bid
+    executionLayerManager = createExecutionLayerChannelImpl(true, true, Optional.empty());
+    setBuilderOnline();
+
+    final ExecutionPayloadContext executionPayloadContext =
+        dataStructureUtil.randomPayloadExecutionContext(false, true);
+    final UInt64 slot = executionPayloadContext.getForkChoiceState().getHeadBlockSlot();
+    final BeaconState state = dataStructureUtil.randomBeaconState(slot);
+
+    prepareBuilderGetHeaderResponse(executionPayloadContext, false);
+    final ExecutionPayload payload =
+        prepareEngineGetPayloadResponse(executionPayloadContext, UInt256.ZERO, slot);
+
+    final ExecutionPayloadHeader header =
+        spec.getGenesisSpec()
+            .getSchemaDefinitions()
+            .toVersionBellatrix()
+            .orElseThrow()
+            .getExecutionPayloadHeaderSchema()
+            .createFromExecutionPayload(payload);
+
+    // we expect local engine header as result
+    final HeaderWithFallbackData expectedResult =
+        HeaderWithFallbackData.create(
+            header, new FallbackData(payload, FallbackReason.BUILDER_ERROR));
+    assertThat(executionLayerManager.builderGetHeader(executionPayloadContext, state))
+        .isCompletedWithValue(expectedResult);
+
+    verifyFallbackToLocalEL(slot, executionPayloadContext, expectedResult);
+  }
+
   @Test
   public void builderGetHeaderGetPayload_shouldReturnHeaderAndPayloadViaEngineOnBuilderFailure() {
     setBuilderOnline();
@@ -773,7 +809,7 @@ private ExecutionLayerManagerImpl createExecutionLayerChannelImpl(
   private ExecutionLayerManagerImpl createExecutionLayerChannelImpl(
       final boolean builderEnabled,
       final boolean builderValidatorEnabled,
-      final Optional<Integer> builderBidChallengePercentaqe) {
+      final Optional<Integer> builderBidCompareFactor) {
     when(builderCircuitBreaker.isEngaged(any())).thenReturn(false);
     return ExecutionLayerManagerImpl.create(
         eventLogger,
@@ -786,7 +822,7 @@ private ExecutionLayerManagerImpl createExecutionLayerChannelImpl(
             : BuilderBidValidator.NOOP,
         builderCircuitBreaker,
         BlobsBundleValidator.NOOP,
-        builderBidChallengePercentaqe);
+        builderBidCompareFactor);
   }
 
   private void updateBuilderStatus(final SafeFuture<Response<Void>> builderClientResponse) {

services/executionlayer/src/main/java/tech/pegasys/teku/services/executionlayer/ExecutionLayerConfiguration.java

@@ -20,18 +20,23 @@
 import java.util.Locale;
 import java.util.Optional;
 import org.apache.commons.lang3.StringUtils;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import tech.pegasys.teku.infrastructure.exceptions.InvalidConfigurationException;
 import tech.pegasys.teku.spec.Spec;
 import tech.pegasys.teku.spec.SpecMilestone;
 import tech.pegasys.teku.spec.executionlayer.ExecutionLayerChannel.Version;
 
 public class ExecutionLayerConfiguration {
+  private static final Logger LOG = LogManager.getLogger();
+
   public static final boolean DEFAULT_BUILDER_CIRCUIT_BREAKER_ENABLED = true;
   public static final int DEFAULT_BUILDER_CIRCUIT_BREAKER_WINDOW = 32;
   public static final int DEFAULT_BUILDER_CIRCUIT_BREAKER_ALLOWED_FAULTS = 5;
   public static final int DEFAULT_BUILDER_CIRCUIT_BREAKER_ALLOWED_CONSECUTIVE_FAULTS = 3;
   public static final int BUILDER_CIRCUIT_BREAKER_WINDOW_HARD_CAP = 64;
-  public static final int DEFAULT_BUILDER_BID_CHALLENGE_PERCENTAGE = 100;
+  public static final int DEFAULT_BUILDER_BID_COMPARE_FACTOR = 100;
+  public static final String BUILDER_ALWAYS_KEYWORD = "BUILDER_ALWAYS";
 
   private final Spec spec;
   private final Optional<String> engineEndpoint;
@@ -42,7 +47,7 @@ public class ExecutionLayerConfiguration {
   private final int builderCircuitBreakerWindow;
   private final int builderCircuitBreakerAllowedFaults;
   private final int builderCircuitBreakerAllowedConsecutiveFaults;
-  private final Optional<Integer> builderBidChallengePercentage;
+  private final Optional<Integer> builderBidCompareFactor;
   private final boolean exchangeCapabilitiesEnabled;
 
   private ExecutionLayerConfiguration(
@@ -55,7 +60,7 @@ private ExecutionLayerConfiguration(
       final int builderCircuitBreakerWindow,
       final int builderCircuitBreakerAllowedFaults,
       final int builderCircuitBreakerAllowedConsecutiveFaults,
-      final Optional<Integer> builderBidChallengePercentage,
+      final Optional<Integer> builderBidCompareFactor,
       final boolean exchangeCapabilitiesEnabled) {
     this.spec = spec;
     this.engineEndpoint = engineEndpoint;
@@ -67,7 +72,7 @@ private ExecutionLayerConfiguration(
     this.builderCircuitBreakerAllowedFaults = builderCircuitBreakerAllowedFaults;
     this.builderCircuitBreakerAllowedConsecutiveFaults =
         builderCircuitBreakerAllowedConsecutiveFaults;
-    this.builderBidChallengePercentage = builderBidChallengePercentage;
+    this.builderBidCompareFactor = builderBidCompareFactor;
     this.exchangeCapabilitiesEnabled = exchangeCapabilitiesEnabled;
   }
 
@@ -122,8 +127,8 @@ public boolean isExchangeCapabilitiesEnabled() {
     return exchangeCapabilitiesEnabled;
   }
 
-  public Optional<Integer> getBuilderBidChallengePercentage() {
-    return builderBidChallengePercentage;
+  public Optional<Integer> getBuilderBidCompareFactor() {
+    return builderBidCompareFactor;
   }
 
   public static class Builder {
@@ -137,17 +142,32 @@ public static class Builder {
     private int builderCircuitBreakerAllowedFaults = DEFAULT_BUILDER_CIRCUIT_BREAKER_ALLOWED_FAULTS;
     private int builderCircuitBreakerAllowedConsecutiveFaults =
         DEFAULT_BUILDER_CIRCUIT_BREAKER_ALLOWED_CONSECUTIVE_FAULTS;
-    private String builderBidChallengePercentage =
-        Integer.toString(DEFAULT_BUILDER_BID_CHALLENGE_PERCENTAGE);
+    private String builderBidCompareFactor = Integer.toString(DEFAULT_BUILDER_BID_COMPARE_FACTOR);
     private boolean exchangeCapabilitiesEnabled = false;
 
     private Builder() {}
 
     public ExecutionLayerConfiguration build() {
       validateStubEndpoints();
       validateBuilderCircuitBreaker();
-      Optional<Integer> builderChallengePercentage =
-          validateAndParseBuilderBidChallengePercentage();
+      final Optional<Integer> builderBidCompareFactor = validateAndParseBuilderBidCompareFactor();
+
+      if (builderEndpoint.isPresent()) {
+        if (builderBidCompareFactor.isEmpty()) {
+          LOG.info(
+              "During block production, a valid builder bid will always be chosen over locally produced payload.");
+        } else {
+          final String additionalHint =
+              builderBidCompareFactor.get() == DEFAULT_BUILDER_BID_COMPARE_FACTOR
+                  ? " Can be configured via --builder-bid-compare-factor"
+                  : "";
+          LOG.info(
+              "During block production, locally produced payload will be chosen when its value is equal or greater than {}% of the builder bid value."
+                  + additionalHint,
+              builderBidCompareFactor.get());
+        }
+      }
+
       return new ExecutionLayerConfiguration(
           spec,
           engineEndpoint,
@@ -158,7 +178,7 @@ public ExecutionLayerConfiguration build() {
           builderCircuitBreakerWindow,
           builderCircuitBreakerAllowedFaults,
           builderCircuitBreakerAllowedConsecutiveFaults,
-          builderChallengePercentage,
+          builderBidCompareFactor,
           exchangeCapabilitiesEnabled);
     }
 
@@ -210,8 +230,8 @@ public Builder builderEndpoint(final String builderEndpoint) {
       return this;
     }
 
-    public Builder builderBidChallengePercentage(final String builderBidChallengePercentage) {
-      this.builderBidChallengePercentage = builderBidChallengePercentage;
+    public Builder builderBidCompareFactor(final String builderBidCompareFactor) {
+      this.builderBidCompareFactor = builderBidCompareFactor;
       return this;
     }
 
@@ -239,25 +259,26 @@ private void validateBuilderCircuitBreaker() {
       }
     }
 
-    private Optional<Integer> validateAndParseBuilderBidChallengePercentage() {
-      if (builderBidChallengePercentage.toUpperCase(Locale.ROOT).equals("NEVER")) {
+    private Optional<Integer> validateAndParseBuilderBidCompareFactor() {
+      if (builderBidCompareFactor.toUpperCase(Locale.ROOT).equals(BUILDER_ALWAYS_KEYWORD)) {
         return Optional.empty();
       }
-      if (builderBidChallengePercentage.endsWith("%")) {
-        builderBidChallengePercentage =
-            builderBidChallengePercentage.substring(0, builderBidChallengePercentage.length() - 1);
+      if (builderBidCompareFactor.endsWith("%")) {
+        builderBidCompareFactor =
+            builderBidCompareFactor.substring(0, builderBidCompareFactor.length() - 1);
       }
-      final int builderBidChallengePercentageInt;
+      final int builderBidCompareFactorInt;
       try {
-        builderBidChallengePercentageInt = Integer.parseInt(builderBidChallengePercentage);
+        builderBidCompareFactorInt = Integer.parseInt(builderBidCompareFactor);
       } catch (final NumberFormatException ex) {
         throw new InvalidConfigurationException(
-            "Expecting number, percentage or NEVER keyword for Builder bid challenge percentage");
+            "Expecting number, percentage or "
+                + BUILDER_ALWAYS_KEYWORD
+                + " keyword for Builder bid compare factor");
       }
       checkArgument(
-          builderBidChallengePercentageInt >= 0,
-          "Builder bid value challenge percentage should be >= 0");
-      return Optional.of(builderBidChallengePercentageInt);
+          builderBidCompareFactorInt >= 0, "Builder bid compare factor percentage should be >= 0");
+      return Optional.of(builderBidCompareFactorInt);
     }
   }
 }

services/executionlayer/src/main/java/tech/pegasys/teku/services/executionlayer/ExecutionLayerService.java

@@ -237,7 +237,7 @@ private static ExecutionLayerManager createRealExecutionLayerManager(
         new BuilderBidValidatorImpl(EVENT_LOG),
         builderCircuitBreaker,
         blobsBundleValidator,
-        config.getBuilderBidChallengePercentage());
+        config.getBuilderBidCompareFactor());
   }
 
   ExecutionLayerService(