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

fix: use all refresh msgs for combining new keyshares #30

Merged
merged 3 commits into from
Oct 11, 2023

Conversation

ivokub
Copy link
Contributor

@ivokub ivokub commented Oct 6, 2023

Summary of changes

In FS-DKR subprotocol for key refresh, the parties secret share their current private share and send the corresponding shares to other parties who then recombine their shares to get their new private share.

However, in the current implementation, all parties only use first current_t+1 received shares to combine their new secret share. The new private shares are valid (they are secret shared parts of the global private key) due to SS, but they do not include the contribution of all parties taking part in the key refresh round. This degrades the entropy of the private shares.

This PR changes the behaviour of the parties to take into account of all received messages. Additionally I removed the threshold argument in methods which do not need it anymore.

This PR corresponds to webb-tools/fs-dkr#16 with also using all refresh messages in join messages.

The tests mostly succeed. May it be that #28 affects also refresh tests?

@drewstone
Copy link
Contributor

drewstone commented Oct 7, 2023

Webassembly check failing, unsure why since it's deep in the crates.

@tmpfs
Copy link
Contributor

tmpfs commented Oct 9, 2023

@ivokub @drewstone , so this webassembly error I resolved twice, once when using the nightly compiler by removing the reference (ZenGo-X/curv#183) but after I migrated to the stable toolchain the problem was resolved so I moved on (FWIW that fix for nightly does not work on stable).

The github runner appears to be installing the stable toolchain so I am not sure what's happening here.

Will compare to the successful runs on my earlier PR to see if anything changed.

Edit: the earlier build that passed on CI was on nightly, looks like I got it wrong, need to look into it more.

@tmpfs
Copy link
Contributor

tmpfs commented Oct 9, 2023

Ok ,this may be only on Linux both of these work for me locally on Apple silicon:

          cargo check --target wasm32-unknown-unknown \
            --features js \
            --features num-bigint \
            --no-default-features
          cargo +nightly check --target wasm32-unknown-unknown \
            --features js \
            --features num-bigint \
            --no-default-features

@tmpfs
Copy link
Contributor

tmpfs commented Oct 9, 2023

Yep, it's Linux specific I just managed to reproduce on a Linux box using the stable and the nightly toolchain - which is confusing me even more now!

@tmpfs
Copy link
Contributor

tmpfs commented Oct 9, 2023

Ok, so this is a real mess, the fix on Linux is to remove the reference as it uses div_ceil() core/src/num/mod.rs which does not expect a reference. However, that breaks MacOS which is using div_ceil() from the num-integer crate which expects a reference.

We can fix this with conditional compilation (but I suspect there is a much better fix - like not using the num-integer crate at all) but it raises a bigger question @drewstone about what to do about the curv-kzen library which we are heavily dependent on.

From what I can tell it does not seem to be maintained and there is an important issue which @ivokub and I discussed (ZenGo-X/curv#177) that we should implement to make the WASM version more robust (see the warning here) and ideally pave the way to removing the native GMP bindings.

So the question then becomes do you want me to fix this and we use [patch.crates-io] to apply the patch or should we just fork curv-kzen and move on? Becuase this is deep in the dependency tree it implies we also need to fork all the paillier libraries.

What do you think?

@tmpfs
Copy link
Contributor

tmpfs commented Oct 10, 2023

@drewstone, can you please review and merge this if you are happy and I will fix the webassembly checks in a subsequent PR please?

@drewstone drewstone merged commit 448296d into webb-tools:main Oct 11, 2023
4 of 5 checks passed
@ivokub ivokub deleted the fix/all-refresh-msgs branch October 23, 2023 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants