-
Notifications
You must be signed in to change notification settings - Fork 138
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
Update revertibleRandom
interface (FLIP 120)
#2712
Comments
FYI, I'll provide the sample rejection method to return a random less than |
@turbolent would it be simpler for the Cadence team if I separate the first implementation item "add new function The reason is that that the first item is what we need to have a safe randomness supported by Cadence. The other items are only functionality improvements.
|
Following up on a slack conversation with Tarak here: If I understood correctly, the
which is essentially the same as the current |
I went ahead and split the item tasks in this issue. The first task is now described in #2869 and I'll try to implement it myself. |
unsafeRandom
interface (FLIP 120)revertibleRandom
interface (FLIP 120)
@tarakby That's a good idea. Just adding a |
Changing the signature might be a breaking change. Existing code working like this might not type-check anymore: let r = revertibleRandom() // `r` is `UInt64` Type inference is maybe not sufficient. |
@tarakby Hi, I had a small question. I can see that Cadence runtime calls So if I want to create a random PS: I'll pick this issue up. |
Right, makes sense. I guess I need to create a random 256 bit integer (r) every time instead of 64 bit and then take modulo, since we also want to support |
@darkdrag00nv2 I deleted my comment after reading the description of the task, it says Basically you will reject random, if it is |
@darkdrag00nv2 Thank you for picking this up! You only need to implement the changes to the Cadence API, @tarakby is going to provide the implementation of the actual implementation (Go code) of the function |
@turbolent Did you mean that I can update the interface of Also, you were right that this would be a breaking change. I've opened up a draft PR for this. |
Why this is breaking change? Is it Stable Cadence related? |
@bluesign
|
Ah yeah, I was thinking we already have this as dynamic, |
Thank you @darkdrag00nv2 for picking up this issue!
Yes
If you confirm these 2 types are missing from the doc, then we can add them to the doc, and then to the FLIP120 and this issue.
I did mention that I'll provide the implementation for the sample rejection here, but if you would like to go ahead and implement it, you can follow this implementation (https://github.com/onflow/flow-go/blob/master/crypto/random/rand.go#L75-L104) and I'll review it when ready. The reason for not simply sampling a random and then use the modulo operation is the known modulo bias (the resulting distribution isn't uniform, it's biased towards the smaller numbers). We can avoid that either with sampling 128 extra bits and compute the modulo (bias negligible, but the mod becomes costly), or use the sample rejection (uniform distribution, algorithm returns fast) |
Yes, PRs: #2497, #2510. @turbolent Any idea why they do not show up at https://cadence-lang.org/docs/language/values-and-types.
@tarakby I'll prefer if you are able to do it. I'll update the Cadence API and will leave the modulo work as a TODO. Draft PR - #2943 |
@darkdrag00nv2 onflow/docs#114 made changes to the upcoming docs, not the doc of the current, released version. https://cadence-lang.org/docs/language/values-and-types also shows the current, version by default (see 0.42 in the top right). If you select 1.0, the upcoming version, the content shows up. |
@tarakby We'll still need the modulo implementation for the Cadence 1.0 release, do you have an estimate of when that would be ready? |
@turbolent It's my next task after finishing the crypto module migration (still fixing issues there). When is the Cadence 1.0 release supposed to be wrapped up? |
side comment regarding the modulo implementation
However, there is a subtle aspect that we should decide on and potentially cover in the documentation
|
@tarakby The plan is to release a release candidate by EOY. We'll need it very latest by IMHO mid/end January. You can see the full plan here: https://forum.flow.com/t/cadence-1-0-upgrade-plan/5477 If it's not tractable to add it in time, we can remove the |
I think it should be fixed cost tbh, predictable costs are better, and there is zero chance of attack on this for resource exhaustion |
@turbolent I can work on it before the holidays. @AlexHentschel Thanks for raising the metering point. So I think we should meter arithmetic operations with a fixed cost, and there is no need to go down the road of implementing constant-time arithmetics. For
|
Issue to be solved
The reasons are discussed in onflow/flips#120.
This issue should be implemented after the first task described in #2869.
Suggested Solution
revertibleRandom
interface. The new function signature should befun revertibleRandom<T: FixedSizeUnsignedInteger>([modulo: T]): T
FixedSizeUnsignedInteger
is the listUInt8
,UInt16
,UInt32
,UInt64
,UInt128
,UInt256
,Word8
,Word16
,Word32
,Word64
modulo
is an optional parameter, if provided the returned valuer
satisfies0<= r < modulo
. The function panics ifmodulo == 0
- algorithm implemented should be sample rejection.Note that
fun unsafeRandom(): UInt64
should not be removed to avoid immediate breaking change.Implementation can start by merging #2706 to use the new
Interface
method.The text was updated successfully, but these errors were encountered: