-
Notifications
You must be signed in to change notification settings - Fork 102
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
docs: Add draft ADR for validators outside of the active set #1879
base: main
Are you sure you want to change the base?
Conversation
Note Reviews PausedUse the following commands to manage reviews:
WalkthroughThe newly introduced Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
@coderabbitai pause |
@p-offtermatt: Understood, I will pause the review for this PR. Actions PerformedReviews paused. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
||
## References | ||
|
||
* [adr-016-securityaggregation.md] has similar concerns where the staking validator set will differ from the consensus validator set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The [adr-016-securityaggregation.md]
does not point anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, will fix!
I believe I understand the general idea: Increase the However I was thinking the following: Unbonded validators can opt in or assign a consumer key to a consumer chain. So, we could send the unbonded validators' public keys down to the consumer chains. For those validators to actually secure a consumer chain we need:
For 1. couldn't we just slash those unbonded validators on our own (without utilizing directly the A final note on the approach: Do we want to have a way for a validator to be able to secure a consumer chain without being forced to secure the Hub? If not, then it might make sense to simply increase the validator-set size on the Hub. After having a discussion with @MSalopek, it seems that increasing the validator-set size would lead to an increase in block times and this makes sense. However, it's not clear to me how big that increase would be. If it is in the range of a few hundred milliseconds, then just increasing the validator-set might be a viable approach (?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just discussing this with someone the other day, really great idea. At worst you are improving decentralization, at best you are rewarding actual contributors that oltherwise get overlooked.
this is extremely cool |
We discussed offline, but just to summarize:
I think we want a solution where the potential validator set for consumer chains could be thousands of validators. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @p-offtermatt. See my comments below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait for #1910 to be merged and update intro.md
### Negative | ||
|
||
* We need to be very careful in making sure all existing references to the validator set are updated to refer to the "actual active set" of the provider chain if necessary | ||
* In the worst case, this might mean we need many changes to existing Cosmos SDK modules (and this would negate one of the positives of this solution) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove.
We could instead adapt the *staking module* with a similar change. | ||
This might be better if it turns out that the staking module active set is used in many other places. | ||
|
||
### Allowing unbonding validators to validate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would stop a set of unbonded validators from double signing on a consumer chain and withdraw all their ATOMs before the double sining evidence reaches the Hub?
|
||
## References | ||
|
||
* [Security Aggregation](./adr-016-securityaggregation.md) has similar concerns where the staking validator set will differ from the consensus validator set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace "the consensus validator set" with "the validator set provided to the consensus engine". Or define at the beginning of the ADR what you mean by the consensus validator set.
I changed it to draft as I think it's still very much work in progress. |
Co-authored-by: Marius Poke <marius.poke@posteo.de>
Co-authored-by: Marius Poke <marius.poke@posteo.de>
Co-authored-by: Marius Poke <marius.poke@posteo.de>
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
docs:
prefix in the PR titleReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
docs:
prefix in the PR titlemake build-docs
)Summary by CodeRabbit