Update PRG interface and implementation #6

sisyphusSmiling · 2023-10-31T20:55:25Z

Related: #5

tarakby

I made a partial review and left the PRG main functions (init and next 64 bits) as I'm still thinking through the design. I'll continue the review shortly, but left some first comments for now.

tarakby · 2023-11-02T18:21:41Z

CODE_OF_CONDUCT.md

+## Enforcement
+
+Instances of abusive, harassing, or otherwise unacceptable behavior may be
+reported by contacting the project team at [email protected]. All


not sure if there is a Flow team address available

Good point. This was taken from an existing open source template, so I'm not sure if we have a dedicated flow team email address for this purpose.

I asked around but didn't get an answer so far

README.md

tarakby · 2023-11-03T00:53:48Z

README.md

+
+> :warning: This repo is still a work in progress - the underlying RandomBeaconHistory is also still a work in progress
+
+## Overview


Clear and easy-to-read doc 👌🏼

transactions/random-beacon-history/heartbeat.cdc

transactions/pseudo-random-generator/next_uint64.cdc

tarakby · 2023-11-03T01:33:26Z

transactions/pseudo-random-generator/next_uint64.cdc

+        if let prg = signer.borrow<&PseudoRandomGenerator.PRG>(from: PseudoRandomGenerator.StoragePath) {
+            var i = 0
+            while i < generationLength {
+                prg.nextUInt64()


This transaction seems to evolve the state of the PRG but does not return the randoms generated. What is the purpose of it?
Or maybe it could return maybe return an array of Uint64 of sizegenerationLength ?

The purpose here is just for demonstration of how one would use the PRG to generate a number of random values. We don't need this for our use case here as the PRG is simply used in the contract, but it could be helpful for others depending on their use case.

We aren't able to return values from transactions, so a caller would need to advance the state of the PRG then query the generated values from a script. If the caller wanted to capture the generated values, they would need to be stored in some resource. I'm also realizing this would all be helpful context in this transaction's prefixed comment

tarakby · 2023-11-03T01:39:11Z

scripts/pseudo-random-generator/next_uint64_new_prg.cdc

+
+    let prg <- PseudoRandomGenerator.createPRG(sourceOfRandomness: seed, salt: salt)
+
+    let randUInt64 = prg.nextUInt64()


Similarly, I think this script could be generalized to take an integer input and return the first outputs of the RNG as a[UInt64]

This script is wrapped in the random function we pass to BasicDistributionTest so we can get the distribution of random numbers. I believe the test method requires a single UInt64 be returned from said random function, so we'll need at least one script with this or similar signature. What if we add a script that does what you're describing which, as I understand it, would generate an array of random UInt64 of length generationLength?

I believe the test method requires a single UInt64 be returned from said random function

You're right about the testing function constraint.
However I'm thinking if we could implement the test this way: have a generalized script that returns [UInt64]. The test calls the script and saves the huge array in a temp bufffer, then the testing function would just read a random at a time from the temp buffer.
This way we avoid the complicated/non-intuitive process of : calling a transaction to advance the PRG state each time but without reading the random, and then call a script that only reads one random.

Curious to know what you think.

Ah of course, that make sense. I'll need to experiment with the largest supported generation length - I wonder if we'll run into limits depending on the size of the array we're trying to generate

you're right, size might be an issue, I think it's about 80000 values of 8 bytes each.
Btw on what network does the test run, and how often?

The test runs in emulator AKA locally on push & PR, but I'll update the test to run on PR since we don't need a stat test every push

tarakby · 2023-11-03T01:42:34Z

scripts/pseudo-random-generator/next_uint64_from_address.cdc

+    return getAccount(prgAddress).getCapability<&PseudoRandomGenerator.PRG>(
+            PseudoRandomGenerator.PublicPath
+        ).borrow()
+        ?.nextUInt64()


The call to nextUInt64() updates the PRG state in the execution state. Maybe this is supposed to be a transaction and not a script?

We aren't able to return values from transactions, so we'll need to use a script to retrieve the value.

Given the inability for transaction to return values, retrieving a newly generated value will involve some arrangement of both a transaction and a script. This isn't very intuitive, but I don't see a way around this for those requesting random numbers offchain.

I agree the combination of a transaction and script for each random is non-intuitive.

I somehow think that saving a PRG object as a resource might not be needed. I believe most use cases would create a PRG and get all the randoms they need at the same operation (transaction or script). (btw this is what I'm also suggesting for the distribution test)

If the same randoms need to be retrieved, it's the seed (or block height of history contract) and resource ID that need to be stored on-chain.

I agree, I questioned myself whether the PRG needed to be a resource, and don't have a good reason why it shouldn't be. Maybe I might care about uniqueness and continuity of PRGs which a resource can give guarantees of - i.e. my Receipt has attached to it some PRG instance. I'll update the PRG object to a struct

contracts/PseudoRandomGenerator.cdc

tarakby

More comments about the xorshift128+ itself

contracts/PseudoRandomGenerator.cdc

Co-authored-by: Tarak Ben Youssef <[email protected]>

…. iterative

tarakby

Thanks for addressing the initial comments, I made another round of comments.

tarakby · 2023-11-07T21:43:42Z

contracts/XorShift128Plus.cdc

+    /// Creates a new XORSift128+ PRG with the given source of randomness and salt
+    ///
+    /// @param sourceOfRandomness: The 32 byte source of randomness used to seed the PRG


In theory, this contract can be used with the source of randomness from the history contract, or with any other source of entropy the developer wanna use.

Also adding a condition about the input length. 16 is enough although we have 32 in our case with the history contract.

Suggested change

/// Creates a new XORSift128+ PRG with the given source of randomness and salt

///

/// @param sourceOfRandomness: The 32 byte source of randomness used to seed the PRG

/// Creates a new xorshift128+ PRG with the given source of randomness and salt

///

/// @param sourceOfRandomness: The entropy bytes used to seed the PRG. It is recommended to use at least 16 bytes of entropy.

tarakby · 2023-11-07T21:45:26Z

contracts/XorShift128Plus.cdc

+    /// @param salt: The bytes used to salt the source of randomness
+    ///
+    /// @return A new PRG resource
+    access(all) fun createPRG(sourceOfRandomness: [UInt8], salt: [UInt8]): @PRG {


Enforcing a minimum length on the input

Suggested change

access(all) fun createPRG(sourceOfRandomness: [UInt8], salt: [UInt8]): @PRG {

access(all) fun createPRG(sourceOfRandomness: [UInt8], salt: [UInt8]): @PRG {

pre {

sourceOfRandomness.length >= 16 "At least 16 bytes of entropy should be used"

}

See comment on feedback below

contracts/XorShift128Plus.cdc

tarakby · 2023-11-07T21:49:17Z

contracts/XorShift128Plus.cdc

+
+        /// Advances the PRG state and generates the next UInt64 value
+        /// See https://arxiv.org/pdf/1404.0390.pdf for implementation details and reasoning for triplet selection.
+        /// Note that state only advances when this function is called from a transaction. Calls from within a script


Nice doc 👌🏼

contracts/XorShift128Plus.cdc

joshuahannan · 2023-11-07T21:49:27Z

CONTRIBUTING.md

+# Contributing to the Non-Fungible Token Standard
+
+The following is a set of guidelines for contributing to the Flow NFT standard. These are mostly guidelines, not rules. Use your best judgment, and feel free to propose changes to this document in a pull request.


Should this be changed to say ramdom coin toss implementation instead of NFT standard?

ha you're right! Good catch

…tances

Co-authored-by: Tarak Ben Youssef <[email protected]>

tarakby · 2023-11-08T01:14:32Z

README.md

@@ -0,0 +1,32 @@
+# [WIP] Random Coin Toss
+
+> :warning: This repo is still a work in progress - the underlying RandomBeaconHistory is also still a work in progress


Is the RandomBeaconHistory contract in this repo the same core-protocol one? If yes I guess it is not WIP right?

You're right, the contract is finalized. I'll update this section

contracts/CoinToss.cdc

Co-authored-by: Tarak Ben Youssef <[email protected]>

tarakby

Nice work! Thank you for your patience with all my comments 🙏🏼

sisyphusSmiling added 7 commits October 3, 2023 16:09

create README

1a2ff37

first pass as PRG implementation

ef58175

add references

6347a40

update flow.json

411763d

add open source docs

47d84a3

fix CoinToss contract bugs + add supporting txns

dd7b295

add FlowToken transfer txn

e4fcf22

sisyphusSmiling added the Feature label Oct 31, 2023

sisyphusSmiling self-assigned this Oct 31, 2023

sisyphusSmiling requested a review from tarakby October 31, 2023 20:55

sisyphusSmiling mentioned this pull request Nov 2, 2023

Pseudo-random generator statistical tests #4

Merged

tarakby reviewed Nov 3, 2023

View reviewed changes

tarakby reviewed Nov 4, 2023

View reviewed changes

sisyphusSmiling and others added 7 commits November 6, 2023 15:14

Apply suggestions from code review

17a1416

Co-authored-by: Tarak Ben Youssef <[email protected]>

update bigEndianBytesToWord64() pre-condition & message

30bbdc4

update instances of PRG.bytesToWord64() to .bigEndianBytesToWord64()

19c1190

remove PRG.sourceOfRandomness field - state storage not necessary

f1d006f

update PRG.nextUInt64() final xor shift

3645694

rename PseudoRandomGenerator to XorShift128Plus & rename instances

90ca8fe

update prg.next_uint64() calling transactions distinguishing single v…

8e7f756

…. iterative

sisyphusSmiling marked this pull request as ready for review November 6, 2023 22:37

sisyphusSmiling requested a review from a team November 6, 2023 22:37

rename PseudoRandomGenerator.cdc to XorShift128Plus.cdc

d1dd0fc

tarakby reviewed Nov 7, 2023

View reviewed changes

joshuahannan approved these changes Nov 7, 2023

View reviewed changes

sisyphusSmiling added 5 commits November 7, 2023 16:32

remove RandomBeaconHistory transactions

7e44dda

rename XorShift128Plus to Xorshift128plus & update instances

9e5e9c4

refactor Xorshift128plus.PRG from resource to struct & update its ins…

ce51fcd

…tances

add getter script for array of random UInt64 from prg

d980bae

simplify next_uint64 script & add comment

8a392d0

sisyphusSmiling added 6 commits November 7, 2023 17:05

clarify script comments

e05641a

fix iterative_next_uint64 txn and clarify comments

850cca7

fix PRG instantiation in CoinToss to match new interface

d9eecda

fix iterative_next_uint64 PRG instantiation

6e963b3

add clarifying comments to Xorshift128plus contract

5f5ea13

fix PRG instantiation in script to match new interface

c439f1c

sisyphusSmiling changed the base branch from pre-prg-impl to main November 7, 2023 23:17

sisyphusSmiling changed the base branch from main to pre-prg-impl November 7, 2023 23:17

sisyphusSmiling and others added 6 commits November 7, 2023 17:22

update CONTRIBUTING to name random-coin-toss repo

1f72ce7

Apply suggestions from code review

cbf5d3e

Co-authored-by: Tarak Ben Youssef <[email protected]>

update Xorshift128plus contract with PR feedback

e9157d5

Rename XorShift128Plus.cdc to Xorshift128plus.cdc

61cc807

update Xorshift128plus comments & error messages

ccf6d0e

add comments to CoinToss contract

2f578d6

tarakby reviewed Nov 8, 2023

View reviewed changes

contracts/CoinToss.cdc Outdated Show resolved Hide resolved

Update CoinToss.cdc

256c1b2

Co-authored-by: Tarak Ben Youssef <[email protected]>

tarakby approved these changes Nov 8, 2023

View reviewed changes

update README info dialog about WIP status

bf00c4e

sisyphusSmiling merged commit c4d2b21 into pre-prg-impl Nov 8, 2023

sisyphusSmiling mentioned this pull request Nov 8, 2023

Update xorshift128+ PRG implementation for our source of randomness #5

Closed

sisyphusSmiling deleted the update-prg-impl branch November 8, 2023 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update PRG interface and implementation #6

Update PRG interface and implementation #6

sisyphusSmiling commented Oct 31, 2023

tarakby left a comment

tarakby Nov 2, 2023

sisyphusSmiling Nov 6, 2023

tarakby Nov 7, 2023

tarakby Nov 3, 2023

tarakby Nov 3, 2023

sisyphusSmiling Nov 6, 2023 •

edited

Loading

tarakby Nov 3, 2023

sisyphusSmiling Nov 6, 2023

tarakby Nov 7, 2023

sisyphusSmiling Nov 7, 2023 •

edited

Loading

tarakby Nov 7, 2023

sisyphusSmiling Nov 8, 2023

tarakby Nov 3, 2023

sisyphusSmiling Nov 6, 2023

tarakby Nov 7, 2023

sisyphusSmiling Nov 7, 2023

tarakby left a comment

tarakby left a comment

tarakby Nov 7, 2023

tarakby Nov 7, 2023

sisyphusSmiling Nov 7, 2023

tarakby Nov 7, 2023

joshuahannan Nov 7, 2023

sisyphusSmiling Nov 7, 2023

tarakby Nov 8, 2023

sisyphusSmiling Nov 8, 2023

tarakby left a comment


		> :warning: This repo is still a work in progress - the underlying RandomBeaconHistory is also still a work in progress

		## Overview


		let prg <- PseudoRandomGenerator.createPRG(sourceOfRandomness: seed, salt: salt)

		let randUInt64 = prg.nextUInt64()

		# Contributing to the Non-Fungible Token Standard

		The following is a set of guidelines for contributing to the Flow NFT standard. These are mostly guidelines, not rules. Use your best judgment, and feel free to propose changes to this document in a pull request.

		@@ -0,0 +1,32 @@
		# [WIP] Random Coin Toss

		> :warning: This repo is still a work in progress - the underlying RandomBeaconHistory is also still a work in progress

Update PRG interface and implementation #6

Update PRG interface and implementation #6

Conversation

sisyphusSmiling commented Oct 31, 2023

tarakby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sisyphusSmiling Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sisyphusSmiling Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarakby left a comment

Choose a reason for hiding this comment

tarakby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarakby left a comment

Choose a reason for hiding this comment

sisyphusSmiling Nov 6, 2023 •

edited

Loading

sisyphusSmiling Nov 7, 2023 •

edited

Loading