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

Add library prefix to all ckzg symbols #312

Closed
michaelsproul opened this issue Jun 1, 2023 · 7 comments
Closed

Add library prefix to all ckzg symbols #312

michaelsproul opened this issue Jun 1, 2023 · 7 comments

Comments

@michaelsproul
Copy link
Contributor

Currently the c-kzg library uses #define constants for the mainnet/minimal paramaters like FIELD_ELEMENTS_PER_BLOB:

/*
* This value represents the number of field elements in a blob. It must be
* supplied externally. It is expected to be 4096 for mainnet and 4 for minimal.
*/
#ifndef FIELD_ELEMENTS_PER_BLOB

This makes it very hard (but not impossible*) to link two copies of the library to enable switching between mainnet and minimal configurations at runtime.

*My attempt at doing this is documented here: sigp/lighthouse#4276. I had to use objcopy to rename c-kzg's symbols so that they wouldn't conflict. Needless to say this is not a sensible approach to take.

To do this better I was thinking the library could accept another #define constant which it prepends to all symbol names. Then the downstream consumer of the library could easily avoid collisions, with names like ckzg_blob_to_kzg_commitment and ckzgmin_blob_to_kzg_commitment instead of just blob_to_kzg_commitment. The ckzg prefix could be the default if no prefix is specified.

@asn-d6
Copy link
Contributor

asn-d6 commented Jun 1, 2023

Hello, that seems like a promising approach to solving the problem, and requires less tinkering of the audited code than making FIELD_ELEMENTS_PER_BLOB configurable. I can look next week at how to achieve this (since honestly I have no idea). Thanks for the suggestion.

@divagant-martian
Copy link
Contributor

digging this approach 🎉

@asn-d6
Copy link
Contributor

asn-d6 commented Jun 6, 2023

So, I pushed #313 which includes some cursed macro code 2c58b2a that gets used around the codebase 6253751 .

To use this cursed feature you need to define a LIB_PREFIX as follows: LIB_PREFIX=ckzg

Would this work for you guys?

Either way, I think we should probably wait until Justin is back from OOO to see if this is acceptable in terms of code quality. I don't really like it, but if it's a needed feature I can probably turn a blind eye.

@michaelsproul
Copy link
Contributor Author

@asn-d6 That looks like it would work for us, and I don't think the macros are too bad from an aesthetics/maintainability PoV. If I get some time I'll try to re-work the Rust bindings and confirm that it works end-to-end.

Thanks for your work on that!

@ppopth
Copy link
Member

ppopth commented Jun 9, 2023

Either way, I think we should probably wait until Justin is back from OOO to see if this is acceptable in terms of code quality. I don't really like it, but if it's a needed feature I can probably turn a blind eye.

Yeah, let's wait for his comment, but I think the PR already looks good.

@jtraglia
Copy link
Member

Either way, I think we should probably wait until Justin is back from OOO to see if this is acceptable in terms of code quality. I don't really like it, but if it's a needed feature I can probably turn a blind eye.

Yeah, let's wait for his comment, but I think the PR already looks good.

Reviewed the PR yesterday. I'm not ecstatic about it either, but I think it's a clever solution with minimal changes. I want to look into how complicated it would be to do it "properly". Maybe we do it this way now & the complicated way later.

@jtraglia
Copy link
Member

Fixed in #317. Closing.

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

No branches or pull requests

5 participants