Fix post merge chain reog with invalid block (#4131)

* fix infinite loop if a reorg contain a bad block
* add cache for latest valid ancestors for bad blocks

Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>

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

@@ -34,6 +34,7 @@ import org.hyperledger.besu.ethereum.core.Difficulty;
 import org.hyperledger.besu.ethereum.core.MiningParameters;
 import org.hyperledger.besu.ethereum.core.Transaction;
 import org.hyperledger.besu.ethereum.eth.sync.backwardsync.BackwardSyncContext;
+import org.hyperledger.besu.ethereum.eth.sync.backwardsync.BadChainListener;
 import org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter;
 import org.hyperledger.besu.ethereum.mainnet.AbstractGasLimitSpecification;
 import org.hyperledger.besu.ethereum.mainnet.HeaderValidationMode;
@@ -52,7 +53,7 @@ import org.apache.tuweni.bytes.Bytes32;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-public class MergeCoordinator implements MergeMiningCoordinator {
+public class MergeCoordinator implements MergeMiningCoordinator, BadChainListener {
   private static final Logger LOG = LoggerFactory.getLogger(MergeCoordinator.class);
 
   final AtomicLong targetGasLimit;
@@ -95,6 +96,8 @@ public class MergeCoordinator implements MergeMiningCoordinator {
                 address.or(miningParameters::getCoinbase).orElse(Address.ZERO),
                 this.miningParameters.getMinBlockOccupancyRatio(),
                 parentHeader);
+
+    this.backwardSyncContext.subscribeBadChainListener(this);
   }
 
   @Override
@@ -243,7 +246,7 @@ public class MergeCoordinator implements MergeMiningCoordinator {
   }
 
   private Void logSyncException(final Hash blockHash, final Throwable exception) {
-    LOG.warn("Sync to block hash " + blockHash.toHexString() + " failed", exception);
+    LOG.warn("Sync to block hash " + blockHash.toHexString() + " failed", exception.getMessage());
     return null;
   }
 
@@ -545,6 +548,42 @@ public class MergeCoordinator implements MergeMiningCoordinator {
     return payloadAttributes.getTimestamp() > headBlockHeader.getTimestamp();
   }
 
+  @Override
+  public void onBadChain(
+      final Block badBlock,
+      final List<Block> badBlockDescendants,
+      final List<BlockHeader> badBlockHeaderDescendants) {
+    LOG.trace("Adding bad block {} and all its descendants", badBlock.getHash());
+    final BadBlockManager badBlockManager = getBadBlockManager();
+
+    final Optional<BlockHeader> parentHeader =
+        protocolContext.getBlockchain().getBlockHeader(badBlock.getHeader().getParentHash());
+    final Optional<Hash> maybeLatestValidHash =
+        parentHeader.isPresent() && isPoSHeader(parentHeader.get())
+            ? Optional.of(parentHeader.get().getHash())
+            : Optional.empty();
+
+    badBlockManager.addBadBlock(badBlock);
+
+    badBlockDescendants.forEach(
+        block -> {
+          LOG.trace("Add descendant block {} to bad blocks", block.getHash());
+          badBlockManager.addBadBlock(block);
+          maybeLatestValidHash.ifPresent(
+              latestValidHash ->
+                  badBlockManager.addLatestValidHash(block.getHash(), latestValidHash));
+        });
+
+    badBlockHeaderDescendants.forEach(
+        header -> {
+          LOG.trace("Add descendant header {} to bad blocks", header.getHash());
+          badBlockManager.addBadHeader(header);
+          maybeLatestValidHash.ifPresent(
+              latestValidHash ->
+                  badBlockManager.addLatestValidHash(header.getHash(), latestValidHash));
+        });
+  }
+
   @FunctionalInterface
   interface MergeBlockCreatorFactory {
     MergeBlockCreator forParams(BlockHeader header, Optional<Address> feeRecipient);
@@ -560,11 +599,28 @@ public class MergeCoordinator implements MergeMiningCoordinator {
 
   @Override
   public boolean isBadBlock(final Hash blockHash) {
+    final BadBlockManager badBlocksManager = getBadBlockManager();
+    return badBlocksManager.getBadBlock(blockHash).isPresent()
+        || badBlocksManager.getBadHash(blockHash).isPresent();
+  }
+
+  private BadBlockManager getBadBlockManager() {
     final BadBlockManager badBlocksManager =
         protocolSchedule
             .getByBlockNumber(protocolContext.getBlockchain().getChainHeadBlockNumber())
             .getBadBlocksManager();
-    return badBlocksManager.getBadBlock(blockHash).isPresent()
+    return badBlocksManager;
-        || badBlocksManager.getBadHash(blockHash).isPresent();
+  }
+
+  @Override
+  public Optional<Hash> getLatestValidHashOfBadBlock(Hash blockHash) {
+    return protocolSchedule
+        .getByBlockNumber(protocolContext.getBlockchain().getChainHeadBlockNumber())
+        .getBadBlocksManager()
+        .getLatestValidHash(blockHash);
+  }
+
+  private boolean isPoSHeader(final BlockHeader header) {
+    return header.getDifficulty().equals(Difficulty.ZERO);
   }
 }

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

@@ -68,6 +68,8 @@ public interface MergeMiningCoordinator extends MiningCoordinator {
 
   boolean isBadBlock(Hash blockHash);
 
+  Optional<Hash> getLatestValidHashOfBadBlock(final Hash blockHash);
+
   class ForkchoiceResult {
     public enum Status {
       VALID,

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

@@ -208,4 +208,9 @@ public class TransitionCoordinator extends TransitionUtils<MiningCoordinator>
   public boolean isBadBlock(final Hash blockHash) {
     return mergeCoordinator.isBadBlock(blockHash);
   }
+
+  @Override
+  public Optional<Hash> getLatestValidHashOfBadBlock(final Hash blockHash) {
+    return mergeCoordinator.getLatestValidHashOfBadBlock(blockHash);
+  }
 }

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

@@ -87,7 +87,7 @@ public class EngineForkchoiceUpdated extends ExecutionEngineJsonRpcMethod {
           new EngineUpdateForkchoiceResult(
               INVALID,
               mergeCoordinator
-                  .getLatestValidAncestor(forkChoice.getHeadBlockHash())
+                  .getLatestValidHashOfBadBlock(forkChoice.getHeadBlockHash())
                   .orElse(Hash.ZERO),
               null,
               Optional.of(forkChoice.getHeadBlockHash() + " is an invalid block")));

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

@@ -141,11 +141,13 @@ public class EngineNewPayload extends ExecutionEngineJsonRpcMethod {
       LOG.debug("block already present");
       return respondWith(reqId, blockParam, blockParam.getBlockHash(), VALID);
     }
-    if (mergeCoordinator.isBadBlock(blockParam.getParentHash())) {
+    if (mergeCoordinator.isBadBlock(blockParam.getBlockHash())) {
       return respondWith(
           reqId,
           blockParam,
-          mergeCoordinator.getLatestValidAncestor(blockParam.getParentHash()).orElse(Hash.ZERO),
+          mergeCoordinator
+              .getLatestValidHashOfBadBlock(blockParam.getBlockHash())
+              .orElse(Hash.ZERO),
           INVALID);
     }
 
@@ -174,7 +176,8 @@ public class EngineNewPayload extends ExecutionEngineJsonRpcMethod {
           .appendNewPayloadToSync(block)
           .exceptionally(
               exception -> {
-                LOG.warn("Sync to block " + block.toLogString() + " failed", exception);
+                LOG.warn(
+                    "Sync to block " + block.toLogString() + " failed", exception.getMessage());
                 return null;
               });
       return respondWith(reqId, blockParam, null, SYNCING);

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

@@ -123,7 +123,7 @@ public class EngineForkchoiceUpdatedTest {
     BlockHeader mockHeader = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
     Hash latestValidHash = Hash.hash(Bytes32.fromHexStringLenient("0xcafebabe"));
     when(mergeCoordinator.isBadBlock(mockHeader.getHash())).thenReturn(true);
-    when(mergeCoordinator.getLatestValidAncestor(mockHeader.getHash()))
+    when(mergeCoordinator.getLatestValidHashOfBadBlock(mockHeader.getHash()))
         .thenReturn(Optional.of(latestValidHash));
 
     assertSuccessWithPayloadForForkchoiceResult(

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

@@ -184,13 +184,13 @@ public class EngineNewPayloadTest {
   }
 
   @Test
-  public void shouldReturnInvalidWithLatestValidHashIfDescendingFromBadBlock() {
+  public void shouldReturnInvalidWithLatestValidHashIsABadBlock() {
     BlockHeader mockHeader = createBlockHeader();
     Hash latestValidHash = Hash.hash(Bytes32.fromHexStringLenient("0xcafebabe"));
 
     when(blockchain.getBlockByHash(mockHeader.getHash())).thenReturn(Optional.empty());
-    when(mergeCoordinator.isBadBlock(mockHeader.getParentHash())).thenReturn(true);
+    when(mergeCoordinator.isBadBlock(mockHeader.getHash())).thenReturn(true);
-    when(mergeCoordinator.getLatestValidAncestor(mockHeader.getParentHash()))
+    when(mergeCoordinator.getLatestValidHashOfBadBlock(mockHeader.getHash()))
         .thenReturn(Optional.of(latestValidHash));
 
     var resp = resp(mockPayload(mockHeader, Collections.emptyList()));

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/BadBlockManager.java

@@ -30,6 +30,8 @@ public class BadBlockManager {
       CacheBuilder.newBuilder().maximumSize(100).concurrencyLevel(1).build();
   private final Cache<Hash, BlockHeader> badHeaders =
       CacheBuilder.newBuilder().maximumSize(100).concurrencyLevel(1).build();
+  private final Cache<Hash, Hash> latestValidHashes =
+      CacheBuilder.newBuilder().maximumSize(100).concurrencyLevel(1).build();
 
   /**
    * Add a new invalid block.
@@ -68,4 +70,12 @@ public class BadBlockManager {
   public Optional<BlockHeader> getBadHash(final Hash blockHash) {
     return Optional.ofNullable(badHeaders.getIfPresent(blockHash));
   }
+
+  public void addLatestValidHash(final Hash blockHash, final Hash latestValidHash) {
+    this.latestValidHashes.put(blockHash, latestValidHash);
+  }
+
+  public Optional<Hash> getLatestValidHash(final Hash blockHash) {
+    return Optional.ofNullable(latestValidHashes.getIfPresent(blockHash));
+  }
 }

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

@@ -19,7 +19,6 @@ import static org.hyperledger.besu.util.Slf4jLambdaHelper.debugLambda;
 import static org.slf4j.LoggerFactory.getLogger;
 
 import org.hyperledger.besu.datatypes.Hash;
-import org.hyperledger.besu.ethereum.chain.BadBlockManager;
 import org.hyperledger.besu.ethereum.core.Block;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
 import org.hyperledger.besu.ethereum.core.BlockHeaderFunctions;
@@ -160,6 +159,14 @@ public class BackwardChain {
     hashesToAppend.clear();
   }
 
+  public synchronized Optional<Hash> getDescendant(final Hash blockHash) {
+    return chainStorage.get(blockHash);
+  }
+
+  public synchronized Optional<Block> getBlock(final Hash hash) {
+    return blocks.get(hash);
+  }
+
   public synchronized Optional<BlockHeader> getHeader(final Hash hash) {
     return headers.get(hash);
   }
@@ -180,17 +187,4 @@ public class BackwardChain {
       hashesToAppend.remove(hashToRemove);
     }
   }
-
-  public void addBadChainToManager(final BadBlockManager badBlocksManager, final Hash hash) {
-    final Optional<Hash> ancestor = chainStorage.get(hash);
-    while (ancestor.isPresent()) {
-      final Optional<Block> block = blocks.get(ancestor.get());
-      if (block.isPresent()) {
-        badBlocksManager.addBadBlock(block.get());
-      } else {
-        final Optional<BlockHeader> blockHeader = headers.get(ancestor.get());
-        blockHeader.ifPresent(badBlocksManager::addBadHeader);
-      }
-    }
-  }
 }

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

@@ -21,16 +21,19 @@ import static org.hyperledger.besu.util.Slf4jLambdaHelper.traceLambda;
 import org.hyperledger.besu.datatypes.Hash;
 import org.hyperledger.besu.ethereum.BlockValidator;
 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;
 import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule;
 import org.hyperledger.besu.plugin.services.MetricsSystem;
+import org.hyperledger.besu.util.Subscribers;
 
 import java.time.Duration;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.Optional;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.atomic.AtomicReference;
@@ -64,6 +67,8 @@ public class BackwardSyncContext {
 
   private final long millisBetweenRetries = DEFAULT_MILLIS_BETWEEN_RETRIES;
 
+  private final Subscribers<BadChainListener> badChainListeners = Subscribers.create();
+
   public BackwardSyncContext(
       final ProtocolContext protocolContext,
       final ProtocolSchedule protocolSchedule,
@@ -277,6 +282,10 @@ public class BackwardSyncContext {
     return currentBackwardSyncFuture.get();
   }
 
+  public void subscribeBadChainListener(final BadChainListener badChainListener) {
+    badChainListeners.subscribe(badChainListener);
+  }
+
   // In rare case when we request too many headers/blocks we get response that does not contain all
   // data and we might want to retry with smaller batch size
   public int getBatchSize() {
@@ -308,12 +317,7 @@ public class BackwardSyncContext {
           .appendBlock(block, optResult.blockProcessingOutputs.get().receipts);
       possiblyMoveHead(block);
     } else {
-      final BadBlockManager badBlocksManager =
+      emitBadChainEvent(block);
-          protocolSchedule
-              .getByBlockNumber(getProtocolContext().getBlockchain().getChainHeadBlockNumber())
-              .getBadBlocksManager();
-      badBlocksManager.addBadBlock(block);
-      getBackwardChain().addBadChainToManager(badBlocksManager, block.getHash());
       throw new BackwardSyncException(
           "Cannot save block "
               + block.toLogString()
@@ -360,4 +364,25 @@ public class BackwardSyncContext {
         .map(Optional::get)
         .findFirst();
   }
+
+  private void emitBadChainEvent(final Block badBlock) {
+    final List<Block> badBlockDescendants = new ArrayList<>();
+    final List<BlockHeader> badBlockHeaderDescendants = new ArrayList<>();
+
+    Optional<Hash> descendant = backwardChain.getDescendant(badBlock.getHash());
+
+    while (descendant.isPresent()) {
+      final Optional<Block> block = backwardChain.getBlock(descendant.get());
+      if (block.isPresent()) {
+        badBlockDescendants.add(block.get());
+      } else {
+        backwardChain.getHeader(descendant.get()).ifPresent(badBlockHeaderDescendants::add);
+      }
+
+      descendant = backwardChain.getDescendant(descendant.get());
+    }
+
+    badChainListeners.forEach(
+        listener -> listener.onBadChain(badBlock, badBlockDescendants, badBlockHeaderDescendants));
+  }
 }

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

@@ -0,0 +1,27 @@
+/*
+ * Copyright Hyperledger Besu Contributors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+package org.hyperledger.besu.ethereum.eth.sync.backwardsync;
+
+import org.hyperledger.besu.ethereum.core.Block;
+import org.hyperledger.besu.ethereum.core.BlockHeader;
+
+import java.util.List;
+
+public interface BadChainListener {
+  void onBadChain(
+      final Block badBlock,
+      final List<Block> badBlockDescendants,
+      final List<BlockHeader> badBlockHeaderDescendants);
+}

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

@@ -23,7 +23,6 @@ import static org.hyperledger.besu.ethereum.core.InMemoryKeyValueStorageProvider
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.when;
 
@@ -32,7 +31,6 @@ import org.hyperledger.besu.datatypes.Hash;
 import org.hyperledger.besu.ethereum.BlockValidator;
 import org.hyperledger.besu.ethereum.BlockValidator.Result;
 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.BlockDataGenerator;
@@ -52,6 +50,7 @@ import org.hyperledger.besu.plugin.data.TransactionType;
 import org.hyperledger.besu.plugin.services.MetricsSystem;
 import org.hyperledger.besu.services.kvstore.InMemoryKeyValueStorage;
 
+import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ExecutionException;
@@ -276,21 +275,36 @@ public class BackwardSyncContextTest {
   }
 
   @Test
-  public void makeSureWeRememberBadBlocks() {
+  public void shouldEmitBadChainEvent() {
     Block block = Mockito.mock(Block.class);
-    when(block.getHash()).thenReturn(Hash.ZERO);
+    BlockHeader blockHeader = Mockito.mock(BlockHeader.class);
+    when(block.getHash()).thenReturn(Hash.fromHexStringLenient("0x42"));
+    when(block.getHeader()).thenReturn(blockHeader);
+    when(blockHeader.getHash()).thenReturn(Hash.fromHexStringLenient("0x42"));
+    BadChainListener badChainListener = Mockito.mock(BadChainListener.class);
+    context.subscribeBadChainListener(badChainListener);
+
+    BlockHeader childBlockHeader =
+        remoteBlockchain.getBlockByNumber(LOCAL_HEIGHT + 2).get().getHeader();
+    BlockHeader grandChildBlockHeader =
+        remoteBlockchain.getBlockByNumber(LOCAL_HEIGHT + 1).get().getHeader();
+
+    backwardChain.clear();
+    backwardChain.prependAncestorsHeader(grandChildBlockHeader);
+    backwardChain.prependAncestorsHeader(childBlockHeader);
+    backwardChain.prependAncestorsHeader(block.getHeader());
+
     doReturn(blockValidator).when(context).getBlockValidatorForBlock(any());
     Result result = new Result("custom error");
     doReturn(result).when(blockValidator).validateAndProcessBlock(any(), any(), any(), any());
 
-    final BadBlockManager manager = mock(BadBlockManager.class);
-    doReturn(manager).when(mockProtocolSpec).getBadBlocksManager();
-
     assertThatThrownBy(() -> context.saveBlock(block))
         .isInstanceOf(BackwardSyncException.class)
         .hasMessageContaining("custom error");
 
-    Mockito.verify(manager).addBadBlock(block);
+    Mockito.verify(badChainListener)
+        .onBadChain(
+            block, Collections.emptyList(), List.of(childBlockHeader, grandChildBlockHeader));
   }
 
   @Test