Skip to content

feat(gateway-contracts): extend keygen EIP712 with extra data#2056

Open
isaacdecoded wants to merge 7 commits intofeat/kms-context-switchfrom
isaac/feat/1125/extend-eip712-keygen-and-crs-attestation
Open

feat(gateway-contracts): extend keygen EIP712 with extra data#2056
isaacdecoded wants to merge 7 commits intofeat/kms-context-switchfrom
isaac/feat/1125/extend-eip712-keygen-and-crs-attestation

Conversation

@isaacdecoded
Copy link
Contributor

Eikix and others added 3 commits March 6, 2026 08:59
* chore(coprocessor): remove legacy tfhe-worker grpc path

* fix(tfhe-worker): resolve clippy dead_code in bench/test utils

* refactor(tfhe-worker): remove unused computation module

* test(tfhe-worker): cap event operator coverage at uint64

* fix(coprocessor): address review noise and typos

* chore(tfhe-worker): reduce bench fmt churn in dex migration

* chore(tfhe-worker): revert formatting-only bench_id wraps

* chore(tfhe-worker): remove remaining bench format-only churn

* bench(tfhe-worker): restore dex workload parity with legacy grpc

* test(tfhe-worker): restore non-ignored coverage after grpc removal

* test(tfhe-worker): deduplicate operator event coverage

* test(tfhe-worker): harden event test stability

* test(tfhe-worker): run full event type matrix in CI

* test(tfhe-worker): default full event matrix with mode logging

* test(tfhe-worker): simplify event matrix selection

* docs(tfhe-worker): document event test matrix modes

* test(tfhe-worker): expand random event tests across types

* test(tfhe-worker): restore random type matrix parity

* test(tfhe-worker): use query! in invalid operation event test

* fix(bench): stabilize benchmark pipeline after grpc refactor

* fix(bench): allow dex setup trivial encrypt handles

* charts: bump coprocessor chart version

* tfhe-worker: propagate gpu feature to test-harness

* test(tfhe-worker): allow dependent schedule setup handle

* test(tfhe-worker): fix event test matrix CI regressions

* refactor(tfhe-worker): deduplicate test helpers and remove dead code

- Migrate operators_from_events.rs to use shared event_helpers
  (setup_event_harness, next_handle, to_ty, tfhe_event, log_with_tx)
- Remove duplicate test_invalid_operation_marks_error (kept in errors.rs)
- Move wait_for_error to event_helpers for shared use
- Extract TEST_CHAIN_ID const, remove debug eprintln calls
- Remove 16 dead CoprocessorError variants from types.rs

* refactor(tfhe-worker): destructure EventHarness to reduce PR diff

Destructure setup_event_harness() return into {app, pool, listener_db}
so variable names match the original code, minimising the review diff.

* chore(tfhe-worker): remove dead deps and batch event test waits

Remove 6 Cargo dependencies that were only used by the deleted gRPC
server (sha3, lru, rayon, tfhe-zk-pok, regex, actix-web).

Restructure 4 event tests (unary, cast, if-then-else, rand) to use
batch-then-wait pattern: insert all events first, call
wait_until_all_allowed_handles_computed once, then verify. This
eliminates ~200 redundant waits in CI, saving ~10 minutes of sleep.

Also remove unnecessary pub(super) from test_fhe_rand_events.

* refactor(tfhe-worker): address PR review feedback

- Upgrade as_scalar_uint to accept &BigInt directly
- Deduplicate helpers in operators_from_events.rs (delete
  insert_tfhe_event, allow_handle, as_scalar_uint copies; use
  event_helpers versions)
- Delete redundant test_fhe_rand_events (subset of random.rs tests)
- Expand test_op_trivial_encrypt to cover all supported types with
  edge-case values
- Add 5 error test scenarios: circular dependency, too many inputs,
  scalar division by zero, binary boolean inputs, unary boolean inputs

* fix(tfhe-worker): replace validation-time error tests with execution-time ones

Remove 3 error tests (circular dependency, too many inputs, scalar div
by zero) that trigger validation-time errors in check_fhe_operand_types.
These errors propagate via ? without being persisted to the DB, causing
an infinite retry loop in event-driven mode.

Replace with test_type_mismatch_error (FheAdd on uint8 + uint16) which
passes validation but properly fails at execution time with
UnsupportedFheTypes.

The validation-path error propagation is tracked as a separate issue.

* docs: update FHE computation diagram to reflect event-driven architecture

Replace the obsolete AsyncCompute gRPC flow with the current
host-listener event-driven architecture in the sequence diagram.

* fix(tfhe-worker): fix GPU test failures in error and random bounded tests

test_coprocessor_computation_errors: Replace Cast-to-type-255 with
FheSub on mismatched types (uint32 + uint64).  The old test panicked on
the GPU path during memory reservation in trivial_encrypt_be_bytes,
preventing the error from being persisted to the DB.  Type-mismatch
errors return a proper Result::Err on both CPU and GPU.

test_fhe_random_bounded: Use per-type bounds from the old gRPC test
instead of upper_bound=1.  The 0-random-bits edge case (bound=1)
behaves differently on GPU vs CPU.  Also check bool results as
true/false rather than assuming a specific numeric value, since CPU
and GPU produce different deterministic outputs for the same seed.

* docs(tfhe-worker): fix stale README heading after gRPC removal

The server was removed; only the background worker remains.
#2029)

Error tests now assert the specific error message instead of only
checking is_error == true.

The bounded random test now generates two samples per type with different
seeds and asserts they differ, catching any constant-output RNG
implementation including always-zero.

Closes zama-ai/fhevm-internal#1077
@cla-bot cla-bot bot added the cla-signed label Mar 6, 2026
@mergify
Copy link

