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

424 presign testing #454

Merged
merged 3 commits into from
Aug 2, 2023
Merged

424 presign testing #454

merged 3 commits into from
Aug 2, 2023

Conversation

marsella
Copy link

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.

src/auxinfo/output.rs Show resolved Hide resolved
src/keygen/participant.rs Show resolved Hide resolved
src/presign/record.rs Show resolved Hide resolved
src/presign/record.rs Show resolved Hide resolved
Base automatically changed from 451-rearrange-presign to main July 27, 2023 13:41
Copy link

@hridambasu hridambasu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me so far!

Copy link

@gatoWololo gatoWololo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had one minor comment about rewriting one loop. Everything else looks good.

@marsella marsella marked this pull request as ready for review August 2, 2023 21:18
@marsella marsella merged commit 9d82421 into main Aug 2, 2023
2 checks passed
@marsella marsella deleted the 424-presign-testing branch August 2, 2023 21:19
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.

Add presign tests and testing infrastructure
3 participants