[Aptos Framework][API][Transaction][VM] Add a richer multisig account model #5894

movekevin · 2022-12-16T07:32:40Z

Description

Technical design doc: https://www.notion.so/aptoslabs/Multisig-accounts-a518df6e2af842f5b98519386624e37c
Will open an AIP for discussion soon.

This adds a multisig account module that will be paired with a new transaction payload type (existing user transaction). Detailed flow:

Owners can create a new multisig account by calling create (signer is default single owner) or with create_with_owners where multiple initial owner addresses can be specified. This is different (and easier) from the native multisig scheme where the owners' public keys have to be specified. Here, only addresses are needed.
Owners can be added/removed any time by calling add_owners or remove_owners. The transactions to do still need to follow the k-of-n scheme specified for the multisig account.
To create a new transaction, an owner can call create_transaction with the transaction payload. This will store the full transaction payload on chain, which adds decentralization (censorship is not possible) and makes it easier to fetch all transactions waiting for execution. If saving gas is desired, an owner can alternatively call create_transaction_with_hash where only the payload hash is stored. Later execution will be verified using the hash. Only owners can create transactions and a transaction id (incremeting id) will be assigned.
To approve or reject a transaction, other owners can call approve() or reject() with the transaction id.
If there are enough approvals, any owner can execute the transaction using the special MultisigTransaction type with the transaction id if the full payload is already stored on chain or with the transaction payload if only a hash is stored. Transaction execution will first check with this module that the transaction payload has gotten enough signatures. If so, it will be executed as the multisig account. The owner who executes will pay for gas.
If there are enough rejections, any owner can remove the transaction by calling remove().

Test Plan

Unit + e2e tests in api/ + Typescript example in ecosystem/typescript/sdk/examples/typescript/multisig_account.ts

aptos-move/framework/aptos-framework/sources/multisig_account.move

davidiw

coming together well... looking forward to the other half of this

aptos-move/framework/aptos-framework/sources/multisig_account.move

lightmark

The first round review. I haven't fully figured out how the special txn works to execute.

aptos-move/framework/aptos-framework/sources/multisig_account.move

davidiw · 2023-03-01T16:11:11Z

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

+        // Charge gas for writeset before we do cleanup. This ensures we don't charge gas for
+        // cleanup writeset changes, which is consistent with outer-level success cleanup
+        // flow. We also wouldn't need to worry that we run out of gas when doing cleanup.
+        let inner_function_change_set_ext = session
+            .finish(&mut (), gas_meter.change_set_configs())
+            .map_err(|e| e.into_vm_status())?;
+        gas_meter.charge_write_set_gas_for_io(inner_function_change_set_ext.write_set().iter())?;
+        gas_meter.charge_storage_fee(
+            inner_function_change_set_ext.write_set().iter(),
+            inner_function_change_set_ext.change_set().events(),
+            txn_data.transaction_size,
+            txn_data.gas_unit_price,
+        )?;


this same chunk of code seems to be in a few places

Yep unfortunately. But they need to be done in different places in the entry function vs multisig tx execution flows. I'ev found a good way to deduplicate them yet.

davidiw · 2023-03-01T16:13:54Z

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

