-
Notifications
You must be signed in to change notification settings - Fork 113
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
Arrabbiata: introduce the structure Program #3075
Conversation
The structure Program would encapsulate all the information reg. an element of the NP relation. In other words, it contains all the public and private values, including the challenges. It will also contain the input and the expected output that will be passed in public values. In addition to that, the accumulations will also be kept. The environment used by the interpreter will work with two different instances of the program structure, one for each curve cycle.
6c6abf0
to
117d96d
Compare
Fp: PrimeField, | ||
Fq: PrimeField, |
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.
as discussed offline, I would remove these fields
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.
If I change (in setup.rs
) to:
pub struct IndexedRelation<
E1: ArrabbiataCurve<ScalarField = E2::BaseField, BaseField = E2::ScalarField>,
E2: ArrabbiataCurve<ScalarField = E1::BaseField, BaseField = E1::ScalarField>,
> where
E1::BaseField: PrimeField,
E2::BaseField: PrimeField,
I get a circular dependency (which makes sense). The fields seem to be required to avoid the circular dependency.
pub struct Program< | ||
Fp: PrimeField, | ||
Fq: PrimeField, | ||
E1: ArrabbiataCurve<ScalarField = Fp, BaseField = Fq>, |
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: rename to E, E1 must be a copy paste from the preceeding code
I didn't review in details the methods for the program struct, as I assume these were copy pasted from the env method, which have been reviewed |
sponge_state.iter().for_each(|x| { | ||
E1::absorb_fq( | ||
&mut sponge, | ||
E1::BaseField::from_biguint(&x.to_biguint().unwrap()).unwrap(), |
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 conversion seems supicious
I understand it is to go from one field to another ?
But if our challenge are 128 bits we should not need it.
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 doesn't go from one field to another. It converts into base field elements the state of the sponge that is passed as an argument, previously saved as base field elements. Again, the usage of BigInt
would need to be revisited.
However, I thought that absorbing three times would set the state to the values we passed without running the cipher, but it is wrong:
fn absorb(&mut self, x: &[F]) {
for x in x.iter() {
match self.sponge_state {
SpongeState::Absorbed(n) => {
if n == self.rate {
self.poseidon_block_cipher();
self.sponge_state = SpongeState::Absorbed(1);
self.state[0].add_assign(x);
} else {
self.sponge_state = SpongeState::Absorbed(n + 1);
self.state[n].add_assign(x);
}
}
SpongeState::Squeezed(_n) => {
self.state[0].add_assign(x);
self.sponge_state = SpongeState::Absorbed(1);
}
}
}
}
it does absorb twice and after that run the cipher.
I'm opening a follow-up (as unrelated to this PR as it is mostly moving code).
I will revisit the sponge state management in a future PR.
No description provided.