Coordinator: replace teku ExecutionPlayloadV1 by Domain Block class - fix block encoding #406

jpnovais · 2024-12-06T19:17:50Z

This PR implements issue(s) #

Checklist

I wrote new tests for my new core changes.
I have successfully ran tests, style checker and build against my new changes locally.
I have informed the team of any breaking changes if there are any.

codecov-commenter · 2024-12-06T19:26:47Z

Codecov Report

Attention: Patch coverage is 27.46711% with 441 lines in your changes missing coverage. Please review.

Project coverage is 67.46%. Comparing base (8cf2866) to head (41f40b0).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...omain-models/src/main/kotlin/linea/domain/Block.kt	25.68%	79 Missing and 2 partials ⚠️
...models/src/main/kotlin/linea/domain/Transaction.kt	34.86%	46 Missing and 25 partials ⚠️
...ain/kotlin/linea/domain/MapperLineaDomainToBesu.kt	43.90%	40 Missing and 6 partials ⚠️
...essor/src/main/kotlin/linea/blob/BlobCompressor.kt	0.00%	46 Missing ⚠️
...ain/kotlin/linea/domain/MapperBesuToLineaDomain.kt	0.00%	41 Missing ⚠️
...tlin/linea/web3j/EthGetBlockToLineaBlockMappers.kt	39.21%	27 Missing and 4 partials ⚠️
.../coordinator/blockcreation/BlockCreationMonitor.kt	0.00%	30 Missing ⚠️
...src/main/kotlin/linea/rlp/BesuRlpMainnetEncoder.kt	0.00%	19 Missing ⚠️
...u-rlp-and-mappers/src/main/kotlin/linea/rlp/RLP.kt	0.00%	18 Missing ⚠️
...nsions/src/main/kotlin/linea/web3j/Web3JFactory.kt	0.00%	15 Missing ⚠️
... and 12 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #406      +/-   ##
============================================
- Coverage     70.17%   67.46%   -2.72%     
- Complexity     1070     1086      +16     
============================================
  Files           306      315       +9     
  Lines         12337    12638     +301     
  Branches       1179     1267      +88     
============================================
- Hits           8658     8526     -132     
- Misses         3200     3605     +405     
- Partials        479      507      +28

Flag	Coverage Δ		*Carryforward flag
hardhat	`98.70% <ø> (ø)`		Carriedforward from f4a0147
kotlin	`64.99% <27.46%> (-2.88%)`	⬇️

*This pull request uses carry forward flags. Click here to find out more.

Files with missing lines	Coverage Δ
.../consensys/zkevm/coordinator/app/CoordinatorApp.kt	`0.00% <ø> (ø)`
...clients/prover/FileBasedExecutionProverClientV2.kt	`59.01% <100.00%> (ø)`
...dination/blockcreation/BlockCreationCoordinator.kt	`100.00% <100.00%> (ø)`
...on/conflation/BlockToBatchSubmissionCoordinator.kt	`91.66% <100.00%> (-0.23%)`	⬇️
...conflation/upgrade/SwitchAwareConflationHandler.kt	`100.00% <100.00%> (ø)`
.../src/main/kotlin/net/consensys/StringExtensions.kt	`85.71% <100.00%> (+19.04%)`	⬆️
...src/main/kotlin/net/consensys/TypingsExtensions.kt	`78.37% <100.00%> (ø)`
.../kotlin/net/consensys/linea/web3j/ExtendedWeb3J.kt	`0.00% <ø> (ø)`
...nsys/zkevm/ethereum/coordination/MaxLongTracker.kt	`19.23% <0.00%> (ø)`
...um/coordination/blockcreation/SafeBlockProvider.kt	`0.00% <0.00%> (-19.05%)`	⬇️
... and 20 more

... and 4 files with indirect coverage changes

Filter94 · 2024-12-10T10:18:13Z

build.gradle

