Skip to content

Conversation

@gbotrel
Copy link
Collaborator

@gbotrel gbotrel commented Dec 16, 2022

See #276

@gbotrel gbotrel requested a review from Tabaie December 16, 2022 21:29
@gbotrel gbotrel linked an issue Dec 16, 2022 that may be closed by this pull request
@gbotrel gbotrel requested a review from yelhousni December 16, 2022 21:44
func TestG1IsogenyVectors(t *testing.T) {
t.Parallel()

// TODO @gbotrel fix me test vectors shouldn't set words directly
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, if these come from the standard docs they should be available in decimal or hex form.

@samlaf
Copy link

samlaf commented Oct 2, 2023

Trying to upgrade our version of gnark-crypto from 0.8 to 0.9.
We were using

config := ecc.MultiExpConfig{ScalarsMont: true}

whereas now MultiExpConfig does not have a ScalarsMont argument.

I see that the default was set to false in 0.8. Can I just remove that field and everything will work fine? Or will this lead to breaking changes?

Disclaimer: I don't know what montgomery form is.

@gbotrel
Copy link
Collaborator Author

gbotrel commented Oct 2, 2023

That was actually an overlook on my sight, pointed by other users in the past;

there is actually a subtle breaking change here..

the default is now effectively ecc.MultiExpConfig{ScalarsMont: true} so if you used that everywhere, you should be fine.

But callers without an explicit value would have had the default at: ecc.MultiExpConfig{ScalarsMont: false} and the upgrade would be a silent change.. --> TLDR; all the APIs are now consistant and don't expose the underlying form / encoding of the field elements.

@samlaf
Copy link

samlaf commented Oct 2, 2023

That was actually an overlook on my sight, pointed by other users in the past;

there is actually a subtle breaking change here..

the default is now effectively ecc.MultiExpConfig{ScalarsMont: true} so if you used that everywhere, you should be fine.

But callers without an explicit value would have had the default at: ecc.MultiExpConfig{ScalarsMont: false} and the upgrade would be a silent change.. --> TLDR; all the APIs are now consistant and don't expose the underlying form / encoding of the field elements.

Awesome thanks! Small note: would have been useful if you guys added a Breaking Changes section to the release notes.
Otherwise is there a good reference that you would recommend to understand these montgomery forms and how they are used in this library?

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.

What are the usecases for ToBigInt method?

4 participants