+            DeltaStateView::new(&storage_with_changes, &delta_write_set).into_move_resolver();
+        let resolver = self.0.new_move_resolver(&storage_with_changes);
+        let mut cleanup_session = self.0.new_session(&resolver, SessionId::txn_meta(txn_data));
+        cleanup_session.execute_function_bypass_visibility(


can't we just dispatch here based upon the type of txn? it seems like the rest of this code largely is copy pasta/

What do you mean by dispatch here based upon the type of txn? This is calling a specific private function in the multisig_account module that's meant to resolve the successfully executed multisig tx.

davidiw · 2023-03-01T16:18:35Z

types/src/transaction/script.rs

-    module: ModuleId,
-    function: Identifier,
-    ty_args: Vec<TypeTag>,
+    pub module: ModuleId,
+    pub function: Identifier,
+    pub ty_args: Vec<TypeTag>,
    #[serde(with = "vec_bytes")]
-    args: Vec<Vec<u8>>,
+    pub args: Vec<Vec<u8>>,


why when we have getters below?

Needed for conversion logic which creates this struct. I can add a constructor but that doesn't seem better than making these public, at least in my noob Rust experience?

Well the data structure went from immutable to mutable. So this is definitely not a desirable pattern.

davidiw

wen python support?

movekevin · 2023-03-01T16:56:02Z

wen python support?

Good call. I'll do this in a separate followup PR

clay-aptos · 2023-03-01T19:12:01Z

Hey @movekevin , please help me get this feature documented in Aptos.dev. I saw the changes to the Move reference files.

Let me know if I should open an issue. Thanks!

github-actions · 2023-03-01T19:30:10Z

✅ Forge suite `land_blocking` success on `6d73d4a9aa101369b4e6e51a6df57847de0d7efb`

performance benchmark with full nodes : 6109 TPS, 6486 ms latency, 13200 ms p99 latency,(!) expired 360 out of 2609320 txns
Test Ok

github-actions · 2023-03-01T19:30:21Z

✅ Forge suite `framework_upgrade` success on `cb4ba0a57c998c60cbab65af31a64875d2588ca5` ==> `6d73d4a9aa101369b4e6e51a6df57847de0d7efb`

Compatibility test results for cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> 6d73d4a9aa101369b4e6e51a6df57847de0d7efb (PR)
Upgrade the nodes to version: 6d73d4a9aa101369b4e6e51a6df57847de0d7efb
framework_upgrade::framework-upgrade::full-framework-upgrade : 6788 TPS, 5695 ms latency, 7800 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> 6d73d4a9aa101369b4e6e51a6df57847de0d7efb passed
Test Ok

github-actions · 2023-03-01T19:31:43Z

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `6d73d4a9aa101369b4e6e51a6df57847de0d7efb`

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 6d73d4a9aa101369b4e6e51a6df57847de0d7efb (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7841 TPS, 4904 ms latency, 6800 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 6d73d4a9aa101369b4e6e51a6df57847de0d7efb
compatibility::simple-validator-upgrade::single-validator-upgrade : 5017 TPS, 7902 ms latency, 11300 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 6d73d4a9aa101369b4e6e51a6df57847de0d7efb
compatibility::simple-validator-upgrade::half-validator-upgrade : 5018 TPS, 8004 ms latency, 10800 ms p99 latency,no expired txns
4. upgrading second batch to new version: 6d73d4a9aa101369b4e6e51a6df57847de0d7efb
compatibility::simple-validator-upgrade::rest-validator-upgrade : 7008 TPS, 5593 ms latency, 9800 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 6d73d4a9aa101369b4e6e51a6df57847de0d7efb passed
Test Ok

zekun000 · 2023-03-02T01:53:46Z

aptos-move/framework/aptos-framework/sources/multisig_account.move

+        assert!(
+            num_rejections >= multisig_account_resource.num_signatures_required,
+            error::invalid_state(ENOT_ENOUGH_REJECTIONS),
+        );


this is the same as remove_executed_transaction? we just need to return (num_approvals, num_rejections)

That's true. Should be an easy refactor. I can do this in a followup PR

clay-aptos · 2023-03-21T16:31:20Z

@movekevin , we have published Al Noki's multisig v1 doc in:
#7308 (comment)

How can I help get references to v2 in the docs? I have notes from your preso and want to collaborate with you. Might need a distinct issue to track... Thanks!

alnoki · 2023-05-27T01:17:30Z

#8346 Adds CLI support

JackyWYX reviewed Dec 16, 2022

View reviewed changes

aptos-move/framework/aptos-framework/sources/multisig_account.move Outdated Show resolved Hide resolved

aptos-move/framework/aptos-framework/sources/multisig_account.move Outdated Show resolved Hide resolved

movekevin force-pushed the multisig branch 5 times, most recently from af1bb6a to 53248af Compare December 16, 2022 13:14

movekevin marked this pull request as ready for review December 16, 2022 13:30

movekevin requested review from davidiw and wrwg as code owners December 16, 2022 13:30

movekevin requested a review from JackyWYX December 18, 2022 01:44

davidiw reviewed Dec 18, 2022

View reviewed changes

aptos-move/framework/aptos-framework/sources/multisig_account.move Outdated Show resolved Hide resolved

aptos-move/framework/aptos-framework/sources/multisig_account.move Outdated Show resolved Hide resolved

movekevin force-pushed the multisig branch 4 times, most recently from e2a06e6 to 9ac9274 Compare December 18, 2022 18:36

movekevin requested review from davidiw, JackyWYX, 0xchloe, areshand, lightmark and CapCap and removed request for JackyWYX December 19, 2022 08:09

movekevin force-pushed the multisig branch 3 times, most recently from 4849e13 to 43bbc29 Compare December 21, 2022 10:03

LeviHHH reviewed Dec 22, 2022

View reviewed changes

LeviHHH reviewed Dec 23, 2022

View reviewed changes

aptos-move/framework/aptos-framework/sources/multisig_account.move Show resolved Hide resolved

lightmark reviewed Dec 26, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

davidiw reviewed Mar 1, 2023

View reviewed changes

Update status codes

6d73d4a

movekevin force-pushed the multisig branch from fc9b3fe to 6d73d4a Compare March 1, 2023 16:50

davidiw approved these changes Mar 1, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

movekevin merged commit 068cb0d into main Mar 1, 2023

movekevin deleted the multisig branch March 1, 2023 17:43

clay-aptos added the release-notes This tag indicates the associated change should be documented in the Aptos Release Notes. label Mar 1, 2023

This comment has been minimized.

Sign in to view

zekun000 reviewed Mar 2, 2023

View reviewed changes

alnoki mentioned this pull request Mar 21, 2023

Extend multisig tutorial/SDK with automation tool #7297

Closed

clay-aptos mentioned this pull request Mar 27, 2023

[Docs] Document use of scripts in Aptos #6318

Closed

alnoki mentioned this pull request May 28, 2023

[Bug][Aptos Framework][API][Transaction][VM] Multisig v2 FIFO invariant facilitates DoS attacks #8411

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Aptos Framework][API][Transaction][VM] Add a richer multisig account model #5894

[Aptos Framework][API][Transaction][VM] Add a richer multisig account model #5894

movekevin commented Dec 16, 2022 •

edited

Loading

davidiw left a comment

lightmark left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

davidiw Mar 1, 2023

movekevin Mar 1, 2023

davidiw Mar 1, 2023

movekevin Mar 1, 2023

davidiw Mar 1, 2023

movekevin Mar 1, 2023

davidiw Mar 1, 2023

movekevin Mar 1, 2023

davidiw left a comment

movekevin commented Mar 1, 2023

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.

clay-aptos commented Mar 1, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Mar 1, 2023

github-actions bot commented Mar 1, 2023

github-actions bot commented Mar 1, 2023

zekun000 Mar 2, 2023

movekevin Mar 2, 2023

clay-aptos commented Mar 21, 2023

alnoki commented May 27, 2023

[Aptos Framework][API][Transaction][VM] Add a richer multisig account model #5894

[Aptos Framework][API][Transaction][VM] Add a richer multisig account model #5894

Conversation

movekevin commented Dec 16, 2022 • edited Loading

Description

Test Plan

davidiw left a comment

Choose a reason for hiding this comment

lightmark left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

davidiw left a comment

Choose a reason for hiding this comment

movekevin commented Mar 1, 2023

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.

clay-aptos commented Mar 1, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Mar 1, 2023

✅ Forge suite land_blocking success on 6d73d4a9aa101369b4e6e51a6df57847de0d7efb

github-actions bot commented Mar 1, 2023

✅ Forge suite framework_upgrade success on cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> 6d73d4a9aa101369b4e6e51a6df57847de0d7efb

github-actions bot commented Mar 1, 2023

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 6d73d4a9aa101369b4e6e51a6df57847de0d7efb

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clay-aptos commented Mar 21, 2023

alnoki commented May 27, 2023

movekevin commented Dec 16, 2022 •

edited

Loading

✅ Forge suite `land_blocking` success on `6d73d4a9aa101369b4e6e51a6df57847de0d7efb`

✅ Forge suite `framework_upgrade` success on `cb4ba0a57c998c60cbab65af31a64875d2588ca5` ==> `6d73d4a9aa101369b4e6e51a6df57847de0d7efb`

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `6d73d4a9aa101369b4e6e51a6df57847de0d7efb`