[SC-2291] Invalidate broadcast on State Chain via witnessing it's AggKey nonce consumption

[dandanlen]

Description

Since we generate the threshold signature openly and submit it to the state chain, anyone (not just our validators) can build a valid transaction and broadcast it (for free, afaict, since we refund the sender).

This isn't bad in itself - we want to send it means that the signature is no longer valid and any subsequent broadcasts with the same signature will fail. With the current implementation this could send us into an infinite retry-loop unless the CFE recognizes that the transaction is no longer required an cancels it.

However note that we don't have the transaction hash in advance if we don't know the sender. So we would have to somehow monitor the chain for events and cancel the broadcast if we see that the signature has been used.

[morelazers]

We could emit an event which contains the nonce signed over by the threshold sig, that's not too tricky.

Could this slot into our existing witness processes or does this imply that we also need an extra set of witness functions?

[dandanlen]

Yeah, you mean emit an eth event from the signing code to say that a nonce has been invalidated?

We would need an extra witnessing round for this event, yes. But it might be a bit tricky to manage the race condition between this event and the "real" event.

For example.

Happy Path: Transaction accepted. => 'Broadcast success' reported. We get two events:

• NonceInvalidated => Do nothing, this is normal? Witness it anyway and let the state chain decide what to do?
• KeyChanged => Process the event normally.

Weird Path: Transaction rejected, nonce re-use. => 'Broadcast failed 'reported. We get two events:

• NonceInvalidated => Somehow interrupt the retry loop.
• KeyChanged => Process the event normally.

Basically, we can wrap the weird case up inside one of the BroadcastFailed conditions and submit it that way, which pushes the 'logic' part to the CFE. Ie. the CFE has to distinguish between the normal and the weird case. OR We can provide a separate nonce invalidated extrinsic and the SC decides what to do based on its own view (basically, it would just make sure we never retry this broadcast).

IMO the second option is cleanest.

[morelazers]

Yeah, you mean emit an eth event from the signing code to say that a nonce has been invalidated?

Something like that, basically adding an extra event to the function which invalidates the nonce, separate to whatever the signature is being used for.

event SignatureAccepted(SigData _sigData, address _broadcaster); where broadcaster is set to tx.origin, the "gas-paying" account.

The SigData struct currently is

struct SigData {
    uint msgHash;
    uint sig;
    uint nonce;
    address kTimesGAddr;
}

This should give more than enough information about what has been accepted by the contract.

Re. the weird case, we would get the KeyChanged event before the BroadcastFailed condition was met, since the event to change the key must have been emitted before the subsequent transaction reverted. I think we don't really care that the State-Chain-scheduled Broadcast failed in this case, but we should probably make a note of the fact that it was not the nominated Broadcaster's signed transaction which ended up being accepted (hence the addition of the broadcaster param to the new event).

Ordering-wise, I think you're right that yes, once we know on the State Chain that the nonce has been invalidated (by witnessing the new SignatureAccepted event) then we just ensure that any attempt or attempted-retries for that broadcast are aborted.

In this case, the nominated Broadcaster has not done anything wrong, as long as they submit their signed transaction back to the chain we should not be in any danger of penalising them.

WDYT?

[dandanlen]

That all sounds good to me.

I think the conclusion then is this: We need to emit an event from the signature verification code on ethereum and this event is witnessed and forwarded to the SC to a new extrinsic on the broadcast pallet.

I don't think we need the originator of the tx included in the event - we don't care who it was, just that it happened and therefore we can short-circuit the retry loop. But it might be good to have it there for informational purposes anyhow.

[morelazers]

Yeah I think we want the data just to be aware of it - we should chuck it on the chain and the block explorer can say "not a validator" for the sake of having an audit trail and preventing too much headscratching.

I will make the Ethereum event at some stage, have made a SC story here.

Can you add the other relevant stories to the Epic? I've attached it to Sandstorm because I don't think it's particularly important yet.

[morelazers]

Ethereum work is complete on the branch: https://github.com/chainflip-io/chainflip-eth-contracts/tree/pre-release/sandstorm

[Janislav]

I think the solution to implement this on the rust side of things (state-chain and CFE) could work like this:

state-chain:

1) Add a new witness extrinsic in the broadcast pallet called signature_accepted 2) When the signature_accepted extrinsic reached the threshold we add the sig data to an array 3) In the on_initialize hook we iterate over the array and call basically the same code as in the transmission_success extrinsic (after we figured out which transaction based on the sig data)

CFE:

1) Handle the SignatureAccepted event in the KeyManager struct 2) Call the witness function (signature_accepted) on the state-chain with sig data

[Janislav]

Do you think it's fine to change the event in the KeyManager contract to event SignatureAccepted(msgHash). I think this is the only thing we need. Also happy to do it👌.

[morelazers]

I would prefer to emit the whole thing from the contract, as well as the tx.origin. The State Chain shouldn't care what the contract emits, only that we can get the correct data into the witness function, which we can do with the above spec.

[Janislav]

One question: I need some test data for the CFE integration. At the moment I don't think the ABI in the PR is updated. Moreover, I found some hints in the comments of the CFE that I have to generate test data by running the deploy_and.py script. Any advice for this?

[morelazers]

The ABI should be the correct one on the eth-contracts branch pre-release/sandstorm @Janislav

[Janislav]

Was done in this PR: https://github.com/chainflip-io/chainflip-backend/pull/1344

[Janislav]

The CFE part is still to be done - I guess. We have to witness the event. But because we've to support the new contracts in the CFE anyway we also have to do some other stuff that @j4m1ef0rd was working on. There should be an open PR for this as well.