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

Update secp256k1-zkp #2261

Merged
merged 4 commits into from
May 16, 2022
Merged

Update secp256k1-zkp #2261

merged 4 commits into from
May 16, 2022

Conversation

onvej-sl
Copy link
Contributor

TLDR

This PR updates the secp256k1-zkp submodule and fixes #2085.

Details

@onvej-sl onvej-sl removed the request for review from prusnak May 10, 2022 11:41
Copy link
Member

@prusnak prusnak left a comment

Choose a reason for hiding this comment

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

untested ACK

@matejcik
Copy link
Contributor

  • The option USE_ECMULT_STATIC_PRECOMPUTATION was removed

does this mean that the binary size is ~same as before?
i.e., did we have the precomputed tables before too, except now they are separate obj files?

@onvej-sl
Copy link
Contributor Author

does this mean that the binary size is ~same as before?

I suppose so but I didn't test it.

i.e., did we have the precomputed tables before too, except now they are separate obj files?

Yes, this is exactly what happened.

@onvej-sl
Copy link
Contributor Author

I suppose so but I didn't test it.

This PR doesn't seem to change code_length in TT firmware header, which is a bit suspicious.

Copy link
Contributor

@andrewkozlik andrewkozlik left a comment

Choose a reason for hiding this comment

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

LGTM.

Just two details:

  1. We can probably remove the reference to gen_context.o from core/embed/rust/build.rs.
  2. See below.

crypto/Makefile Outdated Show resolved Hide resolved
@onvej-sl
Copy link
Contributor Author

We can probably remove the reference to gen_context.o from core/embed/rust/build.rs.

Yes, that is true. Done in d24d4a4.

@onvej-sl onvej-sl requested a review from andrewkozlik May 13, 2022 12:19
@onvej-sl onvej-sl force-pushed the onvej-sl/update-secp256k1-zkp branch from d24d4a4 to f642c1e Compare May 13, 2022 16:31
@onvej-sl onvej-sl force-pushed the onvej-sl/update-secp256k1-zkp branch from 37d99c4 to 1624d7c Compare May 16, 2022 13:59
@onvej-sl onvej-sl merged commit bdfc453 into master May 16, 2022
@onvej-sl onvej-sl deleted the onvej-sl/update-secp256k1-zkp branch May 16, 2022 14:23
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.

Trezor's RFC6979 operation diverges from that of Bitcoin core
4 participants