Wait for Manual KeyGen for Triggering Genesis #63
Conversation
This PR changes the strategy for doing genesis to wait for the `KeyGen` event to be manually triggered. With this change, we no longer need to start indexing from block 0 and can just start with `latest`. Now, the genesis process looks a bit different: 1. Each validator starts with their parameters 2. Someone manually triggers a `keyGen` call on the coordinator contract in order to kick off the process 3. Profit $$$ This simplifies the startup significantly and no longer requires nodes to support querying events from huge block ranges, or taking along time to get to the current block (with the downside of a more manual genesis process).
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 introduces a significant change to the genesis process, shifting from an automatic key generation to one that is manually triggered by a 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the genesis process to be manually triggered via a KeyGen event, which simplifies the node's startup by allowing it to sync from the latest block instead of from genesis. The changes are well-structured and include beneficial refactorings, such as improving contract interaction logic in tests for better clarity and centralizing the calculation of group parameters. My review includes a couple of suggestions to further enhance maintainability and robustness.
| await initiatorClient.writeContract({ | ||
| ...coordinator, | ||
| functionName: "keyGen", | ||
| args: [calculateParticipantsRoot(participants), 3n, 2n, zeroHash], | ||
| }); |
There was a problem hiding this comment.
To improve maintainability and avoid magic numbers, it's better to derive the count and threshold values from the participants array dynamically rather than hardcoding them. This will make the test more robust if the number of participants changes in the future.
const count = BigInt(participants.length);
const threshold = count / 2n + 1n;
await initiatorClient.writeContract({
...coordinator,
functionName: "keyGen",
args: [calculateParticipantsRoot(participants), count, threshold, zeroHash],
});| export const calcGroupParameters = (participantCount: number, consensus: Address, epoch: bigint): GroupParameters => { | ||
| const count = BigInt(participantCount); | ||
| const threshold = count / 2n + 1n; | ||
| const context = encodePacked(["uint32", "address", "uint64"], [0, consensus, epoch]); | ||
|
|
||
| // TODO: Handle cases where the group size is too small. | ||
| return { count, threshold, context }; | ||
| }; |
There was a problem hiding this comment.
The TODO comment on line 18 correctly points out that cases with small group sizes are not handled. To make this function more robust and self-contained, it would be beneficial to add a validation check at the beginning to ensure participantCount is sufficient (e.g., at least 2). This aligns with the requirements of the FROSTCoordinator contract and centralizes the validation logic, preventing potential issues in different call sites.
export const calcGroupParameters = (participantCount: number, consensus: Address, epoch: bigint): GroupParameters => {
if (participantCount < 2) {
// TODO: Use a more specific error type
throw new Error("Group size must be at least 2");
}
const count = BigInt(participantCount);
const threshold = count / 2n + 1n;
const context = encodePacked(["uint32", "address", "uint64"], [0, consensus, epoch]);
return { count, threshold, context };
};
| case "event_transaction_attested": { | ||
| return await handleTransactionAttested(machineStates, transition); | ||
| } | ||
| case "event_key_gen": |
This PR changes the strategy for doing genesis to wait for the
KeyGenevent to be manually triggered. With this change, we no longer need to start indexing from block 0 and can just start withlatest.Now, the genesis process looks a bit different:
keyGencall on the coordinator contract in order to kick off the processThis simplifies the startup significantly and no longer requires nodes to support querying events from huge block ranges, or taking along time to get to the current block (with the downside of a more manual genesis process).