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

Governance - MultiSig #26

Closed
3 of 5 tasks
malikankit opened this issue Apr 26, 2021 · 17 comments
Closed
3 of 5 tasks

Governance - MultiSig #26

malikankit opened this issue Apr 26, 2021 · 17 comments
Assignees

Comments

@malikankit
Copy link
Contributor

malikankit commented Apr 26, 2021

  • Set up a multi-sig contract that will act as Governance for Lido
  • The contract (given sufficient signatures) should be able to call functions in the Lido program and the Stake Pool Program
  • The contract should be able to upgrade the two programs
  • The contract should act as owner - aka upgrade authority - of the Lido Program and Stake Pool Program
  • Make this program upgreadable, i.e. given enough signatures, it can replace itself as the owner of the two programs.
@malikankit
Copy link
Contributor Author

IF @naomijub 's health is better by Wednesday 28 Apr
THEN Ruud and Julia can collaborate on this
ELSE Ruud and Fynn can collaborate on this

@ruuda :Let's also discuss this in our catch up call on Wednesday.

@enriquefynn
Copy link
Member

enriquefynn commented Apr 27, 2021

I created a branch at 0e1c2ed

The code skeleton is at dao/program

I think it can be a very simple design: We can have mainly one function:
process_instruction that takes as an argument an instruction. It fails if less than 50% of signatures from members are present in the transaction.
The owner of LIDO and stake_pool will be a derived address from <DAO_program_id, bump_seed> (we can change that in the future so users can define multiple derived addresses by passing the seeds as an argument).

Suppose we want to add a validator, everything is done in the UI level.
We craft an instruction add_validator(..) and pass it to the process_instruction together with validators signatures signed in the transaction.
Inside the process_instruction all we do is verify if enough signatures are valid and execute the add_validator(..) instruction on behalf of the derived address < DAO_program_id, bump_seed>

@ruuda
Copy link
Contributor

ruuda commented Apr 29, 2021

We discovered https://github.com/project-serum/multisig which appears to do exactly what we need. Some notes from playing around with it:

  • It needs version 1.6 of the Solana SDK, 1.5 does not work.
  • There needs to be only one Multisig program on chain, and it can manage multiple “multisig accounts” (which is a set of public keys + threshold). A new multisig account can be created at https://multisig.projectserum.com/.
  • It might make sense to deploy our own program nonetheless, so we are in control of upgrades.

So far I haven’t been able to make the UI connect to my local testnet. I edited this line in multisig-ui to point to the program address of my locally deployed multisig program, and added the upgrade authority as well. It prints

Error: Invalid account discriminator

In the developer console. Investigating further ...

@malikankit
Copy link
Contributor Author

@joncinque : ^ Any thoughts/opinion on using Serum Multisig for Lido Multisig Governance?

@joncinque
Copy link
Contributor

joncinque commented Apr 29, 2021

This is probably related to how anchor fetches on-chain information. Anchor is the framework used to develop the multisig program, and I believe it includes publishing the instruction / state data on-chain. So if you've only published the new program for yourself, you may be missing this extra information. I'll have to look into anchor a bit more in general, but if you have specific questions about it, you can ask Armani in the Serum discord, since I believe he led the development on anchor.

Until we release the governance program, this multisig will likely be the best option for Lido program governance.

@ruuda
Copy link
Contributor

ruuda commented Apr 29, 2021

With the Rust client, I was able to create a multisig account on my local testnet. Now for testing the other features!

I pushed this to the cli branch for now (ChorusOne/multisig@9a07431).

@ruuda
Copy link
Contributor

ruuda commented Apr 29, 2021

A bit more detail about how the Serum Multisig works. I am going to refer to the Multisig program as Multisig (with a capital M), and to a group of owners who vote on transactions as a multisig (lowercase) account. The Multisig program can facilitate many multisig accounts, and each multisig account can execute many transactions.

  • You create a multisig account. This account holds the data for who the owners of the multisig are (those who can sign) and what the thresold (minimum number of signatures) is.
  • From the multisig account and the Multisig program id, we get a program derived address. This is the address that the multisig can sign with, so this is the one that you can set as e.g. the upgrade authority for the Lido program.
  • Any owner of the multisig can propose a transaction. The proposal holds one Solana Instruction. To propose a transaction, yet another account is created, that holds the instruction, a boolean per owner that indicates whether that owner approved the transaction, and a boolean to prevent executing twice.
  • Owners can approve a transaction by calling approve in the Multisig program for a given multisig account and transaction account.
  • If there are enough approvals, anybody can call execute_transaction in the Multisig program for a given multisig account and transaction, and it will invoke_signed the instruction, signed by the program derived account.

How we would use it:

  • In practice, the instruction to propose would be bpf_loader_upgradable::upgrade, and that is how the multisig can be used to update programs.
  • It could also upgrade the Multisig program itself in this way.
  • We can also propose a set_upgrade_authority instruction. That way the multisig could later set the upgrade authority for Lido to a more elaborate governance program.
  • For anything in Lido that we want to gate access to by the multisig, we can have the Lido program check that the caller is equal to the upgrade authority.

How to interact with the Multisig program:

  • There is the UI at https://multisig.projectserum.com/, but I couldn’t fully get it to work with my local testnet. It does recognize the multisig after I created it by a different means, but then creating transactions doesn’t work, and it does not display them either. I found this very hard to debug because it’s all javascript with runtime-generated objects.
  • So instead I am now creating a CLI program to interact with the Multisig program.
  • So far I managed to create a new multisig, and propose an upgrade transaction.
  • If we have any Lido-specific things for the multisig to approve later (like adding a validator), we could add that to this program.
  • While https://multisig.projectserum.com interacts with a browser-based wallet, the CLI reads your private key from ~/.config/solana/id.json 😬

