[move] Type builder & fix calculation of node count in types #13028

georgemitenkov · 2024-04-25T10:16:06Z

Description

Previously the type size was calculated prior to performing the substitution. The calculation accounted for all nodes in non-substituted types and was not well-encapsulated. This PR adds node counting to subst function (in addition to depth) to make sure it is correctly accounted for as well.

No performance regression is expected because the counter is used in existing type traversals. An alternative design to store count and depth with types have been considered, but is significantly more verbose and it is not clear if it is needed before new type representation is designed.

The second change introduced by this PR is a new TypeBuilder struct which should be the only instance which can construct types in a checked fashion (respecting maximum depth and count). All type creation methods have been moved there, including:

constant creation
creation from type tags
creation via substituition
creation of vectors and references
creation of struct instantiations

Lastly, paranoid mode is made nicer (syntactically), making type explicit and ensuring their creation is gated.

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Replays:

legacy: https://github.com/aptos-labs/aptos-core/actions/runs/9463782826 (PASSED)
limits 256, 128: https://github.com/aptos-labs/aptos-core/actions/runs/9463791438 (PASSED)
limits 256, 20: https://github.com/aptos-labs/aptos-core/actions/runs/9463801219 (PASSED)
limits 128, 128: https://github.com/aptos-labs/aptos-core/actions/runs/9463812643 (PASSED)

Also, many unit tests gave been added for type creation and substitution.

Key Areas to Review

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2024-04-25T10:16:09Z

⏱️ 87h 23m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
replay-mainnet / replay-verify (16)	5h 48m	🟩
replay-mainnet / replay-verify (15)	5h 21m	🟩
replay-mainnet / replay-verify (17)	4h 47m	🟩
replay-testnet / replay-verify (16)	4h 27m	🟩
rust-smoke-tests	3h 56m	⬜ 🟩 🟩 🟩 🟥 (+4 more)
replay-mainnet / replay-verify (12)	3h 55m	🟩
replay-mainnet / replay-verify (11)	3h 15m	🟩
execution-performance / single-node-performance	2h 58m	🟩 🟥 🟩 🟩 🟩 (+4 more)
replay-testnet / replay-verify (9)	2h 53m	🟩
rust-targeted-unit-tests	2h 21m	🟥 ⬜ 🟥 🟥 🟥 (+3 more)
replay-mainnet / replay-verify (10)	2h 17m	🟩
rust-move-unit-coverage	2h 15m	🟩 ⬜ 🟩 🟩 🟩 (+3 more)
replay-testnet / replay-verify (10)	2h 10m	🟩
replay-testnet / replay-verify (8)	1h 58m	🟩
replay-mainnet / replay-verify (14)	1h 56m	🟩
forge-e2e-test / forge	1h 45m	🟩 🟩 🟩 🟥 🟥 (+5 more)
rust-images / rust-all	1h 44m	🟩 🟩 🟩 🟩 🟩 (+5 more)
replay-testnet / replay-verify (17)	1h 42m	🟩
replay-mainnet / replay-verify (3)	1h 40m	🟩
replay-testnet / replay-verify (15)	1h 38m	🟩
forge-compat-test / forge	1h 32m	⬜ 🟩 🟩 🟩 🟥 (+3 more)
replay-testnet / replay-verify (11)	1h 31m	🟩
rust-move-tests	1h 24m	🟩 ⬜ 🟩 🟩 🟩 (+4 more)
replay-testnet / replay-verify (2)	1h 21m	🟩
replay-testnet / replay-verify (0)	1h 18m	🟩
replay-testnet / replay-verify (6)	1h 16m	🟩
replay-mainnet / replay-verify (9)	1h 16m	🟩
replay-testnet / replay-verify (12)	1h 15m	🟩
replay-mainnet / replay-verify (6)	1h 13m	🟩
replay-mainnet / replay-verify (13)	1h 6m	🟩
replay-testnet / replay-verify (13)	1h 1m	🟩
replay-mainnet / replay-verify (8)	1h 1m	🟩
replay-mainnet / replay-verify (7)	1h 1m	🟩
replay-testnet / replay-verify (1)	59m	🟩
replay-testnet / replay-verify (4)	58m	🟩
replay-testnet / replay-verify (14)	56m	🟩
replay-testnet / replay-verify (5)	53m	🟩
replay-testnet / replay-verify (7)	47m	🟩
replay-mainnet / replay-verify (4)	47m	🟩
replay-testnet / replay-verify (3)	44m	🟩
rust-lints	43m	🟩 🟩 🟩 🟩 🟩 (+3 more)
replay-mainnet / replay-verify (5)	43m	🟩
run-tests-main-branch	42m	🟩 🟩 🟩 🟩 🟩 (+5 more)
replay-mainnet / replay-verify (2)	36m	🟩
cli-e2e-tests / run-cli-tests	35m	🟩 ⬜ 🟩 🟩 🟩 (+2 more)
rust-build-cached-packages	35m	🟩 🟩 🟩 🟩 🟩 (+3 more)
replay-mainnet / replay-verify (0)	35m	🟩
check	32m	🟩 🟩 🟩 🟩 🟩 (+3 more)
replay-mainnet / replay-verify (1)	19m	🟩
general-lints	14m	🟩 🟩 🟩 🟩 🟩 (+3 more)
check-dynamic-deps	11m	🟩 🟩 🟩 🟩 🟩 (+4 more)
indexer-grpc-e2e-tests / test-indexer-grpc-docker-compose	11m	🟥 ⬜ 🟩 🟥 🟩 (+2 more)
node-api-compatibility-tests / node-api-compatibility-tests	6m	🟩 ⬜ 🟩 🟩 🟩 (+2 more)
framework-upgrade-determinator	5m	🟥 🟥
semgrep/ci	4m	🟩 🟩 🟩 🟩 🟩 (+4 more)
file_change_determinator	2m	🟩 🟩 🟩 🟩 🟩 (+5 more)
file_change_determinator	2m	🟩 🟩 🟩 🟩 🟩 (+5 more)
execution-performance / file_change_determinator	2m	🟩 🟩 🟩 🟩 🟩 (+4 more)
file_change_determinator	2m	🟩 🟩 🟩 🟩 🟩 (+3 more)
permission-check	32s	🟩 🟩 🟩 🟩 🟩 (+6 more)
permission-check	32s	🟩 🟩 🟩 🟩 🟩 (+6 more)
permission-check	31s	🟩 🟩 🟩 🟩 🟩 (+5 more)
permission-check	31s	🟩 🟩 🟩 🟩 🟩 (+6 more)
determine-docker-build-metadata	24s	🟩 🟩 🟩 🟩 🟩 (+5 more)
permission-check	23s	🟩 🟩 🟩 🟩 🟩 (+6 more)
determine-test-metadata	7s	🟩

🚨 1 job on the last run was significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
rust-targeted-unit-tests	22m	16m

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

codecov · 2024-04-25T10:35:29Z

Codecov Report

Attention: Patch coverage is 90.11544% with 137 lines in your changes missing coverage. Please review.

Project coverage is 58.6%. Comparing base (bbce0f1) to head (1f99379).
Report is 1 commits behind head on main.

Files	Patch %	Lines
...ove/move-vm/types/src/loaded_data/runtime_types.rs	94.9%	50 Missing ⚠️
aptos-move/aptos-vm/src/move_vm_ext/vm.rs	0.0%	22 Missing ⚠️
aptos-move/aptos-vm/src/aptos_vm.rs	0.0%	19 Missing ⚠️
...ptos-vm/src/verifier/transaction_arg_validation.rs	0.0%	19 Missing ⚠️
...aptos-gas-schedule/src/gas_schedule/transaction.rs	0.0%	10 Missing ⚠️
third_party/move/move-vm/runtime/src/loader/mod.rs	92.2%	8 Missing ⚠️
third_party/move/move-vm/runtime/src/session.rs	0.0%	3 Missing ⚠️
types/src/on_chain_config/aptos_features.rs	25.0%	3 Missing ⚠️
types/src/vm/configs.rs	0.0%	2 Missing ⚠️
aptos-move/aptos-vm/src/gas.rs	0.0%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             main   #13028     +/-   ##
=========================================
+ Coverage    58.4%    58.6%   +0.1%     
=========================================
  Files         823      823             
  Lines      197664   198412    +748     
