ethereum / c-kzg-4844

A minimal implementation of the Polynomial Commitments API for EIP-4844 and EIP-7594, written in C.
Apache License 2.0
112 stars 105 forks source link

Simplify nim bindings #482

Closed jtraglia closed 1 month ago

jtraglia commented 1 month ago

I would like to simplify the Nim bindings before the big v2.0 release. This PR does the following:

Some thoughts on this from our discussion on discord:

image

Opening as a draft until @agnxsh tests it.

I would also appreciate a review from @jangko, the author of these bindings.

jtraglia commented 1 month ago

@jangko Oh cool. Yes let's do that in another PR.

Any thoughts on the optimization issue on Windows? I've disabled compiler optimizations as a bandaid fix until we figure it out. I asked about it on Discord (in Nimbus's PeerDAS channel):

Also, I think I've run into a Nim compiler bug. For some reason, the Windows release build will crash when calling computeCellsAndKzgProofs. The debug build works and the release build with --opt:none works too. I spent a couple hours trying to figure it out. No issues on Linux either. Is there a Nim expert that could look into this?

To be clear, this happens when the high level wrapper function (computeCellsAndKzgProofs), not the ABI function, is called & it is not crashing due to anything the function is doing. I put an echo before the function was called and another at the top of the function. Only the first one printed.

jtraglia commented 1 month ago

Thank you @jangko for identifying the issue:

Maybe related to completeStruct pragma being absent from KzgCell? Because I see other types/struct are using completeStruct. Not completely sure though.

After adding this to the type, the Windows CI tests pass again.

jtraglia commented 1 month ago

This branch was tested by Agnish (nimbus dev) and confirmed it compiles & passes the reference tests. Going to merge.