Fix overflow when ctx.host_current_height().revision_height < ctx.host_chain_history_size()

[vitorenesduarte]

Closes: #685

Description

---

For contributor use:

• Updated the Unreleased section of CHANGELOG.md with the issue.
• If applicable: Unit tests written, added test to CI.
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Updated relevant documentation (docs/) and code comments.
• Re-reviewed Files changed in the Github PR explorer.

[adizere]

Nice find, Vitor! I had two minor remarks.

[vitorenesduarte]

Thanks for the review @adizere. I've addressed your suggestions.

I'm thinking that the tests for connection open ack were relying on wrong behaviour since they started failing with this PR. I tried to fix them in 4da1d34 but there's one (now commented out) I didn't figure out how.

[codecov-io]

Codecov Report

Merging #686 (254f853) into master (b1b37f5) will increase coverage by 30.5%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    informalsystems/hermes#686      +/-   ##
=========================================
+ Coverage    13.6%   44.2%   +30.5%     
=========================================
  Files          69     150      +81     
  Lines        3752    9776    +6024     
  Branches     1374       0    -1374     
=========================================
+ Hits          513    4325    +3812     
- Misses       2618    5451    +2833     
+ Partials      621       0     -621     

Impacted Files	Coverage Δ
modules/src/address.rs	100.0% <ø> (ø)
...application/ics20_fungible_token_transfer/error.rs	0.0% <ø> (ø)
...ion/ics20_fungible_token_transfer/msgs/transfer.rs	0.0% <ø> (ø)
modules/src/events.rs	0.0% <ø> (ø)
modules/src/handler.rs	100.0% <ø> (ø)
modules/src/ics02_client/client_def.rs	48.3% <ø> (ø)
modules/src/ics02_client/client_type.rs	79.1% <ø> (+31.5%)	⬆️
modules/src/ics02_client/context.rs	100.0% <ø> (ø)
modules/src/ics02_client/error.rs	100.0% <ø> (ø)
modules/src/ics02_client/events.rs	15.7% <ø> (+15.7%)	⬆️
... and 255 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55450d8...254f853. Read the comment docs.

[vitorenesduarte]

I'm thinking that the tests for connection open ack were relying on wrong behaviour since they started failing with this PR. I tried to fix them in 4da1d34 but there's one (now commented out) I didn't figure out how.

The test that was commented out is expecting a MissingLocalConsensusState to be thrown here: https://github.com/informalsystems/ibc-rs/blob/vitor/overflow/modules/src/ics03_connection/handler/verify.rs#L167-L170

However, before this, we're already checking if the height is stale here: https://github.com/informalsystems/ibc-rs/blob/vitor/overflow/modules/src/ics03_connection/handler/verify.rs#L200-L203

If the height is not stale, then the consensus state must exist, in which case the error MissingLocalConsensusState should instead be a panic. If you agree with this analysis, I'll replace the error with a panic in a follow-up PR.

[adizere]

I'm thinking that the tests for connection open ack were relying on wrong behaviour since they started failing with this PR. I tried to fix them in 4da1d34 but there's one (now commented out) I didn't figure out how.

That may well be. Tried to consolidate tests in that module recently, but there's still some work to be done. Your improvements definitely help!

[adizere]

I'm thinking that the tests for connection open ack were relying on wrong behaviour since they started failing with this PR. I tried to fix them in 4da1d34 but there's one (now commented out) I didn't figure out how.

The test that was commented out is expecting a MissingLocalConsensusState to be thrown here: https://github.com/informalsystems/ibc-rs/blob/vitor/overflow/modules/src/ics03_connection/handler/verify.rs#L167-L170

However, before this, we're already checking if the height is stale here: https://github.com/informalsystems/ibc-rs/blob/vitor/overflow/modules/src/ics03_connection/handler/verify.rs#L200-L203

If the height is not stale, then the consensus state must exist, in which case the error MissingLocalConsensusState should instead be a panic. If you agree with this analysis, I'll replace the error with a panic in a follow-up PR.

I agree with your analysis. To summarize: 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. I agree there is some redundancy there. I would suggest to open an issue documenting this, but I wouldn't fix things with a panic (we have no panics in the modules yet, and not sure if it's a good idea to fix it like that).

[vitorenesduarte]

I would suggest to open an issue documenting this, but I wouldn't fix things with a panic (we have no panics in the modules yet, and not sure if it's a good idea to fix it like that).

I opened https://github.com/informalsystems/ibc-rs/issues/698. Once this PR gets merged, I'll update the links there to not point to my branch.

[vitorenesduarte]

I've updated cosmos/ibc-rs#33. @adizere please confirm that it reflects the discussion here.

Regarding panics, there are already some unwraps, which can also panic. My view is that both are okay as long as they are prefixed with some explanation why there are "safe".