hyperledger / besu-native

Apache License 2.0
13 stars 36 forks source link

Constantine bindings for EIP196 #184

Closed NickSneo closed 1 month ago

NickSneo commented 3 months ago

Fixes https://github.com/hyperledger/besu/issues/7242 https://github.com/hyperledger/besu-native/issues/195 https://github.com/hyperledger/besu/issues/7243

garyschulte commented 3 months ago

👀

just a tracking comment

NickSneo commented 2 months ago

@garyschulte @mratsim Pushed some new commits, I shall be removing the files inside constantine/constantine-jni/lib/ folder and planning to compile constantine lib using gradle task and use those files directly. The PR is still in draft but can you review to check my approach is I am going in correct direction.

Thanks

NickSneo commented 2 months ago

This PR is looking good. A good next step would be to implement the EIP196 test cases that exist for the matter-labs and gnark-crypto implementations.

Much of the contract 'shim' implementation for the besu evm can be copied wholesale, including the test cases.

Edit: looking at the CI output, it looks like there is a problem loading the generated library in java.

Fixing it right now

NickSneo commented 2 months ago

generally LGTM. some suggestions in comments. It would be helpful to add a README.md that describes the library, purpose, and build instructions (like installing nim, clang, etc as build dependencies).

The changes I suggested to build.gradle plus adding nim to the package installs here, here, and here portions of .github/workflows/build.yml should get things building properly in CI

This and adding tests from Gnark and matter-labs is remaining, working on them.

NickSneo commented 2 months ago

This PR is looking good. A good next step would be to implement the EIP196 test cases that exist for the matter-labs and gnark-crypto implementations.

Much of the contract 'shim' implementation for the besu evm can be copied wholesale, including the test cases.

Edit: looking at the CI output, it looks like there is a problem loading the generated library in java.

Added test cases from Gnark Lib

NickSneo commented 2 months ago

@garyschulte Hey, seems the CI pipeline is failing, I tried to fix it but seems nim and nimble installed during the jobs are not available during final-assembly. Can you help me in finding out the issue? Is it due to I am doing make and compile using build.gradle and I should switch to build.sh for this compiling and make lib thing?

garyschulte commented 2 months ago

I repeated naive testing I did when comparing matter-labs, blst, and gnark-crypto. This PR compares favorably at least on this test machine:

Screenshot 2024-07-19 at 12 52 08 PM Screenshot 2024-07-19 at 12 52 22 PM Screenshot 2024-07-19 at 12 52 15 PM
NickSneo commented 2 months ago

This looks good, performance seems similar to Gnark but in G1Mul Gnark seems better. Should we go ahead with EIP2537?

garyschulte commented 2 months ago

This looks good, performance seems similar to Gnark but in G1Mul Gnark seems better. Should we go ahead with EIP2537?

👍 I think yes, these are good results that warrant adding bls12-381 also. Probably in a follow-on PR to keep the review scope down. I will :eyes: on the CI build issue

NickSneo commented 2 months ago

This looks good, performance seems similar to Gnark but in G1Mul Gnark seems better. Should we go ahead with EIP2537?

👍 I think yes, these are good results that warrant adding bls12-381 also. Probably in a follow-on PR to keep the review scope down. I will 👀 on the CI build issue

Sure, I will also try to fix it, maybe just missing some small part. Thanks!

garyschulte commented 2 months ago

Sure, I will also try to fix it, maybe just missing some small part. Thanks!

I will defer to you, happy to help if you need unblocking.

mratsim commented 2 months ago

I'm curious how I can reproduce this bench, on my machine raw perf (i.e. not the EVM wrapper), constantine is faster than Gnark by 15% image image


Edit instead of reply confusion by garyschulte

it is also worth noting that we are not using a release version of gnark-crypto, but rather built from this sha

garyschulte commented 2 months ago

I'm curious how I can reproduce this bench, on my machine raw perf (i.e. not the EVM wrapper), constantine is faster than Gnark by 15%

