Skip to content
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

o1vm/mips: fix failing constraints #2729

Merged
merged 4 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions o1vm/src/interpreters/mips/constraints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,22 @@ impl<Fp: Field> InterpreterEnv for Env<Fp> {
self.variable(position)
}

fn is_zero(&mut self, x: &Self::Variable) -> Self::Variable {
let res = {
let pos = self.alloc_scratch();
unsafe { self.test_zero(x, pos) }
};
let x_inv_or_zero = {
let pos = self.alloc_scratch();
unsafe { self.inverse_or_zero(x, pos) }
};
// If x = 0, then res = 1 and x_inv_or_zero = 0
// If x <> 0, then res = 0 and x_inv_or_zero = x^(-1)
self.add_constraint(x.clone() * x_inv_or_zero.clone() + res.clone() - Self::constant(1));
self.add_constraint(x.clone() * res.clone());
res
}

unsafe fn inverse_or_zero(
&mut self,
_x: &Self::Variable,
Expand Down
16 changes: 1 addition & 15 deletions o1vm/src/interpreters/mips/interpreter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -698,21 +698,7 @@ pub trait InterpreterEnv {
position: Self::Position,
) -> Self::Variable;

fn is_zero(&mut self, x: &Self::Variable) -> Self::Variable {
let res = {
let pos = self.alloc_scratch();
unsafe { self.test_zero(x, pos) }
};
let x_inv_or_zero = {
let pos = self.alloc_scratch();
unsafe { self.inverse_or_zero(x, pos) }
};
// If x = 0, then res = 1 and x_inv_or_zero = 0
// If x <> 0, then res = 0 and x_inv_or_zero = x^(-1)
self.add_constraint(x.clone() * x_inv_or_zero.clone() + res.clone() - Self::constant(1));
self.add_constraint(x.clone() * res.clone());
res
}
fn is_zero(&mut self, x: &Self::Variable) -> Self::Variable;

/// Returns 1 if `x` is equal to `y`, or 0 otherwise, storing the result in `position`.
fn equal(&mut self, x: &Self::Variable, y: &Self::Variable) -> Self::Variable;
Expand Down
43 changes: 37 additions & 6 deletions o1vm/src/interpreters/mips/witness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -361,13 +361,44 @@ impl<Fp: Field, PreImageOracle: PreImageOracleT> InterpreterEnv for Env<Fp, PreI
}
}

fn equal(&mut self, x: &Self::Variable, y: &Self::Variable) -> Self::Variable {
// To avoid subtraction overflow in the witness interpreter for u32
if x > y {
self.is_zero(&(*x - *y))
fn is_zero(&mut self, x: &Self::Variable) -> Self::Variable {
// write the result
let pos = self.alloc_scratch();
let res = if *x == 0 { 1 } else { 0 };
self.write_column(pos, res);
// write the non deterministic advice inv_or_zero
let pos = self.alloc_scratch();
let inv_or_zero = if *x == 0 {
Fp::zero()
} else {
self.is_zero(&(*y - *x))
}
Fp::inverse(&Fp::from(*x)).unwrap()
};
self.write_field_column(pos, inv_or_zero);
// return the result
res
}

fn equal(&mut self, x: &Self::Variable, y: &Self::Variable) -> Self::Variable {
// We replicate is_zero(x-y), but working on field elt,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it's less error prone if you move the is_zero() in interpreter.rs into constraint.rs and define a variant of is_zero() in witness.rs containing precisely your replication here in equal(). Otherwise, one could call the is_zero() from interpreter.rs for the witness and incur in the same problem being fixed in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed it is more clean

// to avoid subtraction overflow in the witness interpreter for u32
let to_zero_test = Fp::from(*x) - Fp::from(*y);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to convert as early as here in Fp.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, but I believe this to be more clear, and the performance is negligibly impacted

let res = {
let pos = self.alloc_scratch();
let is_zero: u64 = if to_zero_test == Fp::zero() { 1 } else { 0 };
self.write_column(pos, is_zero);
is_zero
};
let _to_zero_test_inv_or_zero = {
let pos = self.alloc_scratch();
let inv_or_zero = if to_zero_test == Fp::zero() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably call inverse_or_zero in these lines, no? I guess the bug comes from the res block, there's no need to unroll the inv_or_zero to solve the bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought so at first as well, but Inverse_or_zero is forced to return a Variable by the signature, so we can't get the resulting field_inversion we desire here.

Fp::zero()
} else {
Fp::inverse(&to_zero_test).unwrap()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See a direct optimisation available here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here, built on top of that one

};
self.write_field_column(pos, inv_or_zero);
1 // Placeholder value
};
res
}

unsafe fn test_less_than(
Expand Down
18 changes: 1 addition & 17 deletions o1vm/src/pickles/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use o1vm::{
constraints as mips_constraints,
interpreter::{self, InterpreterEnv},
witness::{self as mips_witness},
ITypeInstruction, Instruction, RTypeInstruction,
Instruction,
},
pickles::{
proof::{Proof, ProofInputs},
Expand Down Expand Up @@ -78,27 +78,11 @@ pub fn main() -> ExitCode {
let mut mips_wit_env =
mips_witness::Env::<Fp, PreImageOracle>::create(cannon::PAGE_SIZE as usize, state, po);

// FIXME: This is a hack to skip some instructions that have failing constraints.
// It might be related to env.equal.
let failing_instructions = [
Instruction::RType(RTypeInstruction::SyscallOther),
Instruction::RType(RTypeInstruction::SyscallFcntl),
Instruction::IType(ITypeInstruction::BranchEq),
Instruction::IType(ITypeInstruction::BranchNeq),
Instruction::IType(ITypeInstruction::LoadWordLeft),
Instruction::IType(ITypeInstruction::LoadWordRight),
Instruction::IType(ITypeInstruction::StoreWordLeft),
Instruction::IType(ITypeInstruction::StoreWordRight),
];
let constraints = {
let mut mips_con_env = mips_constraints::Env::<Fp>::default();
let mut constraints = Instruction::iter()
.flat_map(|instr_typ| instr_typ.into_iter())
.fold(vec![], |mut acc, instr| {
if failing_instructions.contains(&instr) {
debug!("Skipping instruction {:?}", instr);
return acc;
}
interpreter::interpret_instruction(&mut mips_con_env, instr);
let selector = mips_con_env.get_selector();
let constraints_with_selector: Vec<E<Fp>> = mips_con_env
Expand Down
Loading