@@ -23,6 +23,12 @@ allprojects {
  apply plugin: 'java' // do not add kotlin plugin here, it will add unnecessary Kotlin runtime dependencies
  apply plugin: 'jacoco'

+  configurations.all {
+    resolutionStrategy {
+      force "org.web3j:core:${libs.versions.web3j.get()}"


I pulled your code, removed this addition, built the project and it went fine. What was breaking for you so you had to add it? I want to check if we can work around it some other way on my machine

Yes, it works. The reason I added this is because we have multiple versions of web3j, through transitive dependencies and when coding/debugging intellIJ was linking to the wrong one. So did this a quick workaround and ended up pushing by accident.

I am aware that this is not a solution that scales! We shall use proper management like spring plugin but I don't have time to address it now.
I am happy to revert or keep it since it does not cause any harm anyway.

If it doesn't cause issues for you, I'd like to remove it, because it creates a bad precedent

...or/app/src/main/kotlin/net/consensys/zkevm/coordinator/blockcreation/BlockCreationMonitor.kt

Filter94 · 2024-12-10T10:37:15Z

...or/app/src/main/kotlin/net/consensys/zkevm/coordinator/blockcreation/BlockCreationMonitor.kt

@@ -21,7 +24,7 @@ import kotlin.time.Duration.Companion.days

 class BlockCreationMonitor(
  private val vertx: Vertx,
-  private val extendedWeb3j: ExtendedWeb3J,
+  private val web3j: Web3j,


Why are we going back to plain Web3j here and in other places?

Because it was simpler this way and this extension is not properly designed and refactoring adapting would be more work. We created this interface to basically facilitate mock<> and return responses, if we create a fake rpc client this won't be necessary, we can tests against state that concrete implementations. That's my pan for new tests of BlockCreationMonitor.

getNetNextSafeBlock is more complex now. Is it possible to make this change later, at the same time with the new tests?

...pp/src/test/kotlin/net/consensys/zkevm/coordinator/blockcreation/BlockCreationMonitorTest.kt

Filter94 · 2024-12-10T10:46:27Z

jvm-libs/generic/extensions/kotlin/src/test/kotlin/net/consensys/StringExtensionsTest.kt

@@ -27,4 +27,30 @@ class StringExtensionsTest {
    assertThat("this includes lorem ipsum".containsAny(stringList, ignoreCase = true)).isTrue()
    assertThat("this string won't match".containsAny(stringList, ignoreCase = true)).isFalse()
  }
+
+  @OptIn(ExperimentalStdlibApi::class)


I didn't catch what experimental features we're using here

It had before, then I forgot to remove, will clean it, thanks

jvm-libs/linea/besu-rlp-and-mappers/src/main/kotlin/linea/domain/TransactionTypeMapper.kt

…r/blockcreation/BlockCreationMonitor.kt Co-authored-by: Roman Vaseev <[email protected]> Signed-off-by: Pedro Novais <[email protected]>

.../linea/web3j-extensions/src/test/kotlin/net/consensys/linea/web3j/DomainObjectMappersTest.kt

jonesho · 2024-12-10T17:11:00Z

gradle/libs.versions.toml

@@ -17,6 +17,7 @@ junit = "5.10.1"
 kotlinxDatetime = "0.6.1"
 ktlint = "0.47.0"
 log4j = "2.20.0"
+slf4j = "1.7.30"


Wondering where is it being used?

jonesho · 2024-12-10T17:20:50Z

jvm-libs/linea/besu-rlp-and-mappers/src/main/kotlin/linea/domain/MapperBesuToLineaDomain.kt

+      nonce = transaction.nonce.toULong(),
+      gasPrice = transaction.getGasPrice().getOrNull()?.toBigInteger()?.toULong(),
+      gasLimit = transaction.gasLimit.toULong(),
+      to = transaction.to?.getOrNull()?.toArray(),


Without the ? operator should be sufficient as to = transaction.to.getOrNull()?.toArray(), same for the ? operators before getOrNull() below

# Conflicts: # coordinator/clients/shomei-client/build.gradle # jvm-libs/linea/testing/teku-helper/build.gradle

jpnovais requested a review from a team December 6, 2024 19:17

jpnovais temporarily deployed to docker-build-and-e2e December 6, 2024 19:18 — with GitHub Actions Inactive

jpnovais temporarily deployed to docker-build-and-e2e December 6, 2024 20:11 — with GitHub Actions Inactive

jpnovais temporarily deployed to docker-build-and-e2e December 6, 2024 22:39 — with GitHub Actions Inactive

coordinator: replace Teku ExecutionPayloadV1.kt by our own Domain Block

adf559c

jpnovais force-pushed the coordinator-fix-blockencoding branch from b8a6102 to adf559c Compare December 9, 2024 18:41

jpnovais temporarily deployed to docker-build-and-e2e December 9, 2024 18:43 — with GitHub Actions Inactive

coordinator: fix tx mappings

1082b74

jpnovais temporarily deployed to docker-build-and-e2e December 9, 2024 21:07 — with GitHub Actions Inactive

jpnovais requested review from gauravahuja, jonesho and Filter94 December 9, 2024 21:54

coordinator: improve tests

1044b2b

jpnovais had a problem deploying to docker-build-and-e2e December 9, 2024 22:14 — with GitHub Actions Error

Filter94 reviewed Dec 10, 2024

View reviewed changes

...or/app/src/main/kotlin/net/consensys/zkevm/coordinator/blockcreation/BlockCreationMonitor.kt Outdated Show resolved Hide resolved

Filter94 reviewed Dec 10, 2024

View reviewed changes

...pp/src/test/kotlin/net/consensys/zkevm/coordinator/blockcreation/BlockCreationMonitorTest.kt Show resolved Hide resolved

Filter94 reviewed Dec 10, 2024

View reviewed changes

jvm-libs/linea/besu-rlp-and-mappers/src/main/kotlin/linea/domain/TransactionTypeMapper.kt Show resolved Hide resolved

Update coordinator/app/src/main/kotlin/net/consensys/zkevm/coordinato…

f4a0147

…r/blockcreation/BlockCreationMonitor.kt Co-authored-by: Roman Vaseev <[email protected]> Signed-off-by: Pedro Novais <[email protected]>

jpnovais had a problem deploying to docker-build-and-e2e December 10, 2024 11:00 — with GitHub Actions Error

Filter94 reviewed Dec 10, 2024

View reviewed changes

.../linea/web3j-extensions/src/test/kotlin/net/consensys/linea/web3j/DomainObjectMappersTest.kt Show resolved Hide resolved

coordinator: carveout compressor of coordinator

796d7fe

jpnovais had a problem deploying to docker-build-and-e2e December 10, 2024 12:18 — with GitHub Actions Error

coordinator: update maven release runner

db99fa1

jpnovais had a problem deploying to docker-build-and-e2e December 10, 2024 14:08 — with GitHub Actions Error

coordinator: update maven release runner

386e64d

jpnovais had a problem deploying to docker-build-and-e2e December 10, 2024 14:49 — with GitHub Actions Error

jpnovais added 3 commits December 10, 2024 16:19

coordinator: improver block encoder/decoder interfaces

8daa2d8

coordinator: improver block encoder/decoder interfaces

3632b12

coordinator: improver block encoder/decoder interfaces

41f40b0

jpnovais had a problem deploying to docker-build-and-e2e December 10, 2024 16:20 — with GitHub Actions Error

jonesho reviewed Dec 10, 2024

View reviewed changes

jpnovais added 5 commits December 10, 2024 21:55

coordinator: fix web3j to domain mapper

b008017

coordinator: increase maxExpectedCompressionRatio=20

2ec3ab1

coordinator: adds tool for backtesting compression/decompression

8058937

coordinator: fixes on Block decoder from L1 Blobs

2da1710

Merge branch 'main' into coordinator-fix-blockencoding

0d3bef6

# Conflicts: # coordinator/clients/shomei-client/build.gradle # jvm-libs/linea/testing/teku-helper/build.gradle

jpnovais requested a deployment to docker-build-and-e2e December 12, 2024 16:28 — with GitHub Actions Waiting

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coordinator: replace teku ExecutionPlayloadV1 by Domain Block class - fix block encoding #406

Coordinator: replace teku ExecutionPlayloadV1 by Domain Block class - fix block encoding #406

jpnovais commented Dec 6, 2024

codecov-commenter commented Dec 6, 2024 •

edited

Loading

Filter94 Dec 10, 2024

jpnovais Dec 10, 2024

Filter94 Dec 10, 2024

Filter94 Dec 10, 2024

jpnovais Dec 10, 2024 •

edited

Loading

Filter94 Dec 10, 2024

Filter94 Dec 10, 2024

jpnovais Dec 10, 2024

jonesho Dec 10, 2024

jonesho Dec 10, 2024 •

edited

Loading

Coordinator: replace teku ExecutionPlayloadV1 by Domain Block class - fix block encoding #406

Are you sure you want to change the base?

Coordinator: replace teku ExecutionPlayloadV1 by Domain Block class - fix block encoding #406

Conversation

jpnovais commented Dec 6, 2024

Checklist

codecov-commenter commented Dec 6, 2024 • edited Loading

Codecov Report

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

jpnovais Dec 10, 2024 • edited Loading

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

jonesho Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Dec 6, 2024 •

edited

Loading

jpnovais Dec 10, 2024 •

edited

Loading

jonesho Dec 10, 2024 •

edited

Loading