mergify bot commented Mar 6, 2026

🧪 CI Insights

Here's what we observed from your CI run for 73c6d91.

🟢 All jobs passed!

But CI Insights is watching 👀

@isaacdecoded
Copy link
Contributor Author

@claude /pr-review

@claude
Copy link

claude bot commented Mar 6, 2026

Claude finished @isaacdecoded's task in 11m 31s —— View job


PR Review: feat(gateway-contracts): extend keygen EIP712 with extra data

  • Pre-review validation
  • Gather context (CLAUDE.md, PR diff, linked issue)
  • Run parallel review agents (bug hunter, guideline compliance, error handling)
  • Validate findings
  • Post review summary

Code Review

[AUTOMATED]

Summary

This PR makes two key changes to the gateway-contracts KMSGeneration contract:

  1. Extends EIP-712 with extraData: Adds a generic bytes extraData field (encoding [version | context_id | epoch_id]) to both the KeygenVerification and CrsgenVerification EIP-712 typed structs. This field is propagated through function signatures, event emissions, and hash computations.

  2. Removes deprecated key resharing: Removes prssInit(), keyReshareSameSet(), their events (PRSSInit, KeyReshareSameSet), the reshareKeys Hardhat task, and associated test infrastructure.

Net result: +1123/-2658 lines, which is a healthy reduction in code complexity.

Detailed Analysis

✅ EIP-712 Implementation (Correct)

The EIP-712 type strings and hash encodings are implemented correctly:

  • KeygenVerification: "KeygenVerification(uint256 prepKeygenId,uint256 keyId,KeyDigest[] keyDigests,bytes extraData)KeyDigest(uint8 keyType,bytes digest)" — Referenced type KeyDigest is correctly appended alphabetically.
  • CrsgenVerification: "CrsgenVerification(uint256 crsId,uint256 maxBitLength,bytes crsDigest,bytes extraData)" — Standalone type, no referenced types needed.
  • Both _hashKeygenVerification and _hashCrsgenVerification correctly hash extraData as keccak256(extraData) per the EIP-712 spec for bytes fields.

✅ Storage Layout Compatibility (Correct)

The keyReshareCounter field is preserved in the storage struct (marked as @notice Deprecated: key resharing is now managed by KMSVerifier on Ethereum.), preventing storage slot shifts for existing deployments. This is the correct pattern for upgradeable contracts.

✅ Upgrade Mechanism (Correct)

  • REINITIALIZER_VERSION bumped from 5 → 6
  • reinitializeV4() renamed to reinitializeV5() with empty body (no new state to initialize)
  • Version bumped from 0.4.0 → 0.5.0
  • The upgrade task auto-discovers reinitialize functions by prefix, so reinitializeV5() will be picked up correctly.

✅ Event Consistency (Correct)

KeygenResponse and CrsgenResponse events consistently include extraData before signature across:

  • Interface (IKMSGeneration.sol)
  • Implementation (KMSGeneration.sol)
  • Mock (KMSGenerationMock.sol)
  • Rust bindings (ikms_generation.rs, kms_generation.rs)

✅ Test Coverage (Good)

  • Tests properly include extraData via createExtraData(1, 1) helper
  • Event assertions correctly verify extraData ordering
  • EIP-712 test utilities updated with extraData in type definitions and message payloads
  • Access control tests include extraData parameter

✅ Scoping Block in crsgenResponse (Correct)

The new scoping block around kmsSigner in crsgenResponse is a standard Solidity pattern to manage stack depth — needed after adding the extraData parameter. Behavior is identical to the previous implementation.

✅ CI Workflow Fix (Correct)

Re-enabling the KMSGeneration upgrade test by uncommenting if: false# if: false is appropriate since upgrade compatibility should be validated with the new version bump.

Nit: Minor Observations

Nit: Inconsistent keccak256(abi.encodePacked(...)) vs keccak256(...) — In _hashCrsgenVerification, crsDigest is hashed as keccak256(abi.encodePacked(crsDigest)) while extraData uses keccak256(extraData). For a single bytes argument, these produce identical results, so there's no bug. But for consistency, you might want to standardize the pattern.

Nit: KEY_RESHARE_COUNTER_BASE still exported — The constant KEY_RESHARE_COUNTER_BASE is still defined in KMSRequestCounters.sol (line 41) and the KeyReshare enum variant still exists (line 16), even though they're no longer imported by KMSGeneration.sol. This is fine since other contracts may still reference them, but worth noting for future cleanup.

⚠️ Downstream Impact (Informational)

This PR introduces breaking changes that will affect downstream consumers:

  1. Coprocessor mock (coprocessor/fhevm-engine/gw-listener/contracts/KMSGeneration.sol): Still uses old function signatures without extraData and still declares prssInit()/keyReshareSameSet(). This will need a corresponding update.

  2. Helm chart (charts/contracts/values-kmsgen.yaml): Still references prssInit and keyReshareSameSet Hardhat tasks that have been deleted. These commands will fail at runtime.

  3. KMS Connector: Extensive references to KeyReshareSameSet, PRSSInit, and related events throughout the kms-connector crate. The Rust bindings in this PR are updated, but the connector code will need corresponding changes.

These are expected for a feature branch (feat/kms-context-switch) and likely will be addressed in companion PRs.

Verdict

No bugs found. The implementation is clean, well-tested, and follows established patterns in the codebase. The EIP-712 extension is correctly implemented, storage layout is preserved for upgrade safety, and tests have been appropriately updated.
|

@isaacdecoded isaacdecoded marked this pull request as ready for review March 9, 2026 09:08
@isaacdecoded isaacdecoded requested review from a team as code owners March 9, 2026 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants