Validate that blockperiodseconds is set to a positive integer (#3186)

Implemented getPositiveInt function in JsonUtil to validate a JSON positive number value. Using this function, the blockperiodseconds should now be validated whenever retrieved from the genesis config, including transitions.

Signed-off-by: George Patterson <g-patt@outlook.com>

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Changelog
 
+## Next RC
+
+### Additions and Improvements
+- Genesis file parameter `blockperiodseconds` is validated as a positive integer on startup to prevent unexpected runtime behaviour [#3186](https://github.com/hyperledger/besu/pull/3186)
+
 ## 22.1.0-RC2
 
 ### 22.1.0-RC2 Breaking Changes
@@ -7,10 +12,10 @@
 
 ### Additions and Improvements
 - Re-order external services (e.g JsonRpcHttpService) to start before blocks start processing [#3118](https://github.com/hyperledger/besu/pull/3118)
-- Stream JSON RPC responses to avoid creating big JSON strings in memory [#3076] (https://github.com/hyperledger/besu/pull/3076)
+- Stream JSON RPC responses to avoid creating big JSON strings in memory [#3076](https://github.com/hyperledger/besu/pull/3076)
 
 ### Bug Fixes
-- Make 'to' field optional in eth_call method according to the spec [#3177] (https://github.com/hyperledger/besu/pull/3177)
+- Make 'to' field optional in eth_call method according to the spec [#3177](https://github.com/hyperledger/besu/pull/3177)
 - Update to log4j 2.17.1. Resolves potential vulnerability only exploitable when using custom log4j configurations that are writable by untrusted users.
 
 ## 21.10.6

config/build.gradle

@@ -29,6 +29,7 @@ jar {
 
 dependencies {
   implementation project(':datatypes')
+  implementation project(':util')
 
   implementation 'com.fasterxml.jackson.core:jackson-databind'
   implementation 'com.google.guava:guava'

config/src/main/java/org/hyperledger/besu/config/BftFork.java

@@ -48,7 +48,7 @@ public class BftFork {
   }
 
   public OptionalInt getBlockPeriodSeconds() {
-    return JsonUtil.getInt(forkConfigRoot, BLOCK_PERIOD_SECONDS_KEY);
+    return JsonUtil.getPositiveInt(forkConfigRoot, BLOCK_PERIOD_SECONDS_KEY);
   }
 
   public Optional<BigInteger> getBlockRewardWei() {

config/src/main/java/org/hyperledger/besu/config/CliqueConfigOptions.java

@@ -38,7 +38,8 @@ public class CliqueConfigOptions {
   }
 
   public int getBlockPeriodSeconds() {
-    return JsonUtil.getInt(cliqueConfigRoot, "blockperiodseconds", DEFAULT_BLOCK_PERIOD_SECONDS);
+    return JsonUtil.getPositiveInt(
+        cliqueConfigRoot, "blockperiodseconds", DEFAULT_BLOCK_PERIOD_SECONDS);
   }
 
   Map<String, Object> asMap() {

config/src/main/java/org/hyperledger/besu/config/IbftLegacyConfigOptions.java

@@ -40,7 +40,8 @@ public class IbftLegacyConfigOptions {
   }
 
   public int getBlockPeriodSeconds() {
-    return JsonUtil.getInt(ibftConfigRoot, "blockperiodseconds", DEFAULT_BLOCK_PERIOD_SECONDS);
+    return JsonUtil.getPositiveInt(
+        ibftConfigRoot, "blockperiodseconds", DEFAULT_BLOCK_PERIOD_SECONDS);
   }
 
   public int getRequestTimeoutSeconds() {

config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java

@@ -51,7 +51,8 @@ public class JsonBftConfigOptions implements BftConfigOptions {
 
   @Override
   public int getBlockPeriodSeconds() {
-    return JsonUtil.getInt(bftConfigRoot, "blockperiodseconds", DEFAULT_BLOCK_PERIOD_SECONDS);
+    return JsonUtil.getPositiveInt(
+        bftConfigRoot, "blockperiodseconds", DEFAULT_BLOCK_PERIOD_SECONDS);
   }
 
   @Override

config/src/main/java/org/hyperledger/besu/config/JsonUtil.java

@@ -14,6 +14,8 @@
  */
 package org.hyperledger.besu.config;
 
+import org.hyperledger.besu.util.number.PositiveNumber;
+
 import java.io.IOException;
 import java.util.Locale;
 import java.util.Map;
@@ -141,6 +143,27 @@ public class JsonUtil {
     return getInt(node, key).orElse(defaultValue);
   }
 
+  public static OptionalInt getPositiveInt(final ObjectNode node, final String key) {
+    return getValueAsString(node, key)
+        .map(v -> OptionalInt.of(parsePositiveInt(key, v)))
+        .orElse(OptionalInt.empty());
+  }
+
+  public static int getPositiveInt(
+      final ObjectNode node, final String key, final int defaultValue) {
+    final String value = getValueAsString(node, key, String.valueOf(defaultValue));
+    return parsePositiveInt(key, value);
+  }
+
+  private static int parsePositiveInt(final String key, final String value) {
+    try {
+      return PositiveNumber.fromString(value).getValue();
+    } catch (IllegalArgumentException e) {
+      throw new IllegalArgumentException(
+          "Invalid property value, " + key + " should be a positive integer: " + value);
+    }
+  }
+
   public static OptionalLong getLong(final ObjectNode json, final String key) {
     return getValue(json, key)
         .filter(jsonNode -> validateType(jsonNode, JsonNodeType.NUMBER))

config/src/test/java/org/hyperledger/besu/config/CliqueConfigOptionsTest.java

@@ -17,6 +17,7 @@ package org.hyperledger.besu.config;
 import static java.util.Collections.emptyMap;
 import static java.util.Collections.singletonMap;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import java.util.Map;
 
@@ -30,7 +31,7 @@ public class CliqueConfigOptionsTest {
 
   @Test
   public void shouldGetEpochLengthFromConfig() {
-    final CliqueConfigOptions config = fromConfigOptions(singletonMap("EpochLength", 10_000));
+    final CliqueConfigOptions config = fromConfigOptions(singletonMap("epochlength", 10_000));
     assertThat(config.getEpochLength()).isEqualTo(10_000);
   }
 
@@ -48,7 +49,7 @@ public class CliqueConfigOptionsTest {
 
   @Test
   public void shouldGetBlockPeriodFromConfig() {
-    final CliqueConfigOptions config = fromConfigOptions(singletonMap("BlockPeriodSeconds", 5));
+    final CliqueConfigOptions config = fromConfigOptions(singletonMap("blockperiodseconds", 5));
     assertThat(config.getBlockPeriodSeconds()).isEqualTo(5);
   }
 
@@ -64,6 +65,13 @@ public class CliqueConfigOptionsTest {
         .isEqualTo(EXPECTED_DEFAULT_BLOCK_PERIOD);
   }
 
+  @Test
+  public void shouldThrowOnNonPositiveBlockPeriod() {
+    final CliqueConfigOptions config = fromConfigOptions(singletonMap("blockperiodseconds", -1));
+    assertThatThrownBy(() -> config.getBlockPeriodSeconds())
+        .isInstanceOf(IllegalArgumentException.class);
+  }
+
   private CliqueConfigOptions fromConfigOptions(final Map<String, Object> cliqueConfigOptions) {
     final ObjectNode rootNode = JsonUtil.createEmptyObjectNode();
     final ObjectNode configNode = JsonUtil.createEmptyObjectNode();

config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java

@@ -17,6 +17,7 @@ package org.hyperledger.besu.config;
 import static java.util.Collections.emptyMap;
 import static java.util.Collections.singletonMap;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import java.util.Map;
 
@@ -36,7 +37,7 @@ public class JsonBftConfigOptionsTest {
 
   @Test
   public void shouldGetEpochLengthFromConfig() {
-    final BftConfigOptions config = fromConfigOptions(singletonMap("EpochLength", 10_000));
+    final BftConfigOptions config = fromConfigOptions(singletonMap("epochlength", 10_000));
     assertThat(config.getEpochLength()).isEqualTo(10_000);
   }
 
@@ -54,7 +55,7 @@ public class JsonBftConfigOptionsTest {
 
   @Test
   public void shouldGetBlockPeriodFromConfig() {
-    final BftConfigOptions config = fromConfigOptions(singletonMap("BlockPeriodSeconds", 5));
+    final BftConfigOptions config = fromConfigOptions(singletonMap("blockperiodseconds", 5));
     assertThat(config.getBlockPeriodSeconds()).isEqualTo(5);
   }
 
@@ -70,9 +71,16 @@ public class JsonBftConfigOptionsTest {
         .isEqualTo(EXPECTED_DEFAULT_BLOCK_PERIOD);
   }
 
+  @Test
+  public void shouldThrowOnNonPositiveBlockPeriod() {
+    final BftConfigOptions config = fromConfigOptions(singletonMap("blockperiodseconds", -1));
+    assertThatThrownBy(() -> config.getBlockPeriodSeconds())
+        .isInstanceOf(IllegalArgumentException.class);
+  }
+
   @Test
   public void shouldGetRequestTimeoutFromConfig() {
-    final BftConfigOptions config = fromConfigOptions(singletonMap("RequestTimeoutSeconds", 5));
+    final BftConfigOptions config = fromConfigOptions(singletonMap("requesttimeoutseconds", 5));
     assertThat(config.getRequestTimeoutSeconds()).isEqualTo(5);
   }
 
@@ -90,7 +98,7 @@ public class JsonBftConfigOptionsTest {
 
   @Test
   public void shouldGetGossipedHistoryLimitFromConfig() {
-    final BftConfigOptions config = fromConfigOptions(singletonMap("GossipedHistoryLimit", 100));
+    final BftConfigOptions config = fromConfigOptions(singletonMap("gossipedhistorylimit", 100));
     assertThat(config.getGossipedHistoryLimit()).isEqualTo(100);
   }
 
@@ -108,7 +116,7 @@ public class JsonBftConfigOptionsTest {
 
   @Test
   public void shouldGetMessageQueueLimitFromConfig() {
-    final BftConfigOptions config = fromConfigOptions(singletonMap("MessageQueueLimit", 100));
+    final BftConfigOptions config = fromConfigOptions(singletonMap("messagequeuelimit", 100));
     assertThat(config.getMessageQueueLimit()).isEqualTo(100);
   }
 
@@ -126,7 +134,7 @@ public class JsonBftConfigOptionsTest {
 
   @Test
   public void shouldGetDuplicateMessageLimitFromConfig() {
-    final BftConfigOptions config = fromConfigOptions(singletonMap("DuplicateMessageLimit", 50));
+    final BftConfigOptions config = fromConfigOptions(singletonMap("duplicatemessagelimit", 50));
     assertThat(config.getDuplicateMessageLimit()).isEqualTo(50);
   }
 
@@ -145,7 +153,7 @@ public class JsonBftConfigOptionsTest {
 
   @Test
   public void shouldGetFutureMessagesLimitFromConfig() {
-    final BftConfigOptions config = fromConfigOptions(singletonMap("FutureMessagesLimit", 50));
+    final BftConfigOptions config = fromConfigOptions(singletonMap("futuremessageslimit", 50));
     assertThat(config.getFutureMessagesLimit()).isEqualTo(50);
   }
 
@@ -164,7 +172,7 @@ public class JsonBftConfigOptionsTest {
   @Test
   public void shouldGetFutureMessagesMaxDistanceFromConfig() {
     final BftConfigOptions config =
-        fromConfigOptions(singletonMap("FutureMessagesMaxDistance", 50));
+        fromConfigOptions(singletonMap("futuremessagesmaxdistance", 50));
     assertThat(config.getFutureMessagesMaxDistance()).isEqualTo(50);
   }
 

config/src/test/java/org/hyperledger/besu/config/JsonUtilTest.java

@@ -514,6 +514,101 @@ public class JsonUtilTest {
         .hasMessageContaining("Expected boolean value but got number");
   }
 
+  @Test
+  public void getPositiveInt_validValue() {
+    final ObjectNode node = mapper.createObjectNode();
+    final int validValue = 2;
+    node.put("test", validValue);
+    final OptionalInt result = JsonUtil.getPositiveInt(node, "test");
+    assertThat(result).hasValue(validValue);
+  }
+
+  @Test
+  public void getPositiveInt_nonExistentKey() {
+    final ObjectNode node = mapper.createObjectNode();
+    final OptionalInt result = JsonUtil.getPositiveInt(node, "test");
+    assertThat(result).isEmpty();
+  }
+
+  @Test
+  public void getPositiveInt_decimalValue() {
+    final ObjectNode node = mapper.createObjectNode();
+    final float decimalValue = Float.MAX_VALUE;
+    node.put("test", decimalValue);
+    assertThatThrownBy(() -> JsonUtil.getPositiveInt(node, "test"))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Invalid property value, test should be a positive integer: " + decimalValue);
+  }
+
+  @Test
+  public void getPositiveInt_nonPositiveValue() {
+    final ObjectNode node = mapper.createObjectNode();
+    final int nonPositiveValue = 0;
+    node.put("test", nonPositiveValue);
+    assertThatThrownBy(() -> JsonUtil.getPositiveInt(node, "test"))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage(
+            "Invalid property value, test should be a positive integer: " + nonPositiveValue);
+  }
+
+  @Test
+  public void getPositiveInt_negativeValue() {
+    final ObjectNode node = mapper.createObjectNode();
+    final int negativeValue = Integer.MIN_VALUE;
+    node.put("test", negativeValue);
+    assertThatThrownBy(() -> JsonUtil.getPositiveInt(node, "test"))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Invalid property value, test should be a positive integer: " + negativeValue);
+  }
+
+  @Test
+  public void getPositiveInt_validValue_withDefault() {
+    final ObjectNode node = mapper.createObjectNode();
+    final int validValue = 2;
+    node.put("test", validValue);
+    final int result = JsonUtil.getPositiveInt(node, "test", 1);
+    assertThat(result).isEqualTo(validValue);
+  }
+
+  @Test
+  public void getPositiveInt_nonExistentKey_withDefault() {
+    final ObjectNode node = mapper.createObjectNode();
+    final int defaultValue = 1;
+    final int result = JsonUtil.getPositiveInt(node, "test", defaultValue);
+    assertThat(result).isEqualTo(defaultValue);
+  }
+
+  @Test
+  public void getPositiveInt_decimalValue_withDefault() {
+    final ObjectNode node = mapper.createObjectNode();
+    final float decimalValue = Float.MAX_VALUE;
+    node.put("test", decimalValue);
+    assertThatThrownBy(() -> JsonUtil.getPositiveInt(node, "test", 1))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Invalid property value, test should be a positive integer: " + decimalValue);
+  }
+
+  @Test
+  public void getPositiveInt_nonPositiveValue_withDefault() {
+    final ObjectNode node = mapper.createObjectNode();
+    final int nonPositiveValue = 0;
+    node.put("test", nonPositiveValue);
+    assertThatThrownBy(() -> JsonUtil.getPositiveInt(node, "test", 1))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage(
+            "Invalid property value, test should be a positive integer: " + nonPositiveValue);
+  }
+
+  @Test
+  public void getPositiveInt_negativeValue_withDefault() {
+    final ObjectNode node = mapper.createObjectNode();
+    final int negativeValue = Integer.MIN_VALUE;
+    node.put("test", negativeValue);
+    assertThatThrownBy(() -> JsonUtil.getPositiveInt(node, "test", 1))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Invalid property value, test should be a positive integer: " + negativeValue);
+  }
+
   @Test
   public void objectNodeFromMap() {
     final Map<String, Object> map = new TreeMap<>();

util/src/test/java/org/hyperledger/besu/util/number/PositiveNumberTest.java

@@ -0,0 +1,62 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.junit.Test;
+
+public class PositiveNumberTest {
+
+  @Test
+  public void shouldBuildPositiveNumberFromString() {
+    final String positiveNumberString = "1";
+    final int positiveNumberInt = 1;
+    PositiveNumber positiveNumber = PositiveNumber.fromString(positiveNumberString);
+    assertThat(positiveNumber).isNotNull();
+    assertThat(positiveNumber.getValue()).isEqualTo(positiveNumberInt);
+  }
+
+  @Test
+  public void shouldThrowOnDecimalValueFromString() {
+    assertThatThrownBy(() -> PositiveNumber.fromString("1.1"))
+        .isInstanceOf(IllegalArgumentException.class);
+  }
+
+  @Test
+  public void shouldThrowOnNonPositiveValueFromString() {
+    assertThatThrownBy(() -> PositiveNumber.fromString("0"))
+        .isInstanceOf(IllegalArgumentException.class);
+  }
+
+  @Test
+  public void shouldThrowOnNegativeValueFromString() {
+    assertThatThrownBy(() -> PositiveNumber.fromString("-1"))
+        .isInstanceOf(IllegalArgumentException.class);
+  }
+
+  @Test
+  public void shouldThrowOnEmptyValueFromString() {
+    assertThatThrownBy(() -> PositiveNumber.fromString(""))
+        .isInstanceOf(IllegalArgumentException.class);
+  }
+
+  @Test
+  public void shouldThrowOnNullValueFromString() {
+    assertThatThrownBy(() -> PositiveNumber.fromString(null))
+        .isInstanceOf(IllegalArgumentException.class);
+  }
+}