Fixes to shamir

[mikelodder7]

The shamir methods fail running the following tests

• Threshold can be specified as 1, which doesn't matter in a threshold setting
• Duplicate share id's
• Share id of zero.

#[test]
    #[should_panic]
    fn invalid_case() {
        let mut rng = StdRng::seed_from_u64(0u64);
        // Shouldn't allow sharing threshold of 1 but succeeds
        let (secret, shares, poly) = deal_random_secret::<_, Fr>(&mut rng, 1, 1).unwrap();
        assert_eq!(shares.0.len(), 1);
        assert_eq!(secret, shares.0[0].share);
        assert_eq!(poly.degree(), 0);
    }

    #[test]
    fn invalid_recombine_dup_id() {
        let mut rng = StdRng::seed_from_u64(0u64);
        let (secret, mut shares, poly) = deal_random_secret::<_, Fr>(&mut rng, 3, 3).unwrap();
        shares.0[1].id = 1;
        // Should fail because of duplicate share id. Duplicate share id's result in lagrange divide by zero
        assert!(shares.reconstruct_secret().is_err());
        let secret1 = shares.reconstruct_secret().unwrap();
        assert_eq!(secret, secret1);
    }


    #[test]
    fn invalid_recombine_zero_id() {
        let mut rng = StdRng::seed_from_u64(0u64);
        let (secret, mut shares, poly) = deal_random_secret::<_, Fr>(&mut rng, 2, 3).unwrap();
        shares.0[0].id = 0;
        // Should fail because of zero share id. Zero id results in lagrange multiply by zero
        // which nullifies the share
        // assert!(shares.reconstruct_secret().is_err());
        let secret1 = shares.reconstruct_secret().unwrap();
        // shouldn't happen
        assert_eq!(secret1, shares.0[0].share * lagrange_basis_at_0::<Fr>(&[0, 2], 0));
    }

[lovesh]

// Shouldn't allow sharing threshold of 1 but succeeds
let (secret, shares, poly) = deal_random_secret::<_, Fr>(&mut rng, 1, 1).unwrap();

The problem isn't the threshold but total, which shouldn't be 1. deal_random_secret::<_, Fr>(&mut rng, 1, 3).unwrap(); is fine. Adding a check for total. Thanks.

fn invalid_recombine_dup_id() {
....
// Should fail because of duplicate share id. Duplicate share id's result in lagrange divide by zero

reconstruct_secret expects the shares to be unique as its comment says.

fn invalid_recombine_dup_id() {
....
assert_eq!(secret, secret1);

This check fails. What was the intention here?

#[test]
fn invalid_recombine_zero_id() {

Thanks for reporting this. Fixing.

[mikelodder7]

Expecting the comments to catch bugs is not good practice.

[mikelodder7]

Yes that test intentionally fails and will pass once you fix the bug

[lovesh]

Expecting the comments to catch bugs is not good practice.

I am not expecting the comment to catch the bug but was setting the expectation through it.