Skip to content

Ensure cadence don't access unexpected operations while parsing (part 3)#3359

Merged
bors[bot] merged 1 commit intomasterfrom
patrick/parse-restrict-3
Oct 14, 2022
Merged

Ensure cadence don't access unexpected operations while parsing (part 3)#3359
bors[bot] merged 1 commit intomasterfrom
patrick/parse-restrict-3

Conversation

@pattyshack
Copy link
Contributor

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2022

Codecov Report

Merging #3359 (6e6782f) into master (5a6646f) will increase coverage by 0.02%.
The diff coverage is 54.91%.

@@            Coverage Diff             @@
##           master    #3359      +/-   ##
==========================================
+ Coverage   55.00%   55.02%   +0.02%     
==========================================
  Files         734      694      -40     
  Lines       67341    65398    -1943     
==========================================
- Hits        37040    35985    -1055     
+ Misses      27264    26412     -852     
+ Partials     3037     3001      -36     
Flag Coverage Δ
unittests 55.02% <54.91%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
admin/commands/common/get_identity.go 0.00% <0.00%> (ø)
cmd/execution_builder.go 0.00% <0.00%> (ø)
cmd/util/ledger/reporters/account_reporter.go 0.00% <0.00%> (ø)
cmd/verification_builder.go 0.00% <0.00%> (ø)
engine/access/ping/engine.go 0.00% <ø> (ø)
engine/access/rest/router.go 100.00% <ø> (ø)
engine/collection/compliance/core.go 77.72% <0.00%> (-1.18%) ⬇️
engine/common/follower/engine.go 51.29% <0.00%> (-0.51%) ⬇️
engine/common/synchronization/engine.go 73.30% <ø> (ø)
engine/consensus/compliance/core.go 74.30% <0.00%> (-0.90%) ⬇️
... and 162 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
Contributor

github-actions bot commented Oct 7, 2022

FVM Benchstat comparison

This branch with compared with the base branch onflow:master commit 4cd3c83

