-
Notifications
You must be signed in to change notification settings - Fork 36
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
Constantine bindings for EIP196 #184
Conversation
Signed-off-by: Nischal Sharma <[email protected]>
👀 just a tracking comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There shouldn't be a need for indirection and allocation, the function can be called and write directly in the relevant buffer, see C, Rust and Go code as referemce.
Also is optimizing for ARM desirable?
On non-x86 Constantine compiles through uint128 as it's the most portable and best perf across all compilers. Both GCC and Clang support an add with overflow
called __builtin__addcll
but the perf is abysmal on GCC: https://gcc.godbolt.org/z/jdecvffaP see overall issue mratsim/constantine#357
I do have a plan for optimizing for ARM targets (see mratsim/constantine#200 ) but so far no time to tackle it.
cc @Vindaar as well as he is currently maintaining the external languages dependencies.
var inputSeq: seq[byte] = cast[seq[byte]](inputs[0 ..< inputsLen]) | ||
let status = ethereum_evm_precompiles.eth_evm_bn254_g1add(result, inputSeq) | ||
copyMem(r, addr result[0], rLen) | ||
return status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There shouldn't be a need for this indirection and allocation.
If from Nim (ptr, length) pair can be zero-copied transformed to openarrays with https://nim-lang.org/docs/system.html#toOpenArray%2Cptr.UncheckedArray%5BT%5D%2Cint%2Cint
If from C, Go, Rust, you can directly call eth_evm_bn254_g1add
because it's compiled to (ptr, len):
/**
* Elliptic Curve addition on BN254_Snarks
* (also called alt_bn128 in Ethereum specs
* and bn256 in Ethereum tests)
*
* Name: ECADD
*
* Inputs:
* - A G1 point P with coordinates (Px, Py)
* - A G1 point Q with coordinates (Qx, Qy)
*
* Each coordinate is a 32-byte bigEndian integer
* They are serialized concatenated in a byte array [Px, Py, Qx, Qy]
* If the length is less than 128 bytes, input is virtually padded with zeros.
* If the length is greater than 128 bytes, input is truncated to 128 bytes.
*
* Output
* - Output buffer MUST be of length 64 bytes
* - A G1 point R = P+Q with coordinates (Rx, Ry)
* - Status code:
* cttEVM_Success
* cttEVM_InvalidOutputSize
* cttEVM_IntLargerThanModulus
* cttEVM_PointNotOnCurve
*
* Spec https://eips.ethereum.org/EIPS/eip-196
*/
ctt_evm_status ctt_eth_evm_bn254_g1add(
byte* r, ptrdiff_t r_len,
const byte* inputs, ptrdiff_t inputs_len
) __attribute__((warn_unused_result));
See how to call SHA256 from C (same interface):
https://github.com/mratsim/constantine/blob/8db2639/examples-c/ethereum_evm_precompiles.c#L47-L51
byte result[32] = {0};
const char txt[] = "Foo, Bar and Baz are all friends.";
evm_status = ctt_eth_evm_sha256(result, 32, (const byte*)txt, sizeof(txt));
Rust slices to Nim with zero-copy or alloc:
https://github.com/mratsim/constantine/blob/8db2639/constantine-rust/constantine-ethereum-evm-precompiles/src/lib.rs#L53-L69
pub fn evm_bn254_g1add(
result: &mut [u8],
inputs: &[u8]
) -> Result<bool, ctt_evm_status> {
unsafe {
let status = ctt_eth_evm_bn254_g1add(
result.as_mut_ptr() as *mut byte,
result.len() as isize,
inputs.as_ptr() as *const byte,
inputs.len() as isize,
);
match status {
ctt_evm_status::cttEVM_Success => Ok(true),
_ => Err(status)
}
}
}
Go slices to Nim with zero-copy or alloc:
https://github.com/mratsim/constantine/blob/8db2639/constantine-go/constantine.go#L705-L718
func EvmBn254G1Mul(result []byte, inputs []byte) (bool, error) {
status := C.ctt_eth_evm_bn254_g1mul((*C.byte)(getAddr(result)),
(C.ptrdiff_t)(len(result)),
(*C.byte)(getAddr(inputs)),
(C.ptrdiff_t)(len(inputs)),
)
if status != C.cttEVM_Success {
err := errors.New(
C.GoString(C.ctt_evm_status_to_string(status)),
)
return false, err
}
return true, nil
}
One thing is that in an excess zeal moment I set the type to ptrdiff_t
but I'll change it to size_t
.
In general Constantine was written to minimize allocations and for precompiles it's only needed for MSMs and KZG point precompiles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mratsim Thanks for the inputs, I have been working on a new approach based on your inputs, taking inspiration from example c code.
ethereum_evm_precompiles.nim -> compile into shared library -> Create C Bindings: ethereum_evm_precompiles.c -> Define Java Native Methods -> Compile and Run
Signed-off-by: Nischal Sharma <[email protected]>
Signed-off-by: Nischal Sharma <[email protected]>
@garyschulte @mratsim Pushed some new commits, I shall be removing the files inside constantine/constantine-jni/lib/ folder Thanks |
constantine/constantine-jni/src/main/c/ethereum_evm_precompiles.c
Outdated
Show resolved
Hide resolved
|
||
JNIEXPORT jint JNICALL Java_Constantine_ctt_1eth_1evm_1bn254_1g1add(JNIEnv *env, jobject obj, jbyteArray jr, jint r_len, jbyteArray jinputs, jint inputs_len) { | ||
jbyte *r = (*env)->GetByteArrayElements(env, jr, NULL); | ||
jbyte *inputs = (*env)->GetByteArrayElements(env, jinputs, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I'm not familiar with Java at all but it seems like what is also being done in c-kzg-4844 so it's likely the expected way to wrap:
jbyte *r = (*env)->GetByteArrayElements(env, jr, NULL); | ||
jbyte *inputs = (*env)->GetByteArrayElements(env, jinputs, NULL); | ||
|
||
ctt_evm_status status = ctt_eth_evm_bn254_g1mul((byte *)r, (ptrdiff_t)r_len, (const byte *)inputs, (ptrdiff_t)inputs_len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very tidy that constantine already has precompile friendly endpoints 👍 . Is this the same for eip2537?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and SHA256 and MODEXP. The plan is to implement all cryptographic precompiles.
For the record, Constantine was actually started with the idea of having a clean from scratch BN254 backend for Nim but it was too hard/time-consuming at the time so a 1-1 port of https://github.com/zcash-hackworks/bn was made instead (https://github.com/status-im/nim-bncurve).
However performance is bad as even a simple add with carry instruction is transformed into 7 instructions: https://github.com/zcash-hackworks/bn/blob/master/src/arith.rs#L396-L405
The issue of zcash's bn
were already mentioned in 2008 in EIP-1108 https://eips.ethereum.org/EIPS/eip-1108 that reduced gas cost price. And moving to Cloudflare bn256 improved speed by 10x:
and corroborated in a later note, the computational cost of ECADD, ECMUL, and pairing checks (excepting the constant) has dropped roughly an order of magnitude across the board.
However that Cloudflare library was about 2x slower than state-of-the-art in 2021 https://hackmd.io/@gnark/eccbench
https://user-images.githubusercontent.com/22738317/162632923-b057c0a2-6929-419e-b0de-4fd187a5e508.png
and Constantine there had a parameter passing bug which made it 2x slower than necessary on pairings (nim-lang/Nim#16897)
See full context in https://ethresear.ch/t/releasing-constantine-v0-1-0-a-modular-cryptography-stack-for-ethereum/19990
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
constantine/build.gradle
Outdated
task compileJavaSource(type: Exec) { | ||
description = 'Compiles the Java source files' | ||
commandLine 'javac', 'src/main/java/org/hyperledger/besu/nativelib/constantine/LibConstantineEIP196.java' | ||
} | ||
|
||
task generateJNIHeader(type: Exec) { | ||
description = 'Generates the JNI header file' | ||
commandLine 'javac', '-h', 'constantine/constantine-jni/include', 'src/main/java/org/hyperledger/besu/nativelib/constantine/LibConstantineEIP196.java' | ||
} | ||
|
||
task compileNativeLibrary(type: Exec) { | ||
description = 'Compiles the native library' | ||
commandLine 'clang', '-I', "${System.env.JAVA_HOME}/include", '-I', "${System.env.JAVA_HOME}/include/darwin", '-shared', '-o', 'constantine-jni/lib/libconstantine.jnilib', 'constantine-jni/src/main/c/ethereum_evm_precompiles.c', '-I', 'constantine/include', '-I', 'src/main/java/org/hyperledger/besu/nativelib/constantine', '-L', 'constantine-jni/lib', '-lconstantine' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idiomatic besu-native builds incorporate the build of the library into the root build.sh
script and use automake or a similar tool to ensure that the appropriate toolchain is configured (cc, gcc, clang, etc).
I think it is reasonable to use gradle for these tasks, but we should expect to be able to build on a variety of platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, adding a dependsOn
chain between these dependent tasks will make for an easier build ux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Fixing it right now |
Signed-off-by: Nischal Sharma <[email protected]>
This and adding tests from Gnark and matter-labs is remaining, working on them. |
Signed-off-by: Nischal Sharma <[email protected]>
Added test cases from Gnark Lib |
Signed-off-by: Nischal Sharma <[email protected]>
Signed-off-by: Nischal Sharma <[email protected]>
Signed-off-by: Nischal Sharma <[email protected]>
Signed-off-by: Nischal Sharma <[email protected]>
Signed-off-by: Nischal Sharma <[email protected]>
Signed-off-by: Nischal Sharma <[email protected]>
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% 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 |
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). |
@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. |
Sorry - somehow I edited your comment rather than replying. |
@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 |
Signed-off-by: Nischal Sharma <[email protected]>
Signed-off-by: Nischal Sharma <[email protected]>
Signed-off-by: Nischal Sharma <[email protected]>
Signed-off-by: Nischal Sharma <[email protected]>
Signed-off-by: Nischal Sharma <[email protected]>
Signed-off-by: Nischal Sharma <[email protected]>
Signed-off-by: Nischal Sharma <[email protected]>
Signed-off-by: Nischal Sharma <[email protected]>
@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? |
Signed-off-by: Nischal Sharma <[email protected]>
The problem is the way CI expects to build these libraries. It expects to run 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 Since CI is expecting to build across architectures, we should probably put a constantine build step into build.sh like:
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:
and in the final-assembly task, pull all of the artifacts down by adding a step like:
|
Yes, I followed the exact same approach in my last commit - 7c9f645 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 |
Signed-off-by: Nischal Sharma <[email protected]>
Hey @garyschulte , I was able to fix the pipeline issue. Now pipeline is passing. Please review and let me know if anything else is required from my side in this PR. |
# Skip if OSARCH is linux-gnu-aarch64 | ||
if [[ "$OSARCH" == "linux-gnu-aarch64" ]]; then | ||
echo "Skipping build for OSARCH: ${OSARCH}" | ||
return | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a CI-specific workaround? it looks like an arm64 linux host env would not be able to build constantine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I skipped Linux arm64 build since nim installer was not compatible with it, and I didn't have an Linux arm environment to test it.
Will test and add in the follow on PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM 👍
One non-blocking question about linux arm64 builds though.
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:
|
Fixes
hyperledger/besu#7242
#195
hyperledger/besu#7243