[move] implement resource groups by davidiw · Pull Request #6040 · aptos-labs/aptos-core

davidiw · 2023-01-02T08:54:48Z

A resource group allows multiple resources to occupy the same slot in storage
Hides resource groups in the Move Resolver layer so that storage need not know explicitly about them
Added a new wrapper around the move resolver, so that the VM could be injected for re-use of the VM's metadata cache that includes resource groups
Tests that verify the lifetime of resource groups

The following remains incomplete:

Adding a feature for resource groups to be enabled
Handling compatibility on upgrades of upgrade
Potentially caching of resources within a resource group -- though this is not a blocker

movekevin · 2023-01-04T07:05:36Z

Are there more unit tests we can add to test the changeset conversions and resource group operation grouping in session? This would allow testing more edge cases than what the e2e test can cover.

movekevin

I wonder if there should be some limit on how big a resource group can get. By arbitrarily grouping resources into a resource group, especially with cross-module grouping, a resource group can get very large.

CapCap · 2023-01-04T18:17:30Z

I wonder if there should be some limit on how big a resource group can get. By arbitrarily grouping resources into a resource group, especially with cross-module grouping, a resource group can get very large.

assuming rockdsb allows reading arbitrary byte offsets of values, we could potentially change the key to be something like (struct_tag: StructTag, start_offset_bytes: u64, num_bytes: u64), and store the keys separate from values, so that we can deserialize them separately and pick/choose (if desired) which values to deserialize

zekun000

lgtm

zekun000 · 2023-01-19T15:05:45Z

I guess what @msmouse wanted is to always call this and decide which access path to use?

zekun000 · 2023-01-19T15:10:46Z

msmouse · 2023-01-18T23:44:57Z

Is this slow? It can damage API performance whenever non-existent resources are requested? -- it's the best if we can cache the metadata and ensure it's cached.

msmouse

Trusting you to fix the cache.

Thank you boss for making this reality.

* A resource group allows multiple resources to occupy the same slot in storage * Hides resource groups in the Move Resolver layer so that storage need not know explicitly about them * Added a new wrapper around the move resolver, so that the VM could be injected for re-use of the VM's metadata cache that includes resource groups * Tests that verify the lifetime of resource groups

* verify at compilation time that scope is correct * verify during module initialization that scope is correct

* individual resource reads just move to the move resolver, which is seamless * all account resources require expansion -- this can result in adding too more than the limit -- but resources in a group are somewhat limited in size

Oh the ways to analyze resource groups... this checks for compatible upgrades: Perform upgrade checks on resource grups only -- this doesn't validate that the contents are correct. * Acquire all relevant pieces of metadata * Verify that there are no duplicate attributes, though this should be handled upstream, so this becomes more an invariant, duplicate check. * Ensure that each member has a membership and it does not change * Ensure that each group has a scope and that it does not become more restrictive

This adds a clean path to better support around ResourceGroups on the storage side without having to retrofit the containers. From a security persepective, it means one does not have to reason about the possibility of data existing in a slot prior to resource groups.

The original approach separated upgrade compatibility from resource group intrinsics. This resulted in evaluating modules twice. Interestingly enough, this didn't reduce the code much.

namely this reproduces the checks done in the compiler

The existing interface here is a bit of a mess: * We have an adapter that pretends we can have different types of VMs * We only really have Move and everything in this codebase knows it * There are typed parameters everywhere, so it isn't possible to seamlessly use trait objects, so everything needs to be explicit * sessions expect references, references cannot be easily moved after kicking off a session, so the SessionExt cannot house the resolver / storage

github-actions · 2023-01-21T10:39:54Z

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `dac888e04385b98fbb5950890550d4477db337c0`

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> dac888e04385b98fbb5950890550d4477db337c0 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7467 TPS, 5198 ms latency, 6700 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: dac888e04385b98fbb5950890550d4477db337c0
compatibility::simple-validator-upgrade::single-validator-upgrade : 4516 TPS, 9124 ms latency, 12400 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: dac888e04385b98fbb5950890550d4477db337c0
compatibility::simple-validator-upgrade::half-validator-upgrade : 4384 TPS, 9089 ms latency, 11600 ms p99 latency,no expired txns
4. upgrading second batch to new version: dac888e04385b98fbb5950890550d4477db337c0
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6065 TPS, 6497 ms latency, 11300 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> dac888e04385b98fbb5950890550d4477db337c0 passed
Test Ok

github-actions · 2023-01-21T10:41:23Z

✅ Forge suite `land_blocking` success on `dac888e04385b98fbb5950890550d4477db337c0`

performance benchmark with full nodes : 5931 TPS, 6691 ms latency, 11400 ms p99 latency,no expired txns
Test Ok

lightmark · 2023-01-22T16:01:10Z

-            .collect::<Result<_>>()?;
+            .collect::<Result<Vec<(StructTag, Vec<u8>)>>>()?;
+
+        // We should be able to do an unwrap here, otherwise the above db read would fail.


It seem state_view_at_version just never fail.

lightmark · 2023-01-23T08:53:08Z

