Refactor Besu custom error prone dependency #6692

usmansaleem · 2024-03-07T06:07:52Z

PR description

Move Besu custom error-prone checks into its own repository. This allows to move to a newer version of Google errorprone checks as well while cleaning up build.gradle file.

Key changes:

String toLowerCase and toUpperCase to use Locale.ROOT as argument
Use interface such as List,Map or NavigatableMap instead of concrete class where appropriate.
Simplify StringBuilder to plain String
Supress warnings where appropriate.

Thanks for sending a pull request! Have you done the following?

Checked out our contribution guidelines?
Considered documentation and added the doc-change-required label to this PR if updates are required.
Considered the changelog and included an update if required.
For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Most advanced CI tests are deferred until PR approval, but you could:

locally run all unit tests via: ./gradlew build
locally run all acceptance tests via: ./gradlew acceptanceTest
locally run all integration tests via: ./gradlew integrationTest
locally run all reference tests via: ./gradlew ethereum:referenceTests:referenceTests

Fixed Issue(s)

Signed-off-by: Usman Saleem <usman@usmans.info>

Signed-off-by: Usman Saleem <usman@usmans.info> # Conflicts: # ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/request/StorageRangeDataRequest.java

Signed-off-by: Usman Saleem <usman@usmans.info>

Signed-off-by: Usman Saleem <usman@usmans.info> # Conflicts: # ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/BadBlockCause.java # ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/BadBlockReason.java

Signed-off-by: Usman Saleem <usman@usmans.info>

Signed-off-by: Usman Saleem <usman@usmans.info> # Conflicts: # ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/AbstractGasLimitSpecification.java

Signed-off-by: Usman Saleem <usman@usmans.info>

macfarla

couple of comments

macfarla · 2024-03-25T04:40:24Z

gradle/verification-metadata.xml

are all these dependencies required? I thought we'd end up with fewer dependencies by moving error-prone out?

@macfarla This is gradle verification metadata. This is automagically updated by running command ./gradlew --write-verification-metadata sha256 help. This file simply records the sha256 signaturs of various jars that gradle resolves.

I think its a good idea to re-generate the whole file (by deleting it first and rerunning above command), otherwise it will keep updating it rather than removing older entries.

macfarla · 2024-03-25T04:42:14Z

build.gradle

@@ -226,8 +229,9 @@ allprojects {
    ]

    options.errorprone {
-      excludedPaths = '.*/(generated/*.*|.*ReferenceTest_.*|build/.*/annotation-output/.*)'
-
+      //excludedPaths = '.*/(generated/*.*|.*ReferenceTest_.*|build/.*/annotation-output/.*)'


what's the consequence of this change - do we end up flagging issues within ref tests?

@macfarla Should have removed the commented line. I think the option disableWarningsInGeneratedCode = true has started ignoring quite a bit of files. Also, the ReferenceTest_ tests are generated in the generated sub-folder. In summary, we are still good by updating the excludedPath to only ignore generated subfolders.

Signed-off-by: Usman Saleem <usman@usmans.info>

jframe · 2024-03-26T03:59:16Z

.../api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/websocket/WebSocketService.java

-                            allowlistEntry.toLowerCase().equals(header.toLowerCase())))
+                            allowlistEntry
+                                .toLowerCase(Locale.ROOT)
+                                .equals(header.toLowerCase(Locale.ROOT))))


Better to replace it with equalsIgnoreCase

jframe · 2024-03-26T05:37:44Z

...p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryTimestampsTest.java

