-
Notifications
You must be signed in to change notification settings - Fork 54
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
Refactor Query #1146
Refactor Query #1146
Conversation
0cec606
to
a309297
Compare
c69001e
to
90d76ea
Compare
let (key, value) = s.car_cdr(kv).expect("kv missing"); | ||
// NOTE: This is an unconstrained allocation, but when actually hooked up to Lurk reduction, it must be | ||
// constrained appropriately. | ||
let allocated_key = | ||
AllocatedPtr::alloc(ns!(cs, "allocated_key"), || Ok(s.hash_ptr(&key))).unwrap(); | ||
// NOTE: The returned value is unused here, but when actually hooked up to Lurk reduction, it must be used | ||
// as the result of evaluating the query. | ||
let _allocated_value = | ||
self.synthesize_toplevel_query(cs, g, s, i, &allocated_key, value)?; |
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.
By "constrained appropriately", you mean that the Lurk reduction will be computed the allocated_key
and that the circuit_scope
would allocate the allocated_value
that would be reused here ?
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.
Yes.
@@ -87,8 +88,13 @@ where | |||
store: &Store<F>, | |||
result: AllocatedPtr<F>, | |||
dependency_provenances: Vec<AllocatedPtr<F>>, | |||
allocated_key: Option<&AllocatedPtr<F>>, |
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.
I don't understand why you're passing an option here, since you only call this function once on a Some
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.
Because I wasn't sure if we would need to do this the other way. It's just for generality. Once we get this completely nailed down (and optimized), we can remove any unneeded code paths.
q.clone() | ||
} else { | ||
self.synthesize_query(ns!(cs, "query"), g, store)? | ||
}; |
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 seems dangerous. none/some will lead to non-uniform circuits
509e227
to
868f59e
Compare
* Refactor Query to be more granular. * Use allocated_key when proving query. * Fixed `allocated_key` (#1156) --------- Co-authored-by: porcuquine <porcuquine@users.noreply.github.com> Co-authored-by: Gabriel Barreto <gabriel.aquino.barreto@gmail.com>
This PR refactors the
CircuitQuery
andRecursiveCircuitQuery
traits to allow more granular implementations. This is a step toward more flexible and optimized Coroutine circuit proofs.UPDATE: 90d76ea intends to a fix a bug whereby the allocated keys and values were not properly connected.
Notice that this fix remove a few hashes and noticeably reduces constraints, since we are now correctly reusing an allocated value rather than expensively recomputing it.