[FEAT] Support rotate left and right operations #141
Conversation
| @@ -13,7 +13,7 @@ export const EInputType = [ | |||
| // FYI: Operations ["sealoutput", "seal", "decrypt", "ne"] are the minimum required for | |||
| // non failing generated code. | |||
|
|
|||
| const patternAllowedOperationsEbool = ["ne|eq|^and$|or|xor|sealoutput|select|seal|decrypt|not"]; | |||
| const patternAllowedOperationsEbool = ["ne|eq|^and$|^or$|xor|sealoutput|select|seal|decrypt|not"]; | |||
There was a problem hiding this comment.
Fix for conflict with ror
|
LGTM, generally I'm missing some unittests in Engine, Driver, etc. please make sure to follow the other operations and see what are the tests that are written for them |
| @@ -1354,6 +1355,104 @@ func Shr(utype byte, lhsHash []byte, rhsHash []byte, tp *TxParams) ([]byte, uint | |||
| return ctHash[:], gas, nil | |||
| } | |||
|
|
|||
| func Rol(utype byte, lhsHash []byte, rhsHash []byte, tp *TxParams) ([]byte, uint64, error) { | |||
There was a problem hiding this comment.
Rol and Ror functions are and always will be the same, therefore I would probably create a helper function somewhere for "rotate" that does everything for which the only differences (based on function input or something?) are the "functionName" and the direction (for which you can choose calling ror or rol)
There was a problem hiding this comment.
I've noticed that it might corrupt the solgen generator due to the way thecontracts_parser.ts works, cuz it's relying on function calls like get2VerifiedOperands to understand that it's 2 operands, and having if lhs.UintType != rhs.UintType to make sure they are the same types and etc..
BUT, i'll add support and lmk if you think it's better
There was a problem hiding this comment.
I think it can be said for every function that
- receives two euints
- returns 1 euint
I mean we can create a 2OperandMathImpl()™ bc I think all these functions of the same type have basically the same implementation.
There was a problem hiding this comment.
Following the conversations with Tom and Itzik, i'm reverting this refactor for now as it's out of scope here and can be handled better as a standalone separated task
| contract RolTest { | ||
| using Utils for *; | ||
|
|
||
| function rol(string calldata test, uint256 a, uint256 b) public pure returns (uint256 output) { |
There was a problem hiding this comment.
Did you write tests to trigger and test those functions?
| @@ -152,7 +152,7 @@ func getRawPrecompileGas(precompileName types.PrecompileName, uintType fhe.Encry | |||
| case fhe.Uint128: | |||
| return 250000 | |||
| } | |||
| case types.Shl, types.Shr: | |||
| case types.Shl, types.Shr, types.Rol, types.Ror: | |||
There was a problem hiding this comment.
Benchmark for mean time consumption on 10 runs each -
(running bench of 100 each now as well)
There was a problem hiding this comment.
After 100 runs each :
solgen/contracts_parser.ts
Outdated
| @@ -85,6 +86,21 @@ async function analyzeGoFile( | |||
| continue; | |||
| } | |||
|
|
|||
| // Support for shoretened util-based functions with "solgen: 2 verified operands" | |||
| const match = trimmedLine.match(solgenVerifiedOperands); | |||
There was a problem hiding this comment.
maybe put this block together with the other solgen: matches above? it's kinda separate. Also why does it require a separate push to the specificFunctionAnalysis that is not required for other solgen comments? I may have missed something
| @@ -1354,6 +1355,104 @@ func Shr(utype byte, lhsHash []byte, rhsHash []byte, tp *TxParams) ([]byte, uint | |||
| return ctHash[:], gas, nil | |||
| } | |||
|
|
|||
| func Rol(utype byte, lhsHash []byte, rhsHash []byte, tp *TxParams) ([]byte, uint64, error) { | |||
There was a problem hiding this comment.
I think it can be said for every function that
- receives two euints
- returns 1 euint
I mean we can create a 2OperandMathImpl()™ bc I think all these functions of the same type have basically the same implementation.
solidity/tests/precompiles.test.ts
Outdated
| const generateTestCases = (bitSize: number) => { | ||
| const basePattern = BigInt(`0b${'11100000'.repeat(bitSize / 8)}`); | ||
| const mask = (1n << BigInt(bitSize)) - 1n; | ||
|
|
||
| return [1, 2, 3, 4, 5].map(rotateAmount => { | ||
| const rotated = ((basePattern >> BigInt(rotateAmount)) | (basePattern << BigInt(bitSize - rotateAmount))) & mask; | ||
| return { | ||
| a: basePattern, | ||
| b: rotateAmount, | ||
| expectedResult: rotated, | ||
| name: ` ror ${rotateAmount}` | ||
| }; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
If I'm not mistaken, these test cases check only rotates of zeroes - can we also test rotates of a turned on bit?
roeezolantz
deleted the
implement-rotate-left-and-right-fhe-operations-1625101289
branch
rol(rotate left) andror(rotate right) operations, supports bothFhe.rol(a,b)and boundeda.rol(b)for all euint<8-128> typespatternAllowedOperationsEboolto preventoraffectrorand make it available for booleanshttps://github.com/FhenixProtocol/warp-drive/pull/42 - Corresponding warp-drive PR