-
Notifications
You must be signed in to change notification settings - Fork 56
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
Standard Circom Gadget with Keccak example #997
Standard Circom Gadget with Keccak example #997
Conversation
e84369e
to
224004d
Compare
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 like the approach of grabbing the files from an online location: even if I'm a bit sceptical about the bit about doing, specifically, a Github release, which I feel might add friction.
Have you created a small toy repo where we'd be able to test this import mechanism? Perhaps try to run a GH import on nightly?
pub(crate) fn create_circom_gadget(circom_folder: &Utf8PathBuf, reference: &str) -> Result<()> { | ||
let circom_gadget = circom_dir().join(reference); | ||
|
||
// We expect a format <AUTHOR>/<NAME> for 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.
nit: GADGET_NAME
may be more useful, otherwise the reader might just wonder who this is supposed to be the name of. (nit -> you can safely ignore this)
src/coprocessor/circom/mod.rs
Outdated
/// TODO: Generalize | ||
fn arity(&self) -> usize { | ||
0 | ||
} | ||
|
||
fn synthesize_simple<CS: ConstraintSystem<F>>( | ||
&self, | ||
cs: &mut CS, | ||
g: &crate::lem::circuit::GlobalAllocator<F>, | ||
_s: &Store<F>, | ||
_not_dummy: &bellpepper_core::boolean::Boolean, | ||
args: &[AllocatedPtr<F>], | ||
) -> std::result::Result<AllocatedPtr<F>, SynthesisError> { | ||
let input = self.gadget.clone().into_circom_input(args); | ||
let witness = | ||
circom_scotia::calculate_witness(&self.config, input, true).map_err(|e| { | ||
eprintln!("{:?}", e); | ||
SynthesisError::Unsatisfiable | ||
})?; | ||
let output = circom_scotia::synthesize(cs, self.config.r1cs.clone(), Some(witness))?; | ||
let num_tag = g.alloc_tag(cs, &crate::tag::ExprTag::Num); | ||
let res = AllocatedPtr::from_parts(num_tag.clone(), output); | ||
|
||
Ok(res) | ||
} | ||
} | ||
|
||
impl<F: LurkField, C: CircomGadget<F> + Debug> Coprocessor<F> for CircomCoprocessor<F, C> { | ||
/// TODO: Generalize | ||
fn eval_arity(&self) -> usize { | ||
0 | ||
} |
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.
Yep, those todos are going to be the next steps to improve on.
Not 100% sure about what you mean but I have adapted the template in a keccak example. The keccak example is also using this remote logic.
Would you have a recommendation of another place to host them? |
Oh, nice!
For us at Lurk Lab? I'm fine with releases, for now (those gadgets can get huge) But in I was thinking user may just give us 2 URLs to their S3 bucket, and perhaps we could work with that. OTOH your template repo completely removes any issue re: friction, only the price of hosting large files on GH remains as an issue. |
cc59ac1
to
fe798a8
Compare
4620c24
to
8642a16
Compare
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 is amazing work! I can see that it's already quite polished.
Suggestion: print the output of the computation in the Keccak example so we can see the hash that Lurk spits out. The output is in the last frame, in the first component of the output
attribute. You can print it with the fmt_to_string_simple
method.
518b243
to
4e7b8ce
Compare
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.
🎉
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 is excellent, I <3 the careful error messages. I left a few inline comments, nothing blocking. Thank you so much!
examples/keccak.rs
Outdated
fn bytes_to_bits(bytes: &[u8]) -> Vec<bool> { | ||
let mut bits = Vec::new(); // Create a new, empty vector to store bits | ||
|
||
for &byte in bytes.iter() { | ||
// Iterate over each byte in the input slice | ||
for j in 0..8 { | ||
// For each bit in the byte | ||
if byte & (1 << j) > 0 { | ||
// Check if the bit is set | ||
bits.push(true); // If the bit is set, push 1 to the vector | ||
} else { | ||
bits.push(false); // If the bit is not set, push 0 | ||
} | ||
} | ||
} | ||
bits // Return the vector of bits | ||
} |
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.
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.
Switched to bytes_to_bit_le
fn bits_to_bytes(bits: &[bool]) -> Vec<u8> { | ||
let mut bytes = vec![0; (bits.len() + 7) / 8]; // Initialize a vector with zeroes | ||
|
||
for (i, &bit) in bits.iter().enumerate() { | ||
// Iterate over each bit with its index | ||
let byte_index = i / 8; // Calculate the byte index for the current bit | ||
if bit { | ||
// If the current bit is true, | ||
bytes[byte_index] |= 1 << (i % 8); // Set the corresponding bit in the byte | ||
} | ||
} | ||
bytes // Return the array of bytes | ||
} |
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.
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 to be only available in the crate context, not publicly
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.
Thanks a ton!
- With the new implementation, we first try to fetc hthe gadget locally and fallback on remote when needed. - Introduced a version over Circom Gadgets. Useful to point to a particular remote release. - Better error propagation - Precise error message based on where the flow is failing
This is WIP as there is a need to fix circom-scotia before being able to make the Keccak example work. - Update framework -> stack in the README - Create an example skeleton for a remote Keccak example - Fix the circom example to use an imported Circom Gadget from the CLI
- Removed deleted keccak mod - Fixed clippy warning
- Adapted circom-scotia to use main instead of dev - Created a structure, CircomReference to qualify an identifier for CircomGadget - Better check on CircomReference initialization
Arity is now inherited directly from the gadget, making it declarative by the gadget developer.
Co-authored-by: François Garillot <4142+huitseeker@users.noreply.github.com>
c5a5103
to
85d4897
Compare
@arthurpaulino thanks for noticing! Fixed this! |
Goal of this PR
The goal of this PR is to properly implement some standard Circom Gadget in our Lurk library, along with a Keccak example.
Current progress
CircomCoprocessor
to look for gadget, either via CLI or directly on a remote Github repositorykeccak-circom-gadget
repository based on the template.Left TODO
lurk-rs
to make it workCircomKeccak
implementingCircomGadget
directly in our repository.Current caveats
The goal of the following bullet points is to point out diverse part of the flow where I think I need external opinion before setting them in stone :)
CircomCoprocessor
looks for theCircomGadget
files locally before fetching them from the remote rpeository. This is a great simple caching solution BUT it might be too naïve because it does not emcompasss the version of the gadget. Thus if I update myCircomGadget
version it will still use the old one. Should I fix that?Related issue
This is part of tackling the issue #962