chainflip-io / chainflip-backend

The Chainflip backend repo, including the Chainflip Node and CFE.
50 stars 15 forks source link

[CH-1822] - Adding a Key Identifier to Vault Events #442

Closed kylezs closed 3 years ago

kylezs commented 3 years ago

CH-1822

The vaults pallet currently doesn't contain a notion of "KeyId" which is used to identify which key to sign with, within the signing module. It currently emits a RequestIndex alongside the relevant data.

RequestIndex is not the best identifier of a key, because it doesn't allow us to use old keys to sign.

For example. If we generate a key on RequestIndex = 0 and then RequestIndex = 2 , therefore making the effective KeyId = 2 and then on request 7, we wish to sign with the old key (effective KeyId = 0). We cannot currently do this in the current setup.

A few ideas to fix this (definitely not an exhaustive list):

  1. Introduce an explicit KeyId - previously this was the AuctionIndex but I believe this was only supposed to be a temporary thing.
  2. Emit the Agg Pubkey we wish to sign with. The CFE would use this as the "KeyId"

Some relevant previous discussions:

msgmaxim commented 3 years ago

Don't have a strong opinion here. One small advantage of (1) is that CFE can use the same key id during the keygen ceremony as well as for signing (it is known before the key is ready). (2) appears to be more clear though, I would probably lean towards it.

kylezs commented 3 years ago

So, had a chat to @andyjsbell and I realised that this would be pretty pointless. At the very least, at this point in time. Even if we can sign with the old keys, and construct a valid Agg Sig, it would be for the old agg sig... and therefore the contract would reject the tx, since it will have already rotated to the new sig, by definition.

We should just always be signing with the most recent key. If this changes later e.g. with the introduction of funds and vault rotations, we can change it then.

dandanlen commented 3 years ago

The issue is that the signer doesn't know when a newly generated key becomes valid since it will generate a new key k_1 and then needs to sign the rotation transaction with the previous key k_0. k_1 only becomes the signing key once the rotation transaction has been processed by the smart contract. So there is a (short) period of time when k_0 is still the 'current' key even though k_1 is the most recent one.

morelazers commented 3 years ago

the contract would reject the tx, since it will have already rotated to the new sig

Good point. In the case of ETH the Current Set should always have access to deposited funds due to our usage of CREATE2.

In BTC (and other UTXO chains), this is not the case, and funds deposited to addresses created by the Outgoing Set would need to be recovered by that set, and cannot be recovered by the Current Set.

morelazers commented 3 years ago

@dandanlen is there a case for pausing signing between witnessing the new key, and witnessing the confirmation of the transaction to set the new key?

dandanlen commented 3 years ago

Yes I think we should avoid sending out any transactions during the rotation phase, too much potential for things to get out-of-order.

My point was more that the rule "always sign with the most recent key" doesn't work for the aggKey rotation since the rotation transaction needs to be signed with the old key.

So at the very least, either the SC or the CFE needs to be able to tell the Signer to sign with the previous key instead of the latest. I think this was the original motivation for introducing the idea of a keyid.

edit: note we might eventually need this anyway for retrieving erroneous transfers to old vaults

morelazers commented 3 years ago

So at the very least, either the SC or the CFE needs to be able to tell the Signer to sign with the previous key instead of the latest.

Or, is the latest key only updated once the key rotation transaction to the KeyManager contract is witnessed, and until then sits in some pending state?

We definitely want the ability to tell the Validators which key to sign with, yes.

dandanlen commented 3 years ago

The signer doesn't and shouldn't need to know anything about the state of the system. It should just sign what we tell it to. I don't think we can get around having the key_id as an input param to the signer.

kylezs commented 3 years ago

My point was more that the rule "always sign with the most recent key" doesn't work for the aggKey rotation since the rotation transaction needs to be signed with the old key.

Depends how you define most recent key. I'm talking about the most recent key that is actually valid i.e. can sign things that are accepted by the contract / map to the current active Pub Agg Key. One that is not yet valid, is not the latest/most recent, but the "soon to be latest". Sure, we have technically generated a new key, but I'd say that, to the signing module, this should have no effect on how it signs, until, like Tom says, we've witnessed this (and it's been confirmed by the SC).

This removes the need for KeyId.

dandanlen commented 3 years ago

How does the signer know which is the latest valid key?

msgmaxim commented 3 years ago

Keep in mind that for Bitcoin at least, there will be cases where there are two active vaults (50% rotation) and the signer/validator (who happened to win two auctions in a row) would need to be able to sign with two different keys for some non-trivial amount of time.

andyjsbell commented 3 years ago

Addressed in #473