The command (for i in {1..10}; 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
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/reference_tx-223.1ms ± 5%23.3ms ± 8%~(p=0.631 n=10+10)
RuntimeTransaction/convert_int_to_string-226.6ms ± 7%25.6ms ±13%~(p=0.156 n=9+10)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-229.3ms ±13%29.2ms ±16%~(p=0.912 n=10+10)
RuntimeTransaction/get_signer_address-223.7ms ± 5%24.5ms ± 7%~(p=0.059 n=8+9)
RuntimeTransaction/get_public_account-228.5ms ±12%28.6ms ± 8%~(p=0.739 n=10+10)
RuntimeTransaction/get_account_and_get_balance-2256ms ± 5%256ms ± 4%~(p=0.489 n=9+9)
RuntimeTransaction/get_account_and_get_available_balance-2236ms ± 2%236ms ± 2%~(p=0.897 n=8+10)
RuntimeTransaction/get_account_and_get_storage_used-230.8ms ±10%30.6ms ±10%~(p=0.684 n=10+10)
RuntimeTransaction/get_account_and_get_storage_capacity-2208ms ± 5%209ms ± 4%~(p=0.436 n=10+10)
RuntimeTransaction/get_signer_vault-233.1ms ± 4%33.2ms ± 5%~(p=0.971 n=10+10)
RuntimeTransaction/get_signer_receiver-243.0ms ± 5%42.5ms ± 4%~(p=0.497 n=10+9)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-231.7ms ± 6%31.7ms ± 8%~(p=0.853 n=10+10)
RuntimeTransaction/load_and_save_long_string_on_signers_address-270.1ms ± 1%70.2ms ± 5%~(p=0.696 n=8+10)
RuntimeTransaction/create_new_account-2749ms ± 1%753ms ± 2%~(p=0.497 n=9+10)
RuntimeTransaction/call_empty_contract_function-226.6ms ± 5%26.8ms ± 6%~(p=0.796 n=10+10)
RuntimeTransaction/emit_event-239.5ms ± 4%39.4ms ± 6%~(p=0.853 n=10+10)
RuntimeTransaction/borrow_array_from_storage-2120ms ± 3%117ms ± 4%~(p=0.053 n=9+10)
RuntimeTransaction/copy_array_from_storage-2123ms ± 5%122ms ± 6%~(p=0.631 n=10+10)
RuntimeNFTBatchTransfer-2104ms ± 6%104ms ± 3%~(p=0.400 n=10+9)
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/16/cols/128/txes-24.40s ± 7%4.44s ± 5%~(p=0.579 n=10+10)
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/transfer_tokens-2184ms ± 3%180ms ± 3%−2.36%(p=0.006 n=9+10)
 
computationdelta
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/reference_tx-2202 ± 0%202 ± 0%~(all equal)
RuntimeTransaction/convert_int_to_string-2402 ± 0%402 ± 0%~(all equal)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-2502 ± 0%502 ± 0%~(all equal)
RuntimeTransaction/get_signer_address-2302 ± 0%302 ± 0%~(all equal)
RuntimeTransaction/get_public_account-2402 ± 0%402 ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_balance-21.00k ± 0%1.00k ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_available_balance-22.60k ± 0%2.60k ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_storage_used-2402 ± 0%402 ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_storage_capacity-21.30k ± 0%1.30k ± 0%~(all equal)
RuntimeTransaction/get_signer_vault-2402 ± 0%402 ± 0%~(all equal)
RuntimeTransaction/get_signer_receiver-2602 ± 0%602 ± 0%~(all equal)
RuntimeTransaction/transfer_tokens-23.50k ± 0%3.50k ± 0%~(all equal)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-2602 ± 0%602 ± 0%~(all equal)
RuntimeTransaction/load_and_save_long_string_on_signers_address-2602 ± 0%602 ± 0%~(all equal)
RuntimeTransaction/create_new_account-2202 ± 0%202 ± 0%~(all equal)
RuntimeTransaction/call_empty_contract_function-2402 ± 0%402 ± 0%~(all equal)
RuntimeTransaction/emit_event-2602 ± 0%602 ± 0%~(all equal)
RuntimeTransaction/borrow_array_from_storage-22.60k ± 0%2.60k ± 0%~(all equal)
RuntimeTransaction/copy_array_from_storage-22.60k ± 0%2.60k ± 0%~(all equal)
 
interactionsdelta
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/reference_tx-244.3k ± 0%44.3k ± 0%~(all equal)
RuntimeTransaction/convert_int_to_string-244.3k ± 0%44.3k ± 0%~(all equal)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-244.3k ± 0%44.3k ± 0%~(all equal)
RuntimeTransaction/get_signer_address-244.3k ± 0%44.3k ± 0%~(all equal)
RuntimeTransaction/get_public_account-244.3k ± 0%44.3k ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_balance-216.7M ± 0%16.7M ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_available_balance-25.13M ± 0%5.13M ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_storage_used-244.3k ± 0%44.3k ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_storage_capacity-25.13M ± 0%5.13M ± 0%~(all equal)
RuntimeTransaction/get_signer_vault-244.5k ± 0%44.5k ± 0%~(all equal)
RuntimeTransaction/get_signer_receiver-244.9k ± 0%44.9k ± 0%~(all equal)
RuntimeTransaction/transfer_tokens-245.2k ± 0%45.2k ± 0%~(all equal)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-244.4k ± 0%44.4k ± 0%~(all equal)
RuntimeTransaction/load_and_save_long_string_on_signers_address-247.2k ± 0%47.2k ± 0%~(all equal)
RuntimeTransaction/create_new_account-28.39M ± 0%8.39M ± 0%~(all equal)
RuntimeTransaction/call_empty_contract_function-244.5k ± 0%44.5k ± 0%~(all equal)
RuntimeTransaction/emit_event-244.5k ± 0%44.5k ± 0%~(all equal)
RuntimeTransaction/borrow_array_from_storage-249.7k ± 0%49.7k ± 0%~(all equal)
RuntimeTransaction/copy_array_from_storage-249.7k ± 0%49.7k ± 0%~(all equal)
 
alloc/opdelta
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/reference_tx-235.1MB ± 3%35.1MB ± 7%~(p=0.853 n=10+10)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-238.2MB ± 6%37.2MB ± 4%~(p=0.529 n=10+10)
RuntimeTransaction/get_signer_address-235.0MB ± 5%35.3MB ± 7%~(p=0.739 n=10+10)
RuntimeTransaction/get_public_account-238.4MB ± 3%38.6MB ± 6%~(p=0.661 n=9+10)
RuntimeTransaction/get_account_and_get_balance-2127MB ± 3%127MB ± 3%~(p=0.631 n=10+10)
RuntimeTransaction/get_account_and_get_available_balance-2108MB ± 3%108MB ± 4%~(p=0.739 n=10+10)
RuntimeTransaction/get_account_and_get_storage_used-239.3MB ± 5%38.5MB ± 9%~(p=0.182 n=9+10)
RuntimeTransaction/get_account_and_get_storage_capacity-2105MB ± 4%104MB ± 4%~(p=0.353 n=10+10)
RuntimeTransaction/get_signer_vault-239.4MB ± 5%39.7MB ± 6%~(p=0.579 n=10+10)
RuntimeTransaction/get_signer_receiver-242.8MB ± 5%42.5MB ± 6%~(p=0.489 n=9+9)
RuntimeTransaction/transfer_tokens-284.9MB ± 3%85.7MB ± 2%~(p=0.436 n=10+10)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-238.0MB ± 6%38.1MB ± 6%~(p=1.000 n=10+10)
RuntimeTransaction/load_and_save_long_string_on_signers_address-254.5MB ± 2%55.4MB ± 4%~(p=0.274 n=8+10)
RuntimeTransaction/create_new_account-2198MB ± 2%195MB ± 1%~(p=0.123 n=10+10)
RuntimeTransaction/call_empty_contract_function-236.5MB ± 4%36.5MB ± 8%~(p=0.912 n=10+10)
RuntimeTransaction/emit_event-240.1MB ± 6%39.6MB ± 5%~(p=0.853 n=10+10)
RuntimeTransaction/borrow_array_from_storage-269.6MB ± 5%67.6MB ± 4%~(p=0.089 n=10+10)
RuntimeTransaction/copy_array_from_storage-281.9MB ± 2%81.4MB ± 3%~(p=0.631 n=10+10)
RuntimeNFTBatchTransfer-256.9MB ± 4%57.1MB ± 2%~(p=0.968 n=10+9)
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/16/cols/128/txes-21.28GB ± 1%1.28GB ± 1%~(p=0.739 n=10+10)
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/convert_int_to_string-237.7MB ± 6%35.5MB ± 3%−5.65%(p=0.002 n=10+8)
 
allocs/opdelta
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/reference_tx-275.5k ± 0%75.6k ± 0%+0.20%(p=0.000 n=10+10)
RuntimeTransaction/get_signer_address-279.7k ± 0%79.8k ± 0%+0.20%(p=0.000 n=10+10)
RuntimeNFTBatchTransfer-2281k ± 1%281k ± 0%+0.18%(p=0.026 n=10+9)
RuntimeTransaction/call_empty_contract_function-290.4k ± 0%90.6k ± 0%+0.17%(p=0.000 n=10+10)
RuntimeTransaction/convert_int_to_string-287.7k ± 0%87.8k ± 0%+0.16%(p=0.000 n=10+8)
RuntimeTransaction/get_public_account-2101k ± 0%101k ± 0%+0.16%(p=0.000 n=9+10)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-298.9k ± 0%99.1k ± 0%+0.15%(p=0.000 n=10+9)
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/16/cols/128/txes-219.8M ± 0%19.8M ± 0%+0.15%(p=0.000 n=10+9)
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/get_signer_vault-2118k ± 0%119k ± 0%+0.14%(p=0.000 n=10+9)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-2119k ± 0%119k ± 0%+0.13%(p=0.000 n=10+10)
RuntimeTransaction/get_account_and_get_storage_used-2121k ± 0%121k ± 0%+0.12%(p=0.000 n=10+10)
RuntimeTransaction/emit_event-2129k ± 0%129k ± 0%+0.11%(p=0.000 n=10+10)
RuntimeTransaction/get_signer_receiver-2190k ± 0%190k ± 0%+0.08%(p=0.000 n=10+6)
RuntimeTransaction/load_and_save_long_string_on_signers_address-2221k ± 0%221k ± 0%+0.07%(p=0.000 n=10+10)
RuntimeTransaction/copy_array_from_storage-2289k ± 0%289k ± 0%+0.05%(p=0.000 n=10+10)
RuntimeTransaction/borrow_array_from_storage-2333k ± 0%333k ± 0%+0.04%(p=0.000 n=10+8)
RuntimeTransaction/transfer_tokens-2848k ± 0%848k ± 0%+0.02%(p=0.000 n=10+10)
RuntimeTransaction/get_account_and_get_storage_capacity-21.21M ± 0%1.21M ± 0%+0.01%(p=0.000 n=10+10)
RuntimeTransaction/get_account_and_get_available_balance-21.34M ± 0%1.34M ± 0%+0.01%(p=0.000 n=10+10)
RuntimeTransaction/get_account_and_get_balance-21.50M ± 0%1.50M ± 0%+0.01%(p=0.000 n=10+10)
RuntimeTransaction/create_new_account-22.55M ± 0%2.55M ± 0%~(p=0.095 n=10+9)
 
us/txdelta
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/16/cols/128/txes-22.15k ± 7%2.17k ± 5%~(p=0.579 n=10+10)
 

@pattyshack pattyshack force-pushed the patrick/parse-restrict-3 branch from 2f77bd3 to 0deefba Compare October 12, 2022 19:14
}

