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.

Sign up for GitHub

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

Jump to bottom

Disable re-randomization under more conditions #474

Merged
merged 3 commits into from
Jul 19, 2022
Merged

Disable re-randomization under more conditions #474

merged 3 commits into from
Jul 19, 2022

Conversation

apoelstra
Copy link
Member

Fixes #470

apoelstra added 3 commits July 14, 2022 14:06
@apoelstra
apply global-context-not-secure logic to Secp256k1::new
7482846 
Disable auto-rerandomization for both global and local contexts.
@apoelstra
context: unconditionally disable auto-rerandomization on wasm
b01337c 
This causes panics. We can't add catch the panic, we can't change its output, we
can't detect if it'll happen, etc. Rather than dealing with confused bug reports
let's just drop this.

If users want to rerandomize their contexts they can do so manually.

There is probably a better solution to this but it is still under debate, even
upstream in the C library, what this should look like. Meanwhile we have bug
reports now.
@apoelstra
bump version to 0.23.4
d206891
@apoelstra
Copy link
Member Author

cc @Kixunil @elichai @real-or-random

Kixunil
Kixunil approved these changes Jul 14, 2022
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK d206891

tcharding
tcharding approved these changes Jul 14, 2022
View reviewed changes
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK d206891

@apoelstra
Copy link
Member Author

Thanks both of you! I need an ack from @TheBlueMatt or @elichai to merge.

@elichai
Copy link
Member

elichai commented Jul 16, 2022

Isn't this just "solved" by not enabling rand-std?

@apoelstra
Copy link
Member Author

@elichai rust-bitcoin enables rand-std, so if you use rust-bitcoin then this feature is on.

We could probably soften that, or at least make it optional, in rust-bitcoin ... but it will remain the case that if anything in your dep tree turns on rand-std there's no way to turn it off. (Also, rand-std provides useful functionality that wasm people may want.)

@apoelstra apoelstra mentioned this pull request Jul 19, 2022
Merged
@apoelstra
Copy link
Member Author

@sanket1729 I have made you a maintainer of this project. Could you ACK this so we can get this out before #475?

Unless @elichai has an objection.

@tcharding tcharding mentioned this pull request Jul 19, 2022
Closed
@tcharding
Copy link
Member

tcharding commented Jul 19, 2022 •
edited
Loading

Do we want the version bump to 0.23.4 if we are going to merge #475 straight away and bump to 0.24.0?

FTR I removed the version bump from #475 and did a tracking PR #476

sanket1729
sanket1729 approved these changes Jul 19, 2022
View reviewed changes
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK d206891

@sanket1729 sanket1729 merged commit 125211d into rust-bitcoin:master Jul 19, 2022
@apoelstra
Copy link
Member Author

@tcharding yes, we absolutely want this in a minor version bump.

@apoelstra apoelstra deleted the 2022-07--fix-470 branch July 19, 2022 12:43
@tcharding
Copy link
Member

Oh, I think I get it now. I'll write it just to see I'm understanding. It required a patch version because it was a bug fix to WASM, so without it 0.23.x is broken, hence doing 0.23.4 - is that correct?

@tcharding
Copy link
Member

tcharding commented Jul 19, 2022 •
edited
Loading

@sanket1729 just answered me over here: #475 (comment)

Thanks!

@apoelstra
Copy link
Member Author

Correct!

@notmandatory notmandatory mentioned this pull request Aug 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tcharding tcharding tcharding approved these changes

@sanket1729 sanket1729 sanket1729 approved these changes

@Kixunil Kixunil Kixunil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Problem at use secp256k1 0.22.1 with wasm
5 participants
@apoelstra @elichai @tcharding @Kixunil @sanket1729