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

[FVM] Remove old quick fixes #3219

Merged
merged 1 commit into from Sep 13, 2022
Merged

[FVM] Remove old quick fixes #3219

merged 1 commit into from Sep 13, 2022

Conversation

janezpodhostnik
Copy link
Contributor

Both of these were already properly fixed either in the FVM or in cadence.

The reason I am removing them now is because the fuzz tests test exactly what these quick fixes were for: a scenario where the node crashes but shouldn't, because of reaching the limits at a bad location in the code.

I ran the fuzzer for an entire day after removing this.

@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2022

Codecov Report

Merging #3219 (0212dd5) into master (248a79d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3219      +/-   ##
==========================================
+ Coverage   53.89%   53.91%   +0.01%     
==========================================
  Files         743      743              
  Lines       68407    68391      -16     
==========================================
+ Hits        36869    36870       +1     
+ Misses      28487    28472      -15     
+ Partials     3051     3049       -2     
Flag Coverage Δ
unittests 53.91% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
fvm/transaction.go 89.79% <ø> (+11.82%) ⬆️
fvm/transactionInvoker.go 68.82% <100.00%> (-1.07%) ⬇️
...e/orchestrator/mock_corruptible_conduit_factory.go 80.59% <0.00%> (-2.99%) ⬇️
engine/access/ingestion/engine.go 59.55% <0.00%> (-0.19%) ⬇️
fvm/handler/contract.go 72.68% <0.00%> (ø)
consensus/hotstuff/eventloop/event_loop.go 74.82% <0.00%> (ø)
network/p2p/dns/resolver.go 96.36% <0.00%> (+4.84%) ⬆️
insecure/orchestrator/corrupted_connection.go 82.35% <0.00%> (+7.84%) ⬆️

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

FVM Benchstat comparison

This branch with compared with the base branch onflow:master commit 248a79d

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/get_signer_vault-236.6ms ± 5%37.6ms ± 5%+2.64%(p=0.028 n=10+9)
RuntimeTransaction/get_signer_address-228.9ms ± 5%29.7ms ± 4%+2.61%(p=0.023 n=10+10)
RuntimeTransaction/convert_int_to_string-230.8ms ±13%31.0ms ±14%~(p=0.853 n=10+10)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-231.9ms ± 7%31.7ms ±10%~(p=0.447 n=9+10)
RuntimeTransaction/get_public_account-232.6ms ± 3%32.8ms ± 6%~(p=0.971 n=10+10)
RuntimeTransaction/get_account_and_get_balance-2313ms ± 3%312ms ± 4%~(p=0.931 n=9+9)
RuntimeTransaction/get_account_and_get_available_balance-2291ms ± 4%295ms ± 4%~(p=0.053 n=10+9)
RuntimeTransaction/get_account_and_get_storage_used-234.2ms ± 8%34.1ms ± 5%~(p=0.720 n=10+9)
RuntimeTransaction/get_account_and_get_storage_capacity-2254ms ± 1%258ms ± 3%~(p=0.353 n=10+10)
RuntimeTransaction/get_signer_receiver-247.0ms ± 6%46.8ms ± 6%~(p=0.684 n=10+10)
RuntimeTransaction/transfer_tokens-2213ms ± 2%215ms ± 5%~(p=0.905 n=9+10)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-237.2ms ± 6%36.0ms ± 7%~(p=0.095 n=10+9)
RuntimeTransaction/load_and_save_long_string_on_signers_address-280.7ms ± 5%79.4ms ± 5%~(p=0.218 n=10+10)
RuntimeTransaction/create_new_account-2848ms ± 1%854ms ± 2%~(p=0.408 n=8+10)
RuntimeTransaction/call_empty_contract_function-232.4ms ± 5%31.7ms ± 3%~(p=0.203 n=10+8)
RuntimeTransaction/emit_event-247.4ms ± 5%47.0ms ± 3%~(p=0.604 n=9+10)
RuntimeTransaction/borrow_array_from_storage-2134ms ± 4%134ms ± 4%~(p=0.436 n=10+10)
RuntimeTransaction/copy_array_from_storage-2140ms ± 6%140ms ± 3%~(p=0.912 n=10+10)
RuntimeNFTBatchTransfer-2114ms ± 3%114ms ± 2%~(p=0.666 n=9+9)
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/16/cols/128/txes-24.99s ± 2%4.94s ± 3%~(p=0.089 n=10+10)
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/reference_tx-229.3ms ±11%27.1ms ±11%−7.54%(p=0.009 n=10+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.4k ± 0%44.4k ± 0%~(all equal)
RuntimeTransaction/convert_int_to_string-244.4k ± 0%44.4k ± 0%~(all equal)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-244.4k ± 0%44.4k ± 0%~(all equal)
RuntimeTransaction/get_signer_address-244.4k ± 0%44.4k ± 0%~(all equal)
RuntimeTransaction/get_public_account-244.4k ± 0%44.4k ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_balance-216.8M ± 0%16.8M ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_available_balance-25.28M ± 0%5.28M ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_storage_used-248.0k ± 0%48.0k ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_storage_capacity-25.27M ± 0%5.27M ± 0%~(all equal)
RuntimeTransaction/get_signer_vault-244.7k ± 0%44.7k ± 0%~(all equal)
RuntimeTransaction/get_signer_receiver-245.0k ± 0%45.0k ± 0%~(all equal)
RuntimeTransaction/transfer_tokens-245.8k ± 0%45.8k ± 0%~(all equal)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-244.8k ± 0%44.8k ± 0%~(all equal)
RuntimeTransaction/load_and_save_long_string_on_signers_address-249.7k ± 0%49.7k ± 0%~(all equal)
RuntimeTransaction/create_new_account-28.53M ± 0%8.53M ± 0%~(all equal)
RuntimeTransaction/call_empty_contract_function-244.6k ± 0%44.6k ± 0%~(all equal)
RuntimeTransaction/emit_event-244.6k ± 0%44.6k ± 0%~(all equal)
RuntimeTransaction/borrow_array_from_storage-249.8k ± 0%49.8k ± 0%~(all equal)
RuntimeTransaction/copy_array_from_storage-249.8k ± 0%49.8k ± 0%~(all equal)
 
alloc/opdelta
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/get_signer_address-236.9MB ± 5%38.8MB ± 8%+5.19%(p=0.003 n=10+10)
RuntimeTransaction/reference_tx-237.6MB ± 9%36.2MB ± 7%~(p=0.063 n=10+10)
RuntimeTransaction/convert_int_to_string-237.6MB ± 6%37.5MB ± 5%~(p=0.912 n=10+10)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-237.5MB ± 8%37.3MB ± 9%~(p=0.579 n=10+10)
RuntimeTransaction/get_public_account-238.9MB ± 4%38.7MB ± 4%~(p=0.631 n=10+10)
RuntimeTransaction/get_account_and_get_balance-2141MB ± 2%141MB ± 2%~(p=0.853 n=10+10)
RuntimeTransaction/get_account_and_get_available_balance-2123MB ± 3%124MB ± 3%~(p=0.481 n=10+10)
RuntimeTransaction/get_account_and_get_storage_used-238.5MB ± 5%38.3MB ± 2%~(p=0.364 n=10+7)
RuntimeTransaction/get_account_and_get_storage_capacity-2117MB ± 2%118MB ± 2%~(p=0.353 n=10+10)
RuntimeTransaction/get_signer_vault-238.6MB ± 6%39.2MB ± 6%~(p=0.481 n=10+10)
RuntimeTransaction/get_signer_receiver-242.0MB ± 7%41.6MB ± 5%~(p=0.684 n=10+10)
RuntimeTransaction/transfer_tokens-291.0MB ± 3%90.6MB ± 2%~(p=0.796 n=10+10)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-239.1MB ± 5%38.6MB ±10%~(p=0.436 n=10+10)
RuntimeTransaction/load_and_save_long_string_on_signers_address-257.9MB ± 4%57.2MB ± 4%~(p=0.353 n=10+10)
RuntimeTransaction/create_new_account-2209MB ± 2%209MB ± 0%~(p=0.958 n=10+6)
RuntimeTransaction/call_empty_contract_function-238.5MB ± 7%38.0MB ± 4%~(p=0.393 n=10+10)
RuntimeTransaction/emit_event-243.3MB ± 5%43.1MB ± 4%~(p=0.842 n=9+10)
RuntimeTransaction/borrow_array_from_storage-268.9MB ± 6%68.8MB ± 6%~(p=0.579 n=10+10)
RuntimeTransaction/copy_array_from_storage-283.8MB ± 3%84.7MB ± 2%~(p=0.356 n=9+10)
RuntimeNFTBatchTransfer-257.3MB ± 2%57.6MB ± 4%~(p=0.661 n=9+10)
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/16/cols/128/txes-21.32GB ± 1%1.33GB ± 1%~(p=0.247 n=10+10)
 
allocs/opdelta
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/get_account_and_get_storage_used-2139k ± 0%139k ± 0%+0.01%(p=0.003 n=10+10)
RuntimeTransaction/get_account_and_get_balance-21.71M ± 0%1.71M ± 0%+0.00%(p=0.031 n=9+9)
RuntimeTransaction/reference_tx-289.5k ± 0%89.5k ± 0%~(p=0.060 n=10+10)
RuntimeTransaction/convert_int_to_string-2104k ± 0%104k ± 0%~(p=0.400 n=10+10)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-2118k ± 0%118k ± 0%~(p=0.493 n=10+10)
RuntimeTransaction/get_signer_address-294.7k ± 0%94.7k ± 0%~(p=0.059 n=10+10)
RuntimeTransaction/get_public_account-2118k ± 0%118k ± 0%~(p=0.565 n=10+10)
RuntimeTransaction/get_account_and_get_available_balance-21.58M ± 0%1.58M ± 0%~(p=0.342 n=10+10)
RuntimeTransaction/get_account_and_get_storage_capacity-21.42M ± 0%1.42M ± 0%~(p=0.278 n=10+9)
RuntimeTransaction/get_signer_vault-2142k ± 0%142k ± 0%~(p=0.239 n=10+10)
RuntimeTransaction/get_signer_receiver-2222k ± 0%222k ± 0%~(p=0.516 n=10+10)
RuntimeTransaction/transfer_tokens-2971k ± 0%971k ± 0%~(p=0.225 n=10+10)
RuntimeTransaction/load_and_save_long_string_on_signers_address-2242k ± 0%242k ± 0%~(p=0.566 n=10+10)
RuntimeTransaction/create_new_account-22.80M ± 0%2.80M ± 0%~(p=0.811 n=10+10)
RuntimeTransaction/call_empty_contract_function-2106k ± 0%106k ± 0%~(p=0.368 n=8+10)
RuntimeTransaction/emit_event-2151k ± 0%151k ± 0%~(p=0.184 n=10+10)
RuntimeTransaction/borrow_array_from_storage-2379k ± 0%379k ± 0%~(p=0.345 n=10+9)
RuntimeTransaction/copy_array_from_storage-2335k ± 0%335k ± 0%~(p=0.754 n=10+10)
RuntimeNFTBatchTransfer-2304k ± 0%304k ± 0%~(p=0.459 n=9+10)
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/16/cols/128/txes-221.0M ± 0%21.0M ± 0%~(p=0.165 n=10+10)
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/load_and_save_empty_string_on_signers_address-2140k ± 0%140k ± 0%−0.01%(p=0.037 n=10+9)
 
us/txdelta
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/16/cols/128/txes-22.44k ± 2%2.41k ± 3%~(p=0.086 n=10+10)
 

@janezpodhostnik
Copy link
Contributor Author

bors merge

@bors bors bot merged commit c58f31f into master Sep 13, 2022
@bors bors bot deleted the janez/remove-old-qick-fixes branch September 13, 2022 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Execution Cadence Execution Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants