tangle-network / cggmp-threshold-ecdsa

MPC protocols for threshold ECDSA
GNU General Public License v3.0
46 stars 10 forks source link

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

Closed ivokub closed 1 year ago

ivokub commented 1 year ago

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 https://github.com/webb-tools/fs-dkr/pull/16 with also using all refresh messages in join messages.

The tests mostly succeed. May it be that https://github.com/webb-tools/cggmp-threshold-ecdsa/issues/28 affects also refresh tests?

drewstone commented 1 year ago

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

tmpfs commented 1 year ago

@ivokub @drewstone , so this webassembly error I resolved twice, once when using the nightly compiler by removing the reference (https://github.com/ZenGo-X/curv/pull/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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 (https://github.com/ZenGo-X/curv/issues/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 commented 1 year ago

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