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

Add left shift and right shift gadgets to o1js #1194

Merged
merged 31 commits into from
Nov 2, 2023
Merged

Conversation

MartinMinkov
Copy link
Contributor

@MartinMinkov MartinMinkov commented Oct 23, 2023

Summary

This PR adds the implementation for left shift and right shift gates to the existing Gadgets namespace. Bitwise shifting is an operation that moves the bits of a binary number to the left or right. Unlike rotation, the bits that "fall off" at the end are discarded and the vacant positions are filled with zeros.

The implementations is based on the original OCaml implementation which is specified here for left shift and right shift

In our shift implementations, we handle the constant case by using the added functionality in the bindings, and for the prover case, we specify the implementation in this PR.

Tested

This PR adds unit tests, and verification key tests for the shift gates.

🔗 bindings: o1-labs/o1js-bindings#191

…ion on Field elements

This function is similar to the '<<' shift operation in JavaScript. It uses the rotation method internally to perform the operation. It throws an error if the input value exceeds 64 bits.
…ation on Field elements

This function is similar to the `>>` shift operation in JavaScript, where bits are moved to the right. It throws an error if the input value exceeds 64 bits. This feature enhances the functionality of the Gadgets library by providing more operations for Field elements.
@MartinMinkov MartinMinkov changed the base branch from main to feat/ROT-gadget October 23, 2023 19:30
@MartinMinkov MartinMinkov linked an issue Oct 23, 2023 that may be closed by this pull request
@MartinMinkov MartinMinkov marked this pull request as ready for review October 24, 2023 17:22
Copy link
Member

@querolita querolita left a comment

Choose a reason for hiding this comment

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

Looks good to me! Just a nit to clarify some things in the docs

checkMaxBits(field);
return new Field(Fp.rightShift(field.toBigInt(), bits));
}
const [, excess] = rotate(field, bits, 'right');
Copy link
Member

Choose a reason for hiding this comment

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

Why is there only one , whereas there's three outputs returned by rotate?

src/lib/gadgets/gadgets.ts Outdated Show resolved Hide resolved
src/lib/gadgets/gadgets.ts Show resolved Hide resolved
@@ -171,6 +171,14 @@
"rot": {
"rows": 17,
"digest": "916f4017a60f48d56d487c6869919b9c"
},
"leftShift": {
"rows": 9,
Copy link
Member

Choose a reason for hiding this comment

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

Okay so this is 4 + 4 + 1 nice

Copy link
Member

@Trivo25 Trivo25 left a comment

Choose a reason for hiding this comment

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

Good job!

src/lib/gadgets/gadgets.ts Outdated Show resolved Hide resolved
src/lib/gadgets/gadgets.unit-test.ts Outdated Show resolved Hide resolved
src/lib/gadgets/gadgets.unit-test.ts Outdated Show resolved Hide resolved
src/lib/gadgets/gadgets.unit-test.ts Outdated Show resolved Hide resolved
src/lib/gadgets/gadgets.unit-test.ts Outdated Show resolved Hide resolved
Base automatically changed from feat/ROT-gadget to main October 26, 2023 17:29
@MartinMinkov MartinMinkov requested a review from a team as a code owner October 26, 2023 17:29
src/lib/gadgets/gadgets.ts Show resolved Hide resolved
Copy link
Member

@Trivo25 Trivo25 left a comment

Choose a reason for hiding this comment

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

Great job!

src/lib/gadgets/bitwise.unit-test.ts Show resolved Hide resolved
*
* **Important:** The gadgets assumes that its input is at most 64 bits in size.
*
* If the input exceeds 64 bits, the gadget is invalid and does not prove correct execution of the shift.
Copy link
Member

Choose a reason for hiding this comment

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

If the input exceeds 64 bit, does proving fail or does the gadget provide invalid results?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the former. I just tried it as a sanity check and I get a prover error:
Error: the proof could not be constructed: rest of division by vanishing polynomial

Copy link
Collaborator

Choose a reason for hiding this comment

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

But that's because you're not maliciously changing the witness!! If you'd modify the witness blocks, and could also control the inputs (and the circuit doesn't prevent them from exceeding 64 bits) you could create a proof and get the gadget to return invalid results

Copy link
Collaborator

Choose a reason for hiding this comment

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

Anyway, the updated wording also reads good to me 👍🏻

src/lib/gadgets/gadgets.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@mitschabaude mitschabaude left a comment

Choose a reason for hiding this comment

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

Looks good to me! Changelog update would be nice (and while you're at it -- maybe you can also add the one for Gadgets.and() which we forgot)

…gets.rightShift(), and Gadgets.and() to support bitwise operations for native field elements
@MartinMinkov MartinMinkov merged commit 1500523 into main Nov 2, 2023
13 checks passed
@MartinMinkov MartinMinkov deleted the feat/LCL-LSR-gadget branch November 2, 2023 21:08
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.

Port LSL and LSR gadgets to TypeScript
5 participants