Check use of invalidated resources at runtime #1464

SupunS · 2022-03-01T01:25:48Z

Work towards https://github.com/dapperlabs/cadence-private-issues/issues/19

Description

This adds second layer of runtime validation to check use of invalidated (moved/destroyed) resources. This check relies on:

An external mapping, which keeps the resource-to-variable mapping.
Assumes there could be at-most one mapping for each resource value at any given time.

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

…me-resource-tracking

turbolent

Nice!

runtime/interpreter/value.go

codecov-commenter · 2022-03-01T01:36:12Z

Codecov Report

Merging #1464 (fcb87ba) into master (3705ef1) will decrease coverage by 0.02%.
The diff coverage is 77.46%.

❗ Current head fcb87ba differs from pull request most recent head d892eba. Consider uploading reports for the commit d892eba to get more accurate results

@@            Coverage Diff             @@
##           master    #1464      +/-   ##
==========================================
- Coverage   72.54%   72.51%   -0.03%     
==========================================
  Files         288      288              
  Lines       39456    39523      +67     
==========================================
+ Hits        28624    28662      +38     
- Misses       9376     9397      +21     
- Partials     1456     1464       +8

Flag	Coverage Δ
unittests	`72.51% <77.46%> (-0.03%)`	⬇️

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

Impacted Files	Coverage Δ
runtime/interpreter/interpreter.go	`86.54% <71.42%> (-0.46%)`	⬇️
runtime/interpreter/interpreter_expression.go	`82.33% <100.00%> (+0.26%)`	⬆️
runtime/interpreter/interpreter_statement.go	`85.89% <100.00%> (+0.11%)`	⬆️
runtime/interpreter/value.go	`56.15% <100.00%> (-0.20%)`	⬇️
runtime/interpreter/function.go	`51.28% <0.00%> (+2.56%)`	⬆️

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 3705ef1...d892eba. Read the comment docs.

github-actions · 2022-03-01T01:39:29Z

Cadence Benchstat comparison

This branch with compared with the base branch onflow:master commit 3705ef1
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
QualifiedIdentifierCreation/Three_levels-2	166ns ± 8%	180ns ± 7%	+8.26%	(p=0.006 n=7+7)
NewInterpreter/new_interpreter-2	1.25µs ± 8%	1.35µs ± 2%	+7.60%	(p=0.018 n=7+5)
ParseArray-2	14.3ms ± 2%	15.1ms ± 8%	+5.18%	(p=0.035 n=6+7)
CheckContractInterfaceFungibleTokenConformance-2	165µs ± 6%	173µs ± 8%	+4.88%	(p=0.038 n=7+7)
RuntimeFungibleTokenTransfer-2	1.34ms ±32%	1.39ms ±48%	~	(p=0.620 n=7+7)
RuntimeResourceDictionaryValues-2	16.7ms ± 9%	16.4ms ± 8%	~	(p=0.620 n=7+7)
ParseFungibleToken-2	227µs ± 6%	230µs ± 5%	~	(p=0.805 n=7+7)
ParseInfix-2	9.75µs ± 7%	9.78µs ± 7%	~	(p=1.000 n=7+7)
ParseDeploy/byte_array-2	23.2ms ± 4%	23.4ms ± 7%	~	(p=0.589 n=6+6)
ParseDeploy/decode_hex-2	1.42ms ± 7%	1.43ms ± 8%	~	(p=0.902 n=7+7)
QualifiedIdentifierCreation/One_level-2	3.20ns ± 9%	3.22ns ± 6%	~	(p=0.620 n=7+7)
ContractInterfaceFungibleToken-2	47.1µs ± 5%	48.9µs ± 6%	~	(p=0.053 n=7+7)
InterpretRecursionFib-2	2.85ms ± 9%	2.87ms ± 8%	~	(p=1.000 n=7+7)
NewInterpreter/new_sub-interpreter-2	2.50µs ±11%	2.56µs ±11%	~	(p=0.535 n=7+7)

	alloc/op		delta
NewInterpreter/new_interpreter-2	768B ± 0%	848B ± 0%	+10.42%	(p=0.001 n=7+7)
NewInterpreter/new_sub-interpreter-2	1.24kB ± 0%	1.32kB ± 0%	+6.45%	(p=0.001 n=7+7)
RuntimeFungibleTokenTransfer-2	272kB ± 0%	273kB ± 0%	+0.24%	(p=0.001 n=7+7)
RuntimeResourceDictionaryValues-2	4.05MB ± 0%	4.05MB ± 0%	~	(p=0.902 n=7+7)
QualifiedIdentifierCreation/One_level-2	0.00B	0.00B	~	(all equal)
QualifiedIdentifierCreation/Three_levels-2	64.0B ± 0%	64.0B ± 0%	~	(all equal)
ContractInterfaceFungibleToken-2	26.6kB ± 0%	26.6kB ± 0%	~	(p=0.538 n=6+7)
CheckContractInterfaceFungibleTokenConformance-2	66.2kB ± 0%	66.2kB ± 0%	~	(all equal)
InterpretRecursionFib-2	1.24MB ± 0%	1.24MB ± 0%	~	(p=0.489 n=7+7)

	allocs/op		delta
