KZG point eval precompile #4860

jflo · 2022-12-22T16:22:02Z

PR description

Adds a new precompile as part of EIP-4844 which allows contracts to evaluate KZG commitments.

Fixed Issue(s)

fixes #4822

Documentation

I thought about documentation and added the doc-change-required label to this PR if
updates are required.

Changelog

I thought about the changelog and included a changelog update if required.

evm/src/main/java/org/hyperledger/besu/evm/precompile/KZGPointEvalPrecompiledContract.java

.github/workflows/dco.yml

CHANGELOG.md

siladu

Added some comments but still want to understand and play with the tests a bit more.

Regarding the task list on #4822...

package the mainnet kzg trusted setup file so it is delivered via distribution in such a way that it can be loaded by native lib from the filesystem; loading from classpath won't work.

What was the solution for this?

add support for reftests that cover it

Was this done in this PR?

CHANGELOG.md

evm/src/main/java/org/hyperledger/besu/evm/internal/EvmConfiguration.java

evm/src/main/java/org/hyperledger/besu/evm/precompile/MainnetPrecompiledContracts.java

ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/peers/EnodeURLImplTest.java

evm/src/main/java/org/hyperledger/besu/evm/precompile/KZGPointEvalPrecompiledContract.java

evm/src/test/java/org/hyperledger/besu/evm/precompile/KZGPointEvalPrecompileContractTest.java

evm/src/main/java/org/hyperledger/besu/evm/precompile/MainnetPrecompiledContracts.java

Signed-off-by: Justin Florentine <justin+github@florentine.us>

jflo · 2023-01-15T17:46:39Z

Loading from classpath is fine, and user will be able to override that later. We are still waiting on retesteth updates to cover it.

Signed-off-by: Justin Florentine <justin+github@florentine.us>

fab-10

looks good, please check the comments

...rc/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetPrecompiledContractRegistries.java

besu/src/main/java/org/hyperledger/besu/cli/options/unstable/EvmOptions.java

evm/src/main/java/org/hyperledger/besu/evm/internal/EvmConfiguration.java

evm/src/main/java/org/hyperledger/besu/evm/precompile/KZGPointEvalPrecompiledContract.java

Signed-off-by: Justin Florentine <justin+github@florentine.us>

…GpointEvalPrecompile

Signed-off-by: Justin Florentine <justin+github@florentine.us>

fab-10

Some old and new comments

CHANGELOG.md

besu/src/main/java/org/hyperledger/besu/cli/options/unstable/EvmOptions.java

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetProtocolSpecs.java

evm/src/main/java/org/hyperledger/besu/evm/internal/EvmConfiguration.java

evm/src/main/java/org/hyperledger/besu/evm/precompile/KZGPointEvalPrecompiledContract.java

…GpointEvalPrecompile

Signed-off-by: Justin Florentine <justin+github@florentine.us>

evm/src/main/java/org/hyperledger/besu/evm/precompile/KZGPointEvalPrecompiledContract.java

+          KZGPointEvalPrecompiledContract.class.getResourceAsStream(
+              "mainnet_kzg_trusted_setup_4096.txt");
+      try {
+        File jniWillLoadFrom = File.createTempFile("kzgTrustedSetup", "txt");


Signed-off-by: Justin Florentine <justin+github@florentine.us>

fab-10

LGTM, just a nit

fab-10 · 2023-01-20T18:23:56Z

besu/src/main/java/org/hyperledger/besu/cli/options/unstable/EvmOptions.java

-        jumpDestCacheWeightKilobytes, Optional.ofNullable(kzgTrustedSetupFilePath));
+        jumpDestCacheWeightKilobytes,
+        kzgTrustedSetupFilePath != null
+            ? Optional.ofNullable(Path.of(kzgTrustedSetupFilePath))


nit: result of Path::of is never null

This is not a base level configuration option. This is the wrong place in code to do it. It should be an argument in the MainnetEVMs methods for relevant versions.

shemnon

I don't like how the KZG files are being brought in. Especially if we already have hard-coded a default as part of the library. It's a config for one precompile so it shouldn't impact the configuration of the base EVM.

Also, once it's up and running how often will alternate KZGs be brought in? Do we really need configurability?

shemnon · 2023-01-20T18:32:02Z

besu/src/main/java/org/hyperledger/besu/cli/options/unstable/EvmOptions.java

@@ -29,6 +31,8 @@ public class EvmOptions implements CLIOptions<EvmConfiguration> {
  /** The constant JUMPDEST_CACHE_WEIGHT. */
  public static final String JUMPDEST_CACHE_WEIGHT = "--Xevm-jumpdest-cache-weight-kb";

+  public static final String KZG_TRUSTED_SETUP_FILE_PATH = "--evm-kzg-trusted-setup-path";


I think this should be an --X option. Once the ceremony is done we will have the kzg embedded in the distribution, correct? So this is just transitional for testing?

shemnon · 2023-01-20T18:33:09Z

besu/src/main/java/org/hyperledger/besu/cli/options/unstable/EvmOptions.java

-        jumpDestCacheWeightKilobytes, Optional.ofNullable(kzgTrustedSetupFilePath));
+        jumpDestCacheWeightKilobytes,
+        kzgTrustedSetupFilePath != null
+            ? Optional.ofNullable(Path.of(kzgTrustedSetupFilePath))


This is not a base level configuration option. This is the wrong place in code to do it. It should be an argument in the MainnetEVMs methods for relevant versions.

shemnon · 2023-01-20T18:35:22Z

build.gradle

