Skip to content

Fixes to shamir #23

Open
Open

Description

@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));
    }

Activity

  1. lovesh commented on May 17, 2024

    @lovesh
    Collaborator

    // 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.

  2. mikelodder7 commented on May 17, 2024

    @mikelodder7
    Author

    Expecting the comments to catch bugs is not good practice.

  3. mikelodder7 commented on May 17, 2024

    @mikelodder7
    Author

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

  4. lovesh commented on May 17, 2024

    @lovesh
    Collaborator

    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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions