Refactor State Machine Specification #60

nlordell · 2025-12-04T20:20:58Z

This refactor fixes some of the pending issues that we had previously identified. Namely:

We have a minimum group size (derived from the size of the total participation set).
We skip key generation and signing ceremonies if we do not have a large enough participant set or signer selection respectively. This adds a "skipped" state to epoch rollove to indicate that we should wait until the next epoch to try again. (We do not need an additional skipped state for signatures, we just remove them from state if they are dropped).
Preprocess is now an Option instead of a Map, this is because the state machine only allows a single pending nonces chunk per group, and it simplifies the spec a bit.
The responsible and last_participant are now Options, where None is used to indicate that "everyone is responsible".
Old nonces are garbage-collected
All cryptography and Ethereum specific code has been moved into their own modules to not clutter the Validator module.
Stronger typing on group and signature IDs.

This is a pre-amble to the additions for the new complaint flow. The refac

gemini-code-assist · 2025-12-04T20:21:37Z

Summary of Changes

Hello @nlordell, I'm Gemini Code Assist¹! 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 refactoring of the state machine specification, primarily focusing on improving modularity, type safety, and the robustness of cryptographic ceremonies. Key changes include the extraction of cryptographic and Ethereum-specific logic into dedicated modules, stricter enforcement of participant group sizes for key generation and signing, and a more streamlined handling of preprocessing and participant roles. These updates lay foundational improvements for future features like the complaint flow, making the state machine more maintainable and resilient.

Highlights

Minimum Group Size Enforcement: Key generation and signing ceremonies now enforce a minimum participant group size, derived from the total participation set, ensuring robust consensus and preventing operations with insufficient participants.
Conditional Ceremony Execution: Key generation and signing ceremonies are now conditionally skipped if the participant set or signer selection is insufficient, preventing unnecessary processing and potential failures.
Simplified Preprocessing State: The preprocess state has been refactored to use an Option instead of a Map for pending nonces, reflecting that the state machine only allows a single pending nonces chunk per group, which simplifies the specification.
Optional Participant Roles: The responsible and last_participant fields are now Option types, allowing None to explicitly indicate scenarios where "everyone is responsible" or a participant is not yet assigned, improving clarity.
Modular Cryptography and Ethereum Logic: All cryptography and Ethereum-specific code has been extracted into dedicated Crypto and Eth modules, significantly reducing clutter and improving modularity within the Validator module.
Enhanced Type Safety for IDs: Group and signature identifiers now benefit from stronger typing through dedicated Id.Make modules, improving code clarity and preventing type-related errors.
Introduction of Local State Marker: A new 'a local type has been introduced to explicitly mark state components that are local to a specific validator and known only by them, enhancing state management clarity.
Garbage Collection for Preprocess Data: A new garbage_collect function has been added to manage and clean up preprocess data, ensuring that only relevant group data is retained and reducing memory footprint.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces a significant and valuable refactoring of the state machine specification. The changes successfully move cryptographic and Ethereum-specific logic into separate modules, introduce stronger typing for IDs, and refine the state machine logic as described. These improvements enhance modularity, type safety, and clarity. My review has identified a critical bug related to variable shadowing that would prevent secret shares from being distributed, as well as several high-severity issues where physical equality (==) was used instead of structural equality for abstract types, which could lead to incorrect behavior. I have also included a few medium-severity suggestions to improve code correctness and consistency. Overall, this is a solid refactoring, and addressing these points will make it even more robust.

spec/validator.ml

spec/crypto.ml

spec/validator.ml

This refactor fixes some of the pending issues that we had previously identified. Namely: 1. We have a minimum group size (derived from the size of the total participation set). 2. We skip key generation and signing ceremonies if we do not have a large enough participant set or signer selection respectively. 3. Preprocess is now an `Option` instead of a `Map`, this is because the state machine only allows a single pending nonces chunk per group, and it simplifies the spec a bit. 4. The `responsible` and `last_participant` are now `Option`s, where `None` is used to indicate that "everyone is responsible". 5. All cryptography and Ethereum specific code has been moved into their own modules to not clutter the `Validator` module. 6. Stronger typing on group and signature IDs. This is a pre-amble to the additions for the new complaint flow. The refac

rmeissner · 2025-12-05T08:13:01Z

