3621 parent timestamps (#3726)

* timestamp being behind or at head does not make a block invalid

Signed-off-by: Justin Florentine <justin+github@florentine.us>

Co-authored-by: Sally MacFarlane <sally.macfarlane@consensys.net>

consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java

@@ -257,15 +257,19 @@ public class MergeCoordinator implements MergeMiningCoordinator {
       final Hash headBlockHash, final Hash finalizedBlockHash) {
     MutableBlockchain blockchain = protocolContext.getBlockchain();
     Optional<BlockHeader> currentFinalized = mergeContext.getFinalized();
-
     final Optional<BlockHeader> newFinalized = blockchain.getBlockHeader(finalizedBlockHash);
-
+    BlockHeader newHead = blockchain.getBlockHeader(headBlockHash).orElse(null);
+    if (newHead == null) {
+      return ForkchoiceResult.withFailure(
+          String.format("not able to find new head block %s", headBlockHash), Optional.empty());
+    }
+    final Optional<Hash> latestValid = getLatestValidAncestor(newHead);
     if (newFinalized.isEmpty() && !finalizedBlockHash.equals(Hash.ZERO)) {
       // we should only fail to find when it's the special value 0x000..000
       return ForkchoiceResult.withFailure(
           String.format(
               "should've been able to find block hash %s but couldn't", finalizedBlockHash),
-          null);
+          latestValid);
     }
 
     if (currentFinalized.isPresent()
@@ -275,15 +279,10 @@ public class MergeCoordinator implements MergeMiningCoordinator {
           String.format(
               "new finalized block %s is not a descendant of current finalized block %s",
               finalizedBlockHash, currentFinalized.get().getBlockHash()),
-          null);
+          latestValid);
     }
 
     // ensure we have headBlock:
-    BlockHeader newHead = blockchain.getBlockHeader(headBlockHash).orElse(null);
-    if (newHead == null) {
-      return ForkchoiceResult.withFailure(
-          String.format("not able to find new head block %s", headBlockHash), null);
-    }
 
     // ensure new head is descendant of finalized
     Optional<String> descendantError =
@@ -298,16 +297,14 @@ public class MergeCoordinator implements MergeMiningCoordinator {
                         newHead.getBlockHash(), finalized.getBlockHash()));
 
     if (descendantError.isPresent()) {
-      return ForkchoiceResult.withFailure(descendantError.get(), null);
+      return ForkchoiceResult.withFailure(descendantError.get(), latestValid);
     }
 
     Optional<BlockHeader> parentOfNewHead = blockchain.getBlockHeader(newHead.getParentHash());
     if (parentOfNewHead.isPresent()
         && parentOfNewHead.get().getTimestamp() >= newHead.getTimestamp()) {
-      final Optional<Hash> latestValid = getLatestValidAncestor(newHead);
       return ForkchoiceResult.withFailure(
-          "new head timestamp not greater than parent",
-          latestValid.isPresent() ? latestValid.get() : null);
+          "new head timestamp not greater than parent", latestValid);
     }
     // set the new head
     blockchain.rewindToBlock(newHead.getHash());

consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeMiningCoordinator.java

@@ -68,12 +68,10 @@ public interface MergeMiningCoordinator extends MiningCoordinator {
       this.latestValid = latestValid;
     }
 
