Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom parameters for fhevm-go #31

Merged
merged 3 commits into from
Nov 8, 2023
Merged

Custom parameters for fhevm-go #31

merged 3 commits into from
Nov 8, 2023

Conversation

david-zk
Copy link
Collaborator

@david-zk david-zk commented Nov 4, 2023

Adds clear api to customize fhevm-go, user needs to provide *FhevmParams struct, defaults exist but can be customized by user

@dartdart26 @youben11

Copy link
Collaborator

@dartdart26 dartdart26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good! The only thing I think we need to change is to not remove the SLOAD/SSTORE gas costs.

@@ -32,6 +32,7 @@ type EVMEnvironment interface {
CreateContract2(caller common.Address, code []byte, codeHash common.Hash, gas uint64, value *big.Int, address common.Address) ([]byte, common.Address, uint64, error)

GetFhevmData() *FhevmData
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Maybe we can rename GetFhevmData() to FhevmData() too. Not part of this PR, just noticed.

fhevm/params.go Outdated
Comment on lines 94 to 104
// TODO: The values here are chosen somewhat arbitrarily (at least the 8 bit ones). Also, we don't
// take into account whether a ciphertext existed (either "current" or "original") for the given handle.
// Finally, costs are likely to change in the future.
FheUint8ProtectedStorageSstoreGas uint64 = EvmNetSstoreInitGas + 2000
FheUint16ProtectedStorageSstoreGas uint64 = FheUint8ProtectedStorageSstoreGas * 2
FheUint32ProtectedStorageSstoreGas uint64 = FheUint16ProtectedStorageSstoreGas * 2

// TODO: We don't take whether the slot is cold or warm into consideration.
FheUint8ProtectedStorageSloadGas uint64 = ColdSloadCostEIP2929 + 200
FheUint16ProtectedStorageSloadGas uint64 = FheUint8ProtectedStorageSloadGas * 2
FheUint32ProtectedStorageSloadGas uint64 = FheUint16ProtectedStorageSloadGas * 2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not remove these. Instead, if they are unused, I think it indicates another issue in that we don't charge for protected storage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restored

@david-zk
Copy link
Collaborator Author

david-zk commented Nov 8, 2023

@dartdart26 restored deleted constants, renamed also GetFhevmData to FhevmData and GetFhevmParams to FhevmParams

@david-zk david-zk merged commit 41f7a61 into main Nov 8, 2023
1 check passed
@david-zk david-zk deleted the fix/custom-params branch November 8, 2023 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants