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

Validator Sets + ink! implementation of a multisig tracker #53

Merged
merged 15 commits into from
Jul 22, 2022

Conversation

kayabaNerve
Copy link
Member

@kayabaNerve kayabaNerve commented Jul 18, 2022

#52 is relevant as this is already potentially deprecated, yet I do believe this is still a solid first step.

Does document how the processor should react to this, and is tested on the success route.

cc @noot who wanted to leave comments on a PR.

cc @TheArchitect108 as it seems we may want this to be multi-multisig from the get go (just asking for your quick feedback on if we should do that here and now or...).

@kayabaNerve kayabaNerve added feature New feature or request substrate labels Jul 18, 2022
@TheArchitect108
Copy link
Collaborator

Yeah might as well do it as a part of this PR.

@kayabaNerve
Copy link
Member Author

kayabaNerve commented Jul 19, 2022

This then largely becomes merged with the pallet move, as we need to coordinate which validator is in which multisig which will be non-ink dependent. Immediately, to keep progress, it can be shimmed by adding yet another key + exposed function but...

@kayabaNerve
Copy link
Member Author

kayabaNerve commented Jul 19, 2022

I'm running the benchmarks overnight (single-threaded, hence overnight). I believe 500 nodes would be just over a minute to key gen, and ~5s to sign (each) based on my initial math. Accordingly, a single multisig should be fine? The remaining question would be if/when we move specific coins to specific validators to scale better there. That's not directly incompatible with this design though, as BTC could be Secp256k1-0 while ETH would be Secp256k1-1. Regardless, I'd like to drop multiple multisigs from consideration overall for now, once my benchmark finishes and confirms my rough math.

@kayabaNerve
Copy link
Member Author

kayabaNerve commented Jul 19, 2022

running 1 test
test tests::literal::kp256::secp256k1_non_ietf has been running for over 60 seconds
KeyGen: 7s per participant for 500 participants
Sign: 0s per participant for 500 participants
test tests::literal::kp256::secp256k1_non_ietf ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 3724.77s

I vote we drop all multi-multisig conversations until after launch.

EDIT: I divved the sign time by 500. This only had 334 parties signing as that was the threshold. Sign was still <0.7s per participant though.

contracts/multisig/lib.rs Outdated Show resolved Hide resolved
contracts/multisig/lib.rs Outdated Show resolved Hide resolved
contracts/multisig/lib.rs Outdated Show resolved Hide resolved
@kayabaNerve kayabaNerve marked this pull request as draft July 20, 2022 07:02
via an additive offset created by hashing the network's name (among other
things). The network's key is used for all coins on that network.

Networks are not acknowledged by the Serai network, solely by the processor.
Copy link
Member Author

Choose a reason for hiding this comment

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

This could be moved under Serai, with the benefit that validator sets would be defined over Networks, not over Coins. This is a level of indirection though, when handling Oraclizations, despite slightly cleaning the validator set definition (and ensuring that property now), So... pros and cons? It may be worth to move it on chain, have that literal impl, it may be worth it to leave it off chain, as it has no direct reason to be on?

@@ -0,0 +1,25 @@
# Oraclization (message)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an ugly as hell document which is out of scope for this PR. I just moved it out of a document I did correct under this PR.

@kayabaNerve kayabaNerve changed the title ink! implementation of a multisig tracker Validator Sets + ink! implementation of a multisig tracker Jul 20, 2022
keys remain valid for the validator set until it is changed. If a validator set
remains consistent despite the global validator set updating, their keys carry.
If a validator set adds a new member, and then loses them, their historical keys
are not reused.
Copy link
Member Author

Choose a reason for hiding this comment

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

This is decently annoying to implement. We can't index from H(validators) to keys due to this last property, which was intended to let nodes move to another node without their history and obligations (at least, for this specific issue). We also can't index by global set ID, validator set index as the validator set may be unchanged across global set ID changes. Considering the multisig contract just tracks the most recent keys until updated, we could simply have the processor NOP in this case, handling it off chain? Reduce dev work and consensus obligations?

@kayabaNerve kayabaNerve marked this pull request as ready for review July 20, 2022 08:59
@kayabaNerve
Copy link
Member Author

This PR did end up defining Validator Sets, as this ink! contract needed to be built with those considerations in mind. Bit of scope creep, yet I'm fine just moving forward there.

@kayabaNerve kayabaNerve mentioned this pull request Jul 21, 2022
docs/protocol/Validator Sets.md Outdated Show resolved Hide resolved
from 0.
- `bond` (Amount): Amount of bond per key-share of this validator set.
- `coins` (Vec<Coin>): Coins managed by this validator set.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a status for the set?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather simply append in hard forks, and if we ever remove for some reason, set coins.len to 0. I don't see a benefit for an additional status as a protocol level, especially since rewards would be based on their managed coins (encouraging validators to provide bond where users need it), so no coins would cause it to have no effect. Seems more like an RPC comment. Feel free to clarify why though :)

docs/protocol/Validators.md Outdated Show resolved Hide resolved
@kayabaNerve kayabaNerve merged commit 9f6eb20 into develop Jul 22, 2022
@kayabaNerve kayabaNerve deleted the ink-multisig branch July 22, 2022 04:30
@kayabaNerve kayabaNerve added this to the Protonet milestone Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants