-
Notifications
You must be signed in to change notification settings - Fork 0
State Machine Complaint Flow Specification #61
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
Conversation
Summary of ChangesHello @nlordell, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness of the key generation process within the state machine by implementing a comprehensive complaint flow. It introduces new states and functions to manage accusations and responses during the collection and confirmation of secret shares. The author notes that the design of the complaint mechanism is largely stateless, which facilitated its integration with minimal additional state requirements, ultimately aiming to ensure the integrity and reliability of the key generation ceremony. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the key generation process within the validator.ml module, introducing a new rollover_secret_shares type and a Confirming_key_gen state to manage the collection and confirmation of secret shares. The spec/crypto.ml file was updated to change the secret_shares function to secret_share, now generating individual shares instead of a list. Key changes include the introduction of register_secret_share, key_gen_confirm, key_gen_complaint, and key_gen_complaint_response functions to handle the new key generation flow, including complaint mechanisms. A significant portion of the changes, highly praised by reviewers, involves replacing physical equality (==) with structural equality (=, GroupId.equal, SignatureId.equal, String.equal) across various comparisons, addressing potential bugs and improving code correctness and consistency.
This PR implements the complaint flow in the state machine. Note that we do not need to make use of the `KeyGenFinalized` event that occurs when all participants confirmed (cc @remedcu). Additionally, it turns out that the whole complaint flow is quite stateless (in that you respond to the accusations right away, or you collect the complaint responses into the existing secret share collection logic), making it fairly easy to graft on; with very little additional state required.
0df1968 to
0c18dea
Compare
| let participants' = | ||
| AddressSet.remove address st.group.participants | ||
| in | ||
| key_gen_and_commit state participants' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we only mark the participant as unhonest and trigger keygen only after the timeout (in case that there are more unhonest participants)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this in person, and ideally we should. I tried to implement this but it turns out to be a bit more complicated than expected. You can also get away Scott-free if you get a complaint but another participant times out secret sharing, and I want to deal with these cases more holistically in a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so do you want to keep this implementation, or do we go with that we handle a invalid response like not responded (so merging the two branches)?
Note: asking so that we can handle it the same way in the validator code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Captured here: #69
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would personally leave it as-is for now. There are a lot of other cases where we are suboptimal with removing as many dishonest participants as possible at once related to the complaint flow, and would tackle this separately.
I can add the "invalid responses are no responses" for now, but there are still cases where you might not remove the optimal number of dishonest participants.
In particular with the "invalid response is no response", then a dishonest participant may not be removed if they provide an invalid complaint response but another participant times out during secret sharing. I think for this to be implemented correctly, we need to track more state and potentially make the state machine handle these edge cases better.
This PR implements the complaint flow in the state machine.