Merge branch 'main' into zkbesu

Consensys · Mar 22, 2024 · b37f645 · b37f645
2 parents 5a0618f + 42f4748
commit b37f645
Show file tree

Hide file tree

Showing 13 changed files with 125 additions and 45 deletions.
diff --git a/...r/besu/tests/acceptance/permissioning/NodeSmartContractPermissioningV2AcceptanceTest.java b/...r/besu/tests/acceptance/permissioning/NodeSmartContractPermissioningV2AcceptanceTest.java
@@ -47,10 +47,7 @@ public void setUp() {
     permissionedNode.execute(allowNode(permissionedNode));
     permissionedNode.verify(connectionIsAllowed(permissionedNode));
 
-    allowedNode.verify(eth.syncingStatus(false));
-    bootnode.verify(eth.syncingStatus(false));
-    permissionedNode.verify(eth.syncingStatus(false));
-    forbiddenNode.verify(eth.syncingStatus(false));
+    verifyAllNodesHaveFinishedSyncing();
   }
 
   @Test
@@ -92,6 +89,8 @@ public void permissioningUpdatesPropagateThroughNetwork() {
     permissionedNode.verify(admin.addPeer(allowedNode));
     permissionedNode.verify(net.awaitPeerCount(2));
 
+    verifyAllNodesHaveFinishedSyncing();
+
     // permissioning changes in peer should propagate to permissioned node
     allowedNode.execute(allowNode(forbiddenNode));
     allowedNode.verify(connectionIsAllowed(forbiddenNode));
@@ -101,6 +100,13 @@ public void permissioningUpdatesPropagateThroughNetwork() {
     permissionedNode.verify(net.awaitPeerCount(3));
   }
 
+  private void verifyAllNodesHaveFinishedSyncing() {
+    allowedNode.verify(eth.syncingStatus(false));
+    bootnode.verify(eth.syncingStatus(false));
+    permissionedNode.verify(eth.syncingStatus(false));
+    forbiddenNode.verify(eth.syncingStatus(false));
+  }
+
   @Test
   public void onchainPermissioningAllowlistShouldPersistAcrossRestarts() {
     permissionedCluster.stop();

diff --git a/...nrpc/engine/shanghai/test-cases/05_shanghai_prepare_payload_invalid_null_withdrawals.json b/...nrpc/engine/shanghai/test-cases/05_shanghai_prepare_payload_invalid_null_withdrawals.json
@@ -20,8 +20,8 @@
     "jsonrpc" : "2.0",
     "id" : 67,
     "error" : {
-      "code" : -38003,
-      "message" : "Invalid payload attributes"
+      "code" : -32602,
+      "message" : "Invalid params"
     }
   },
   "statusCode" : 200

diff --git a/...er/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdated.java b/...er/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdated.java
@@ -172,7 +172,7 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext)
       if (!getWithdrawalsValidator(
               protocolSchedule.get(), newHead, maybePayloadAttributes.get().getTimestamp())
           .validateWithdrawals(withdrawals)) {
-        return new JsonRpcErrorResponse(requestId, getInvalidPayloadError());
+        return new JsonRpcErrorResponse(requestId, getInvalidParametersError());
       }
     }
 
@@ -241,7 +241,7 @@ protected Optional<JsonRpcErrorResponse> isPayloadAttributeRelevantToNewHead(
     if (payloadAttributes.getTimestamp() <= headBlockHeader.getTimestamp()) {
       LOG.warn(
           "Payload attributes timestamp is smaller than timestamp of header in fork choice update");
-      return Optional.of(new JsonRpcErrorResponse(requestId, getInvalidPayloadError()));
+      return Optional.of(new JsonRpcErrorResponse(requestId, getInvalidPayloadAttributesError()));
     }
 
     return Optional.empty();
@@ -364,10 +364,14 @@ protected boolean requireTerminalPoWBlockValidation() {
     return false;
   }
 
-  protected RpcErrorType getInvalidPayloadError() {
+  protected RpcErrorType getInvalidParametersError() {
     return RpcErrorType.INVALID_PARAMS;
   }
 
+  protected RpcErrorType getInvalidPayloadAttributesError() {
+    return RpcErrorType.INVALID_PAYLOAD_ATTRIBUTES;
+  }
+
   // fcU calls are synchronous, no need to make volatile
   private long lastFcuInfoLog = System.currentTimeMillis();
   private static final String logMessage =

diff --git a/...erledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1.java b/...erledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1.java
@@ -54,7 +54,7 @@ protected boolean requireTerminalPoWBlockValidation() {
   }
 
   @Override
-  protected RpcErrorType getInvalidPayloadError() {
+  protected RpcErrorType getInvalidParametersError() {
     return RpcErrorType.INVALID_PAYLOAD_ATTRIBUTES;
   }
 }
diff --git a/...erledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV2.java b/...erledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV2.java
@@ -62,15 +62,9 @@ protected Optional<JsonRpcErrorResponse> isPayloadAttributesValid(
     } else if (payloadAttributes.getParentBeaconBlockRoot() != null) {
       LOG.error(
           "Parent beacon block root hash present in payload attributes before cancun hardfork");
-      return Optional.of(
-          new JsonRpcErrorResponse(requestId, RpcErrorType.INVALID_PAYLOAD_ATTRIBUTES));
+      return Optional.of(new JsonRpcErrorResponse(requestId, getInvalidPayloadAttributesError()));
     } else {
       return Optional.empty();
     }
   }
-
-  @Override
-  protected RpcErrorType getInvalidPayloadError() {
-    return RpcErrorType.INVALID_PAYLOAD_ATTRIBUTES;
-  }
 }
