Limit memory used in handling invalid blocks (#6138)

Signed-off-by: Jason Frame <jason.frame@consensys.net>
hyperledger · Nov 8, 2023 · 701cbb0 · 701cbb0
1 parent 8319fae
commit 701cbb0
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 10 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -24,6 +24,7 @@
 - Upgrade netty to address CVE-2023-44487, CVE-2023-34462 [#6100](https://github.com/hyperledger/besu/pull/6100)
 - Upgrade grpc to address CVE-2023-32731, CVE-2023-33953, CVE-2023-44487, CVE-2023-4785 [#6100](https://github.com/hyperledger/besu/pull/6100)
 - Fix blob gas calculation in reference tests [#6107](https://github.com/hyperledger/besu/pull/6107)
+- Limit memory used in handling invalid blocks [#6138](https://github.com/hyperledger/besu/pull/6138)
 
 ---
 

diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/BadBlockManager.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/BadBlockManager.java
@@ -28,12 +28,13 @@
 
 public class BadBlockManager {
 
+  public static final int MAX_BAD_BLOCKS_SIZE = 100;
   private final Cache<Hash, Block> badBlocks =
-      CacheBuilder.newBuilder().maximumSize(100).concurrencyLevel(1).build();
+      CacheBuilder.newBuilder().maximumSize(MAX_BAD_BLOCKS_SIZE).concurrencyLevel(1).build();
   private final Cache<Hash, BlockHeader> badHeaders =
-      CacheBuilder.newBuilder().maximumSize(100).concurrencyLevel(1).build();
+      CacheBuilder.newBuilder().maximumSize(MAX_BAD_BLOCKS_SIZE).concurrencyLevel(1).build();
   private final Cache<Hash, Hash> latestValidHashes =
-      CacheBuilder.newBuilder().maximumSize(100).concurrencyLevel(1).build();
+      CacheBuilder.newBuilder().maximumSize(MAX_BAD_BLOCKS_SIZE).concurrencyLevel(1).build();
 
   /**
    * Add a new invalid block.

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
@@ -19,6 +19,7 @@
 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;
@@ -46,6 +47,7 @@ public class BackwardSyncContext {
   private static final int DEFAULT_MAX_RETRIES = 20;
   private static final long MILLIS_DELAY_BETWEEN_PROGRESS_LOG = 10_000L;
   private static final long DEFAULT_MILLIS_BETWEEN_RETRIES = 5000;
+  private static final int DEFAULT_MAX_CHAIN_EVENT_ENTRIES = BadBlockManager.MAX_BAD_BLOCKS_SIZE;
 
   protected final ProtocolContext protocolContext;
   private final ProtocolSchedule protocolSchedule;
@@ -56,6 +58,7 @@ public class BackwardSyncContext {
   private final BackwardChain backwardChain;
   private int batchSize = BATCH_SIZE;
   private final int maxRetries;
+  private final int maxBadChainEventEntries;
   private final long millisBetweenRetries = DEFAULT_MILLIS_BETWEEN_RETRIES;
   private final Subscribers<BadChainListener> badChainListeners = Subscribers.create();
 
@@ -73,7 +76,8 @@ public BackwardSyncContext(
         ethContext,
         syncState,
         backwardChain,
-        DEFAULT_MAX_RETRIES);
+        DEFAULT_MAX_RETRIES,
+        DEFAULT_MAX_CHAIN_EVENT_ENTRIES);
   }
 
   public BackwardSyncContext(
@@ -83,7 +87,8 @@ public BackwardSyncContext(
       final EthContext ethContext,
       final SyncState syncState,
       final BackwardChain backwardChain,
-      final int maxRetries) {
+      final int maxRetries,
+      final int maxBadChainEventEntries) {
 
     this.protocolContext = protocolContext;
     this.protocolSchedule = protocolSchedule;
@@ -92,6 +97,7 @@ public BackwardSyncContext(
     this.syncState = syncState;
     this.backwardChain = backwardChain;
     this.maxRetries = maxRetries;
+    this.maxBadChainEventEntries = maxBadChainEventEntries;
   }
 
   public synchronized boolean isSyncing() {
@@ -368,7 +374,9 @@ private void emitBadChainEvent(final Block badBlock) {
 
     Optional<Hash> descendant = backwardChain.getDescendant(badBlock.getHash());
 
-    while (descendant.isPresent()) {
+    while (descendant.isPresent()
+        && badBlockDescendants.size() < maxBadChainEventEntries
+        && badBlockHeaderDescendants.size() < maxBadChainEventEntries) {
       final Optional<Block> block = backwardChain.getBlock(descendant.get());
       if (block.isPresent()) {
         badBlockDescendants.add(block.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
@@ -17,10 +17,11 @@
 
 package org.hyperledger.besu.ethereum.eth.sync.backwardsync;
 
-import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
-import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.hyperledger.besu.ethereum.core.InMemoryKeyValueStorageProvider.createInMemoryBlockchain;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
@@ -64,6 +65,7 @@
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.Answers;
+import org.mockito.ArgumentCaptor;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.Spy;
@@ -75,11 +77,12 @@
 @MockitoSettings(strictness = Strictness.LENIENT)
 public class BackwardSyncContextTest {
 
-  public static final int REMOTE_HEIGHT = 50;
   public static final int LOCAL_HEIGHT = 25;
+  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 TEST_MAX_BAD_CHAIN_EVENT_ENTRIES = 25;
 
   private BackwardSyncContext context;
 
@@ -173,7 +176,8 @@ public void setup() {
                 ethContext,
                 syncState,
                 backwardChain,
-                NUM_OF_RETRIES));
+                NUM_OF_RETRIES,
+                TEST_MAX_BAD_CHAIN_EVENT_ENTRIES));
     doReturn(true).when(context).isReady();
     doReturn(2).when(context).getBatchSize();
   }
@@ -347,6 +351,72 @@ public void shouldEmitBadChainEvent() {
             block, Collections.emptyList(), List.of(childBlockHeader, grandChildBlockHeader));
   }
 
+  @Test
+  @SuppressWarnings("unchecked")
+  public void shouldEmitBadChainEventWithIncludedBlockHeadersLimitedToMaxBadChainEventsSize() {
+    Block block = Mockito.mock(Block.class);
+    BlockHeader blockHeader = Mockito.mock(BlockHeader.class);
+    when(block.getHash()).thenReturn(Hash.fromHexStringLenient("0x42"));
+    when(blockHeader.getHash()).thenReturn(Hash.fromHexStringLenient("0x42"));
+    BadChainListener badChainListener = Mockito.mock(BadChainListener.class);
+    context.subscribeBadChainListener(badChainListener);
+
+    backwardChain.clear();
+
+    for (int i = REMOTE_HEIGHT; i >= 0; i--) {
+      backwardChain.prependAncestorsHeader(remoteBlockchain.getBlockByNumber(i).get().getHeader());
+    }
+    backwardChain.prependAncestorsHeader(blockHeader);
+
+    doReturn(blockValidator).when(context).getBlockValidatorForBlock(any());
+    BlockProcessingResult result = new BlockProcessingResult("custom error");
+    doReturn(result).when(blockValidator).validateAndProcessBlock(any(), any(), any(), any());
+
+    assertThatThrownBy(() -> context.saveBlock(block))
+        .isInstanceOf(BackwardSyncException.class)
+        .hasMessageContaining("custom error");
+
+    final ArgumentCaptor<List<BlockHeader>> badBlockHeaderDescendants =
+        ArgumentCaptor.forClass(List.class);
+    verify(badChainListener)
+        .onBadChain(eq(block), eq(Collections.emptyList()), badBlockHeaderDescendants.capture());
+    assertThat(badBlockHeaderDescendants.getValue()).hasSize(TEST_MAX_BAD_CHAIN_EVENT_ENTRIES);
+  }
+
+  @SuppressWarnings("unchecked")
+  @Test
+  public void shouldEmitBadChainEventWithIncludedBlocksLimitedToMaxBadChainEventsSize() {
+    Block block = Mockito.mock(Block.class);
+    BlockHeader blockHeader = Mockito.mock(BlockHeader.class);
+    when(block.getHash()).thenReturn(Hash.fromHexStringLenient("0x42"));
+    when(blockHeader.getHash()).thenReturn(Hash.fromHexStringLenient("0x42"));
+    BadChainListener badChainListener = Mockito.mock(BadChainListener.class);
+    context.subscribeBadChainListener(badChainListener);
+
+    backwardChain.clear();
+    for (int i = REMOTE_HEIGHT; i >= 0; i--) {
+      backwardChain.prependAncestorsHeader(remoteBlockchain.getBlockByNumber(i).get().getHeader());
+    }
+    backwardChain.prependAncestorsHeader(blockHeader);
+
+    for (int i = REMOTE_HEIGHT; i >= 0; i--) {
+      backwardChain.appendTrustedBlock(remoteBlockchain.getBlockByNumber(i).get());
+    }
+
+    doReturn(blockValidator).when(context).getBlockValidatorForBlock(any());
+    BlockProcessingResult result = new BlockProcessingResult("custom error");
+    doReturn(result).when(blockValidator).validateAndProcessBlock(any(), any(), any(), any());
+
+    assertThatThrownBy(() -> context.saveBlock(block))
+        .isInstanceOf(BackwardSyncException.class)
+        .hasMessageContaining("custom error");
+
+    final ArgumentCaptor<List<Block>> badBlockDescendants = ArgumentCaptor.forClass(List.class);
+    verify(badChainListener)
+        .onBadChain(eq(block), badBlockDescendants.capture(), eq(Collections.emptyList()));
+    assertThat(badBlockDescendants.getValue()).hasSize(TEST_MAX_BAD_CHAIN_EVENT_ENTRIES);
+  }
+
   @Test
   public void shouldFailAfterMaxNumberOfRetries() {
     doReturn(CompletableFuture.failedFuture(new Exception()))