spec/validator.ml

 type preprocess_group = {
-  nonces : FROST.nonces IntMap.t;
-  pending : FROST.nonces list StringMap.t;
+  nonces : (FROST.nonces * MerkleTree.proof) IntMap.t;


what does the * do here and in the next line? What would be the corresponding ts type?

It's a product type - i.e. a tuple.

So, (int * string) is a type for int-string tuple (42, "the answer").

spec/validator.ml

rmeissner · 2025-12-05T08:15:15Z

spec/validator.ml

  rollover : rollover;
-  preprocess : preprocess_group StringMap.t;
-  signing_ceremonies : signing_ceremony StringMap.t;
+  preprocess : preprocess_group GroupMap.t local;


what does the "local" do here?

shieldnet/spec/validator.ml

Lines 36 to 38 in 6927202

(** A marker type to indicate state that is local to a specific validator and

only known by them. *)

type 'a local = Local of 'a

It just an indicator that the state is only known to the specific validator and not globally derived public state.

rmeissner · 2025-12-05T08:18:55Z

spec/validator.ml

+  val account : Address.t
  val blocks_per_epoch : int
-  val all_participants : Address.t list
+  val all_participants : AddressSet.t


nit: for me a set is not sorted, afaik we do require the participant set to be in some specific order. but I didn't check in detail if the sorting happens elsewhere (couldn't quickly find it)

It's guaranteed by the Set type in OCaml (it's implemented as binary-tree set specifically). Added a comment in the relevant sections so that it is clear that sorting matters.

rmeissner · 2025-12-05T08:20:40Z

spec/validator.ml


-  let assert_assumption c m = if c then () else failwith m
-  let account_address = Secp256k1.addr Configuration.account
+  let participant_identifier participants address =


This (so line 148 to 152) is super hard to understand as I am lacking the understanding of the syntax

I will add a comment to explain what the function does.

rmeissner · 2025-12-05T08:23:08Z

spec/validator.ml

+      (fun id _ set -> ParticipantSet.add id set)
+      items ParticipantSet.empty
+
+  let group_threshold count =


note: It would be nice to document the logic about the group size and threshold in "human readable" terms somewhere too

rmeissner · 2025-12-05T08:28:03Z

spec/validator.ml

+      items ParticipantSet.empty
+
+  let group_threshold count =
+    let ( /^ ) q d = (q + d - 1) / d in


this is Math.ceil?

Its ceiling division on integers. Clarified in the code.

rmeissner · 2025-12-05T08:29:58Z

spec/validator.ml

+        in
+        (state', actions)
+    | _ ->
+        let state' = { state with rollover = Skipped { epoch } } in


A comment here would be good (similar to how it was done for the signing flow https://github.com/safe-research/shieldnet/pull/60/files#diff-27235707e04ce8bc66a78e46551b0f782475b9fcd817f491fbfc5d546583cd9cR733-R734)

rmeissner · 2025-12-05T08:31:07Z

spec/validator.ml

    | Collecting_key_gen_secret_shares { epoch; _ }
-    | Signing_epoch_rollover { epoch = { id = epoch; _ }; _ }
+    | Signing_epoch_rollover { epoch = { epoch; _ }; _ }
+    | Skipped { epoch }


Important reminder: This is a new state that we need to implement in the validator

nlordell requested a review from a team as a code owner December 4, 2025 20:20

nlordell force-pushed the pr/state-machine-spec-v2 branch from cc4d908 to 33d9da7 Compare December 4, 2025 20:21

gemini-code-assist bot reviewed Dec 4, 2025

View reviewed changes

nlordell force-pushed the pr/state-machine-spec-v2 branch from 33d9da7 to cc3dc1c Compare December 4, 2025 20:24

comments

6927202

rmeissner reviewed Dec 5, 2025

View reviewed changes

spec/validator.ml Outdated Show resolved Hide resolved

rmeissner reviewed Dec 5, 2025

View reviewed changes

spec/validator.ml Outdated Show resolved Hide resolved

rmeissner reviewed Dec 5, 2025

View reviewed changes

rmeissner approved these changes Dec 5, 2025

View reviewed changes

nlordell added 2 commits December 5, 2025 11:24

fix equality

8943484

comments

4c5d43c

nlordell merged commit 4321324 into main Dec 5, 2025
3 checks passed

nlordell deleted the pr/state-machine-spec-v2 branch December 5, 2025 10:45

	(** A marker type to indicate state that is local to a specific validator and
	only known by them. *)
	type 'a local = Local of 'a

Refactor State Machine Specification #60

Refactor State Machine Specification #60

Uh oh!

Conversation

nlordell commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot commented Dec 4, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlordell Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlordell Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmeissner Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlordell Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nlordell commented Dec 4, 2025 •

edited

Loading

nlordell Dec 5, 2025 •

edited

Loading

nlordell Dec 5, 2025 •

edited

Loading

rmeissner Dec 5, 2025 •

edited

Loading

nlordell Dec 5, 2025 •

edited

Loading