feat(coprocessor): add FHE.sum native opcode (op 28) end-to-end#2274
feat(coprocessor): add FHE.sum native opcode (op 28) end-to-end#2274
FHE.sum native opcode (op 28) end-to-end#2274Conversation
Coprocessor Changed Lines CoverageCoverage of added/modified lines in Per-file breakdownDiff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
coprocessor/fhevm-engine/fhevm-engine-common/src/tfhe_ops.rscoprocessor/fhevm-engine/host-listener/src/database/tfhe_event_propagate.rscoprocessor/fhevm-engine/scheduler/src/dfg/scheduler.rs |
f7bffd5 to
08912b5
Compare
9cbb51a to
80a4f8d
Compare
FHE.sum native opcode (op 35) end-to-end
80a4f8d to
d44bb67
Compare
d44bb67 to
b550cc9
Compare
|
Were make update-conformance && make prettier run? |
b550cc9 to
ba98167
Compare
ba98167 to
52c0638
Compare
| } | ||
|
|
||
| result = keccak256( | ||
| bytes32 preimage = keccak256( |
There was a problem hiding this comment.
One of the goal was to share the handle generation between all nary op. (like other ops)
There was a problem hiding this comment.
Well it will not be the same preimage, because the resultType used for sum is not needed for other other opearations (i.e. isIn).
For sum the resultType is used so corpocessor knows what will be the result, however this is not needed to be known for bolean operations.
Do you think it is better to have common preimage although it is not used always?
…ion instead of reverting
…d Math.max for empty arrays
13e3ee4 to
3b32f80
Compare
3b32f80 to
3bc002e
Compare
|
@claude /pr-review |
There was a problem hiding this comment.
[AUTOMATED]
Code Review: feat(coprocessor): add FHE.sum native opcode (op 28) end-to-end
Summary
Thorough review covering bug hunting (2× opus passes), guideline compliance, and error handling audit across all 37 changed files. This is a well-implemented end-to-end feature addition.
Verdict: ✅ Approve
No blocking issues found. The implementation is consistent across all layers (Solidity → Rust coprocessor) and follows established patterns.
What was checked
Cross-layer consistency ✅
- Solidity
FheTypeenum values (Uint8=2 through Uint128=6) match Rust integer type checks incheck_fhe_operand_typesandperform_fhe_operation_impl - Array size limits are consistent: 60 for Uint64/Uint128, 100 for Uint8–Uint32 in both Solidity (
fheSum,checkHCUForFheSum) and Rust (FHE_SUM_MAX_INPUTS_WIDE/NARROW) - Protobuf
common.protocorrectly updated withFHE_SUM = 28, matchingSupportedFheOperations::FheSum = 28 - Handle encoding:
_appendMetadataToPrehandleembeds type byte at position 30, consistent withget_ct_typereadinghandle[30]in Rust
Scheduler integration ✅
output_typecorrectly extracted fromnode.result_handleviaget_ct_type, threaded throughrun_computation→perform_fhe_operation→perform_fhe_operation_impl- Only consumed in FheSum branch; other operations ignore it — backward compatible
Event propagation ✅
event_to_op_intcorrectly mapsFheSum→28insert_computationreceives variable-length dependencies viavalues.iter().collect()tfhe_result_handleandtfhe_inputs_handlecorrectly updated
Edge cases ✅
- Empty arrays: Solidity allows it and charges HCU; Rust returns trivial encryption of zero via
trivial_encrypt_be_bytes(output_type, &[0u8]) - Single-element arrays: work correctly through standard sum path
- Uninitialized values:
FHE.sollibrary replaces with trivially encrypted zeros before passing to executor - Scalar rejection: FheSum rejects scalar inputs at both Solidity and Rust layers
Security ✅
- ACL checks properly enforced in
_naryOpfor each input handle checkHCUForFheSumenforcesmsg.sender == fhevmExecutorAddress- Type consistency enforced:
_typeOf(values[i]) != resultTypecheck in_naryOp
Pattern consistency ✅
- New
_naryOphelper appropriately abstracts the variadic ACL/type check pattern collect_operands_asRust helper cleanly extracts typed references- HCU bucketed pricing follows the established
checkHCU*patterns - Version bumps consistent (FHEVMExecutor 0.3→0.4, HCULimit 0.2→0.3, reinitializer versions incremented)
Tests ✅
- Rust unit tests cover: op type, i16 roundtrip, empty/single/boundary inputs, type mismatch, scalar rejection
- Solidity manual test suite covers all 5 FHE types, duplicate handles, uninitialized values, empty arrays, single elements
- TypeScript
coprocessorUtils.tsFheSum event handler correctly sums clear texts with modular arithmetic - Foundry tests updated for fheSum HCU validation
Nits (non-blocking)
- Nit: The
_naryOpfunction'sIncompatibleTypes()error could include the index and actual/expected types for easier debugging of large arrays. The existingIncompatibleTypeserror is used elsewhere without parameters, so this is consistent but could be enhanced. - Nit: The shift operator parenthesization changes in
coprocessorUtils.ts(lines 17-23, 32-37) are correctness-preserving clarity improvements —%already has higher precedence than<</>>in JS. Good to make explicit regardless.
Reviewed with 4 parallel specialized agents: 2× bug hunter (opus), guideline compliance (sonnet), error handling auditor (sonnet).
|
All four review agents have now completed. The review was already posted to PR #2274 with an approval. Here's the final tally:
All agents converged on the same conclusion: this is a well-implemented, consistent end-to-end feature addition with no blocking issues. |
Summary
FHE.sumas a native coprocessor opcode (op 28). Solidity emits a singleFheSumevent carrying the full input array; the coprocessor executes the sum using tfhe-rs.euint8,euint16,euint32,euint64,euint128.Closes: https://github.com/zama-ai/fhevm-internal/issues/1226