Misbehaviour check not working due to downcast to tendermint header failure

[ancazamfir]

Summary of Bug

Misbehavior checking doesn't work anymore due to downcast_header() failure for Tendermint header.

Version

master

Steps to Reproduce

Run two chains (using gaiad v7.0.0) with misbehavior enabled, instrument code to show clear errors.
Create a channel. Start hermes and you will see some errors. While hermes is started issue client update to see misbehavior checking on a single header.

hermes update client --host-chain ibc-0 --client 07-tendermint-0

For example with these diffs you will see:

2022-10-17T09:11:11.032270Z ERROR ThreadId(41) spawn:chain{chain=ibc-0}:client{client=07-tendermint-0}:connection{connection=connection-0}:channel{channel=channel-0}:worker.client.misbehaviour{client=07-tendermint-0 src_chain=ibc-1 dst_chain=ibc-0}:foreign_client.detect_misbehaviour_and_submit_evidence{update_event=Some(UpdateClient { common: Attributes { client_id: ClientId("07-tendermint-0"), client_type: Tendermint, consensus_height: Height { revision: 1, height: 1056 } }, header: Some(Tendermint( Header {...})) }) client=ibc-1->ibc-0:07-tendermint-0}:foreign_client.detect_misbehaviour{client=ibc-1->ibc-0:07-tendermint-0 update_height=Some(Height { revision: 1, height: 1056 })}: downcast failed for Tendermint( Header {...})
2022-10-17T09:11:11.032743Z ERROR ThreadId(43) spawn:chain{chain=ibc-0}:client{client=07-tendermint-0}:connection{connection=connection-0}:channel{channel=channel-0}:worker.client.misbehaviour{client=07-tendermint-0 src_chain=ibc-1 dst_chain=ibc-0}:foreign_client.detect_misbehaviour_and_submit_evidence{update_event=Some(UpdateClient { common: Attributes { client_id: ClientId("07-tendermint-0"), client_type: Tendermint, consensus_height: Height { revision: 1, height: 1056 } }, header: Some(Tendermint( Header {...})) }) client=ibc-1->ibc-0:07-tendermint-0}: cannot check misbehavior on Some(UpdateClient { common: Attributes { client_id: ClientId("07-tendermint-0"), client_type: Tendermint, consensus_height: Height { revision: 1, height: 1056 } }, header: Some(Tendermint( Header {...})) }): error raised while checking for misbehaviour evidence: failed to check misbehaviour for 07-tendermint-0 at consensus height 1-1056: error raised while submitting the misbehaviour evidence: header type incompatible for chain ibc-1
2022-10-17T09:11:11.032991Z TRACE ThreadId(43) spawn:chain{chain=ibc-0}:client{client=07-tendermint-0}:connection{connection=connection-0}:channel{channel=channel-0}:worker.client.misbehaviour{client=07-tendermint-0 src_chain=ibc-1 dst_chain=ibc-0}: detect misbehavior result: CannotExecute
2022-10-17T09:11:11.033128Z DEBUG ThreadId(43) spawn:chain{chain=ibc-0}:client{client=07-tendermint-0}:connection{connection=connection-0}:channel{channel=channel-0}:worker.client.misbehaviour{client=07-tendermint-0 src_chain=ibc-1 dst_chain=ibc-0}: aborting task

Acceptance Criteria

Misbehavior can run on tendermint chains

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate milestone (priority) applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[romac]

Interestingly, the cast seems to go through if done unsafely (via the inline version of https://doc.rust-lang.org/stable/std/any/trait.Any.html#method.downcast_ref_unchecked):

diff --git a/crates/relayer-types/src/core/ics02_client/header.rs b/crates/relayer-types/src/core/ics02_client/header.rs
index 99f05c886..cb5e923bd 100644
--- a/crates/relayer-types/src/core/ics02_client/header.rs
+++ b/crates/relayer-types/src/core/ics02_client/header.rs
@@ -52,6 +52,8 @@ dyn_clone::clone_trait_object!(Header);
 erased_serde::serialize_trait_object!(Header);

 pub fn downcast_header<H: Header>(h: &dyn Header) -> Option<&H> {
-     h.as_any().downcast_ref::<H>()
+     let a = h.as_any();
+     Some(unsafe { &*(a as *const dyn std::any::Any as *const H) })
 }

[romac]

To me, the best fix here is to move back to an AnyClient enum.

[plafer]

Interestingly, the cast seems to go through if done unsafely

What is that diff exactly? downcast_ref() has a self.is::<T>() check wrapping the unsafe code

[plafer]

I was not able to reproduce:

hermes update client --host-chain ibc-0 --client 07-tendermint-1
2022-10-20T14:49:54.197322Z  INFO ThreadId(01) using default configuration from '/Users/plafer/.hermes/config.toml'
2022-10-20T14:49:54.211877Z TRACE ThreadId(35) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:foreign_client.wait_and_build_update_client_with_trusted{client=ibc-1->ibc-0:07-tendermint-1 target_height=1-153}:foreign_client.build_update_client_with_trusted{client=ibc-1->ibc-0:07-tendermint-1 target_height=1-153}: light client verification trusted=1-101 target=1-153
2022-10-20T14:49:54.217699Z TRACE ThreadId(35) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:foreign_client.wait_and_build_update_client_with_trusted{client=ibc-1->ibc-0:07-tendermint-1 target_height=1-153}:foreign_client.build_update_client_with_trusted{client=ibc-1->ibc-0:07-tendermint-1 target_height=1-153}: adjusting headers with 0 supporting headers trusted=1-101 target=153
2022-10-20T14:49:54.217719Z TRACE ThreadId(35) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:foreign_client.wait_and_build_update_client_with_trusted{client=ibc-1->ibc-0:07-tendermint-1 target_height=1-153}:foreign_client.build_update_client_with_trusted{client=ibc-1->ibc-0:07-tendermint-1 target_height=1-153}: fetching header height=1-102
2022-10-20T14:49:54.224806Z DEBUG ThreadId(01) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:foreign_client.wait_and_build_update_client_with_trusted{client=ibc-1->ibc-0:07-tendermint-1 target_height=1-153}:foreign_client.build_update_client_with_trusted{client=ibc-1->ibc-0:07-tendermint-1 target_height=1-153}: building a MsgUpdateAnyClient from trusted height 1-101 to target height 1-153
2022-10-20T14:49:54.226772Z DEBUG ThreadId(13) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:send_messages_and_wait_commit{chain=ibc-0 tracking_id=update client}: sending 1 messages as 1 batches to chain ibc-0 in parallel
2022-10-20T14:49:54.226794Z DEBUG ThreadId(13) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:send_messages_and_wait_commit{chain=ibc-0 tracking_id=update client}:send_tx_with_account_sequence_retry{chain=ibc-0 account.sequence=10}: max fee, for use in tx simulation: Fee { amount: "100001stake", gas_limit: 10000000 }
2022-10-20T14:49:54.231110Z DEBUG ThreadId(13) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:send_messages_and_wait_commit{chain=ibc-0 tracking_id=update client}:send_tx_with_account_sequence_retry{chain=ibc-0 account.sequence=10}:estimate_gas: tx simulation successful, gas amount used: 88368
2022-10-20T14:49:54.231159Z DEBUG ThreadId(13) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:send_messages_and_wait_commit{chain=ibc-0 tracking_id=update client}:send_tx_with_account_sequence_retry{chain=ibc-0 account.sequence=10}: send_tx: using 88368 gas, fee Fee { amount: "973stake", gas_limit: 97204 } id=ibc-0
2022-10-20T14:49:54.233053Z DEBUG ThreadId(13) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:send_messages_and_wait_commit{chain=ibc-0 tracking_id=update client}:send_tx_with_account_sequence_retry{chain=ibc-0 account.sequence=10}: gas estimation succeeded
2022-10-20T14:49:54.233067Z DEBUG ThreadId(13) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:send_messages_and_wait_commit{chain=ibc-0 tracking_id=update client}:send_tx_with_account_sequence_retry{chain=ibc-0 account.sequence=10}: tx was successfully broadcasted, increasing account sequence number response=Response { code: Ok, data: Data([]), log: Log("[]"), hash: transaction::Hash(0DB0D03751342F050EFB13C068D88B2B047E75DF507593D2ADC1AE2161497BBA) } account.sequence.old=10 account.sequence.new=11
2022-10-20T14:49:54.233127Z DEBUG ThreadId(13) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:send_messages_and_wait_commit{chain=ibc-0 tracking_id=update client}: wait_for_block_commits: waiting for commit of tx hashes(s) 0DB0D03751342F050EFB13C068D88B2B047E75DF507593D2ADC1AE2161497BBA id=ibc-0
2022-10-20T14:49:59.420244Z TRACE ThreadId(13) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:send_messages_and_wait_commit{chain=ibc-0 tracking_id=update client}: wait_for_block_commits: retrieved 1 tx results after 5187ms id=ibc-0
SUCCESS [
    UpdateClient(
        UpdateClient {
            common: Attributes {
                client_id: ClientId(
                    "07-tendermint-1",
                ),
                client_type: Tendermint,
                consensus_height: Height {
                    revision: 1,
                    height: 30,
                },
            },
            header: Some(
                Tendermint(
                     Header {...},
                ),
            ),
        },
    ),
]

I double-checked that my configuration has misbehaviour enabled

[mode.clients]
enabled = true
refresh = true
misbehaviour = true

and used a release build to make sure that optimizations are turned on (in case that was the issue).

Did I set things up properly?

[ancazamfir]

Do you also run hermes start in parallel with the update CLI?

[plafer]

Ah I forgot to do that, my bad. Here's my output when I run hermes start

2022-10-20T14:54:28.904772Z  INFO ThreadId(01) Hermes has started
2022-10-20T14:54:29.286453Z  WARN ThreadId(33) spawn:chain{chain=ibc-0}:client{client=07-tendermint-1}:connection{connection=connection-1}:channel{channel=channel-0}:worker.client.misbehaviour{client=07-tendermint-1 src_chain=ibc-1 dst_chain=ibc-0}:foreign_client.detect_misbehaviour_and_submit_evidence{update_event=None client=ibc-1->ibc-0:07-tendermint-1}: misbehaviour checking result: error raised while checking for misbehaviour evidence: failed to check misbehaviour for 07-tendermint-1 at consensus height 1-15: error raised while submitting the misbehaviour evidence: header type incompatible for chain ibc-1
2022-10-20T14:54:29.293100Z  WARN ThreadId(36) spawn:chain{chain=ibc-1}:client{client=07-tendermint-1}:connection{connection=connection-0}:channel{channel=channel-0}:worker.client.misbehaviour{client=07-tendermint-1 src_chain=ibc-0 dst_chain=ibc-1}:foreign_client.detect_misbehaviour_and_submit_evidence{update_event=None client=ibc-0->ibc-1:07-tendermint-1}: misbehaviour checking result: error raised while checking for misbehaviour evidence: failed to check misbehaviour for 07-tendermint-1 at consensus height 0-17: error raised while submitting the misbehaviour evidence: header type incompatible for chain ibc-0

However, when I run the update client command, I don't see your errors

./target/release/hermes update client --host-chain ibc-0 --client 07-tendermint-1
2022-10-20T14:57:23.702150Z  INFO ThreadId(01) using default configuration from '/Users/plafer/.hermes/config.toml'
2022-10-20T14:57:23.714573Z TRACE ThreadId(35) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:foreign_client.wait_and_build_update_client_with_trusted{client=ibc-1->ibc-0:07-tendermint-1 target_height=1-52}:foreign_client.build_update_client_with_trusted{client=ibc-1->ibc-0:07-tendermint-1 target_height=1-52}: light client verification trusted=1-50 target=1-52
2022-10-20T14:57:23.721645Z TRACE ThreadId(35) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:foreign_client.wait_and_build_update_client_with_trusted{client=ibc-1->ibc-0:07-tendermint-1 target_height=1-52}:foreign_client.build_update_client_with_trusted{client=ibc-1->ibc-0:07-tendermint-1 target_height=1-52}: adjusting headers with 0 supporting headers trusted=1-50 target=52
2022-10-20T14:57:23.721673Z TRACE ThreadId(35) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:foreign_client.wait_and_build_update_client_with_trusted{client=ibc-1->ibc-0:07-tendermint-1 target_height=1-52}:foreign_client.build_update_client_with_trusted{client=ibc-1->ibc-0:07-tendermint-1 target_height=1-52}: fetching header height=1-51
2022-10-20T14:57:23.733512Z DEBUG ThreadId(01) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:foreign_client.wait_and_build_update_client_with_trusted{client=ibc-1->ibc-0:07-tendermint-1 target_height=1-52}:foreign_client.build_update_client_with_trusted{client=ibc-1->ibc-0:07-tendermint-1 target_height=1-52}: building a MsgUpdateAnyClient from trusted height 1-50 to target height 1-52
2022-10-20T14:57:23.735254Z DEBUG ThreadId(13) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:send_messages_and_wait_commit{chain=ibc-0 tracking_id=update client}: sending 1 messages as 1 batches to chain ibc-0 in parallel
2022-10-20T14:57:23.735272Z DEBUG ThreadId(13) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:send_messages_and_wait_commit{chain=ibc-0 tracking_id=update client}:send_tx_with_account_sequence_retry{chain=ibc-0 account.sequence=10}: max fee, for use in tx simulation: Fee { amount: "100001stake", gas_limit: 10000000 }
2022-10-20T14:57:23.737111Z DEBUG ThreadId(13) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:send_messages_and_wait_commit{chain=ibc-0 tracking_id=update client}:send_tx_with_account_sequence_retry{chain=ibc-0 account.sequence=10}:estimate_gas: tx simulation successful, gas amount used: 88265
2022-10-20T14:57:23.737138Z DEBUG ThreadId(13) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:send_messages_and_wait_commit{chain=ibc-0 tracking_id=update client}:send_tx_with_account_sequence_retry{chain=ibc-0 account.sequence=10}: send_tx: using 88265 gas, fee Fee { amount: "971stake", gas_limit: 97091 } id=ibc-0
2022-10-20T14:57:23.738676Z DEBUG ThreadId(13) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:send_messages_and_wait_commit{chain=ibc-0 tracking_id=update client}:send_tx_with_account_sequence_retry{chain=ibc-0 account.sequence=10}: gas estimation succeeded
2022-10-20T14:57:23.738689Z DEBUG ThreadId(13) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:send_messages_and_wait_commit{chain=ibc-0 tracking_id=update client}:send_tx_with_account_sequence_retry{chain=ibc-0 account.sequence=10}: tx was successfully broadcasted, increasing account sequence number response=Response { code: Ok, data: Data([]), log: Log("[]"), hash: transaction::Hash(AEF7FAF711E963D31F680C34A521FFF33F4D89CE2586696770D48D28A95129F9) } account.sequence.old=10 account.sequence.new=11
2022-10-20T14:57:23.738751Z DEBUG ThreadId(13) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:send_messages_and_wait_commit{chain=ibc-0 tracking_id=update client}: wait_for_block_commits: waiting for commit of tx hashes(s) AEF7FAF711E963D31F680C34A521FFF33F4D89CE2586696770D48D28A95129F9 id=ibc-0
2022-10-20T14:57:26.185069Z TRACE ThreadId(13) foreign_client.build_update_client_and_send{client=ibc-1->ibc-0:07-tendermint-1 target_query_height=latest height}:send_messages_and_wait_commit{chain=ibc-0 tracking_id=update client}: wait_for_block_commits: retrieved 1 tx results after 2446ms id=ibc-0
SUCCESS [
    UpdateClient(
        UpdateClient {
            common: Attributes {
                client_id: ClientId(
                    "07-tendermint-1",
                ),
                client_type: Tendermint,
                consensus_height: Height {
                    revision: 1,
                    height: 52,
                },
            },
            header: Some(
                Tendermint(
                     Header {...},
                ),
            ),
        },
    ),
]

[ancazamfir]

Looks like you repro-ed the issue. You can see the problem in the hermes start output (header type incompatible...).
The tendermint chain light client receives a client update event from the client worker. The update event looks like this:

pub struct UpdateClient {
    pub common: Attributes,
    pub header: Option<Box<dyn Header>>,
}

This event is sent by the client worker over a crossbeam_channel to the runtime who eventually calls the light client concrete implementation for tendermint. There we try to "downcast" the Box<dyn Header> to a tendermint header:

hermes/crates/relayer/src/light_client/tendermint.rs

Lines 99 to 120 in 5630043

	fn check_misbehaviour(
	&mut self,
	update: &UpdateClient,
	client_state: &AnyClientState,
	) -> Result<Option<MisbehaviourEvidence>, Error> {
	crate::time!("light client check_misbehaviour");
	let update_header = update.header.clone().ok_or_else(|| {
	Error::misbehaviour(format!(
	"missing header in update client event {}",
	self.chain_id
	))
	})?;
	let update_header: &TmHeader =
	downcast_header(update_header.as_ref()).ok_or_else(|| {
	Error::misbehaviour(format!(
	"header type incompatible for chain {}",
	self.chain_id
	))
	})?;

With the problem @romac notes in #2739 (comment)