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

Update to new version of jc-kzg-4844 lib #7849

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

vaidikcode
Copy link
Contributor

Signed-off-by: vaidikcode [email protected]

PR description

Updated all the related dependencies

Fixed Issue(s)

fixes #7848

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

Locally, you can run these tests to catch failures early:

  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

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

Hi @vaidikcode can you outline any testing you have done for this please?
I am wondering if we need to check there isn't a performance regression.

@siladu
Copy link
Contributor

siladu commented Nov 4, 2024

From https://github.com/ethereum/c-kzg-4844/releases/tag/v2.0.0 sounds like there could be some negative memory impact along with the speed improvement

Compared to precompute=0, precompute=8 is ~75% faster but uses 96 MiB of memory.

@vaidikcode
Copy link
Contributor Author

Hi @siladu ,
I didn't pay much attention to the precompute value when I updated the dependency; I followed the recommendation from the official dependency website. However, now that you've mentioned it, I realize it could significantly impact memory usage. What do you think I should do next?

@fab-10
Copy link
Contributor

fab-10 commented Nov 4, 2024

Teku already did the update Consensys/teku#8651, so we can ask to @StefanBratanov insight about the possible memory impact

@StefanBratanov
Copy link

StefanBratanov commented Nov 4, 2024

Teku already did the update Consensys/teku#8651, so we can ask to @StefanBratanov insight about the possible memory impact

This value is for PeerDAS, I was told to have the same memory footprint as previous ckzg versions should set the value to 0.

Signed-off-by: vaidikcode <[email protected]>
Copy link
Contributor

@jflo jflo left a comment

Choose a reason for hiding this comment

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

I'd like to put this on hold till we can verify the dependency via verification-metadata.xml

Copy link
Contributor

@jflo jflo left a comment

Choose a reason for hiding this comment

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

confirmed, this looks good thanks

@siladu
Copy link
Contributor

siladu commented Nov 5, 2024

@vaidikcode there's some failing tests, could you try to reproduce locally please?

@vaidikcode
Copy link
Contributor Author

@siladu working on it

Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

Remove the version where not needed, and according to the c-kzg-4844 changelog

The trusted setup format has been updated to include G1 points in monomial form.

This version of c-kzg-4844 requires the new trusted setup file.
The new trusted setup file can be used with previous versions of c-kzg-4844.
Download the new file from [trusted_setup.txt](https://github.com/ethereum/c-kzg-4844/blob/4b915a6617a85ef8c49d456374c1bc7f8647d968/src/trusted_setup.txt) in this repository.
This should match [trusted_setup_4096.json](https://github.com/ethereum/consensus-specs/blob/13ac373a2c284dc66b48ddd2ef0a10537e4e0de6/presets/mainnet/trusted_setups/trusted_setup_4096.json) in the specifications.

So I expect that the trusted setup file should be update as well, @StefanBratanov was that the case in Teku?

evm/build.gradle Outdated Show resolved Hide resolved
besu/build.gradle Outdated Show resolved Hide resolved
ethereum/core/build.gradle Outdated Show resolved Hide resolved
@StefanBratanov
Copy link

Remove the version where not needed, and according to the c-kzg-4844 changelog

The trusted setup format has been updated to include G1 points in monomial form.

This version of c-kzg-4844 requires the new trusted setup file.
The new trusted setup file can be used with previous versions of c-kzg-4844.
Download the new file from [trusted_setup.txt](https://github.com/ethereum/c-kzg-4844/blob/4b915a6617a85ef8c49d456374c1bc7f8647d968/src/trusted_setup.txt) in this repository.
This should match [trusted_setup_4096.json](https://github.com/ethereum/consensus-specs/blob/13ac373a2c284dc66b48ddd2ef0a10537e4e0de6/presets/mainnet/trusted_setups/trusted_setup_4096.json) in the specifications.

So I expect that the trusted setup file should be update as well, @StefanBratanov was that the case in Teku?

Yes. It was updated. The new one is in the repo itself.

@vaidikcode
Copy link
Contributor Author

I followed the instructions to update the trusted setup, but the build still fails, and I am unable to identify the issue.

@fab-10
Copy link
Contributor

fab-10 commented Nov 7, 2024

something in the native code, try running Gradle locally with these options to replicate https://github.com/hyperledger/besu/actions/runs/11717776620?pr=7849#summary-32644913643

----------- Stack Backtrace -----------
free+0x1e (0x00007F09DF6A53FE [libc.so.6+0xa53fe])
free_trusted_setup+0x2a (0x00007F09CCFCB6EA [libckzg4844jni.so+0xe6ea])
Java_ethereum_ckzg4844_CKZG4844JNI_loadTrustedSetup__Ljava_lang_String_2J+0x101 (0x00007F09CCFC39F1 [libckzg4844jni.so+0x69f1])
ffi_call_unix64+0x52 (0x00007F09DF01424A [libj9vm29.so+0x21424a])
ffi_call_int+0x1a1 (0x00007F09DF0133E1 [libj9vm29.so+0x2133e1])
_ZN32VM_BytecodeInterpreterCompressed3runEP10J9VMThread+0x11718 (0x00007F09DEEB6AB8 [libj9vm29.so+0xb6ab8])
bytecodeLoopCompressed+0xca (0x00007F09DEEA538A [libj9vm29.so+0xa538a])
 (0x00007F09DEFA0922 [libj9vm29.so+0x1a0922])
---------------------------------------

@vaidikcode
Copy link
Contributor Author

Do i need to update these files too?
Directory Name
if yes how?

@vaidikcode
Copy link
Contributor Author

@fab-10 do i need to update the above mentioned files too?

@fab-10
Copy link
Contributor

fab-10 commented Nov 25, 2024

@vaidikcode sorry I haven't too much time to look into this now, will check later what it is needed

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.

5 participants