Decouple `msg_update_client` method from `query_latest_ibc_events`

[Farhad-Shabani]

Part of #93

Description

With Tendermint consensus engines, when we have a transaction in block H that changes the state to X, Tendermint supports proofs by including the related application hash to block H+1. So to get light-client proof for that new state X, we should wait for the new block to be committed. Then we can call for msg_update_client Steps (methods) the hyperspace relayer already takes before sending a message to the counterparty chain:

1. query_latest_ibc_events > Collect ibc events of block H 1.2. query_latest_ibc_events > Create msg_update_client for block H
2. parse_events > Parsing ibc events 2.2. parse_events > Fetching proofs & putting together messages to be sent
3. process_finality_event > Prepend themsg_update_client to the others
4. flush_message_batch > Submit msgs to the chain

Tendermint needs this logic refactored to a more generic one. Before creating the msg_update_client, we must get proofs and wait for one block generation to ensure proofs (that we fetched from the current block) will be included, as following steps:

1. query_latest_ibc_events > Collect block H ibc events of block H
2. parse_events > Parsing ibc events 2.2. parse_events > Fetching proofs and putting out messages to be sent
3. build_update_client_msg > While/loop to wait for the next block creation 3.1. build_update_client_msg Create msg_update_client according to block H + 1
4. process_finality_event > Prepend themsg_update_client to the others
5. flush_message_batch > Submit msgs to the chains

Reference

https://github.com/ComposableFi/centauri/blob/master/hyperspace/core/src/macros.rs

[Wizdave97]

@Farhad-Shabani Instead of refactoring and waiting for block H + 1, why not fetch events for block H - 1 when we receive block H instead, so the proofs are available instantly?

[Farhad-Shabani]

Possible, but the thing is, we primarily listen to events through a WebSocket, getting current events (unless you are saying to poll events instead of socket connection?), and with that one height difference, we should put a wait somewhere. Maybe it could be placed inside the finality_notification() method or the relaying loop.

[Wizdave97]

We should still use the web socket to listen for new block events, but when we receive the event for a block H, can we just use the rpc or grpc to query for the events for block H - 1 and the assoiciated proofs?

[Farhad-Shabani]

Yeah, doable. let me check something with you, by finality_notification(), are we supposed to get notified by each block creation or just by emitted IBC event? As if it's the latter one, there might be situations you will not have an event for a while, so there wouldn't be a notification to trigger the processing of block H

[Wizdave97]

Each block creation

[Farhad-Shabani]

Alright, thanks. Let me have a final check on this

[Farhad-Shabani]

Another case with this issue is related to situations there may be pending events from the past several blocks/heights. Basically, the relayer processes them all together upon receiving a new notification and prepends a single MsgUpdateClient to the response messages. It might not work since events of each height require a separate MsgUpdateClient to get proofs approved. So there could be several msg_update_client() calls for a single query_latest_ibc_events() @Wizdave97 Let me know if you think it's manageable.

[Wizdave97]

@Farhad-Shabani We can modify query_latest_ibc_events to return (Vec<Any>, Vec<IbcEvent>, UpdateType) where the first item in the tuple is a batch of update client messages.

[Farhad-Shabani]

This way, we either have to change the second return from Vec<IbcEvent> to Vec<Vec<IbcEvent>>, so can relate each Any with corresponding events of the same height/block , or add another function to do something like that afterward, before prepending Any to ready_packets of each height

I think, the following should also be refactored: https://github.com/ComposableFi/centauri/blob/282906cbd2ddca769e997eb4b05aed324498c156/hyperspace/core/src/macros.rs#L75-L76

[Wizdave97]

I don't think Vec<Vec<IbcEvent>> is necessary as along as the update client messages are processed before events, there would be no issues, there's no need to relate update client messages with events at the relayer level.

[Farhad-Shabani]

Am I missing something? Didn't catch that how we will manage finding and pushing each of Anys (built from MsgUpdateClient) to the events of same height, so you can flush each batch, having the client state at its proper height. Consider events of height 10, 11 for example:

query_latest_ibc_events -- if return Vector of Any and IbcEvent as discussed -->

(vec![Any11, Any12], vec![IbcEvent10_1, IbcEvent10_2, IbcEvent11_1], update_hedear) 

---- needs a conversion somewhere to get followings --->

vec![Any11, IbcEvent10_1, IbcEvent10_2]
vec![Any12, IbcEvent11_1]

[Wizdave97]

I'm saying if we submit the following, it should still work fine vec![Any11, Any12, IbcEvent10_1, IbcEvent10_2, IbcEvent11_1] Since the update messages will still be processed first because they come first in the batch.

[Farhad-Shabani]

Alright. Got it. Let me try it out on the light version

[Farhad-Shabani]

Seems this issue has been handled in #227