Fix erroneous report of resource invalidation when executing a pre-condition #1625

dsainati1 · 2022-05-05T16:18:08Z

Fixes issue reported in Discord

Description

The safety analysis for resource validation, which was introduced in #1464, was not properly handling pre and post conditions for functions in composites that implemented interfaces. In order to implement this behavior, we "wrap" the composite function in another "function" that contains the code for the pre and post conditions, and redeclare all the parameters. If one of the parameters is given a resource-kinded argument, this tricks the analysis into thinking this resource is being duplicated, when it is not.

To handle this, we invalidate any resources that are parameters to the precondition before executing the body, then re-validate them after the body, before executing the post condition.

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

…urce-reference

github-actions · 2022-05-05T16:31:58Z

Cadence Benchstat comparison

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

Results

	old.txt	new.txt
	time/op		delta
InterpretRecursionFib-2	2.66ms ± 2%	2.77ms ± 2%	+4.25%	(p=0.001 n=7+7)
ParseArray-2	12.2ms ± 0%	12.4ms ± 2%	+1.83%	(p=0.015 n=6+6)
ParseInfix-2	8.39µs ± 1%	8.51µs ± 2%	+1.40%	(p=0.011 n=7+7)
ContractInterfaceFungibleToken-2	38.9µs ± 1%	39.2µs ± 1%	+0.86%	(p=0.035 n=6+7)
RuntimeResourceDictionaryValues-2	6.34ms ± 1%	6.34ms ± 1%	~	(p=1.000 n=6+5)
RuntimeFungibleTokenTransfer-2	1.07ms ±34%	1.10ms ±30%	~	(p=0.318 n=7+7)
Transfer-2	83.7ns ± 3%	83.8ns ± 1%	~	(p=0.434 n=7+7)
ParseFungibleToken-2	182µs ± 0%	190µs ± 6%	~	(p=0.101 n=6+7)
ParseDeploy/byte_array-2	19.9ms ±10%	19.9ms ± 9%	~	(p=0.620 n=7+7)
ParseDeploy/decode_hex-2	1.16ms ± 3%	1.15ms ± 2%	~	(p=0.620 n=7+7)
QualifiedIdentifierCreation/One_level-2	2.34ns ± 0%	2.34ns ± 0%	~	(p=0.841 n=7+6)
QualifiedIdentifierCreation/Three_levels-2	139ns ± 3%	138ns ± 1%	~	(p=0.516 n=7+7)
CheckContractInterfaceFungibleTokenConformance-2	131µs ± 3%	134µs ± 6%	~	(p=0.318 n=7+7)
NewInterpreter/new_interpreter-2	1.14µs ± 2%	1.14µs ± 1%	~	(p=0.861 n=6+7)
NewInterpreter/new_sub-interpreter-2	2.20µs ± 0%	2.21µs ± 1%	~	(p=0.122 n=6+7)

	alloc/op		delta
RuntimeResourceDictionaryValues-2	2.25MB ± 0%	2.25MB ± 0%	~	(p=0.535 n=7+7)
RuntimeFungibleTokenTransfer-2	273kB ± 0%	273kB ± 0%	~	(p=0.251 n=6+7)
Transfer-2	48.0B ± 0%	48.0B ± 0%	~	(all equal)
QualifiedIdentifierCreation/One_level-2	0.00B	0.00B	~	(all equal)
QualifiedIdentifierCreation/Three_levels-2	64.0B ± 0%	64.0B ± 0%	~	(all equal)
CheckContractInterfaceFungibleTokenConformance-2	66.3kB ± 0%	66.3kB ± 0%	~	(all equal)
ContractInterfaceFungibleToken-2	26.7kB ± 0%	26.7kB ± 0%	~	(p=1.000 n=7+7)
NewInterpreter/new_interpreter-2	848B ± 0%	848B ± 0%	~	(all equal)
NewInterpreter/new_sub-interpreter-2	1.34kB ± 0%	1.34kB ± 0%	~	(all equal)
InterpretRecursionFib-2	1.14MB ± 0%	1.14MB ± 0%	~	(p=0.409 n=7+7)

	allocs/op		delta
RuntimeResourceDictionaryValues-2	37.6k ± 0%	37.6k ± 0%	~	(p=0.452 n=7+7)
RuntimeFungibleTokenTransfer-2	4.58k ± 0%	4.58k ± 0%	~	(p=0.298 n=7+7)
Transfer-2	1.00 ± 0%	1.00 ± 0%	~	(all equal)
QualifiedIdentifierCreation/One_level-2	0.00	0.00	~	(all equal)
QualifiedIdentifierCreation/Three_levels-2	2.00 ± 0%	2.00 ± 0%	~	(all equal)
CheckContractInterfaceFungibleTokenConformance-2	1.07k ± 0%	1.07k ± 0%	~	(all equal)
ContractInterfaceFungibleToken-2	460 ± 0%	460 ± 0%	~	(all equal)
NewInterpreter/new_interpreter-2	13.0 ± 0%	13.0 ± 0%	~	(all equal)
NewInterpreter/new_sub-interpreter-2	40.0 ± 0%	40.0 ± 0%	~	(all equal)
InterpretRecursionFib-2	23.8k ± 0%	23.8k ± 0%	~	(all equal)

codecov · 2022-05-05T16:33:16Z

Codecov Report

Merging #1625 (6d18f84) into master (f615bea) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1625      +/-   ##
==========================================
+ Coverage   74.73%   74.76%   +0.02%     
==========================================
  Files         288      288              
  Lines       55340    55382      +42     
==========================================
+ Hits        41357    41404      +47     
+ Misses      12489    12483       -6     
- Partials     1494     1495       +1

Flag	Coverage Δ
unittests	`74.76% <100.00%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
runtime/interpreter/interpreter.go	`88.88% <100.00%> (+0.17%)`	⬆️
runtime/interpreter/value.go	`63.66% <0.00%> (+0.03%)`	⬆️
runtime/interpreter/errors.go	`23.11% <0.00%> (+1.00%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f615bea...6d18f84. Read the comment docs.

dsainati1 · 2022-05-05T16:36:04Z

runtime/missingmember_test.go

@@ -4163,3 +4163,837 @@ transaction(recipient: Address, amount: UFix64) {
 	)
 	require.NoError(t, err)
 }
+
+func TestRuntimeMissingMemberExampleMarketplace(t *testing.T) {


This test currently reports "no value for member" as detailed in #1539

The issue with resource invalidation was originally discovered as it was preventing reproducing this behavior. I have added this test here to document this behavior, with a comment indicating that its behavior is not ideal. I will work on another PR to fix the issue here.

turbolent

Great work here @dsainati1, both the analysis of the root problem and the solution are 👌

Like you described, in addition to the inner function body, also the condition wrapper functions bind the arguments, which leads to multiple variables being associated with the same resource.

Temporarily un-tracking the resource-kinded arguments and restoring the tracking makes sense.

runtime/interpreter/interpreter.go

runtime/missingmember_test.go

runtime/runtime_test.go

Co-authored-by: Bastian Müller <[email protected]>

turbolent

Great work!

@SupunS can you please also have a look, as you authored the run-time resource tracking facility 🙏

SupunS

Nice! 👌

dsainati1 added 7 commits April 26, 2022 12:31

add test to repro error

8f05eba

uncommit contract

ca080e8

add tests for pre and post conditions with resources

b126abf

Merge branch 'master' of github.com:onflow/cadence into optional-reso…

37744fa

…urce-reference

invalidate and revalidate resource around bodies of wrapped functions

d36dac1

add additional test

8a83b8c

remove redundant test

e499e83

dsainati1 added the Bugfix label May 5, 2022

dsainati1 self-assigned this May 5, 2022

dsainati1 requested review from turbolent and SupunS as code owners May 5, 2022 16:18

dsainati1 changed the title ~~Optional resource reference~~ Fix erroneous report of resource invalidation when executing a pre-condition May 5, 2022

fix lint

22a5801

dsainati1 commented May 5, 2022

View reviewed changes

turbolent reviewed May 5, 2022

View reviewed changes

runtime/interpreter/interpreter.go Outdated Show resolved Hide resolved

runtime/missingmember_test.go Show resolved Hide resolved

runtime/missingmember_test.go Show resolved Hide resolved

runtime/runtime_test.go Outdated Show resolved Hide resolved

dsainati1 and others added 4 commits May 6, 2022 10:38

Update runtime/interpreter/interpreter.go

262337a

Co-authored-by: Bastian Müller <[email protected]>

Update runtime/missingmember_test.go

cc8ffe8

Co-authored-by: Bastian Müller <[email protected]>

move tests to interpreter module

386b399

add test for additional params

fdc2d84

dsainati1 requested a review from turbolent May 6, 2022 15:22

turbolent approved these changes May 6, 2022

View reviewed changes

formatting

6d18f84

SupunS approved these changes May 6, 2022

View reviewed changes

dsainati1 merged commit 33b1f89 into master May 6, 2022

turbolent mentioned this pull request May 6, 2022

Auto Cadence Update: Fix erroneous report of resource invalidation when executing a pre-condition onflow/flow-go#2380

Closed

turbolent deleted the optional-resource-reference branch May 6, 2022 23:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix erroneous report of resource invalidation when executing a pre-condition #1625

Fix erroneous report of resource invalidation when executing a pre-condition #1625

dsainati1 commented May 5, 2022 •

edited by turbolent

Loading

github-actions bot commented May 5, 2022 •

edited

Loading

codecov bot commented May 5, 2022 •

edited

Loading

dsainati1 May 5, 2022

turbolent left a comment

turbolent left a comment

SupunS left a comment

Fix erroneous report of resource invalidation when executing a pre-condition #1625

Fix erroneous report of resource invalidation when executing a pre-condition #1625

Conversation

dsainati1 commented May 5, 2022 • edited by turbolent Loading

Description

github-actions bot commented May 5, 2022 • edited Loading

Cadence Benchstat comparison

Results

codecov bot commented May 5, 2022 • edited Loading

Codecov Report

dsainati1 May 5, 2022

Choose a reason for hiding this comment

turbolent left a comment

Choose a reason for hiding this comment

turbolent left a comment

Choose a reason for hiding this comment

SupunS left a comment

Choose a reason for hiding this comment

dsainati1 commented May 5, 2022 •

edited by turbolent

Loading

github-actions bot commented May 5, 2022 •

edited

Loading

codecov bot commented May 5, 2022 •

edited

Loading