=========================================
+ Hits       115603   116369    +766     
+ Misses      82061    82043     -18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vgao1996

As we discussed offline

The core logic looks good
Both the old and new checks need feature-gating
Better make the limits configurable (can store those as part of the gas schedule, and then pass those values into the vm config)
It sounds like a good idea to incorporate your type builder work into this, providing a context that can carry the configs

runtian-zhou · 2024-04-26T19:21:48Z

Are we going to bundle this change with type builder then?

github-actions · 2024-06-12T16:08:27Z

✅ Forge suite `realistic_env_max_load` success on `1f99379161e1734836e2940192b98405516be43c`

two traffics test: inner traffic : committed: 9400.268190577093 txn/s, latency: 4183.229495239363 ms, (p50: 4200 ms, p90: 4500 ms, p99: 7500 ms), latency samples: 4056180
two traffics test : committed: 100.0003799910461 txn/s, latency: 2017.1208791208792 ms, (p50: 1900 ms, p90: 2200 ms, p99: 4900 ms), latency samples: 1820
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.233, avg: 0.213", "QsPosToProposal: max: 1.990, avg: 1.930", "ConsensusProposalToOrdered: max: 0.304, avg: 0.284", "ConsensusOrderedToCommit: max: 0.368, avg: 0.359", "ConsensusProposalToCommit: max: 0.651, avg: 0.642"]
Max round gap was 1 [limit 4] at version 1878579. Max no progress secs was 5.295248 [limit 15] at version 1878579.
Test Ok

github-actions · 2024-06-12T16:08:28Z

✅ Forge suite `compat` success on `a68e71c05caebf01504d4499110f3fba213fb53d` ==> `1f99379161e1734836e2940192b98405516be43c`

Compatibility test results for a68e71c05caebf01504d4499110f3fba213fb53d ==> 1f99379161e1734836e2940192b98405516be43c (PR)
1. Check liveness of validators at old version: a68e71c05caebf01504d4499110f3fba213fb53d
compatibility::simple-validator-upgrade::liveness-check : committed: 9851.572614374585 txn/s, latency: 3411.027865528282 ms, (p50: 2600 ms, p90: 6600 ms, p99: 21400 ms), latency samples: 374800
2. Upgrading first Validator to new version: 1f99379161e1734836e2940192b98405516be43c
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6054.119942842393 txn/s, latency: 5002.675812156754 ms, (p50: 4900 ms, p90: 7300 ms, p99: 8400 ms), latency samples: 214860
3. Upgrading rest of first batch to new version: 1f99379161e1734836e2940192b98405516be43c
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7007.379690657908 txn/s, latency: 4291.523570204781 ms, (p50: 4200 ms, p90: 6000 ms, p99: 6500 ms), latency samples: 245140
4. upgrading second batch to new version: 1f99379161e1734836e2940192b98405516be43c
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 10059.237951597886 txn/s, latency: 3039.5250381213787 ms, (p50: 2700 ms, p90: 5100 ms, p99: 6000 ms), latency samples: 327900
5. check swarm health
Compatibility test for a68e71c05caebf01504d4499110f3fba213fb53d ==> 1f99379161e1734836e2940192b98405516be43c passed
Test Ok

github-actions · 2024-06-12T16:10:53Z

✅ Forge suite `framework_upgrade` success on `a68e71c05caebf01504d4499110f3fba213fb53d` ==> `1f99379161e1734836e2940192b98405516be43c`

Compatibility test results for a68e71c05caebf01504d4499110f3fba213fb53d ==> 1f99379161e1734836e2940192b98405516be43c (PR)
Upgrade the nodes to version: 1f99379161e1734836e2940192b98405516be43c
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1308.8385833929256 txn/s, submitted: 1313.1823271631379 txn/s, failed submission: 4.343743770212329 txn/s, expired: 4.343743770212329 txn/s, latency: 2442.0712751091705 ms, (p50: 1800 ms, p90: 3900 ms, p99: 9600 ms), latency samples: 114500
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1100.3376235420958 txn/s, submitted: 1101.522055860225 txn/s, failed submission: 1.1844323181292742 txn/s, expired: 1.1844323181292742 txn/s, latency: 2902.248040904198 ms, (p50: 2100 ms, p90: 5400 ms, p99: 10500 ms), latency samples: 92900
5. check swarm health
Compatibility test for a68e71c05caebf01504d4499110f3fba213fb53d ==> 1f99379161e1734836e2940192b98405516be43c passed
Upgrade the remaining nodes to version: 1f99379161e1734836e2940192b98405516be43c
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1150.9964029119078 txn/s, submitted: 1153.6628038452943 txn/s, failed submission: 2.666400933386659 txn/s, expired: 2.666400933386659 txn/s, latency: 2718.3374613899614 ms, (p50: 2100 ms, p90: 4500 ms, p99: 10200 ms), latency samples: 103600
Test Ok

