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

Fix Cadence's unsafe random #459

Merged
merged 6 commits into from
Aug 9, 2023
Merged

Fix Cadence's unsafe random #459

merged 6 commits into from
Aug 9, 2023

Conversation

tarakby
Copy link
Contributor

@tarakby tarakby commented Aug 4, 2023

Closes onflow/cadence#2691

Emulator now supports Cadence random function(s). The randoms are currently deterministic and equal for all blocks.


For contributor use:

  • Targeted PR against master branch
  • Linked to GitHub issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Re-reviewed Files changed in the GitHub PR explorer
  • Added appropriate labels

@tarakby tarakby mentioned this pull request Aug 4, 2023
6 tasks
// which provides a source of entropy to fvm context (required for Cadence's randomness).
type dummyEntropyProvider struct{}

var dummySource = make([]byte, 32)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This causes randomness on emulator to be:

  1. deterministic
  2. same for all txs/blocks

Feel free to update this to be any behaviour you prefer.

@codecov-commenter
Copy link

Codecov Report

Merging #459 (39979a1) into master (d770012) will increase coverage by 0.10%.
The diff coverage is 37.50%.

@@            Coverage Diff             @@
##           master     #459      +/-   ##
==========================================
+ Coverage   54.89%   55.00%   +0.10%     
==========================================
  Files          28       28              
  Lines        3614     3620       +6     
==========================================
+ Hits         1984     1991       +7     
  Misses       1470     1470              
+ Partials      160      159       -1     
Flag Coverage Δ
unittests 55.00% <37.50%> (+0.10%) ⬆️

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

Files Changed Coverage Δ
server/access/rest.go 0.00% <0.00%> (ø)
emulator/blockchain.go 72.41% <100.00%> (+0.07%) ⬆️

... and 1 file with indirect coverage changes

@tarakby tarakby marked this pull request as ready for review August 5, 2023 00:54
@tarakby tarakby changed the title [WIP] fix Cadence's unsafe random Fix Cadence's unsafe random Aug 5, 2023
@bluesign
Copy link
Collaborator

bluesign commented Aug 7, 2023

does this break mainnet/testnet transaction replay ( simulation ) ? Is there a way we can solve this ? maybe by getting block (transaction ) random seed from archive nodes or similar?

@janezpodhostnik
Copy link
Contributor

does this break mainnet/testnet transaction replay ( simulation ) ? Is there a way we can solve this ? maybe by getting block (transaction ) random seed from archive nodes or similar?

yes, it does break it... but, like you said, we can fix it by exposing the random beacon.
@tarakby is there anyway to access the random beacon for sealed blocks?

@bluesign
Copy link
Collaborator

bluesign commented Aug 8, 2023

does this break mainnet/testnet transaction replay ( simulation ) ? Is there a way we can solve this ? maybe by getting block (transaction ) random seed from archive nodes or similar?

yes, it does break it... but, like you said, we can fix it by exposing the random beacon. @tarakby is there anyway to access the random beacon for sealed blocks?

maybe we can add to transaction result as random seed.

@tarakby
Copy link
Contributor Author

tarakby commented Aug 8, 2023

@tarakby is there anyway to access the random beacon for sealed blocks?

The issue with the source of randomness is that it is part of the block payload data not the execution state. Archive nodes and other nodes in general are not required to store these data for all the history (most of them only store data since the root block of the current spork only, but not before that).

maybe we can add to transaction result as random seed.

Yes this is a good idea - We can restrict it only to transactions that used Cadence's random function, since most of the transactions do not need it.

I have no idea how to plug that seed into the emulator later when replaying a transaction. Does this PR need further changes to accommodate that?

@bluesign
Copy link
Collaborator

bluesign commented Aug 8, 2023

I think we can merge this in the mean time, for sure random seed in tx result would take some time

@btspoony
Copy link

Hi @turbolent, Approximately when can this PR be included in a certain release of flow-cli?

@turbolent
Copy link
Member

@btspoony I only assist with reviews in this project. Please open an issue in this and the CLI repo. Thank you!

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.

BUG: unsafeRandom() is not working in stable cadence on the emulator
6 participants