Fix ETH65 causes crashes issues (hyperledger#1601)

This PR adds a waiting list for NewPooledTransactionHashesMessage in order to group several hashes into a single GetPooledTransactionsFromPeerTask Signed-off-by: Karim TAAM <[email protected]> Co-authored-by: Ratan Rai Sur <[email protected]>
elenaizaguirre · Dec 1, 2020 · d4a330b · d4a330b
1 parent 55f7c50
commit d4a330b
Show file tree

Hide file tree

Showing 26 changed files with 499 additions and 215 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,10 +12,10 @@
 * Ibft2 will discard any received messages targeting a chain height <= current head - this resolves some corner cases in system correctness directly following block import. [#1575](https://github.com/hyperledger/besu/pull/1575)
 * EvmTool now throws `UnsupportedForkException` when there is an unknown fork and is YOLOv2 compatible [\#1584](https://github.com/hyperledger/besu/pull/1584)
 * `eth_newFilter` now supports `blockHash` parameter as per the spec [\#1548](https://github.com/hyperledger/besu/issues/1540). (`blockhash` is also still supported.)
+* Fixed an issue that caused loss of peers and desynchronization when eth65 was enabled [\#1601](https://github.com/hyperledger/besu/pull/1601)
 
 #### Previously identified known issues
 
-- [Eth/65 loses peers](KNOWN_ISSUES.md#eth65-loses-peers)
 - [Fast sync when running Besu on cloud providers](KNOWN_ISSUES.md#fast-sync-when-running-besu-on-cloud-providers)
 - [Privacy users with private transactions created using v1.3.4 or earlier](KNOWN_ISSUES.md#privacy-users-with-private-transactions-created-using-v134-or-earlier)
 

diff --git a/KNOWN_ISSUES.md b/KNOWN_ISSUES.md
@@ -5,14 +5,6 @@ in the current release are provided in the [Changelog](CHANGELOG.md).
 
 Known issues are open issues categorized as [Very High or High impact](https://wiki.hyperledger.org/display/BESU/Defect+Prioritisation+Policy). 
 
-## Eth/65 loses peers 
-
-From v1.4.4, `eth/65` is [disabled by default](https://github.com/hyperledger/besu/pull/741). 
-
-If enabled, peers will slowly drop off and eventually Besu will fall out of sync or stop syncing.
-
-A fix for this issue is being actively worked on. 
-
 ## Fast sync when running Besu on cloud providers  
 
 A known [RocksDB issue](https://github.com/facebook/rocksdb/issues/6435) causes fast sync to fail 

diff --git a/...sl/src/main/java/org/hyperledger/besu/tests/acceptance/dsl/node/ThreadBesuNodeRunner.java b/...sl/src/main/java/org/hyperledger/besu/tests/acceptance/dsl/node/ThreadBesuNodeRunner.java
@@ -156,7 +156,7 @@ public void startNode(final BesuNode node) {
             .privacyParameters(node.getPrivacyParameters())
             .nodeKey(new NodeKey(new KeyPairSecurityModule(KeyPairUtil.loadKeyPair(dataDir))))
             .metricsSystem(metricsSystem)
-            .transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
+            .transactionPoolConfiguration(TransactionPoolConfiguration.DEFAULT)
             .ethProtocolConfiguration(EthProtocolConfiguration.defaultConfig())
             .clock(Clock.systemUTC())
             .isRevertReasonEnabled(node.isRevertReasonEnabled())

diff --git a/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java b/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java
@@ -146,6 +146,7 @@
 import org.hyperledger.besu.util.NetworkUtility;
 import org.hyperledger.besu.util.PermissioningConfigurationValidator;
 import org.hyperledger.besu.util.number.Fraction;
+import org.hyperledger.besu.util.number.Percentage;
 import org.hyperledger.besu.util.number.PositiveNumber;
 
 import java.io.File;
@@ -2077,7 +2078,7 @@ private TransactionPoolConfiguration buildTransactionPoolConfiguration() {
         .txPoolMaxSize(txPoolMaxSize)
         .pooledTransactionHashesSize(pooledTransactionHashesSize)
         .pendingTxRetentionPeriod(pendingTxRetentionPeriod)
-        .priceBump(priceBump)
+        .priceBump(Percentage.fromInt(priceBump))
         .txFeeCap(txFeeCap)
         .build();
   }

diff --git a/besu/src/main/java/org/hyperledger/besu/cli/options/unstable/TransactionPoolOptions.java b/besu/src/main/java/org/hyperledger/besu/cli/options/unstable/TransactionPoolOptions.java
@@ -16,17 +16,23 @@
 
 import org.hyperledger.besu.cli.options.CLIOptions;
 import org.hyperledger.besu.cli.options.OptionParser;
+import org.hyperledger.besu.ethereum.eth.transactions.ImmutableTransactionPoolConfiguration;
 import org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolConfiguration;
 
+import java.time.Duration;
 import java.util.Arrays;
 import java.util.List;
 
 import picocli.CommandLine;
 
-public class TransactionPoolOptions implements CLIOptions<TransactionPoolConfiguration.Builder> {
+public class TransactionPoolOptions
+    implements CLIOptions<ImmutableTransactionPoolConfiguration.Builder> {
   private static final String TX_MESSAGE_KEEP_ALIVE_SEC_FLAG =
       "--Xincoming-tx-messages-keep-alive-seconds";
 
+  private static final String ETH65_TX_ANNOUNCED_BUFFERING_PERIOD_FLAG =
+      "--Xeth65-tx-announced-buffering-period-milliseconds";
+
   @CommandLine.Option(
       names = {TX_MESSAGE_KEEP_ALIVE_SEC_FLAG},
       paramLabel = "<INTEGER>",
@@ -37,6 +43,16 @@ public class TransactionPoolOptions implements CLIOptions<TransactionPoolConfigu
   private Integer txMessageKeepAliveSeconds =
       TransactionPoolConfiguration.DEFAULT_TX_MSG_KEEP_ALIVE;
 
+  @CommandLine.Option(
+      names = {ETH65_TX_ANNOUNCED_BUFFERING_PERIOD_FLAG},
+      paramLabel = "<LONG>",
+      hidden = true,
+      description =
+          "The period for which the announced transactions remain in the buffer before being requested from the peers in milliseconds (default: ${DEFAULT-VALUE})",
+      arity = "1")
+  private long eth65TrxAnnouncedBufferingPeriod =
+      TransactionPoolConfiguration.ETH65_TRX_ANNOUNCED_BUFFERING_PERIOD.toMillis();
+
   private TransactionPoolOptions() {}
 
   public static TransactionPoolOptions create() {
@@ -46,18 +62,24 @@ public static TransactionPoolOptions create() {
   public static TransactionPoolOptions fromConfig(final TransactionPoolConfiguration config) {
     final TransactionPoolOptions options = TransactionPoolOptions.create();
     options.txMessageKeepAliveSeconds = config.getTxMessageKeepAliveSeconds();
+    options.eth65TrxAnnouncedBufferingPeriod =
+        config.getEth65TrxAnnouncedBufferingPeriod().toMillis();
     return options;
   }
 
   @Override
-  public TransactionPoolConfiguration.Builder toDomainObject() {
-    return TransactionPoolConfiguration.builder()
-        .txMessageKeepAliveSeconds(txMessageKeepAliveSeconds);
+  public ImmutableTransactionPoolConfiguration.Builder toDomainObject() {
+    return ImmutableTransactionPoolConfiguration.builder()
+        .txMessageKeepAliveSeconds(txMessageKeepAliveSeconds)
+        .eth65TrxAnnouncedBufferingPeriod(Duration.ofMillis(eth65TrxAnnouncedBufferingPeriod));
   }
 
   @Override
   public List<String> getCLIOptions() {
     return Arrays.asList(
-        TX_MESSAGE_KEEP_ALIVE_SEC_FLAG, OptionParser.format(txMessageKeepAliveSeconds));
+        TX_MESSAGE_KEEP_ALIVE_SEC_FLAG,
+        OptionParser.format(txMessageKeepAliveSeconds),
+        ETH65_TX_ANNOUNCED_BUFFERING_PERIOD_FLAG,
+        OptionParser.format(eth65TrxAnnouncedBufferingPeriod));
   }
 }
diff --git a/besu/src/test/java/org/hyperledger/besu/PrivacyReorgTest.java b/besu/src/test/java/org/hyperledger/besu/PrivacyReorgTest.java
@@ -167,7 +167,7 @@ public void setUp() throws IOException {
             .dataDirectory(dataDir)
             .clock(TestClock.fixed())
             .privacyParameters(privacyParameters)
-            .transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
+            .transactionPoolConfiguration(TransactionPoolConfiguration.DEFAULT)
             .gasLimitCalculator(GasLimitCalculator.constant())
             .build();
   }

diff --git a/besu/src/test/java/org/hyperledger/besu/PrivacyTest.java b/besu/src/test/java/org/hyperledger/besu/PrivacyTest.java
@@ -113,7 +113,7 @@ private BesuController setUpControllerWithPrivacyEnabled(final boolean onChainEn
         .dataDirectory(dataDir)
         .clock(TestClock.fixed())
         .privacyParameters(privacyParameters)
-        .transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
+        .transactionPoolConfiguration(TransactionPoolConfiguration.DEFAULT)
         .gasLimitCalculator(GasLimitCalculator.constant())
         .build();
   }

diff --git a/besu/src/test/java/org/hyperledger/besu/RunnerTest.java b/besu/src/test/java/org/hyperledger/besu/RunnerTest.java
@@ -164,7 +164,7 @@ private void syncFromGenesis(final SyncMode mode, final GenesisConfigFile genesi
             .metricsSystem(noOpMetricsSystem)
             .privacyParameters(PrivacyParameters.DEFAULT)
             .clock(TestClock.fixed())
-            .transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
+            .transactionPoolConfiguration(TransactionPoolConfiguration.DEFAULT)
             .storageProvider(createKeyValueStorageProvider(dataDirAhead, dbAhead))
             .gasLimitCalculator(GasLimitCalculator.constant())
             .build()) {
@@ -184,7 +184,7 @@ private void syncFromGenesis(final SyncMode mode, final GenesisConfigFile genesi
             .metricsSystem(noOpMetricsSystem)
             .privacyParameters(PrivacyParameters.DEFAULT)
             .clock(TestClock.fixed())
-            .transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
+            .transactionPoolConfiguration(TransactionPoolConfiguration.DEFAULT)
             .storageProvider(createKeyValueStorageProvider(dataDirAhead, dbAhead))
             .gasLimitCalculator(GasLimitCalculator.constant())
             .build();
@@ -248,7 +248,7 @@ private void syncFromGenesis(final SyncMode mode, final GenesisConfigFile genesi
               .metricsSystem(noOpMetricsSystem)
               .privacyParameters(PrivacyParameters.DEFAULT)
               .clock(TestClock.fixed())
-              .transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
+              .transactionPoolConfiguration(TransactionPoolConfiguration.DEFAULT)
               .gasLimitCalculator(GasLimitCalculator.constant())
               .build();
       final EnodeURL enode = runnerAhead.getLocalEnode().get();

diff --git a/besu/src/test/java/org/hyperledger/besu/chainexport/RlpBlockExporterTest.java b/besu/src/test/java/org/hyperledger/besu/chainexport/RlpBlockExporterTest.java
@@ -90,7 +90,7 @@ private static BesuController createController() throws IOException {
         .privacyParameters(PrivacyParameters.DEFAULT)
         .dataDirectory(dataDir)
         .clock(TestClock.fixed())
-        .transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
+        .transactionPoolConfiguration(TransactionPoolConfiguration.DEFAULT)
         .gasLimitCalculator(GasLimitCalculator.constant())
         .build();
   }

diff --git a/besu/src/test/java/org/hyperledger/besu/chainimport/JsonBlockImporterTest.java b/besu/src/test/java/org/hyperledger/besu/chainimport/JsonBlockImporterTest.java
@@ -427,7 +427,7 @@ protected BesuController createController(final GenesisConfigFile genesisConfigF
         .privacyParameters(PrivacyParameters.DEFAULT)
         .dataDirectory(dataDir)
         .clock(TestClock.fixed())
-        .transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
+        .transactionPoolConfiguration(TransactionPoolConfiguration.DEFAULT)
         .gasLimitCalculator(GasLimitCalculator.constant())
         .build();
   }

diff --git a/besu/src/test/java/org/hyperledger/besu/chainimport/RlpBlockImporterTest.java b/besu/src/test/java/org/hyperledger/besu/chainimport/RlpBlockImporterTest.java
@@ -70,7 +70,7 @@ public void blockImport() throws IOException {
             .privacyParameters(PrivacyParameters.DEFAULT)
             .dataDirectory(dataDir)
             .clock(TestClock.fixed())
-            .transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
+            .transactionPoolConfiguration(TransactionPoolConfiguration.DEFAULT)
             .gasLimitCalculator(GasLimitCalculator.constant())
             .build();
     final RlpBlockImporter.ImportResult result =
@@ -98,7 +98,7 @@ public void blockImportRejectsBadPow() throws IOException {
             .privacyParameters(PrivacyParameters.DEFAULT)
             .dataDirectory(dataDir)
             .clock(TestClock.fixed())
-            .transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
+            .transactionPoolConfiguration(TransactionPoolConfiguration.DEFAULT)
             .gasLimitCalculator(GasLimitCalculator.constant())
             .build();
 
@@ -126,7 +126,7 @@ public void blockImportCanSkipPow() throws IOException {
             .privacyParameters(PrivacyParameters.DEFAULT)
             .dataDirectory(dataDir)
             .clock(TestClock.fixed())
-            .transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
+            .transactionPoolConfiguration(TransactionPoolConfiguration.DEFAULT)
             .gasLimitCalculator(GasLimitCalculator.constant())
             .build();
 
@@ -166,7 +166,7 @@ public void ibftImport() throws IOException {
             .privacyParameters(PrivacyParameters.DEFAULT)
             .dataDirectory(dataDir)
             .clock(TestClock.fixed())
-            .transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
+            .transactionPoolConfiguration(TransactionPoolConfiguration.DEFAULT)
             .gasLimitCalculator(GasLimitCalculator.constant())
             .build();
     final RlpBlockImporter.ImportResult result =

diff --git a/besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java b/besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java
@@ -3411,6 +3411,18 @@ public void txMessageKeepAliveSecondsWithInvalidInputShouldFail() {
             "Invalid value for option '--Xincoming-tx-messages-keep-alive-seconds': 'acbd' is not an int");
   }
 
+  @Test
+  public void eth65TrxAnnouncedBufferingPeriodWithInvalidInputShouldFail() {
+    parseCommand("--Xeth65-tx-announced-buffering-period-milliseconds", "acbd");
+
+    Mockito.verifyZeroInteractions(mockRunnerBuilder);
+
+    assertThat(commandOutput.toString()).isEmpty();
+    assertThat(commandErrorOutput.toString())
+        .contains(
+            "Invalid value for option '--Xeth65-tx-announced-buffering-period-milliseconds': 'acbd' is not a long");
+  }
+
   @Test
   public void tomlThatHasInvalidOptions() throws IOException {
     final URL configFile = this.getClass().getResource("/complete_config.toml");

diff --git a/besu/src/test/java/org/hyperledger/besu/cli/options/TransactionPoolOptionsTest.java b/besu/src/test/java/org/hyperledger/besu/cli/options/TransactionPoolOptionsTest.java
@@ -17,12 +17,16 @@
 import static org.assertj.core.api.Assertions.assertThat;
 
 import org.hyperledger.besu.cli.options.unstable.TransactionPoolOptions;
+import org.hyperledger.besu.ethereum.eth.transactions.ImmutableTransactionPoolConfiguration;
 import org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolConfiguration;
 
+import java.time.Duration;
+
 import org.junit.Test;
 
 public class TransactionPoolOptionsTest
-    extends AbstractCLIOptionsTest<TransactionPoolConfiguration.Builder, TransactionPoolOptions> {
+    extends AbstractCLIOptionsTest<
+        ImmutableTransactionPoolConfiguration.Builder, TransactionPoolOptions> {
 
   @Test
   public void txMessageKeepAliveSeconds() {
@@ -40,20 +44,44 @@ public void txMessageKeepAliveSeconds() {
     assertThat(commandErrorOutput.toString()).isEmpty();
   }
 
+  @Test
+  public void eth65TrxAnnouncedBufferingPeriod() {
+    final long eth65TrxAnnouncedBufferingPeriod = 999;
+    final TestBesuCommand cmd =
+        parseCommand(
+            "--Xeth65-tx-announced-buffering-period-milliseconds",
+            String.valueOf(eth65TrxAnnouncedBufferingPeriod));
+
+    final TransactionPoolOptions options = getOptionsFromBesuCommand(cmd);
+    final TransactionPoolConfiguration config = options.toDomainObject().build();
+    assertThat(config.getEth65TrxAnnouncedBufferingPeriod())
+        .hasMillis(eth65TrxAnnouncedBufferingPeriod);
+
+    assertThat(commandOutput.toString()).isEmpty();
+    assertThat(commandErrorOutput.toString()).isEmpty();
+  }
+
   @Override
-  TransactionPoolConfiguration.Builder createDefaultDomainObject() {
-    return TransactionPoolConfiguration.builder();
+  ImmutableTransactionPoolConfiguration.Builder createDefaultDomainObject() {
+    final ImmutableTransactionPoolConfiguration defaultValue =
+        ImmutableTransactionPoolConfiguration.builder().build();
+    return ImmutableTransactionPoolConfiguration.builder()
+        .txMessageKeepAliveSeconds(defaultValue.getTxMessageKeepAliveSeconds())
+        .eth65TrxAnnouncedBufferingPeriod(defaultValue.getEth65TrxAnnouncedBufferingPeriod());
   }
 
   @Override
-  TransactionPoolConfiguration.Builder createCustomizedDomainObject() {
-    return TransactionPoolConfiguration.builder()
-        .txMessageKeepAliveSeconds(TransactionPoolConfiguration.DEFAULT_TX_MSG_KEEP_ALIVE + 1);
+  ImmutableTransactionPoolConfiguration.Builder createCustomizedDomainObject() {
+    return ImmutableTransactionPoolConfiguration.builder()
+        .txMessageKeepAliveSeconds(TransactionPoolConfiguration.DEFAULT_TX_MSG_KEEP_ALIVE + 1)
+        .eth65TrxAnnouncedBufferingPeriod(
+            TransactionPoolConfiguration.ETH65_TRX_ANNOUNCED_BUFFERING_PERIOD.plus(
+                Duration.ofMillis(100)));
   }
 
   @Override
   TransactionPoolOptions optionsFromDomainObject(
-      final TransactionPoolConfiguration.Builder domainObject) {
+      final ImmutableTransactionPoolConfiguration.Builder domainObject) {
     return TransactionPoolOptions.fromConfig(domainObject.build());
   }
 

diff --git a/besu/src/test/java/org/hyperledger/besu/services/BesuEventsImplTest.java b/besu/src/test/java/org/hyperledger/besu/services/BesuEventsImplTest.java
@@ -40,6 +40,7 @@
 import org.hyperledger.besu.ethereum.eth.manager.EthScheduler;
 import org.hyperledger.besu.ethereum.eth.sync.BlockBroadcaster;
 import org.hyperledger.besu.ethereum.eth.sync.state.SyncState;
+import org.hyperledger.besu.ethereum.eth.transactions.ImmutableTransactionPoolConfiguration;
 import org.hyperledger.besu.ethereum.eth.transactions.TransactionPool;
 import org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolConfiguration;
 import org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolFactory;
@@ -129,7 +130,7 @@ public void setUp() {
     blockBroadcaster = new BlockBroadcaster(mockEthContext);
     syncState = new SyncState(blockchain, mockEthPeers);
     TransactionPoolConfiguration txPoolConfig =
-        TransactionPoolConfiguration.builder().txPoolMaxSize(1).build();
+        ImmutableTransactionPoolConfiguration.builder().txPoolMaxSize(1).build();
 
     transactionPool =
         TransactionPoolFactory.createTransactionPool(