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

feat: add true randomness #119

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: add true randomness #119

wants to merge 2 commits into from

Conversation

immortal-tofu
Copy link
Contributor

@immortal-tofu immortal-tofu commented Jun 14, 2024

Simple PR to review.

  • I'm wondering if I can do better to transform upperBound to a number of bits.
  • Not sure about the used seed.

@jatZama
Copy link
Member

jatZama commented Jun 15, 2024

Previously fheRand and fheRandBounded did not support uint4 so I excluded the type 1 from supported types in Coprocessor contract : https://github.com/zama-ai/coprocessor-contracts/blob/main/contracts/FHEVMCoprocessor.sol#L393-L402
Did this change it seems?
Also I noticed you changed upperBound to numberOfBits, does this mean that I should drop the isPowerOfTwo function in coprocessor contract? https://github.com/zama-ai/coprocessor-contracts/blob/main/contracts/FHEVMCoprocessor.sol#L73
Another issue is that this would mean @david-zk would also need to refactor fhevm-go-coprocessor. I was not aware this feature was needed for current release in one week.

@immortal-tofu
Copy link
Contributor Author

immortal-tofu commented Jun 15, 2024

We decided to display a upper bound instead of a number of bits in the Solidity API, so nothing change from a developer perspective. cc @mortendahl
We can add rand and randBounded for euint4, but in Q3. What I have added is not in Coprocessor yet, and maybe won't be in coprocessor in Q2. cc @david-zk

randBigInt.SetUint64(randUint)
randCt.TrivialEncrypt(*randBigInt, resultType)
insertCiphertextToMemory(environment, randCt)
randCt, err := tfhe.GenerateObliviousPseudoRandom(resultType, *(*uint64)(unsafe.Pointer(&seed.Bytes()[0])), numberOfBits)
Copy link
Collaborator

@dartdart26 dartdart26 Jun 18, 2024

Choose a reason for hiding this comment

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

AFAIU, the seed can just be a counter. Right now, we hash a fixed global seed and the caller address, leading to the same seed every time. We should double check with tfhe-rs and, if that's the case, change the seed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK you mean we could use directly globalRngSeed ?

Choose a reason for hiding this comment

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

Is globalRngSeed incremented after each use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe nextRngNonce is a better choice. I don't see where we use it right now

Choose a reason for hiding this comment

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

I am getting it right, that globalRngSeed/nextRngNonce is local state? For the coprocessor we might want to avoid this, and instead use a counter from Solidity. This would make the coprocessor computation stateless and reproducible. To generate the symbolic values for random encryptions, we need a Solidity counter (plaintext integer) anyways. This counter could be used here as well.

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.

4 participants