Share recovery failure at a random point when using new_decryption_share

[aramikm]

Description

test_dkg_simple_tdec_share_recovery doesn't seem to use the new_decryption_share when combining shares to get new_shared_secret.

This is evident in following code section which always uses the old shares.

If we modify the test as below to use to use the new_decryption_share

        decryption_shares.push(new_decryption_share);
        domain_points.insert(new_validator_share_index, x_r);

        let domain_points = domain_points
            .values()
            .take(security_threshold as usize)
            .cloned()
            .collect::<Vec<_>>();
    //    let decryption_shares =
    //        &decryption_shares[..security_threshold as usize];   since this slice does not include the new_decryption_share let's modify it to get included for combine_shares_simple
        let decryption_shares =
            &decryption_shares[(decryption_shares.len() - security_threshold as usize)..]; // this is the new line replacing the commented line above
        assert_eq!(domain_points.len(), security_threshold as usize);
        assert_eq!(decryption_shares.len(), security_threshold as usize);

        let new_shared_secret = combine_shares_simple(decryption_shares);

3 of the test cases fail with following message Shared secret reconstruction failed', ferveo/src/api.rs:1257:9 which might be an indication that the share recovery at a random point doesn't work as expected.

failures:
    api::test_ferveo_api::test_dkg_simple_tdec_share_recovery::number_of_shares_validators_is_a_power_of_2
    api::test_ferveo_api::test_dkg_simple_tdec_share_recovery::number_of_shares_validators_is_not_a_power_of_2
    api::test_ferveo_api::test_dkg_simple_tdec_share_recovery::number_of_validators_greater_than_the_number_of_shares

test result: FAILED. 1 passed; 3 failed; 0 ignored; 0 measured; 59 filtered out; finished in 0.52s

[cygnusv]

Thanks for spotting this! Note that you're referring to a test in a PR that's still WIP, we're currently on a deep overhauling of the refresh & recovery process. The Spongebob PR (#186 ) in fact is refactoring all the code and tests related to this with a new design. The original test in the current main branch seems OK to me:

ferveo/ferveo/src/lib.rs

Line 525 in 2fe8753

let decryption_shares = &decryption_shares[1..];

Having said that, this is a great issue. I recall we had a similar problem in the past with other refresh & recovery tests. I'll keep this issue open and see that #186 closes it.