Hermes worker improvements (follow-up to #1066)

[adizere]

A few thoughts triggered by this:

• we initially discussed to start uni path workers on packet events and not for every open channel. I am now wondering, from the perspective of this issue/ PR, if it's not better to start them for open channels that have valid cross-referenced IDs and NOT start them for packet events anymore.
  • this would mean that every time a chanel is open-open (i.e. on confirm events) we start the two uni path workers if channel is valid (as per restore check in this PR)
  • we may end up with a lot of passive workers if the channels are not active but
  • the advantage is that we don't churn on packet events (create worker and immediately stop if the check fails)
• with the two chains with same name, hermes client worker detects misbehavior but submission fails as the two chains are not really forks (i.e. there is not common ancestor consensus state). The worker reports an error in the log but continues to run, skipping misbehaviour checking, for the purpose of client refresh which will fail of course.
  • the original design was that when the MisbehaviorEvent is received, the worker submits the evidence to the full node and exits which is fine, but
  • in this case though the evidence submission fails (so no event on the wire to let the client worker exit). We do not check the returned event (ChainError versus MisbehaviorEvent). It would probably be good to check and exit (so we don't try some update client later on refresh timeout)
  • if you try hermes create connection ibc-0 --client-a 07-tendermint-1 --client-b 07-tendermint-0 hermes retries a number of times with errors like this:
    Jun 14 15:55:34.525 ERROR ibc_relayer::connection: Failed ConnTry ConnectionSide { chain: ProdChainHandle { chain_id: ChainId { id: "ibc-1", version: 1 }, runtime_sender: Sender { .. } }, client_id: ClientId("07-tendermint-0"), connection_id: ConnectionId("connection-0") }: failed with underlying cause: tx response error: deliver_tx reports error: log=Log("failed to execute message; message index: 1: connection handshake open try failed: failed consensus state verification for client (07-tendermint-0): chained membership proof failed to verify membership of value: 0A2E2F6962632E6C69676874636C69656E74732E74656E6465726D696E742E76312E436F6E73656E737573537461746512540A0C088FC49E860610A8EF97CF0212220A20B23FEDC50B4967BEDDBCE52D9FB566BD95C390D04B63A8279D1830C4356CA10B1A200E65CB2CF370D4DA97FDAB1C81413C75D6E3655E4B87ABC172C1B52795325F7B in subroot 58703300193DCC0B2D296DBD5BD446C9569534E5AFEF33956A57206BD5C31A1B at index 0. Please ensure the path and value are both correct.: invalid proof")
  • This could be avoided if hermes would blacklist 07-tendermint-1 on ibc-0. Similarly it shouldn't try with frozen or expired clients...not sure this is checked.

Originally posted by @ancazamfir in #1066 (comment)

[adizere]

Not specific for worker(s), but important:

I did some tests on cosmoshub with updates in the middle with older and newer trusted states. All looks good.
Not related to this PR but one thing I noticed is that there are no traces coming from the light client and sometimes the hermes update client seems to be stuck when it involves hundreds of headers. There is no feedback to the user.
And this also reminds me of the other aspect brought up by DEX about MsgUpdateClient not being rewarded at all (even with the fee proposal) and therefore hermes potentially paying a lot for updates on clients, especially on malicious chains. I think we need to address that in a separate PR.

Originally posted by @ancazamfir in #1058 (comment)

[adizere]

From @ancazamfir:

i think this PR (#1091) will need a follow up to have a client update as first message in packet transactions. As part of this, I think we should maybe change the send_msgs() API to take Vec<Msg> instead of vec of already serialized msgs, i.e. Vec<Any> that we have today. This would also help with logging in the chain impl. But it’s a big change that touches every worker.

[adizere]

Another corner-case noticed while testing for #1141: it's possible that Hermes starts a channel worker for a pair of chains, and for that worker to be prevented from advancing the channel open handshake protocol because of filtering constraints. In this case, the worker may do a step of the handshake, but then will just sit around doing nothing.

Concrete example:

• take two chains ibc-0 and ibc-1
• make sure strategy = 'all'
• configure ibc-1 to have channel filter active with type allow for say channel-0
• create a channel between the two chains, the uni-path worker should start and work fine, since the created channel will have id channel-0
  • so far so good
• trigger the opening handshake for a new channel, with Init step to ibc-0
  • hermes tx raw chan-open-init ibc-0 ibc-1 connection-0 transfer transfer
• the worker for completing this handshake should start up, and successfully advance the state of the new channel on ibc-1:
  • send_tx: broadcast_tx_sync to ibc-1 ... OpenTryChannel.
  • suppose that the channel has id channel-1
  • now filtering will kick in and will ignore the event which ibc-1 generated:

Jul 07 10:48:44.670 TRACE skipping events for 'channel-1/transfer:ibc-1 -> ibc-0'. reason: filtering is enabled and channel does not match any allowed channels

In this case, the channel worker has no idea that it cannot ever progress. It should exit.