Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initialize KZG native lib at startup if Cancun is enabled #5084

Merged
merged 14 commits into from
Feb 20, 2023

Conversation

fab-10
Copy link
Contributor

@fab-10 fab-10 commented Feb 10, 2023

Signed-off-by: Fabio Di Fabio fabio.difabio@consensys.net

PR description

Move c-kzg native lib initialization at startup, when all the other native lib are configured, and also because it is not only used by the precompile, but also for transaction validation.
Added the option --kzg-trusted-setup to pass a custom setup file for custom networks or to override the default one for named networks

Fixed Issue(s)

Documentation

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

Acceptance Tests (Non Mainnet)

  • I have considered running ./gradlew acceptanceTestNonMainnet locally if my PR affects non-mainnet modules.

Changelog

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
@fab-10 fab-10 added the doc-change-required Indicates an issue or PR that requires doc to be updated label Feb 10, 2023
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
# Conflicts:
#	evm/src/main/java/org/hyperledger/besu/evm/precompile/KZGPointEvalPrecompiledContract.java
#	evm/src/test/java/org/hyperledger/besu/evm/precompile/KZGPointEvalPrecompileContractTest.java
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
… kzg-trusted-setup-param

# Conflicts:
#	CHANGELOG.md
#	besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java
#	evm/src/main/java/org/hyperledger/besu/evm/precompile/KZGPointEvalPrecompiledContract.java
#	evm/src/test/java/org/hyperledger/besu/evm/precompile/KZGPointEvalPrecompileContractTest.java
… kzg-trusted-setup-param

# Conflicts:
#	CHANGELOG.md
#	besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java
#	evm/src/main/java/org/hyperledger/besu/evm/precompile/KZGPointEvalPrecompiledContract.java
#	evm/src/test/java/org/hyperledger/besu/evm/precompile/KZGPointEvalPrecompileContractTest.java
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
@fab-10 fab-10 marked this pull request as ready for review February 16, 2023 18:08
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Copy link
Contributor

@shemnon shemnon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static class initialization of the mainnet KZG is a hard stop for me. It should be loaded lazily or on-demand.

private static final Bytes successResult;

static {
CKZG4844JNI.loadNativeLibrary(CKZG4844JNI.Preset.MAINNET);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this initialization to a lazy or on-demand initialization.

This will incur a large startup penalty on every CLI call that touches the EVM. This imposes a debilitating penalty on the EVM Tool and will make using Besu in reference tests impractical.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense, will move it in the init methods

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
@fab-10 fab-10 requested a review from shemnon February 16, 2023 20:09
@shemnon shemnon dismissed their stale review February 17, 2023 15:54

Requested change to init was applied

Copy link
Contributor

@shemnon shemnon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed that this eliminates the ~2s startup hit I've seen.

@fab-10 fab-10 enabled auto-merge (squash) February 20, 2023 10:38
@fab-10 fab-10 merged commit 53b4afc into hyperledger:main Feb 20, 2023
@fab-10 fab-10 deleted the kzg-trusted-setup-param branch February 20, 2023 10:58
@bgravenorst bgravenorst removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Feb 22, 2023
ensi321 pushed a commit to ensi321/besu that referenced this pull request Mar 6, 2023
…r#5084)


Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
elenduuche pushed a commit to elenduuche/besu that referenced this pull request Aug 16, 2023
…r#5084)


Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
…r#5084)


Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants