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

stake-contract: add support for multisig #2312

Merged
merged 7 commits into from
Sep 10, 2024
Merged

stake-contract: add support for multisig #2312

merged 7 commits into from
Sep 10, 2024

Conversation

herr-seppia
Copy link
Member

@herr-seppia herr-seppia commented Sep 9, 2024

The only keys required to run a node are the so called "provisioner keys"
Basically they are a BLS keypair stored on a node machine.

If the node got compromised, the key pair can be used to move stake funds to a different address

With this PR, instead, the "provisioner keys" would only be used to sign consensus messages.
Indeed, any other operation like "unstake/withdraw" requires to be multisigned with an additional key (aka "funds key") that is enrolled during the "stake" operation

In order to not introduce any breaking change into our wallet, the current execution-core library still has Stake::new that takes just a single key, and consider the "funds key" to be the same.

Future implementation of execution-core can handle the funds key properly

Base automatically changed from block_size to master September 9, 2024 14:37
ureeves
ureeves previously approved these changes Sep 9, 2024
Copy link
Member

@ureeves ureeves left a comment

Choose a reason for hiding this comment

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

LGTM

autholykos
autholykos previously approved these changes Sep 9, 2024
Copy link
Member

@autholykos autholykos left a comment

Choose a reason for hiding this comment

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

LGTM - using BLS multisig might lead to a more pervasive change when we swap the signature scheme for transactions (for example to ECDSA), but we will think of that when we'll cross that bridge

@herr-seppia herr-seppia dismissed stale reviews from autholykos and ureeves via 7c6f3a8 September 9, 2024 16:26
@herr-seppia herr-seppia force-pushed the stake-multisig branch 3 times, most recently from ba4fef9 to 69764d4 Compare September 9, 2024 19:43
- Change `Stake` to have both funds key and account key
- Change `Stake` to use `BlsMultiSignature`
- Change `Withdraw` to use `BlsMultiSignature`
Additionally:
- change the `reward` and `slash` calls to panic if no stake is found
- change `stakes` call to return `StakeKeys` instead of `BlsPublicKey` as key
- add `get_stake_keys` call
@herr-seppia
Copy link
Member Author

herr-seppia commented Sep 9, 2024

@ureeves @autholykos
Sorry, I need another round of review

I had to add stake-contract: allow to change funds_key when restaking to not lock-in the stake to a specific key

Copy link
Member

@autholykos autholykos left a comment

Choose a reason for hiding this comment

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

LGTM

@herr-seppia herr-seppia merged commit d6bc7a6 into master Sep 10, 2024
15 checks passed
@herr-seppia herr-seppia deleted the stake-multisig branch September 10, 2024 08:10
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.

3 participants