-
Notifications
You must be signed in to change notification settings - Fork 108
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
feat: Add Goblin Ultra Circuit builder #587
Conversation
1bc1069
to
4e7fd04
Compare
13abba1
to
779f064
Compare
3a8c9e7
to
09e33ba
Compare
@@ -11,4 +11,30 @@ TEST(ECCOpQueueTest, Basic) | |||
EXPECT_EQ(op_queue.raw_ops[1].add, false); | |||
} | |||
|
|||
TEST(ECCOpQueueTest, Accumulator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear what the test is supposed to test from the name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to InternalAccumulatorCorrectness
* @param expected | ||
* @return whether accumulator point == expected point | ||
*/ | ||
bool eq(const Point& expected) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This interface forces us to always redo this computation. First you need to compute the value externally, then the same value is computed in OpQueue. What do you think of making the return value the point and having no argument to the function? Then we can check the value correctness in tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this so that instead of calling get_accumulator()
explicitly, the equality op implicitly uses used the internal accumulator and that value is returned when calling queue_ecc_eq()
for use elsewhere in the circuit if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: codygunton <codygunton@gmail.com>
Co-authored-by: codygunton <codygunton@gmail.com>
Description
Initial conception of the Ultra Goblin Circuit Builder.
The Goblin builder extends the existing Ultra builder. For now I've implemented the new methods directly in the existing UltraCircuitBuilder. So essentially the UltraCircuitBuilder now is the Goblin Ultra circuit builder.
This work essentially adds three components on top of the Ultra builder: 1) An
op_queue
that stores the ecc operations in a raw form, e.g. {op, point, scalar}, and also computes the corresponding operations natively behind the scenes, 2)op_wires
which are similar to the existingwires
object but are used to store the values in the "ecc op gates", and 3) methods likequeue_ecc_mul_accum()
andqueue_ecc_add_accum()
which update both theop_queue
and theop_wires
accordingly.I've included some tests that check the underlying native ECC operations in the op queue, the consistency of the values written to the
op_wires
by the builder, and some checks of higher level functionality likebatch_mul()
.There are lot's of small TODOs to be addressed in the coming weeks.
Checklist:
@brief
describing the intended functionality.include
directives have been added.