From 645fdd08ee940cb770d2415a7a958b32d0c3b94c Mon Sep 17 00:00:00 2001 From: mbaxter Date: Thu, 23 Jun 2022 01:12:08 -0400 Subject: [PATCH] [Issue 3867] Make eth subprotocol message size limit configurable (#4002) Signed-off-by: Meredith Baxter --- .../options/unstable/EthProtocolOptions.java | 14 +++++ .../cli/options/EthProtocolOptionsTest.java | 40 ++++++++---- .../eth/EthProtocolConfiguration.java | 62 ++++++++++++++++--- .../eth/manager/EthProtocolManager.java | 13 +++- .../eth/manager/EthProtocolManagerTest.java | 33 ++++++++-- .../ethereum/eth/manager/EthServerTest.java | 17 +++-- .../TransactionPoolFactoryTest.java | 2 +- .../besu/util/number/ByteUnits.java | 22 +++++++ 8 files changed, 169 insertions(+), 34 deletions(-) create mode 100644 util/src/main/java/org/hyperledger/besu/util/number/ByteUnits.java diff --git a/besu/src/main/java/org/hyperledger/besu/cli/options/unstable/EthProtocolOptions.java b/besu/src/main/java/org/hyperledger/besu/cli/options/unstable/EthProtocolOptions.java index 58c4533376..b1b06a52b5 100644 --- a/besu/src/main/java/org/hyperledger/besu/cli/options/unstable/EthProtocolOptions.java +++ b/besu/src/main/java/org/hyperledger/besu/cli/options/unstable/EthProtocolOptions.java @@ -25,6 +25,7 @@ import java.util.List; import picocli.CommandLine; public class EthProtocolOptions implements CLIOptions { + private static final String MAX_MESSAGE_SIZE_FLAG = "--Xeth-max-message-size"; private static final String MAX_GET_HEADERS_FLAG = "--Xewp-max-get-headers"; private static final String MAX_GET_BODIES_FLAG = "--Xewp-max-get-bodies"; private static final String MAX_GET_RECEIPTS_FLAG = "--Xewp-max-get-receipts"; @@ -33,6 +34,15 @@ public class EthProtocolOptions implements CLIOptions private static final String LEGACY_ETH_64_FORK_ID_ENABLED = "--compatibility-eth64-forkid-enabled"; + @CommandLine.Option( + hidden = true, + names = {MAX_MESSAGE_SIZE_FLAG}, + paramLabel = "", + description = + "Maximum message size (in bytes) for Ethereum Wire Protocol messages. (default: ${DEFAULT-VALUE})") + private PositiveNumber maxMessageSize = + PositiveNumber.fromInt(EthProtocolConfiguration.DEFAULT_MAX_MESSAGE_SIZE); + @CommandLine.Option( hidden = true, names = {MAX_GET_HEADERS_FLAG}, @@ -93,6 +103,7 @@ public class EthProtocolOptions implements CLIOptions public static EthProtocolOptions fromConfig(final EthProtocolConfiguration config) { final EthProtocolOptions options = create(); + options.maxMessageSize = PositiveNumber.fromInt(config.getMaxMessageSize()); options.maxGetBlockHeaders = PositiveNumber.fromInt(config.getMaxGetBlockHeaders()); options.maxGetBlockBodies = PositiveNumber.fromInt(config.getMaxGetBlockBodies()); options.maxGetReceipts = PositiveNumber.fromInt(config.getMaxGetReceipts()); @@ -105,6 +116,7 @@ public class EthProtocolOptions implements CLIOptions @Override public EthProtocolConfiguration toDomainObject() { return EthProtocolConfiguration.builder() + .maxMessageSize(maxMessageSize) .maxGetBlockHeaders(maxGetBlockHeaders) .maxGetBlockBodies(maxGetBlockBodies) .maxGetReceipts(maxGetReceipts) @@ -117,6 +129,8 @@ public class EthProtocolOptions implements CLIOptions @Override public List getCLIOptions() { return Arrays.asList( + MAX_MESSAGE_SIZE_FLAG, + OptionParser.format(maxMessageSize.getValue()), MAX_GET_HEADERS_FLAG, OptionParser.format(maxGetBlockHeaders.getValue()), MAX_GET_BODIES_FLAG, diff --git a/besu/src/test/java/org/hyperledger/besu/cli/options/EthProtocolOptionsTest.java b/besu/src/test/java/org/hyperledger/besu/cli/options/EthProtocolOptionsTest.java index 0169fe4017..a95085ee8b 100644 --- a/besu/src/test/java/org/hyperledger/besu/cli/options/EthProtocolOptionsTest.java +++ b/besu/src/test/java/org/hyperledger/besu/cli/options/EthProtocolOptionsTest.java @@ -20,7 +20,6 @@ import static org.mockito.Mockito.verifyNoInteractions; import org.hyperledger.besu.cli.options.unstable.EthProtocolOptions; import org.hyperledger.besu.ethereum.eth.EthProtocolConfiguration; -import org.hyperledger.besu.util.number.PositiveNumber; import org.junit.Test; import org.junit.runner.RunWith; @@ -30,6 +29,28 @@ import org.mockito.junit.MockitoJUnitRunner; public class EthProtocolOptionsTest extends AbstractCLIOptionsTest { + @Test + public void parsesValidMaxMessageSizeOptions() { + + final TestBesuCommand cmd = parseCommand("--Xeth-max-message-size", "4"); + + final EthProtocolOptions options = getOptionsFromBesuCommand(cmd); + final EthProtocolConfiguration config = options.toDomainObject(); + assertThat(config.getMaxMessageSize()).isEqualTo(4); + assertThat(commandOutput.toString(UTF_8)).isEmpty(); + assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); + } + + @Test + public void parsesInvalidMaxMessageSizeOptionsShouldFail() { + parseCommand("--Xeth-max-message-size", "-4"); + verifyNoInteractions(mockRunnerBuilder); + assertThat(commandOutput.toString(UTF_8)).isEmpty(); + assertThat(commandErrorOutput.toString(UTF_8)) + .contains( + "Invalid value for option '--Xeth-max-message-size': cannot convert '-4' to PositiveNumber"); + } + @Test public void parsesValidEwpMaxGetHeadersOptions() { @@ -127,17 +148,12 @@ public class EthProtocolOptionsTest @Override EthProtocolConfiguration createCustomizedDomainObject() { return EthProtocolConfiguration.builder() - .maxGetBlockHeaders( - PositiveNumber.fromInt(EthProtocolConfiguration.DEFAULT_MAX_GET_BLOCK_HEADERS + 2)) - .maxGetBlockBodies( - PositiveNumber.fromInt(EthProtocolConfiguration.DEFAULT_MAX_GET_BLOCK_BODIES + 2)) - .maxGetReceipts( - PositiveNumber.fromInt(EthProtocolConfiguration.DEFAULT_MAX_GET_RECEIPTS + 2)) - .maxGetNodeData( - PositiveNumber.fromInt(EthProtocolConfiguration.DEFAULT_MAX_GET_NODE_DATA + 2)) - .maxGetPooledTransactions( - PositiveNumber.fromInt( - EthProtocolConfiguration.DEFAULT_MAX_GET_POOLED_TRANSACTIONS + 2)) + .maxMessageSize(EthProtocolConfiguration.DEFAULT_MAX_MESSAGE_SIZE * 2) + .maxGetBlockHeaders(EthProtocolConfiguration.DEFAULT_MAX_GET_BLOCK_HEADERS + 2) + .maxGetBlockBodies(EthProtocolConfiguration.DEFAULT_MAX_GET_BLOCK_BODIES + 2) + .maxGetReceipts(EthProtocolConfiguration.DEFAULT_MAX_GET_RECEIPTS + 2) + .maxGetNodeData(EthProtocolConfiguration.DEFAULT_MAX_GET_NODE_DATA + 2) + .maxGetPooledTransactions(EthProtocolConfiguration.DEFAULT_MAX_GET_POOLED_TRANSACTIONS + 2) .build(); } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/EthProtocolConfiguration.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/EthProtocolConfiguration.java index 64a7e0a8ee..61dd935baa 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/EthProtocolConfiguration.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/EthProtocolConfiguration.java @@ -14,6 +14,7 @@ */ package org.hyperledger.besu.ethereum.eth; +import org.hyperledger.besu.util.number.ByteUnits; import org.hyperledger.besu.util.number.PositiveNumber; import java.util.Objects; @@ -22,6 +23,7 @@ import com.google.common.base.MoreObjects; public class EthProtocolConfiguration { + public static final int DEFAULT_MAX_MESSAGE_SIZE = 10 * ByteUnits.MEGABYTE; public static final int DEFAULT_MAX_GET_BLOCK_HEADERS = 192; public static final int DEFAULT_MAX_GET_BLOCK_BODIES = 128; public static final int DEFAULT_MAX_GET_RECEIPTS = 256; @@ -29,6 +31,11 @@ public class EthProtocolConfiguration { public static final int DEFAULT_MAX_GET_POOLED_TRANSACTIONS = 256; public static final boolean DEFAULT_LEGACY_ETH_64_FORK_ID_ENABLED = false; + // Limit the size of p2p messages (in bytes) + private final int maxMessageSize; + + // These options impose limits on the max number of elements returned when responding to + // peers' p2p RPC requests private final int maxGetBlockHeaders; private final int maxGetBlockBodies; private final int maxGetReceipts; @@ -36,13 +43,15 @@ public class EthProtocolConfiguration { private final int maxGetPooledTransactions; private final boolean legacyEth64ForkIdEnabled; - public EthProtocolConfiguration( + private EthProtocolConfiguration( + final int maxMessageSize, final int maxGetBlockHeaders, final int maxGetBlockBodies, final int maxGetReceipts, final int maxGetNodeData, final int maxGetPooledTransactions, final boolean legacyEth64ForkIdEnabled) { + this.maxMessageSize = maxMessageSize; this.maxGetBlockHeaders = maxGetBlockHeaders; this.maxGetBlockBodies = maxGetBlockBodies; this.maxGetReceipts = maxGetReceipts; @@ -52,19 +61,17 @@ public class EthProtocolConfiguration { } public static EthProtocolConfiguration defaultConfig() { - return new EthProtocolConfiguration( - DEFAULT_MAX_GET_BLOCK_HEADERS, - DEFAULT_MAX_GET_BLOCK_BODIES, - DEFAULT_MAX_GET_RECEIPTS, - DEFAULT_MAX_GET_NODE_DATA, - DEFAULT_MAX_GET_POOLED_TRANSACTIONS, - DEFAULT_LEGACY_ETH_64_FORK_ID_ENABLED); + return builder().build(); } public static Builder builder() { return new Builder(); } + public int getMaxMessageSize() { + return maxMessageSize; + } + public int getMaxGetBlockHeaders() { return maxGetBlockHeaders; } @@ -122,6 +129,9 @@ public class EthProtocolConfiguration { } public static class Builder { + private PositiveNumber maxMessageSize = + PositiveNumber.fromInt(EthProtocolConfiguration.DEFAULT_MAX_MESSAGE_SIZE); + private PositiveNumber maxGetBlockHeaders = PositiveNumber.fromInt(EthProtocolConfiguration.DEFAULT_MAX_GET_BLOCK_HEADERS); @@ -140,31 +150,66 @@ public class EthProtocolConfiguration { private boolean legacyEth64ForkIdEnabled = EthProtocolConfiguration.DEFAULT_LEGACY_ETH_64_FORK_ID_ENABLED; + public Builder maxMessageSize(final PositiveNumber maxMessageSize) { + this.maxMessageSize = maxMessageSize; + return this; + } + + public Builder maxMessageSize(final int maxMessageSize) { + this.maxMessageSize = PositiveNumber.fromInt(maxMessageSize); + return this; + } + public Builder maxGetBlockHeaders(final PositiveNumber maxGetBlockHeaders) { this.maxGetBlockHeaders = maxGetBlockHeaders; return this; } + public Builder maxGetBlockHeaders(final int maxGetBlockHeaders) { + this.maxGetBlockHeaders = PositiveNumber.fromInt(maxGetBlockHeaders); + return this; + } + public Builder maxGetBlockBodies(final PositiveNumber maxGetBlockBodies) { this.maxGetBlockBodies = maxGetBlockBodies; return this; } + public Builder maxGetBlockBodies(final int maxGetBlockBodies) { + this.maxGetBlockBodies = PositiveNumber.fromInt(maxGetBlockBodies); + return this; + } + public Builder maxGetReceipts(final PositiveNumber maxGetReceipts) { this.maxGetReceipts = maxGetReceipts; return this; } + public Builder maxGetReceipts(final int maxGetReceipts) { + this.maxGetReceipts = PositiveNumber.fromInt(maxGetReceipts); + return this; + } + public Builder maxGetNodeData(final PositiveNumber maxGetNodeData) { this.maxGetNodeData = maxGetNodeData; return this; } + public Builder maxGetNodeData(final int maxGetNodeData) { + this.maxGetNodeData = PositiveNumber.fromInt(maxGetNodeData); + return this; + } + public Builder maxGetPooledTransactions(final PositiveNumber maxGetPooledTransactions) { this.maxGetPooledTransactions = maxGetPooledTransactions; return this; } + public Builder maxGetPooledTransactions(final int maxGetPooledTransactions) { + this.maxGetPooledTransactions = PositiveNumber.fromInt(maxGetPooledTransactions); + return this; + } + public Builder legacyEth64ForkIdEnabled(final boolean legacyEth64ForkIdEnabled) { this.legacyEth64ForkIdEnabled = legacyEth64ForkIdEnabled; return this; @@ -172,6 +217,7 @@ public class EthProtocolConfiguration { public EthProtocolConfiguration build() { return new EthProtocolConfiguration( + maxMessageSize.getValue(), maxGetBlockHeaders.getValue(), maxGetBlockBodies.getValue(), maxGetReceipts.getValue(), diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManager.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManager.java index 5cd3879154..c1187c7aa9 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManager.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManager.java @@ -71,6 +71,8 @@ public class EthProtocolManager implements ProtocolManager, MinedBlockObserver { private final Blockchain blockchain; private final BlockBroadcaster blockBroadcaster; private final List peerValidators; + // The max size of messages (in bytes) + private final int maxMessageSize; public EthProtocolManager( final Blockchain blockchain, @@ -89,6 +91,7 @@ public class EthProtocolManager implements ProtocolManager, MinedBlockObserver { this.peerValidators = peerValidators; this.scheduler = scheduler; this.blockchain = blockchain; + this.maxMessageSize = ethereumWireProtocolConfiguration.getMaxMessageSize(); this.shutdown = new CountDownLatch(1); genesisHash = blockchain.getBlockHashByNumber(0L).get(); @@ -241,9 +244,13 @@ public class EthProtocolManager implements ProtocolManager, MinedBlockObserver { return; } - if (messageData.getSize() > 10 * 1_000_000 /*10MB*/) { - LOG.debug("Received message over 10MB. Disconnecting from {}", ethPeer); - ethPeer.disconnect(DisconnectReason.BREACH_OF_PROTOCOL); + if (messageData.getSize() > maxMessageSize) { + LOG.warn( + "Received message exceeding size limit of {} bytes: {} bytes. Disconnecting from {}", + maxMessageSize, + messageData.getSize(), + ethPeer); + ethPeer.disconnect(DisconnectReason.SUBPROTOCOL_TRIGGERED); return; } diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManagerTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManagerTest.java index 8d67d051d7..690ed20941 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManagerTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManagerTest.java @@ -211,7 +211,7 @@ public final class EthProtocolManagerTest { transactionPool, EthProtocolConfiguration.defaultConfig())) { final MessageData messageData = mock(MessageData.class); - when(messageData.getSize()).thenReturn(10 * 1_000_000 + 1 /* just over 10MB*/); + when(messageData.getSize()).thenReturn(EthProtocolConfiguration.DEFAULT_MAX_MESSAGE_SIZE + 1); when(messageData.getCode()).thenReturn(EthPV62.TRANSACTIONS); final MockPeerConnection peer = setupPeer(ethManager, (cap, msg, conn) -> {}); @@ -220,6 +220,25 @@ public final class EthProtocolManagerTest { } } + @Test + public void doNotDisconnectOnLargeMessageWithinLimits() { + try (final EthProtocolManager ethManager = + EthProtocolManagerTestUtil.create( + blockchain, + () -> false, + protocolContext.getWorldStateArchive(), + transactionPool, + EthProtocolConfiguration.defaultConfig())) { + final MessageData messageData = mock(MessageData.class); + when(messageData.getSize()).thenReturn(EthProtocolConfiguration.DEFAULT_MAX_MESSAGE_SIZE); + when(messageData.getCode()).thenReturn(EthPV62.TRANSACTIONS); + final MockPeerConnection peer = setupPeer(ethManager, (cap, msg, conn) -> {}); + + ethManager.processMessage(EthProtocol.ETH63, new DefaultMessage(peer, messageData)); + assertThat(peer.isDisconnected()).isFalse(); + } + } + @Test public void disconnectOnWrongGenesisHash() { try (final EthProtocolManager ethManager = @@ -309,13 +328,15 @@ public final class EthProtocolManagerTest { public void respondToGetHeadersWithinLimits() throws ExecutionException, InterruptedException { final CompletableFuture done = new CompletableFuture<>(); final int limit = 5; + final EthProtocolConfiguration config = + EthProtocolConfiguration.builder().maxGetBlockHeaders(limit).build(); try (final EthProtocolManager ethManager = EthProtocolManagerTestUtil.create( blockchain, () -> false, protocolContext.getWorldStateArchive(), transactionPool, - new EthProtocolConfiguration(limit, limit, limit, limit, limit, false))) { + config)) { final long startBlock = 5L; final int blockCount = 10; final MessageData messageData = @@ -601,13 +622,15 @@ public final class EthProtocolManagerTest { public void respondToGetBodiesWithinLimits() throws ExecutionException, InterruptedException { final CompletableFuture done = new CompletableFuture<>(); final int limit = 5; + final EthProtocolConfiguration config = + EthProtocolConfiguration.builder().maxGetBlockBodies(limit).build(); try (final EthProtocolManager ethManager = EthProtocolManagerTestUtil.create( blockchain, () -> false, protocolContext.getWorldStateArchive(), transactionPool, - new EthProtocolConfiguration(limit, limit, limit, limit, limit, false))) { + config)) { // Setup blocks query final int blockCount = 10; final long startBlock = blockchain.getChainHeadBlockNumber() - blockCount; @@ -739,13 +762,15 @@ public final class EthProtocolManagerTest { public void respondToGetReceiptsWithinLimits() throws ExecutionException, InterruptedException { final CompletableFuture done = new CompletableFuture<>(); final int limit = 5; + final EthProtocolConfiguration config = + EthProtocolConfiguration.builder().maxGetReceipts(limit).build(); try (final EthProtocolManager ethManager = EthProtocolManagerTestUtil.create( blockchain, () -> false, protocolContext.getWorldStateArchive(), transactionPool, - new EthProtocolConfiguration(limit, limit, limit, limit, limit, false))) { + config)) { // Setup blocks query final int blockCount = 10; final long startBlock = blockchain.getChainHeadBlockNumber() - blockCount; diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/EthServerTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/EthServerTest.java index d1b5f05f8d..c715546c3c 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/EthServerTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/EthServerTest.java @@ -50,12 +50,17 @@ public class EthServerTest { @Before public void setUp() { - new EthServer( - blockchain, - worldStateArchive, - transactionPool, - ethMessages, - new EthProtocolConfiguration(2, 2, 2, 2, 2, false)); + final int limit = 2; + final EthProtocolConfiguration ethConfig = + EthProtocolConfiguration.builder() + .maxGetBlockHeaders(limit) + .maxGetBlockBodies(limit) + .maxGetReceipts(limit) + .maxGetNodeData(limit) + .maxGetPooledTransactions(limit) + .build(); + + new EthServer(blockchain, worldStateArchive, transactionPool, ethMessages, ethConfig); } @Test diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolFactoryTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolFactoryTest.java index 347659a87c..9bb380a483 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolFactoryTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolFactoryTest.java @@ -106,7 +106,7 @@ public class TransactionPoolFactoryTest { BigInteger.ONE, mock(WorldStateArchive.class), pool, - new EthProtocolConfiguration(5, 5, 5, 5, 5, false), + EthProtocolConfiguration.defaultConfig(), ethPeers, mock(EthMessages.class), ethContext, diff --git a/util/src/main/java/org/hyperledger/besu/util/number/ByteUnits.java b/util/src/main/java/org/hyperledger/besu/util/number/ByteUnits.java new file mode 100644 index 0000000000..ebc1bb9207 --- /dev/null +++ b/util/src/main/java/org/hyperledger/besu/util/number/ByteUnits.java @@ -0,0 +1,22 @@ +/* + * 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.util.number; + +public class ByteUnits { + public static final int MEGABYTE = 1 << 20; + + private ByteUnits() {} +}