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

Optimized ECC chip #73

Closed
wants to merge 49 commits into from
Closed

Optimized ECC chip #73

wants to merge 49 commits into from

Conversation

therealyingtong
Copy link
Contributor

@therealyingtong therealyingtong commented Apr 28, 2021

Closes #38.

Depends on Utils chip (#89).

@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2021

Codecov Report

Merging #73 (6d1059b) into utils-chip (fea88c8) will increase coverage by 3.54%.
The diff coverage is 86.13%.

Impacted file tree graph

@@              Coverage Diff               @@
##           utils-chip      #73      +/-   ##
==============================================
+ Coverage       82.36%   85.90%   +3.54%     
==============================================
  Files              36       50      +14     
  Lines            2438     3781    +1343     
==============================================
+ Hits             2008     3248    +1240     
- Misses            430      533     +103     
Impacted Files Coverage Δ
...gadget/ecc/chip/witness_scalar_fixed/full_width.rs 75.00% <75.00%> (ø)
src/circuit/gadget/ecc/chip/mul_fixed/short.rs 77.22% <77.22%> (ø)
src/constants/load.rs 78.57% <78.57%> (ø)
src/circuit/gadget/ecc.rs 86.48% <79.24%> (+86.48%) ⬆️
src/circuit/gadget/ecc/chip.rs 80.00% <80.00%> (ø)
src/circuit/gadget/ecc/chip/mul_fixed.rs 82.17% <82.17%> (ø)
src/circuit/gadget/ecc/chip/mul.rs 82.55% <82.55%> (ø)
src/circuit/gadget/ecc/chip/mul/complete.rs 84.50% <84.50%> (ø)
src/circuit/gadget/ecc/chip/witness_point.rs 86.66% <86.66%> (ø)
src/circuit/gadget/ecc/chip/add.rs 89.63% <89.63%> (ø)
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fea88c8...6d1059b. Read the comment docs.

@therealyingtong therealyingtong changed the title [WIP] Optimized ECC chip Optimized ECC chip Apr 29, 2021
@therealyingtong therealyingtong marked this pull request as ready for review April 29, 2021 22:03
@therealyingtong therealyingtong force-pushed the ecc-impl branch 2 times, most recently from 4c6ff3a to b77f4b2 Compare May 4, 2021 13:49
@therealyingtong therealyingtong changed the base branch from constants to ecc-gadget May 4, 2021 13:49
@therealyingtong therealyingtong force-pushed the ecc-impl branch 2 times, most recently from 53803b0 to 6f605a7 Compare May 6, 2021 07:47
@therealyingtong therealyingtong marked this pull request as draft May 18, 2021 10:51
@therealyingtong therealyingtong marked this pull request as ready for review May 18, 2021 11:53
Base automatically changed from ecc-gadget to constants May 18, 2021 19:35
Base automatically changed from constants to main May 18, 2021 19:45
@str4d str4d added this to the Core Sprint 2021-18 milestone May 18, 2021
src/circuit/gadget/ecc/chip.rs Outdated Show resolved Hide resolved
src/circuit/gadget/ecc/chip.rs Outdated Show resolved Hide resolved
src/circuit/gadget/ecc/chip.rs Outdated Show resolved Hide resolved
src/circuit/gadget/ecc/chip.rs Outdated Show resolved Hide resolved
src/circuit/gadget/ecc/chip.rs Outdated Show resolved Hide resolved
src/circuit/gadget/ecc/chip.rs Outdated Show resolved Hide resolved
src/circuit/gadget/ecc/chip.rs Outdated Show resolved Hide resolved
src/circuit/gadget/ecc/chip.rs Outdated Show resolved Hide resolved
src/circuit/gadget/ecc/chip.rs Outdated Show resolved Hide resolved
src/circuit/gadget/ecc/chip/add.rs Outdated Show resolved Hide resolved
@therealyingtong therealyingtong force-pushed the ecc-impl branch 2 times, most recently from 3f90a2a to eada6a9 Compare May 22, 2021 07:28
therealyingtong and others added 15 commits June 5, 2021 00:15
The EccConfig only knows about 10 generic advice columns. These
columns are renamed by individual components (e.g. AddConfig,
MulConfig). The mapping for each individual config is captured
in its impl From<EccConfig>.
…requires a doubling.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
…on that requires a doubling.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Inline equality assertions on values to individual components
(flagged off with #[cfg(test)].

Move module-specific tests into respective modules.
…pes.

We do not load the fixed bases into a single location in the circuit;
rather, they are assigned to fixed columns at the offsets where they
need to be used.

The EccLoaded struct was not storing any references to circuit variables.
It was holding constants in memory which the caller can instead directly
access.

Similarly, there is no need to have the FixedPoint or FixedPointShort
associated types in the EccInstructions trait, since these types do not
contain references to circuit variables. The corresponding get_fixed()
and get_fixed_short() instructions are also removed.
This was defining shared behaviour between the mul_fixed::short and
mul_fixed::full_width modules. But the behaviour can be captured by
simply sharing the same mul_fixed::Config.
There is no need to enumerate FixedPointsShort or to make a newtype,
since ValueCommitV is the only fixed base used with short signed
scalars.
@therealyingtong therealyingtong changed the base branch from main to utils-chip June 4, 2021 16:27
@therealyingtong
Copy link
Contributor Author

I am closing this PR in favour of #107.

This is because the changes to constants.rs and the constants module have been factored out into #106. I will link the unresolved comments from this PR on the corresponding positions in the #107.

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.

Implement generic ECC gadget
7 participants