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

Fix bugs in ECAdd bloq #1489

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Fix bugs in ECAdd bloq #1489

wants to merge 10 commits into from

Conversation

fpapa250
Copy link
Contributor

@fpapa250 fpapa250 commented Nov 7, 2024

The circuit described in the paper had 4 bugs (#1461) which caused (with edge case inputs) flags f1, f2, f4 and lambda to be freed (considered = 0 in the paper) while having values > 0.

Bugs:
1 - Step 2: lam = lam_r undoes f1 when it should only happen if lam is set to lam_r (very rare case)
BUGFIX:

  1. Set a new flag bit when computed lam = lam_r (before writing lam from computed z register to lam register)
  2. Control existing equals bloq to clear f1 only when lam = lam_r AND the ancilla flag is set
  3. Unset ancilla and free when z4 (containing computed lam) = lam_r

2 - Step 5: free dirty reg lam when a,b=x,y
BUGFIX:

  1. set a new flag if x (denominator of lambda computation) = 0
  2. Clear lam using lam_r controlled on ctrl and new flag
  3. unset new flag when x = 0

3 - Step 6: free dirty reg when x,y = 0,0 and b = y = 0, but x != 0: f2 never gets flipped back
BUGFIX:

  1. unset f2 when x,y = 0 and b = 0 OR a,b = 0 and y = 0

4 - Step 6: when p1 = p2 and f4 not set f4 gets flipped on
BUGFIX:

  1. Move CModSub and CModAdd to before Equals(xy, ab) bloq

@fpapa250 fpapa250 marked this pull request as ready for review November 27, 2024 20:48
fpapa250 and others added 2 commits November 29, 2024 14:59
Co-authored-by: Anurudh Peduri <7265746+anurudhp@users.noreply.github.com>
@fpapa250
Copy link
Contributor Author

fpapa250 commented Dec 3, 2024

@mpharrigan Here are the bug fixes to the ECAdd circuit that the paper didn't consider. They are thoroughly tested with the existing unit tests.

@mpharrigan
Copy link
Collaborator

Nice!

The issue and PR description describes the modifications and why they were needed -- but I suspect only the most dedicated users will find them! Do you want to put a description of the modifications into the library somewhere? Ideally this would be in the docstrings of the public bloq classes so they'd get rendered into the docs

@mpharrigan
Copy link
Collaborator

an alternative would be to have a dedicated jupyter notebook written as a mini-paper that goes through all the crypto bloqs and includes a section on the modifications. This would be more work but this is the sort of thing we can slap a byline on and get a zenodo doi to make it citable

Comment on lines +917 to +929
aby_arr = np.concatenate([bb.split(a), bb.split(b), bb.split(y)])
aby_arr, f2 = bb.add(MultiControlX(cvs=[0] * 3 * self.n), controls=aby_arr, target=f2)
aby_arr = np.split(aby_arr, 3)
a = bb.join(aby_arr[0], dtype=QMontgomeryUInt(self.n))
b = bb.join(aby_arr[1], dtype=QMontgomeryUInt(self.n))
y = bb.join(aby_arr[2], dtype=QMontgomeryUInt(self.n))

xyb_arr = np.concatenate([bb.split(x), bb.split(y), bb.split(b)])
xyb_arr, f2 = bb.add(MultiControlX(cvs=[0] * 3 * self.n), controls=xyb_arr, target=f2)
xyb_arr = np.split(xyb_arr, 3)
x = bb.join(xyb_arr[0], dtype=QMontgomeryUInt(self.n))
y = bb.join(xyb_arr[1], dtype=QMontgomeryUInt(self.n))
b = bb.join(xyb_arr[2], dtype=QMontgomeryUInt(self.n))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can replace this with the default controlled to avoid manual splits and joins:

mcx = XGate().controlled(CtrlSpec(qdtypes=QMontgomeryUInt(self.n), cvs=[0, 0, 0]))
[a, b, y], f2 = bb.add(mcx, ctrl=[a, b, y], q=f2)
[x, y, b], f2 = bb.add(mcx, ctrl=[x, y, b], q=f2)

Though I suspect the types may not be propagated correctly yet. In case you try the above suggestion and it fails, could you please open an issue?

p.s. this would also enable decomposing for symbolic self.n which would be an added benefit.

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.

3 participants