[compiler v2] Resource access control (read-write sets) #10480

wrwg · 2023-10-12T01:26:09Z

Description

This is an e2e implementation of resource access control for Move, with most parts in place:

Replaces the acquires syntax in a downwards-compatible way
The extended syntax is only available in compiler v2
One can now specify acquires, reads, and writes
One can specify the address of a resource in dependency of parameters
Multiple levels of wildcards are allowed, e.g. acquires *(object::address_of(param)) specifies that all resources at the given address are read or written.
Implements parsing->expansion->move model->file format generator
Extends file_format::FunctionHandle to carry the new information, introducing bytecode version v7. v7 became the new experimental version only available in test code for now.
TODO: dynamic runtime checking of resource access. Static analysis is also on the horizon, but not needed for an MVP of this feature.
TODO: bytecode verification of access specifiers

An AIP for this new feature will be filed soon.

As an example, here is some extract from the tests:

module 0x42::m {

    struct S has store {}
    struct R has store {}
    struct T has store {}
    struct G<T> has store {}

    fun f1() acquires S {
    }

    fun f2() reads S {
    }

    fun f3() writes S {
    }

    fun f4() acquires S(*) {
    }

    fun f_multiple() acquires R reads R writes T, S reads G<u64> {
    }

    fun f5() acquires 0x42::*::* {
    }

    fun f6() acquires 0x42::m::R {
    }

    fun f7() acquires *(*) {
    }

    fun f8() acquires *(0x42) {
    }

    fun f9(a: address) acquires *(a) {
    }

    fun f10(x: u64) acquires *(make_up_address(x)) {
    }

    fun make_up_address(x: u64): address {
        @0x42
    }
}

Test Plan

Unit tests for v2 and v1 compiler.

brmataptos

Will have more later, but send what I have.

brmataptos · 2023-10-12T20:47:07Z

aptos-move/aptos-vm/src/data_cache.rs

@@ -100,7 +100,7 @@ impl<'e, E: ExecutorView> StorageAdapter<'e, E> {
            self.accurate_byte_count = true;
        }
        self.max_binary_format_version =
-            get_max_binary_format_version(features, gas_feature_version);
+            get_max_binary_format_version(features, Some(gas_feature_version));


What does this have to do with Read/Write sets?

We constantly improve the code while we are visiting it. Here I was improving the way how the the current default binary format version was determined, since I'm adding a new version. There were multiple places how the default was retrieved ad hoc, and it took me some to figure this out, so I unified them into this function here. However, for the unification I needed to make the 2nd parameter optional.

brmataptos · 2023-10-12T20:48:27Z

aptos-move/aptos-vm/src/move_vm_ext/vm.rs