@@ -41,25 +40,25 @@ public void lastSeenAndFirstDiscoveredTimestampsUpdatedOnMessage() {
    final Packet pong = helper.createPongPacket(agent, Hash.hash(agentPing.getHash()));
    helper.sendMessageBetweenAgents(testAgent, agent, pong);

-    final AtomicLong lastSeen = new AtomicLong();
-    final AtomicLong firstDiscovered = new AtomicLong();
+    long lastSeen;


Can't see why an AtomicLong was ever needed, but it's normally a very deliberate choice. Is there some concurrency happening in this test?

@jframe I checked the git history. It used to have code similar to:

await() .atMost(1, TimeUnit.SECONDS) .untilAsserted( () -> { assertThat(controller.getPeers()).hasSize(1);

which since has been removed. Now there is no need to have AtomicLong anymore (and errorprone reported it as UnnecessaryAsync warning).

jframe · 2024-03-26T05:43:00Z

.../hyperledger/besu/ethereum/eth/transactions/layered/AbstractSequentialTransactionsLayer.java

@@ -141,6 +141,7 @@ protected boolean hasExpectedNonce(
  }

  @Override
+  @SuppressWarnings("NonApiType")
  protected void internalConsistencyCheck(
      final Map<Address, TreeMap<Long, PendingTransaction>> prevLayerTxsBySender) {


I think the TreeMap can be changed to a NavigableMap. I think the method we use on the Map is computeIfPresent which is also present on the NavigableMap. I changed this locally and for types that inherit this method and seemed to compile for me.

jframe · 2024-03-26T05:44:43Z

...n/java/org/hyperledger/besu/ethereum/eth/transactions/layered/AbstractTransactionsLayer.java

@@ -568,6 +568,7 @@ public String logSender(final Address sender) {

  protected abstract String internalLogStats();

+  @SuppressWarnings("NonApiType")
  boolean consistencyCheck(
      final Map<Address, TreeMap<Long, PendingTransaction>> prevLayerTxsBySender) {


Can this also be changed to use a NavigableMap?

jframe · 2024-03-26T05:46:19Z

...ger/besu/ethereum/eth/sync/snapsync/request/heal/AccountFlatDatabaseHealingRangeRequest.java

@@ -130,6 +130,7 @@ public boolean isResponseReceived() {
    return true;
  }

+  @SuppressWarnings("NonApiType")
  public void addLocalData(
      final WorldStateProofProvider worldStateProofProvider,
      final TreeMap<Bytes32, Bytes> accounts,


Can this be a NavigableMap?

jframe · 2024-03-26T05:46:30Z

...in/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/request/StorageRangeDataRequest.java

@@ -203,6 +204,7 @@ public Bytes32 getStorageRoot() {
    return storageRoot;
  }

+  @SuppressWarnings("NonApiType")
  public TreeMap<Bytes32, Bytes> getSlots() {


NavigableMap?

jframe · 2024-03-26T05:47:48Z

...in/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/request/StorageRangeDataRequest.java

@@ -126,6 +126,7 @@ protected int doPersist(
    return nbNodesSaved.get();
  }

+  @SuppressWarnings("NonApiType")
  public void addResponse(
      final SnapWorldDownloadState downloadState,


Is is the TreeMap is doesn't like here?

jframe · 2024-03-26T05:49:20Z

build.gradle

@@ -129,12 +129,12 @@ allprojects {
  }

  task sourcesJar(type: Jar, dependsOn: classes) {
-    classifier = 'sources'
+    archiveClassifier = 'sources'


Why did this need to change?

These were warnings in build.gradle file that will be an issue in future release of gradle. Although they should not be technically part of this PR, I fixed them originally while researching this problem and were using debug flags to the build.

Signed-off-by: Usman Saleem <usman@usmans.info>

Move Besu custom error-prone checks into its own repository and use it as an external dependency. This allows to move to a newer version of Google errorprone checks as well while cleaning up build.gradle file. Key changes resulted due to this change: * String toLowerCase and toUpperCase to use Locale.ROOT as argument * Use interface such as List,Map or NavigatableMap instead of concrete class where appropriate. * Simplify StringBuilder to plain String * Suppress warnings where appropriate. ----- Signed-off-by: Usman Saleem <usman@usmans.info> Signed-off-by: amsmota <antonio.mota@citi.com>

Move Besu custom error-prone checks into its own repository and use it as an external dependency. This allows to move to a newer version of Google errorprone checks as well while cleaning up build.gradle file. Key changes resulted due to this change: * String toLowerCase and toUpperCase to use Locale.ROOT as argument * Use interface such as List,Map or NavigatableMap instead of concrete class where appropriate. * Simplify StringBuilder to plain String * Suppress warnings where appropriate. ----- Signed-off-by: Usman Saleem <usman@usmans.info>

usmansaleem added 8 commits March 7, 2024 16:04

Use external custom error prone dependency

06bc097

Signed-off-by: Usman Saleem <usman@usmans.info>

spotless fix

c59b33e

Signed-off-by: Usman Saleem <usman@usmans.info>

Merge remote-tracking branch 'upstream/main' into werror_issue

3ad6fb4

Signed-off-by: Usman Saleem <usman@usmans.info>

custom errorprone check fail test

c3be7b4

Signed-off-by: Usman Saleem <usman@usmans.info>

testing errorprone checks

613ac16

Signed-off-by: Usman Saleem <usman@usmans.info>

Using latest version of google errorprone

c8d2e0f

Signed-off-by: Usman Saleem <usman@usmans.info>

Merge remote-tracking branch 'upstream/main' into werror_issue

e33b131

Signed-off-by: Usman Saleem <usman@usmans.info> # Conflicts: # ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/request/StorageRangeDataRequest.java

errorprone reported fix

9e43c2a

Signed-off-by: Usman Saleem <usman@usmans.info>

usmansaleem mentioned this pull request Mar 12, 2024

Investigate CI incorrectly passing code with compilation errors #6685

Closed

usmansaleem added 8 commits March 13, 2024 09:05

Merge remote-tracking branch 'upstream/main' into werror_issue

f65fd38

Signed-off-by: Usman Saleem <usman@usmans.info>

Add suppression for ComparisonOutOfRange

28847c2

Signed-off-by: Usman Saleem <usman@usmans.info>

Resolve errorprone locally

06e8a21

Signed-off-by: Usman Saleem <usman@usmans.info>

Merge remote-tracking branch 'upstream/main' into werror_issue

1f18502

Signed-off-by: Usman Saleem <usman@usmans.info>

Merge remote-tracking branch 'upstream/main' into werror_issue

652a840

Signed-off-by: Usman Saleem <usman@usmans.info> # Conflicts: # ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/AbstractGasLimitSpecification.java

build: Update besu errorprone library version

d8ac80e

Signed-off-by: Usman Saleem <usman@usmans.info>

chore: Revert changes to StorageRangeDataRequest

8536cc0

Signed-off-by: Usman Saleem <usman@usmans.info>

usmansaleem marked this pull request as ready for review March 25, 2024 00:50

usmansaleem added 7 commits March 25, 2024 10:57

chore: Remove mavanLocal used in local testing

abc5e43

Signed-off-by: Usman Saleem <usman@usmans.info>

chore: Cleanup StringBuilder

9c9bee7

Signed-off-by: Usman Saleem <usman@usmans.info>

errorprone: Fix reported issues

01d72f3

Signed-off-by: Usman Saleem <usman@usmans.info>

chore: Fix supress warning typo

345e499

Signed-off-by: Usman Saleem <usman@usmans.info>

fix: Fix invalid assertion

b13262d

Signed-off-by: Usman Saleem <usman@usmans.info>

errorprone: Fix reported issues

456b6bd

Signed-off-by: Usman Saleem <usman@usmans.info>

chore: spotless apply

71a2fa7

Signed-off-by: Usman Saleem <usman@usmans.info>

usmansaleem changed the title ~~Use external custom error prone dependency~~ Refactor Besu custom error prone dependency Mar 25, 2024

fix: Remove custom error prone source

becbbb0

Signed-off-by: Usman Saleem <usman@usmans.info>

usmansaleem requested review from jflo and macfarla March 25, 2024 04:29

macfarla reviewed Mar 25, 2024

View reviewed changes

usmansaleem added 11 commits March 25, 2024 16:45

chore: Remove comment from build.gradle

988310c

Signed-off-by: Usman Saleem <usman@usmans.info>

Merge remote-tracking branch 'upstream/main' into werror_issue

a2c1ab2

Signed-off-by: Usman Saleem <usman@usmans.info>

chore: Update gradle verification metadata

1a28596

Signed-off-by: Usman Saleem <usman@usmans.info>

chore: Update gradle verification metadata

597a920

Signed-off-by: Usman Saleem <usman@usmans.info>

chore: Update gradle verification metadata

3afd788

Signed-off-by: Usman Saleem <usman@usmans.info>

chore: Update gradle verification metadata

e96c52c

Signed-off-by: Usman Saleem <usman@usmans.info>

chore: Update fixing gradle verification metadata

72ee8cb

Signed-off-by: Usman Saleem <usman@usmans.info>

build: Update build.gradle, removing temporary errorprone check

603c21d

Signed-off-by: Usman Saleem <usman@usmans.info>

build: verification metadata for spotless

095c112

Signed-off-by: Usman Saleem <usman@usmans.info>

build: verification metadata for spotless

cb8f07a

Signed-off-by: Usman Saleem <usman@usmans.info>

Merge remote-tracking branch 'upstream/main' into werror_issue

6ad2b70

Signed-off-by: Usman Saleem <usman@usmans.info>

usmansaleem requested a review from macfarla March 26, 2024 01:31

jframe reviewed Mar 26, 2024

View reviewed changes

macfarla approved these changes Mar 26, 2024

View reviewed changes

usmansaleem added 6 commits March 26, 2024 16:32

Merge remote-tracking branch 'upstream/main' into werror_issue

6b77b18

Signed-off-by: Usman Saleem <usman@usmans.info>

review: Simplify checkHostInAllowlist logic

13b9607

Signed-off-by: Usman Saleem <usman@usmans.info>

review: Change TreeMap to NavigatableMap

fa9053e

Signed-off-by: Usman Saleem <usman@usmans.info>

review: Change TreeMap to NavigatableMap

ecacd19

Signed-off-by: Usman Saleem <usman@usmans.info>

Merge remote-tracking branch 'upstream/main' into werror_issue

4287430

Signed-off-by: Usman Saleem <usman@usmans.info>

review: Change TreeMap to NavigableMap

0dd03d4

Signed-off-by: Usman Saleem <usman@usmans.info>

usmansaleem merged commit e954537 into hyperledger:main Mar 26, 2024
42 checks passed

usmansaleem deleted the werror_issue branch March 26, 2024 20:17

usmansaleem mentioned this pull request Apr 4, 2024

[build] - Extract errorprone-checks as a separate library #6690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Besu custom error prone dependency #6692

Refactor Besu custom error prone dependency #6692

usmansaleem commented Mar 7, 2024 •

edited

Loading

macfarla left a comment

macfarla Mar 25, 2024

usmansaleem Mar 25, 2024

usmansaleem Mar 25, 2024

macfarla Mar 25, 2024

usmansaleem Mar 25, 2024

jframe Mar 26, 2024

usmansaleem Mar 26, 2024

jframe Mar 26, 2024

usmansaleem Mar 26, 2024

jframe Mar 26, 2024

jframe Mar 26, 2024

jframe Mar 26, 2024

jframe Mar 26, 2024

jframe Mar 26, 2024

jframe Mar 26, 2024

usmansaleem Mar 26, 2024

Refactor Besu custom error prone dependency #6692

Refactor Besu custom error prone dependency #6692

Conversation

usmansaleem commented Mar 7, 2024 • edited Loading

PR description

Thanks for sending a pull request! Have you done the following?

Most advanced CI tests are deferred until PR approval, but you could:

Fixed Issue(s)

macfarla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

usmansaleem commented Mar 7, 2024 •

edited

Loading