NewInterpreter/new_interpreter-2	12.0 ± 0%	13.0 ± 0%	+8.33%	(p=0.001 n=7+7)
NewInterpreter/new_sub-interpreter-2	38.0 ± 0%	39.0 ± 0%	+2.63%	(p=0.001 n=7+7)
RuntimeFungibleTokenTransfer-2	4.52k ± 0%	4.53k ± 0%	+0.10%	(p=0.001 n=6+7)
RuntimeResourceDictionaryValues-2	102k ± 0%	102k ± 0%	~	(p=0.182 n=7+7)
QualifiedIdentifierCreation/One_level-2	0.00	0.00	~	(all equal)
QualifiedIdentifierCreation/Three_levels-2	2.00 ± 0%	2.00 ± 0%	~	(all equal)
ContractInterfaceFungibleToken-2	458 ± 0%	458 ± 0%	~	(all equal)
CheckContractInterfaceFungibleTokenConformance-2	1.07k ± 0%	1.07k ± 0%	~	(all equal)
InterpretRecursionFib-2	25.0k ± 0%	25.0k ± 0%	~	(all equal)

turbolent · 2022-03-01T17:42:37Z

@SupunS Could you please check performance locally? I'd be curious to see if it's better than what CI says the current benchstat numbers above are not quite reliable, right? (e.g. parsing is supposedly slower) Allocation increase is not that big a deal, but would be good to keep overhead <~10%

SupunS · 2022-03-01T18:29:12Z

Yeah the CI results are kind of not accurate. here. e.g: QualifiedIdentifierCreation is just testing a utility method that doesn't run any cadence code, and has no impact from the change.

local benchmark results:

name                                              old time/op    new time/op    delta
pkg:github.com/onflow/cadence/runtime goos:darwin goarch:amd64
RuntimeFungibleTokenTransfer-8                       599µs ± 4%     599µs ± 5%   ~     (p=1.000 n=3+3)
RuntimeResourceDictionaryValues-8                   9.02ms ± 1%    8.77ms ± 1%   ~     (p=0.100 n=3+3)
pkg:github.com/onflow/cadence/runtime/parser2 goos:darwin goarch:amd64
ParseDeploy/byte_array-8                            13.6ms ± 0%    14.4ms ±12%   ~     (p=1.000 n=3+3)
ParseDeploy/decode_hex-8                             841µs ± 0%     864µs ± 5%   ~     (p=0.200 n=3+3)
ParseFungibleToken-8                                 143µs ± 1%     144µs ± 3%   ~     (p=0.400 n=3+3)
ParseInfix-8                                        6.58µs ± 2%    6.53µs ± 1%   ~     (p=1.000 n=3+3)
ParseArray-8                                        9.18ms ± 4%    8.90ms ± 1%   ~     (p=0.700 n=3+3)
pkg:github.com/onflow/cadence/runtime/sema goos:darwin goarch:amd64
QualifiedIdentifierCreation/One_level-8             1.95ns ± 2%    1.94ns ± 2%   ~     (p=0.400 n=3+3)
QualifiedIdentifierCreation/Three_levels-8          88.1ns ± 1%    87.2ns ± 1%   ~     (p=0.400 n=3+3)
pkg:github.com/onflow/cadence/runtime/tests/checker goos:darwin goarch:amd64
ContractInterfaceFungibleToken-8                    31.2µs ± 2%    30.8µs ± 2%   ~     (p=0.400 n=3+3)
CheckContractInterfaceFungibleTokenConformance-8     101µs ± 1%     100µs ± 1%   ~     (p=0.400 n=3+3)
pkg:github.com/onflow/cadence/runtime/tests/interpreter goos:darwin goarch:amd64
InterpretRecursionFib-8                             1.95ms ± 2%    2.05ms ± 2%   ~     (p=0.100 n=3+3)
NewInterpreter/new_interpreter-8                     833ns ± 1%     883ns ± 3%   ~     (p=0.100 n=3+3)
NewInterpreter/new_sub-interpreter-8                1.67µs ± 2%    1.72µs ± 1%   ~     (p=0.100 n=3+3)

