Remove check in fcU if new blocks are descendants of the previous fin…

…alized block (hyperledger#3975) * removed check from fcU if new blocks are descendants of the previous finalized block * backwards sync: remove check that block must be a descendant of finalized Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
lacchain · Nov 3, 2023 · 8caf8c0 · 8caf8c0
1 parent eea00bd
commit 8caf8c0
Show file tree

Hide file tree

Showing 6 changed files with 10 additions and 214 deletions.
diff --git a/...ge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java b/...ge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java
@@ -274,38 +274,10 @@ public ForkchoiceResult updateForkChoice(
       final Hash safeBlockHash,
       final Optional<PayloadAttributes> maybePayloadAttributes) {
     MutableBlockchain blockchain = protocolContext.getBlockchain();
-    Optional<BlockHeader> currentFinalized = mergeContext.getFinalized();
     final Optional<BlockHeader> newFinalized = blockchain.getBlockHeader(finalizedBlockHash);
 
     final Optional<Hash> latestValid = getLatestValidAncestor(newHead);
 
-    if (currentFinalized.isPresent()
-        && newFinalized.isPresent()
-        && !isDescendantOf(currentFinalized.get(), newFinalized.get())) {
-      return ForkchoiceResult.withFailure(
-          INVALID,
-          String.format(
-              "new finalized block %s is not a descendant of current finalized block %s",
-              finalizedBlockHash, currentFinalized.get().getBlockHash()),
-          latestValid);
-    }
-
-    // ensure new head is descendant of finalized
-    Optional<String> descendantError =
-        newFinalized
-            .map(Optional::of)
-            .orElse(currentFinalized)
-            .filter(finalized -> !isDescendantOf(finalized, newHead))
-            .map(
-                finalized ->
-                    String.format(
-                        "new head block %s is not a descendant of current finalized block %s",
-                        newHead.getBlockHash(), finalized.getBlockHash()));
-
-    if (descendantError.isPresent()) {
-      return ForkchoiceResult.withFailure(INVALID, descendantError.get(), latestValid);
-    }
-
     Optional<BlockHeader> parentOfNewHead = blockchain.getBlockHeader(newHead.getParentHash());
     if (parentOfNewHead.isPresent()
         && parentOfNewHead.get().getTimestamp() >= newHead.getTimestamp()) {

diff --git a/...rc/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java b/...rc/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java
@@ -262,119 +262,6 @@ public void updateForkChoiceShouldPersistLastFinalizedBlockHash() {
     verify(mergeContext).setSafeBlock(lastFinalizedHeader);
   }
 
-  @Test
-  public void updateForkChoiceShouldFailIfLastFinalizedNotDescendantOfPreviousFinalized() {
-    BlockHeader terminalHeader = terminalPowBlock();
-    coordinator.executeBlock(new Block(terminalHeader, BlockBody.empty()));
-
-    BlockHeader prevFinalizedHeader = nextBlockHeader(terminalHeader);
-    Block prevFinalizedBlock = new Block(prevFinalizedHeader, BlockBody.empty());
-    coordinator.executeBlock(prevFinalizedBlock);
-
-    when(mergeContext.getFinalized()).thenReturn(Optional.of(prevFinalizedHeader));
-
-    // not descendant of previous finalized block
-    BlockHeader lastFinalizedHeader = disjointBlockHeader(prevFinalizedHeader);
-    Block lastFinalizedBlock = new Block(lastFinalizedHeader, BlockBody.empty());
-    coordinator.executeBlock(lastFinalizedBlock);
-
-    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(
-            headBlockHeader,
-            lastFinalizedBlock.getHash(),
-            lastFinalizedBlock.getHash(),
-            Optional.empty());
-    assertThat(res.isValid()).isFalse();
-    assertThat(res.getStatus()).isEqualTo(ForkchoiceResult.Status.INVALID);
-
-    verify(blockchain, never()).setFinalized(lastFinalizedBlock.getHash());
-    verify(mergeContext, never()).setFinalized(lastFinalizedHeader);
-    verify(blockchain, never()).setSafeBlock(lastFinalizedBlock.getHash());
-    verify(mergeContext, never()).setSafeBlock(lastFinalizedHeader);
-  }
-
-  @Test
-  public void updateForkChoiceShouldFailIfHeadNotDescendantOfLastFinalized() {
-    BlockHeader terminalHeader = terminalPowBlock();
-    coordinator.executeBlock(new Block(terminalHeader, BlockBody.empty()));
-
-    BlockHeader prevFinalizedHeader = nextBlockHeader(terminalHeader);
-    Block prevFinalizedBlock = new Block(prevFinalizedHeader, BlockBody.empty());
-    coordinator.executeBlock(prevFinalizedBlock);
-
-    when(mergeContext.getFinalized()).thenReturn(Optional.of(prevFinalizedHeader));
-
-    BlockHeader lastFinalizedHeader = nextBlockHeader(prevFinalizedHeader);
-    Block lastFinalizedBlock = new Block(lastFinalizedHeader, BlockBody.empty());
-    coordinator.executeBlock(lastFinalizedBlock);
-
-    // not descendant of last finalized block
-    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(
-            headBlockHeader,
-            lastFinalizedBlock.getHash(),
-            lastFinalizedBlock.getHash(),
-            Optional.empty());
-    assertThat(res.isValid()).isFalse();
-    assertThat(res.getStatus()).isEqualTo(ForkchoiceResult.Status.INVALID);
-
-    verify(blockchain, never()).setFinalized(lastFinalizedBlock.getHash());
-    verify(mergeContext, never()).setFinalized(lastFinalizedHeader);
-    verify(blockchain, never()).setSafeBlock(lastFinalizedBlock.getHash());
-    verify(mergeContext, never()).setSafeBlock(lastFinalizedHeader);
-  }
-
-  @Test
-  public void updateForkChoiceShouldFailIfFinalizedBlockNotFound() {
-    BlockHeader terminalHeader = terminalPowBlock();
-    coordinator.executeBlock(new Block(terminalHeader, BlockBody.empty()));
-
-    BlockHeader prevFinalizedHeader = nextBlockHeader(terminalHeader);
-    Block prevFinalizedBlock = new Block(prevFinalizedHeader, BlockBody.empty());
-    coordinator.executeBlock(prevFinalizedBlock);
-
-    when(mergeContext.getFinalized()).thenReturn(Optional.of(prevFinalizedHeader));
-
-    BlockHeader lastFinalizedHeader = nextBlockHeader(prevFinalizedHeader);
-    Block lastFinalizedBlock = new Block(lastFinalizedHeader, BlockBody.empty());
-    // note this block is not executed, so not known by us
-
-    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(
-            headBlockHeader,
-            lastFinalizedBlock.getHash(),
-            lastFinalizedBlock.getHash(),
-            Optional.empty());
-    assertThat(res.isValid()).isFalse();
-    assertThat(res.getStatus()).isEqualTo(ForkchoiceResult.Status.INVALID);
-
-    verify(blockchain, never()).setFinalized(lastFinalizedBlock.getHash());
-    verify(mergeContext, never()).setFinalized(lastFinalizedHeader);
-    verify(blockchain, never()).setSafeBlock(lastFinalizedBlock.getHash());
-    verify(mergeContext, never()).setSafeBlock(lastFinalizedHeader);
-  }
-
   @Test
   public void assertGetOrSyncForBlockAlreadyPresent() {
     BlockHeader mockHeader =
@@ -609,24 +496,6 @@ private BlockHeader nextBlockHeader(
         .buildHeader();
   }
 
-  private BlockHeader disjointBlockHeader(final BlockHeader disjointFromHeader) {
-    Hash disjointParentHash = Hash.wrap(disjointFromHeader.getParentHash().shiftRight(1));
-
-    return headerGenerator
-        .difficulty(Difficulty.ZERO)
-        .parentHash(disjointParentHash)
-        .gasLimit(genesisState.getBlock().getHeader().getGasLimit())
-        .number(disjointFromHeader.getNumber() + 1)
-        .stateRoot(genesisState.getBlock().getHeader().getStateRoot())
-        .baseFeePerGas(
-            feeMarket.computeBaseFee(
-                genesisState.getBlock().getHeader().getNumber() + 1,
-                disjointFromHeader.getBaseFee().orElse(Wei.of(0x3b9aca00)),
-                0,
-                15000000l))
-        .buildHeader();
-  }
-
   MergeCoordinator terminalAncestorMock(final long chainDepth, final boolean hasTerminalPoW) {
     final Difficulty mockTTD = Difficulty.of(1000);
     BlockHeaderTestFixture builder = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE);

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
@@ -184,7 +184,7 @@ private CompletableFuture<Void> prepareBackwardSyncFuture() {
             this,
             FinalBlockConfirmation.confirmationChain(
                 FinalBlockConfirmation.genesisConfirmation(blockchain),
-                FinalBlockConfirmation.finalizedConfirmation(blockchain)))
+                FinalBlockConfirmation.ancestorConfirmation(blockchain)))
         .executeBackwardsSync(null);
   }
 

diff --git a/...main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardsSyncAlgorithm.java b/...main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardsSyncAlgorithm.java
@@ -84,8 +84,8 @@ public CompletableFuture<Void> pickNextStep() {
           firstAncestorHeader::toLogString);
     }
 
