witnet-rust icon indicating copy to clipboard operation
witnet-rust copied to clipboard

Radon Mode operator only works correctly up to 127 equal elements

Open tmpolaczyk opened this issue 2 years ago • 1 comments

Because it uses a i8 counter internally, so it overflows on more than 127 equal elements:

https://github.com/witnet/witnet-rust/blob/c3b1d86ca229ebf8fe5651bfb35d8db9c902eb54/rad/src/reducers/mode.rs#L10

This affects all radon scripts, as well as the tally validation logic. In practice I believe this has not caused any issues yet because a data request with more than 127 witnesses is near the block weight limit, maybe even too large. The fix should be applied in a controlled manner to not cause any accidental hard forks, for example using TAPI with the next protocol upgrade, or a separate WIP.

These tests should pass:

// rad/src/reducers/mode.rs
#[test]
fn test_operate_reduce_mode_128_equal_elements() {
    // reduce::mode uses i8 to count frequency, so it only works when there are at most
    // 127 equal elements
    // Actually, this test passes if compiled in release mode
    let array1 = RadonArray::from(vec![RadonInteger::from(1i128).into()]);

    let input = RadonArray::from(vec![array1.clone().into(); 128]);

    let expected = RadonTypes::from(array1);
    let output = mode(&input).unwrap();
    assert_eq!(output, expected);
}

#[test]
fn test_operate_reduce_mode_128_equal_elements_and_1_different() {
    // reduce::mode uses i8 to count frequency, so it only works when there are at most
    // 127 equal elements
    // When compiled in debug mode this panics, and in release mode it returns "2".
    let array1 = RadonArray::from(vec![RadonInteger::from(1i128).into()]);

    let mut input_vec = vec![array1.clone().into(); 128];
    input_vec.push(RadonArray::from(vec![RadonInteger::from(2i128).into()]).into());
    let input = RadonArray::from(input_vec);

    let expected = RadonTypes::from(array1);
    let output = mode(&input).unwrap();
    assert_eq!(output, expected);
}

This is the output of the second test in release mode:

---- reducers::mode::tests::test_operate_reduce_mode_128_equal_elements_and_1_different stdout ----
thread 'reducers::mode::tests::test_operate_reduce_mode_128_equal_elements_and_1_different' panicked at 'assertion failed: `(left == right)`
  left: `Array(RadonArray { value: [Integer(RadonInteger { value: 2 })], is_homogeneous: true })`,
 right: `Array(RadonArray { value: [Integer(RadonInteger { value: 1 })], is_homogeneous: true })`', rad/src/reducers/mode.rs:208:9

tmpolaczyk avatar Apr 07 '22 15:04 tmpolaczyk

Nice catch! :kudos:

aesedepece avatar Apr 07 '22 16:04 aesedepece