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

Add panic after continuing storage iteration after a storage mutation #1892

Merged
merged 4 commits into from
Aug 22, 2022

Conversation

dsainati1
Copy link
Contributor

Closes #208

Because modifying storage can arbitrarily rearrange the way that values are stored within it, we want to prevent users from iterating over a mutated storage. Thus, if users continue a storage iteration after performing a mutating operation, we will panic and abort execution. If users end iteration after their mutation however, this is safe, as no operations are performed that rely on the stored values maintaining their original, pre-mutation, arrangement.


  • 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
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@codecov
Copy link

codecov bot commented Aug 18, 2022

Codecov Report

Merging #1892 (8ab6195) into master (38e80fb) will increase coverage by 0.22%.
The diff coverage is 92.85%.

❗ Current head 8ab6195 differs from pull request most recent head 7eea70e. Consider uploading reports for the commit 7eea70e to get more accurate results

@@            Coverage Diff             @@
##           master    #1892      +/-   ##
==========================================
+ Coverage   77.65%   77.87%   +0.22%     
==========================================
  Files         302      300       -2     
  Lines       61976    62239     +263     
==========================================
+ Hits        48126    48470     +344     
+ Misses      12128    12050      -78     
+ Partials     1722     1719       -3     
Flag Coverage Δ
unittests 77.87% <92.85%> (+0.22%) ⬆️

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

Impacted Files Coverage Δ
runtime/interpreter/errors.go 26.94% <0.00%> (-0.25%) ⬇️
runtime/interpreter/interpreter.go 89.70% <100.00%> (+0.70%) ⬆️
runtime/interpreter/storage.go 69.04% <0.00%> (-1.59%) ⬇️
runtime/environment.go 88.96% <0.00%> (-0.18%) ⬇️
runtime/interpreter/interpreter_statement.go 91.79% <0.00%> (-0.09%) ⬇️
runtime/repl.go 0.00% <0.00%> (ø)
runtime/stackdepth.go
runtime/interpreter/sharedstate.go
runtime/ast/expression.go 88.82% <0.00%> (+0.35%) ⬆️
runtime/interpreter/value.go 72.14% <0.00%> (+0.42%) ⬆️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link

github-actions bot commented Aug 18, 2022

Cadence Benchstat comparison

This branch with compared with the base branch onflow:master commit a22f767
The command for i in {1..N}; do go test ./... -run=XXX -bench=. -benchmem -shuffle=on; done was used.
Bench tests were run a total of 7 times on each branch.

Collapsed results for better readability

old.txtnew.txt
time/opdelta
CheckContractInterfaceFungibleTokenConformance-2169µs ± 3%167µs ± 1%~(p=0.310 n=6+6)
ContractInterfaceFungibleToken-246.8µs ± 3%48.7µs ±10%~(p=0.073 n=7+7)
InterpretRecursionFib-23.00ms ± 2%3.03ms ± 3%~(p=0.445 n=6+7)
NewInterpreter/new_interpreter-21.28µs ± 1%1.31µs ± 2%~(p=0.147 n=6+7)
NewInterpreter/new_sub-interpreter-2808ns ± 2%811ns ± 3%~(p=0.836 n=7+6)
ParseArray-29.75ms ± 2%10.04ms ± 6%~(p=0.295 n=6+7)
ParseDeploy/byte_array-215.0ms ± 6%14.8ms ± 2%~(p=1.000 n=7+6)
ParseDeploy/decode_hex-21.39ms ± 4%1.37ms ± 3%~(p=0.805 n=7+7)
ParseFungibleToken/With_memory_metering-2239µs ± 6%239µs ± 2%~(p=0.710 n=7+7)
ParseFungibleToken/Without_memory_metering-2181µs ± 3%184µs ± 5%~(p=0.383 n=7+7)
ParseInfix-28.51µs ± 2%8.38µs ± 1%~(p=0.073 n=7+7)
QualifiedIdentifierCreation/One_level-23.19ns ± 2%3.16ns ± 0%~(p=0.111 n=7+6)
QualifiedIdentifierCreation/Three_levels-2163ns ± 1%165ns ± 3%~(p=0.219 n=7+7)
RuntimeFungibleTokenTransfer-2936µs ± 5%776µs ±29%~(p=0.073 n=6+7)
RuntimeResourceDictionaryValues-25.88ms ± 4%5.86ms ± 5%~(p=0.805 n=7+7)
RuntimeScriptNoop-228.6µs ±12%22.8µs ±34%~(p=0.138 n=6+7)
ValueIsSubtypeOfSemaType-2105ns ± 4%105ns ± 9%~(p=0.782 n=7+7)
 
alloc/opdelta
CheckContractInterfaceFungibleTokenConformance-262.0kB ± 0%62.0kB ± 0%~(all equal)
ContractInterfaceFungibleToken-224.3kB ± 0%24.3kB ± 0%~(p=1.000 n=7+7)
InterpretRecursionFib-21.23MB ± 0%1.23MB ± 0%~(p=1.000 n=6+6)
NewInterpreter/new_interpreter-2832B ± 0%848B ± 0%+1.92%(p=0.001 n=7+7)
NewInterpreter/new_sub-interpreter-2312B ± 0%312B ± 0%~(all equal)
ParseArray-22.77MB ± 0%2.80MB ± 2%~(p=0.366 n=6+7)
ParseDeploy/byte_array-24.49MB ± 4%4.55MB ± 0%~(p=0.530 n=7+5)
ParseDeploy/decode_hex-2214kB ± 0%214kB ± 0%~(p=0.774 n=7+7)
ParseFungibleToken/With_memory_metering-236.3kB ± 0%36.3kB ± 0%~(p=0.110 n=7+7)
ParseFungibleToken/Without_memory_metering-236.3kB ± 0%36.3kB ± 0%~(p=0.898 n=7+7)
ParseInfix-22.17kB ± 0%2.17kB ± 0%~(p=0.592 n=7+7)
QualifiedIdentifierCreation/One_level-20.00B 0.00B ~(all equal)
QualifiedIdentifierCreation/Three_levels-264.0B ± 0%64.0B ± 0%~(all equal)
RuntimeFungibleTokenTransfer-2116kB ± 1%116kB ± 1%~(p=0.097 n=7+7)
RuntimeResourceDictionaryValues-22.27MB ± 0%2.27MB ± 0%~(p=0.710 n=7+7)
RuntimeScriptNoop-29.24kB ± 0%9.25kB ± 0%~(p=0.515 n=7+7)
ValueIsSubtypeOfSemaType-248.0B ± 0%48.0B ± 0%~(all equal)
 
allocs/opdelta
CheckContractInterfaceFungibleTokenConformance-21.03k ± 0%1.03k ± 0%~(all equal)
ContractInterfaceFungibleToken-2428 ± 0%428 ± 0%~(all equal)
InterpretRecursionFib-222.5k ± 0%22.5k ± 0%~(all equal)
NewInterpreter/new_interpreter-214.0 ± 0%14.0 ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-26.00 ± 0%6.00 ± 0%~(all equal)
ParseArray-270.0k ± 0%70.0k ± 0%~(p=0.303 n=5+7)
ParseDeploy/byte_array-2105k ± 0%105k ± 0%~(p=0.780 n=7+7)
ParseDeploy/decode_hex-277.0 ± 0%77.0 ± 0%~(all equal)
ParseFungibleToken/With_memory_metering-21.06k ± 0%1.06k ± 0%~(all equal)
ParseFungibleToken/Without_memory_metering-21.06k ± 0%1.06k ± 0%~(all equal)
ParseInfix-266.0 ± 0%66.0 ± 0%~(all equal)
QualifiedIdentifierCreation/One_level-20.00 0.00 ~(all equal)
QualifiedIdentifierCreation/Three_levels-22.00 ± 0%2.00 ± 0%~(all equal)
RuntimeFungibleTokenTransfer-22.26k ± 0%2.26k ± 0%~(p=0.245 n=6+7)
RuntimeResourceDictionaryValues-236.9k ± 0%36.9k ± 0%~(p=0.564 n=7+6)
RuntimeScriptNoop-2149 ± 0%149 ± 0%~(all equal)
ValueIsSubtypeOfSemaType-21.00 ± 0%1.00 ± 0%~(all equal)
 

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice! This is going to make the iteration feature even more useful and ergonomic 👍

docs/language/accounts.mdx Outdated Show resolved Hide resolved
docs/language/accounts.mdx Show resolved Hide resolved
runtime/interpreter/interpreter.go Outdated Show resolved Hide resolved
runtime/interpreter/interpreter.go Show resolved Hide resolved
runtime/tests/interpreter/account_test.go Show resolved Hide resolved
@dsainati1 dsainati1 requested a review from turbolent August 22, 2022 17:57
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

👍

@dsainati1 dsainati1 merged commit 1231268 into master Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Storage Querying API
2 participants