-
Notifications
You must be signed in to change notification settings - Fork 54
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
could you please add some info regarding |
Are all the contracts available on testnet? I didn't see contract addresses. |
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]>
No |
None of the contracts are available, the user deploys them in the guide |
There was a problem hiding this 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.
uint64 randomNumber = abi.decode(data, (uint64)); | ||
|
||
// Return the number in the specified range | ||
return (randomNumber % (max + 1 - min)) + min; |
There was a problem hiding this comment.
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.
docs/evm/guides/vrf.md
Outdated
**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. |
There was a problem hiding this comment.
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
.
docs/evm/guides/vrf.md
Outdated
|
||
mapping(address => Bet) public bets; | ||
|
||
// Step 1: Commit the bet by submitting a hash (commit phase) |
There was a problem hiding this comment.
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?
docs/evm/guides/vrf.md
Outdated
1. Call **`revealBet(uint64 nonce, uint64 guess)`** to reveal your bet: | ||
|
||
```solidity | ||
revealBet(12345, 1); // nonce: 12345, guess: 1 (tails) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
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]>
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.
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". |
@tarakby can you suggest a resource or point me to a methodology for safe implementation of 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. |
@sisyphusSmiling there are two main ways to avoid the modulo bias:
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 |
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 🙏 |
No description provided.