Finalised blocks should not prevent reorgs in BWS (#4097)

Signed-off-by: Jiri Peinlich <jiri.peinlich@gmail.com>

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContext.java

@@ -23,7 +23,6 @@ import org.hyperledger.besu.ethereum.ProtocolContext;
 import org.hyperledger.besu.ethereum.chain.BadBlockManager;
 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.EthContext;
 import org.hyperledger.besu.ethereum.eth.sync.state.SyncState;
 import org.hyperledger.besu.ethereum.mainnet.HeaderValidationMode;
@@ -253,7 +252,6 @@ public class BackwardSyncContext {
 
   protected Void saveBlock(final Block block) {
     traceLambda(LOG, "Going to validate block {}", block::toLogString);
-    checkFinalizedSuccessionRuleBeforeSave(block);
     var optResult =
         this.getBlockValidatorForBlock(block)
             .validateAndProcessBlock(
@@ -285,63 +283,6 @@ public class BackwardSyncContext {
     return null;
   }
 
-  @VisibleForTesting
-  protected synchronized void checkFinalizedSuccessionRuleBeforeSave(final Block block) {
-    final Optional<Hash> finalized = findMaybeFinalized();
-    if (finalized.isPresent()) {
-      final Optional<BlockHeader> maybeFinalizedHeader =
-          protocolContext
-              .getBlockchain()
-              .getBlockByHash(finalized.get())
-              .map(Block::getHeader)
-              .or(() -> backwardChain.getHeader(finalized.get()));
-      if (maybeFinalizedHeader.isEmpty()) {
-        throw new BackwardSyncException(
-            "We know a block "
-                + finalized.get().toHexString()
-                + " was finalized, but we don't have it downloaded yet, cannot save new block",
-            true);
-      }
-      final BlockHeader finalizedHeader = maybeFinalizedHeader.get();
-      if (finalizedHeader.getHash().equals(block.getHash())) {
-        debugLambda(LOG, "Saving new finalized block {}", block::toLogString);
-        return;
-      }
-
-      if (finalizedHeader.getNumber() == block.getHeader().getNumber()) {
-        throw new BackwardSyncException(
-            "This block is not the target finalized block. Is "
-                + block.toLogString()
-                + " but was expecting "
-                + finalizedHeader.toLogString());
-      }
-      if (!getProtocolContext().getBlockchain().contains(finalizedHeader.getHash())) {
-        debugLambda(
-            LOG,
-            "Saving block {} before finalized {} reached",
-            block::toLogString,
-            finalizedHeader::toLogString); // todo: some check here??
-        return;
-      }
-      final Hash canonicalHash =
-          getProtocolContext()
-              .getBlockchain()
-              .getBlockByNumber(finalizedHeader.getNumber())
-              .orElseThrow()
-              .getHash();
-      if (finalizedHeader.getNumber() < block.getHeader().getNumber()
-          && !canonicalHash.equals(finalizedHeader.getHash())) {
-        throw new BackwardSyncException(
-            "Finalized block "
-                + finalizedHeader.toLogString()
-                + " is not on canonical chain. Canonical is"
-                + canonicalHash.toHexString()
-                + ". We need to reorg before saving this block.");
-      }
-    }
-    LOG.debug("Finalized block not known yet...");
-  }
-
   @VisibleForTesting
   protected void possiblyMoveHead(final Block lastSavedBlock) {
     final MutableBlockchain blockchain = getProtocolContext().getBlockchain();

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContextTest.java

@@ -258,63 +258,6 @@ public class BackwardSyncContextTest {
     assertThat(localBlockchain.getChainHeadBlock().getHeader().getNumber()).isEqualTo(4);
   }
 
-  @Test
-  public void testSuccessionRuleAfterUpdatingFinalized() {
-
-    backwardChain.appendTrustedBlock(
-        remoteBlockchain.getBlockByNumber(LOCAL_HEIGHT + 1).orElseThrow());
-    // null check
-    context.updateHeads(null, null);
-    context.checkFinalizedSuccessionRuleBeforeSave(null);
-    // zero check
-    context.updateHeads(null, Hash.ZERO);
-    context.checkFinalizedSuccessionRuleBeforeSave(null);
-
-    // cannot save if we don't know what is finalized
-    context.updateHeads(
-        null, remoteBlockchain.getBlockHashByNumber(LOCAL_HEIGHT + 10).orElseThrow());
-    assertThatThrownBy(() -> context.checkFinalizedSuccessionRuleBeforeSave(null))
-        .isInstanceOf(BackwardSyncException.class)
-        .hasMessageContaining(
-            "was finalized, but we don't have it downloaded yet, cannot save new block");
-
-    // updating with new finalized
-    context.updateHeads(
-        null, remoteBlockchain.getBlockHashByNumber(LOCAL_HEIGHT + 1).orElseThrow());
-    context.checkFinalizedSuccessionRuleBeforeSave(
-        remoteBlockchain.getBlockByNumber(LOCAL_HEIGHT + 1).orElseThrow());
-
-    // updating when we know finalized is in futre
-    context.updateHeads(
-        null, remoteBlockchain.getBlockHashByNumber(LOCAL_HEIGHT + 4).orElseThrow());
-    context.checkFinalizedSuccessionRuleBeforeSave(
-        remoteBlockchain.getBlockByNumber(LOCAL_HEIGHT + 1).orElseThrow());
-
-    // updating with block that is not finalized when we expected finalized on this height
-    context.updateHeads(
-        null, remoteBlockchain.getBlockHashByNumber(LOCAL_HEIGHT + 1).orElseThrow());
-    assertThatThrownBy(
-            () ->
-                context.checkFinalizedSuccessionRuleBeforeSave(
-                    createUncle(LOCAL_HEIGHT + 1, localBlockchain.getChainHeadHash())))
-        .isInstanceOf(BackwardSyncException.class)
-        .hasMessageContaining("This block is not the target finalized block");
-
-    // updating with a block when finalized is not on canonical chain
-    context.updateHeads(null, uncle.getHash());
-    assertThatThrownBy(
-            () ->
-                context.checkFinalizedSuccessionRuleBeforeSave(
-                    remoteBlockchain.getBlockByNumber(LOCAL_HEIGHT + 1).orElseThrow()))
-        .isInstanceOf(BackwardSyncException.class)
-        .hasMessageContaining("is not on canonical chain. Canonical is");
-
-    // updating when finalized is on canonical chain
-    context.updateHeads(null, localBlockchain.getBlockHashByNumber(UNCLE_HEIGHT).orElseThrow());
-    context.checkFinalizedSuccessionRuleBeforeSave(
-        remoteBlockchain.getBlockByNumber(LOCAL_HEIGHT + 1).orElseThrow());
-  }
-
   @Test
   public void shouldProcessExceptionsCorrectly() {
     assertThatThrownBy(