feat(host-contracts): split canonical-host deploy task and remove on-chain in-flight guard#2318
feat(host-contracts): split canonical-host deploy task and remove on-chain in-flight guard#2318Eikix wants to merge 14 commits intofeat/RFC013from
Conversation
84f546a to
13a6f13
Compare
859f864 to
185b0d9
Compare
Removes the on-chain in-flight check from ProtocolConfig.defineNewKmsContext and the corresponding view from KMSGeneration/IKMSGeneration, plus the KeyManagementRequestInFlight error from IProtocolConfig. The guard made ProtocolConfig unusable on non-canonical host chains (where KMSGeneration is not deployed) and was unreliable as a DAO-proposal safety net anyway. Coordination of conflicting keygen requests becomes a runbook concern. Part of zama-ai/fhevm-internal#1268
Adds task:deployCommonHostContracts (runs on every host chain) and task:deployCanonicalHostContracts (runs only on the canonical chain — deploys KMSGeneration). task:deployAllHostContracts is kept as a thin alias calling both, preserving behavior for existing call sites. Mirrors the split in task:deployEmptyUUPSProxies. Part of zama-ai/fhevm-internal#1268
Renames the aggregate deploy task to task:deployCommonHostContracts and adds an intentionally no-op task:deployCanonicalHostContracts for symmetry with host-contracts. task:deployAllHostContracts stays as an alias. library-solidity does not deploy ProtocolConfig or KMSGeneration, so the canonical task has no bodies to run — the split exists for consistent task naming across the two hardhat projects. Part of zama-ai/fhevm-internal#1268
buildExtraHostScOverride now overrides the deploy service's command on non-default host chains to call task:deployCommonHostContracts, skipping KMSGeneration (canonical-only). Default chain template is unchanged and still calls task:deployAllHostContracts (alias that invokes both). Part of zama-ai/fhevm-internal#1268
Default host chain's address file is now expected to contain PROTOCOL_CONFIG_CONTRACT_ADDRESS and KMS_GENERATION_CONTRACT_ADDRESS. Extra host chains are expected to contain PROTOCOL_CONFIG_CONTRACT_ADDRESS but NOT KMS_GENERATION_CONTRACT_ADDRESS. A new assertGeneratedAddressFileLacks helper enforces the negative constraint. Part of zama-ai/fhevm-internal#1268
Documents the operator SOP for keygen/CRS/context-rotation DAO proposals after the on-chain hasPendingKeyManagementRequest guard was removed, and the manual mirror workflow for non-canonical ProtocolConfig replicas. Part of zama-ai/fhevm-internal#1268
…arration comments Narrow compile:specific to KMSGeneration.sol (the only contract that reads kmsGenerationAdd after the guard removal), avoiding a full contracts/ recompile in the canonical path. Drop two narration-only comments in deployCanonicalEmptyUUPSProxies. Part of zama-ai/fhevm-internal#1268
Adds two tests to the multi-chain isolation suite that verify on-chain state across all host RPCs: - KMSGeneration must have code only on the canonical host, not extras. - ProtocolConfig must have code on every host chain. Plumbs PROTOCOL_CONFIG_CONTRACT_ADDRESS and KMS_GENERATION_CONTRACT_ADDRESS through the fhevm-cli test-suite env so the e2e process can read them. Part of zama-ai/fhevm-internal#1268
Regenerates rust bindings and contract selectors after removing hasPendingKeyManagementRequest and KeyManagementRequestInFlight. Part of zama-ai/fhevm-internal#1268
185b0d9 to
ec25b6f
Compare
… ProtocolConfig Two review findings on PR #2318: - generate/addresses.ts: add PROTOCOL_CONFIG_CONTRACT_ADDRESS and KMS_GENERATION_CONTRACT_ADDRESS to the host allowlist so generateRuntime() refreshes preserve the new canonical-host address keys in both .env.host and FHEVMHostAddresses.sol. Without this, any post-deploy regenerate (e.g. resume) silently drops them and breaks downstream tasks that recompile against protocolConfigAdd / kmsGenerationAdd. Mirrors the same addition in renderHostChainAddressesSolidity and requires PROTOCOL_CONFIG_CONTRACT_ADDRESS in discovery validation. - multiChainIsolation.ts: plumb HOST_CHAIN_N_PROTOCOL_CONFIG_CONTRACT_ADDRESS through env generation and ChainConfig so the per-chain ProtocolConfig test uses each chain's discovered address rather than the primary's. The KMSGeneration test keeps the canonical-address RPC check (semantic is "canonical address must not have code on extras"); the deploy-time negative assertion in up-flow.ts already guards against extras holding any KMS_GENERATION_CONTRACT_ADDRESS entry. Part of zama-ai/fhevm-internal#1268
|
|
||
| task('task:deployCanonicalHostContracts').setAction(async function (_, hre) { | ||
| await hre.run('task:deployCanonicalEmptyUUPSProxies'); | ||
| // KMSGeneration reads its own address from the generated FHEVMHostAddresses.sol, so it must be |
There was a problem hiding this comment.
KMSGeneration reads its own address from the generated FHEVMHostAddresses.sol not sure about that
It imports only protocolConfigAdd?
| }); | ||
|
|
||
| // Alias: keeps existing call sites working. Runs both as if the one chain were canonical. | ||
| task('task:deployAllHostContracts').setAction(async function (_, hre) { |
There was a problem hiding this comment.
should this be kept as the default?
The canonical is the exception no?
There was a problem hiding this comment.
i dont think we should change the behaviour no? for backward compat
we just need to make sure infra doesnt use it anymore?
… requests Adds task:assertNoPendingKeyManagementRequest as the task-layer replacement for the removed hasPendingKeyManagementRequest on-chain guard. The task scans KMSGeneration event logs and throws if a PrepKeygenRequest or CrsgenRequest has no paired Activate/Abort event. Operators run it as the pre-flight before submitting any DAO proposal that mutates KMS state (defineNewKmsContext, keygen, crsgenRequest). Updates the runbook in host-contracts/README.md to reference the task instead of describing private-storage-slot reads that operators cannot perform without manual slot computation. Part of zama-ai/fhevm-internal#1268
…light Two review findings on the new pre-flight task: - Throw on no-code at the resolved address instead of logging "Skipping pending-request check" and exiting 0. A wrong --network, stale address, or non-canonical host must fail the pre-flight; a silent waive is worse than useless for a gate replacing the removed on-chain guard. - Resolve the KMSGeneration address from --address flag, then env var, then addresses/.env.host. The previous implementation required the generated file, which is not tracked in git, so a clean operator checkout could not run the documented runbook command. The flag form is now the documented invocation in host-contracts/README.md. Part of zama-ai/fhevm-internal#1268
Previously the pre-flight only verified that some contract had code at the resolved address. If an operator passed another live canonical-host contract (e.g. ProtocolConfig, KMSVerifier), queryFilter returned zero matching logs and the task reported "No pending key management requests" — a false green. Add a getVersion() call after the code check and require the returned string to start with "KMSGeneration". getVersion() reverts if the selector is missing (wrong contract entirely) and the prefix check disambiguates peer upgradeable contracts (all expose getVersion() but with different CONTRACT_NAME prefixes). Part of zama-ai/fhevm-internal#1268
| }); | ||
|
|
||
| // Off-chain pre-flight replacing the removed on-chain hasPendingKeyManagementRequest guard. | ||
| // A key/CRS request is pending iff more ?*Request events have fired than paired Activate/Abort events. |
| } | ||
|
|
||
| const code = await hre.ethers.provider.getCode(kmsGenAddress); | ||
| if (code === '0x' || code.length === 2) { |
There was a problem hiding this comment.
not sure about this condition?
Why have code.length === 2?
| kmsGen.queryFilter(kmsGen.filters.PrepKeygenRequest()), | ||
| kmsGen.queryFilter(kmsGen.filters.ActivateKey()), | ||
| kmsGen.queryFilter(kmsGen.filters.AbortKeygen()), |
There was a problem hiding this comment.
not sure this possible and not sure events scanning is the way to go: this do not have a fromBlock and RPC limits to 10K blocks eth_getLogs if not mistaken. So reliability is not perfect
Storage read would be more convenient but would require view functions to be added:
- getKeyCounter() / getCrsCounter() to get the lastest requests
- isRequestDone(uint256) to see if they are done or not.
There was a problem hiding this comment.
Maybe isRequestDone is not even mandatory and it is possible to use getKeyMaterials(keyCounter) / getCrsMaterials(crsCounter) with reverts but would reduce lisibility and comprehension for a discutable reduction in getters
Root cause of the docker-compose CI failure: OZ upgrades.upgradeProxy can return before the upgradeToAndCall tx is mined on interval-mining networks (anvil --block-time 0.5 in the host-contracts docker stack). Pre-split, #2309's deployAllHostContracts had deployKMSGeneration between deployProtocolConfig and deployKMSVerifier, mining blocks that happened to absorb this race. Our split moved KMSGeneration out of that gap, exposing it. Add a bounded poll on a state-dependent view (getCurrentKmsContextId) so task:deployProtocolConfig only returns once the new impl is observable on-chain. Fails loudly after 2.5s if the upgrade never lands. Also addresses Oba's review on deployCanonicalHostContracts: the "KMSGeneration reads its own address" comment was wrong (KMSGeneration only imports protocolConfigAdd, not kmsGenerationAdd), and the contracts/KMSGeneration.sol recompile was therefore unnecessary. Both removed. Part of zama-ai/fhevm-internal#1268
Summary
Splits
task:deployAllHostContractsintodeployCommonHostContracts(runs on every host chain) anddeployCanonicalHostContracts(runs only on the canonical host — Ethereum — and deploysKMSGeneration). Keepstask:deployAllHostContractsas a thin alias calling both so existing call sites (gitops values, docker-compose templates, downstream consumers) keep working through the rollout.Also removes the on-chain
hasPendingKeyManagementRequestguard fromProtocolConfig.defineNewKmsContext: it was unreliable against DAO-proposal execution and madedefineNewKmsContextunusable on non-canonical host chains (where noKMSGenerationexists). Coordination becomes a runbook concern, documented inhost-contracts/README.md.Closes zama-ai/fhevm-internal#1268.
Note on issue scope vs. implementation
Issue #1268's "What" table references
test-suite/fhevm/scripts/deploy-fhevm-stack.shandtest-suite/fhevm/env/staging/.env.host-sc. Those paths predate the fhevm-cli (PR #2134,ef080b9dc) and no longer exist. The orchestration edits land in the CLI equivalents:src/generate/compose.ts,src/flow/up-flow.ts,src/flow/artifacts.ts, andsrc/render-compose.test.ts.library-soliditymirror is cosmeticlibrary-soliditydoes not deployProtocolConfigorKMSGeneration, so itsdeployCanonicalHostContractsis intentionally a no-op. The split exists for task-name symmetry withhost-contractsso external callers can use the same task names consistently.Binding regen commit includes tooling drift
The bindings-regen commit (
ef43076d4) touches ~40 rust binding files beyond the 4 KMS-related ones — extra#[allow(dead_code)]attributes from an alloy-sol-macro version difference. Pre-existing drift, not caused by this PR. Clippy clean.Test plan
forge testonhost-contracts— 445/445 passing.npx hardhat testonhost-contracts— 2518 passing.make check-bindings/make check-selectors— clean locally.bun test src/render-compose.test.tsontest-suite/fhevm— 11/11 passing, including two new assertions (extras call common-only; default template still calls the alias).fhevm-cli updeploys 8 host contracts (includesKMSGeneration).fhevm-cli up --scenario multi-chaindeploysKMSGenerationonly on the default host; chain-b's addresses file lacksKMS_GENERATION_CONTRACT_ADDRESS.Coordination
migrate-KMSVerifier-to-ProtocolConfig) — it also editstaskDeploy.ts. Whichever merges second resolves the conflict.Follow-ups (out of scope)
deployCommandsvalues (mainnet eth → both tasks; dev/staging/testnet → common only). Can land any time because the alias keeps today's behavior.host-contractsaddress references from compile-timeconstants (Isaac's smell on PR feat(host-contracts): rename abortKeygen parameter to prepKeygenId #2311).defineNewKmsContexton non-canonical — meaningful only after PR feat(host-contracts): migrate KMSVerifier to ProtocolConfig #2309 merges.