Replace usage of getByTimestamp in WithdrawalsValidatorProvider (#5261)

Use schedule.getByBlockHeader for newPayload, constructing a dummy header for the sole purpose of getting the correct WithdrawalsValidator for this block. See reviews comments below for context. Use schedule.getForNextBlockHeader for forkchoiceUpdated. Once we remove usages of schedule.getByTimestamp, we can unify the protocol schedules. Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
hyperledger · Mar 31, 2023 · 65c0bb3 · 65c0bb3
1 parent 64efb66
commit 65c0bb3
Show file tree

Hide file tree

Showing 5 changed files with 41 additions and 14 deletions.
diff --git a/...er/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdated.java b/...er/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdated.java
@@ -207,7 +207,8 @@ private boolean isPayloadAttributesValid(
     final boolean newTimestampGreaterThanHead =
         payloadAttributes.getTimestamp() > headBlockHeader.getTimestamp();
     return newTimestampGreaterThanHead
-        && getWithdrawalsValidator(timestampSchedule, payloadAttributes.getTimestamp())
+        && getWithdrawalsValidator(
+                timestampSchedule, headBlockHeader, payloadAttributes.getTimestamp())
             .validateWithdrawals(maybeWithdrawals);
   }
 

diff --git a/...perledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayload.java b/...perledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayload.java
@@ -104,7 +104,9 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext)
     final Optional<List<Withdrawal>> maybeWithdrawals =
         Optional.ofNullable(blockParam.getWithdrawals())
             .map(ws -> ws.stream().map(WithdrawalParameter::toWithdrawal).collect(toList()));
-    if (!getWithdrawalsValidator(timestampSchedule, blockParam.getTimestamp())
+
+    if (!getWithdrawalsValidator(
+            timestampSchedule, blockParam.getTimestamp(), blockParam.getBlockNumber())
         .validateWithdrawals(maybeWithdrawals)) {
       return new JsonRpcErrorResponse(reqId, INVALID_PARAMS);
     }
@@ -188,10 +190,10 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext)
           "Block already present in bad block manager.");
     }
 
-    Optional<BlockHeader> parentHeader =
+    final Optional<BlockHeader> maybeParentHeader =
         protocolContext.getBlockchain().getBlockHeader(blockParam.getParentHash());
-    if (parentHeader.isPresent()
-        && (blockParam.getTimestamp() <= parentHeader.get().getTimestamp())) {
+    if (maybeParentHeader.isPresent()
+        && (blockParam.getTimestamp() <= maybeParentHeader.get().getTimestamp())) {
       return respondWithInvalid(
           reqId,
           blockParam,
@@ -206,7 +208,7 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext)
             new BlockBody(
                 transactions, Collections.emptyList(), maybeWithdrawals, Optional.empty()));
 
-    if (parentHeader.isEmpty()) {
+    if (maybeParentHeader.isEmpty()) {
       LOG.atDebug()
           .setMessage("Parent of block {} is not present, append it to backward sync")
           .addArgument(block::toLogString)

diff --git a/...edger/besu/ethereum/api/jsonrpc/internal/methods/engine/WithdrawalsValidatorProvider.java b/...edger/besu/ethereum/api/jsonrpc/internal/methods/engine/WithdrawalsValidatorProvider.java
@@ -15,17 +15,42 @@
 
 package org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.engine;
 
+import org.hyperledger.besu.ethereum.core.BlockHeader;
+import org.hyperledger.besu.ethereum.core.BlockHeaderBuilder;
 import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec;
 import org.hyperledger.besu.ethereum.mainnet.TimestampSchedule;
 import org.hyperledger.besu.ethereum.mainnet.WithdrawalsValidator;
 
+import java.util.Optional;
+
 public class WithdrawalsValidatorProvider {
 
   static WithdrawalsValidator getWithdrawalsValidator(
-      final TimestampSchedule timestampSchedule, final long newPayloadTimestamp) {
+      final TimestampSchedule timestampSchedule,
+      final long blockTimestamp,
+      final long blockNumber) {
+
+    final BlockHeader blockHeader =
+        BlockHeaderBuilder.createDefault()
+            .timestamp(blockTimestamp)
+            .number(blockNumber)
+            .buildBlockHeader();
+    return Optional.ofNullable(timestampSchedule.getByBlockHeader(blockHeader))
+        .map(ProtocolSpec::getWithdrawalsValidator)
+        // TODO Withdrawals this is a quirk of the fact timestampSchedule doesn't fallback to the
+        // previous fork. This might be resolved when
+        // https://github.com/hyperledger/besu/issues/4789 is played
+        // and if we can combine protocolSchedule and timestampSchedule.
+        .orElseGet(WithdrawalsValidator.ProhibitedWithdrawals::new);
+  }
+
+  static WithdrawalsValidator getWithdrawalsValidator(
+      final TimestampSchedule timestampSchedule,
+      final BlockHeader parentBlockHeader,
+      final long timestampForNextBlock) {
 
-    return timestampSchedule
-        .getByTimestamp(newPayloadTimestamp)
+    return Optional.ofNullable(
+            timestampSchedule.getForNextBlockHeader(parentBlockHeader, timestampForNextBlock))
         .map(ProtocolSpec::getWithdrawalsValidator)
         // TODO Withdrawals this is a quirk of the fact timestampSchedule doesn't fallback to the
         // previous fork. This might be resolved when

diff --git a/...esu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java b/...esu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java
@@ -116,7 +116,7 @@ public void before() {
     when(protocolContext.getBlockchain()).thenReturn(blockchain);
     when(protocolSpec.getWithdrawalsValidator())
         .thenReturn(new WithdrawalsValidator.ProhibitedWithdrawals());
-    when(timestampSchedule.getByTimestamp(anyLong())).thenReturn(Optional.of(protocolSpec));
+    when(timestampSchedule.getForNextBlockHeader(any(), anyLong())).thenReturn(protocolSpec);
     this.method =
         methodFactory.create(
             vertx, timestampSchedule, protocolContext, mergeCoordinator, engineCallListener);
@@ -619,7 +619,7 @@ public void shouldReturnValidIfWithdrawalsIsNotNull_WhenWithdrawalsAllowed() {
 
   @Test
   public void shouldReturnValidIfTimestampScheduleIsEmpty() {
-    when(timestampSchedule.getByTimestamp(anyLong())).thenReturn(Optional.empty());
+    when(timestampSchedule.getForNextBlockHeader(any(), anyLong())).thenReturn(null);
 
     BlockHeader mockParent = blockHeaderBuilder.number(9L).buildHeader();
     BlockHeader mockHeader =

diff --git a/...edger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayloadTest.java b/...edger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayloadTest.java
@@ -22,7 +22,6 @@
 import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.parameters.WithdrawalParameterTestFixture.WITHDRAWAL_PARAM_1;
 import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcError.INVALID_PARAMS;
 import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.spy;
@@ -124,7 +123,7 @@ public void before() {
     when(protocolContext.getBlockchain()).thenReturn(blockchain);
     when(protocolSpec.getWithdrawalsValidator())
         .thenReturn(new WithdrawalsValidator.ProhibitedWithdrawals());
-    when(timestampSchedule.getByTimestamp(anyLong())).thenReturn(Optional.of(protocolSpec));
+    when(timestampSchedule.getByBlockHeader(any())).thenReturn(protocolSpec);
     when(ethPeers.peerCount()).thenReturn(1);
     this.method =
         methodFactory.create(
@@ -462,7 +461,7 @@ public void shouldReturnValidIfWithdrawalsIsNotNull_WhenWithdrawalsAllowed() {
 
   @Test
   public void shouldReturnValidIfTimestampScheduleIsEmpty() {
-    when(timestampSchedule.getByTimestamp(anyLong())).thenReturn(Optional.empty());
+    when(timestampSchedule.getByBlockHeader(any())).thenReturn(null);
     BlockHeader mockHeader =
         setupValidPayload(
             new BlockProcessingResult(Optional.of(new BlockProcessingOutputs(null, List.of()))),