boltlabs-inc / tss-ecdsa

An implementation of a threshold ECDSA signature scheme
Other
12 stars 5 forks source link

424 presign testing #454

Closed marsella closed 1 year ago

marsella commented 1 year ago

Closes #424

This adds a basic happy test for presigning, defines the characteristics that a valid set of PresignRecords should satisfy, and adds a method to simulate a valid presign run.

There's lots of boilerplate to get the presign test running; this is mostly copied from the equivalent keygen and auxinfo tests. I think some of this could be extracted into a testing utility module because many of the functionalities (e.g. maintain an inbox to store and "send" messages) are the same across different participant types. I also think some of them could be simplified (e.g. we don't need separate inboxes for each party; we can just make one big inbox for all messages). But I refrain from addressing that here.

There's a bit of rearranging of code, like for the simulate methods on keygen and auxinfo, which I just moved into the test module so they're more obviously not-for-general-use. I'll mark the big chunks of moved code inline. I also had to add some functionality for simulating auxinfo and keygen output; before, we had methods to create a single valid instance; now we needed a method to create a valid set. The simulate and simulate_set methods have a lot of similar code, but they take slightly different inputs and I didn't figure out a nice way to consolidate them.

The most important cryptography component for @hridambasu to review is the tests on PresignRecord. The paper never explicitly defines what properties a record needs to hold. I generated the properties here by reading it and also reading @amaloz's excellent documentation on the PresignParticipant type. But I would like you read the same things and think about whether there seem to be any other conditions that should hold for correctly-generated records.

One other point of interest: in testing some of the presign stuff, I realized that the CurvePoint::multiply_by_scalar method is actually multiplying by a BigNumber which it converts to a Scalar. This raised a few code quality concerns:

  1. The bn_to_scalar method it uses has some cloning of the input; we often call this method on secrets so this is a potential point of leakage we should address
  2. In fact, there are a variety of secret and intermediate types across the protocols that are drawn from š¯”½_q. The paper isn't explicit about what that means, but š¯”½_q is (by definition) the scalar field of the elliptic curve. I think it's worth looking through these types to see if some of our BigNumbers can / should actually be k256::Scalars.

For the purposes of this PR, I just changed the name to multiply_by_bignumber and made a separate, infallible multiply_by_scalar method to make the types line up more nicely. This is why there are so many changed files... sorry. But perhaps in the future we will find that multiply_by_scalar is the correct method to be using and we just have some types wrong.