it is a naive performance comparison, just running the unit test cases repeatedly 1000x for each input in a tight loop (only 100x for pairing test).

garyschulte commented 2 months ago

@NickSneo do you want or need help with the CI troubles? I think this PR is very promising, especially in light of the fact that we would like to have Constantine verkle bindings in besu-native.

IMO besu-native would benefit from having generalized bindings for both gnark-crypto and Constantine, rather than a collection of bespoke libraries. In that regard, I am keenly interested in getting this PR merged to get Constantine into the repo.

garyschulte commented 2 months ago

I'm curious how I can reproduce this bench, on my machine raw perf (i.e. not the EVM wrapper), constantine is faster than Gnark by 15%

it is also worth noting that we are not using a release version of gnark-crypto, but rather built from this sha

Sorry - somehow I edited your comment rather than replying.

NickSneo commented 2 months ago

@NickSneo do you want or need help with the CI troubles? I think this PR is very promising, especially in light of the fact that we would like to have Constantine verkle bindings in besu-native.

IMO besu-native would benefit from having generalized bindings for both gnark-crypto and Constantine, rather than a collection of bespoke libraries. In that regard, I am keenly interested in getting this PR merged to get Constantine into the repo.

@garyschulte I tried fixing pipeline with build.gradle approach but was not successful, looking into another approach to build like other projects and include this process in the root build.sh file

NickSneo commented 2 months ago

@garyschulte Hey, with latest commit, everything is working fine with correct build.sh compiling nim shared lib and then in gradle.build copy it to correct place and tests working fine in my local machine. But Don't know why it is failing in pipeline.

Can you please check once? Thanks

garyschulte commented 2 months ago

The problem is the way CI expects to build these libraries. It expects to run ./build.sh on four different architectures to build the corresponding shared library - once each for x86 and arm64 versions of both linux and mac. The result of each build step should be the upload of the shared library file(or files) as an artifact with the operating system and arch as part of the path inside the artifact. Then on the final assembly step it expects each subproject to take each of the native artifacts and build a jar with the java code and the native artifacts embedded within it.

In the final product, the process expects to be able to find the four native artifacts from each of the four build steps so it can embed them into the final multi-arch jar:

darwin-aarch64/libconstantine.dylib darwin-x86-64/libconstantine.dylib linux-x86-64/libconstantine.so linux-aarch64/lib/libconstantine.so

Since CI is expecting to build across architectures, we should probably put a constantine build step into build.sh like:

build_constantine() {
  cat <<EOF
  ############################
  ####### build constantine #######
  ############################
EOF

  cd "$SCRIPTDIR"

  # delete old build dir, if exists
  rm -rf "$SCRIPTDIR/constantine/build" || true
  mkdir -p "$SCRIPTDIR/constantine/build/lib"

  if [[ "$OSTYPE" == "msys" ]]; then
        LIBRARY_EXTENSION=dll
  elif [[ "$OSTYPE" == "linux-gnu"* ]]; then
    LIBRARY_EXTENSION=so
  elif [[ "$OSTYPE" == "darwin"* ]]; then
    LIBRARY_EXTENSION=dylib
  fi

  ./gradlew :constantine:clean :constantine:compileNativeLibrary

  mkdir -p "$SCRIPTDIR/constantine/build/${OSARCH}/lib"
  cp constantine/constantine/lib/libconstantine.* "$SCRIPTDIR/constantine/build/${OSARCH}/lib"
}

...

build_constantine

and make the requisite changes to the jar building process for the constantine subproject so that it pulls in the jars from the os-arch friendly artifact directories.

Lastly, we need to add upload and download steps for constantine artifacts in build.yml native build tasks. In each native build task, add an additional step like:

      - uses: actions/upload-artifact@v3.1.0
        with:
          name: constantine native build artifacts
          path: constantine/build/{PUT_OS_ARCH_STRING_HERE}/lib

