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

Feature handing changes #438

Closed
josephlr opened this issue May 29, 2024 · 2 comments
Closed

Feature handing changes #438

josephlr opened this issue May 29, 2024 · 2 comments
Milestone

Comments

@josephlr
Copy link
Member

Splitting off from #433 to discuss any breaking feature flag changes we might want to make to the crate. CC @briansmith @newpavlov @dhardy @vks Let me know what you think or if I missed anything.

rdrand feature

I would propose fixing #230 by simply having the custom feature take precedence over the rdrand feature. This would allow users who are fine with the CSPRNG of rdrand to simply specify the rdrand feature, while users who want more complexity can use the custom feature to provide their own implementation. If necessary, we can also have an x86-only fn rdrand_fill(&mut [u8]) -> Result<(), Error> or fn rdrand() -> Result<[u8; 8], Error> helper function to make such implementations easier.

For SGX targets we currently unconditionally use the rdrand implementation. This is probably not ideal, given that target_env = "sgx" may not mean the same thing across different targets. I would make the following changes:

  • Only use the rdrand implemenation by default on x86_64-fortanix-unknown-sgx. Other unsupported targets will need to explicitly set rdrand.
  • Even on x86_64-fortanix-unknown-sgx, have custom take precedence over rdrand, ensuring that our rdrand implementation always interacts with custom in a consistent way.
  • Effectively, this just makes the rdrand feature on by default for x86_64-fortanix-unknown-sgx which seems ideal.

js feature and wasm32-unknown-unknown

Ideally, we would have the custom feature take precedence over the JS implementation (#346), so even if the js feature is unconditionally enabled in a user's dependency graph (which is fairly common), a user in a non-web/node environment can provide their own implementation.

If we did this, my initial thought was that we might be able to get rid of the js feature entirely, and have the "JS" implementation be the default for wasm32-unknown-unknown while still allowing users to select their own implementation if they are using wasm32-unknown-unknown not in a JS environment (uncommon but possible). However, given some of the issues observed by the ahash crate (tkaitchuck/aHash#140, tkaitchuck/aHash#143, tkaitchuck/aHash#145), I'm not sure we can do this.

@josephlr josephlr added this to the Next Release milestone May 29, 2024
@briansmith
Copy link
Contributor

I would propose fixing #230 by

I think we should discuss how to resolve #230 in #230.

Similarly, isn't this issue exactly duplicating #346, which is also partially duplicating #230.

If we want to change how wasm32 targets work currently, then we should do that in a separate issue. (IMO the current implementation for wasm32 is as good as we can make it.)

@josephlr
Copy link
Member Author

josephlr commented Jun 1, 2024

All good points @briansmith closing in favor of #230 and #346

@josephlr josephlr closed this as completed Jun 1, 2024
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

2 participants