name                                              old alloc/op   new alloc/op   delta
pkg:github.com/onflow/cadence/runtime goos:darwin goarch:amd64
RuntimeFungibleTokenTransfer-8                       273kB ± 0%     273kB ± 0%   ~     (p=0.100 n=3+3)
RuntimeResourceDictionaryValues-8                   4.05MB ± 0%    4.05MB ± 0%   ~     (p=1.000 n=3+3)
pkg:github.com/onflow/cadence/runtime/sema goos:darwin goarch:amd64
QualifiedIdentifierCreation/One_level-8              0.00B          0.00B        ~     (all equal)
QualifiedIdentifierCreation/Three_levels-8           64.0B ± 0%     64.0B ± 0%   ~     (all equal)
pkg:github.com/onflow/cadence/runtime/tests/checker goos:darwin goarch:amd64
ContractInterfaceFungibleToken-8                    26.6kB ± 0%    26.6kB ± 0%   ~     (all equal)
CheckContractInterfaceFungibleTokenConformance-8    66.2kB ± 0%    66.2kB ± 0%   ~     (p=1.000 n=3+3)
pkg:github.com/onflow/cadence/runtime/tests/interpreter goos:darwin goarch:amd64
InterpretRecursionFib-8                             1.24MB ± 0%    1.24MB ± 0%   ~     (p=0.800 n=3+3)
NewInterpreter/new_interpreter-8                      768B ± 0%      848B ± 0%   ~     (p=0.100 n=3+3)
NewInterpreter/new_sub-interpreter-8                1.24kB ± 0%    1.32kB ± 0%   ~     (p=0.100 n=3+3)

name                                              old allocs/op  new allocs/op  delta
pkg:github.com/onflow/cadence/runtime goos:darwin goarch:amd64
RuntimeFungibleTokenTransfer-8                       4.53k ± 0%     4.53k ± 0%   ~     (p=0.100 n=3+3)
RuntimeResourceDictionaryValues-8                     102k ± 0%      102k ± 0%   ~     (p=0.400 n=3+3)
pkg:github.com/onflow/cadence/runtime/sema goos:darwin goarch:amd64
QualifiedIdentifierCreation/One_level-8               0.00           0.00        ~     (all equal)
QualifiedIdentifierCreation/Three_levels-8            2.00 ± 0%      2.00 ± 0%   ~     (all equal)
pkg:github.com/onflow/cadence/runtime/tests/checker goos:darwin goarch:amd64
ContractInterfaceFungibleToken-8                       458 ± 0%       458 ± 0%   ~     (all equal)
CheckContractInterfaceFungibleTokenConformance-8     1.07k ± 0%     1.07k ± 0%   ~     (all equal)
pkg:github.com/onflow/cadence/runtime/tests/interpreter goos:darwin goarch:amd64
InterpretRecursionFib-8                              25.0k ± 0%     25.0k ± 0%   ~     (all equal)
NewInterpreter/new_interpreter-8                      12.0 ± 0%      13.0 ± 0%   ~     (p=0.100 n=3+3)
NewInterpreter/new_sub-interpreter-8                  38.0 ± 0%      39.0 ± 0%   ~     (p=0.100 n=3+3)

turbolent

Great work! 👏

SupunS added 15 commits February 18, 2022 15:25

Add a runtime check for resource duplication

cf456da

Add tests for resource duplication

d28798e

Panic with error

485cc50

Refactore code

02bbc78

Fix runtime resource invalidation in destroy expression

e3cd207

Refactor: move resource tracking to interpreter

b2a6bbb

Merge branch 'supun/resource-tracking' of ../cadence into supun/runti…

64b5985

…me-resource-tracking

Add test-case for force move

e672843

Use visitor-method for variable getter

5828c6e

Add tests for usages after invalidation

2202f10

Avoid using locationRangeGetter

54cf697

Refactor code

d5ca6b6

Refactor identifier expression getter-setter

5b455ed

Resolve conflicts

24a37fd

Hide external resource tracking behind a flag

1fa5207

SupunS added the Improvement label Mar 1, 2022

SupunS requested review from turbolent and dsainati1 as code owners March 1, 2022 01:25

SupunS self-assigned this Mar 1, 2022

turbolent approved these changes Mar 1, 2022

View reviewed changes

SupunS commented Mar 1, 2022

View reviewed changes

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

Fix lint errors

fcb87ba

Refactor to handle invalidated optional resources

d892eba

turbolent self-requested a review March 1, 2022 17:39

turbolent approved these changes Mar 1, 2022

View reviewed changes

SupunS merged commit fd7cc41 into master Mar 1, 2022

SupunS deleted the supun/runtime-resource-tracking branch March 1, 2022 21:17

turbolent mentioned this pull request May 5, 2022

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

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check use of invalidated resources at runtime #1464

Check use of invalidated resources at runtime #1464

SupunS commented Mar 1, 2022

turbolent left a comment

codecov-commenter commented Mar 1, 2022 •

edited

Loading

github-actions bot commented Mar 1, 2022 •

edited

Loading

turbolent commented Mar 1, 2022

SupunS commented Mar 1, 2022

turbolent left a comment

Check use of invalidated resources at runtime #1464

Check use of invalidated resources at runtime #1464

Conversation

SupunS commented Mar 1, 2022

Description

turbolent left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 1, 2022 • edited Loading

Codecov Report

github-actions bot commented Mar 1, 2022 • edited Loading

Cadence Benchstat comparison

Results

turbolent commented Mar 1, 2022

SupunS commented Mar 1, 2022

turbolent left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 1, 2022 •

edited

Loading

github-actions bot commented Mar 1, 2022 •

edited

Loading