and in the final-assembly task, pull all of the artifacts down by adding a step like:

      - name: Download constantine
        uses: actions/download-artifact@v3
        with:
          name: constantine native build artifacts
          path: constantine/build
NickSneo commented 2 months ago

The problem is the way CI expects to build these libraries. It expects to run ./build.sh on four different architectures to build the corresponding shared library - once each for x86 and arm64 versions of both linux and mac. The result of each build step should be the upload of the shared library file(or files) as an artifact with the operating system and arch as part of the path inside the artifact. Then on the final assembly step it expects each subproject to take each of the native artifacts and build a jar with the java code and the native artifacts embedded within it.

In the final product, the process expects to be able to find the four native artifacts from each of the four build steps so it can embed them into the final multi-arch jar:

darwin-aarch64/libconstantine.dylib darwin-x86-64/libconstantine.dylib linux-x86-64/libconstantine.so linux-aarch64/lib/libconstantine.so

Since CI is expecting to build across architectures, we should probably put a constantine build step into build.sh like:

build_constantine() {
  cat <<EOF
  ############################
  ####### build constantine #######
  ############################
EOF

  cd "$SCRIPTDIR"

  # delete old build dir, if exists
  rm -rf "$SCRIPTDIR/constantine/build" || true
  mkdir -p "$SCRIPTDIR/constantine/build/lib"

  if [[ "$OSTYPE" == "msys" ]]; then
      LIBRARY_EXTENSION=dll
  elif [[ "$OSTYPE" == "linux-gnu"* ]]; then
    LIBRARY_EXTENSION=so
  elif [[ "$OSTYPE" == "darwin"* ]]; then
    LIBRARY_EXTENSION=dylib
  fi

  ./gradlew :constantine:clean :constantine:compileNativeLibrary

  mkdir -p "$SCRIPTDIR/constantine/build/${OSARCH}/lib"
  cp constantine/constantine/lib/libconstantine.* "$SCRIPTDIR/constantine/build/${OSARCH}/lib"
}

...

build_constantine

and make the requisite changes to the jar building process for the constantine subproject so that it pulls in the jars from the os-arch friendly artifact directories.

Lastly, we need to add upload and download steps for constantine artifacts in build.yml native build tasks. In each native build task, add an additional step like:

      - uses: actions/upload-artifact@v3.1.0
        with:
          name: constantine native build artifacts
          path: constantine/build/{PUT_OS_ARCH_STRING_HERE}/lib

and in the final-assembly task, pull all of the artifacts down by adding a step like:

      - name: Download constantine
        uses: actions/download-artifact@v3
        with:
          name: constantine native build artifacts
          path: constantine/build

Yes, I followed the exact same approach in my last commit - https://github.com/hyperledger/besu-native/pull/184/commits/7c9f6459edac570a764a1c84ff31c91554d08fcc but seems it is still failing. The upload and download steps are also working fine. But still the test is failing.

Seems the issue is from something else, I will investigate it further

NickSneo commented 1 month ago

Hey @garyschulte , I was able to fix the pipeline issue. I was using my work laptop which is Mac (m2) for testing and everything was working fine, but since the pipeline was failing in ubuntu x86_64, so I dual booted my PC with ubuntu and tested it in there. Seems there was an issue with LD_LIBRARY_PATH for which I added the required fix.

Now pipeline is passing. Please review and let me know if anything else is required from my side in this PR.

garyschulte commented 1 month ago

Merging as-is, with an expectation that linux-arm64 will be added in a follow-on PR, as the CHOOSENIM install method does not work on linux arm64, and ubuntu does not have nim support until 23.10:

choosenim-init: Error: Sorry, your platform (linux_arm64) is not supported by choosenim.
choosenim-init: Error: You will need to install Nim using an alternative method.
choosenim-init: Error: See the following link for more info: https://nim-lang.org/install.html