Pseudo-random generator statistical tests #4

sisyphusSmiling · 2023-10-26T19:52:30Z

Closes: #3

Introduces statistical tests for the xorshift128+ implementation in PseudoRandomGenerator contract, asserting a uniform distribution across a sample of UInt64 random results.

janezpodhostnik

I can only review the go code, and that looks good. But i don't know if this is the proper structure for this sort of project.

flow.json

lib/go/templates/go.mod

joshuahannan

If we're going to create new test suites, it would probably be better to use the cadence testing framework or overflow so we don't have to continue to rely on the poorly organized test suites from the other repos any more. Are either of those possible?

lib/go/contracts/contracts.go

sisyphusSmiling · 2023-10-30T16:33:50Z

@joshuahannan Agreed, I'll build out tests for the other contracts & their scripts using the Cadence testing framework. The PRG test needs to be done a non-Cadence framework since we need to run an analysis on the statistical distribution of random numbers returned from the PRG. Go is preferable here given the supporting BasicDistributionTest() method in flow-go and was raised by @tarakby in #3

joshuahannan · 2023-10-30T16:35:48Z

Are we able to use overflow though? that is much better design framework than my tests

sisyphusSmiling · 2023-10-30T16:37:26Z

I'd imagine so. I'm not familiar with it, but can look into it for the test setup & execution. Better for me to familiarize myself with it since it's been mentioned as a powerful tool.

sisyphusSmiling · 2023-10-30T20:53:06Z

Quick update: the PRG stat tests now use overflow as suggested by @joshuahannan. Since overflow pulls the contract config from local flow.json, I've moved the test & its helper to the root dir where the project's flow.json is located. And since we're using overflow, we no longer need the contract bindata or templates, so the lib/go/ directory containing both contracts/ and templates/ has been removed.

Once this PR is done and merged, I can move on to building out Cadence unit tests.

tarakby · 2023-10-31T18:20:48Z

Looking at this PR shortly, apologies for the delay

tarakby · 2023-10-31T19:23:58Z

prg_test.go

+		classWidth := (math.MaxUint64 / uint64(n)) + 1
+
+		// using the same seed, the salt varies in getNextUInt64()
+		seed := GetRandomSeed(t)


The uniformity checked in this test is different from the uniformity that we expect to test when assessing a PRG quality : this test looks at the the uniformity of the first generated output across multiple PRG instances (diversified per salt), the quality of a PRG is evaluated using successive outputs of the same PRG instance (same seed and salt).
A PRG that succeeds the first criteria may fail the second (the main quality one).

I suggest two things:

update the current test to use the same PRG instance and evaluate the uniformity of it successive outputs.

add another test, to make sure both the seed and salt are taken into account in the implementation. This doesn't need to be a statistical test. It's enough to check that two instances with different seeds but same salt output different randoms (the first output is enough to check), and then the same for 2 instances with the same seed but different salts.

This is really helpful, thank you! I'll make these changes shortly.

tarakby · 2023-10-31T19:26:52Z

go.mod

+require (
+	github.com/bjartek/overflow v1.14.1
+	github.com/onflow/cadence v0.42.2
+	github.com/onflow/flow-go/crypto v0.24.9


I made a recent PR to improve the statistical tests (onflow/flow-go#4844). It can help reduce flakiness in the test. Maybe I can merge that PR and tag a new version to be used here first.

Sounds good. I'll bump the version once it's ready to go.

I just merged the PR and tagged a new version v0.24.10 that you can use

prg_test_helpers.go

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

Update PRG interface and implementation

…ft128plus

…e while testing

sisyphusSmiling · 2023-11-08T15:50:01Z

Quick update - PR should be ready for another round of reviews. Some notes on changes:

PRG implementation has been updated with changes from Update PRG interface and implementation #6
PRG initialization had a bug created during previous refactor and has been corrected after discovery while testing
Generating sufficient quantity of random numbers for BasicDistributionTest required persisting PRG state across a number of transactions (due to computational limits) and recording generated random numbers. In order to do this, I introduced a simple helper contract for this express purpose - RandomResultStorage. As suggested by @tarakby in this comment, I first generated the numbers, then query the contract to retrieve the generated results - both steps done in batches to prevent reaching computational limits. These results are then passed to BasicDistributionTest to assess uniformity of distribution
Two additional test cases have been added, covering alternate random results from same seed + different salts and different seeds + same salt

Note that the sample size assignment in the result distribution test had to be hardcoded due to its calculation in BasicDistributionTest. This is not ideal as it creates a fragile test, breaking if/when the sample size constants are updated in crypto/random/rand_utils. Open to suggestions on making this more robust.

tarakby

Nice and clean work in such a short time 👌🏼 Thank you!

I didn't think of the complexity of having to batch the queries (and the txs and scripts that had to be implemented). I suggest to merge the PR in its current state, and that we continue to think of a way to simplify the tests. We can add/modify the tools in crypto/random if that helps.
In particular I'm thinking if there is a way to make a transaction return a value somehow (maybe through the transaction results?).
Suggestions on simplifying the tests are welcome!

tarakby · 2023-11-08T17:22:04Z

transactions/coin-toss/1_reveal_coin_toss.cdc

        let receipt <- signer.load<@CoinToss.Receipt>(from: CoinToss.ReceiptStoragePath)
            ?? panic("No Receipt found!")

-        // Reveal by redeeming my receipt
+        // Reveal by redeeming my receipt - fingers crossed!


prg_test_helpers.go

tarakby · 2023-11-08T17:28:27Z

prg_test_helpers.go

+// calls a script which creates a new PRG struct from the given seed and
+// random salt, gets the next uint64 and destroys the prg resource before
+// returning the next uint64
+func GetNextUInt64NewPRGRandSalt(


I think this function isn't used and can be deleted

tarakby · 2023-11-08T18:05:02Z

prg_test.go

+
+		// get the results in batches again due to query computational limit
+		results := make([]uint64, sampleSize)
+		ProcessBatches(sampleSize, maxBatchSize, func(startIdx, batchSize int) {


apologies, my suggestion on how to structure the test wasn't simple to implement! I missed the computation limit on both the transaction (generating the randoms) and script (reading the randoms), and didn't think we may need to perform that in batches.
Nice and clean implementation 👌🏼

tarakby · 2023-11-08T18:07:57Z

prg_test.go

+		var sampleSize int
+
+		// taken from BasicDistributionTest constants - hardcoding here is
+		/// fragile, but it's determined within flow-go/crypto/rand_utils


I agree this part to get the sampleSize isn't ideal, I also missed that this should have been exported by the tools in flow-go/crypto/random just like BasicDistributionTest.

tarakby · 2023-11-08T18:23:12Z

My last comment is about the repo structure and whether developers can be confused about what’s a test, example and production code. For instance /contracts contains 2 production contracts (history + xorshift) and 1 example (cointoss). Same for /transaction where there are also test transactions. I’m wondering if it’s better to have a /test subfolder and /example subfolder under /contracts , /transactions and /scripts , or another way to clear the possible confusion.

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

sisyphusSmiling · 2023-11-08T18:45:37Z

Thank you @tarakby!

My last comment is about the repo structure and whether developers can be confused about what’s a test, example and production code. For instance /contracts contains 2 production contracts (history + xorshift) and 1 example (cointoss). Same for /transaction where there are also test transactions. I’m wondering if it’s better to have a /test subfolder and /example subfolder under /contracts , /transactions and /scripts , or another way to clear the possible confusion.

Good point. Just to make sure main is updated ASAP, I'll merge PR as-is and create a fast-follow PR with a directory restructure.

sisyphusSmiling added 6 commits October 26, 2023 09:51

add initial go tests

74898bf

add ci workflow

4f0cb30

update flow config with RandomBeaconHistory aliases

96d6e4c

update go test package

0dc510a

add go script & txn templates

e65b0aa

update contracts/go.mod formatting

b8d5739

sisyphusSmiling self-assigned this Oct 26, 2023

sisyphusSmiling added 2 commits October 27, 2023 18:59

add prg distribution test

29c63a0

pin flow-cli version to v1.4.5 in ci workflow

e156b1b

sisyphusSmiling changed the title ~~[WIP] Pseudo random generator statistical tests~~ Pseudo-random generator statistical tests Oct 28, 2023

sisyphusSmiling marked this pull request as ready for review October 28, 2023 00:19

sisyphusSmiling requested a review from a team October 28, 2023 00:20

fix implementation detail in prg_tests & update comments

6adf6f5

sisyphusSmiling requested review from tarakby and janezpodhostnik October 28, 2023 13:36

janezpodhostnik approved these changes Oct 30, 2023

View reviewed changes

flow.json Outdated Show resolved Hide resolved

lib/go/templates/go.mod Outdated Show resolved Hide resolved

joshuahannan reviewed Oct 30, 2023

View reviewed changes

lib/go/contracts/contracts.go Outdated Show resolved Hide resolved

sisyphusSmiling added 2 commits October 30, 2023 14:12

update flow.json with contract aliases

654b4b7

move PRG test to root & update to use overflow

db70090

sisyphusSmiling requested a review from joshuahannan October 31, 2023 17:43

joshuahannan approved these changes Oct 31, 2023

View reviewed changes

tarakby reviewed Oct 31, 2023

View reviewed changes

sisyphusSmiling mentioned this pull request Oct 31, 2023

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

Closed

sisyphusSmiling and others added 2 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

sisyphusSmiling and others added 18 commits November 7, 2023 18:20

add comments to CoinToss contract

2f578d6

Update CoinToss.cdc

256c1b2

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

update README info dialog about WIP status

bf00c4e

Merge pull request #6 from onflow/update-prg-impl

c4d2b21

Update PRG interface and implementation

Merge branch 'update-prg-impl' into prg-tests

5f32620

remove references to PseudoRandomGenerator in favor of renamed Xorshi…

bfcc703

…ft128plus

update ci workflow to run prg tests on PR only

500777a

bump flow-go/crypto to v0.24.10

36a077d

fix bug in PRG state initialization

b7e34c8

add prg test cases

d23fbc6

update next_uint64 script signature & instance in tests

4dff925

remove WIP notice from README

62ef960

add RandomResultStorage helper contract & scripts to persist PRG stat…

cb10350

…e while testing

add RandomBeaconHistory aliases in flow.json

9ce0fed

fix bug in prg_test_helpers ProcessBatches method

dbad339

add comments to RandomResultStorage scripts & txns

0a03bb4

add clarifying comments to CoinToss contract & scripts

3e70948

fix distribution test sampleSize calculation

cb62aac

sisyphusSmiling requested a review from tarakby November 8, 2023 15:40

sisyphusSmiling requested a review from joshuahannan November 8, 2023 15:50

sisyphusSmiling added 2 commits November 8, 2023 10:40

update ci workflow to only run on PR with changes to Xorshift128plus

3db48f2

add @onflow/flow-smart-contracts as CODEOWNERS

2d32ea7

tarakby approved these changes Nov 8, 2023

View reviewed changes

Update prg_test_helpers.go

10596ce

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

sisyphusSmiling merged commit 17767ff into main Nov 8, 2023

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

sisyphusSmiling mentioned this pull request Nov 8, 2023

Restructure project directory for clarity #8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pseudo-random generator statistical tests #4

Pseudo-random generator statistical tests #4

sisyphusSmiling commented Oct 26, 2023 •

edited

Loading

janezpodhostnik left a comment

joshuahannan left a comment

sisyphusSmiling commented Oct 30, 2023

joshuahannan commented Oct 30, 2023

sisyphusSmiling commented Oct 30, 2023

sisyphusSmiling commented Oct 30, 2023 •

edited

Loading

tarakby commented Oct 31, 2023

tarakby Oct 31, 2023

sisyphusSmiling Oct 31, 2023

tarakby Oct 31, 2023

sisyphusSmiling Oct 31, 2023

tarakby Nov 8, 2023

sisyphusSmiling commented Nov 8, 2023 •

edited

Loading

tarakby left a comment

tarakby Nov 8, 2023

tarakby Nov 8, 2023

tarakby Nov 8, 2023

tarakby Nov 8, 2023

tarakby commented Nov 8, 2023

sisyphusSmiling commented Nov 8, 2023

Pseudo-random generator statistical tests #4

Pseudo-random generator statistical tests #4

Conversation

sisyphusSmiling commented Oct 26, 2023 • edited Loading

janezpodhostnik left a comment

Choose a reason for hiding this comment

joshuahannan left a comment

Choose a reason for hiding this comment

sisyphusSmiling commented Oct 30, 2023

joshuahannan commented Oct 30, 2023

sisyphusSmiling commented Oct 30, 2023

sisyphusSmiling commented Oct 30, 2023 • edited Loading

tarakby commented Oct 31, 2023

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 commented Nov 8, 2023 • edited Loading

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

tarakby commented Nov 8, 2023

sisyphusSmiling commented Nov 8, 2023

sisyphusSmiling commented Oct 26, 2023 •

edited

Loading

sisyphusSmiling commented Oct 30, 2023 •

edited

Loading

sisyphusSmiling commented Nov 8, 2023 •

edited

Loading