-
Notifications
You must be signed in to change notification settings - Fork 4
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/circuit-order: Reliable order of circuits in proofs #522
Changes from 7 commits
23cb06c
7134f60
9c5cd99
458ec44
edffdf5
854b65c
1f46d74
5781d19
60ace44
b72fc02
7b40c48
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -205,7 +205,8 @@ impl<E: ExtensionField> ZKVMFixedTraces<E> { | |
|
||
#[derive(Default)] | ||
pub struct ZKVMWitnesses<E: ExtensionField> { | ||
pub witnesses: BTreeMap<String, RowMajorMatrix<E::BaseField>>, | ||
witnesses_opcodes: BTreeMap<String, RowMajorMatrix<E::BaseField>>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't we want the kinds instead of the opcodes? Or what does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There can be other circuits despite the name. It is called "opcode" everywhere until we find a more precise name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So what's actually in there? The instructions or something else? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's what the caller of this type passed in assign_opcode_circuit, respectively assign_table_circuit. |
||
witnesses_tables: BTreeMap<String, RowMajorMatrix<E::BaseField>>, | ||
lk_mlts: BTreeMap<String, LkMultiplicity>, | ||
combined_lk_mlt: Option<Vec<HashMap<u64, usize>>>, | ||
} | ||
|
@@ -222,7 +223,8 @@ impl<E: ExtensionField> ZKVMWitnesses<E> { | |
let cs = cs.get_cs(&OC::name()).unwrap(); | ||
let (witness, logup_multiplicity) = | ||
OC::assign_instances(config, cs.num_witin as usize, records)?; | ||
assert!(self.witnesses.insert(OC::name(), witness).is_none()); | ||
assert!(self.witnesses_opcodes.insert(OC::name(), witness).is_none()); | ||
assert!(!self.witnesses_tables.contains_key(&OC::name())); | ||
assert!( | ||
self.lk_mlts | ||
.insert(OC::name(), logup_multiplicity) | ||
|
@@ -273,10 +275,18 @@ impl<E: ExtensionField> ZKVMWitnesses<E> { | |
self.combined_lk_mlt.as_ref().unwrap(), | ||
input, | ||
)?; | ||
assert!(self.witnesses.insert(TC::name(), witness).is_none()); | ||
assert!(self.witnesses_tables.insert(TC::name(), witness).is_none()); | ||
assert!(!self.witnesses_opcodes.contains_key(&TC::name())); | ||
|
||
Ok(()) | ||
} | ||
|
||
/// Iterate opcode circuits, then table circuits, sorted by name. | ||
pub fn into_iter_sorted(self) -> impl Iterator<Item = (String, RowMajorMatrix<E::BaseField>)> { | ||
self.witnesses_opcodes | ||
naure marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.into_iter() | ||
.chain(self.witnesses_tables) | ||
} | ||
} | ||
|
||
#[derive(Debug)] | ||
|
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.
Isn't that just
first
?Seems a bit weird in any case, why not just ask for the one we know we want by 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.
Or
exactly_once
but somehow didn't work.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.
Ok, I'll have a look.