+        struct_tag: &StructTag,
+    ) -> Result<Option<Vec<u8>>, VMError>;
+
+    fn get_any_resource(


api only? not for vm, right?

lightmark · 2023-01-23T09:33:18Z

+        // The use of this implies that we could theoretically call unwrap with no consequences,
+        // but using unwrap means the code panics if someone can come up with an attack.
+        let common_error = PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR)
+            .finish(Location::Undefined);


Location is undefined? I thought it would be in StructTag::module

lightmark · 2023-01-23T09:36:05Z

+                            let data = source_data.insert(struct_tag, data);
+                            if data.is_some() {
+                                return Err(common_error);
+                            }


can be in one line.

lightmark · 2023-01-23T09:56:28Z

+        match self {
+            ResourceGroupScope::Global => "global",
+            ResourceGroupScope::Address => "address",
+            ResourceGroupScope::Module => "module_",


why underscore?

lightmark · 2023-01-23T10:01:34Z

+            }
+        })?;
+
+        // Every struct contains the "dummy_field".


What's this?

davidiw requested review from lightmark and msmouse January 2, 2023 08:54

davidiw requested review from alinush, movekevin, wrwg and zekun000 as code owners January 2, 2023 08:54

davidiw force-pushed the resource_groups branch from da5f99c to 9e96b65 Compare January 2, 2023 08:55

davidiw changed the title ~~[move] implement a basic resource groups~~ [move] implement resource groups Jan 2, 2023

davidiw added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Jan 2, 2023

davidiw mentioned this pull request Jan 2, 2023

[framework] exploring a working object model in aptos framework #5976

Merged

This comment has been minimized.

Sign in to view

davidiw force-pushed the resource_groups branch from 9e96b65 to b0fd8af Compare January 2, 2023 16:51

This comment has been minimized.

Sign in to view

CapCap reviewed Jan 2, 2023

View reviewed changes

Comment thread aptos-move/framework/src/extended_checks.rs Outdated

CapCap reviewed Jan 2, 2023

View reviewed changes

Comment thread aptos-move/framework/src/extended_checks.rs Outdated

movekevin reviewed Jan 4, 2023

View reviewed changes

zekun000 reviewed Jan 4, 2023

View reviewed changes

Comment thread aptos-move/aptos-vm/src/data_cache.rs Outdated

michelle-aptos mentioned this pull request Jan 19, 2023

[AIP-9][Discussion] Resource Groups aptos-foundation/AIPs#26

Closed

zekun000 reviewed Jan 19, 2023

View reviewed changes

msmouse reviewed Jan 19, 2023

View reviewed changes

msmouse approved these changes Jan 19, 2023

View reviewed changes

davidiw added 10 commits January 20, 2023 18:54

[move] add resource group scope

be494f3

* verify at compilation time that scope is correct * verify during module initialization that scope is correct

[framework] features for resource groups

63dd813

[framework] features for resource groups

e756257

[api] add support for reading resource groups

d102b28

* individual resource reads just move to the move resolver, which is seamless * all account resources require expansion -- this can result in adding too more than the limit -- but resources in a group are somewhat limited in size

[aptos-vm] unify the validation of resource groups during publishing

3d933d3

The original approach separated upgrade compatibility from resource group intrinsics. This resulted in evaluating modules twice. Interestingly enough, this didn't reduce the code much.

[move] verify at publish time resource groups struct data

4da3d6a

namely this reproduces the checks done in the compiler

[resource_groups] move verifier code

a520f90

davidiw force-pushed the resource_groups branch from 3ea4a2e to 69cb47d Compare January 21, 2023 02:59

This comment has been minimized.

Sign in to view

davidiw force-pushed the resource_groups branch from 69cb47d to dac888e Compare January 21, 2023 09:54

This comment has been minimized.

Sign in to view

davidiw merged commit f279d68 into aptos-labs:main Jan 21, 2023

davidiw deleted the resource_groups branch January 21, 2023 10:42

lightmark reviewed Jan 23, 2023

View reviewed changes

jolestar mentioned this pull request Jan 29, 2023

[research] How to support Move Layer1's VM and framework rooch-network/rooch#2

Closed

alnoki mentioned this pull request Mar 20, 2023

Optimize storage through resource groups econia-labs/econia#90

Open

Conversation

davidiw commented Jan 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

movekevin commented Jan 4, 2023

Uh oh!

movekevin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CapCap commented Jan 4, 2023

Uh oh!

zekun000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

msmouse left a comment

Choose a reason for hiding this comment

Uh oh!

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 Jan 21, 2023

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> dac888e04385b98fbb5950890550d4477db337c0

Uh oh!

github-actions Bot commented Jan 21, 2023

✅ Forge suite land_blocking success on dac888e04385b98fbb5950890550d4477db337c0

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

davidiw commented Jan 2, 2023 •

edited

Loading

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `dac888e04385b98fbb5950890550d4477db337c0`

✅ Forge suite `land_blocking` success on `dac888e04385b98fbb5950890550d4477db337c0`