add num nodes check

c046a17

georgemitenkov added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Apr 25, 2024

georgemitenkov requested review from runtian-zhou and vgao1996 April 25, 2024 10:16

georgemitenkov requested a review from ziaptos April 25, 2024 10:16

propagate error in txn arg validation

8088c12

This comment has been minimized.

Sign in to view

unhappy tests

eb00ec4

georgemitenkov requested review from davidiw, wrwg and zekun000 as code owners April 25, 2024 11:05

This comment has been minimized.

Sign in to view

vgao1996 reviewed Apr 26, 2024

View reviewed changes

georgemitenkov added 3 commits April 29, 2024 00:21

add type config

cd2608f

constant types to use builder

404efb1

type substitution to use builder

05c701c

georgemitenkov changed the title ~~[move] Fix calculation of instantiation nodes in substitution~~ [move] Type builder & fix calculation of node count in types Apr 29, 2024

type loading to use builder

7cfc04f

georgemitenkov force-pushed the george/types branch from 746ee1e to 7cfc04f Compare April 29, 2024 11:12

Merge branch 'main' into george/types

78d68ae

georgemitenkov force-pushed the george/types branch from cd62279 to 78d68ae Compare April 29, 2024 11:18

This comment has been minimized.

Sign in to view

Merge branch 'main' into george/types

5840e2b

This comment has been minimized.

Sign in to view

Merge branch 'main' into george/types

1f99379

This comment has been minimized.

Sign in to view

georgemitenkov mentioned this pull request Jun 12, 2024

[move] Type builder & fix calculation of node count in types #13662

Closed

21 tasks

georgemitenkov merged commit ac636d2 into main Jun 12, 2024
47 checks passed

georgemitenkov deleted the george/types branch June 12, 2024 17:01

georgemitenkov mentioned this pull request Jun 13, 2024

[move] Fix paranoid tests with missing substatus #13683

Closed

21 tasks

[move] Type builder & fix calculation of node count in types #13028

[move] Type builder & fix calculation of node count in types #13028

Conversation

georgemitenkov commented Apr 25, 2024 • edited Loading

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

trunk-io bot commented Apr 25, 2024 • edited Loading

This comment has been minimized.

This comment has been minimized.

codecov bot commented Apr 25, 2024 • edited Loading

Codecov Report

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vgao1996 left a comment • edited Loading

Choose a reason for hiding this comment

runtian-zhou commented Apr 26, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jun 12, 2024

✅ Forge suite realistic_env_max_load success on 1f99379161e1734836e2940192b98405516be43c

github-actions bot commented Jun 12, 2024

✅ Forge suite compat success on a68e71c05caebf01504d4499110f3fba213fb53d ==> 1f99379161e1734836e2940192b98405516be43c

github-actions bot commented Jun 12, 2024

✅ Forge suite framework_upgrade success on a68e71c05caebf01504d4499110f3fba213fb53d ==> 1f99379161e1734836e2940192b98405516be43c

georgemitenkov commented Apr 25, 2024 •

edited

Loading

trunk-io bot commented Apr 25, 2024 •

edited

Loading

codecov bot commented Apr 25, 2024 •

edited

Loading

vgao1996 left a comment •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `1f99379161e1734836e2940192b98405516be43c`

✅ Forge suite `compat` success on `a68e71c05caebf01504d4499110f3fba213fb53d` ==> `1f99379161e1734836e2940192b98405516be43c`

✅ Forge suite `framework_upgrade` success on `a68e71c05caebf01504d4499110f3fba213fb53d` ==> `1f99379161e1734836e2940192b98405516be43c`