Add key rotation support #4422
Add key rotation support #4422
Conversation
karlem
force-pushed
the
feature/key-rotation
branch
from
5767b1c
to
10fa132
Compare
karlem
force-pushed
the
feature/key-rotation
branch
from
80fca09
to
40b37bd
Compare
Co-authored-by: Jeremy Letang <me@jeremyletang.com>
karlem
force-pushed
the
feature/key-rotation
branch
from
28821ba
to
ec59463
Compare
karlem
changed the title
| @@ -207,3 +207,37 @@ func (a *Accounting) GetAvailableBalanceInRange( | |||
| func (a *Accounting) GetStakingAssetTotalSupply() *num.Uint { | |||
| return a.stakingAssetTotalSupply.Clone() | |||
| } | |||
|
|
|||
| func (a *Accounting) ValidatorKeyChanged(ctx context.Context, oldKey, newKey string) { | |||
There was a problem hiding this comment.
@ze97286 Thinking about how this affects governance voting: if a vote was put in with oldKey, and then the key was rotated between the vote and the closing of a proposal, the vote will become invalid because there is no longer any balance when we come to count the votes. Is this the expected behaviour?
And actually in this situation we will panic because we will call .IsZero() on nil. Previously governace would not expect a party that existed at voting time to disappear later on. Can this be fixed?: https://github.com/vegaprotocol/vega/blob/develop/governance/engine.go#L792
There was a problem hiding this comment.
I think we have two options -
option 1) do the same thing we do for the other engine - listen to key rotation and update the votes.
option 2) use the nodeid in votes from validator and map it to the current key when going to check balance
Somewhat aside I think it sucks that we're using something that's bound to change like the pub key as the identity of validators or even regular parties. I'm not sure why we're doing that but it doesn't seem right.
There was a problem hiding this comment.
For fear of this PR growing too big and stalling, feel free to just make a ticket for the governance bit to be done afterwards.
And yea I agree about using public keys as identifiers everywhere being a bit annoying. Feels like we need a big pubkey -> stable-partyID map somewhere that can be refered too when the transaction is processed. Then they'll only ever be one place to update when keys are rotated. Because at the moment it feels like we're just guessing. But I guess thats for another day :p
There was a problem hiding this comment.
I think we should fix, I personally would rather stall than have half working half broken code in develop.
There was a problem hiding this comment.
@wwestgarth I agree with you mate. We have talked about having a central mapping at one place already. It's something we should probably create an issue for and get it into sprint in the future. (Although it will bring some challenges from migrating current state point of view)
There was a problem hiding this comment.
So I'm not sure I get the whole matter here, but it's totally possible for a party to have have tokens at the time of the vote, and none at the time the vote is closing, and it's always gonna be possible, that's why we do the final counting of votes at the end.
Now about that pubkey. for parties it's fine that's their identity on the network. For validators it's shite, true, but if they rotate their key, it's their problem to also address changing the votes etc. But I'd be interested to hear about other problem (which there might be for sure)?
There was a problem hiding this comment.
it's possible for a party not to have tokens at the time of counting, it's should not be possible for a party not to have a staking account (because it's validated at the time the vote is received) which will be the case with the current code. So we either just protect against it and say, sorry validators you lost your vote, or we update their vote party when their key is rotated.
There was a problem hiding this comment.
We want key-rotation to be as frictionless as possible. Key-rotation is something that should be done regularly and routinely. It involves coordination, planning and accessing HSM's. So I don't think it is right to penalise the validators, and have them lose votes, for having good security practises.
Also, does the spam engine need to rotate keys too? I think currently if a validator is blocked by a spam policy for 4 epochs for being naughty, a tactical key-rotation means they get their voting/proposal priviledges back?
There was a problem hiding this comment.
@wwestgarth re spam engine, I thought about it, but I don't think it's worth it tbh.
…nto feature/key-rotation
Close #4139