search

chainflip-io / chainflip-backend

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

Case for if ETH `set_agg_key_with_agg_key` fails on the Contract #449

Closed kylezs closed 3 years ago

kylezs commented 3 years ago

Related: https://github.com/chainflip-io/chainflip-backend/issues/440 in particular, this: https://github.com/chainflip-io/chainflip-backend/pull/443#issuecomment-905263987

We need some method of waiting for witnesses of the set_agg_key_with_agg_key tx before we actually confirm / do the rotation.

Currently in cf-vaults/src/lib.rs we have the response of a successful (apparently, the node can lie here) broadcast handled like:

ChainParams::Ethereum(_) => EthereumChain::<T>::vault_rotated(Vault {
                            old_key,
                            new_key,
                            tx,
                        }),

And then we just update the vault:

/// The vault for this chain has been rotated and we store this vault to storage
    fn vault_rotated(vault: Vault<Self::PublicKey, Self::Transaction>) {
        EthereumVault::<T>::set(vault);
    }

Only one node needs to make this call for the vault to be updated and rotated, but there should be consensus here, in case it fails.

The consensus could be done in two ways (that I can think of):

  1. Assume success but wait a reasonable number of blocks before committing to the rotation - enough blocks that we can be pretty sure a node would have witnessed a failure if the tx failed, or wasn't even broadcast.

  2. Go through a "standard" witnessing procedure.

I think 2 is the cleaner choice, since we do this procedure pretty much everywhere else. We can probably think of ways to optimise this later, by being optimistic in some cases.

morelazers commented 3 years ago

Witness consensus seems clean to me, this extrinsic should be triggered by a vote.

dandanlen commented 3 years ago

It looks like this is already enforcing witness consensus on the extrinsic that is used to report the vault rotation response.

kylezs commented 3 years ago

Is that actually being witnessed though? The Req/Rep model here: https://swimlanes.io/u/Ptsk3nVj-

Has just a vault_rotation_response(). This is returned by the broadcasting node, with a tx_hash. At the moment, incorrectly, everyone is broadcasting the tx (this will be fixed in https://github.com/chainflip-io/chainflip-backend/pull/414). There is nothing actually "witnessed" here, no information is sent from the KeyManager witness, the component that's watching the contract itself, back to the SC as part of this process.

Maybe the swimlanes should look more like:

https://swimlanes.io/d/8jCqcT2kD

In particular, the CFE is actually doing a witness of the event on the contract before replying that they are happy to rotate the vault.

@andyjsbell does this seem correct?

andyjsbell commented 3 years ago

Can this now be closed @kylezs ? https://github.com/chainflip-io/chainflip-backend/pull/473

kylezs commented 3 years ago

I think so, I think the specific question is actually kind of invalid now - the case I'm thinking of may still exist in the current implementation, but I think a combination of Dan's broadcasting PR and the corresponding CFE support does cover this.