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

Conversation

asoliman92
Copy link

@asoliman92 asoliman92 commented Feb 15, 2024

This RFC proposes adding an enhanced Multisig pallet to System chains for both better footprint on the blockchain and better enhanced control and ergonomics.

@asoliman92 asoliman92 changed the title Add Enhanced Multisig Pallet Add Enhanced Multisig Pallet to Collective chain Feb 15, 2024
@asoliman92 asoliman92 changed the title Add Enhanced Multisig Pallet to Collective chain Add Enhanced Multisig Pallet to Collectives chain Feb 15, 2024
text/0074-stateful-multisig-pallet.md Outdated Show resolved Hide resolved
text/0074-stateful-multisig-pallet.md Outdated Show resolved Hide resolved
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

text/0074-stateful-multisig-pallet.md Show resolved Hide resolved
Copy link
Contributor

@xlc xlc left a comment

Choose a reason for hiding this comment

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

I don't see a section explain why to add this pallet to collectives chain (instead of other chains

@asoliman92
Copy link
Author

asoliman92 commented Feb 16, 2024

I actually had it as an open question to begin with and then moved it to subject as a suggestion. I did it by process of elimination of other system chains. But now that you mention it, I notice that the original multisig is available on all system chains. What are your thoughts?

-- We discussed offline and @xlc also thinks that having it on all system chains makes sense.

@asoliman92 asoliman92 changed the title Add Enhanced Multisig Pallet to Collectives chain Add Enhanced Multisig Pallet to System chains Feb 16, 2024
@asoliman92 asoliman92 requested a review from xlc February 16, 2024 10:15
/// * `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(
Copy link

@Tbaut Tbaut Feb 16, 2024

Choose a reason for hiding this comment

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

A major hurdle for users, is that they need some offline communication to pass around the call data, so that they can verify that the hash is actually matching, and for the last signer to execute the call.

While this is great for privacy, as in it's hard to guess as an external watcher what the call is about by knowing only the hash. Experience has shown that most users don't need this additional privacy. Also the reality is that they don't verify the call data and blindly approve multisig calls, trusting the original proposer. Passing around this call data becomes a hurdle, and weakens the security, because verification is a cumbersome process. As a result, Multix and many other multisig management platforms use multisig.asMulti for the proposer call, hence exposing the call in the block and relying on an indexer to later decode the call, and show users what they are signing.

If we add such a pallet to a system chain, I'd strongly advocate to include a mechanism to optionally store the call in the state, for start_proposal and then have it optional as well for the execute_proposal and retrieve it from the state if possible.

Copy link
Author

Choose a reason for hiding this comment

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

I don't really have strong opinion here. I initially had it as a RuntimeCall and changed it in favor of the hash for both privacy and block footprint. I was thinking using preimage as well but maybe that's just complicating it.

the reality is that they don't verify the call data and blindly approve multisig calls, trusting the original proposer.

Don't you think we can offload this part to the platforms instead to get the best of both worlds? Multix can get the call and hash it with the correct hash before calling the extrinsic using preimage pallet. That will ensure seamless and private experience for correct users without sacrificing block footprint. It might not be the best for people who want to use it directly from polkadot.js for example but still good enough for most people who'll use some tool. What are your thoughts?

Copy link

Choose a reason for hiding this comment

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

We're building a decentralized network, yet for average users to be able to use multisigs we have to resort to centralized indexers (in the case of Multix) or IMHO worse, full blown web2 stacks, storing the callData. Letting such important part of the puzzle to centralized entities is certainly not the best of both worlds. If we let users choose, then yes. But today (and in your proposed solution) there's no choice.

Copy link
Author

Choose a reason for hiding this comment

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

I agree 👍. I'm coming from web2 and in general I'm more inclined to make the API clearer and easier to use. I didn't want that to influence me much and trying to embrace optimizing for the blockchain itself as well. @shawntabrizi suggested using the hash as an optimization and using preimage pallet when I discussed with him during the PBA earlier. I know it's different schools but I'm interested to hear others thoughts as well.

CC @xlc @Ank4n @bkchr

Copy link

Choose a reason for hiding this comment

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

Preimage would work, but in practice it's still cumbersome because only the submitter can delete/free the preimage that was set. This means that the submitter would need an additional call, once the multisig proposal has been executed, to free their preimage. My choice would be to have the same logic as the preimage, and the same deposit, but this preimage would be linked to the multisig proposal, and would be freed when the call is executed.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can just have both. Make it something like

enum CallOrHash {
  Call(Vec<u8>,
  Hash(H256),
}

and up to the user to decide how to use it

In fact, this is how the current multisig works

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking having the CallOrHash will be complicating the solution more when I started working on this. Now I see it might be better to use it with some preimage in the state as well. I will change the RFC accordingly then.

Copy link
Author

Choose a reason for hiding this comment

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

I've changed the RFC to use CallOrHash in all extrinsics.

I'd strongly advocate to include a mechanism to optionally store the call in the state, for start_proposal and then have it optional as well for the execute_proposal and retrieve it from the state if possible.

@Tbaut
I've not written the details of storing the preimage but the execute_proposal specs explains it and added a new error if the preimage is not found by the time of execution.

Copy link

@Ank4n Ank4n left a comment

Choose a reason for hiding this comment

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

afaiu RFC is not about implementation detail (although in some special cases it can be) but high level technical design decisions. I would like to see more examples/evidences of why stateful multisig would be significant improvement over stateless multisig such that it warrants the existing system chains to add a new pallet in its runtime. Introducing this pallet for new runtimes probably needs much less convincing.

* 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.
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?


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


## Unresolved Questions

* On account deletion, should we transfer remaining deposits to treasury or remove signers' addition deposits completely and consider it as fees to start with?
Copy link

Choose a reason for hiding this comment

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

Then the user has no incentive to delete the account. Having the deposit that should be returned on clearing makes sense to me.

Copy link
Author

Choose a reason for hiding this comment

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

The caller will get back the deposit. This is only for adding new singers to the multisig after it's been created. In this case the add_signer is a multisig operation and the deposit is taken from the multisig account itself.

text/0074-stateful-multisig-pallet.md Show resolved Hide resolved

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

text/0074-stateful-multisig-pallet.md Outdated Show resolved Hide resolved
@asoliman92
Copy link
Author

afaiu RFC is not about implementation detail (although in some special cases it can be) but high level technical design decisions. I would like to see more examples/evidences of why stateful multisig would be significant improvement over stateless multisig such that it warrants the existing system chains to add a new pallet in its runtime. Introducing this pallet for new runtimes probably needs much less convincing.

So in your opinion the enhanced functionality, ease of use and less footprint (from quadratic to linear) are not enough? Or you think that these evidences are lacking enough backing from the RFC as is?

@gavofyork
Copy link
Contributor

Some things which should be mentioned in drawbacks:

  • adding a new pallet which can control accounts across system chains substantially increases attack surface.
  • stateful code is especially hard to analyse fully.
  • multisig/pure proxy support is already patchy at best; adding another way of doing the same thing could stoke fragmentation with different UIs supporting different multisig configurations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants