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

[FVM] unsafeRandom for script execution does not error #4604

Merged
merged 1 commit into from
Aug 5, 2023

Conversation

tarakby
Copy link
Contributor

@tarakby tarakby commented Aug 4, 2023

This PR enables cadence's unsafeRandom for script execution instead of erroring. This allows scripts not to panic although the returned random value is not equal to the value returned when run as a transaction.

This behaviour is clarified in onflow/flips#120 (onflow/flips@051c82b)

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2023

FVM Benchstat comparison

This branch with compared with the base branch onflow:master commit 6ece1c3

The command (for i in {1..7}; do go test ./fvm ./engine/execution/computation --bench . --tags relic -shuffle=on --benchmem --run ^$; done) was used.

Collapsed results for better readability

old.txtnew.txt
time/opdelta
ComputeBlock/16/cols/128/txes/1/max-concurrency-26.08s ± 0%6.17s ± 0%~(p=1.000 n=1+1)
ComputeBlock/16/cols/128/txes/2/max-concurrency-26.43s ± 0%6.47s ± 0%~(p=1.000 n=1+1)
 
us/txdelta
ComputeBlock/16/cols/128/txes/1/max-concurrency-22.97k ± 0%3.01k ± 0%~(p=1.000 n=1+1)
ComputeBlock/16/cols/128/txes/2/max-concurrency-23.14k ± 0%3.16k ± 0%~(p=1.000 n=1+1)
 
alloc/opdelta
ComputeBlock/16/cols/128/txes/1/max-concurrency-21.67GB ± 0%1.65GB ± 0%~(p=1.000 n=1+1)
ComputeBlock/16/cols/128/txes/2/max-concurrency-21.71GB ± 0%1.70GB ± 0%~(p=1.000 n=1+1)
 
allocs/opdelta
ComputeBlock/16/cols/128/txes/1/max-concurrency-222.0M ± 0%21.9M ± 0%~(p=1.000 n=1+1)
ComputeBlock/16/cols/128/txes/2/max-concurrency-222.6M ± 0%22.6M ± 0%~(p=1.000 n=1+1)
 

@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2023

Codecov Report

Merging #4604 (e97a236) into master (2528190) will decrease coverage by 1.70%.
Report is 479 commits behind head on master.
The diff coverage is 63.25%.

@@            Coverage Diff             @@
##           master    #4604      +/-   ##
==========================================
- Coverage   56.25%   54.55%   -1.70%     
==========================================
  Files         653      915     +262     
  Lines       64699    85352   +20653     
==========================================
+ Hits        36396    46567   +10171     
- Misses      25362    35200    +9838     
- Partials     2941     3585     +644     
Flag Coverage Δ
unittests 54.55% <63.25%> (-1.70%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
cmd/bootstrap/transit/cmd/utils.go 31.54% <ø> (ø)
cmd/execution_builder.go 0.00% <0.00%> (ø)
engine/access/rest/middleware/metrics.go 0.00% <0.00%> (ø)
engine/access/rest/routes/execution_result.go 56.75% <0.00%> (ø)
engine/access/rpc/backend/backend_network.go 34.11% <0.00%> (ø)
engine/access/state_stream/engine.go 0.00% <0.00%> (ø)
engine/common/rpc/convert/blocks.go 78.76% <ø> (ø)
fvm/environment/env.go 100.00% <ø> (ø)
model/flow/identifierList.go 22.36% <0.00%> (+0.84%) ⬆️
model/verification/chunkDataPackRequest.go 43.75% <0.00%> (-6.25%) ⬇️
... and 101 more

... and 245 files with indirect coverage changes

@tarakby tarakby merged commit 04e12b3 into master Aug 5, 2023
@tarakby tarakby deleted the tarak/fvm-random-script-emulator-fix branch August 5, 2023 00:29
@tarakby tarakby changed the title [FVM] enable unsafe Random for script execution [FVM] unsafeRandom for script execution does not error Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants