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.

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

Jump to bottom

175 complete ecdsa bbf #304

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

175 complete ecdsa bbf #304

wants to merge 11 commits into from

Conversation

AntoineCyr
Copy link
Contributor

@AntoineCyr AntoineCyr commented Jan 30, 2025 •
edited
Loading

Migrated 3 new circuits in bbf:

  • ec_two_t_plus_q
  • ec_scalar_mult
  • ecdsa_recovery

Normalizing imports and variables accros subcomponents

@AntoineCyr AntoineCyr self-assigned this Jan 30, 2025
@AntoineCyr AntoineCyr linked an issue Jan 30, 2025 that may be closed by this pull request
ECDSA in bbf #175
Open
@AntoineCyr AntoineCyr marked this pull request as draft January 30, 2025 21:00
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 30, 2025 •
edited
Loading

Clang Test Results

  177 files  + 3    177 suites  +3   22m 2s ⏱️ + 1m 26s
8 549 tests +10  8 543 ✅ +10  6 💤 ±0  0 ❌ ±0 
8 615 runs  + 3  8 609 ✅ + 3  6 💤 ±0  0 ❌ ±0 

Results for commit 4039020. ± Comparison against base commit 8b0390b.

This pull request removes 1 and adds 11 tests. Note that renamed tests count towards both. 
blueprint_plonk_test_suite ‑ blueprint_plonk_bbf_addition_mod_p_test

blueprint_plonk_test_suite ‑ blueprint_plonk_bbf_add_sub_mod_p_test
blueprint_plonk_test_suite ‑ blueprint_plonk_bbf_ec_scalar_mult_test
blueprint_plonk_test_suite ‑ blueprint_plonk_bbf_ec_two_t_plus_q_test
blueprint_plonk_test_suite ‑ blueprint_plonk_hashes_keccak_round_bbf_not_random_pallas
blueprint_plonk_test_suite ‑ blueprint_plonk_hashes_keccak_round_bbf_random_pallas
blueprint_plonk_test_suite ‑ blueprint_plonk_pubkey_non_native_ecdsa_pallas
blueprint_plonk_test_suite ‑ blueprint_plonk_pubkey_non_native_ecdsa_vesta
lpc_math_polynomial_suite ‑ lpc_basic_skipping_layers_test
lpc_math_polynomial_suite ‑ lpc_dfs_basic_test
lpc_params_test_suite ‑ lpc_batches_num_3_test
…

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 30, 2025 •
edited
Loading

Gcc Test Results

  177 files  +3    177 suites  +3   21m 5s ⏱️ + 2m 33s
8 545 tests +4  8 539 ✅ +4  6 💤 ±0  0 ❌ ±0 
8 611 runs  +4  8 605 ✅ +4  6 💤 ±0  0 ❌ ±0 

Results for commit 4039020. ± Comparison against base commit 8b0390b.

This pull request removes 1 and adds 5 tests. Note that renamed tests count towards both. 
blueprint_plonk_test_suite ‑ blueprint_plonk_bbf_addition_mod_p_test

blueprint_plonk_test_suite ‑ blueprint_plonk_bbf_add_sub_mod_p_test
blueprint_plonk_test_suite ‑ blueprint_plonk_bbf_ec_scalar_mult_test
blueprint_plonk_test_suite ‑ blueprint_plonk_bbf_ec_two_t_plus_q_test
blueprint_plonk_test_suite ‑ blueprint_plonk_pubkey_non_native_ecdsa_pallas
blueprint_plonk_test_suite ‑ blueprint_plonk_pubkey_non_native_ecdsa_vesta

♻️ This comment has been updated with latest results.

@AntoineCyr AntoineCyr force-pushed the 175-complete-ECDSA-bbf branch from bcb84ee to d06a946 Compare February 4, 2025 16:48
@AntoineCyr AntoineCyr marked this pull request as ready for review February 4, 2025 16:49
@AntoineCyr AntoineCyr force-pushed the 175-complete-ECDSA-bbf branch from d8c0bb6 to e815fb7 Compare February 5, 2025 16:21
@AntoineCyr AntoineCyr requested a review from ayashunsky February 5, 2025 17:02
@AntoineCyr AntoineCyr force-pushed the 175-complete-ECDSA-bbf branch 3 times, most recently from cbdce3b to 87e57d0 Compare February 12, 2025 13:10
@AntoineCyr AntoineCyr force-pushed the 175-complete-ECDSA-bbf branch from 9cb02d3 to 2c62d6a Compare February 13, 2025 19:50
ayashunsky
ayashunsky reviewed Feb 14, 2025
View reviewed changes
...s/blueprint/include/nil/blueprint/bbf/components/algebra/fields/non_native/add_sub_mod_p.hpp
}
allocate(Q);

// Add range check for Y if it is negative
if (is_add) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be !is_add ?

ayashunsky
ayashunsky reviewed Feb 14, 2025
View reviewed changes
...s/blueprint/include/nil/blueprint/bbf/components/algebra/fields/non_native/add_sub_mod_p.hpp
}
}

for (std::size_t i = 0; i < num_chunks; ++i) {
allocate(R[i]);
allocate(Y[i]);
Copy link
Contributor

@ayashunsky ayashunsky Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is wrong, sorry. You can allocate those Y's but they cant't be connected to your input variables via copy constraints. Actually, instead of using p-y which is very sensible in general, you have do adopt a different strategy to have everything constrained. I guess, that, depending on is_add, you should either prove
x + y = r + pq
or
y + r = x + pq
with range checks on r. Maybe this has to be cross-checked for the second case but I have a feeling it is right.

ayashunsky
ayashunsky reviewed Feb 14, 2025
View reviewed changes
...bs/blueprint/include/nil/blueprint/bbf/components/algebra/curves/weierstrass/ec_full_add.hpp
CopyConstrain(XR, t21); // xR = t21
auto t22 = MultModP(YR, t20); // t22 = yR(ZPQ + WPQ)
CopyConstrain(YR, t22); // yR = t22
auto t1 = NegModP(input_xP); // t1 = -xP
Copy link
Contributor

@ayashunsky ayashunsky Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It seems that if we do have substraction, this is no longer needed. It is used to compute -3x_P² further on, but you can replace that with 3x_P² and just do substraction, I guess...

ayashunsky
ayashunsky reviewed Feb 14, 2025
View reviewed changes
.../blueprint/include/nil/blueprint/bbf/components/algebra/fields/non_native/negation_mod_p.hpp
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably check if this is used at all now...

ayashunsky
ayashunsky reviewed Feb 14, 2025
View reviewed changes
crypto3/libs/blueprint/test/bbf/algebra/fields/non_native/add_sub_mod_p.cpp

auto [at, A, desc] = B.assign(raw_input);
bool pass = B.is_satisfied(at);
std::cout << "Is_satisfied = " << pass << std::endl;

assert(pass == true);
if (!is_add) {
Copy link
Contributor

Choose a reason for hiding this comment

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

While the circuit needs correction, this is perfectly fine. Please, do not touch these lines :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ayashunsky ayashunsky ayashunsky left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@AntoineCyr AntoineCyr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ECDSA in bbf
2 participants
@AntoineCyr @ayashunsky