[vm] Resource access control: runtime engine #10544

wrwg · 2023-10-17T17:56:52Z

Implements the runtime engine for resource access control:

a representation of access control specifiers in loaded_data::runtime_access_specifiers.
a loader for access specifiers in runtime::loader::access_specifier_loader.
a new stateful object representing the access control logic in runtime::access_control.
finally the use of the AccessControlState in runtime::interpreter
unit test and combinatorial proptests

crates/aptos-admin-service/src/server/mod.rs

third_party/move/move-compiler-v2/src/bytecode_generator.rs

brmataptos · 2023-10-23T20:36:03Z

third_party/move/move-compiler-v2/src/bytecode_generator.rs

-                self.gen_select(target, id, mid.qualified_inst(*sid, inst), *fid, &arg)
+                // Get the instantiation of the struct. It's not contained in the select
+                // expression but in the type of it's operand.
+                if let Some((_, inst)) = self


lines 528-532 are nearly identical (modulo "arg" and "oper") to lines 944-948. Maybe a helper function with a meaningful name would be useful for readability?

I tried to extract it into a function but it is not entirely possible because of lifetime issues. env().get_node_type() returns a temporary type on the stack from which you can't borrow inside of a helper function. However, I introduced a helper for get_node_type reducing a little of the boilerplate, and applied the new helper over the code.

Yeah, I now understand this pain of Rust. (Lifetimes block abstraction.)

brmataptos · 2023-10-23T20:47:32Z

third_party/move/move-compiler-v2/src/file_format_generator/module_generator.rs

