Emit event upon setting upgrade consensus state

[colin-axner]

Summary

When we set the upgrade consensus state, we should emit an event so relayers know that they can now upgrade the counterparty client. Unsure if this needs to be backported since I don't know if hermes can listen to events in begin/end blocker without the next tendermint version

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[technicallyty]

i think this can be closed, https://github.com/cosmos/ibc-go/blob/main/modules/core/02-client/keeper/client.go#L191-L198 ? if not lmk, not sure if thats the right function in question here

[colin-axner]

No, we need the entire consensus state to be emitted when it is being set before a chain upgrades. The event emitted is on the counterparty chain (which didn't upgrade). It is emitting an event for the processing of a message which conveys proof of the counterparty upgrade.

Chain A upgrades (setting client/consensus states)
Chain B processed MsgUpgradeClient which shows proof of chain A upgrading

[adizere]

This issue is a dependency for informalsystems/hermes#1148.

Unsure if this needs to be backported since I don't know if hermes can listen to events in begin/end blocker without the next tendermint version

Can confirm that Hermes at the moment can listen & act on begin/end block events (informalsystems/hermes#1793).

[colin-axner]

The event function used should something like look like:

func EmitUpgradeChainEvent(ctx sdk.Context, height int64) {
	ctx.EventManager().EmitEvents(sdk.Events{
		sdk.NewEvent(
			types.EventTypeUpgradeChain,
			sdk.NewAttribute(types.AttributeKeyPlanHeight, height), 
	                sdk.NewAttribute(types.AttributeKeyUpgradeStore, upgradetypes.StoreKey), // which store to query proof of consensus state from
		),
	})
}

Is any other information needed @adizere? Will relayers know which counterparty clients belong to this chain that is upgrading?

[adizere]

This looks good at first glance.

Is there a way we can actually test this in practice and reproduce the event emission? I'm asking because it's difficult to say a firm yes/no whether further info would be necessary unless we run through the whole exercise end-to-end. Would be glad to use simapp to test this new event, or is there another way? Any instructions would be appreciated!

Will relayers know which counterparty clients belong to this chain that is upgrading?
I think relayers should know yes; they should have support for tracking the relevant counterparty clients that need to be upgraded.

[crodriguezvega]

Is there a way we can actually test this in practice and reproduce the event emission? I'm asking because it's difficult to say a firm yes/no whether further info would be necessary unless we run through the whole exercise end-to-end. Would be glad to use simapp to test this new event, or is there another way? Any instructions would be appreciated!

Yes, we can make the changes in a branch and you could build simapp from that branch to test.

[catShaark]

Can we test this using test unit instead of running the whole thing e2e ?

[crodriguezvega]

Can we test this using test unit instead of running the whole thing e2e ?

It's a bit overkill to assert in the tests that the right values are emitted in the events (i.e. you need to loop through a []abci.Event and for each event loop through again a []abci.EventAttribute to find the attributes you want to assert). So I think in this case it's not really worth it...

[colin-axner]

Not possible with a unit test yet. I tried taking a stab at it in #1169, but some testing package changes are needed before we can do that.

I'd add the event without tests for now. Hermes can test using the event whenever they are ready to do so, if something is off, we can do a patch release (events are non state machine breaking)

[damiannolan]

closed by #1741