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

Expose SHA-256 functions #146

Closed
wants to merge 3 commits into from

Conversation

jtraglia
Copy link
Contributor

@jtraglia jtraglia commented Jan 13, 2023

This PR exposes the blst sha256 functions in the auxiliary header. This is for c-kzg-4844 which currently patches blst to expose these functions. It would beneficial to us if these functions were exported. This PR only makes these functions available from C and not any of the other bindings. This is all we want/need at the moment.

@henridf
Copy link

henridf commented Jan 20, 2023

A couple other repos that would benefit from this change are nim-blscurve (current workaround to expose sha256 at https://github.com/status-im/nim-blscurve/blob/master/blscurve/blst/blst_sha256.c) and the in-progress nim-kzg-4844 repo.

@dot-asm
Copy link
Collaborator

dot-asm commented Jan 20, 2023

I don't see a compelling need for exposing the CTX structure. I mean why not instead of exposing init/update/final expose single-shot subroutine? One that simply takes data and emits its hash... With private CTX allocated on stack...

@dot-asm
Copy link
Collaborator

dot-asm commented Jan 20, 2023

I mean

void blst_sha256(unsigned char md[32], const void *msg, size_t len)
{
    SHA256_CTX ctx;

    sha256_init(&ctx);
    sha256_update(&ctx, msg, len);
    sha256_final(md, &ctx);
}

for src/exports.c.

@jtraglia
Copy link
Contributor Author

That's a good idea too. I like that approach. Will make the change.

/*
* SHA-256 hash function.
*/
void blst_sha256(unsigned char md[32], const void *msg, size_t len);
Copy link
Collaborator

@dot-asm dot-asm Jan 20, 2023

Choose a reason for hiding this comment

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

Not that it actually matters in the context, but blst.h follows a specific convention for naming arguments that reflects so called typemaps in blst.swg. For this reason I'll change this line as I commit.

BTW, how would byte *msg look at your side? I mean void* is a convenient way to avoid specific type warnings [or rather excessive casts], but at the same type time it masks potentially meaningful programming mistakes. For example passing pointer to an integer might work in specific context, but formally speaking it's not exactly legit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A byte *msg would be fine on our end. Our input is uint8_t* (byte*) so it works well.

@dot-asm
Copy link
Collaborator

dot-asm commented Jan 20, 2023

Any hard objections, @henridf?

@henridf
Copy link

henridf commented Jan 20, 2023

nope, I think this should work fine on my end too

@dot-asm dot-asm closed this in aa3cdfe Jan 24, 2023
@dot-asm
Copy link
Collaborator

dot-asm commented Jan 24, 2023

Thanks!

@jtraglia jtraglia deleted the expose-sha256-funcs branch January 24, 2023 16:51
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

3 participants