Add ECAdd() Bloq #1425
Add ECAdd() Bloq #1425
Conversation
|
Dependent on #1424. For testing purposes I put the bug fix in this branch too, but it should be merged first. |
There was a problem hiding this comment.
A momentus piece of work! I did not check the exact circuits but the classical testing should give us a great deal of confidence. It would give more confidence if we had matching toffoli costs (and/or defensible reasons for any differences). Some small-ish things for improvement.
| (0, 0), | ||
| ], | ||
| ) | ||
| def test_ec_add_steps_classical(n, m, a, b, x, y): |
There was a problem hiding this comment.
the parameterization here seems a little gratuitous. do we need the outer product of all possible n x m? What are we probing with different n values? it either fits or it doesn't, no? how about picking some extremal pairs? Do we need the complete outer product of point1 x point2? Maybe cross all possible point1 with some select extremal point2's
There was a problem hiding this comment.
I pretty much halved the testing by removing a big chunk of the pairs. I did it in a way such that all 4 bugs from #1461 would still trigger if I made the frees non-dirty. Let me know if you want me to further reduce the tests. All-in-all it runs in 1 min 30 s on my pc with the slow tests running.
| @@ -39,6 +42,20 @@ def build_call_graph(self, ssa: SympySymbolAllocator) -> BloqCountDictT: | |||
| return {Toffoli(): self.n - 2} | |||
|
|
|||
|
|
|||
| @frozen | |||
There was a problem hiding this comment.
I don't think this should be a shim
There was a problem hiding this comment.
I agree it's easy to implement, but I think it is out of scope for this PR - it is a component of ModInv not ECAdd. I just changed it in this PR to make the Toffoli count test more accurate.
| @@ -103,6 +117,29 @@ def build_call_graph(self, ssa: 'SympySymbolAllocator') -> 'BloqCountDictT': | |||
| Swap(self.n): 1, | |||
| } | |||
|
|
|||
| def on_classical_vals( | |||
There was a problem hiding this comment.
why use the shim instead of the actual implementation?
There was a problem hiding this comment.
I only implemented enough to test that ECAdd works - when ModInverse bloq is merged I'm happy to replace the shim with that in the construction/tests.
Head branch was pushed to by a user without write access
Adds ECAdd bloq with classical simulation. I broke the bloq into the 6 steps shown in https://arxiv.org/abs/2306.08585. I classically simulated all steps. I put in a shimmed ModInv classical implementation (it's not montgomery mod inversion, but it still works on ECPoints not in montgomery form; I'm a little stuck on understanding what to do for R in montgomery mod inversion).