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

revertible random #15

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -30,37 +30,66 @@ fun assert(_ condition: Bool, message: String)

The message argument is optional.

## unsafeRandom
## `revertibleRandom`

```cadence
fun unsafeRandom(): UInt64
fun revertibleRandom(): UInt64
```

Returns a pseudo-random number.
<Callout type="info">
`unsafeRandom()` function currently behaves the same way as `revertibleRandom()`.
It is deprecated and will be removed in an upcoming release of Cadence.
</Callout>
Comment on lines +39 to +42
Copy link
Member

Choose a reason for hiding this comment

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

do we need this here? We already have a very similar callout for unsafeRandom:

<Callout type="info">
`unsafeRandom()` function currently behaves the same way as `revertibleRandom()`.
It is deprecated and will be removed in an upcoming release of Cadence.
</Callout>

I feel this is largely duplicated and best fits within the context of unsafeRandom

Suggested change
<Callout type="info">
`unsafeRandom()` function currently behaves the same way as `revertibleRandom()`.
It is deprecated and will be removed in an upcoming release of Cadence.
</Callout>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually duplicated it purposely. I think there is an advantage to clarify why this function is here and what's its relationship with the existing unsafeRandom. Developers used to unsafeRandom may be asking how revertible is different and this provides them with a quick answer.

I added the section for unsafeRandom afterwards as per your suggestion to keep a section for deprecated functions. I think the duplication is minor and it is temporary anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay you guys win 😄 onflow/docs#439 (comment)


NOTE:
Smart contract developers should be mindful about the limitations of unsafeRandom.
The stream of random numbers produced is potentially unsafe in the following two regards:
Returns a pseudo-random number.

1. The sequence of random numbers is potentially predictable by transactions within the same block
and by other smart contracts calling into your smart contract.
2. A transaction calling into your smart contract can potentially bias the sequence of random numbers which
your smart contract internally generates.
The sequence of returned random numbers is independent for
every transaction in each block.

tarakby marked this conversation as resolved.
Show resolved Hide resolved
We are working towards removing these limitations incrementally. Once these points are addressed,
Flow’s randomness is safe and we will remove the "unsafe" qualifier.
Under the hood, Cadence instantiates a cryptographically-secure pseudo-random number
generator (CSPRG) for each transaction independently, where the seeds of any two transactions
are different with near certainty.

Nevertheless, there is an additional safety-relevant aspect that developers need to be mindful about:
The random numbers returned are unpredictable
(unpredictable for miners at block construction time,
and unpredictable for cadence logic at time of call),
verifiable, as well as unbiasable by miners and previously-running Cadence code.
See [Secure random number generator for Flow’s smart contracts](https://forum.flow.com/t/secure-random-number-generator-for-flow-s-smart-contracts/5110)
and [FLIP120](https://github.com/onflow/flips/pull/120) for more details.

* A transaction can atomically revert all its action at any time. Therefore, it is possible for a transaction calling into
your smart contract to post-select favourable results and revert the transaction for unfavourable results.
([example](https://consensys.github.io/smart-contract-best-practices/development-recommendations/general/public-data/))
Nevertheless, developers need to be mindful to use `revertibleRandom()` correctly.
tarakby marked this conversation as resolved.
Show resolved Hide resolved

This limitation is inherent to any smart contract platform that allows transactions to roll back atomically and cannot be
solved through safe randomness alone. Providing additional Cadence language primitives to simplify this challenge for
developers is on our roadmap as well. Nevertheless, with safe randomness (points 1 and 2 above resolved), developers can prevent
clients from post-select favourable outcomes using approaches such as described in the
[example](https://consensys.github.io/smart-contract-best-practices/development-recommendations/general/public-data/).
<Callout type="warning">

A transaction can atomically revert all its action.
It is possible for a transaction submitted by an untrusted party
to post-select favorable results and revert the transaction for unfavorable results.

</Callout>

The function usage remains safe when called by a trusted party that does not
perform post-selection on the returned random numbers.

This limitation is inherent to any smart contract platform that allows transactions to roll back atomically
and cannot be solved through safe randomness alone.

Flow protocol has suggested a [solution to implement safe
commit-reveal schemes](https://github.com/onflow/flips/pull/123) and address this limitation.
In cases where a non-trusted party can interact with smart contracts generating
random numbers through their own transactions, **use a commit-reveal scheme**
as described in this [rudimentary example](https://github.com/onflow/flips/blob/main/protocol/20230728-commit-reveal.md#tutorials-and-examples)
tarakby marked this conversation as resolved.
Show resolved Hide resolved


## `unsafeRandom`

This function is superseded by `revertibleRandom()`.
`unsafeRandom` has the same interface and implementation as `revertibleRandom()` although
it is called unsafe.
Comment on lines +86 to +87
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`unsafeRandom` has the same interface and implementation as `revertibleRandom()` although
it is called unsafe.
Its implementation has been upgraded to the same implementation as `revertibleRandom()` for improved security. However, the name was retained for downwards compatibility despite it technically being no longer unsafe (see `revertibleRandom()` for details).


<Callout type="info">
`unsafeRandom` is deprecated and will be removed in an upcoming release of Cadence.
Use `revertibleRandom()` instead.
</Callout>


## RLP
Expand Down