-    if (finalBlockConfirmation.finalHeaderReached(firstAncestorHeader)) {
-      LOG.info("Backward sync reached final header, starting Forward sync");
+    if (finalBlockConfirmation.ancestorHeaderReached(firstAncestorHeader)) {
+      LOG.info("Backward sync reached ancestor header, starting Forward sync");
       return executeForwardAsync();
     }
 
@@ -180,9 +180,6 @@ protected void runFinalizedSuccessionRule(
                   () ->
                       new BackwardSyncException(
                           "The header " + oldFinalized.toHexString() + "not found"));
-      if (newFinalizedHeader.getNumber() < oldFinalizedHeader.getNumber()) {
-        throw new BackwardSyncException("Cannot finalize below already finalized...");
-      }
       LOG.info(
           "Updating finalized {} block to new finalized block {}",
           oldFinalizedHeader.toLogString(),

diff --git a/...main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/FinalBlockConfirmation.java b/...main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/FinalBlockConfirmation.java
@@ -18,16 +18,14 @@
 package org.hyperledger.besu.ethereum.eth.sync.backwardsync;
 
 import org.hyperledger.besu.ethereum.chain.MutableBlockchain;
-import org.hyperledger.besu.ethereum.core.Block;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
 
 import java.util.Arrays;
-import java.util.Optional;
 
 public interface FinalBlockConfirmation {
-  boolean finalHeaderReached(final BlockHeader firstHeader);
+  boolean ancestorHeaderReached(final BlockHeader firstHeader);
 
-  public static FinalBlockConfirmation genesisConfirmation(final MutableBlockchain blockchain) {
+  static FinalBlockConfirmation genesisConfirmation(final MutableBlockchain blockchain) {
     final BlockHeader genesisBlockHeader = blockchain.getBlockHeader(0).orElseThrow();
     return firstHeader -> {
       if (firstHeader.getNumber() > 0) {
@@ -45,29 +43,17 @@ public static FinalBlockConfirmation genesisConfirmation(final MutableBlockchain
     };
   }
 
-  public static FinalBlockConfirmation finalizedConfirmation(final MutableBlockchain blockchain) {
-    return firstHeader -> {
-      final Optional<Block> finalized =
-          blockchain.getFinalized().flatMap(blockchain::getBlockByHash);
-      if (finalized.isPresent()
-          && (finalized.get().getHeader().getNumber() >= firstHeader.getNumber())) {
-        throw new BackwardSyncException("Cannot continue below finalized...");
-      }
-      if (blockchain.contains(firstHeader.getParentHash())) {
-        return true;
-      }
-      return false;
-    };
+  static FinalBlockConfirmation ancestorConfirmation(final MutableBlockchain blockchain) {
+    return firstHeader -> blockchain.contains(firstHeader.getParentHash());
   }
 
-  public static FinalBlockConfirmation confirmationChain(
-      final FinalBlockConfirmation... confirmations) {
+  static FinalBlockConfirmation confirmationChain(final FinalBlockConfirmation... confirmations) {
     return firstHeader -> {
       return Arrays.stream(confirmations)
           .reduce(
               false,
               (aBoolean, confirmation2) ->
-                  aBoolean || confirmation2.finalHeaderReached(firstHeader),
+                  aBoolean || confirmation2.ancestorHeaderReached(firstHeader),
               (aBoolean, aBoolean2) -> aBoolean || aBoolean2);
     };
   }

diff --git a/...rc/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncAlgSpec.java b/...rc/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncAlgSpec.java
@@ -100,7 +100,7 @@ public void setUp() throws Exception {
                 context,
                 FinalBlockConfirmation.confirmationChain(
                     FinalBlockConfirmation.genesisConfirmation(localBlockchain),
-                    FinalBlockConfirmation.finalizedConfirmation(localBlockchain))));
+                    FinalBlockConfirmation.ancestorConfirmation(localBlockchain))));
     when(context.getProtocolContext().getBlockchain()).thenReturn(localBlockchain);
   }
 
@@ -323,34 +323,6 @@ public void successionShouldKeepFinalizedWhenNotChanged() {
     assertThat(finalized).contains(localBlockchain.getChainHead().getHash());
   }
 
-  @Test
-  public void successionShouldThrowWhenFinalizingBellowPreviousFinalized() {
-    final BackwardsSyncAlgorithm backwardsSyncAlgorithm =
-        new BackwardsSyncAlgorithm(context, firstHeader -> false);
-
-    Optional<Hash> finalized = localBlockchain.getFinalized();
-    assertThat(finalized).isEmpty();
-
-    backwardsSyncAlgorithm.runFinalizedSuccessionRule(
-        localBlockchain, Optional.of(localBlockchain.getChainHead().getHash()));
-
-    finalized = localBlockchain.getFinalized();
-    assertThat(finalized).isPresent();
-    assertThat(finalized).contains(localBlockchain.getChainHead().getHash());
-
-    assertThatThrownBy(
-            () ->
-                backwardsSyncAlgorithm.runFinalizedSuccessionRule(
-                    localBlockchain,
-                    Optional.of(
-                        localBlockchain
-                            .getBlockHeader(LOCAL_HEIGHT - 1)
-                            .orElseThrow()
-                            .getBlockHash())))
-        .isInstanceOf(BackwardSyncException.class)
-        .hasMessageContaining("Cannot finalize below already finalized");
-  }
-
   @Test
   public void successionShouldUpdateOldFinalizedToNewFinalized() {
     final BackwardsSyncAlgorithm backwardsSyncAlgorithm =