supranational / blst

Multilingual BLS12-381 signature library
Apache License 2.0
458 stars 175 forks source link

multi-exponentiation with single point is wrong #177

Closed vmx closed 1 year ago

vmx commented 1 year ago

I got a bug report on blstrs about multi-exponentiation returning the wrong result at https://github.com/filecoin-project/blstrs/issues/57. When I looked into it, I found that it's on the blst level.

Here's an example of a reproduction of the problem:

use blst::{
    blst_p1, blst_p1_affine, blst_p1_affine_generator, blst_p1_generator, blst_p1_mult,
    blst_p1_to_affine, p1_affines,
};

const NBITS: usize = 255;

fn main() {
    let a = unsafe { *blst_p1_generator() };
    let scalar_bytes = [
        1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
        0, 0,
    ];

    // Multiplying a single element works as expected.
    let mult = unsafe {
        let mut out = blst_p1::default();
        blst_p1_mult(&mut out, &a, &scalar_bytes as _, NBITS);
        out
    };
    println!("vmx: mult: {:?}", mult);
    let mult_affine = unsafe {
        let mut out = blst_p1_affine::default();
        blst_p1_to_affine(&mut out, &mult);
        out
    };
    println!("vmx: mult affine: {:?}", mult_affine);
    assert_eq!(mult_affine, unsafe { *blst_p1_affine_generator() });

    // Using multi_exp doesn't return the expected result.
    let points = p1_affines::from(&[a]);
    let multi_exp = points.mult(&scalar_bytes, NBITS);
    println!("vmx: multi_exp: {:?}", multi_exp);
    let multi_exp_affine = unsafe {
        let mut out = blst_p1_affine::default();
        blst_p1_to_affine(&mut out, &multi_exp);
        out
    };
    println!("vmx: multi_exp affine: {:?}", multi_exp_affine);
    assert_eq!(multi_exp_affine, mult_affine);
}

I then found that blst has tests for multi-exponentation, which makes the problem even easier to reproduce. Change this line https://github.com/supranational/blst/blob/327d30a51c858e9c34f5b6eb3a6966b2cf6bc9cc/bindings/rust/src/pippenger-test_mod.rs#L21 to const npoints: usize = 1; and you'll see the test fail when running cargo test p1_multi_scalar::test_mult.

dot-asm commented 1 year ago

Come on! It's in the name, multi, so 1 does not qualify-) But even on a serious note, formally speaking it shouldn't come as a surprise that a subroutine that expects multiple inputs doesn't handle one. But all right. I suppose if the library qsort works with a single input, then MSM could as well. Though the comparison is not really adequate. In the sense that if MSM did if (n ==1) return then you would complain, while qsort doing the same would qualify as doing-the-job. Sigh... Anyway, fixed.

vmx commented 1 year ago

I'm just forwarding bug reports ;) I agree though that it's meant for multiple things. Though it was still a bit unexpected to not getting an error, but really some unexpected result. Thanks for fixing!