diff --git a/...erledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV3.java b/...erledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV3.java
@@ -56,16 +56,19 @@ protected ValidationResult<RpcErrorType> validateParameter(
       final EngineForkchoiceUpdatedParameter fcuParameter,
       final Optional<EnginePayloadAttributesParameter> maybePayloadAttributes) {
     if (fcuParameter.getHeadBlockHash() == null) {
-      return ValidationResult.invalid(getInvalidPayloadError(), "Missing head block hash");
+      return ValidationResult.invalid(
+          getInvalidPayloadAttributesError(), "Missing head block hash");
     } else if (fcuParameter.getSafeBlockHash() == null) {
-      return ValidationResult.invalid(getInvalidPayloadError(), "Missing safe block hash");
+      return ValidationResult.invalid(
+          getInvalidPayloadAttributesError(), "Missing safe block hash");
     } else if (fcuParameter.getFinalizedBlockHash() == null) {
-      return ValidationResult.invalid(getInvalidPayloadError(), "Missing finalized block hash");
+      return ValidationResult.invalid(
+          getInvalidPayloadAttributesError(), "Missing finalized block hash");
     }
     if (maybePayloadAttributes.isPresent()) {
       if (maybePayloadAttributes.get().getParentBeaconBlockRoot() == null) {
         return ValidationResult.invalid(
-            getInvalidPayloadError(), "Missing parent beacon block root hash");
+            getInvalidPayloadAttributesError(), "Missing parent beacon block root hash");
       }
     }
     return ValidationResult.valid();
@@ -93,18 +96,13 @@ protected Optional<JsonRpcErrorResponse> isPayloadAttributesValid(
     if (payloadAttributes.getParentBeaconBlockRoot() == null) {
       LOG.error(
           "Parent beacon block root hash not present in payload attributes after cancun hardfork");
-      return Optional.of(new JsonRpcErrorResponse(requestId, getInvalidPayloadError()));
+      return Optional.of(new JsonRpcErrorResponse(requestId, getInvalidPayloadAttributesError()));
     } else if (payloadAttributes.getTimestamp().longValue() == 0) {
-      return Optional.of(new JsonRpcErrorResponse(requestId, getInvalidPayloadError()));
+      return Optional.of(new JsonRpcErrorResponse(requestId, getInvalidPayloadAttributesError()));
     } else if (payloadAttributes.getTimestamp() < cancun.get().milestone()) {
       return Optional.of(new JsonRpcErrorResponse(requestId, RpcErrorType.UNSUPPORTED_FORK));
     } else {
       return Optional.empty();
     }
   }
-
-  @Override
-  protected RpcErrorType getInvalidPayloadError() {
-    return RpcErrorType.INVALID_PAYLOAD_ATTRIBUTES;
-  }
 }
diff --git a/...esu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java b/...esu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java
@@ -475,7 +475,7 @@ public void shouldReturnInvalidIfPayloadTimestampNotGreaterThanHead() {
                 mockHeader.getHash(), Hash.ZERO, mockParent.getHash()),
             Optional.of(payloadParams));
 
-    assertInvalidForkchoiceState(resp, expectedInvalidPayloadError());
+    assertInvalidForkchoiceState(resp, RpcErrorType.INVALID_PAYLOAD_ATTRIBUTES);
     verify(engineCallListener, times(1)).executionEngineCalled();
   }
 

