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

Refactory #39

Merged
merged 34 commits into from
Nov 8, 2024
Merged

Conversation

davxy
Copy link
Collaborator

@davxy davxy commented Nov 2, 2024

  • test-vectors feature: zk rows set to zero for deterministic proof
  • share some code between te and sw cond add
  • reduced explicit trait dependencies using AffineCondAdd trait
  • default transcript type set to ArkTranscript

davxy and others added 29 commits August 10, 2024 17:26
naming convention and clean up.

Co-authored-by: Davide Galassi <[email protected]>
- make test compiles for std (and fails due to lack of rng).
- make sure random and hashed points are not identity elements.
@davxy davxy requested a review from drskalman November 2, 2024 18:34
common/src/gadgets/cond_add.rs Outdated Show resolved Hide resolved
common/src/gadgets/sw_cond_add.rs Show resolved Hide resolved
common/src/gadgets/te_cond_add.rs Show resolved Hide resolved
common/src/gadgets/cond_add.rs Show resolved Hide resolved
ring/src/piop/params.rs Show resolved Hide resolved
ring/src/lib.rs Show resolved Hide resolved
{
type CondAddValT = TeCondAddValues<F, Curve>;
type Values = TECondAddValues<C>;

// Populates the acc column starting from the supplied seed (as 0 doesn't work with the addition formula).
// As the TE addition formula used is not complete, the seed must be selected in a way that would prevent
// exceptional cases (doublings or adding the opposite point).
// The last point of the input column is ignored, as adding it would made the acc column overflow due the initial point.
fn init(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here you could check if the seed is inside the group.

Copy link
Collaborator Author

@davxy davxy Nov 7, 2024

Choose a reason for hiding this comment

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

In general, SW "works" even when the seed is within the prime order subgroup, provided that the cond add do not (maliciously or not) stumble to the identity.

Does TE strictly require the seed to be within the prime order subgroup, or could it also operate with a seed outside the prime order subgroup (with some failure cases that are more or less probable)?
Specifically, if for TE a point outside the prime order group is chosen as a seed, what would be the resulting behavior? I haven't yet analyzed the constraint formula for the TE construction...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, there is a slightly higher risk. Given you have enforced PoP, I think the probablity of failure if you choose S outside the group is like 8-9 times higher than if you choose it inside, for the TE case. For

$$P = (x1, y1)$$,

$$P+Q$$ fails, if $Q$ 's is inside one of these sets.
failing-set

You could argue that 8 times of negligible is still negligble. I'd have put a check to prevent choosing out of the prime subgroup here, as it doesn't have performance hit, but I agree it is somehow a subjective, so your choice.

{
type CondAddValT = SwCondAddValues<F>;
type Values = SWCondAddValues<C>;

// Populates the acc column starting from the supplied seed (as 0 doesn't have an affine SW representation).
// As the SW addition formula used is not complete, the seed must be selected in a way that would prevent
// exceptional cases (doublings or adding the opposite point).
// The last point of the input column is ignored, as adding it would made the acc column overflow due the initial point.
fn init(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here you could check if the seed is outside of the group.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wouldn't this be more of a suggestion? From my understanding, it is recommended to select a seed point outside the prime order subgroup. However, the implementation still functions even if the seed is within the group (in this case we need PoP). In the codebase there are generic tests that simply use a random point from the prime order group as a seed for SW, which avoids the need for separate code for seed generation (TE vs SW)

I would prefer to add a comment that suggests choosing a seed outside the group, possibly generated via find_complement_point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This true. Sure. You could add a comment saying you need to check PoP unless if S is outside of the group.

@davxy
Copy link
Collaborator Author

davxy commented Nov 7, 2024

@drskalman I applied some of your suggestions. I have questions for the assertions you suggested to introduce

@davxy davxy requested a review from drskalman November 7, 2024 09:53
Copy link
Collaborator

@drskalman drskalman left a comment

Choose a reason for hiding this comment

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

I left a nonbinding comment :-)

{
type CondAddValT = SwCondAddValues<F>;
type Values = SWCondAddValues<C>;

// Populates the acc column starting from the supplied seed (as 0 doesn't have an affine SW representation).
// As the SW addition formula used is not complete, the seed must be selected in a way that would prevent
// exceptional cases (doublings or adding the opposite point).
// The last point of the input column is ignored, as adding it would made the acc column overflow due the initial point.
fn init(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This true. Sure. You could add a comment saying you need to check PoP unless if S is outside of the group.

{
type CondAddValT = TeCondAddValues<F, Curve>;
type Values = TECondAddValues<C>;

// Populates the acc column starting from the supplied seed (as 0 doesn't work with the addition formula).
// As the TE addition formula used is not complete, the seed must be selected in a way that would prevent
// exceptional cases (doublings or adding the opposite point).
// The last point of the input column is ignored, as adding it would made the acc column overflow due the initial point.
fn init(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, there is a slightly higher risk. Given you have enforced PoP, I think the probablity of failure if you choose S outside the group is like 8-9 times higher than if you choose it inside, for the TE case. For

$$P = (x1, y1)$$,

$$P+Q$$ fails, if $Q$ 's is inside one of these sets.
failing-set

You could argue that 8 times of negligible is still negligble. I'd have put a check to prevent choosing out of the prime subgroup here, as it doesn't have performance hit, but I agree it is somehow a subjective, so your choice.

@davxy davxy merged commit 2d6e73a into w3f:skalman--powers-of-two-gadget Nov 8, 2024
4 checks passed
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.

2 participants