From 701cbb000f7e0205e2e7f9f5bbe103e3343d2937 Mon Sep 17 00:00:00 2001 From: Jason Frame Date: Thu, 9 Nov 2023 09:23:53 +1000 Subject: [PATCH] Limit memory used in handling invalid blocks (#6138) Signed-off-by: Jason Frame --- CHANGELOG.md | 1 + .../besu/ethereum/chain/BadBlockManager.java | 7 +- .../backwardsync/BackwardSyncContext.java | 14 +++- .../backwardsync/BackwardSyncContextTest.java | 78 ++++++++++++++++++- 4 files changed, 90 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a3dbc24f571..13816f6bf15 100644 --- 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 index da7055b51c3..8d5d57a9975 100644 --- 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 badBlocks = - CacheBuilder.newBuilder().maximumSize(100).concurrencyLevel(1).build(); + CacheBuilder.newBuilder().maximumSize(MAX_BAD_BLOCKS_SIZE).concurrencyLevel(1).build(); private final Cache badHeaders = - CacheBuilder.newBuilder().maximumSize(100).concurrencyLevel(1).build(); + CacheBuilder.newBuilder().maximumSize(MAX_BAD_BLOCKS_SIZE).concurrencyLevel(1).build(); private final Cache 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/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContext.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContext.java index fc02409ce78..6a681bec288 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContext.java +++ b/ethereum/eth/src/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 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 descendant = backwardChain.getDescendant(badBlock.getHash()); - while (descendant.isPresent()) { + while (descendant.isPresent() + && badBlockDescendants.size() < maxBadChainEventEntries + && badBlockHeaderDescendants.size() < maxBadChainEventEntries) { final Optional block = backwardChain.getBlock(descendant.get()); if (block.isPresent()) { badBlockDescendants.add(block.get()); diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContextTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContextTest.java index b5909c4d429..3187f55ef9f 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContextTest.java +++ b/ethereum/eth/src/test/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> 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> 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()))