chore!: Define single ecdsa constraint generator #16612
Conversation
federicobarbacovi
changed the title
alejoamiras
assigned
alejoamiras and
federicobarbacovi and unassigned
federicobarbacovi and
alejoamiras
| pub_key_y_fq.assert_is_in_field(); | ||
| typename secp256k1_ct::g1_bigfr_ct public_key = typename secp256k1_ct::g1_bigfr_ct(pub_key_x_fq, pub_key_y_fq); | ||
| for (size_t i = 0; i < 32; ++i) { | ||
| sig.r[i].assert_equal(field_ct::from_witness_index(&builder, input.signature[i])); |
There was a problem hiding this comment.
These assert_equal are asserting equality of sig.r[i] with the witness at index input.signature[i]. In the new code, sig.r[i] is built to be the builder variable whose underlying value is the witness value at index input.signature[i], so this check is not needed.
A similar comment applies to the other asserts.
In this version of the code the asserts were needed because the constructor for sig creates new builder variables out of rr and ss, which we must check are equal to the variables point to the indices representing r and s.
I think the asserts were not needed for message and pub_key_*_byte_array.
| typename secp256k1_ct::bigfr_ct, | ||
| typename secp256k1_ct::g1_bigfr_ct>( | ||
| message, public_key, sig); | ||
| bool_ct signature_result_normalized = signature_result.normalize(); |
There was a problem hiding this comment.
Normalization was required in this case because the assertion in the following lines is checking equality by looking at the witness indices pointed by the variables signature_result_normalized and input.result. In the new code we directly call assert_equal on signature_result, which handles the normalization on its own.
federicobarbacovi
force-pushed
the
fb/ecdsa_audit_5
branch
from
1ef0232 to
4f1fd8c
Compare
federicobarbacovi
changed the title
barretenberg/cpp/src/barretenberg/dsl/acir_format/ecdsa_constraints.cpp
Outdated
Show resolved
Hide resolved
| return offset; | ||
| } | ||
|
|
||
| TEST_F(ECDSASecp256k1, TestECDSAConstraintSucceed) |
There was a problem hiding this comment.
This test is not needed, successes/failures are tested in stdlib_ecdsa_tests.
| EXPECT_TRUE(CircuitChecker::check(builder)); | ||
| } | ||
|
|
||
| TEST_F(ECDSASecp256k1, TestECDSAConstraintFail) |
There was a problem hiding this comment.
This test is not needed, successes/failures are tested in stdlib_ecdsa_tests.
| EXPECT_TRUE(CircuitChecker::check(builder)); | ||
| } | ||
|
|
||
| TEST(ECDSASecp256r1, TestECDSAConstraintSucceed) |
There was a problem hiding this comment.
This test is not needed, successes/failures are tested in stdlib_ecdsa_tests.
| EXPECT_TRUE(CircuitChecker::check(builder)); | ||
| } | ||
|
|
||
| TEST(ECDSASecp256r1, TestECDSAConstraintFail) |
There was a problem hiding this comment.
This test is not needed, successes/failures are tested in stdlib_ecdsa_tests.
|
|
||
| TYPED_TEST_SUITE(EcdsaConstraintsTest, CurveTypes); | ||
|
|
||
| TYPED_TEST(EcdsaConstraintsTest, GenerateVKFromConstraints) |
There was a problem hiding this comment.
This is the only type of test needed here: checking that the vk generated from ACIR constraints with a witness is the same as the one generated by ACIR constraints without a witness
iakovenkos
left a comment
iakovenkos
left a comment
There was a problem hiding this comment.
nice! thank for cleaning it up
federicobarbacovi
force-pushed
the
fb/ecdsa_audit_5
branch
from
f6e0c5f to
7a0f009
Compare
federicobarbacovi
added
the
bberg-int-audit
BEGIN_COMMIT_OVERRIDE chore!: Define single ecdsa constraint generator (#16612) END_COMMIT_OVERRIDE
Audit part 5: Define single ecdsa constraint generator In this PR we merge the two ECDSA constraint generators (secp256k1 and secp256r1) into a single one templated over `Curve` type. We also restructure the code of the constraint generator to be more similar to other parts of the `dsl` package. There are some changes to the circuits. This is due to some `assert_equal` that have been removed, and `normalize` calls that are not needed. We also add documentation and define a `utils.hpp` file in the `dsl` package containing some functions that are used multiple times in different files.
Audit part 5: Define single ecdsa constraint generator In this PR we merge the two ECDSA constraint generators (secp256k1 and secp256r1) into a single one templated over `Curve` type. We also restructure the code of the constraint generator to be more similar to other parts of the `dsl` package. There are some changes to the circuits. This is due to some `assert_equal` that have been removed, and `normalize` calls that are not needed. We also add documentation and define a `utils.hpp` file in the `dsl` package containing some functions that are used multiple times in different files.
Audit part 5: Define single ecdsa constraint generator
In this PR we merge the two ECDSA constraint generators (secp256k1 and secp256r1) into a single one templated over
Curvetype. We also restructure the code of the constraint generator to be more similar to other parts of thedslpackage.There are some changes to the circuits. This is due to some
assert_equalthat have been removed, andnormalizecalls that are not needed.We also add documentation and define a
utils.hppfile in thedslpackage containing some functions that are used multiple times in different files.