diff --git a/...dger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV2Test.java b/...dger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV2Test.java
@@ -46,6 +46,6 @@ protected String getMethodName() {
 
   @Override
   protected RpcErrorType expectedInvalidPayloadError() {
-    return RpcErrorType.INVALID_PAYLOAD_ATTRIBUTES;
+    return RpcErrorType.INVALID_PARAMS;
   }
 }
diff --git a/.../main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncAlgorithm.java b/.../main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncAlgorithm.java
@@ -22,11 +22,13 @@
 import org.hyperledger.besu.ethereum.chain.MutableBlockchain;
 import org.hyperledger.besu.ethereum.core.Block;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
+import org.hyperledger.besu.ethereum.eth.manager.exceptions.MaxRetriesReachedException;
 import org.hyperledger.besu.ethereum.eth.manager.task.WaitForPeersTask;
 import org.hyperledger.besu.plugin.services.BesuEvents;
 
 import java.util.Optional;
 import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
@@ -61,16 +63,40 @@ public CompletableFuture<Void> executeBackwardsSync(final Void unused) {
   public CompletableFuture<Void> pickNextStep() {
     final Optional<Hash> firstHash = context.getBackwardChain().getFirstHashToAppend();
     if (firstHash.isPresent()) {
-      return executeSyncStep(firstHash.get())
-          .thenAccept(
-              result -> {
-                LOG.atDebug()
-                    .setMessage("Backward sync target block is {}")
-                    .addArgument(result::toLogString)
-                    .log();
-                context.getBackwardChain().removeFromHashToAppend(firstHash.get());
-                context.getStatus().updateTargetHeight(result.getHeader().getNumber());
+      final CompletableFuture<Void> syncStep = new CompletableFuture<>();
+      executeSyncStep(firstHash.get())
+          .whenComplete(
+              (result, error) -> {
+                if (error != null) {
+                  if (error instanceof CompletionException
+                      && error.getCause() instanceof MaxRetriesReachedException) {
+                    context.getBackwardChain().removeFromHashToAppend(firstHash.get());
+                    LOG.atWarn()
+                        .setMessage(
+                            "Unable to retrieve block {} from any peer, with {} peers available. Could be a reorged block. Waiting for the next block from the consensus client to try again.")
+                        .addArgument(firstHash.get())
+                        .addArgument(context.getEthContext().getEthPeers().peerCount())
+                        .addArgument(context.getBackwardChain().getFirstHashToAppend())
+                        .log();
+                    LOG.atDebug()
+                        .setMessage("Removing hash {} from hashesToAppend")
+                        .addArgument(firstHash.get())
+                        .log();
+                    syncStep.complete(null);
+                  } else {
+                    syncStep.completeExceptionally(error);
+                  }
+                } else {
+                  LOG.atDebug()
+                      .setMessage("Backward sync target block is {}")
+                      .addArgument(result::toLogString)
+                      .log();
+                  context.getBackwardChain().removeFromHashToAppend(firstHash.get());
+                  context.getStatus().updateTargetHeight(result.getHeader().getNumber());
+                  syncStep.complete(null);
+                }
               });
+      return syncStep;
     }
     if (!context.isReady()) {
       return waitForReady();

diff --git a/...rc/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContext.java b/...rc/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContext.java
@@ -408,7 +408,7 @@ private void logBlockImportProgress(final long currImportedHeight) {
     final float completedPercentage = 100.0f * imported / estimatedTotal;
 
     if (completedPercentage < 100.0f) {
-      if (currentStatus.progressLogDue()) {
+      if (currentStatus.progressLogDue() && targetHeight > 0) {
         LOG.info(
             String.format(
                 "Backward sync phase 2 of 2, %.2f%% completed, imported %d blocks of at least %d (current head %d, target head %d). Peers: %d",

diff --git a/...h/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncStep.java b/...h/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncStep.java
@@ -61,7 +61,8 @@ protected CompletableFuture<List<BlockHeader>> requestHeaders(final Hash hash) {
         context.getProtocolContext().getBlockchain().getBlockHeader(hash);
     if (blockHeader.isPresent()) {
       LOG.debug(
-          "Hash {} already present in local blockchain no need to request headers to peers", hash);
+          "Hash {} already present in local blockchain no need to request headers from peers",
+          hash);
       return CompletableFuture.completedFuture(List.of(blockHeader.get()));
     }
 

diff --git a/...est/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContextTest.java b/...est/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContextTest.java
@@ -83,7 +83,7 @@ public class BackwardSyncContextTest {
   public static final int REMOTE_HEIGHT = 50;
   public static final int UNCLE_HEIGHT = 25 - 3;
 
-  public static final int NUM_OF_RETRIES = 100;
+  public static final int NUM_OF_RETRIES = 1;
   public static final int TEST_MAX_BAD_CHAIN_EVENT_ENTRIES = 25;
 
   private BackwardSyncContext context;
@@ -186,13 +186,16 @@ public void setup() {
   }
 
   private Block createUncle(final int i, final Hash parentHash) {
+    return createBlock(i, parentHash);
+  }
+
+  private Block createBlock(final int i, final Hash parentHash) {
     final BlockDataGenerator.BlockOptions options =
         new BlockDataGenerator.BlockOptions()
             .setBlockNumber(i)
             .setParentHash(parentHash)
             .transactionTypes(TransactionType.ACCESS_LIST);
-    final Block block = blockDataGenerator.block(options);
-    return block;
+    return blockDataGenerator.block(options);
   }
 
   public static BackwardChain inMemoryBackwardChain() {
@@ -438,4 +441,53 @@ public void shouldFailAfterMaxNumberOfRetries() {
       }
     }
   }
+
+  @SuppressWarnings("BannedMethod")
+  @Test
+  public void whenBlockNotFoundInPeers_shouldRemoveBlockFromQueueAndProgressInNextSession() {
+    // This scenario can happen due to a reorg
+    // Expectation we progress beyond the reorg block upon receiving the next FCU
+
+    // choose an intermediate remote block to create a reorg block from
+    int reorgBlockHeight = REMOTE_HEIGHT - 1; // 49
+    final Hash reorgBlockParentHash = getBlockByNumber(reorgBlockHeight - 1).getHash();
+    final Block reorgBlock = createBlock(reorgBlockHeight, reorgBlockParentHash);
+
+    // represents first FCU with a block that will become reorged away
+    final CompletableFuture<Void> fcuBeforeReorg = context.syncBackwardsUntil(reorgBlock.getHash());
+    respondUntilFutureIsDone(fcuBeforeReorg);
+    assertThat(localBlockchain.getChainHeadBlockNumber()).isLessThan(reorgBlockHeight);
+
+    // represents subsequent FCU with successfully reorged version of the same block
+    final CompletableFuture<Void> fcuAfterReorg =
+        context.syncBackwardsUntil(getBlockByNumber(reorgBlockHeight).getHash());
+    respondUntilFutureIsDone(fcuAfterReorg);
+    assertThat(localBlockchain.getChainHeadBlock())
+        .isEqualTo(remoteBlockchain.getBlockByNumber(reorgBlockHeight).orElseThrow());
+  }
+
+  @SuppressWarnings("BannedMethod")
+  @Test
+  public void
+      whenBlockNotFoundInPeers_shouldRemoveBlockFromQueueAndProgressWithQueueInSameSession() {
+    // This scenario can happen due to a reorg
+    // Expectation we progress beyond the reorg block due to FCU we received during the same session
+
+    // choose an intermediate remote block to create a reorg block from
+    int reorgBlockHeight = REMOTE_HEIGHT - 1; // 49
+    final Hash reorgBlockParentHash = getBlockByNumber(reorgBlockHeight - 1).getHash();
+    final Block reorgBlock = createBlock(reorgBlockHeight, reorgBlockParentHash);
+
+    // represents first FCU with a block that will become reorged away
+    final CompletableFuture<Void> fcuBeforeReorg = context.syncBackwardsUntil(reorgBlock.getHash());
+    // represents subsequent FCU with successfully reorged version of the same block
+    // received during the first FCU's BWS session
+    final CompletableFuture<Void> fcuAfterReorg =
+        context.syncBackwardsUntil(getBlockByNumber(reorgBlockHeight).getHash());
+
+    respondUntilFutureIsDone(fcuBeforeReorg);
+    respondUntilFutureIsDone(fcuAfterReorg);
+    assertThat(localBlockchain.getChainHeadBlock())
+        .isEqualTo(remoteBlockchain.getBlockByNumber(reorgBlockHeight).orElseThrow());
+  }
 }
diff --git a/gradle.properties b/gradle.properties
@@ -17,6 +17,5 @@ org.gradle.jvmargs=-Xmx4g \
 --add-opens jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED \
 --add-opens jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED
 # Could be moved to sonar properties after https://sonarsource.atlassian.net/browse/SONARGRADL-134
-systemProp.sonar.gradle.skipCompile=true
 
 besu.run.args="--network=linea"