Unreachable MissingLocalConsensusState error in verify_consensus_proof

[vitorenesduarte]

Crate

Summary of Bug

Summary by @adizere in informalsystems/hermes#686:

in conn open try & ack handlers there are two verifications on the field msg.proofs.consensus_proof().height as follows:

1. first we verify for staleness with check_client_consensus_height, i.e., verify that this height is not older than the oldest height of the host (local) chain, nor newer than the current height of the host chain.
2. second we verify that the consensus state for this height exists in the local chain, as part of verify_consensus_proof.

I think the first verification is more of a fail-fast. While the second verification is a pre-check as part of the whole ensemble of proof verification.

Version

02aef9c

Steps to Reproduce

---

For Admin Use

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

[vitorenesduarte]

As the error is unreachable, this test has been disabled in informalsystems/hermes#686:
https://github.com/informalsystems/ibc-rs/blob/02aef9cf5055a3817579b555c84289b99d5d08a8/modules/src/ics03_connection/handler/conn_open_ack.rs#L228-L238

[adizere]

The test case for exercising this error is still disabled. I think it's a difficult one to reproduce, so we'll keep this issue open until we investigate more on how to exercise the error.

[AlianBenabdallah]

I am not sure how sure what exactly does ctx.host_oldest_height() returns. However, ABCI has a retain_height parameter (see specs and documentation).

Relying on this parameter should guarantee that every blocks with a height greater than retain_height should be stored by every validators. But there are still some edge cases :

• State Sync bootstraps a new node by syncing state machine snapshots at a given height, but not historical blocks and associated data
• Validators could decide to prune this data. From the documentation :

Use RetainHeight with caution! If all nodes in the network remove historical blocks then this data is permanently lost, and no new nodes will be able to join the network and bootstrap. Historical blocks may also be required for other purposes, e.g. auditing, replay of non-persisted heights, light client verification, and so on.

It might be possible for any validator to arbitrarily remove a historical block with a height greater than retain_height without being punished. This could happen in case of failure, misbehavior and possibly many other reasons.

If it's possible for validators then it should also be the case for full nodes.

[plafer]

I think this is now resolved since we no longer have the host_oldest_height check.

We should also remove the outdated commented-out test.