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

create vrf documentation #902

Merged
merged 8 commits into from
Sep 20, 2024
Merged

create vrf documentation #902

merged 8 commits into from
Sep 20, 2024

Conversation

Aliserag
Copy link
Collaborator

No description provided.

Copy link

vercel bot commented Sep 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 20, 2024 2:28am

@nialexsan
Copy link
Collaborator

could you please add some info regarding resetBet and revealBet?
I need to run resetBet between committing bets
and how to know whether my bet has won or lost from revealBet? it doesn't return anything, and I don't see any changes in my balance. I ran it 10 times and didn't win anything

@bthaile
Copy link
Contributor

bthaile commented Sep 18, 2024

Are all the contracts available on testnet? I didn't see contract addresses.

docs/evm/guides/vrf-2.png Outdated Show resolved Hide resolved
docs/evm/guides/vrf.md Outdated Show resolved Hide resolved
docs/evm/guides/vrf.md Outdated Show resolved Hide resolved
docs/evm/guides/vrf.md Outdated Show resolved Hide resolved
@sisyphusSmiling
Copy link
Contributor

Also, theres a similar example included in this PR (without the move obfuscation) as a contrast between Cadence & Solidity implementations. I think it would be helpful to link to the repo

Co-authored-by: Giovanni Sanchez <[email protected]>
@Aliserag
Copy link
Collaborator Author

Are all the contracts available on testnet? I didn't see contract addresses.

No

@Aliserag
Copy link
Collaborator Author

Are all the contracts available on testnet? I didn't see contract addresses.

None of the contracts are available, the user deploys them in the guide

Copy link
Contributor

@tarakby tarakby left a comment

Choose a reason for hiding this comment

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

Nice documentation and examples. Good idea to add a page about VRF.

However the commit-reveal scheme doesn't seem to be described correctly, and the example seems to have issues. cc @sisyphusSmiling to check in case I misread the code.

docs/evm/guides/vrf.md Outdated Show resolved Hide resolved
uint64 randomNumber = abi.decode(data, (uint64));

// Return the number in the specified range
return (randomNumber % (max + 1 - min)) + min;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not promote this % method to generate randoms in a range. It is not secure because of the modulo bias.

On the Cadence side, we have purposely made the function accept a modulo argument, and we implemented internally a secure method. The argument was not ported from Cadence to EVM unfortunately.

Developers can always reproduce that safe method with solidity so I suggest to let developers figure out how to use EVM's revertibleRandom to generate other safe distributions (random less than a modulo, permutations, shuffles..), maybe we could just avoid this section so we don't promote the unsafe usage.

Comment on lines 165 to 170
**Time-Locked Randomness with Commit-Reveal**

The **commit-reveal scheme** solves the post-selection problem by splitting the random process into two phases:

1. **Commit Phase**: The user commits to a hidden guess (via a hash of their guess and a nonce) **before** the randomness is revealed. This locks the user into their guess without revealing it to the smart contract or anyone else.
2. **Reveal Phase**: After the randomness is securely generated and made public (e.g., using `revertibleRandom()` or another source), the user can then reveal their guess and nonce to complete the process.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understood the scheme, or at least this isn't the commit-reveal scheme Flow allows to be used on Cadence and EVM.

@sisyphusSmiling did a nice job explaining the idea here. We could use it to update this section. Btw the commit-reveal scheme doesn't use revertibleRandom, it uses a Cadence contract via another arch called getRandomSource.


mapping(address => Bet) public bets;

// Step 1: Commit the bet by submitting a hash (commit phase)
Copy link
Contributor

Choose a reason for hiding this comment

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

Submitting the hash doesn't seem to add much to the scheme. The bet structure itself could be stored instead of the hash, and we can check later that the user didn't alter their bet. Or maybe it's a way to reduce storage?

1. Call **`revealBet(uint64 nonce, uint64 guess)`** to reveal your bet:

```solidity
revealBet(12345, 1); // nonce: 12345, guess: 1 (tails)
Copy link
Contributor

@tarakby tarakby Sep 20, 2024

Choose a reason for hiding this comment

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

⚠️ If I understood this correctly, a user can call revealBet and decide to revert the result. The user can run revealBet in a later block using a different randomness, and try till they win. This is the problem described in revertibleRandom, and this has the same issue since it uses revertibleRandom.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a proper reading @tarakby. I overlooked that this contract uses revertibleRandom and assumed that it was using getRandomSource in my review

Co-authored-by: Tarak Ben Youssef <[email protected]>
@Aliserag Aliserag dismissed tarakby’s stale review September 20, 2024 02:28

Removing Commit Reveal example due to time urgency of having this live for hackathon. Would apprechiate help from you or Gio to add an appropriate example.

@Aliserag Aliserag merged commit 00936d6 into main Sep 20, 2024
3 checks passed
@Aliserag Aliserag deleted the vrf-doc branch September 20, 2024 02:35
@tarakby
Copy link
Contributor

tarakby commented Sep 20, 2024

ok, good idea to omit to the commit-reveal section for now.

We should also omit the section https://developers.flow.com/evm/guides/vrf#generating-random-numbers-in-a-range as per my comment above as it's promoting an unsafe implementation, especially that is mentions applications like "lotteries".

@sisyphusSmiling
Copy link
Contributor

sisyphusSmiling commented Sep 20, 2024

@tarakby can you suggest a resource or point me to a methodology for safe implementation of function getRandomInRange(uint64 min, uint64 max) public view returns (uint64)? I've looked around and the only resources I could find referenced rejection sampling which doesn't pair well with the computational constraints of the contract environment IMO.

Edit: I revisited the article you shared on modulo bias and see the concept of modulo reduction. I'll explore this and see if it's workable in this context.

@tarakby
Copy link
Contributor

tarakby commented Sep 20, 2024

@sisyphusSmiling there are two main ways to avoid the modulo bias:

  1. rejection sampling as you pointed out. The number of rejections isn't deterministic but the performance isn't necessarily bad if implemented correctly. For instance, if you need a random less than 5 (encoded in 3 bits) and your random generator provides 64 bits. You should not iterate till the 64 bits number is less than 5, you should consider 3 bits at a time from the 64 bits, and check if those 3 bits are less than 5 (it is the case with more than 50% - I let you do the math). This is the method we implemented in Cadence for revertibleRandom(modulo).
  2. get extra bits of randomness so that the modulo bias becomes negligible. If you need a random less than k and our security level target is 128 (considered safe enough and is used in Flow), we need to sample a log_2(k) + 128 bits, and not just 64 bits. Then we can use the % operator.

These 2 methods are cryptographically secure and are recommended by the cryptography standards. Here is a more technical resource on why this works: https://www.zkdocs.com/docs/zkdocs/protocol-primitives/random-sampling/#sampling-from-zq

@sisyphusSmiling
Copy link
Contributor

sisyphusSmiling commented Sep 21, 2024

Thank you for the advice @tarakby! I've added a first pass in onflow/random-coin-toss#22, would appreciate a cursory review when you get the chance 🙏

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.

5 participants