scroll-tech / halo2-snark-aggregator

halo2 ecc circuit
Apache License 2.0
114 stars 24 forks source link

`reduce` before `sub` if needed #3

Open han0110 opened 2 years ago

han0110 commented 2 years ago

Currently the overflow of result in FiveColumnIntegerChip::sub is updated to be a.overflows + (b.overflows + 1) + 1, but this creates the possibility that overflow of result is equal to OVERFLOW_LIMIT, which causes panic in further operations.

https://github.com/scroll-tech/halo2-snark-aggregator/blob/35c449694fa91f40a6eb9939c06dbff95986d197/halo2-ecc-circuit-lib/src/five/integer_chip.rs#L652-L675

Not sure if it makes sense, but adding a check before sub should fix this:

 fn sub( 
     &self, 
     ctx: &mut Context<N>, 
     a: &AssignedInteger<W, N>, 
     b: &AssignedInteger<W, N>, 
 ) -> Result<AssignedInteger<W, N>, Error> { 
+    let a = if a.overflows + (b.overflows + 1) + 1 == OVERFLOW_LIMIT {
+        let mut a = a.clone();
+        self.reduce(ctx, &mut a)?;
+        a
+    } else {
+        a.clone()
+    };
     ...
}
xgaozoyoe commented 2 years ago

I think you are right. I think we do have lazy reduce policy some where, I will have a look.