+                        if callee.module_id == module.get_id() {
+                            let count = usage.len();
+                            usage.extend(usage_map[&callee.id].iter().cloned());
+                            if usage.len() > count {


You can move the stuff involving count out of the callee loop here, just test once per fun. It also simplifies the code, not just an opt.

Good point, done.

brmataptos · 2023-10-23T20:49:52Z

third_party/move/move-compiler-v2/src/file_format_generator/module_generator.rs

+                    let mut usage = usage_map[&fun.get_id()].clone();
+                    let mut local_changes = false;
+                    for callee in callees {
+                        if callee.module_id == module.get_id() {


It's worth a comment to explain why we don't care about calls to other modules.

brmataptos · 2023-10-23T20:51:23Z

third_party/move/move-compiler-v2/src/file_format_generator/module_generator.rs

+        usage_map
+    }
+
+    fn get_function_acquires(&self, fun: &FunctionEnv) -> BTreeSet<StructId> {


Include "local" in the name so we know this is not including calls.

brmataptos · 2023-10-23T21:08:25Z

third_party/move/move-vm/types/src/loaded_data/runtime_access_specifier.rs

+        }
+    }
+
+    /// Returns true of the concrete access instance is enabled.


"of" -> "if"

brmataptos · 2023-10-23T21:41:21Z

third_party/move/move-vm/types/src/loaded_data/runtime_access_specifier.rs

+//! There is potential for optimization by simplifying exclusions are effectively negations,
+//! such a simplification is not trivial and may require recursive specifiers, which
+//! we like to avoid.
+


Do access control specifiers exactly specify the set of resources which may be accessed, or are they an approximation? If an approximation, which way can errors go?

If this is correct, can we specify it explicitly? 'join' operation corresponds to intersection of the sets of possibly accessed resources.

What does "specialize" mean? I'm guessing, that any address parameters mentioned in an AccessSpecifier are made specific given the address values available at runtime. Can we have an explicit comment about that somewhere? Perhaps on the "specialize" method.

Tried to reformulate and be a bit more precise, take a look again.

* [compiler v2] Pulls out fixes from #10544 to umblock testing There are fixes how to deal with global resources in #10544, pulling them out to unblock testing. * Addressing reviewer comments, fixing tests, adding test from original PR

zekun000 · 2023-11-27T23:12:41Z

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

+                return Err(PartialVMError::new(StatusCode::UNKNOWN_VERSION)
+                    .with_message(format!("bytecode version {} unsupported", version)));
+            } else if version == VERSION_NEXT
+                && !cfg!(test)


nit: I think we can use all() or any() to combine these

* [compiler v2] Pulls out fixes from #10544 to umblock testing There are fixes how to deal with global resources in #10544, pulling them out to unblock testing. * Addressing reviewer comments, fixing tests, adding test from original PR

brmataptos

I have a lot more to read, but here are a few more comments.

brmataptos · 2023-12-21T03:38:55Z

third_party/move/testing-infra/transactional-test-runner/Cargo.toml

@@ -13,7 +13,7 @@ edition = "2021"
 anyhow = "1.0.52"
 clap = { version = "4.3.9", features = ["derive"] }
 colored = "2.0.0"
-move-binary-format = { path = "../../move-binary-format" }
+move-binary-format = { path = "../../move-binary-format", features = ["testing"] }


I don't see a "testing" feature used anywhere in move-binary-format. What does it do? Where is it defined?

Its used to double guard v7 file format, only with this feature one can use v7

brmataptos · 2023-12-21T07:29:49Z

third_party/move/move-vm/runtime/src/interpreter.rs

                    if let Some(frame) = self.call_stack.pop() {
                        // Note: the caller will find the callee's return values at the top of the shared operand stack
                        current_frame = frame;
                        current_frame.pc += 1; // advance past the Call instruction in the caller
                    } else {
                        // end of execution. `self` should no longer be used afterward
+                        // Clean up access control
+                        self.access_control


Didn't you just do this on line 170-172?

vgao1996

2/n Feature gating

vgao1996 · 2024-01-19T01:18:33Z

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

+                && !cfg!(feature = "testing")
+                && !cfg!(feature = "fuzzing")


I'd strongly recommend we move away from gating critical features by the crate feature flags, due to cargo's handling of feature unification.

Right now there is a single fragile test during node startup ensuring that the node binary is not built with the "testing" feature flag

The test does not check the "fuzzing" feature flag

It would be a lot safer to pass in a bool variable indicating whether the next version shall be accepted.

As we discussed offline, this is not feature gating. Its a double safeguard that experimental code cannot run in production. The actual feature gating of the maximally supported bytecode version continues to be done via regular feature gating and on-chain config.

The actual feature gating of the maximally supported bytecode version continues to be done via regular feature gating and on-chain config.

I don't see this in the current implementation though. Have I missed anything?

My worry right now is just that if someone sends a move binary with NEXT_VERSION against the current implementation, it will be accepted if the cargo features are misconfigured, which sounds dangerous for production.

Okay now I see it. So the logic is that even if a binary w/ VERSION_NEXT passes the first check based on dynamic configuration, it will still fail the second check if we're not in any of the test modes. Indeed this is a double safeguard.

vgao1996

Looks good to me now!

Implements the runtime engine for resource access control: - a representation of access control specifiers in `loaded_data::runtime_access_specifiers`. - a loader for access specifiers in `runtime::loader::access_specifier_loader`. - a new stateful object representing the access control logic in `runtime::access_control`. - finally the use of the `AccessControlState` in `runtime::interpreter`.

github-actions · 2024-02-21T21:30:01Z

✅ Forge suite `compat` success on `aptos-node-v1.9.5` ==> `faba733a804774ca13112e3f1555e894f8148082`

Compatibility test results for aptos-node-v1.9.5 ==> faba733a804774ca13112e3f1555e894f8148082 (PR)
1. Check liveness of validators at old version: aptos-node-v1.9.5
compatibility::simple-validator-upgrade::liveness-check : committed: 6200 txn/s, latency: 5391 ms, (p50: 4800 ms, p90: 9000 ms, p99: 17200 ms), latency samples: 217000
2. Upgrading first Validator to new version: faba733a804774ca13112e3f1555e894f8148082
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1737 txn/s, latency: 16328 ms, (p50: 17800 ms, p90: 22300 ms, p99: 22900 ms), latency samples: 90360
3. Upgrading rest of first batch to new version: faba733a804774ca13112e3f1555e894f8148082
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 137 txn/s, submitted: 446 txn/s, expired: 308 txn/s, latency: 50406 ms, (p50: 48500 ms, p90: 58500 ms, p99: 86900 ms), latency samples: 12417
4. upgrading second batch to new version: faba733a804774ca13112e3f1555e894f8148082
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 2110 txn/s, latency: 13872 ms, (p50: 13600 ms, p90: 19600 ms, p99: 19900 ms), latency samples: 103400
5. check swarm health
Compatibility test for aptos-node-v1.9.5 ==> faba733a804774ca13112e3f1555e894f8148082 passed
Test Ok

github-actions · 2024-02-21T21:31:56Z

✅ Forge suite `realistic_env_max_load` success on `faba733a804774ca13112e3f1555e894f8148082`

two traffics test: inner traffic : committed: 7636 txn/s, latency: 5147 ms, (p50: 5000 ms, p90: 6000 ms, p99: 14700 ms), latency samples: 3291400
two traffics test : committed: 100 txn/s, latency: 2247 ms, (p50: 2100 ms, p90: 2300 ms, p99: 11000 ms), latency samples: 1780
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.238, avg: 0.208", "QsPosToProposal: max: 0.514, avg: 0.365", "ConsensusProposalToOrdered: max: 0.605, avg: 0.546", "ConsensusOrderedToCommit: max: 0.496, avg: 0.468", "ConsensusProposalToCommit: max: 1.054, avg: 1.014"]
Max round gap was 1 [limit 4] at version 1431321. Max no progress secs was 9.555537 [limit 15] at version 1431321.
Test Ok

* Fix `iss`-related bug in Groth16 path & refactor (#12017) Co-authored-by: Oliver <heliuchuan@gmail.com> * [aptosvm] Simplify VM flows (#11888) * Duplicated logic for creating the gas meter for view functions has been removed. * Duplicated logic for calculating gas used for view functions has been removed. * There was unreachable code in failure transaction cleanup, where the discarded status has been returned immediately, but then re-checked again. The first check is shifted inside. * No more default transaction metadata. * Scripts are now validated consistently. * Simplifies transaction execution function signature to avoid `Option<String>`. * Removes duplicated features from `AptosVM` and keeps them in `MoveVMExt`. * Fixes a bug when script hash was not computed for `RunOnAbort`. Related tests are moved to `move-e2e-tests`. * [Compiler V2] Critical edge elimination (#11894) Implement a pass to eliminate critical edges by splitting them with empty blocks * [consensus configs] reduce sending block size from 2500 to 1900 (#12091) ### Description The block output limit is no longer hit with p2p txns. ### Test Plan Forge `realistic_env_max_load` TPS improves. * [Indexer-grpc] Add profiling support. (#12034) * Minor aggregator cleanup (#12013) * Minor aggregator cleanup * Addressing PR comments * [move] rotate_authentication_key_call should not modify OriginatingAddress (#12108) Co-authored-by: Alin Tomescu <tomescu.alin@gmail.com> * [Data Streaming Service] Add dynamic prefetching support * [Data Streaming Service] Add dynamic prefetching unit tests. * [Data Streaming Service] Update existing integration tests. * [State Sync] Add backpressure to fast sync receiver. * Update perf baseline for gas charging coverage improvements (reducing throughput) (#12124) * Reduce latency of cloning network sender using Arc pointers (#12103) * Avoid cloning network sender using Arc pointers * Removing a clone * 100 node sweep test * Removing a few clone operations * reset forge test * Removing some clones * Removing clones * adopt AIP-61 terminology for consistency (#12123) adopt AIP-61 terminology for consistency * [Consensus] Remove non-decoupled execution and refactor for cleaner interfaces (#12104) * fix jwk key logging (#12090) * remove spurious error lines (#12137) * randomness #1: types update from randomnet (#12106) * types update from randomnet * update * lint * lint * All validators broadcast commit vote messages (#12059) * All validators broadcast commit messages * Forge testing * Increase timeout for forge * test forge realistic_env_workload_sweep_test * run realistic_env_workload_sweep_test * run realistic_env_workload_sweep_test * run sweep test * increase forge runner duration * forge testing * Letting the proposer also broadcast commit decision for backward compatibility * removing forge changes * Added a TODO * [vm] Resource access control: runtime engine (#10544) * [vm] Resource access control: runtime engine Implements the runtime engine for resource access control: - a representation of access control specifiers in `loaded_data::runtime_access_specifiers`. - a loader for access specifiers in `runtime::loader::access_specifier_loader`. - a new stateful object representing the access control logic in `runtime::access_control`. - finally the use of the `AccessControlState` in `runtime::interpreter`. * Addressing reviewer comments. * Addressing reviewer comments. * typo: PTLA -> PTAL * Rebasing: adjusting to upstream changes * Rebasing * ObjectCodeDeployment API cleanup update (#12133) * ObjectCodeDeployment API cleanup update (#12141) * [Compiler-v2] porting more V1 unit tests to V2 (#12085) * update tests * fix bug * fix-12116 * fix missing space * add expected got * remove live-var tests * fix had_erros * fix * Enable the max object nesting check (#12129) * Resolved the warning for unused variable (#12157) * update * update * update * update * update * update * update * update * update * update * update * update * update * Squashed commit of the following: commit a50ffec Author: Zhoujun Ma <zhoujun@aptoslabs.com> Date: Thu Feb 22 21:10:12 2024 +0000 lint commit 388350f Author: zhoujun.ma <zhoujun@aptoslabs.com> Date: Thu Feb 22 13:04:28 2024 -0800 update commit 76f7eca Author: zhoujun.ma <zhoujun@aptoslabs.com> Date: Thu Feb 22 12:56:04 2024 -0800 update commit a663542 Author: zhoujun.ma <zhoujun@aptoslabs.com> Date: Thu Feb 22 12:54:18 2024 -0800 update commit b439449 Author: zhoujun.ma <zhoujun@aptoslabs.com> Date: Thu Feb 22 12:34:14 2024 -0800 update commit 3378ceb Author: zhoujun.ma <zhoujun@aptoslabs.com> Date: Thu Feb 22 12:17:06 2024 -0800 update commit 6cd6685 Author: zhoujun.ma <zhoujun@aptoslabs.com> Date: Thu Feb 22 12:15:05 2024 -0800 update commit 6d89f37 Author: zhoujun.ma <zhoujun@aptoslabs.com> Date: Thu Feb 22 12:13:51 2024 -0800 update commit 980f257 Author: zhoujun.ma <zhoujun@aptoslabs.com> Date: Thu Feb 22 12:12:04 2024 -0800 update commit 16e9349 Author: Zhoujun Ma <zhoujun@aptoslabs.com> Date: Thu Feb 22 18:25:08 2024 +0000 lint --------- Co-authored-by: Alin Tomescu <tomescu.alin@gmail.com> Co-authored-by: Oliver <heliuchuan@gmail.com> Co-authored-by: George Mitenkov <georgemitenk0v@gmail.com> Co-authored-by: Zekun Wang <41706692+fEst1ck@users.noreply.github.com> Co-authored-by: Brian (Sunghoon) Cho <brian@aptoslabs.com> Co-authored-by: Guoteng Rao <3603304+grao1991@users.noreply.github.com> Co-authored-by: Satya Vusirikala <satyasatya123456@gmail.com> Co-authored-by: David Wolinsky <isaac.wolinsky@gmail.com> Co-authored-by: Josh Lind <josh.lind@hotmail.com> Co-authored-by: igor-aptos <110557261+igor-aptos@users.noreply.github.com> Co-authored-by: Sital Kedia <sitalkedia@users.noreply.github.com> Co-authored-by: Wolfgang Grieskamp <wg@aptoslabs.com> Co-authored-by: Teng Zhang <rahxephon89@163.com> Co-authored-by: Junkil Park <jpark@aptoslabs.com>

wrwg requested review from vineethk, runtian-zhou, rahxephon89, vgao1996 and brmataptos October 17, 2023 17:56

wrwg force-pushed the wg-rw branch from 94eccdd to 6f1841c Compare October 18, 2023 01:55

wrwg force-pushed the wg-access-runtime branch 3 times, most recently from c7035bf to e303ea2 Compare October 18, 2023 15:45

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

wrwg force-pushed the wg-access-runtime branch 2 times, most recently from 1ccb248 to e530af7 Compare October 20, 2023 04:10

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

wrwg force-pushed the wg-access-runtime branch 4 times, most recently from 09f7f1f to 5e7e999 Compare October 20, 2023 18:10

wrwg marked this pull request as ready for review October 20, 2023 18:10

wrwg force-pushed the wg-access-runtime branch from 5e7e999 to 4daf3b5 Compare October 21, 2023 02:59

brmataptos reviewed Oct 23, 2023

View reviewed changes

wrwg mentioned this pull request Oct 26, 2023

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

Merged

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

wrwg requested review from davidiw and zekun000 as code owners October 29, 2023 03:20

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

Base automatically changed from wg-rw to main October 29, 2023 21:52

wrwg requested review from vineethk and brmataptos November 23, 2023 05:20

zekun000 reviewed Nov 27, 2023

View reviewed changes

vineethk approved these changes Nov 30, 2023

View reviewed changes

brmataptos reviewed Dec 21, 2023

View reviewed changes

vgao1996 reviewed Jan 19, 2024

View reviewed changes

wrwg force-pushed the wg-access-runtime branch 2 times, most recently from 011237c to 26bd182 Compare February 5, 2024 16:54

wrwg requested review from runtian-zhou, zekun000, brmataptos and vgao1996 February 5, 2024 16:55

vgao1996 approved these changes Feb 21, 2024

View reviewed changes

wrwg added 6 commits February 21, 2024 12:45

Addressing reviewer comments.

ff889b9

Addressing reviewer comments.

79752a1

typo: PTLA -> PTAL

b0b8d19

Rebasing: adjusting to upstream changes

0cc5efd

Rebasing

faba733

wrwg force-pushed the wg-access-runtime branch from 26bd182 to faba733 Compare February 21, 2024 21:01

wrwg enabled auto-merge (squash) February 21, 2024 21:01

This comment has been minimized.

Sign in to view

wrwg merged commit f325f52 into main Feb 21, 2024
87 of 100 checks passed

wrwg deleted the wg-access-runtime branch February 21, 2024 21:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vm] Resource access control: runtime engine #10544

[vm] Resource access control: runtime engine #10544

wrwg commented Oct 17, 2023 •

edited

Loading

brmataptos Oct 23, 2023

wrwg Oct 26, 2023

brmataptos Dec 5, 2023

brmataptos Oct 23, 2023

wrwg Oct 26, 2023

brmataptos Oct 23, 2023

wrwg Oct 26, 2023

brmataptos Oct 23, 2023

wrwg Oct 26, 2023

brmataptos Oct 23, 2023

wrwg Oct 26, 2023

brmataptos Oct 23, 2023

wrwg Oct 26, 2023

zekun000 Nov 27, 2023

wrwg Feb 5, 2024

brmataptos left a comment

brmataptos Dec 21, 2023

wrwg Feb 5, 2024

brmataptos Dec 21, 2023

vgao1996 left a comment

vgao1996 Jan 19, 2024

wrwg Feb 5, 2024

vgao1996 Feb 21, 2024

vgao1996 Feb 21, 2024

vgao1996 left a comment

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

[vm] Resource access control: runtime engine #10544

[vm] Resource access control: runtime engine #10544

Conversation

wrwg commented Oct 17, 2023 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgao1996 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

vgao1996 left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Feb 21, 2024

✅ Forge suite compat success on aptos-node-v1.9.5 ==> faba733a804774ca13112e3f1555e894f8148082

github-actions bot commented Feb 21, 2024

✅ Forge suite realistic_env_max_load success on faba733a804774ca13112e3f1555e894f8148082

wrwg commented Oct 17, 2023 •

edited

Loading

✅ Forge suite `compat` success on `aptos-node-v1.9.5` ==> `faba733a804774ca13112e3f1555e894f8148082`

✅ Forge suite `realistic_env_max_load` success on `faba733a804774ca13112e3f1555e894f8148082`