Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HashMap parameters are checked for size, not for contents #479

Open
hdevalence opened this issue Aug 17, 2023 · 1 comment
Open

HashMap parameters are checked for size, not for contents #479

hdevalence opened this issue Aug 17, 2023 · 1 comment

Comments

@hdevalence
Copy link
Contributor

Several parts of the DKG API use HashMaps to pass a map that indexes some data by an Identifier. Surprisingly, these methods require that the map has a specific size, not just that it contains all of the relevant data. This makes test code much more verbose. For instance, we ended up writing

    // Round 1 is a broadcast, so it's enough to copy all the round 1 packages.
    let mut round2_secrets = HashMap::new();
    let mut round2_packages = HashMap::new();

    for id in &ids {
        let round1_secret = round1_secrets.remove(id).unwrap();

        let mut round1_packages_except_us = round1_packages.clone();
        round1_packages_except_us.remove(id);

        let (secret, packages) =
            frost::keys::dkg::part2(round1_secret, &round1_packages_except_us)?;
        round2_secrets.insert(*id, secret);
        round2_packages.insert(*id, packages);
    }

    // Round 2 is point-to-point (but we're faking it), so we need to
    // build a map of messages received by each participant.
    let mut shares = HashMap::new();
    let mut public_key_packages = HashMap::new();

    for id in &ids {
        let mut recvd_packages = HashMap::new();
        for (other_id, its_packages) in &round2_packages {
            if other_id == id {
                continue;
            }
            recvd_packages.insert(*other_id, its_packages.get(id).unwrap().clone());
        }

        let mut round1_packages_except_us = round1_packages.clone();
        round1_packages_except_us.remove(id);

        let round2_secret = round2_secrets.remove(id).unwrap();
        let (key_package, public_key_package) =
            frost::keys::dkg::part3(&round2_secret, &round1_packages_except_us, &recvd_packages)?;

        shares.insert(id, key_package);
        public_key_packages.insert(*id, public_key_package);
    }

While in a real application this might not be a problem, it's awkward to have to carefully remove elements from the HashMap in test code.

Instead, the methods could check that all of the required keys are present, and ignore any additional keys that it wouldn't be using.

@conradoplg
Copy link
Contributor

Thanks for the feedback, we're evaluating the best way to improve this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Product Backlog
Development

No branches or pull requests

2 participants