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 a global-context-less-secure feature which skips randomization #279

Merged
merged 1 commit into from
Jun 8, 2021

Conversation

TheBlueMatt
Copy link
Member

This is useful for us downstream as we wish to target WASM with a
global context, and using rand in such a build doesn't seem like a
safe idea.

@real-or-random
Copy link
Collaborator

I guess that's okay but can you explain why using rand isn't a safe idea?

@TheBlueMatt
Copy link
Member Author

TheBlueMatt commented Feb 15, 2021 via email

@elichai
Copy link
Member

elichai commented Feb 15, 2021

@TheBlueMatt which wasm target have std and doesn't have rand support?

@elichai
Copy link
Member

elichai commented Feb 15, 2021

Also, see bitcoin-core/secp256k1#893 for fully non std global context

@TheBlueMatt
Copy link
Member Author

Currently we're building rust-lightning targeting wasm32-wasi, but linking it with C code and not exposing any of the wasi runtime to the wasm binary.

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Feb 19, 2021
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Feb 19, 2021
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Feb 19, 2021
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Feb 19, 2021
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Feb 19, 2021
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Feb 19, 2021
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Feb 19, 2021
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Feb 19, 2021
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Feb 19, 2021
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Feb 22, 2021
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Feb 24, 2021
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Feb 26, 2021
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Feb 26, 2021
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Feb 27, 2021
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Mar 1, 2021
@TheBlueMatt
Copy link
Member Author

Rebased to pick up new upstream CI with no other changes.

This is useful for us downstream as we wish to target WASM with a
global context, and using rand in such a build doesn't seem like a
safe idea.
@TheBlueMatt
Copy link
Member Author

Rebased on latest upstream to fix our Cargo patches. Any update on getting this landed?

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ack ce930ab

I'm not totally thrilled about the "less secure" name but I'm not sure what would be better.

@apoelstra apoelstra merged commit b686de7 into rust-bitcoin:master Jun 8, 2021
@Kixunil
Copy link
Collaborator

Kixunil commented Jul 25, 2024

FYI @TheBlueMatt getrandom now has js feature which you can turn on to get WASM with JS working correctly.

@apoelstra
Copy link
Member

Lol, wow, when I saw your "why is this a feature?" issue I couldn't remember the reason, but I am a little surprised it was "WASM compatibility".

We can drop the features now; we haven't had any issues with randomness in particular for a few years in wasm. Other than remembering to turn on that feature. (But then I think the correct solution is to detect at compile-time if we are targeting wasm and the feature isn't turned on.)

@Kixunil
Copy link
Collaborator

Kixunil commented Jul 25, 2024

But then I think the correct solution is to detect at compile-time if we are targeting wasm and the feature isn't turned on.

Their code already has compile_error but while targeting wasm is possible to detect, it's not possible to detect whether it supports JavaScript. From their doc:

This crate fully supports the wasm32-wasi and wasm32-unknown-emscripten targets. However, the wasm32-unknown-unknown target (i.e. the target used by wasm-pack) is not automatically supported since, from the target name alone, we cannot deduce which JavaScript interface is in use (or if JavaScript is available at all).

Instead, if the js Cargo feature is enabled, this crate will assume that you are building for an environment containing JavaScript, and will call the appropriate methods. Both web browser (main window and Web Workers) and Node.js environments are supported, invoking the methods described above using the wasm-bindgen toolchain.

@apoelstra
Copy link
Member

Oh, wonderful. I'm glad they have a compile error now. At the time of this issue in was a runtime panic in a very obscure location, which as I mentioned elsewhere, would present as a blank webpage.

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.

5 participants