@@ -109,7 +109,7 @@ allprojects {
    }
    maven {
      url 'https://artifacts.consensys.net/public/maven/maven/'
-      content { includeGroupByRegex('tech\\.pegasys\\..*') }
+      content { includeGroupByRegex('tech\\.pegasys.*') }


Suggested change

content { includeGroupByRegex('tech\\.pegasys.*') }

content { includeGroupByRegex('tech\\.pegasys(\\..*)?') }

Removing the dot opens the door wide to uncontrolled DNS names like tech.pegasys-but-not-really:comprimised-kzg. Instead it should be listed as an optional section.

shemnon · 2023-01-20T18:35:57Z

datatypes/src/main/java/org/hyperledger/besu/datatypes/Address.java

@@ -71,7 +71,8 @@ public class Address extends DelegatingBytes implements org.hyperledger.besu.plu
  /** The constant BLS12_MAP_FP2_TO_G2. */
  public static final Address BLS12_MAP_FP2_TO_G2 = Address.precompiled(0x12);

-  /** The constant ZERO. */
+  public static final Address KZG_POINT_EVAL = Address.precompiled(0x14);


Is this address fixed already? Has BLS been "locked in" to those addresses?

As far as i know it is, the specs have been stable on it.

shemnon · 2023-01-20T18:41:03Z

ethereum/core/build.gradle

+repositories {
+  maven { url "https://artifacts.consensys.net/public/maven/maven/" }
+}
+


This should not be needed here and should be removed. The repository is already enumerated in the parent build file.

shemnon · 2023-01-20T18:41:51Z

ethereum/core/build.gradle

@@ -54,6 +58,7 @@ dependencies {
  implementation 'org.apache.tuweni:tuweni-concurrent'
  implementation 'org.apache.tuweni:tuweni-units'
  implementation 'org.apache.tuweni:tuweni-rlp'
+  implementation 'tech.pegasys:jc-kzg-4844:0.1.0'


Versions should not be listed in child build files. Please add an entry to <root>/gradle/versions.gradle

shemnon · 2023-01-20T18:42:29Z

evm/build.gradle

+repositories {
+  maven { url "https://artifacts.consensys.net/public/maven/maven/" }
+}


This should not be here, the root gradle should cover this. If it doesn't it should be fixed there.

evm/build.gradle

evm/src/main/java/org/hyperledger/besu/evm/internal/EvmConfiguration.java

jflo · 2023-01-21T14:09:08Z

I don't like how the KZG files are being brought in. Especially if we already have hard-coded a default as part of the library. It's a config for one precompile so it shouldn't impact the configuration of the base EVM.

Also, once it's up and running how often will alternate KZGs be brought in? Do we really need configurability?

Yeah, the cart may be before the horse. I'm gonna remove configurability for interop, and we can add it back in if we actually discover a use case for it.

Signed-off-by: Justin Florentine <justin+github@florentine.us>

…ry load time Signed-off-by: Justin Florentine <justin+github@florentine.us>

Signed-off-by: Justin Florentine <justin+github@florentine.us>

* KZG implementation Signed-off-by: Justin Florentine <justin+github@florentine.us>

jflo added the doc-change-required Indicates an issue or PR that requires doc to be updated label Dec 22, 2022

jflo force-pushed the KZGpointEvalPrecompile branch from 14c8dfd to 6e98f95 Compare December 22, 2022 16:23

github-advanced-security bot found potential problems Dec 22, 2022

View reviewed changes

evm/src/main/java/org/hyperledger/besu/evm/precompile/KZGPointEvalPrecompiledContract.java Fixed Show fixed Hide fixed

jflo marked this pull request as ready for review January 3, 2023 14:15

siladu reviewed Jan 3, 2023

View reviewed changes

.github/workflows/dco.yml Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

jflo marked this pull request as draft January 4, 2023 02:18

jflo force-pushed the KZGpointEvalPrecompile branch 2 times, most recently from 524830b to 2e21716 Compare January 6, 2023 15:23

jflo requested a review from siladu January 6, 2023 15:34

jflo marked this pull request as ready for review January 6, 2023 15:34

siladu reviewed Jan 11, 2023

View reviewed changes

jflo added 2 commits January 15, 2023 11:11

KZG implementation

a0aabbd

Signed-off-by: Justin Florentine <justin+github@florentine.us>

corrects test that fail if hostname is set to something unresolvable

541365d

Signed-off-by: Justin Florentine <justin+github@florentine.us>

adds precompile error handling

685cb10

Signed-off-by: Justin Florentine <justin+github@florentine.us>

jflo force-pushed the KZGpointEvalPrecompile branch from 2e21716 to 685cb10 Compare January 15, 2023 17:52

jflo requested review from gezero and fab-10 January 15, 2023 21:33

fab-10 reviewed Jan 16, 2023

View reviewed changes

jflo added 7 commits January 19, 2023 14:37

KZG implementation

86212c8

Signed-off-by: Justin Florentine <justin+github@florentine.us>

adds precompile error handling

6c9102d

Signed-off-by: Justin Florentine <justin+github@florentine.us>

checks output in happy path test

113e083

Signed-off-by: Justin Florentine <justin+github@florentine.us>

spotless

883d7d0

Signed-off-by: Justin Florentine <justin+github@florentine.us>

KZG implementation

ebaacd6

Signed-off-by: Justin Florentine <justin+github@florentine.us>

adds precompile error handling

95b3b6c

Signed-off-by: Justin Florentine <justin+github@florentine.us>

Merge branch 'KZGpointEvalPrecompile' of github.com:jflo/besu into KZ…

2bad3ed

…GpointEvalPrecompile

jflo requested review from siladu and fab-10 January 19, 2023 19:41

jflo added 2 commits January 20, 2023 07:03

KZG implementation

5a802e0

Signed-off-by: Justin Florentine <justin+github@florentine.us>

adds precompile error handling

7d2b70a

Signed-off-by: Justin Florentine <justin+github@florentine.us>

jflo added 5 commits January 20, 2023 07:03

KZG implementation

8cb5003

Signed-off-by: Justin Florentine <justin+github@florentine.us>

adds precompile error handling

92a70af

Signed-off-by: Justin Florentine <justin+github@florentine.us>

KZG implementation

96fe0e3

Signed-off-by: Justin Florentine <justin+github@florentine.us>

adds precompile error handling

9a9a51e

Signed-off-by: Justin Florentine <justin+github@florentine.us>

corrects CHANGELOG

c4b9ece

Signed-off-by: Justin Florentine <justin+github@florentine.us>

jflo force-pushed the KZGpointEvalPrecompile branch from 2bad3ed to c4b9ece Compare January 20, 2023 12:08

fab-10 reviewed Jan 20, 2023

View reviewed changes

jflo added 5 commits January 20, 2023 08:03

Merge branch 'KZGpointEvalPrecompile' of github.com:jflo/besu into KZ…

8942c87

…GpointEvalPrecompile

Merge branch 'main' into KZGpointEvalPrecompile

c324a45

updates from main

8a5e696

Signed-off-by: Justin Florentine <justin+github@florentine.us>

put humpty dumpty back together again

8f5a80d

Signed-off-by: Justin Florentine <justin+github@florentine.us>

expected val needed padding

ae8523f

Signed-off-by: Justin Florentine <justin+github@florentine.us>

github-advanced-security bot found potential problems Jan 20, 2023

View reviewed changes

uses paths when possible

4bb9c9c

Signed-off-by: Justin Florentine <justin+github@florentine.us>

fab-10 approved these changes Jan 20, 2023

View reviewed changes

shemnon requested changes Jan 20, 2023

View reviewed changes

jflo added 2 commits January 21, 2023 12:56

removes kzg setup configurability, adjusts dependencies

8f9c1e9

Signed-off-by: Justin Florentine <justin+github@florentine.us>

use a second constructor, since bit length needs to be known at libra…

904d463

…ry load time Signed-off-by: Justin Florentine <justin+github@florentine.us>

jflo requested a review from shemnon January 23, 2023 10:16

shemnon approved these changes Jan 23, 2023

View reviewed changes

Merge branch 'main' into KZGpointEvalPrecompile

e1dd9c1

Signed-off-by: Justin Florentine <justin+github@florentine.us>

jflo enabled auto-merge (squash) January 23, 2023 12:59

Merge branch 'main' into KZGpointEvalPrecompile

3f87644

jflo merged commit f8a0f87 into hyperledger:main Jan 23, 2023

macfarla mentioned this pull request Jan 24, 2023

[MINOR] javadoc for kzg precompile #4988

Merged

2 tasks

garyschulte pushed a commit to garyschulte/besu that referenced this pull request Jan 24, 2023

KZG point eval precompile (hyperledger#4860)

4b77566

* KZG implementation Signed-off-by: Justin Florentine <justin+github@florentine.us>

alexandratran removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Feb 1, 2023

elenduuche pushed a commit to elenduuche/besu that referenced this pull request Aug 16, 2023

KZG point eval precompile (hyperledger#4860)

260a33b

* KZG implementation Signed-off-by: Justin Florentine <justin+github@florentine.us>

eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023

KZG point eval precompile (hyperledger#4860)

6186d19

* KZG implementation Signed-off-by: Justin Florentine <justin+github@florentine.us>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KZG point eval precompile #4860

KZG point eval precompile #4860

jflo commented Dec 22, 2022

siladu left a comment

jflo commented Jan 15, 2023

fab-10 left a comment

fab-10 left a comment

fab-10 left a comment

fab-10 Jan 20, 2023

shemnon Jan 20, 2023

shemnon left a comment

shemnon Jan 20, 2023

shemnon Jan 20, 2023

shemnon Jan 20, 2023

shemnon Jan 20, 2023

jflo Jan 21, 2023

shemnon Jan 20, 2023

shemnon Jan 20, 2023

shemnon Jan 20, 2023

jflo commented Jan 21, 2023

	content { includeGroupByRegex('tech\\.pegasys.*') }
	content { includeGroupByRegex('tech\\.pegasys(\\..*)?') }

KZG point eval precompile #4860

KZG point eval precompile #4860

Conversation

jflo commented Dec 22, 2022

PR description

Fixed Issue(s)

Documentation

Changelog

siladu left a comment

Choose a reason for hiding this comment

jflo commented Jan 15, 2023

fab-10 left a comment

Choose a reason for hiding this comment

fab-10 left a comment

Choose a reason for hiding this comment

fab-10 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shemnon 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

jflo commented Jan 21, 2023