Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Enhanced Multisig Pallet to System chains #74

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
293 changes: 293 additions & 0 deletions text/0074-stateful-multisig-pallet.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,293 @@
# RFC-0074: Stateful Multisig Pallet

| | |
| --------------- | ------------------------------------------------------------------------------------------- |
| **Start Date** | 15 February 2024 |
| **Description** | Add Enhanced Multisig Pallet to Collectives chain |
| **Authors** | Abdelrahman Soliman (Boda) |

## 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 Multisig` 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 less optimal, performance-wise (as we'll explain later in the RFC), and lack fine-grained control over the multisig account.

#### 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 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 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.
asoliman92 marked this conversation as resolved.
Show resolved Hide resolved

### 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.

and much more...

## 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.

![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)
* `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)

### 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:
xlc marked this conversation as resolved.
Show resolved Hide resolved

* `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)
xlc marked this conversation as resolved.
Show resolved Hide resolved
* `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<T: Config> = StorageMap<_, Twox64Concat, T::AccountId, MultisigAccountDetails<T>>;

/// 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<T: Config> = StorageDoubleMap<
_,
Twox64Concat,
T::AccountId, // Multisig Account
Blake2_128Concat,
T::Hash, // Call Hash
MultisigProposal<T>,
>;
```

As for the values:

```rust
pub struct MultisigAccountDetails<T: Config> {
/// 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<T::AccountId, T::MaxSignatories>,
/// The threshold of approvers required for the multisig account to be able to execute a call.
pub threshold: u32,
pub creator: T::AccountId,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need this? looks like it is for refund deposit when the multisig is destroyed. the easy solution is to refund it to the signer that triggers the removal

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoever created the multisig might be not in the signers list and even if they're one of the signers, anyone from the signers can dispatch/cancel the final call. That's why it's better to return it to the original creator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of deposit is to create incentive for people to free up unused resources.
In this case, it is very possible that the creator is not the one with ability to remove the multisig. If the deposit is returned to the creator, it creates a misaligned incentive. Why should I as the current controller of the mulisig remove it to have the creator, which is someone else, to receive the deposit? There are zero incentives for the people with ability to free the resources. So it should be the one triggering the removal taking the deposit.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it common practice though for someone to deposit and another to get their deposit back? I understand the incentive part but having someone else take my deposit if I created the account seems a bit unfair. Maybe have part of the deposit returned to the person executing the deletion process and another part to the original creator?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the original creator have to explicitly transfer the ownership of the multisig so there is nothing unfair. the multisig itself must already worth something more than deposit so the value of deposit won’t change anything

pub deposit: BalanceOf<T>,
}
```

```rust
pub struct MultisigProposal<T: Config> {
/// Proposal creator.
pub creator: T::AccountId,
pub creation_deposit: BalanceOf<T>,
/// The extrinsic when the multisig operation was opened.
pub when: Timepoint<BlockNumberFor<T>>,
/// 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<T::AccountId, T::MaxSignatories>,
xlc marked this conversation as resolved.
Show resolved Hide resolved
/// The block number until which this multisig operation is valid. None means no expiry.
pub expire_after: Option<BlockNumberFor<T>>,
}
```

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

#### Removing an owner 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.

#### 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

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

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

* New pallet to maintain.

## Testing, Security, and Privacy

Standard audit/review requirements apply.

## Performance, Ergonomics, and Compatibility

### 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.

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<T>,
threshold: u16,
other_signatories: Vec<T::AccountId>,
maybe_timepoint: Option<Timepoint<BlockNumberFor<T>>>,
call_hash: [u8; 32],
max_weight: Weight,
```

Stateful Multisig:
We have the following extrinsics:

```rust
pub fn start_proposal(
origin: OriginFor<T>,
multisig_account: T::AccountId,
call_hash: T::Hash,
)
```

```rust
pub fn approve(
origin: OriginFor<T>,
multisig_account: T::AccountId,
call_hash: T::Hash,
)
```

```rust
pub fn execute_proposal(
origin: OriginFor<T>,
multisig_account: T::AccountId,
call: Box<<T as Config>::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 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 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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would also be great to see more realistic storage benchmark of both implementations of multisig pallet.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That means I need to implement it and implement the benchmark and then compare them. I'll do this for sure if I got the green light to proceed with the RFC. As this is an RFC to illustrate the importance and the overall design I think this back of the envelop calculation should be enough unless you see something fundamentally wrong with my calculations. WDYT?


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 |

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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great addition to have a better functional comparison to the stateless pallet instead of a generic statement. Are there use cases where you believe stateless is better?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there use cases where you believe stateless is better?

That's a good question. I don't have any use case in my mind that the stateless is better TBH. Stateful can do all what the stateless do with added functionality, fine tuned control and clearer API. Do you have a case in mind?

Would be great addition to have a better functional comparison to the stateless pallet instead of a generic statement.

I thought the RFC itself is an enough comparison to make it clear by reaching this point. Do you suggest explaining it again like in overview and motivation sections or add something else?


### Compatibility

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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does compatible mean here? Can existing stateless users migrate to stateful without losing the multisig address? How would the process look like?

Also, why (or why not) replace the existing pallet with stateful? Keeping two pallets with same functionality might add more confusion to the end users.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compatible in this section means that it's not a breaking change and no other pallets will have issues if this is deployed.

Can existing stateless users migrate to stateful without losing the multisig address? How would the process look like?

It's possible in theory yes. I'd rather offload this to users though. Once the new pallet is ready they can create a new account and move fund if needed. The incentive is easier/extensible multisig account to deal with.

Keeping two pallets with same functionality might add more confusion to the end users.

I agree. I think having the stateful will make it easier for users to use after knowing the enhanced functionalities and they'll start using the new one but maybe that's not enough. What do you suggest here? I think first get the sentiment about the stateful multisig, deploy and then we can later see the usage on-chain and decide to migrate and kill the older one or not.


## Prior Art and References

[multisig pallet in polkadot-sdk](https://github.com/paritytech/polkadot-sdk/tree/master/substrate/frame/multisig)

## 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?

## Future Directions and Related Material

* [ ] Batch proposals. The ability to batch multiple calls into one proposal.
asoliman92 marked this conversation as resolved.
Show resolved Hide resolved
* [ ] 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.