-    public static ForkchoiceResult withFailure(final String errorMessage, final Hash latestValid) {
+    public static ForkchoiceResult withFailure(
+        final String errorMessage, final Optional<Hash> latestValid) {
       return new ForkchoiceResult(
-          Optional.of(errorMessage),
-          Optional.empty(),
-          Optional.empty(),
-          Optional.ofNullable(latestValid));
+          Optional.of(errorMessage), Optional.empty(), Optional.empty(), latestValid);
     }
 
     public static ForkchoiceResult withResult(

consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java

@@ -266,7 +266,10 @@ public class MergeCoordinatorTest implements MergeGenesisConfigHelper {
     BlockHeader headBlockHeader = nextBlockHeader(lastFinalizedHeader);
     Block headBlock = new Block(headBlockHeader, BlockBody.empty());
     coordinator.executeBlock(headBlock);
-
+    when(blockchain.getBlockHeader(lastFinalizedBlock.getHash()))
+        .thenReturn(Optional.of(lastFinalizedHeader));
+    when(blockchain.getBlockHeader(headBlockHeader.getHash()))
+        .thenReturn(Optional.of(headBlockHeader));
     var res = coordinator.updateForkChoice(headBlock.getHash(), lastFinalizedBlock.getHash());
     assertThat(res.isFailed()).isTrue();
 
@@ -293,7 +296,10 @@ public class MergeCoordinatorTest implements MergeGenesisConfigHelper {
     BlockHeader headBlockHeader = disjointBlockHeader(lastFinalizedHeader);
     Block headBlock = new Block(headBlockHeader, BlockBody.empty());
     coordinator.executeBlock(headBlock);
-
+    when(blockchain.getBlockHeader(lastFinalizedBlock.getHash()))
+        .thenReturn(Optional.of(lastFinalizedHeader));
+    when(blockchain.getBlockHeader(headBlockHeader.getHash()))
+        .thenReturn(Optional.of(headBlockHeader));
     var res = coordinator.updateForkChoice(headBlock.getHash(), lastFinalizedBlock.getHash());
     assertThat(res.isSuccessful()).isFalse();
     assertThat(res.isFailed()).isTrue();
@@ -320,7 +326,6 @@ public class MergeCoordinatorTest implements MergeGenesisConfigHelper {
     BlockHeader headBlockHeader = nextBlockHeader(lastFinalizedHeader);
     Block headBlock = new Block(headBlockHeader, BlockBody.empty());
     // note this block is not executed, so not known by us
-
     var res = coordinator.updateForkChoice(headBlock.getHash(), lastFinalizedBlock.getHash());
     assertThat(res.isFailed()).isTrue();
 
@@ -346,7 +351,10 @@ public class MergeCoordinatorTest implements MergeGenesisConfigHelper {
     BlockHeader headBlockHeader = nextBlockHeader(lastFinalizedHeader);
     Block headBlock = new Block(headBlockHeader, BlockBody.empty());
     coordinator.executeBlock(headBlock);
-
+    when(blockchain.getBlockHeader(lastFinalizedBlock.getHash())).thenReturn(Optional.empty());
+    when(blockchain.getBlockHeader(headBlockHeader.getHash()))
+        .thenReturn(Optional.of(headBlockHeader))
+        .thenReturn(Optional.of(headBlockHeader));
     var res = coordinator.updateForkChoice(headBlock.getHash(), lastFinalizedBlock.getHash());
     assertThat(res.isSuccessful()).isFalse();
     assertThat(res.isFailed()).isTrue();

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayload.java

@@ -30,6 +30,8 @@ import org.hyperledger.besu.ethereum.api.jsonrpc.RpcMethod;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequestContext;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.parameters.EnginePayloadParameter;
+import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcError;
+import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcErrorResponse;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcResponse;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcSuccessResponse;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.results.EnginePayloadStatusResult;
@@ -147,8 +149,8 @@ public class EngineNewPayload extends ExecutionEngineJsonRpcMethod {
           protocolContext.getBlockchain().getBlockHeader(blockParam.getParentHash());
       if (parentHeader.isPresent()
           && (blockParam.getTimestamp() <= parentHeader.get().getTimestamp())) {
-        return respondWithInvalid(
-            reqId, parentHeader.get().getHash(), "Timestamp must be greater than parent");
+        LOG.info("method parameter timestamp not greater than parent");
+        return new JsonRpcErrorResponse(reqId, JsonRpcError.INVALID_PARAMS);
       }
     }
 

ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedTest.java

@@ -144,7 +144,7 @@ public class EngineForkchoiceUpdatedTest {
     when(mergeCoordinator.updateForkChoice(mockHeader.getHash(), parent.getHash()))
         .thenReturn(
             ForkchoiceResult.withFailure(
-                "new head timestamp not greater than parent", parent.getHash()));
+                "new head timestamp not greater than parent", Optional.of(parent.getHash())));
 
     EngineForkchoiceUpdatedParameter param =
         new EngineForkchoiceUpdatedParameter(

ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadTest.java

@@ -220,7 +220,7 @@ public class EngineNewPayloadTest {
   }
 
   @Test
-  public void shouldReturnInvalidOnOldTimestamp() {
+  public void shouldReturnInvalidOnOldTimestampInBlock() {
     BlockHeader parent = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
     BlockHeader mockHeader =
         new BlockHeaderTestFixture()
@@ -232,10 +232,7 @@ public class EngineNewPayloadTest {
     when(blockchain.getBlockHeader(parent.getHash())).thenReturn(Optional.of(parent));
     var resp = resp(mockPayload(mockHeader, Collections.emptyList()));
 
-    EnginePayloadStatusResult res = fromSuccessResp(resp);
-    assertThat(res.getLatestValidHash().get()).isEqualTo(parent.getHash());
-    assertThat(res.getStatusAsString()).isEqualTo(INVALID.name());
-    assertThat(res.getError()).isEqualTo("Timestamp must be greater than parent");
+    assertThat(resp.getType()).isEqualTo(JsonRpcResponseType.ERROR);
   }
 
   @Test