+    // For historical reasons, we support still < gas version 5, but if a new caller don't specify
+    // the gas version, we default to 5, which was introduced in late '22.
+    let gas_feature_version = gas_feature_version_opt.unwrap_or(5);
+    if gas_feature_version < 5 {


This also seems like unrelated code.

This is the same thing as above, dealing with a new file format version in a better way.

brmataptos

Atomically generated lattices are useful, since they are closed under negation, union, intersection. If we can represent subsets of A, B, C as well as * which includes "unknown others", it's best to also include a rep for "unknown others" (to give flexibility in representing unions without jumping to *, and more precision in intersection and negation).

brmataptos · 2023-10-14T23:10:03Z

third_party/move/move-binary-format/src/file_format.rs

+///   // All resources in the module
+///   reads 0xcafe::my_module::*;
+///   // The given resource in the module, at arbitray address
+///   reads 0xcafe::my_module::R(*);


We need a way to represent "Any address except an address of an object or other cryptographically unique namespace" as opposed to "Any address at all". Not sure how to do that in a simple way. We can already write "any object field of type R" R(object::address_of(*)) but we need "any non-object field of type R", e.g., R(literal(*)). But we should probably make the most common one * and less common one wordier.

That sounds interesting. With the recently added negation, we can at least say !acquires R(object::owner(o)). But being able to specify cryptographic namespaces is challenging. At least currently, object addresses are encrypted, and is not possible to test whether an object address is derived from a seed, address, or txn hash. We would need to actually divide addresses into components.

Good point. Though if the the cryptographically unique addresses of object::owner(o) can't be leaked out of runtime, then any address coming in shouldn't alias with that. But in any case, try to make sure the lattice is complete at each level so you can have closure under negation, join, and intersection.

After a chat with Aaron last week I now understand that object::owner(o) can be leaked, so could alias something else unless it's just been created.

@0x42

This is an e2e implementation of resource access control for Move, with most parts in place: - Replaces the acquires syntax in a downwards-compatible way - The extended syntax is only available in compiler v2 - One can now specify `acquires`, `reads`, and `writes` - One can specify the address of a resource in dependency of parameters - Multiple levels of wildcards are allowed, e.g. `acquires *(object::address_of(param))` specifies that all resources at the given address are read or written. - Implements parsing->expansion->move model->file format generator - Extends `file_format::FunctionHandle` to carry the new information, introducing bytecode version v7. v7 became the new experimental version only available in test code for now. - TODO: dynamic runtime checking of resource access. Static analysis is also on the horizon, but not needed for an MVP of this feature. - TODO: bytecode verification of access specifiers An AIP for this new feature will be filed soon. As an example, here is some extract from the tests: ```move module 0x42::m { struct S has store {} struct R has store {} struct T has store {} struct G<T> has store {} fun f1() acquires S { } fun f2() reads S { } fun f3() writes S { } fun f4() acquires S(*) { } fun f_multiple() acquires R reads R writes T, S reads G<u64> { } fun f5() acquires 0x42::*::* { } fun f6() acquires 0x42::m::R { } fun f7() acquires *(*) { } fun f8() acquires *(0x42) { } fun f9(a: address) acquires *(a) { } fun f10(x: u64) acquires *(make_up_address(x)) { } fun make_up_address(x: u64): address { @0x42 } } ```

brmataptos · 2023-10-29T04:11:41Z

third_party/move/move-binary-format/src/deserializer.rs

+}
+
+impl SerializedOption {
+    fn from_u8(value: u8) -> BinaryLoaderResult<bool> {


This isn't great. What do true and false mean as Option enum discriminants? Better if you can have an enum somewhere for the values.

I found this related crate: https://docs.rs/enum-kinds/latest/enum_kinds/ but you can't use it for Option, anyway, so it wouldn't be useful even if you wanted to use it. std::mem::Discriminant<Option<X>> might make sense here, but without X I don't think it's meaningful. But using 'bool' is too confusing.

This is a boolean indicating the presence or the absence of the option. I think is fine.

brmataptos · 2023-10-29T04:14:15Z

third_party/move/move-binary-format/src/file_format.rs

@@ -849,6 +858,116 @@ impl Arbitrary for AbilitySet {
    }
 }

+/// An `AccessSpecifier` describes an approximation of resources accessed by a function.


What does "approximation" mean here? Overestimate? Underestimate?

Documented, Done

brmataptos · 2023-10-29T04:28:55Z

third_party/move/move-compiler/src/parser/ast.rs

+/// chain can be wildcards (`*`).
+#[derive(Debug, Clone, PartialEq)]
+pub enum AccessSpecifier_ {
+    Acquires(bool, NameAccessChain, Option<Vec<Type>>, AddressSpecifier),


I agree with Vineeth that this is nonoptimal, as we don't get to abstract away the kind of access from the access specification. Using Rust's pattern matching is convenient, but wordy. Right now we have 3 clauses to match these three cases, and each clause has to mention all 4 data fields. If the 4 data fields were grouped as a type, then processing the fields would be abstracted away. If we decide to add more entries to Acquires, Reads, and Writes in the future (I can imagine something related to Aggregations/Reductions and PrivatizableResources) then we duplicate the processing.

BUT: This can be refactored later, this is good enough for now.

brmataptos · 2023-10-29T04:30:26Z

third_party/move/move-compiler/src/parser/ast.rs

@@ -267,7 +291,7 @@ pub struct Function {
    pub visibility: Visibility,
    pub entry: Option<Loc>,
    pub signature: FunctionSignature,
-    pub acquires: Vec<NameAccessChain>,
+    pub access_specifiers: Option<Vec<AccessSpecifier>>,


For the access_specifiers field, what does None mean as opposed to Some(empty vector) ?

brmataptos · 2023-10-29T04:45:29Z

third_party/move/move-model/src/builder/module_builder.rs

@@ -77,6 +78,8 @@ pub(crate) struct ModuleBuilder<'env, 'translator> {
    pub inline_spec_builder: Spec,
    /// Translated function definitions, if we are compiling Move code
    pub fun_defs: BTreeMap<Symbol, Exp>,
+    /// Translated access specifiers, if we are compiling Move code


I like how fun everything is in Move.

brmataptos · 2023-10-29T05:44:05Z

Maybe if you say that in a comment. It's obvious to you, maybe...

…

On Sat, Oct 28, 2023, 9:54 PM Wolfgang Grieskamp ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In third_party/move/move-binary-format/src/deserializer.rs <#10480 (comment)> : > @@ -1810,3 +1906,66 @@ impl Opcodes { } } } + +impl SerializedBool { + fn from_u8(value: u8) -> BinaryLoaderResult<bool> { + match value { + 0x1 => Ok(false), + 0x2 => Ok(true), + _ => Err(PartialVMError::new(StatusCode::MALFORMED) + .with_message("malformed boolean".to_owned())), + } + } +} + +impl SerializedOption { + fn from_u8(value: u8) -> BinaryLoaderResult<bool> { This is a boolean indicating the presence or the absence of the option. I think is fine. — Reply to this email directly, view it on GitHub <#10480 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A7S3T43HG5U2VNQ3MPA3F7LYBXOQ7AVCNFSM6AAAAAA5427U36VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMBSHA4TMMZTHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

wrwg · 2023-10-29T21:20:35Z

Maybe if you say that in a comment. It's obvious to you, maybe...

Done

github-actions · 2023-10-29T21:44:04Z

✅ Forge suite `compat` success on `aptos-node-v1.7.2` ==> `88c057ab9bc82a6cb33c994b8ec4c3f91a1d6d92`

Compatibility test results for aptos-node-v1.7.2 ==> 88c057ab9bc82a6cb33c994b8ec4c3f91a1d6d92 (PR)
1. Check liveness of validators at old version: aptos-node-v1.7.2
compatibility::simple-validator-upgrade::liveness-check : committed: 4913 txn/s, latency: 6711 ms, (p50: 6900 ms, p90: 10500 ms, p99: 11700 ms), latency samples: 186700
2. Upgrading first Validator to new version: 88c057ab9bc82a6cb33c994b8ec4c3f91a1d6d92
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1285 txn/s, latency: 16068 ms, (p50: 19000 ms, p90: 22000 ms, p99: 22600 ms), latency samples: 92520
3. Upgrading rest of first batch to new version: 88c057ab9bc82a6cb33c994b8ec4c3f91a1d6d92
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1441 txn/s, latency: 19160 ms, (p50: 18900 ms, p90: 24200 ms, p99: 26800 ms), latency samples: 74980
4. upgrading second batch to new version: 88c057ab9bc82a6cb33c994b8ec4c3f91a1d6d92
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3707 txn/s, latency: 8760 ms, (p50: 9900 ms, p90: 12300 ms, p99: 12600 ms), latency samples: 144580
5. check swarm health
Compatibility test for aptos-node-v1.7.2 ==> 88c057ab9bc82a6cb33c994b8ec4c3f91a1d6d92 passed
Test Ok

github-actions · 2023-10-29T21:45:55Z

✅ Forge suite `realistic_env_max_load` success on `88c057ab9bc82a6cb33c994b8ec4c3f91a1d6d92`

two traffics test: inner traffic : committed: 8110 txn/s, latency: 4836 ms, (p50: 4500 ms, p90: 5700 ms, p99: 9900 ms), latency samples: 3503600
two traffics test : committed: 100 txn/s, latency: 2153 ms, (p50: 2100 ms, p90: 2500 ms, p99: 5200 ms), latency samples: 1800
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.200, avg: 0.196", "QsPosToProposal: max: 0.163, avg: 0.151", "ConsensusProposalToOrdered: max: 0.603, avg: 0.577", "ConsensusOrderedToCommit: max: 0.520, avg: 0.501", "ConsensusProposalToCommit: max: 1.116, avg: 1.079"]
Max round gap was 1 [limit 4] at version 1745846. Max no progress secs was 4.123938 [limit 10] at version 1745846.
Test Ok

github-actions · 2023-10-29T21:58:13Z

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.7.2` ==> `88c057ab9bc82a6cb33c994b8ec4c3f91a1d6d92`

Compatibility test results for aptos-node-v1.7.2 ==> 88c057ab9bc82a6cb33c994b8ec4c3f91a1d6d92 (PR)
Upgrade the nodes to version: 88c057ab9bc82a6cb33c994b8ec4c3f91a1d6d92
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 7065 txn/s, latency: 4807 ms, (p50: 4800 ms, p90: 7800 ms, p99: 8300 ms), latency samples: 247280
5. check swarm health
Compatibility test for aptos-node-v1.7.2 ==> 88c057ab9bc82a6cb33c994b8ec4c3f91a1d6d92 passed
Test Ok

wrwg requested review from vineethk, runtian-zhou, rahxephon89, vgao1996, fEst1ck and brmataptos October 12, 2023 01:26

wrwg requested review from davidiw and zekun000 as code owners October 12, 2023 01:26

wrwg mentioned this pull request Oct 12, 2023

[AIP Resource Access Control] Initial document aptos-foundation/AIPs#261

Merged

brmataptos reviewed Oct 13, 2023

View reviewed changes

brmataptos reviewed Oct 15, 2023

View reviewed changes

wrwg force-pushed the wg-rw branch 5 times, most recently from a65bbd7 to 94eccdd Compare October 17, 2023 17:40

wrwg mentioned this pull request Oct 17, 2023

[vm] Resource access control: runtime engine #10544

Merged

wrwg force-pushed the wg-rw branch 5 times, most recently from a1a82e4 to e6efadf Compare October 19, 2023 23:47

wrwg added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Oct 20, 2023

This comment has been minimized.

Sign in to view

wrwg force-pushed the wg-rw branch from e6efadf to aa31501 Compare October 20, 2023 04:13

This comment has been minimized.

Sign in to view

rahxephon89 approved these changes Oct 29, 2023

View reviewed changes

wrwg added 5 commits October 28, 2023 20:10

Addressing reviewer comments

f99a263

Addressing reviewer comments #2

f0b0a37

Addressing reviewer comments #3

2c45d91

Addressing reviewer comments #4

ef3bf22

wrwg force-pushed the wg-rw branch from 8798357 to 89bd1ef Compare October 29, 2023 03:20

wrwg enabled auto-merge (squash) October 29, 2023 03:20

wrwg force-pushed the wg-rw branch from 89bd1ef to 444d407 Compare October 29, 2023 03:25

This comment has been minimized.

Sign in to view

brmataptos reviewed Oct 29, 2023

View reviewed changes

Reviewer comments #5

88c057a

wrwg force-pushed the wg-rw branch from 444d407 to 88c057a Compare October 29, 2023 21:19

This comment has been minimized.

Sign in to view

wrwg merged commit 58271fd into main Oct 29, 2023
47 checks passed

wrwg deleted the wg-rw branch October 29, 2023 21:52

[compiler v2] Resource access control (read-write sets) #10480

[compiler v2] Resource access control (read-write sets) #10480

Conversation

wrwg commented Oct 12, 2023

Description

Test Plan

brmataptos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brmataptos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wrwg Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brmataptos commented Oct 29, 2023 via email

wrwg commented Oct 29, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Oct 29, 2023

✅ Forge suite compat success on aptos-node-v1.7.2 ==> 88c057ab9bc82a6cb33c994b8ec4c3f91a1d6d92

github-actions bot commented Oct 29, 2023

✅ Forge suite realistic_env_max_load success on 88c057ab9bc82a6cb33c994b8ec4c3f91a1d6d92

github-actions bot commented Oct 29, 2023

✅ Forge suite framework_upgrade success on aptos-node-v1.7.2 ==> 88c057ab9bc82a6cb33c994b8ec4c3f91a1d6d92

wrwg Oct 20, 2023 •

edited

Loading

✅ Forge suite `compat` success on `aptos-node-v1.7.2` ==> `88c057ab9bc82a6cb33c994b8ec4c3f91a1d6d92`

✅ Forge suite `realistic_env_max_load` success on `88c057ab9bc82a6cb33c994b8ec4c3f91a1d6d92`

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.7.2` ==> `88c057ab9bc82a6cb33c994b8ec4c3f91a1d6d92`