I’m just working my way through the Multisig functions, the next things to add are approve and execute_transaction, and then I should be able to upgrade a program on my local testnet through Multisig. The work in progress is here: ChorusOne/multisig#1 (beware, public repository).

@malikankit
Copy link
Contributor Author

malikankit commented May 2, 2021

I just read the entire thread @ruuda : This is great progress, given the short timeline! :)
It's best to start with what you proposed : upgrade a program on my local testnet through Multisig.

Meanwhile - I will work on a couple of issues

These will focus on the governance related additions to be made to these programs. eg. functions to change fees params etc.

One question : Will set_upgrade_authority require any changes / function additions in the Lido Program? Or this will be external to the Lido Program code (as long as its upgrade authority is set to our Multisig PDA on deployment?).

@malikankit malikankit changed the title Governance Governance - MultiSig May 2, 2021
@ruuda
Copy link
Contributor

ruuda commented May 3, 2021

Will set_upgrade_authority require any changes / function additions in the Lido Program? Or this will be external to the Lido Program code (as long as its upgrade authority is set to our Multisig PDA on deployment?).

It will not require changes, everything is external to Lido. The initial deployer of Lido must set the upgrade authority to the multisig PDA.

  • The upgrade authority of a program can be changed with set_upgrade_authority from the BPFLoaderUpgradable program.
  • The BPFLoaderUpgradable enforces that the change is signed by the program’s current upgrade authority. (See this doc comment.)
  • Therefore, we can create a Solana instruction to call set_upgrade_authority (which is also what solana program set-upgrade-authority would do), but instead of executing it directly, we wrap it in a multisig transaction.
  • Once the multisig approves, set_upgrade_authority gets called, signed by the multisig PDA.

@enriquefynn
Copy link
Member

What can be done when there's a malicious owner that keeps proposing an instruction to stagger the multisig?
How is the process of updating the owners (adding, removing)?

@ruuda
Copy link
Contributor

ruuda commented May 3, 2021

What can be done when there's a malicious owner that keeps proposing an instruction to stagger the multisig?

What does “stagger the multisig” mean?

How is the process of updating the owners (adding, removing)?

The Multisig program has a function set_owners and change_threshold for this. (Now that I think of it, this is a bad design, because you can only perform one instruction per multisig transaction, so if you want to require unanimous support, you can’t atomically add a signer — you have to add the new owner and approve, and and then change the threshold and approve. I’ll open an issue about this. Edit: coral-xyz/multisig#4)

But I think that function is not even needed. What you could do instead is just create a new multisig with the updated owners, and then use the old multisig to change the upgrade authority of Solido to the new multisig. This is simpler, it has less moving parts. The only downside I can think of is if you use the multisig to manage many programs, you need to do this for all of them.

@enriquefynn
Copy link
Member

enriquefynn commented May 4, 2021

What does “stagger the multisig” mean?

If I'm a malicious member I can keep proposing some transaction ahead of everyone to "block" the contract, as it processes
only 1 tx per time.

But I think that function is not even needed. What you could do instead is just create a new multisig with the updated owners, and then use the old multisig to change the upgrade authority of Solido to the new multisig. This is simpler, it has less moving parts. The only downside I can think of is if you use the multisig to manage many programs, you need to do this for all of them.

I think you can upgrade the multisig with another address for the owners, in that way you don't need to change any of the programs it controls.

@ruuda
Copy link
Contributor

ruuda commented May 4, 2021

If I'm a malicious member I can keep proposing some transaction ahead of everyone to "block" the contract, as it processes
only 1 tx per time.

As discussed offline, this is not an issue, because many proposed transactions can exist simultaneously; the proposer opens a new account, and the transaction and its status are stored in there.

@ruuda
Copy link
Contributor

ruuda commented May 4, 2021

Status update

I now have the CLI working to the point where it can successfully upgrade a program whose upgrade authority is set to the multisig’s derived address.

As discussed with @enriquefynn, I’ll also add a command to the CLI to change the owners of the multisig. It would be possible to create a new account with the new owners instead, but because multisig accounts cannot be closed at this time, it might be good to stick to a single account.

@ruuda
Copy link
Contributor

ruuda commented May 4, 2021

It would also be nice to add an --output json option to the multisig CLI, just like solana has. That way I can write an automated test that goes through the entire flow of setting up a multisig and upgrading a program.

@ruuda
Copy link
Contributor

ruuda commented May 6, 2021

Status update

The CLI can upgrade a program now, and it can also change its owners/threshold. I am still working on adding an automated test that calls all commands in sequence.

If we want the multisig to also propose calls to Lido, that’s now easy to add — if we have a solana_sdk::Instruction, then it can generate a transaction for that. For example: https://github.com/ChorusOne/multisig/blob/86af57759fe0b86f00ff386501fedf9796edc4c9/cli/src/main.rs#L444-L457

@ruuda
Copy link
Contributor

ruuda commented May 7, 2021

Next steps

The multisig program can now build and view two instructions: upgrade a program, and change the multisig. If we want to use it for managing Lido and the stake pool, it would become a specific utility for interacting with Lido, no longer a generic multisig program. I quickly discussed with @enriquefynn and we think this is the best way forward:

  • Embed the upstream multisig repository as a Git submodule in this repository, but point it at our fork. This makes it easy to keep it up to date, should it change upstream.
  • Move the multisig CLI out of that repository, and into the Lido repository, so it has access to the Rust functions for building and parsing the management solana::Instructions. Then we can add Lido-specific commands.

I’ll wait for the current code to be reviewed, and then I’ll organize it like this. After that I can continue adding management options to the CLI. Possibly we could even merge the Multisig and Lido CLI programs, so multisig becomes a subcommand of solido.

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

No branches or pull requests

5 participants