Skip to content

Conversation

@jtraglia
Copy link
Member

@jtraglia jtraglia commented Apr 25, 2023

This PR changes how blst is built and forces it to always use the portable version. Because we use the supranational/blst Go package, which is compiled before the C-KZG Go bindings, we are unable to force it to be portable from within the bindings. Right now, the only way to do this is with an environment variable (CGO_CFLAGS).

  • Build blst at the same time the C-KZG Go bindings are built.
  • Remove supranational/blst as an import/dependency.
    • We're using the local copy now.
  • Add blst.S file which includes assembly.S from blst so that it's assembled via CGO.
    • I thought this was better than copying the whole file. Less duplication.
    • For CGO to assemble it, it MUST be in the same directory as main.go.
  • Create a new header.h file that defines types & everything.
    • Could do in the main.go file but this is cleaner IMO.
  • Do not include blst.h in c_kzg_4844.h if it's the Go bindings.
    • Because we compile server.c, this would cause problems.
  • Remove blst_headers since we don't need them anymore.

Additionally, we would need to convert the blst submodule to a subtree, meaning that all of blst source files will become tracked files in this repo. Not really a fan of this idea, but it would be necessary. When the ethereum/c-kzg-4844 package is imported with go get it does not do a recursive clone, so none of the blst source files would be available. If we decide to go down this route, we should do this in a separate PR.

@jtraglia jtraglia changed the title Add hack to force portable go bindings Force go bindings to use portable blst Apr 25, 2023
@ppopth
Copy link
Member

ppopth commented Apr 26, 2023

Isn't the portable version of blst slower than the normal one? Are we okay with that?

@jtraglia
Copy link
Member Author

Isn't the portable version of blst slower than the normal one?

For systems that support optimized instructions, yes.

Are we okay with that?

I'm not sure. Something we should all discuss. From a reputation perspective, it may be better to have a slower library that supports everything than a faster library that doesn't support older CPUs and causes headaches for client devs.

@ppopth
Copy link
Member

ppopth commented Apr 26, 2023

For documentation purpose, we compile server.c and assemble assembly.S as suggested in https://github.com/supranational/blst/blob/master/bindings/go/README.md#build

@ppopth
Copy link
Member

ppopth commented Apr 26, 2023

For clarification, is blst_headers not needed even when we don't need to do the portable blst thing? I tried removing it from the main branch and it worked fine.

#include <stdint.h>
#include <stdio.h>

/*
Copy link
Member

Choose a reason for hiding this comment

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

Can you mark it as a TODO so that we don't forget it later?

Copy link
Member

@ppopth ppopth left a comment

Choose a reason for hiding this comment

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

LGTM after all. Mark it as approved for now just in case you want to merge it ASAP.

@jtraglia
Copy link
Member Author

jtraglia commented Apr 26, 2023

For clarification, is blst_headers not needed even when we don't need to do the portable blst thing? I tried removing it from the main branch and it worked fine.

Unfortunately, it is required in the main branch. This is because go get does not do a recursive clone when installing the package. The Go bindings (which builds c-kzg-4844) need the blst headers to exist (because it includes blst.h) in the repo & not a submodule. The CI tests are a bit deceptive because they do a recursive checkout first. I removed them here because we will need to copy blst source files into the repo, and at that point it

@jtraglia
Copy link
Member Author

Thanks for the review @ppopth 😄 I'm going to mark this is a draft so it doesn't get merged. This is only a potential solution and has drawbacks. I'd like to discuss it with the team for a while before merging. Also, it will require another PR that converts blst to a subtree (or something like that).

@jtraglia jtraglia marked this pull request as draft April 26, 2023 12:04
@jtraglia
Copy link
Member Author

jtraglia commented May 9, 2023

Closing. I don't think this is necessary anymore.

@jtraglia jtraglia closed this May 9, 2023
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.

2 participants