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

Implement interactive signing #475

Merged
merged 5 commits into from
Aug 30, 2023
Merged

Implement interactive signing #475

merged 5 commits into from
Aug 30, 2023

Conversation

marsella
Copy link

Closes #467

This PR adds an implementation of interactive signing.

A couple discussion topics:

  • I feel like there's an argument for having the signing info be in a type that's either "partial input" or "SignParticipant", instead of the current setup where the participant is optional and we have to clone the partial input to use it (but we never use it again after that). Thoughts on making a bespoke little enum for that?
  • The input method on ProtocolParticipant wasn't being used in any protocol-participant-generic methods, so I removed it as a method on that trait. I did this because it doesn't make sense for this implementation to keep a separate copy of the private auxinfo and keygen material around in the InteractiveSignParticipant; better to just pass it to the internal PresignParticipant and wash our hands of it. However, that makes it hard to implement the input() method, so I removed it.
  • The Input type here needs a proper constructor. I was thinking of doing that in a separate PR just because this one is already big (I haven't written that issue yet).

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!

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.

The Input type here needs a proper constructor. I was thinking of doing that in a separate PR just because this one is already big (I haven't written that issue yet).

Makes sense.

Same for removing the input method.

Thoughts on making a bespoke little enum for that?

I agree! The enum makes sense for tracking the "state" and avoids the clone. (Even though, from what I can tell, this data isn't security critical). I tend to dislike Options when used as fields like in InteractiveSignParticipant

- Remove `input` method from protocol participant trait
- Add protocol implementation that dispatches to presign and NI sign
- Add a single happy test for interactive sign
@marsella
Copy link
Author

@gatoWololo I added said enum; there's a fairly ugly update method that I'd love your thoughts on. Updating a field in a mutable reference in-place is really awkward; I did it with a lot of mem::swaps

@gatoWololo
Copy link

@marsella Yeah, that's a hard one. Check out my branch: here I think it is slightly cleaner!

@marsella
Copy link
Author

Thanks @gatoWololo, I like your version much better. This is now ready for re-review.

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.

Looks good!

@marsella marsella merged commit 910e9d7 into main Aug 30, 2023
2 checks passed
@marsella marsella deleted the 467-implement-sign branch August 30, 2023 13:03
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.

Implement the interactive signing protocol
3 participants