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

Bump C_KZG_4844_GIT_HASH #257

Merged
merged 11 commits into from
Jan 26, 2024

Conversation

ArtiomTr
Copy link
Contributor

@ArtiomTr ArtiomTr commented Jan 25, 2024

In this PR:

  • Update C_KZG_4844_GIT_HASH to latest c-kzg-4844 commit (5115420)
  • Added support for nim bindings

@sauliusgrigaitis
Copy link
Member

Ok, so this is a funny - there is some issue when constantine in rust-kzg is called using Nim bindings. I think it's very rare use case, especially when rust-kzg has performance penalty for constantine. @mratsim let us know if you see a quick fix for this. If not, then we will just not support this case and will refer Nim users to use constantine directly.

@mratsim
Copy link

mratsim commented Jan 26, 2024

It's multiple definitions of some Nim primitives.

![Screenshot_20240126-093536743.png](https://github.![Screenshot_20240126-093608207.png](https://github.com/sifraitech/rust-kzg/assets/22738317/659000b5-72c1-4d4a-a101-4005ae6a5357)

b-4c3e-bc22-c691f8a29fed)

Uploading Screenshot_20240126-093608207.png …

There is a workaround iirc but have to check and try myself (on mobile)

@mratsim
Copy link

mratsim commented Jan 26, 2024

To ensure there is no naming conflict, Nim core functions can be exported to a RTL (Runtime Library) file: https://nim-lang.org/nimc.html#dll-generation

I think Constantine itself might be RTL-free as I purposely avoided all dependencies on Nim stdlib but I might have missed something or use convenient types like seq for high-level non-hotpath non-cryptographic-secret code.

In the meantime I think it's reasonable to point to Constantine directly.

@sauliusgrigaitis
Copy link
Member

Thanks for information @mratsim . Let's close it for now and reopen of this if this rare use-case will be hit.

@sauliusgrigaitis sauliusgrigaitis merged commit 33989b7 into grandinetech:main Jan 26, 2024
24 checks passed
@ArtiomTr ArtiomTr deleted the Bump-c-kzg-4844-commit branch September 10, 2024 12:59
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