Update errorprone (#401)

* Upgrade errorprone

* Upgrade errorprone from 2.3.1 to 2.3.2
* Upgrade Jenkinsfile so that CI will use Java 11
* Suppress these new rules:
  * EqualsGetClass
  * ImmutableEnumChecker
  * UnnecessaryParentheses
* Change code to conform to these new rules:
  * BadImport
  * BadInstanceof
  * InconsistentHashCode
  * LockNotBeforeTry
  * MathAbsoluteRandom
  * ModifiedButNotUsed
  * UndefinedEquals

Signed-off-by: Adrian Sutton <adrian.sutton@consensys.net>

Jenkinsfile

@@ -24,7 +24,7 @@ try {
             node {
                 checkout scm
                 docker.image('docker:18.06.0-ce-dind').withRun('--privileged') { d ->
-                    docker.image('pegasyseng/pantheon-build:0.0.1').inside("--link ${d.id}:docker") {
+                    docker.image('pegasyseng/pantheon-build:0.0.3').inside("--link ${d.id}:docker") {
                         try {
                             stage('Compile') {
                                 sh './gradlew --no-daemon --parallel clean compileJava'
@@ -71,7 +71,7 @@ try {
             node {
                 checkout scm
                 docker.image('docker:18.06.0-ce-dind').withRun('--privileged') { d ->
-                    docker.image('pegasyseng/pantheon-build:0.0.1').inside("--link ${d.id}:docker") {
+                    docker.image('pegasyseng/pantheon-build:0.0.3').inside("--link ${d.id}:docker") {
                         try {
                             stage('Docker quickstart Tests') {
                                 sh 'DOCKER_HOST=tcp://docker:2375 ./gradlew --no-daemon --parallel clean dockerQuickstartTest'

build.gradle

@@ -154,9 +154,18 @@ allprojects {
 
     options.errorprone {
       excludedPaths '.*/(generated/*.*|.*ReferenceTest_.*)'
+
+      // Our equals need to be symmetric, this checker doesn't respect that.
+      check('EqualsGetClass', CheckSeverity.OFF)
+      // We like to use futures with no return values.
       check('FutureReturnValueIgnored', CheckSeverity.OFF)
-      check('InsecureCryptoUsage', CheckSeverity.WARN)
+      // We use the JSR-305 annotations instead of the Google annotations.
+      check('ImmutableEnumChecker', CheckSeverity.OFF)
+      // This is a style check instead of an error-prone pattern.
+      check('UnnecessaryParentheses', CheckSeverity.OFF)
+
       check('FieldCanBeFinal', CheckSeverity.WARN)
+      check('InsecureCryptoUsage', CheckSeverity.WARN)
       check('WildcardImport', CheckSeverity.WARN)
     }
 

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/PreparedCertificate.java

@@ -15,6 +15,7 @@
 import tech.pegasys.pantheon.ethereum.rlp.RLPInput;
 import tech.pegasys.pantheon.ethereum.rlp.RLPOutput;
 
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Objects;
 import java.util.StringJoiner;
@@ -67,7 +68,7 @@ public boolean equals(final Object o) {
     }
     final PreparedCertificate that = (PreparedCertificate) o;
     return Objects.equals(proposalPayload, that.proposalPayload)
-        && Objects.equals(preparePayloads, that.preparePayloads);
+        && Objects.equals(new ArrayList<>(preparePayloads), new ArrayList<>(that.preparePayloads));
   }
 
   @Override

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/RoundChangeCertificate.java

@@ -15,6 +15,7 @@
 import tech.pegasys.pantheon.ethereum.rlp.RLPInput;
 import tech.pegasys.pantheon.ethereum.rlp.RLPOutput;
 
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.Objects;
@@ -73,7 +74,8 @@ public boolean equals(final Object o) {
       return false;
     }
     final RoundChangeCertificate that = (RoundChangeCertificate) o;
-    return Objects.equals(roundChangePayloads, that.roundChangePayloads);
+    return Objects.equals(
+        new ArrayList<>(roundChangePayloads), new ArrayList<>(that.roundChangePayloads));
   }
 
   @Override

consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/validation/NewRoundMessageValidatorTest.java

@@ -23,7 +23,6 @@
 import tech.pegasys.pantheon.consensus.ibft.blockcreation.ProposerSelector;
 import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.MessageFactory;
 import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.NewRoundPayload;
-import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.NewRoundPayload.Builder;
 import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.PreparedCertificate;
 import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.ProposalPayload;
 import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.RoundChangeCertificate;
@@ -65,7 +64,8 @@ public class NewRoundMessageValidatorTest {
 
   private final SignedData<NewRoundPayload> validMsg =
       createValidNewRoundMessageSignedBy(proposerKey);
-  private final NewRoundPayload.Builder msgBuilder = Builder.fromExisting(validMsg.getPayload());
+  private final NewRoundPayload.Builder msgBuilder =
+      NewRoundPayload.Builder.fromExisting(validMsg.getPayload());
 
   @Before
   public void setup() {

crypto/src/main/java/tech/pegasys/pantheon/crypto/altbn128/AbstractFqp.java

@@ -258,7 +258,9 @@ public boolean equals(final Object obj) {
       return false;
     }
 
-    final AbstractFqp other = (AbstractFqp) obj;
+    final AbstractFqp<?> other = (AbstractFqp<?>) obj;
+    if (degree != other.degree) return false;
+    if (!Arrays.equals(modulusCoefficients, other.modulusCoefficients)) return false;
     return Arrays.equals(coefficients, other.coefficients);
   }
 

errorprone-checks/build.gradle

@@ -43,13 +43,11 @@ dependencies {
 }
 
 test {
-  if (!JavaVersion.current().isJava8()) {
+  if (JavaVersion.current().isJava8()) {
     enabled = false
     logger.info("Disabling {} because errorprone tests always fail in Java {}",
       project.name, JavaVersion.current().majorVersion)
   }
 
-  jvmArgs "-Xbootclasspath/p:${configurations.epJavac.asPath}"
-
   testLogging { showStandardStreams = true }
 }

ethereum/core/src/integration-test/java/tech/pegasys/pantheon/ethereum/vm/EntriesFromIntegrationTest.java

@@ -32,6 +32,7 @@
 public class EntriesFromIntegrationTest {
 
   @Test
+  @SuppressWarnings("MathAbsoluteRandom")
   public void shouldCollectStateEntries() {
     final MutableWorldState worldState = createInMemoryWorldStateArchive().getMutable();
     final WorldUpdater updater = worldState.updater();

ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/mainnet/BlockHeaderValidatorTest.java

@@ -26,7 +26,6 @@
 import tech.pegasys.pantheon.ethereum.chain.MutableBlockchain;
 import tech.pegasys.pantheon.ethereum.core.BlockDataGenerator;
 import tech.pegasys.pantheon.ethereum.core.BlockHeader;
-import tech.pegasys.pantheon.ethereum.mainnet.BlockHeaderValidator.Builder;
 
 import java.util.Optional;
 
@@ -235,7 +234,12 @@ public void shouldRunRulesInOrderOfAdditionDuringFullValidation() {
     final AttachedBlockHeaderValidationRule<Void> rule4 = createPassingAttachedRule();
 
     final BlockHeaderValidator<Void> validator =
-        new Builder<Void>().addRule(rule1).addRule(rule2).addRule(rule3).addRule(rule4).build();
+        new BlockHeaderValidator.Builder<Void>()
+            .addRule(rule1)
+            .addRule(rule2)
+            .addRule(rule3)
+            .addRule(rule4)
+            .build();
 
     final BlockHeader header = generator.header();
     final BlockHeader parent = generator.header();

ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/JsonRpcConfiguration.java

@@ -18,6 +18,7 @@
 
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Objects;
+import com.google.common.collect.Lists;
 
 public class JsonRpcConfiguration {
   private static final String DEFAULT_JSON_RPC_HOST = "127.0.0.1";
@@ -120,9 +121,11 @@ public boolean equals(final Object o) {
     return enabled == that.enabled
         && port == that.port
         && Objects.equal(host, that.host)
-        && Objects.equal(corsAllowedDomains, that.corsAllowedDomains)
-        && Objects.equal(hostsWhitelist, that.hostsWhitelist)
-        && Objects.equal(rpcApis, that.rpcApis);
+        && Objects.equal(
+            Lists.newArrayList(corsAllowedDomains), Lists.newArrayList(that.corsAllowedDomains))
+        && Objects.equal(
+            Lists.newArrayList(hostsWhitelist), Lists.newArrayList(that.hostsWhitelist))
+        && Objects.equal(Lists.newArrayList(rpcApis), Lists.newArrayList(that.rpcApis));
   }
 
   @Override

ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/websocket/WebSocketConfiguration.java

@@ -21,6 +21,7 @@
 
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Objects;
+import com.google.common.collect.Lists;
 
 public class WebSocketConfiguration {
   public static final String DEFAULT_WEBSOCKET_HOST = "127.0.0.1";
@@ -106,7 +107,7 @@ public boolean equals(final Object o) {
     return enabled == that.enabled
         && port == that.port
         && Objects.equal(host, that.host)
-        && Objects.equal(rpcApis, that.rpcApis);
+        && Objects.equal(Lists.newArrayList(rpcApis), Lists.newArrayList(that.rpcApis));
   }
 
   @Override

ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/JsonRpcHttpServiceCorsTest.java

@@ -22,7 +22,6 @@
 import io.vertx.core.Vertx;
 import okhttp3.OkHttpClient;
 import okhttp3.Request;
-import okhttp3.Request.Builder;
 import okhttp3.Response;
 import org.junit.After;
 import org.junit.Before;
@@ -53,7 +52,10 @@ public void requestWithNonAcceptedOriginShouldFail() throws Exception {
     jsonRpcHttpService = createJsonRpcHttpServiceWithAllowedDomains("http://foo.io");
 
     final Request request =
-        new Builder().url(jsonRpcHttpService.url()).header("Origin", "http://bar.me").build();
+        new Request.Builder()
+            .url(jsonRpcHttpService.url())
+            .header("Origin", "http://bar.me")
+            .build();
 
     try (final Response response = client.newCall(request).execute()) {
       assertThat(response.isSuccessful()).isFalse();
@@ -65,7 +67,10 @@ public void requestWithAcceptedOriginShouldSucceed() throws Exception {
     jsonRpcHttpService = createJsonRpcHttpServiceWithAllowedDomains("http://foo.io");
 
     final Request request =
-        new Builder().url(jsonRpcHttpService.url()).header("Origin", "http://foo.io").build();
+        new Request.Builder()
+            .url(jsonRpcHttpService.url())
+            .header("Origin", "http://foo.io")
+            .build();
 
     try (final Response response = client.newCall(request).execute()) {
       assertThat(response.isSuccessful()).isTrue();
@@ -78,7 +83,10 @@ public void requestWithOneOfMultipleAcceptedOriginsShouldSucceed() throws Except
         createJsonRpcHttpServiceWithAllowedDomains("http://foo.io", "http://bar.me");
 
     final Request request =
-        new Builder().url(jsonRpcHttpService.url()).header("Origin", "http://bar.me").build();
+        new Request.Builder()
+            .url(jsonRpcHttpService.url())
+            .header("Origin", "http://bar.me")
+            .build();
 
     try (final Response response = client.newCall(request).execute()) {
       assertThat(response.isSuccessful()).isTrue();
@@ -91,7 +99,10 @@ public void requestWithNoneOfMultipleAcceptedOriginsShouldFail() throws Exceptio
         createJsonRpcHttpServiceWithAllowedDomains("http://foo.io", "http://bar.me");
 
     final Request request =
-        new Builder().url(jsonRpcHttpService.url()).header("Origin", "http://hel.lo").build();
+        new Request.Builder()
+            .url(jsonRpcHttpService.url())
+            .header("Origin", "http://hel.lo")
+            .build();
 
     try (final Response response = client.newCall(request).execute()) {
       assertThat(response.isSuccessful()).isFalse();
@@ -102,7 +113,7 @@ public void requestWithNoneOfMultipleAcceptedOriginsShouldFail() throws Exceptio
   public void requestWithNoOriginShouldSucceedWhenNoCorsConfigSet() throws Exception {
     jsonRpcHttpService = createJsonRpcHttpServiceWithAllowedDomains();
 
-    final Request request = new Builder().url(jsonRpcHttpService.url()).build();
+    final Request request = new Request.Builder().url(jsonRpcHttpService.url()).build();
 
     try (final Response response = client.newCall(request).execute()) {
       assertThat(response.isSuccessful()).isTrue();
@@ -113,7 +124,7 @@ public void requestWithNoOriginShouldSucceedWhenNoCorsConfigSet() throws Excepti
   public void requestWithNoOriginShouldSucceedWhenCorsIsSet() throws Exception {
     jsonRpcHttpService = createJsonRpcHttpServiceWithAllowedDomains("http://foo.io");
 
-    final Request request = new Builder().url(jsonRpcHttpService.url()).build();
+    final Request request = new Request.Builder().url(jsonRpcHttpService.url()).build();
 
     try (final Response response = client.newCall(request).execute()) {
       assertThat(response.isSuccessful()).isTrue();
@@ -125,7 +136,10 @@ public void requestWithAnyOriginShouldNotSucceedWhenCorsIsEmpty() throws Excepti
     jsonRpcHttpService = createJsonRpcHttpServiceWithAllowedDomains("");
 
     final Request request =
-        new Builder().url(jsonRpcHttpService.url()).header("Origin", "http://bar.me").build();
+        new Request.Builder()
+            .url(jsonRpcHttpService.url())
+            .header("Origin", "http://bar.me")
+            .build();
 
     try (final Response response = client.newCall(request).execute()) {
       assertThat(response.isSuccessful()).isFalse();
@@ -137,7 +151,10 @@ public void requestWithAnyOriginShouldSucceedWhenCorsIsStart() throws Exception
     jsonRpcHttpService = createJsonRpcHttpServiceWithAllowedDomains("*");
 
     final Request request =
-        new Builder().url(jsonRpcHttpService.url()).header("Origin", "http://bar.me").build();
+        new Request.Builder()
+            .url(jsonRpcHttpService.url())
+            .header("Origin", "http://bar.me")
+            .build();
 
     try (final Response response = client.newCall(request).execute()) {
       assertThat(response.isSuccessful()).isTrue();
@@ -149,7 +166,7 @@ public void requestWithAccessControlRequestMethodShouldReturnAllowedHeaders() th
     jsonRpcHttpService = createJsonRpcHttpServiceWithAllowedDomains("http://foo.io");
 
     final Request request =
-        new Builder()
+        new Request.Builder()
             .url(jsonRpcHttpService.url())
             .method("OPTIONS", null)
             .header("Access-Control-Request-Method", "OPTIONS")

ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/queries/BlockchainQueriesTest.java

@@ -33,7 +33,7 @@
 import tech.pegasys.pantheon.ethereum.core.Wei;
 import tech.pegasys.pantheon.ethereum.core.WorldState;
 import tech.pegasys.pantheon.ethereum.db.WorldStateArchive;
-import tech.pegasys.pantheon.ethereum.jsonrpc.internal.filter.LogsQuery.Builder;
+import tech.pegasys.pantheon.ethereum.jsonrpc.internal.filter.LogsQuery;
 import tech.pegasys.pantheon.util.uint.UInt256;
 
 import java.util.ArrayList;
@@ -303,14 +303,10 @@ public void logsShouldBeFlaggedAsRemovedWhenBlockIsNotInCanonicalChain() {
     // create initial blockchain
     final BlockchainWithData data = setupBlockchain(3);
     final Block targetBlock = data.blockData.get(data.blockData.size() - 1).block;
-    final List<Block> blocks =
-        data.blockData.stream().map(b -> b.block).collect(Collectors.toList());
-    final List<List<TransactionReceipt>> blockReceipts =
-        blocks.stream().map(gen::receipts).collect(Collectors.toList());
 
     // check that logs have removed = false
     List<LogWithMetadata> logs =
-        data.blockchainQueries.matchingLogs(targetBlock.getHash(), new Builder().build());
+        data.blockchainQueries.matchingLogs(targetBlock.getHash(), new LogsQuery.Builder().build());
     assertThat(logs).isNotEmpty();
     assertThat(logs).allMatch(l -> !l.isRemoved());
 
@@ -326,17 +322,12 @@ public void logsShouldBeFlaggedAsRemovedWhenBlockIsNotInCanonicalChain() {
     final Block fork = gen.block(options);
     final List<TransactionReceipt> forkReceipts = gen.receipts(fork);
 
-    final List<Block> reorgedChain = new ArrayList<>(blocks.subList(0, forkBlock));
-    reorgedChain.add(fork);
-    final List<List<TransactionReceipt>> reorgedReceipts =
-        new ArrayList<>(blockReceipts.subList(0, forkBlock));
-    reorgedReceipts.add(forkReceipts);
-
     // Add fork
     data.blockchain.appendBlock(fork, forkReceipts);
 
     // check that logs have removed = true
-    logs = data.blockchainQueries.matchingLogs(targetBlock.getHash(), new Builder().build());
+    logs =
+        data.blockchainQueries.matchingLogs(targetBlock.getHash(), new LogsQuery.Builder().build());
     assertThat(logs).isNotEmpty();
     assertThat(logs).allMatch(LogWithMetadata::isRemoved);
   }
@@ -542,10 +533,7 @@ private BlockchainWithData setupBlockchain(
     final MutableBlockchain blockchain = createInMemoryBlockchain(blocks.get(0));
     blockData
         .subList(1, blockData.size())
-        .forEach(
-            b -> {
-              blockchain.appendBlock(b.block, b.receipts);
-            });
+        .forEach(b -> blockchain.appendBlock(b.block, b.receipts));
 
     return new BlockchainWithData(blockchain, blockData, worldStateArchive);
   }

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/Bucket.java

@@ -14,7 +14,6 @@
 
 import static java.lang.System.arraycopy;
 import static java.util.Arrays.asList;
-import static java.util.Arrays.copyOf;
 import static java.util.Collections.unmodifiableList;
 
 import tech.pegasys.pantheon.ethereum.p2p.discovery.DiscoveryPeer;
@@ -135,7 +134,7 @@ synchronized boolean evict(final PeerId peer) {
    * @return immutable view of the peer array
    */
   synchronized List<DiscoveryPeer> peers() {
-    return unmodifiableList(asList(copyOf(kBucket, tailIndex + 1)));
+    return unmodifiableList(asList(Arrays.copyOf(kBucket, tailIndex + 1)));
   }
 
   @Override

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/netty/CapabilityMultiplexer.java

@@ -30,7 +30,6 @@
 
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableRangeMap;
-import com.google.common.collect.ImmutableRangeMap.Builder;
 import com.google.common.collect.Range;
 
 public class CapabilityMultiplexer {
@@ -120,7 +119,7 @@ private ImmutableRangeMap<Integer, Capability> calculateAgreedCapabilities(
     caps.sort(CAPABILITY_COMPARATOR);
     caps.retainAll(b);
 
-    final Builder<Integer, Capability> builder = ImmutableRangeMap.builder();
+    final ImmutableRangeMap.Builder<Integer, Capability> builder = ImmutableRangeMap.builder();
     // Reserve some messages for WireProtocol
     int offset = WIRE_PROTOCOL_MESSAGE_SPACE;
     String prevProtocol = null;

ethereum/trie/src/test/java/tech/pegasys/pantheon/ethereum/trie/CompactEncodingTest.java

@@ -35,7 +35,8 @@ public void bytesToPath() {
   public void shouldRoundTripFromBytesToPathAndBack() {
     final Random random = new Random(282943948928429484L);
     for (int i = 0; i < 1000; i++) {
-      final Bytes32 bytes = Hash.keccak256(UInt256.of(Math.abs(random.nextInt())).getBytes());
+      final Bytes32 bytes =
+          Hash.keccak256(UInt256.of(random.nextInt(Integer.MAX_VALUE)).getBytes());
       final BytesValue path = CompactEncoding.bytesToPath(bytes);
       assertThat(CompactEncoding.pathToBytes(path)).isEqualTo(bytes);
     }