From 738ab02fd11bba5e3527403cd7711b43dc64d47f Mon Sep 17 00:00:00 2001 From: asoliman Date: Wed, 14 Feb 2024 16:58:29 +0400 Subject: [PATCH 01/16] Multisig RFC - summary and implementation --- text/0057-stateful-multisig-pallet.md | 214 ++++++++++++++++++++++++++ 1 file changed, 214 insertions(+) create mode 100644 text/0057-stateful-multisig-pallet.md diff --git a/text/0057-stateful-multisig-pallet.md b/text/0057-stateful-multisig-pallet.md new file mode 100644 index 000000000..66fc40d58 --- /dev/null +++ b/text/0057-stateful-multisig-pallet.md @@ -0,0 +1,214 @@ +# RFC-0000: Enhanced Multisig Pallet + +| | | +| --------------- | ------------------------------------------------------------------------------------------- | +| **Start Date** | 13 February 2024 | +| **Description** | Add Enhanced Multisig Pallet to X (system) chain | +| **Authors** | Abdelrahman Soliman | + +## Summary + +A pallet to facilitate enhanced multisig accounts. The main enhancement is that we store a multisig account in the state with related info (owners, threshold,..etc). The module affords enhanced control over administrative operations such as adding/removing owners, changing the threshold, account deletion, canceling an existing proposal. Each owner can approve/revoke a proposal while still exists. The proposal is **not** intended for migrating or getting rid of existing multisig. It's to allow both options to coexist. + +For the rest of the RFC We use the following terms: + +* `proposal` to refer to an extrinsic that is to be dispatched from a multisig account after getting enough approvals. +* `Stateful Multisig` to refer to the proposed pallet. +* `Stateless Multi` to refer to the current multisig pallet in polkadot-sdk. + +## Motivation + +### Problem + +Entities in the Polkadot ecosystem need to have a way to manage their funds and other operations in a secure and efficient way. Multisig accounts are a common way to achieve this. Entities by definition change over time, members of the entity may change, threshold requirements may change, and the multisig account may need to be deleted. For even more enhanced hierarchical control, the multisig account may need to be controlled by other multisig accounts. + +Current native solutions for multisig operations are not efficient and lack functionality. + +#### Stateless Multisig + +We refer to current [multisig pallet in polkadot-sdk](https://github.com/paritytech/polkadot-sdk/tree/master/substrate/frame/multisig) because the multisig account is only created in the state when a proposal is made and is deleted after the proposal is executed or canceled. Although deriving the account is determinsitc it relies on exact users and thershold to derive it. This does not allow for control over the multisig account. It's also tightly coupled to exact users and threshold. This makes it hard for an organization to manage existing accounts and to change the threshold or add/remove owners. + +We believe as well that the stateless multisig is not efficient in terms of block footprint as we'll show in the performance section. + +#### Pure Proxy + +Pure proxy can acheive having the determinstic multisig account from different users but it's unneeded complexity as a way around the limitations of the current multisig pallet. + +### Requirements + +Basic requirements for the Stateful Multisig are: + +* The ability to have concrete and permanent (unless deleted) multisig accounts in the state. +* The ability to add/remove owners from an existing multisig account by the multisig itself. +* The ability to change the threshold of an existing multisig account by the multisig itself. +* The ability to delete an existing multisig account by the multisig itself. +* The ability to cancel an existing proposal by the multisig itself. +* Owners of multisig account can start a proposal on behalf of the multisig account which will be dispatched after getting enough approvals. +* Owners of multisig account can approve/revoke a proposal while still exists. + +### Use Cases + +* Corporate Governance: +In a corporate setting, multisig accounts can be employed for decision-making processes. For example, a company may require the approval of multiple executives to initiate significant financial transactions. + +* Joint Accounts: +Multisig accounts can be used for joint accounts where multiple individuals need to authorize transactions. This is particularly useful in family finances or shared business accounts. + +* Decentralized Autonomous Organizations (DAOs): +DAOs can utilize multisig accounts to ensure that decisions are made collectively. Multiple key holders can be required to approve changes to the organization's rules or the allocation of funds. + +## Stakeholders + +* Polkadot holders +* Polkadot developers + +## Explanation + +I've created the stateful multisig pallet during my studies in Polkadot Blockchain Academy under supervision from @shawntabrizi and @ank4n. After that, I've enhanced it to be fully functional and this is a draft [PR#3300](https://github.com/paritytech/polkadot-sdk/pull/3300) in polkadot-sdk. I'll list all the details and design decisions in the following sections. + +Let's start with a sequence diagram to illustrate the main operations of the Stateful Multisig. + +// TODO: +Put image here. + +Notes on above diagram: + +* It's a 3 step process to execute a proposal. (Start Proposal --> Approvals --> Execute Proposal) +* `Execute` is an explicit extrinsic for a simpler API. It can be optimized to be executed automatically after getting enough approvals. +* Any user can create a multisig account and they don't need to be part of it. (Alice in the diagram) +* A proposal is any extrinsic including control extrinsics (e.g. add/remove owner, change threshold,..etc). +* Any multisig account owner can start a proposal on behalf of the multisig account. (Bob in the diagram) +* Any multisig account owener can execute proposal if it's approved by enough owners. (Dave in the diagram) + +Detail-heavy explanation of the RFC, suitable for explanation to an implementer of the changeset. This should address corner cases in detail and provide justification behind decisions, and provide rationale for how the design meets the solution requirements. + +### State Transition Functions + +All functions have rustdoc in the code. Here is a brief overview of the functions: + +* `create_multisig` - Create a multisig account with a given threshold and initial owners. (Needs Deposit) +* `start_proposal` - Start a multisig proposal. (Needs Deposit) +* `approve` - Approve a multisig proposal. +* `revoke` - Revoke a multisig approval from an existing proposal. +* `execute_proposal` - Execute a multisig proposal. (Releases Deposit) +* `cancel_own_proposal` - Cancel a multisig proposal started by the caller in case no other owners approved it yet. (Releases Deposit) + +Note: Next functions need to be called from the multisig account itself. Deposits are reserved from the multisig account as well. + +* `add_owner` - Add a new owner to a multisig account. (Needs Deposit) +* `remove_owner` - Remove an owner from a multisig account. (Releases Deposit) +* `set_threshold` - Change the threshold of a multisig account. +* `cancel_proposal` - Cancel a multisig proposal. (Releases Deposit) +* `delete_multisig` - Delete a multisig account. (Releases Deposit) + +### Storage/State + +* Use 2 main storage maps to store mutlisig accounts and proposals. + +```rust +#[pallet::storage] + pub type MultisigAccount = StorageMap<_, Twox64Concat, T::AccountId, MultisigAccountDetails>; + +/// The set of open multisig proposals. A proposal is uniquely identified by the multisig account and the call hash. +/// (maybe a nonce as well in the future) +#[pallet::storage] +pub type PendingProposals = StorageDoubleMap< + _, + Twox64Concat, + T::AccountId, // Multisig Account + Blake2_128Concat, + T::Hash, // Call Hash + MultisigProposal, +>; +``` + +As for the values: + +```rust +pub struct MultisigAccountDetails { + /// The owners of the multisig account. This is a BoundedBTreeSet to ensure faster operations (add, remove). + /// As well as lookups and faster set operations to ensure approvers is always a subset from owners. (e.g. in case of removal of an owner during an active proposal) + pub owners: BoundedBTreeSet, + /// The threshold of approvers required for the multisig account to be able to execute a call. + pub threshold: u32, + pub creator: T::AccountId, + pub deposit: BalanceOf, +} +``` + +```rust +pub struct MultisigProposal { + /// Proposal creator. + pub creator: T::AccountId, + pub creation_deposit: BalanceOf, + /// The extrinsic when the multisig operation was opened. + pub when: Timepoint>, + /// The approvers achieved so far, including the depositor. + /// The approvers are stored in a BoundedBTreeSet to ensure faster lookup and operations (approve, revoke). + /// It's also bounded to ensure that the size don't go over the required limit by the Runtime. + pub approvers: BoundedBTreeSet, + /// The block number until which this multisig operation is valid. None means no expiry. + pub expire_after: Option>, +} +``` + +For optimization we're using BoundedBTreeSet to allow for efficient lookups and removals. Especially in the case of approvers, we need to be able to remove an approver from the list when they revoke their approval. (which we do lazily when `execute_proposal` is called) + +### Considerations / Edge cases + +* Removing an owner during an active proposal. + * Suggested approach: + We need to ensure that the approvers are always a subset from owners. This is also partially why we're using BoundedBTreeSet for owners and approvers. Once execute proposal is called we ensure that the proposal is still valid and the approvers are still a subset from current owners. + +* Multisig account deletion and cleaning up existing proposals. + * Suggested approach: +Once the last owner of a multisig account is removed or the multisig approved the account deletion we delete the multisig accound from the state and keep the proposals until someone calls `cleanup_proposals` multiple times which iterates over a max limit per extrinsic. This is to ensure we don't have unbounded iteration over the proposals. Users are already incentivized to call `cleanup_proposals` to get their deposits back. + +* Multisig account deletion and existing deposits: + * Suggested approach: + We currently just delete the account without checking for deposits (Would like to hear your thoughts here). We can either + * Don't make deposits to begin with and make it a fee. + * Transfer to treasury. + * Error on deletion. (don't like this) + +* Approving a proposal after the threshold is changed. + * Suggested approach: +We always use latest threshold and don't store each proposal with different threshold. This allows the following: + * In case threshold is lower than the number of approvers then the proposal is still valid. + * In case threshold is higher than the number of approvers then we catch it during execute proposal and error. + +## Drawbacks + +Description of recognized drawbacks to the approach given in the RFC. Non-exhaustively, drawbacks relating to performance, ergonomics, user experience, security, or privacy. + +## Testing, Security, and Privacy + +Describe the the impact of the proposal on these three high-importance areas - how implementations can be tested for adherence, effects that the proposal has on security and privacy per-se, as well as any possible implementation pitfalls which should be clearly avoided. + +## Performance, Ergonomics, and Compatibility + +Describe the impact of the proposal on the exposed functionality of Polkadot. + +### Performance + +Is this an optimization or a necessary pessimization? What steps have been taken to minimize additional overhead? + +### Ergonomics + +If the proposal alters exposed interfaces to developers or end-users, which types of usage patterns have been optimized for? + +### Compatibility + +Does this proposal break compatibility with existing interfaces, older versions of implementations? Summarize necessary migrations or upgrade strategies, if any. + +## Prior Art and References + +Provide references to either prior art or other relevant research for the submitted design. + +## Unresolved Questions + +Provide specific questions to discuss and address before the RFC is voted on by the Fellowship. This should include, for example, alternatives to aspects of the proposed design where the appropriate trade-off to make is unclear. + +## Future Directions and Related Material + +Describe future work which could be enabled by this RFC, if it were accepted, as well as related RFCs. This is a place to brain-dump and explore possibilities, which themselves may become their own RFCs. From d1a60e7d9b5e22ebe1c492b19b0ee4fda412a048 Mon Sep 17 00:00:00 2001 From: asoliman Date: Wed, 14 Feb 2024 20:47:05 +0400 Subject: [PATCH 02/16] Performance, Erognomics and Future directions --- text/0057-stateful-multisig-pallet.md | 88 ++++++++++++++++++++++++--- 1 file changed, 80 insertions(+), 8 deletions(-) diff --git a/text/0057-stateful-multisig-pallet.md b/text/0057-stateful-multisig-pallet.md index 66fc40d58..e9a9f787d 100644 --- a/text/0057-stateful-multisig-pallet.md +++ b/text/0057-stateful-multisig-pallet.md @@ -179,11 +179,11 @@ We always use latest threshold and don't store each proposal with different thre ## Drawbacks -Description of recognized drawbacks to the approach given in the RFC. Non-exhaustively, drawbacks relating to performance, ergonomics, user experience, security, or privacy. +* New pallet to maintain. ## Testing, Security, and Privacy -Describe the the impact of the proposal on these three high-importance areas - how implementations can be tested for adherence, effects that the proposal has on security and privacy per-se, as well as any possible implementation pitfalls which should be clearly avoided. +Standard audit/review requirements apply. ## Performance, Ergonomics, and Compatibility @@ -191,24 +191,96 @@ Describe the impact of the proposal on the exposed functionality of Polkadot. ### Performance -Is this an optimization or a necessary pessimization? What steps have been taken to minimize additional overhead? +Doing back of the envelop calculation to proof that the stateful multisig is more efficient than the stateless multisig given it's smaller footprint size on blocks. + +Quick review over the extrinsics for both as it affects the block size: + +Stateless Multisig: +Both `as_multi` and `approve_as_multi` has a similar parameters: + +```rust +origin: OriginFor, +threshold: u16, +other_signatories: Vec, +maybe_timepoint: Option>>, +call_hash: [u8; 32], +max_weight: Weight, +``` + +Stateful Multisig: +We have the following extrinsics: + +```rust +pub fn start_proposal( + origin: OriginFor, + multisig_account: T::AccountId, + call_hash: T::Hash, + ) +``` + +```rust +pub fn approve( + origin: OriginFor, + multisig_account: T::AccountId, + call_hash: T::Hash, + ) +``` + +```rust +pub fn execute_proposal( + origin: OriginFor, + multisig_account: T::AccountId, + call: Box<::RuntimeCall>, + ) +``` + +The main takeway is that we don't need to pass the threshold and other signatories in the extrinsics. This is because we already have the threshold and signatories in the state (only once). + +So now for the caclulation + +Given the following: + +* K is the number of multisig accounts. +* N is number of owners in each multisig account. +* For each proposal we need to have 2N/3 approvals. + +The table calculates if everyday each of the K multisig accounts has one proposal and it gets approved by the 2N/3 and then executed. How much did the total footprint on Blocks and States increased by the end of the day. + +Note: We're not calculating the cost of proposal as both in statefull and stateless multisig they're almost the same and gets cleaned up from the state once the proposal is executed or canceled. + +Stateless effect on blocksizes = 2/3*K*N^2 (as each user of the 2/3 users will need to call approve_as_multi with all the other signatories(N) in extrinsic body) + +Stateful effect on blocksizes = K * N (as each user will need to call approve with the multisig account only in extrinsic body) + +Stateless effect on statesizes = Nil (as the multisig account is not stored in the state) + +Stateful effect on statesizes = K*N (as each multisig account (K) will be stored with all the owners (K) in the state) + +| Pallet | Block Size | State Size | +|----------------|:-------------:|-----------:| +| Stateless | 2/3*K*N^2 | Nil | +| Stateful | K*N | K*N | ### Ergonomics -If the proposal alters exposed interfaces to developers or end-users, which types of usage patterns have been optimized for? +The Stateful Multisig will have better ergonomics for managing multisig accounts for both developers and end-users. ### Compatibility -Does this proposal break compatibility with existing interfaces, older versions of implementations? Summarize necessary migrations or upgrade strategies, if any. +This RFC is compatible with the existing implementation and can be handled via upgrades and migration. It's not intended to replace the existing multisig pallet. ## Prior Art and References -Provide references to either prior art or other relevant research for the submitted design. +[multisig pallet in polkadot-sdk](https://github.com/paritytech/polkadot-sdk/tree/master/substrate/frame/multisig) ## Unresolved Questions -Provide specific questions to discuss and address before the RFC is voted on by the Fellowship. This should include, for example, alternatives to aspects of the proposed design where the appropriate trade-off to make is unclear. +* On account deletion, should we transfer remaining deposits to treasury or remove account creation deposits completely and consider it as fees to start with? ## Future Directions and Related Material -Describe future work which could be enabled by this RFC, if it were accepted, as well as related RFCs. This is a place to brain-dump and explore possibilities, which themselves may become their own RFCs. +* [ ] Batch proposals. The ability to batch multiple calls into one proposal. +* [ ] Batch addition/removal of owners. +* [ ] Add expiry to proposals. After a certain time, proposals will not accept any more approvals or executions and will be deleted. +* [ ] Add extra identifier other than call_hash to proposals (e.g. nonce). This will allow same call to be proposed multiple times and be in pending state. +* [ ] Implement call filters. This will allow multisig accounts to only accept certain calls. From be71d1ed321a6594b0d0d6af7309a95e8c123cda Mon Sep 17 00:00:00 2001 From: "Abdelrahman Soliman (Boda)" Date: Wed, 14 Feb 2024 20:49:58 +0400 Subject: [PATCH 03/16] Add sequence diagram --- text/0057-stateful-multisig-pallet.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/text/0057-stateful-multisig-pallet.md b/text/0057-stateful-multisig-pallet.md index e9a9f787d..2541d9b4e 100644 --- a/text/0057-stateful-multisig-pallet.md +++ b/text/0057-stateful-multisig-pallet.md @@ -1,9 +1,9 @@ -# RFC-0000: Enhanced Multisig Pallet +# RFC-0000: Stateful Multisig Pallet | | | | --------------- | ------------------------------------------------------------------------------------------- | -| **Start Date** | 13 February 2024 | -| **Description** | Add Enhanced Multisig Pallet to X (system) chain | +| **Start Date** | 14 February 2024 | +| **Description** | Add Enhanced Multisig Pallet to X (system) chain | | **Authors** | Abdelrahman Soliman | ## Summary @@ -68,8 +68,8 @@ I've created the stateful multisig pallet during my studies in Polkadot Blockcha Let's start with a sequence diagram to illustrate the main operations of the Stateful Multisig. -// TODO: -Put image here. +![multisig operations](https://github.com/asoliman92/RFCs/assets/2677789/4f2e8972-f3b8-4250-b75f-1e4788b35752) + Notes on above diagram: From 0511eb15cdceedcac7ec89c21eb8f64f0ad6b426 Mon Sep 17 00:00:00 2001 From: "Abdelrahman Soliman (Boda)" Date: Wed, 14 Feb 2024 20:57:18 +0400 Subject: [PATCH 04/16] Update 0057-stateful-multisig-pallet.md --- text/0057-stateful-multisig-pallet.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0057-stateful-multisig-pallet.md b/text/0057-stateful-multisig-pallet.md index 2541d9b4e..e456c7251 100644 --- a/text/0057-stateful-multisig-pallet.md +++ b/text/0057-stateful-multisig-pallet.md @@ -4,7 +4,7 @@ | --------------- | ------------------------------------------------------------------------------------------- | | **Start Date** | 14 February 2024 | | **Description** | Add Enhanced Multisig Pallet to X (system) chain | -| **Authors** | Abdelrahman Soliman | +| **Authors** | Abdelrahman Soliman (Boda) | ## Summary From 8e1fdb3818e3326092698afd575712b8e321e80a Mon Sep 17 00:00:00 2001 From: asoliman Date: Wed, 14 Feb 2024 21:21:22 +0400 Subject: [PATCH 05/16] clean ups --- text/0057-stateful-multisig-pallet.md | 63 +++++++++++++++------------ 1 file changed, 35 insertions(+), 28 deletions(-) diff --git a/text/0057-stateful-multisig-pallet.md b/text/0057-stateful-multisig-pallet.md index e456c7251..2bd54ba11 100644 --- a/text/0057-stateful-multisig-pallet.md +++ b/text/0057-stateful-multisig-pallet.md @@ -26,13 +26,13 @@ Current native solutions for multisig operations are not efficient and lack func #### Stateless Multisig -We refer to current [multisig pallet in polkadot-sdk](https://github.com/paritytech/polkadot-sdk/tree/master/substrate/frame/multisig) because the multisig account is only created in the state when a proposal is made and is deleted after the proposal is executed or canceled. Although deriving the account is determinsitc it relies on exact users and thershold to derive it. This does not allow for control over the multisig account. It's also tightly coupled to exact users and threshold. This makes it hard for an organization to manage existing accounts and to change the threshold or add/remove owners. +We refer to current [multisig pallet in polkadot-sdk](https://github.com/paritytech/polkadot-sdk/tree/master/substrate/frame/multisig) because the multisig account is only derived and not stored in the state. Although deriving the account is determinsitc as it relies on exact users (sorted) and thershold to derive it. This does not allow for control over the multisig account. It's also tightly coupled to exact users and threshold. This makes it hard for an organization to manage existing accounts and to change the threshold or add/remove owners. We believe as well that the stateless multisig is not efficient in terms of block footprint as we'll show in the performance section. #### Pure Proxy -Pure proxy can acheive having the determinstic multisig account from different users but it's unneeded complexity as a way around the limitations of the current multisig pallet. +Pure proxy can achieve having a stored and determinstic multisig account from different users but it's unneeded complexity as a way around the limitations of the current multisig pallet. It doesn't also have the same fine grained control over the multisig account. ### Requirements @@ -57,6 +57,8 @@ Multisig accounts can be used for joint accounts where multiple individuals need * Decentralized Autonomous Organizations (DAOs): DAOs can utilize multisig accounts to ensure that decisions are made collectively. Multiple key holders can be required to approve changes to the organization's rules or the allocation of funds. +and much more... + ## Stakeholders * Polkadot holders @@ -70,7 +72,6 @@ Let's start with a sequence diagram to illustrate the main operations of the Sta ![multisig operations](https://github.com/asoliman92/RFCs/assets/2677789/4f2e8972-f3b8-4250-b75f-1e4788b35752) - Notes on above diagram: * It's a 3 step process to execute a proposal. (Start Proposal --> Approvals --> Execute Proposal) @@ -80,11 +81,9 @@ Notes on above diagram: * Any multisig account owner can start a proposal on behalf of the multisig account. (Bob in the diagram) * Any multisig account owener can execute proposal if it's approved by enough owners. (Dave in the diagram) -Detail-heavy explanation of the RFC, suitable for explanation to an implementer of the changeset. This should address corner cases in detail and provide justification behind decisions, and provide rationale for how the design meets the solution requirements. - ### State Transition Functions -All functions have rustdoc in the code. Here is a brief overview of the functions: +All functions have detailed rustdoc in [PR#3300](https://github.com/paritytech/polkadot-sdk/pull/3300). Here is a brief overview of the functions: * `create_multisig` - Create a multisig account with a given threshold and initial owners. (Needs Deposit) * `start_proposal` - Start a multisig proposal. (Needs Deposit) @@ -152,30 +151,34 @@ pub struct MultisigProposal { } ``` -For optimization we're using BoundedBTreeSet to allow for efficient lookups and removals. Especially in the case of approvers, we need to be able to remove an approver from the list when they revoke their approval. (which we do lazily when `execute_proposal` is called) +For optimization we're using BoundedBTreeSet to allow for efficient lookups and removals. Especially in the case of approvers, we need to be able to remove an approver from the list when they revoke their approval. (which we do lazily when `execute_proposal` is called). + +There's an extra storage map for the deposits of the multisig accounts per owner added. This is to ensure that we can release the deposits when the multisig removes them even if the constant deposit per owner changed in the runtime later on. -### Considerations / Edge cases +### Considerations & Edge cases + +#### Removing an owner from the multisig account during an active proposal -* Removing an owner during an active proposal. - * Suggested approach: We need to ensure that the approvers are always a subset from owners. This is also partially why we're using BoundedBTreeSet for owners and approvers. Once execute proposal is called we ensure that the proposal is still valid and the approvers are still a subset from current owners. -* Multisig account deletion and cleaning up existing proposals. - * Suggested approach: +#### Multisig account deletion and cleaning up existing proposals + Once the last owner of a multisig account is removed or the multisig approved the account deletion we delete the multisig accound from the state and keep the proposals until someone calls `cleanup_proposals` multiple times which iterates over a max limit per extrinsic. This is to ensure we don't have unbounded iteration over the proposals. Users are already incentivized to call `cleanup_proposals` to get their deposits back. -* Multisig account deletion and existing deposits: - * Suggested approach: - We currently just delete the account without checking for deposits (Would like to hear your thoughts here). We can either - * Don't make deposits to begin with and make it a fee. - * Transfer to treasury. - * Error on deletion. (don't like this) +#### Multisig account deletion and existing deposits + +We currently just delete the account without checking for deposits (Would like to hear your thoughts here). We can either + +* Don't make deposits to begin with and make it a fee. +* Transfer to treasury. +* Error on deletion. (don't like this) + +#### Approving a proposal after the threshold is changed -* Approving a proposal after the threshold is changed. - * Suggested approach: We always use latest threshold and don't store each proposal with different threshold. This allows the following: - * In case threshold is lower than the number of approvers then the proposal is still valid. - * In case threshold is higher than the number of approvers then we catch it during execute proposal and error. + +* In case threshold is lower than the number of approvers then the proposal is still valid. +* In case threshold is higher than the number of approvers then we catch it during execute proposal and error. ## Drawbacks @@ -187,8 +190,6 @@ Standard audit/review requirements apply. ## Performance, Ergonomics, and Compatibility -Describe the impact of the proposal on the exposed functionality of Polkadot. - ### Performance Doing back of the envelop calculation to proof that the stateful multisig is more efficient than the stateless multisig given it's smaller footprint size on blocks. @@ -236,15 +237,13 @@ pub fn execute_proposal( The main takeway is that we don't need to pass the threshold and other signatories in the extrinsics. This is because we already have the threshold and signatories in the state (only once). -So now for the caclulation - -Given the following: +So now for the caclulations, given the following: * K is the number of multisig accounts. * N is number of owners in each multisig account. * For each proposal we need to have 2N/3 approvals. -The table calculates if everyday each of the K multisig accounts has one proposal and it gets approved by the 2N/3 and then executed. How much did the total footprint on Blocks and States increased by the end of the day. +The table calculates if each of the K multisig accounts has one proposal and it gets approved by the 2N/3 and then executed. How much did the total Blocks and States sizes increased by the end of the day. Note: We're not calculating the cost of proposal as both in statefull and stateless multisig they're almost the same and gets cleaned up from the state once the proposal is executed or canceled. @@ -261,6 +260,14 @@ Stateful effect on statesizes = K*N (as each multisig account (K) will be stored | Stateless | 2/3*K*N^2 | Nil | | Stateful | K*N | K*N | +Simplified table removing K from the equation: +| Pallet | Block Size | State Size | +|----------------|:-------------:|-----------:| +| Stateless | N^2 | Nil | +| Stateful | N | N | + +So even though the stateful multisig has a larger state size, it's still more efficient in terms of block size and total footprint on the blockchain. + ### Ergonomics The Stateful Multisig will have better ergonomics for managing multisig accounts for both developers and end-users. From 6df6b8bfc21ab927d4d2ee4b3b5fde6e2c95c73c Mon Sep 17 00:00:00 2001 From: asoliman Date: Thu, 15 Feb 2024 16:36:40 +0400 Subject: [PATCH 06/16] Clean ups --- text/0057-stateful-multisig-pallet.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/text/0057-stateful-multisig-pallet.md b/text/0057-stateful-multisig-pallet.md index 2bd54ba11..a3e6ec268 100644 --- a/text/0057-stateful-multisig-pallet.md +++ b/text/0057-stateful-multisig-pallet.md @@ -2,8 +2,8 @@ | | | | --------------- | ------------------------------------------------------------------------------------------- | -| **Start Date** | 14 February 2024 | -| **Description** | Add Enhanced Multisig Pallet to X (system) chain | +| **Start Date** | 15 February 2024 | +| **Description** | Add Enhanced Multisig Pallet | | **Authors** | Abdelrahman Soliman (Boda) | ## Summary @@ -14,7 +14,7 @@ For the rest of the RFC We use the following terms: * `proposal` to refer to an extrinsic that is to be dispatched from a multisig account after getting enough approvals. * `Stateful Multisig` to refer to the proposed pallet. -* `Stateless Multi` to refer to the current multisig pallet in polkadot-sdk. +* `Stateless Multisig` to refer to the current multisig pallet in polkadot-sdk. ## Motivation @@ -22,7 +22,7 @@ For the rest of the RFC We use the following terms: Entities in the Polkadot ecosystem need to have a way to manage their funds and other operations in a secure and efficient way. Multisig accounts are a common way to achieve this. Entities by definition change over time, members of the entity may change, threshold requirements may change, and the multisig account may need to be deleted. For even more enhanced hierarchical control, the multisig account may need to be controlled by other multisig accounts. -Current native solutions for multisig operations are not efficient and lack functionality. +Current native solutions for multisig operations are less optimal, performance-wise (as we'll explain later in the RFC), and lack fine-grained control over the multisig account. #### Stateless Multisig @@ -283,6 +283,7 @@ This RFC is compatible with the existing implementation and can be handled via u ## Unresolved Questions * On account deletion, should we transfer remaining deposits to treasury or remove account creation deposits completely and consider it as fees to start with? +* Which chain should we put this on? I suggest collectives like current multisig ## Future Directions and Related Material From 0901367cfe255aea425d49a156c6b708722ff367 Mon Sep 17 00:00:00 2001 From: asoliman Date: Thu, 15 Feb 2024 16:46:56 +0400 Subject: [PATCH 07/16] Add PR number --- text/0057-stateful-multisig-pallet.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0057-stateful-multisig-pallet.md b/text/0057-stateful-multisig-pallet.md index a3e6ec268..79a8aede0 100644 --- a/text/0057-stateful-multisig-pallet.md +++ b/text/0057-stateful-multisig-pallet.md @@ -1,4 +1,4 @@ -# RFC-0000: Stateful Multisig Pallet +# RFC-0074: Stateful Multisig Pallet | | | | --------------- | ------------------------------------------------------------------------------------------- | From 4c8f8e0276cbb547afc2b63a028eadf243612ddf Mon Sep 17 00:00:00 2001 From: asoliman Date: Thu, 15 Feb 2024 16:47:30 +0400 Subject: [PATCH 08/16] Use PR number for both the RFC and Subject --- ...ateful-multisig-pallet.md => 0074-stateful-multisig-pallet.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename text/{0057-stateful-multisig-pallet.md => 0074-stateful-multisig-pallet.md} (100%) diff --git a/text/0057-stateful-multisig-pallet.md b/text/0074-stateful-multisig-pallet.md similarity index 100% rename from text/0057-stateful-multisig-pallet.md rename to text/0074-stateful-multisig-pallet.md From e83f04c513a0615dd4c6bfedf5698e4faa8d6d90 Mon Sep 17 00:00:00 2001 From: asoliman Date: Thu, 15 Feb 2024 16:55:52 +0400 Subject: [PATCH 09/16] fix typo --- text/0074-stateful-multisig-pallet.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0074-stateful-multisig-pallet.md b/text/0074-stateful-multisig-pallet.md index 79a8aede0..4e47f5af6 100644 --- a/text/0074-stateful-multisig-pallet.md +++ b/text/0074-stateful-multisig-pallet.md @@ -282,7 +282,7 @@ This RFC is compatible with the existing implementation and can be handled via u ## Unresolved Questions -* On account deletion, should we transfer remaining deposits to treasury or remove account creation deposits completely and consider it as fees to start with? +* On account deletion, should we transfer remaining deposits to treasury or remove owners' addition deposits completely and consider it as fees to start with? * Which chain should we put this on? I suggest collectives like current multisig ## Future Directions and Related Material From 7d89c188107185389ae11284d1dcbfa13efffeb5 Mon Sep 17 00:00:00 2001 From: asoliman Date: Thu, 15 Feb 2024 17:27:53 +0400 Subject: [PATCH 10/16] Add Collective chain in the subject --- text/0074-stateful-multisig-pallet.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/text/0074-stateful-multisig-pallet.md b/text/0074-stateful-multisig-pallet.md index 4e47f5af6..247c36677 100644 --- a/text/0074-stateful-multisig-pallet.md +++ b/text/0074-stateful-multisig-pallet.md @@ -3,7 +3,7 @@ | | | | --------------- | ------------------------------------------------------------------------------------------- | | **Start Date** | 15 February 2024 | -| **Description** | Add Enhanced Multisig Pallet | +| **Description** | Add Enhanced Multisig Pallet to Collectives chain | | **Authors** | Abdelrahman Soliman (Boda) | ## Summary @@ -283,7 +283,6 @@ This RFC is compatible with the existing implementation and can be handled via u ## Unresolved Questions * On account deletion, should we transfer remaining deposits to treasury or remove owners' addition deposits completely and consider it as fees to start with? -* Which chain should we put this on? I suggest collectives like current multisig ## Future Directions and Related Material From f72feb142819d8e8f98620534516f4daad633581 Mon Sep 17 00:00:00 2001 From: asoliman Date: Fri, 16 Feb 2024 13:56:41 +0400 Subject: [PATCH 11/16] Add extrinsics documentation and change design to include rejectors as well --- text/0074-stateful-multisig-pallet.md | 348 ++++++++++++++++++++++++-- 1 file changed, 321 insertions(+), 27 deletions(-) diff --git a/text/0074-stateful-multisig-pallet.md b/text/0074-stateful-multisig-pallet.md index 247c36677..d26754177 100644 --- a/text/0074-stateful-multisig-pallet.md +++ b/text/0074-stateful-multisig-pallet.md @@ -8,7 +8,7 @@ ## Summary -A pallet to facilitate enhanced multisig accounts. The main enhancement is that we store a multisig account in the state with related info (owners, threshold,..etc). The module affords enhanced control over administrative operations such as adding/removing owners, changing the threshold, account deletion, canceling an existing proposal. Each owner can approve/revoke a proposal while still exists. The proposal is **not** intended for migrating or getting rid of existing multisig. It's to allow both options to coexist. +A pallet to facilitate enhanced multisig accounts. The main enhancement is that we store a multisig account in the state with related info (signers, threshold,..etc). The module affords enhanced control over administrative operations such as adding/removing signers, changing the threshold, account deletion, canceling an existing proposal. Each signer can approve/reject a proposal while still exists. The proposal is **not** intended for migrating or getting rid of existing multisig. It's to allow both options to coexist. For the rest of the RFC We use the following terms: @@ -26,7 +26,7 @@ Current native solutions for multisig operations are less optimal, performance-w #### Stateless Multisig -We refer to current [multisig pallet in polkadot-sdk](https://github.com/paritytech/polkadot-sdk/tree/master/substrate/frame/multisig) because the multisig account is only derived and not stored in the state. Although deriving the account is determinsitc as it relies on exact users (sorted) and thershold to derive it. This does not allow for control over the multisig account. It's also tightly coupled to exact users and threshold. This makes it hard for an organization to manage existing accounts and to change the threshold or add/remove owners. +We refer to current [multisig pallet in polkadot-sdk](https://github.com/paritytech/polkadot-sdk/tree/master/substrate/frame/multisig) because the multisig account is only derived and not stored in the state. Although deriving the account is determinsitc as it relies on exact users (sorted) and thershold to derive it. This does not allow for control over the multisig account. It's also tightly coupled to exact users and threshold. This makes it hard for an organization to manage existing accounts and to change the threshold or add/remove signers. We believe as well that the stateless multisig is not efficient in terms of block footprint as we'll show in the performance section. @@ -39,12 +39,12 @@ Pure proxy can achieve having a stored and determinstic multisig account from di Basic requirements for the Stateful Multisig are: * The ability to have concrete and permanent (unless deleted) multisig accounts in the state. -* The ability to add/remove owners from an existing multisig account by the multisig itself. +* The ability to add/remove signers from an existing multisig account by the multisig itself. * The ability to change the threshold of an existing multisig account by the multisig itself. * The ability to delete an existing multisig account by the multisig itself. * The ability to cancel an existing proposal by the multisig itself. -* Owners of multisig account can start a proposal on behalf of the multisig account which will be dispatched after getting enough approvals. -* Owners of multisig account can approve/revoke a proposal while still exists. +* Signers of multisig account can start a proposal on behalf of the multisig account which will be dispatched after getting enough approvals. +* Signers of multisig account can approve/reject a proposal while still exists. ### Use Cases @@ -77,29 +77,319 @@ Notes on above diagram: * It's a 3 step process to execute a proposal. (Start Proposal --> Approvals --> Execute Proposal) * `Execute` is an explicit extrinsic for a simpler API. It can be optimized to be executed automatically after getting enough approvals. * Any user can create a multisig account and they don't need to be part of it. (Alice in the diagram) -* A proposal is any extrinsic including control extrinsics (e.g. add/remove owner, change threshold,..etc). -* Any multisig account owner can start a proposal on behalf of the multisig account. (Bob in the diagram) -* Any multisig account owener can execute proposal if it's approved by enough owners. (Dave in the diagram) +* A proposal is any extrinsic including control extrinsics (e.g. add/remove signer, change threshold,..etc). +* Any multisig account signer can start a proposal on behalf of the multisig account. (Bob in the diagram) +* Any multisig account owener can execute proposal if it's approved by enough signers. (Dave in the diagram) ### State Transition Functions All functions have detailed rustdoc in [PR#3300](https://github.com/paritytech/polkadot-sdk/pull/3300). Here is a brief overview of the functions: -* `create_multisig` - Create a multisig account with a given threshold and initial owners. (Needs Deposit) +* `create_multisig` - Create a multisig account with a given threshold and initial signers. (Needs Deposit) + +```rust + /// Creates a new multisig account and attach signers with a threshold to it. + /// + /// The dispatch origin for this call must be _Signed_. It is expected to be a nomral AccountId and not a + /// Multisig AccountId. + /// + /// T::BaseCreationDeposit + T::PerSignerDeposit * signers.len() will be held from the caller's account. + /// + /// # Arguments + /// + /// - `signers`: Initial set of accounts to add to the multisig. These may be updated later via `add_signer` + /// and `remove_signer`. + /// - `threshold`: The threshold number of accounts required to approve an action. Must be greater than 0 and + /// less than or equal to the total number of signers. + /// + /// # Errors + /// + /// * `TooManySignatories` - The number of signatories exceeds the maximum allowed. + /// * `InvalidThreshold` - The threshold is greater than the total number of signers. + pub fn create_multisig( + origin: OriginFor, + signers: BoundedBTreeSet, + threshold: u32, + ) -> DispatchResult +``` + * `start_proposal` - Start a multisig proposal. (Needs Deposit) + +```rust + /// Starts a new proposal for a dispatchable call for a multisig account. + /// The caller must be one of the signers of the multisig account. + /// T::ProposalDeposit will be held from the caller's account. + /// + /// # Arguments + /// + /// * `multisig_account` - The multisig account ID. + /// * `call` - The dispatchable call to be executed. + /// + /// # Errors + /// + /// * `MultisigNotFound` - The multisig account does not exist. + /// * `UnAuthorizedSigner` - The caller is not an signer of the multisig account. + /// * `TooManySignatories` - The number of signatories exceeds the maximum allowed. (shouldn't really happen as it's the first approval) + pub fn start_proposal( + origin: OriginFor, + multisig_account: T::AccountId, + call_hash: T::Hash, + ) -> DispatchResult +``` + * `approve` - Approve a multisig proposal. -* `revoke` - Revoke a multisig approval from an existing proposal. + +```rust + /// Approves a proposal for a dispatchable call for a multisig account. + /// The caller must be one of the signers of the multisig account. + /// + /// If a signer did approve -> reject -> approve, the proposal will be approved. + /// If a signer did approve -> reject, the proposal will be rejected. + /// + /// # Arguments + /// + /// * `multisig_account` - The multisig account ID. + /// * `call_hash` - The hash of the call to be approved. (This will be the hash of the call that was used in `start_proposal`) + /// + /// # Errors + /// + /// * `MultisigNotFound` - The multisig account does not exist. + /// * `UnAuthorizedSigner` - The caller is not an signer of the multisig account. + /// * `TooManySignatories` - The number of signatories exceeds the maximum allowed. + /// This shouldn't really happen as it's an approval, not an addition of a new signer. + pub fn approve( + origin: OriginFor, + multisig_account: T::AccountId, + call_hash: T::Hash, + ) -> DispatchResult +``` + +* `reject` - Reject a multisig proposal. + +```rust + /// Rejects a proposal for a multisig account. + /// The caller must be one of the signers of the multisig account. + /// + /// Between approving and rejecting, last call wins. + /// If a signer did approve -> reject -> approve, the proposal will be approved. + /// If a signer did approve -> reject, the proposal will be rejected. + /// + /// # Arguments + /// + /// * `multisig_account` - The multisig account ID. + /// * `call_hash` - The hash of the call to be approved. (This will be the hash of the call that was used in `start_proposal`) + /// + /// # Errors + /// + /// * `MultisigNotFound` - The multisig account does not exist. + /// * `UnAuthorizedSigner` - The caller is not an signer of the multisig account. + /// * `SignerNotFound` - The caller has not approved the proposal. + #[pallet::call_index(3)] + #[pallet::weight(Weight::default())] + pub fn reject( + origin: OriginFor, + multisig_account: T::AccountId, + call_hash: T::Hash, + ) -> DispatchResult +``` + * `execute_proposal` - Execute a multisig proposal. (Releases Deposit) -* `cancel_own_proposal` - Cancel a multisig proposal started by the caller in case no other owners approved it yet. (Releases Deposit) + +```rust + /// Executes a proposal for a dispatchable call for a multisig account. + /// Poropsal needs to be approved by enough signers (exceeds or equal multisig threshold) before it can be executed. + /// The caller must be one of the signers of the multisig account. + /// + /// This function does an extra check to make sure that all approvers still exist in the multisig account. + /// That is to make sure that the multisig account is not compromised by removing an signer during an active proposal. + /// + /// Once finished, the withheld deposit will be returned to the proposal creator. + /// + /// # Arguments + /// + /// * `multisig_account` - The multisig account ID. + /// * `call` - The call to be executed. + /// + /// # Errors + /// + /// * `MultisigNotFound` - The multisig account does not exist. + /// * `UnAuthorizedSigner` - The caller is not an signer of the multisig account. + /// * `NotEnoughApprovers` - approvers don't exceed the threshold. + pub fn execute_proposal( + origin: OriginFor, + multisig_account: T::AccountId, + call: Box<::RuntimeCall>, + ) -> DispatchResult +``` + +* `cancel_proposal` - Cancel a multisig proposal. (Releases Deposit) + +```rust + /// Cancels an existing proposal for a multisig account. + /// Poropsal needs to be rejected by enough signers (exceeds or equal multisig threshold) before it can be executed. + /// The caller must be one of the signers of the multisig account. + /// + /// This function does an extra check to make sure that all rejectors still exist in the multisig account. + /// That is to make sure that the multisig account is not compromised by removing an signer during an active proposal. + /// + /// Once finished, the withheld deposit will be returned to the proposal creator./ + /// + /// # Arguments + /// + /// * `origin` - The origin multisig account who wants to cancel the proposal. + /// * `call_hash` - The hash of the call to be canceled. (This will be the hash of the call that was used in `start_proposal`) + /// + /// # Errors + /// + /// * `MultisigNotFound` - The multisig account does not exist. + /// * `ProposalNotFound` - The proposal does not exist. + pub fn cancel_proposal( + origin: OriginFor, + multisig_account: T::AccountId, + call_hash: T::Hash) -> DispatchResult +``` + +* `cancel_own_proposal` - Cancel a multisig proposal started by the caller in case no other signers approved it yet. (Releases Deposit) + +```rust + /// Cancels an existing proposal for a multisig account Only if the proposal doesn't have approvers other than + /// the proposer. + /// + /// This function needs to be called from a the proposer of the proposal as the origin. + /// + /// The withheld deposit will be returned to the proposal creator. + /// + /// # Arguments + /// + /// * `multisig_account` - The multisig account ID. + /// * `call_hash` - The hash of the call to be canceled. (This will be the hash of the call that was used in `start_proposal`) + /// + /// # Errors + /// + /// * `MultisigNotFound` - The multisig account does not exist. + /// * `ProposalNotFound` - The proposal does not exist. + pub fn cancel_own_proposal( + origin: OriginFor, + multisig_account: T::AccountId, + call_hash: T::Hash, + ) -> DispatchResult +``` + +* `cleanup_proposals` - Cleanup proposals of a multisig account. (Releases Deposit) + +```rust + /// Cleanup proposals of a multisig account. This function will iterate over a max limit per extrinsic to ensure + /// we don't have unbounded iteration over the proposals. + /// + /// The withheld deposit will be returned to the proposal creator. + /// + /// # Arguments + /// + /// * `multisig_account` - The multisig account ID. + /// + /// # Errors + /// + /// * `MultisigNotFound` - The multisig account does not exist. + /// * `ProposalNotFound` - The proposal does not exist. + pub fn cleanup_proposals( + origin: OriginFor, + multisig_account: T::AccountId, + ) -> DispatchResult +``` Note: Next functions need to be called from the multisig account itself. Deposits are reserved from the multisig account as well. -* `add_owner` - Add a new owner to a multisig account. (Needs Deposit) -* `remove_owner` - Remove an owner from a multisig account. (Releases Deposit) +* `add_signer` - Add a new signer to a multisig account. (Needs Deposit) + +```rust + /// Adds a new signer to the multisig account. + /// This function needs to be called from a Multisig account as the origin. + /// Otherwise it will fail with MultisigNotFound error. + /// + /// T::PerSignerDeposit will be held from the multisig account. + /// + /// # Arguments + /// + /// * `origin` - The origin multisig account who wants to add a new signer to the multisig account. + /// * `new_signer` - The AccountId of the new signer to be added. + /// * `new_threshold` - The new threshold for the multisig account after adding the new signer. + /// + /// # Errors + /// * `MultisigNotFound` - The multisig account does not exist. + /// * `InvalidThreshold` - The threshold is greater than the total number of signers or is zero. + /// * `TooManySignatories` - The number of signatories exceeds the maximum allowed. + pub fn add_signer( + origin: OriginFor, + new_signer: T::AccountId, + new_threshold: u32, + ) -> DispatchResult +``` + +* `remove_signer` - Remove an signer from a multisig account. (Releases Deposit) + +```rust + /// Removes an signer from the multisig account. + /// This function needs to be called from a Multisig account as the origin. + /// Otherwise it will fail with MultisigNotFound error. + /// If only one signer exists and is removed, the multisig account and any pending proposals for this account will be deleted from the state. + /// + /// # Arguments + /// + /// * `origin` - The origin multisig account who wants to remove an signer from the multisig account. + /// * `signer_to_remove` - The AccountId of the signer to be removed. + /// * `new_threshold` - The new threshold for the multisig account after removing the signer. Accepts zero if + /// the signer is the only one left.kkk + /// + /// # Errors + /// + /// This function can return the following errors: + /// + /// * `MultisigNotFound` - The multisig account does not exist. + /// * `InvalidThreshold` - The new threshold is greater than the total number of signers or is zero. + /// * `UnAuthorizedSigner` - The caller is not an signer of the multisig account. + pub fn remove_signer( + origin: OriginFor, + signer_to_remove: T::AccountId, + new_threshold: u32, + ) -> DispatchResult +``` + * `set_threshold` - Change the threshold of a multisig account. -* `cancel_proposal` - Cancel a multisig proposal. (Releases Deposit) + +```rust + /// Sets a new threshold for a multisig account. + /// This function needs to be called from a Multisig account as the origin. + /// Otherwise it will fail with MultisigNotFound error. + /// + /// # Arguments + /// + /// * `origin` - The origin multisig account who wants to set the new threshold. + /// * `new_threshold` - The new threshold to be set. + /// # Errors + /// + /// * `MultisigNotFound` - The multisig account does not exist. + /// * `InvalidThreshold` - The new threshold is greater than the total number of signers or is zero. + set_threshold(origin: OriginFor, new_threshold: u32) -> DispatchResult +``` + * `delete_multisig` - Delete a multisig account. (Releases Deposit) +```rust + /// Deletes a multisig account and all related proposals. + /// + /// This function needs to be called from a Multisig account as the origin. + /// Otherwise it will fail with MultisigNotFound error. + /// + /// # Arguments + /// + /// * `origin` - The origin multisig account who wants to cancel the proposal. + /// + /// # Errors + /// + /// * `MultisigNotFound` - The multisig account does not exist. + pub fn delete_account(origin: OriginFor) -> DispatchResult +``` + ### Storage/State * Use 2 main storage maps to store mutlisig accounts and proposals. @@ -125,9 +415,9 @@ As for the values: ```rust pub struct MultisigAccountDetails { - /// The owners of the multisig account. This is a BoundedBTreeSet to ensure faster operations (add, remove). - /// As well as lookups and faster set operations to ensure approvers is always a subset from owners. (e.g. in case of removal of an owner during an active proposal) - pub owners: BoundedBTreeSet, + /// The signers of the multisig account. This is a BoundedBTreeSet to ensure faster operations (add, remove). + /// As well as lookups and faster set operations to ensure approvers is always a subset from signers. (e.g. in case of removal of an signer during an active proposal) + pub signers: BoundedBTreeSet, /// The threshold of approvers required for the multisig account to be able to execute a call. pub threshold: u32, pub creator: T::AccountId, @@ -143,27 +433,31 @@ pub struct MultisigProposal { /// The extrinsic when the multisig operation was opened. pub when: Timepoint>, /// The approvers achieved so far, including the depositor. - /// The approvers are stored in a BoundedBTreeSet to ensure faster lookup and operations (approve, revoke). + /// The approvers are stored in a BoundedBTreeSet to ensure faster lookup and operations (approve, reject). /// It's also bounded to ensure that the size don't go over the required limit by the Runtime. pub approvers: BoundedBTreeSet, + /// The rejectors for the proposal so far. + /// The rejectors are stored in a BoundedBTreeSet to ensure faster lookup and operations (approve, reject). + /// It's also bounded to ensure that the size don't go over the required limit by the Runtime. + pub rejectors: BoundedBTreeSet, /// The block number until which this multisig operation is valid. None means no expiry. pub expire_after: Option>, } ``` -For optimization we're using BoundedBTreeSet to allow for efficient lookups and removals. Especially in the case of approvers, we need to be able to remove an approver from the list when they revoke their approval. (which we do lazily when `execute_proposal` is called). +For optimization we're using BoundedBTreeSet to allow for efficient lookups and removals. Especially in the case of approvers, we need to be able to remove an approver from the list when they reject their approval. (which we do lazily when `execute_proposal` is called). -There's an extra storage map for the deposits of the multisig accounts per owner added. This is to ensure that we can release the deposits when the multisig removes them even if the constant deposit per owner changed in the runtime later on. +There's an extra storage map for the deposits of the multisig accounts per signer added. This is to ensure that we can release the deposits when the multisig removes them even if the constant deposit per signer changed in the runtime later on. ### Considerations & Edge cases -#### Removing an owner from the multisig account during an active proposal +#### Removing an signer from the multisig account during an active proposal - We need to ensure that the approvers are always a subset from owners. This is also partially why we're using BoundedBTreeSet for owners and approvers. Once execute proposal is called we ensure that the proposal is still valid and the approvers are still a subset from current owners. + We need to ensure that the approvers are always a subset from signers. This is also partially why we're using BoundedBTreeSet for signers and approvers. Once execute proposal is called we ensure that the proposal is still valid and the approvers are still a subset from current signers. #### Multisig account deletion and cleaning up existing proposals -Once the last owner of a multisig account is removed or the multisig approved the account deletion we delete the multisig accound from the state and keep the proposals until someone calls `cleanup_proposals` multiple times which iterates over a max limit per extrinsic. This is to ensure we don't have unbounded iteration over the proposals. Users are already incentivized to call `cleanup_proposals` to get their deposits back. +Once the last signer of a multisig account is removed or the multisig approved the account deletion we delete the multisig accound from the state and keep the proposals until someone calls `cleanup_proposals` multiple times which iterates over a max limit per extrinsic. This is to ensure we don't have unbounded iteration over the proposals. Users are already incentivized to call `cleanup_proposals` to get their deposits back. #### Multisig account deletion and existing deposits @@ -240,7 +534,7 @@ The main takeway is that we don't need to pass the threshold and other signatori So now for the caclulations, given the following: * K is the number of multisig accounts. -* N is number of owners in each multisig account. +* N is number of signers in each multisig account. * For each proposal we need to have 2N/3 approvals. The table calculates if each of the K multisig accounts has one proposal and it gets approved by the 2N/3 and then executed. How much did the total Blocks and States sizes increased by the end of the day. @@ -253,7 +547,7 @@ Stateful effect on blocksizes = K * N (as each user will need to call approve wi Stateless effect on statesizes = Nil (as the multisig account is not stored in the state) -Stateful effect on statesizes = K*N (as each multisig account (K) will be stored with all the owners (K) in the state) +Stateful effect on statesizes = K*N (as each multisig account (K) will be stored with all the signers (K) in the state) | Pallet | Block Size | State Size | |----------------|:-------------:|-----------:| @@ -282,12 +576,12 @@ This RFC is compatible with the existing implementation and can be handled via u ## Unresolved Questions -* On account deletion, should we transfer remaining deposits to treasury or remove owners' addition deposits completely and consider it as fees to start with? +* On account deletion, should we transfer remaining deposits to treasury or remove signers' addition deposits completely and consider it as fees to start with? ## Future Directions and Related Material * [ ] Batch proposals. The ability to batch multiple calls into one proposal. -* [ ] Batch addition/removal of owners. +* [ ] Batch addition/removal of signers. * [ ] Add expiry to proposals. After a certain time, proposals will not accept any more approvals or executions and will be deleted. * [ ] Add extra identifier other than call_hash to proposals (e.g. nonce). This will allow same call to be proposed multiple times and be in pending state. * [ ] Implement call filters. This will allow multisig accounts to only accept certain calls. From 6cd9f8e47de2708e2a16f20571fa2763cbba9996 Mon Sep 17 00:00:00 2001 From: asoliman Date: Fri, 16 Feb 2024 14:09:29 +0400 Subject: [PATCH 12/16] Note that PR is not up to date --- text/0074-stateful-multisig-pallet.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/text/0074-stateful-multisig-pallet.md b/text/0074-stateful-multisig-pallet.md index d26754177..f04d61422 100644 --- a/text/0074-stateful-multisig-pallet.md +++ b/text/0074-stateful-multisig-pallet.md @@ -66,7 +66,7 @@ and much more... ## Explanation -I've created the stateful multisig pallet during my studies in Polkadot Blockchain Academy under supervision from @shawntabrizi and @ank4n. After that, I've enhanced it to be fully functional and this is a draft [PR#3300](https://github.com/paritytech/polkadot-sdk/pull/3300) in polkadot-sdk. I'll list all the details and design decisions in the following sections. +I've created the stateful multisig pallet during my studies in Polkadot Blockchain Academy under supervision from @shawntabrizi and @ank4n. After that, I've enhanced it to be fully functional and this is a draft [PR#3300](https://github.com/paritytech/polkadot-sdk/pull/3300) in polkadot-sdk. I'll list all the details and design decisions in the following sections. Note that the PR is not 1-1 exactly to the current RFC as the RFC is a more polished version of the PR after updating based on the feedback and discussions. Let's start with a sequence diagram to illustrate the main operations of the Stateful Multisig. @@ -83,8 +83,6 @@ Notes on above diagram: ### State Transition Functions -All functions have detailed rustdoc in [PR#3300](https://github.com/paritytech/polkadot-sdk/pull/3300). Here is a brief overview of the functions: - * `create_multisig` - Create a multisig account with a given threshold and initial signers. (Needs Deposit) ```rust From 4ab8d0f8881d09f02416f03910677a64a9c45509 Mon Sep 17 00:00:00 2001 From: asoliman Date: Fri, 16 Feb 2024 14:10:27 +0400 Subject: [PATCH 13/16] System chains instead of collectives only --- text/0074-stateful-multisig-pallet.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0074-stateful-multisig-pallet.md b/text/0074-stateful-multisig-pallet.md index f04d61422..9f7b23d06 100644 --- a/text/0074-stateful-multisig-pallet.md +++ b/text/0074-stateful-multisig-pallet.md @@ -3,7 +3,7 @@ | | | | --------------- | ------------------------------------------------------------------------------------------- | | **Start Date** | 15 February 2024 | -| **Description** | Add Enhanced Multisig Pallet to Collectives chain | +| **Description** | Add Enhanced Multisig Pallet to System chains | | **Authors** | Abdelrahman Soliman (Boda) | ## Summary From 0fa55ab95103262c7b78b0181b565269810e191c Mon Sep 17 00:00:00 2001 From: asoliman Date: Sat, 17 Feb 2024 13:22:08 +0400 Subject: [PATCH 14/16] Tbaut comments on pure-proxy --- text/0074-stateful-multisig-pallet.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/text/0074-stateful-multisig-pallet.md b/text/0074-stateful-multisig-pallet.md index 9f7b23d06..ecc751819 100644 --- a/text/0074-stateful-multisig-pallet.md +++ b/text/0074-stateful-multisig-pallet.md @@ -34,6 +34,12 @@ We believe as well that the stateless multisig is not efficient in terms of bloc Pure proxy can achieve having a stored and determinstic multisig account from different users but it's unneeded complexity as a way around the limitations of the current multisig pallet. It doesn't also have the same fine grained control over the multisig account. +Other points mentioned by @tbaut + +* pure proxies aren't (yet) a thing cross chain +* the end user complexity is much much higher with pure proxies, also for new users smart contract multisig are widely known while pure proxies are obscure. +* you can shoot yourself in the foot by deleting the proxy, and effectively loosing access to funds with pure proxies. + ### Requirements Basic requirements for the Stateful Multisig are: From ff36f5722ccfac9eed7e5f8a94567442a7d3ce37 Mon Sep 17 00:00:00 2001 From: asoliman Date: Sun, 18 Feb 2024 23:30:11 +0400 Subject: [PATCH 15/16] Future directions --- text/0074-stateful-multisig-pallet.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/text/0074-stateful-multisig-pallet.md b/text/0074-stateful-multisig-pallet.md index ecc751819..c40f4f141 100644 --- a/text/0074-stateful-multisig-pallet.md +++ b/text/0074-stateful-multisig-pallet.md @@ -584,8 +584,6 @@ This RFC is compatible with the existing implementation and can be handled via u ## Future Directions and Related Material -* [ ] Batch proposals. The ability to batch multiple calls into one proposal. * [ ] Batch addition/removal of signers. * [ ] Add expiry to proposals. After a certain time, proposals will not accept any more approvals or executions and will be deleted. -* [ ] Add extra identifier other than call_hash to proposals (e.g. nonce). This will allow same call to be proposed multiple times and be in pending state. * [ ] Implement call filters. This will allow multisig accounts to only accept certain calls. From 1e8cd39f7cf22cc4ae47feb2ba010f3e17becd84 Mon Sep 17 00:00:00 2001 From: asoliman Date: Sun, 18 Feb 2024 23:58:45 +0400 Subject: [PATCH 16/16] Use CallOrHash enum instead of only hash --- text/0074-stateful-multisig-pallet.md | 42 +++++++++++++++++---------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/text/0074-stateful-multisig-pallet.md b/text/0074-stateful-multisig-pallet.md index c40f4f141..5b025a3c6 100644 --- a/text/0074-stateful-multisig-pallet.md +++ b/text/0074-stateful-multisig-pallet.md @@ -89,6 +89,15 @@ Notes on above diagram: ### State Transition Functions +having the following enum to store the call or the hash: + +```rust +enum CallOrHash { + Call(::RuntimeCall), + Hash(T::Hash), +} +``` + * `create_multisig` - Create a multisig account with a given threshold and initial signers. (Needs Deposit) ```rust @@ -127,7 +136,7 @@ Notes on above diagram: /// # Arguments /// /// * `multisig_account` - The multisig account ID. - /// * `call` - The dispatchable call to be executed. + /// * `call_or_hash` - The enum having the call or the hash of the call to be approved and executed later. /// /// # Errors /// @@ -137,7 +146,7 @@ Notes on above diagram: pub fn start_proposal( origin: OriginFor, multisig_account: T::AccountId, - call_hash: T::Hash, + call_or_hash: CallOrHash, ) -> DispatchResult ``` @@ -153,7 +162,7 @@ Notes on above diagram: /// # Arguments /// /// * `multisig_account` - The multisig account ID. - /// * `call_hash` - The hash of the call to be approved. (This will be the hash of the call that was used in `start_proposal`) + /// * `call_or_hash` - The enum having the call or the hash of the call to be approved. /// /// # Errors /// @@ -164,7 +173,7 @@ Notes on above diagram: pub fn approve( origin: OriginFor, multisig_account: T::AccountId, - call_hash: T::Hash, + call_or_hash: CallOrHash, ) -> DispatchResult ``` @@ -181,7 +190,7 @@ Notes on above diagram: /// # Arguments /// /// * `multisig_account` - The multisig account ID. - /// * `call_hash` - The hash of the call to be approved. (This will be the hash of the call that was used in `start_proposal`) + /// * `call_or_hash` - The enum having the call or the hash of the call to be rejected. /// /// # Errors /// @@ -193,7 +202,7 @@ Notes on above diagram: pub fn reject( origin: OriginFor, multisig_account: T::AccountId, - call_hash: T::Hash, + call_or_hash: CallOrHash, ) -> DispatchResult ``` @@ -212,17 +221,19 @@ Notes on above diagram: /// # Arguments /// /// * `multisig_account` - The multisig account ID. - /// * `call` - The call to be executed. + /// * `call_or_hash` - We should have gotten the RuntimeCall (preimage) and stored it in the proposal by the time the extrinsic is called. /// /// # Errors /// /// * `MultisigNotFound` - The multisig account does not exist. /// * `UnAuthorizedSigner` - The caller is not an signer of the multisig account. /// * `NotEnoughApprovers` - approvers don't exceed the threshold. + /// * `ProposalNotFound` - The proposal does not exist. + /// * `CallPreImageNotFound` - The proposal doesn't have the preimage of the call in the state. pub fn execute_proposal( origin: OriginFor, multisig_account: T::AccountId, - call: Box<::RuntimeCall>, + call_or_hash: CallOrHash, ) -> DispatchResult ``` @@ -241,7 +252,7 @@ Notes on above diagram: /// # Arguments /// /// * `origin` - The origin multisig account who wants to cancel the proposal. - /// * `call_hash` - The hash of the call to be canceled. (This will be the hash of the call that was used in `start_proposal`) + /// * `call_or_hash` - The call or hash of the call to be canceled. /// /// # Errors /// @@ -250,7 +261,7 @@ Notes on above diagram: pub fn cancel_proposal( origin: OriginFor, multisig_account: T::AccountId, - call_hash: T::Hash) -> DispatchResult + call_or_hash: CallOrHash) -> DispatchResult ``` * `cancel_own_proposal` - Cancel a multisig proposal started by the caller in case no other signers approved it yet. (Releases Deposit) @@ -266,7 +277,7 @@ Notes on above diagram: /// # Arguments /// /// * `multisig_account` - The multisig account ID. - /// * `call_hash` - The hash of the call to be canceled. (This will be the hash of the call that was used in `start_proposal`) + /// * `call_or_hash` - The hash of the call to be canceled. /// /// # Errors /// @@ -275,7 +286,7 @@ Notes on above diagram: pub fn cancel_own_proposal( origin: OriginFor, multisig_account: T::AccountId, - call_hash: T::Hash, + call_or_hash: CallOrHash, ) -> DispatchResult ``` @@ -424,7 +435,6 @@ pub struct MultisigAccountDetails { pub signers: BoundedBTreeSet, /// The threshold of approvers required for the multisig account to be able to execute a call. pub threshold: u32, - pub creator: T::AccountId, pub deposit: BalanceOf, } ``` @@ -513,7 +523,7 @@ We have the following extrinsics: pub fn start_proposal( origin: OriginFor, multisig_account: T::AccountId, - call_hash: T::Hash, + call_or_hash: CallOrHash, ) ``` @@ -521,7 +531,7 @@ pub fn start_proposal( pub fn approve( origin: OriginFor, multisig_account: T::AccountId, - call_hash: T::Hash, + call_or_hash: CallOrHash, ) ``` @@ -529,7 +539,7 @@ pub fn approve( pub fn execute_proposal( origin: OriginFor, multisig_account: T::AccountId, - call: Box<::RuntimeCall>, + call_or_hash: CallOrHash, ) ```