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 key assignment ('key delegation') #26

Closed
4 tasks
AdityaSripal opened this issue Sep 14, 2021 · 25 comments · Fixed by #515
Closed
4 tasks

Validator key assignment ('key delegation') #26

AdityaSripal opened this issue Sep 14, 2021 · 25 comments · Fixed by #515
Assignees
Labels
help wanted Open for all. You do not need permission to work on these. type: feature-request New feature or request improvement

Comments

@AdityaSripal
Copy link
Member

Allow Validators to set a different consensus key for each baby chain

  • Create Msg type
  • Implement setting handler (should always override previous value)
  • Default to same consensus key as parent chain
  • In Endblocker, make the transformation from parent keys to baby keys in the change set before sending the packet
@crodriguezvega
Copy link

@AdityaSripal is this an issue that should move to the interchain-security repo?

@AdityaSripal
Copy link
Member Author

Yes thanks

@AdityaSripal AdityaSripal transferred this issue from cosmos/ibc-go Dec 2, 2021
@mpoke mpoke added type: feature-request New feature or request improvement scope: cosmos-sdk Integration with Cosmos SDK ccv and removed scope: cosmos-sdk Integration with Cosmos SDK labels Apr 26, 2022
@mpoke
Copy link
Contributor

mpoke commented Apr 29, 2022

This will be a user tx on the provider CCV module, i.e., CreateKey(valAddr, consumerChainId).

@jtremback jtremback changed the title Create mapping from Validator to their preferred consensus key on baby chain Validator key delegation Apr 29, 2022
@jtremback jtremback assigned jtremback and unassigned jtremback Apr 29, 2022
@jtremback
Copy link
Contributor

jtremback commented Apr 29, 2022

Jack's comment- a validator set change of more than 1/3 will break all the IBC light clients, so if validators with more than 1/3 of the power delegate their keys in one block, it will break the light clients connected to consumer chains. @AdityaSripal any ideas around this?

@mpoke
Copy link
Contributor

mpoke commented Apr 30, 2022

I'm not sure I understand. I thought that the validators can choose their consensus keys for a given chain before the chain is initialized. Can validators do this while validating on the consumer chain? What would be the benefit of that?

@jackzampolin
Copy link
Member

Key rotation is a nice to have feature but could present problems. Setting the key prior to consumer chain start should be fine.

@mpoke
Copy link
Contributor

mpoke commented May 5, 2022

@okwme Is this feature (each validator having a different consensus key for each consumer chain) something we need for V1 or is it a "nice to have"?

@mohammedpatla
Copy link

Key rotation is a nice to have feature but could present problems. Setting the key prior to consumer chain start should be fine.

Honestly waiting for key rotation option.
If this was available it would be more open to reusing priv key across child chains. For now we spin a new one for every chain.

@tac0turtle
Copy link
Member

We are thinking of providing the expected_keeper interface for the ccv module as such:

type LookUpModule interface {
 GetLookupList() []entry
 GetUserEntry(address sdk.Address) entry 
}

Does this make sense from the ccv point of view?

(names aren't final)

@tac0turtle
Copy link
Member

QQ: If a validator joins the set how long would it take the update to propagate to N chains? The issue arises that once a validator joins the active set how long do they have to start spinning up on consumer chains, this would be the deciding factor for how this gets implemented for me

@okwme
Copy link
Contributor

okwme commented May 6, 2022

If a validator joins the set, this should trigger an update to the child chain so it could be quite fast. Faster than the val might have to submit a new key for the child chain. However, maybe it shoudl be possible to associate keys before joining the val set? Also probably shouldn't try to validate whether a key is or is not currently a validator for the same reason. Would this improve the scenario you were worried about @marbar3778 ?

@tac0turtle
Copy link
Member

yea that would solve it. I guess the docs would say when becoming a validator if you have not registered a child chain consensus key we will use the default one. The outstanding question is if a validator changes it after the validator is created on the child chain are there security assumptions to think about?

@tac0turtle
Copy link
Member

tac0turtle commented May 9, 2022

easiest would be fail on create-validator message if there are no entries for that validator in the key lookup table. This would mean we require validators to not use the same key.

@mpoke mpoke added product and removed ccv-core labels May 10, 2022
@okwme
Copy link
Contributor

okwme commented May 12, 2022

I think this is solved if there was a store with Keys of operator PKs and Values of operator PKs. Provider Chain validators would register (or not) their consumer chain PK. These are used to send voting power to the consumer over CCV. A provider operator can change the consumer PK as often as they want. Once updated, they will need to use this new key as part of their node as soon as the update packet has landed on the child chain (they could just as well run two nodes with each PK as one will be ignored until the change happens without risk of double signing). If they consumer chain sends rewards to the parent chain but the parent has recently updated their PK, this is also not a problem, as the previous consumer PK is still a Key in the store pointing to the provider PK. It will be redundant with the new consumer PK that is also pointing to the provider PK, but that's not a problem. We may want to consider a garbage collection that is some period after it is assumed the new key has landed on the consumer chain.

the key of the store is for operators of both consumer and provider. The value is both consumer and provider. There is no collision on the store key even though there may be redundant entries in the store value.

@okwme
Copy link
Contributor

okwme commented May 12, 2022

easiest would be fail on create-validator message if there are no entries for that validator in the key lookup table. This would mean we require validators to not use the same key.

how would this work for validators that are already in the set? Shouldn't we fail over to the same key if they did not register one?

The child chain won't begin producing blocks until 2/3 of the validators come online. It is same to assume that after a proposal passes, these validators won't turn on their nodes until after registering their PKs.

@okwme
Copy link
Contributor

okwme commented May 12, 2022

actually, there is no need to store consumer chain PKs in the store at all.
the only reason would be to associate a consumer PK with a provider PK in order to pay rewards, but this isn't necessary as the rewards go straight to the distribution module where the correct PKs are already present.

@mpoke
Copy link
Contributor

mpoke commented May 13, 2022

actually, there is no need to store consumer chain PKs in the store at all.
the only reason would be to associate a consumer PK with a provider PK in order to pay rewards, but this isn't necessary as the rewards go straight to the distribution module where the correct PKs are already present.

@okwme We still need the mapping from consumer PKs to provider PKs to match SlashPackets to the validators on the provider.

@mpoke
Copy link
Contributor

mpoke commented May 13, 2022

Provider Chain validators would register (or not) their consumer chain PK. These are used to send voting power to the consumer over CCV. A provider operator can change the consumer PK as often as they want.

This may lead to the problem pointed by Jack, i.e., #26 (comment). How does it work currently on a single chain? Can a validator change its PK while part of the validator set?

I think for the first release, we should keep it simple and allow the validators to decide on their consumer chain PK only once before they join the validator set. By default is the same PK as on the provider.

@mpoke mpoke added the question label May 13, 2022
@okwme
Copy link
Contributor

okwme commented May 13, 2022

if it's required then it's also fine to have a general look up table like i first described and the validator can change as often as they want to with no problem.

If the lookup table is initially like this:

Key Value
PK Provider A PK Consumer A
PK Consumer A PK Provider A

and PK Provider A changes their Consumer PK it would just look like this afterwards:

Key Value
PK Provider A PK Consumer B
PK Consumer A PK Provider A
PK Consumer B PK Provider A

query is always up to date whether you're using the old Consumer PK or the new Consumer PK it is still pointing to the correct Provider PK.

@mpoke
Copy link
Contributor

mpoke commented May 13, 2022

Yeah, but on the consumer chain, we may have the case that in two subsequent blocks the validators corresponding to more than 1/3 of the voting power change their PKs. As a result, the client to the consumer chain can no longer be updated.

My question still stands:

In the current version of SDK, can a validator change its PK while part of the validator set?

@mpoke mpoke added the help wanted Open for all. You do not need permission to work on these. label Jun 8, 2022
@mpoke
Copy link
Contributor

mpoke commented Jul 12, 2022

After discussions within the team, the consensus was to allow every validator to change its consensus key on any consumer chain at any point in time. The issue discussed in #26 (comment) can be address by the relayer switching to the sequential protocol for updating the client.

@danwt
Copy link
Contributor

danwt commented Sep 8, 2022

Presumably the mapping should not be opaque? It seems likely that people will want to know which validators on the consumers map to which provider validators and vice versa. Thus we'll need queries/RPC for this mapping, and perhaps additional metadata anyways on the consumer for each validator.

Have I got it right?

Please see related

@danwt
Copy link
Contributor

danwt commented Sep 21, 2022

I made a prototype design, it's a WIP.

It's entirely provider side, the consumer doesn't need to know anything.

@danwt
Copy link
Contributor

danwt commented Oct 27, 2022

I screwed up a bit. With the key delegation I currently have, everything seems to work perfectly except for one thing:

The design takes place so far entirely on the provider side (literally, no consumer code is changed). The provider takes care of mapping tendermint updates through the assigned keys, and forwarding them to the consumer. When a consumer slash packet arrives on the provider, the correct key is looked up, and the validator gets slashed accordingly. So far so good, all seems to work perfectly.

Except, problem is, I oversaw the following, until now: outstandingDowntime. When a validator changes their key for a consumer, it is possible for outstandingDowntime[oldKey]=true and outstandingDowntime[newKey] is going to return false.

I see some solutions, some of which change external behavior, some not.

  1. Change semantics, warn validators that when they change key they can be slashed for downtime twice, potentially, when they change keys. Validators should not change key often, and it would be difficult for it to happen because they'd have to be down for sufficiently many blocks in two signing windows (because after you change key it's a new validator from tendermint perspective). Relevant code
  2. Simplify the system: remove outstandingDowntime as a concept
  3. Simplify the system: remove outstandingDowntime as a concept, and make it so that jailed validators cannot be slashed on the provider, or similar. Please see bullet two here

a validator cannot get slashed multiple times for downtime on the same chain without having to unjail itself first;
and this issue
- #417
I particularly like Shawns idea here #417 (comment)

  1. Simplify the system: remove outstandingDowntime as a concept, leave protection up to the new Jailing circuit breaker feature
  1. Some combination of 2/3/4!
  1. Move most of the logic for keymap, that is currently on the provider, to the consumer, so that the consumer itself can map consAddr -> providerKey when it receives a slash request. Note that it must to be possible to handle slash requests from when the assigned key was different. In the current design the provider takes care of that with pruning, so the pruning should also move to the consumer with this option.

I'm fond of clarifying and simplifying how we deal with various slash scenarios. In particular see

I'd also like to keep the entire keymap logic provider side, as it is now. Aside from the mentioned problem, everything seems to be working well, incl. diff tests.

EDIT:
Regarding outstandingDowntime the spec

The mapping enables the consumer CCV module to avoid sending to the provider chain multiple slashing requests for the same downtime infraction.
The problem I described is for separate infractions so in this sense it's really my test (diff test) that is wrong, and not the solution.

Note that the sdk will not trigger two downtime slashes in a row without forcing two downtime windows to elapse
https://github.com/cosmos/cosmos-sdk/blob/fc38dc277a66080122372ef32cc01a4de1142526/x/slashing/keeper/infractions.go#L108-L111

@danwt danwt changed the title Validator key delegation Validator key assignment ('key delegation') Nov 8, 2022
@danwt danwt mentioned this issue Nov 8, 2022
@mpoke mpoke assigned mpoke and unassigned danwt Nov 23, 2022
@mpoke mpoke mentioned this issue Nov 24, 2022
22 tasks
@danwt
Copy link
Contributor

danwt commented Dec 1, 2022

TODOs

@okwme here is a quick summary. I will flesh out tomorrow morning. I recommend reading this doc to figure out what is going on.

@mpoke mpoke closed this as completed in #515 Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Open for all. You do not need permission to work on these. type: feature-request New feature or request improvement
Projects
No open projects
Status: Done
9 participants