Skip to content

fix: cycle_group validate_is_on_curve bug #4494

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

Merged
lucasxia01 merged 7 commits into from
Feb 9, 2024
Merged

fix: cycle_group validate_is_on_curve bug #4494

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1ab819b
added validation test to cycle_group
lucasxia01 Feb 7, 2024
c10521d
fixed bug and test
lucasxia01 Feb 7, 2024
5ad9af0
updated join_split test that compares hardcoded hash of vk
lucasxia01 Feb 7, 2024
696edbe
added comments
lucasxia01 Feb 8, 2024
16693a5
Merge branch 'master' into lx/cycle_group_validation_bug
lucasxia01 Feb 8, 2024
244eec7
added check_circuit to tests
lucasxia01 Feb 9, 2024
92796be
Merge branch 'master' into lx/cycle_group_validation_bug
Rumata888 Feb 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,8 @@ template <typename Composer> void cycle_group<Composer>::validate_is_on_curve()
auto xx = x * x;
auto xxx = xx * x;
auto res = y.madd(y, -xxx - Group::curve_b);
res *= is_point_at_infinity();
res *= !is_point_at_infinity(); // if the point is marked as the point at infinity, then res should be 0, but
Comment thread
lucasxia01 marked this conversation as resolved.
Outdated
// otherwise, we leave res unchanged
res.assert_is_zero();
}

Expand Down
45 changes: 41 additions & 4 deletions barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,43 @@ template <class Builder> class CycleGroupTest : public ::testing::Test {
using CircuitTypes = ::testing::Types<bb::StandardCircuitBuilder, bb::UltraCircuitBuilder>;
TYPED_TEST_SUITE(CycleGroupTest, CircuitTypes);

TYPED_TEST(CycleGroupTest, TestValidateOnCurveSucceed)
Comment thread
lucasxia01 marked this conversation as resolved.
{
STDLIB_TYPE_ALIASES;
auto builder = Builder();
Comment thread
lucasxia01 marked this conversation as resolved.
Outdated

auto lhs = TestFixture::generators[0];
cycle_group_ct a = cycle_group_ct::from_witness(&builder, lhs);
a.validate_is_on_curve();
EXPECT_FALSE(builder.failed());

@Rumata888 Rumata888 Feb 9, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd recommend doing the check_circuit, too. failed() doesn't check if constraints hold

}

TYPED_TEST(CycleGroupTest, TestValidateOnCurveInfinitySucceed)
{
STDLIB_TYPE_ALIASES;
auto builder = Builder();

auto x = stdlib::field_t<Builder>::from_witness(&builder, 1);
Comment thread
lucasxia01 marked this conversation as resolved.
auto y = stdlib::field_t<Builder>::from_witness(&builder, 1);

cycle_group_ct a(x, y, true);
Comment thread
lucasxia01 marked this conversation as resolved.
Outdated
a.validate_is_on_curve();
EXPECT_FALSE(builder.failed());

@Rumata888 Rumata888 Feb 9, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd recommend doing the check_circuit, too. failed() doesn't check if constraints hold

}

TYPED_TEST(CycleGroupTest, TestValidateOnCurveFail)
{
STDLIB_TYPE_ALIASES;
auto builder = Builder();

auto x = stdlib::field_t<Builder>::from_witness(&builder, 1);
auto y = stdlib::field_t<Builder>::from_witness(&builder, 1);

cycle_group_ct a(x, y, false);
a.validate_is_on_curve();
EXPECT_TRUE(builder.failed());

@Rumata888 Rumata888 Feb 9, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd recommend doing the check_circuit, too. failed() doesn't check if constraints hold

}

TYPED_TEST(CycleGroupTest, TestDbl)
{
STDLIB_TYPE_ALIASES;
Expand Down Expand Up @@ -436,8 +473,8 @@ TYPED_TEST(CycleGroupTest, TestBatchMul)
EXPECT_TRUE(result.is_point_at_infinity().get_value());
}

// case 5, fixed-base MSM with inputs that are combinations of constant and witnesses (group elements are in lookup
// table)
// case 5, fixed-base MSM with inputs that are combinations of constant and witnesses (group elements are in
// lookup table)
{
std::vector<cycle_group_ct> points;
std::vector<typename cycle_group_ct::cycle_scalar> scalars;
Expand Down Expand Up @@ -465,8 +502,8 @@ TYPED_TEST(CycleGroupTest, TestBatchMul)
EXPECT_EQ(result.get_value(), crypto::pedersen_commitment::commit_native(scalars_native));
}

// case 6, fixed-base MSM with inputs that are combinations of constant and witnesses (some group elements are in
// lookup table)
// case 6, fixed-base MSM with inputs that are combinations of constant and witnesses (some group elements are
// in lookup table)
{
std::vector<cycle_group_ct> points;
std::vector<typename cycle_group_ct::cycle_scalar> scalars;
Expand Down