Remove the concept of a "zero Height" (#2346)

Prior to this PR, the `Height` type used a special `Height::zero()` value to denote the absence of height value. This caused ad hoc checks of whether a height is zero in various places.

From now on, we instead use `Option<Height>` on places where the height value may be optional, and require non-zero value in the `Height` constructor.

- Optional height parameters should use `Option<Height>` instead of `Height`.
- Checking of `height == Height::zero()` is replaced with Option matching.

---

* From<Height> for String fix

* Use Option<Height> instead of Height::zero for consensus_height

* recv_packet still had Height::zero() in events

* TryFrom<RawHeight> for Height

* pre height parse changes

* Packet.timeout_height is now an Option<Height>

* rustdoc

* Remove Height::with_revision_height

* commit pre-modifying cli

* Finish Height::new() returns Result

* remove Height::is_zero()

* Remove `Height::zero()`

* Remove Height::default()

* Height fields accessors

* use revision_height accessor

* use revision_number accessor

* make Height fields private (WIP)

* compile error fixes

* FIXME in transfer.rs addressed

* Use existing constants instead of hardcoded strings

* changelog

* TimeoutHeight newtype

* Use TimeoutHeight everywhere

* Fixes after merging with master

* has_expired() fix

* doc url

* fix send_packet test

* Remove timeout_height == 0 && timestamp == 0 check

* Remove `has_timeout()`

* Change TimeoutHeight impl

* docstrings

* tests fix

* fix history manipulation test

* transfer test: no timeout

* msgtransfer test

* packet no timeout or timestamp test

* send_packet timeout height equal destination chain height

* send_packet test with timeout = height - 1

* test invalid timeout height

* fix docstring

* clippy

* Fix clippy warnings introduced in Rust 1.62

* Fix serialization of `TimeoutHeight` to match the format used by `Height`

Co-authored-by: Romain Ruetschi <romain@informal.systems>

.changelog/unreleased/improvements/ibc/1009-height-zero-complete.md

@@ -0,0 +1,2 @@
+- Remove the concept of a zero Height
+  ([#1009](https://github.com/informalsystems/ibc-rs/issues/1009))

e2e/e2e/common.py

@@ -10,6 +10,12 @@ class Height:
     revision_number: int
 
 
+@dataclass
+class TimeoutHeight:
+    revision_height: int
+    revision_number: int
+
+
 @dataclass
 class Duration:
     nanos: int
@@ -39,6 +45,7 @@ class Ordering(Enum):
 ClientType = NewType('ClientType', str)
 BlockHeight = NewType('BlockHeight', str)
 
+
 def split():
     sleep(0.5)
     print()

e2e/e2e/packet.py

@@ -12,7 +12,7 @@ class Packet:
     destination_port: PortId
     destination_channel: ChannelId
     data: Hex
-    timeout_height: Height
+    timeout_height: TimeoutHeight
     timeout_timestamp: Timestamp
 
 

modules/src/applications/transfer/msgs/transfer.rs

@@ -8,7 +8,7 @@ use ibc_proto::ibc::applications::transfer::v1::MsgTransfer as RawMsgTransfer;
 use tendermint_proto::Protobuf;
 
 use crate::applications::transfer::error::Error;
-use crate::core::ics02_client::height::Height;
+use crate::core::ics04_channel::timeout::TimeoutHeight;
 use crate::core::ics24_host::identifier::{ChannelId, PortId};
 use crate::signer::Signer;
 use crate::timestamp::Timestamp;
@@ -37,7 +37,7 @@ pub struct MsgTransfer<C = Coin> {
     pub receiver: Signer,
     /// Timeout height relative to the current block height.
     /// The timeout is disabled when set to None.
-    pub timeout_height: Option<Height>,
+    pub timeout_height: TimeoutHeight,
     /// Timeout timestamp relative to the current block timestamp.
     /// The timeout is disabled when set to 0.
     pub timeout_timestamp: Timestamp,
@@ -63,13 +63,9 @@ impl TryFrom<RawMsgTransfer> for MsgTransfer {
         let timeout_timestamp = Timestamp::from_nanoseconds(raw_msg.timeout_timestamp)
             .map_err(|_| Error::invalid_packet_timeout_timestamp(raw_msg.timeout_timestamp))?;
 
-        let timeout_height: Option<Height> = raw_msg
-            .timeout_height
-            .map(|raw_height| raw_height.try_into())
-            .transpose()
-            .map_err(|e| {
-                Error::invalid_packet_timeout_height(format!("invalid timeout height {}", e))
-            })?;
+        let timeout_height: TimeoutHeight = raw_msg.timeout_height.try_into().map_err(|e| {
+            Error::invalid_packet_timeout_height(format!("invalid timeout height {}", e))
+        })?;
 
         Ok(MsgTransfer {
             source_port: raw_msg
@@ -97,7 +93,7 @@ impl From<MsgTransfer> for RawMsgTransfer {
             token: Some(domain_msg.token),
             sender: domain_msg.sender.to_string(),
             receiver: domain_msg.receiver.to_string(),
-            timeout_height: domain_msg.timeout_height.map(|height| height.into()),
+            timeout_height: domain_msg.timeout_height.into(),
             timeout_timestamp: domain_msg.timeout_timestamp.nanoseconds(),
         }
     }
@@ -134,17 +130,21 @@ pub mod test_util {
 
     use super::MsgTransfer;
     use crate::bigint::U256;
+    use crate::core::ics04_channel::timeout::TimeoutHeight;
     use crate::signer::Signer;
     use crate::{
         applications::transfer::{BaseCoin, PrefixedCoin},
         core::ics24_host::identifier::{ChannelId, PortId},
         test_utils::get_dummy_bech32_account,
         timestamp::Timestamp,
-        Height,
     };
 
-    // Returns a dummy ICS20 `MsgTransfer`, for testing only!
-    pub fn get_dummy_msg_transfer(height: u64) -> MsgTransfer<PrefixedCoin> {
+    // Returns a dummy ICS20 `MsgTransfer`. If no `timeout_timestamp` is
+    // specified, a timestamp of 10 seconds in the future is used.
+    pub fn get_dummy_msg_transfer(
+        timeout_height: TimeoutHeight,
+        timeout_timestamp: Option<Timestamp>,
+    ) -> MsgTransfer<PrefixedCoin> {
         let address: Signer = get_dummy_bech32_account().as_str().parse().unwrap();
         MsgTransfer {
             source_port: PortId::default(),
@@ -156,11 +156,9 @@ pub mod test_util {
             .into(),
             sender: address.clone(),
             receiver: address,
-            timeout_timestamp: Timestamp::now().add(Duration::from_secs(10)).unwrap(),
-            timeout_height: Some(Height {
-                revision_number: 0,
-                revision_height: height,
-            }),
+            timeout_timestamp: timeout_timestamp
+                .unwrap_or_else(|| Timestamp::now().add(Duration::from_secs(10)).unwrap()),
+            timeout_height,
         }
     }
 }

modules/src/applications/transfer/relay/send_transfer.rs

@@ -4,7 +4,6 @@ use crate::applications::transfer::events::TransferEvent;
 use crate::applications::transfer::msgs::transfer::MsgTransfer;
 use crate::applications::transfer::packet::PacketData;
 use crate::applications::transfer::{is_sender_chain_source, Coin, PrefixedCoin};
-use crate::core::ics02_client::height::Height;
 use crate::core::ics04_channel::handler::send_packet::send_packet;
 use crate::core::ics04_channel::packet::Packet;
 use crate::events::ModuleEvent;
@@ -81,7 +80,7 @@ where
         destination_port,
         destination_channel,
         data,
-        timeout_height: msg.timeout_height.unwrap_or_else(Height::zero),
+        timeout_height: msg.timeout_height,
         timeout_timestamp: msg.timeout_timestamp,
     };
 

modules/src/clients/ics07_tendermint/client_def.rs

@@ -53,11 +53,11 @@ impl ClientDef for TendermintClient {
         client_state: Self::ClientState,
         header: Self::Header,
     ) -> Result<(Self::ClientState, Self::ConsensusState), Ics02Error> {
-        if header.height().revision_number != client_state.chain_id.version() {
+        if header.height().revision_number() != client_state.chain_id.version() {
             return Err(Ics02Error::tendermint_handler_error(
                 Error::mismatched_revisions(
                     client_state.chain_id.version(),
-                    header.height().revision_number,
+                    header.height().revision_number(),
                 ),
             ));
         }
@@ -88,11 +88,11 @@ impl ClientDef for TendermintClient {
             header_time: trusted_consensus_state.timestamp,
             height: header
                 .trusted_height
-                .revision_height
+                .revision_height()
                 .try_into()
                 .map_err(|_| {
                     Ics02Error::tendermint_handler_error(Error::invalid_header_height(
-                        header.trusted_height,
+                        header.trusted_height.revision_height(),
                     ))
                 })?,
             next_validators: &header.trusted_validator_set,
@@ -185,7 +185,7 @@ impl ClientDef for TendermintClient {
         }
 
         Ok((
-            client_state.with_header(header.clone()),
+            client_state.with_header(header.clone())?,
             ConsensusState::from(header),
         ))
     }
@@ -205,8 +205,8 @@ impl ClientDef for TendermintClient {
 
         let path = ClientConsensusStatePath {
             client_id: client_id.clone(),
-            epoch: consensus_height.revision_number,
-            height: consensus_height.revision_height,
+            epoch: consensus_height.revision_number(),
+            height: consensus_height.revision_height(),
         };
         let value = expected_consensus_state
             .encode_vec()

modules/src/clients/ics07_tendermint/client_state.rs

@@ -7,6 +7,7 @@ use serde::{Deserialize, Serialize};
 use tendermint_light_client_verifier::options::Options;
 use tendermint_proto::Protobuf;
 
+use ibc_proto::ibc::core::client::v1::Height as RawHeight;
 use ibc_proto::ibc::lightclients::tendermint::v1::ClientState as RawClientState;
 
 use crate::clients::ics07_tendermint::error::Error;
@@ -77,13 +78,6 @@ impl ClientState {
             )));
         }
 
-        // Basic validation for the latest_height parameter.
-        if latest_height <= Height::zero() {
-            return Err(Error::validation(
-                "ClientState latest height must be greater than zero".to_string(),
-            ));
-        }
-
         // `TrustThreshold` is guaranteed to be in the range `[0, 1)`, but a `TrustThreshold::ZERO`
         // value is invalid in this context
         if trust_level == TrustThreshold::ZERO {
@@ -117,22 +111,18 @@ impl ClientState {
         self.latest_height
     }
 
-    pub fn with_header(self, h: Header) -> Self {
-        // TODO: Clarify which fields should update.
-        ClientState {
-            latest_height: self
-                .latest_height
-                .with_revision_height(u64::from(h.signed_header.header.height)),
+    pub fn with_header(self, h: Header) -> Result<Self, Error> {
+        Ok(ClientState {
+            latest_height: Height::new(
+                self.latest_height.revision_number(),
+                h.signed_header.header.height.into(),
+            )
+            .map_err(|_| Error::invalid_header_height(h.signed_header.header.height.value()))?,
             ..self
-        }
+        })
     }
 
     pub fn with_frozen_height(self, h: Height) -> Result<Self, Error> {
-        if h == Height::zero() {
-            return Err(Error::validation(
-                "ClientState frozen height must be greater than zero".to_string(),
-            ));
-        }
         Ok(Self {
             frozen_height: Some(h),
             ..self
@@ -264,14 +254,12 @@ impl TryFrom<RawClientState> for ClientState {
             .clone()
             .ok_or_else(Error::missing_trusting_period)?;
 
-        let frozen_height = raw.frozen_height.and_then(|raw_height| {
-            let height = raw_height.into();
-            if height == Height::zero() {
-                None
-            } else {
-                Some(height)
-            }
-        });
+        // In `RawClientState`, a `frozen_height` of `0` means "not frozen".
+        // See:
+        // https://github.com/cosmos/ibc-go/blob/8422d0c4c35ef970539466c5bdec1cd27369bab3/modules/light-clients/07-tendermint/types/client_state.go#L74
+        let frozen_height = raw
+            .frozen_height
+            .and_then(|raw_height| raw_height.try_into().ok());
 
         Ok(Self {
             chain_id: ChainId::from_string(raw.chain_id.as_str()),
@@ -296,7 +284,8 @@ impl TryFrom<RawClientState> for ClientState {
             latest_height: raw
                 .latest_height
                 .ok_or_else(Error::missing_latest_height)?
-                .into(),
+                .try_into()
+                .map_err(|_| Error::missing_latest_height())?,
             frozen_height,
             upgrade_path: raw.upgrade_path,
             allow_update: AllowUpdate {
@@ -316,12 +305,17 @@ impl From<ClientState> for RawClientState {
             trusting_period: Some(value.trusting_period.into()),
             unbonding_period: Some(value.unbonding_period.into()),
             max_clock_drift: Some(value.max_clock_drift.into()),
-            frozen_height: Some(value.frozen_height.unwrap_or_else(Height::zero).into()),
+            frozen_height: Some(value.frozen_height.map(|height| height.into()).unwrap_or(
+                RawHeight {
+                    revision_number: 0,
+                    revision_height: 0,
+                },
+            )),
             latest_height: Some(value.latest_height.into()),
             proof_specs: value.proof_specs.into(),
+            upgrade_path: value.upgrade_path,
             allow_update_after_expiry: value.allow_update.after_expiry,
             allow_update_after_misbehaviour: value.allow_update.after_misbehaviour,
-            upgrade_path: value.upgrade_path,
         }
     }
 }
@@ -379,7 +373,7 @@ mod tests {
             trusting_period: Duration::new(64000, 0),
             unbonding_period: Duration::new(128000, 0),
             max_clock_drift: Duration::new(3, 0),
-            latest_height: Height::new(0, 10),
+            latest_height: Height::new(0, 10).unwrap(),
             proof_specs: ProofSpecs::default(),
             upgrade_path: vec!["".to_string()],
             allow_update: AllowUpdate {
@@ -433,14 +427,6 @@ mod tests {
                 },
                 want_pass: false,
             },
-            Test {
-                name: "Invalid (too small) latest height".to_string(),
-                params: ClientStateParams {
-                    latest_height: Height::zero(),
-                    ..default_params.clone()
-                },
-                want_pass: false,
-            },
             Test {
                 name: "Invalid (empty) proof specs".to_string(),
                 params: ClientStateParams {
@@ -502,9 +488,9 @@ mod tests {
                 name: "Successful delay verification".to_string(),
                 params: Params {
                     current_time: (now + Duration::from_nanos(2000)).unwrap(),
-                    current_height: Height::new(0, 5),
+                    current_height: Height::new(0, 5).unwrap(),
                     processed_time: (now + Duration::from_nanos(1000)).unwrap(),
-                    processed_height: Height::new(0, 3),
+                    processed_height: Height::new(0, 3).unwrap(),
                     delay_period_time: Duration::from_nanos(500),
                     delay_period_blocks: 2,
                 },
@@ -514,9 +500,9 @@ mod tests {
                 name: "Delay period(time) has not elapsed".to_string(),
                 params: Params {
                     current_time: (now + Duration::from_nanos(1200)).unwrap(),
-                    current_height: Height::new(0, 5),
+                    current_height: Height::new(0, 5).unwrap(),
                     processed_time: (now + Duration::from_nanos(1000)).unwrap(),
-                    processed_height: Height::new(0, 3),
+                    processed_height: Height::new(0, 3).unwrap(),
                     delay_period_time: Duration::from_nanos(500),
                     delay_period_blocks: 2,
                 },
@@ -526,9 +512,9 @@ mod tests {
                 name: "Delay period(blocks) has not elapsed".to_string(),
                 params: Params {
                     current_time: (now + Duration::from_nanos(2000)).unwrap(),
-                    current_height: Height::new(0, 5),
+                    current_height: Height::new(0, 5).unwrap(),
                     processed_time: (now + Duration::from_nanos(1000)).unwrap(),
-                    processed_height: Height::new(0, 4),
+                    processed_height: Height::new(0, 4).unwrap(),
                     delay_period_time: Duration::from_nanos(500),
                     delay_period_blocks: 2,
                 },
@@ -566,7 +552,7 @@ mod tests {
             trusting_period: Duration::new(64000, 0),
             unbonding_period: Duration::new(128000, 0),
             max_clock_drift: Duration::new(3, 0),
-            latest_height: Height::new(1, 10),
+            latest_height: Height::new(1, 10).unwrap(),
             proof_specs: ProofSpecs::default(),
             upgrade_path: vec!["".to_string()],
             allow_update: AllowUpdate {
@@ -585,21 +571,23 @@ mod tests {
         let tests = vec![
             Test {
                 name: "Successful height verification".to_string(),
-                height: Height::new(1, 8),
+                height: Height::new(1, 8).unwrap(),
                 setup: None,
                 want_pass: true,
             },
             Test {
                 name: "Invalid (too large)  client height".to_string(),
-                height: Height::new(1, 12),
+                height: Height::new(1, 12).unwrap(),
                 setup: None,
                 want_pass: false,
             },
             Test {
                 name: "Invalid, client is frozen below current height".to_string(),
-                height: Height::new(1, 6),
+                height: Height::new(1, 6).unwrap(),
                 setup: Some(Box::new(|client_state| {
-                    client_state.with_frozen_height(Height::new(1, 5)).unwrap()
+                    client_state
+                        .with_frozen_height(Height::new(1, 5).unwrap())
+                        .unwrap()
                 })),
                 want_pass: false,
             },
@@ -660,7 +648,8 @@ pub mod test_util {
                 Height::new(
                     ChainId::chain_version(tm_header.chain_id.as_str()),
                     u64::from(tm_header.height),
-                ),
+                )
+                .unwrap(),
                 Default::default(),
                 vec!["".to_string()],
                 AllowUpdate {