return callback(arg)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

iirc golang does not have plan to support generic variadic functions. i'm thinking combining https://pkg.go.dev/cmd/go#hdr-Generate_Go_files_by_processing_source and "text/template" may save us some typing here. but just for entertainment purposes, it is too hacky for production.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not supporting adhoc (i.e., overloading / variadic) polymorphism is the correct thing to do from the language designer's perspective. unlike parametric polymorphism, there's no solid theoretical foundation for adhoc polymorphism.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is why i hate C++ ;)

@pattyshack pattyshack force-pushed the patrick/parse-restrict-3 branch from 0deefba to 6e6782f Compare October 13, 2022 17:19
@pattyshack
Copy link
Contributor Author

bors merge

@bors bors bot merged commit 2440283 into master Oct 14, 2022
@bors bors bot deleted the patrick/parse-restrict-3 branch October 14, 2022 17:42
Copy link
Contributor

@janezpodhostnik janezpodhostnik left a comment

Choose a reason for hiding this comment

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

Hmmm... It might be time to unify opCode, SpanName and common.ComputationKind and use a operation wrapper like the parseRestricted to not only check for IsParseRestricted but also do:

defer info.tracer.StartSpanFromRoot(opCode).End()
err := info.meter.MeterComputation(opCode, intensity())

@pattyshack
Copy link
Contributor Author

yeah, that's reasonable.

@Tonix517
Copy link
Contributor

+1

Hmmm... It might be time to unify opCode, SpanName and common.ComputationKind and use a operation wrapper like the parseRestricted to not only check for IsParseRestricted but also do:

defer info.tracer.StartSpanFromRoot(opCode).End()
err := info.meter.MeterComputation(opCode, intensity())

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.

5 participants