Skip to content

Disable re-randomization under more conditions #474

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
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

@apoelstra apoelstra commented Jul 14, 2022

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

apoelstra commented Jul 14, 2022

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

apoelstra commented Jul 14, 2022

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

apoelstra commented Jul 17, 2022

@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

apoelstra commented Jul 19, 2022

@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

apoelstra commented Jul 19, 2022

@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

tcharding commented Jul 19, 2022

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

apoelstra commented Jul 19, 2022

Correct!

@notmandatory notmandatory mentioned this pull request Aug 26, 2022
Closed
chain-forgexcr45 added a commit to chain-forgexcr45/rust-secp256k1 that referenced this pull request Sep 28, 2025
@chain-forgexcr45
Merge rust-bitcoin/rust-secp256k1#474: Disable re-randomization under…
2a57257 
… more conditions

d206891eaaaee32e1b982f6d1439a3e19d0ebfe8 bump version to 0.23.4 (Andrew Poelstra)
b01337cfb5ca87243d0fcee573480f3ab5747ffd context: unconditionally disable auto-rerandomization on wasm (Andrew Poelstra)
748284633bbabca8ff1f17fe7ed260e3cee5c480 apply `global-context-not-secure` logic to Secp256k1::new (Andrew Poelstra)

Pull request description:

  Fixes #470

ACKs for top commit:
  Kixunil:
    ACK d206891eaaaee32e1b982f6d1439a3e19d0ebfe8
  tcharding:
    ACK d206891eaaaee32e1b982f6d1439a3e19d0ebfe8
  sanket1729:
    ACK d206891eaaaee32e1b982f6d1439a3e19d0ebfe8

Tree-SHA512: 2a7db5b75f55a007aa780b6317804c819c0366e207623220f72a06c2af09087accf1bc834f05899897afcc2035f5e9a5480d8a7ffff83536327c695602ba138d
william2332-limf added a commit to william2332-limf/rust-secp256k1 that referenced this pull request Oct 2, 2025
@william2332-limf
Merge rust-bitcoin/rust-secp256k1#474: Disable re-randomization under…
3e3ffb6 
… more conditions

d206891eaaaee32e1b982f6d1439a3e19d0ebfe8 bump version to 0.23.4 (Andrew Poelstra)
b01337cfb5ca87243d0fcee573480f3ab5747ffd context: unconditionally disable auto-rerandomization on wasm (Andrew Poelstra)
748284633bbabca8ff1f17fe7ed260e3cee5c480 apply `global-context-not-secure` logic to Secp256k1::new (Andrew Poelstra)

Pull request description:

  Fixes #470

ACKs for top commit:
  Kixunil:
    ACK d206891eaaaee32e1b982f6d1439a3e19d0ebfe8
  tcharding:
    ACK d206891eaaaee32e1b982f6d1439a3e19d0ebfe8
  sanket1729:
    ACK d206891eaaaee32e1b982f6d1439a3e19d0ebfe8

Tree-SHA512: 2a7db5b75f55a007aa780b6317804c819c0366e207623220f72a06c2af09087accf1bc834f05899897afcc2035f5e9a5480d8a7ffff83536327c695602ba138d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kixunil Kixunil Kixunil approved these changes

@sanket1729 sanket1729 sanket1729 approved these changes

@tcharding tcharding tcharding 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