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

Use LIB_PREFIX in Rust bindings #317

Merged
merged 10 commits into from
Jun 28, 2023
Merged

Conversation

michaelsproul
Copy link
Contributor

@michaelsproul michaelsproul commented Jun 9, 2023

Extension of #313 to the Rust bindings.

@ppopth
Copy link
Member

ppopth commented Jun 26, 2023

Is this still WIP?

Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I don't love the new macros, but it's probably the best option we have. This PR is a good tradeoff of usability & minor changes to the codebase post-audit. I made a commit that fixes clang-format's complaints and alters the comments a little to match the style of others. The CI check that is failing is a known problem which we can ignore for now. LGTM 👍

@michaelsproul
Copy link
Contributor Author

Still WIP on the Lighthouse integration unfortunately. We've hit a few linking issues that look like they might require further changes

@michaelsproul
Copy link
Contributor Author

Ok, we've got it working reliably in Lighthouse now. I've pushed two commits here that seemed to help:

  1. 5f3a7c1 moves the bindgen-generated Rust code to the Cargo build directory, rather than the source directory. This is the standard practice for bindgen and helps prevent strange caching behaviour when the c-kzg crate is compiled with different mainnet/minimal specs.
  2. 6449e2c removes all unprefixed symbols from the generated object files. We were getting some link errors about symbols like blob_verify_kzg_proof being duplicated. These errors seemed to only occur occasionally, and only on Linux, and could actually have been a manifestation of (1). If you prefer not to make this change I can try reverting it and testing with (1) alone.

@divagant-martian
Copy link
Contributor

I added the generated bindings to the source to be able to see them. So I don't think anyone else has an attachment to that setup. I'm fine with moving them to the build dir

@jtraglia
Copy link
Member

jtraglia commented Jun 28, 2023

Hey @michaelsproul I made a PR to your branch (michaelsproul#1) with some changes I'd like to make. I think this will simplify things a bit, but there were a lot of changes so I didn't feel comfortable committing them directly. Thoughts?

Also, tag @asn-d6 since this is based on your original PR.

@jtraglia
Copy link
Member

Going to merge. I believe it's ready 👍

@jtraglia jtraglia merged commit 13cec82 into ethereum:main Jun 28, 2023
28 of 30 checks passed
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.

None yet

5 participants