diff --git a/.changelog/unreleased/improvements/ibc/1009-height-zero-complete.md b/.changelog/unreleased/improvements/ibc/1009-height-zero-complete.md new file mode 100644 index 0000000000..d38ac13153 --- /dev/null +++ b/.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)) \ No newline at end of file diff --git a/e2e/e2e/common.py b/e2e/e2e/common.py index 10b96b7f51..5d70991fa7 100644 --- a/e2e/e2e/common.py +++ b/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() diff --git a/e2e/e2e/packet.py b/e2e/e2e/packet.py index 84f0c705ea..54190d1a3f 100644 --- a/e2e/e2e/packet.py +++ b/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 diff --git a/modules/src/applications/transfer/msgs/transfer.rs b/modules/src/applications/transfer/msgs/transfer.rs index bb52dbf414..7d92972ce8 100644 --- a/modules/src/applications/transfer/msgs/transfer.rs +++ b/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 { pub receiver: Signer, /// Timeout height relative to the current block height. /// The timeout is disabled when set to None. - pub timeout_height: Option, + 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 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 = 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 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 { + // 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, + ) -> MsgTransfer { 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, } } } diff --git a/modules/src/applications/transfer/relay/send_transfer.rs b/modules/src/applications/transfer/relay/send_transfer.rs index 993f5ba927..6925c6776c 100644 --- a/modules/src/applications/transfer/relay/send_transfer.rs +++ b/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, }; diff --git a/modules/src/clients/ics07_tendermint/client_def.rs b/modules/src/clients/ics07_tendermint/client_def.rs index 9f81f1c2d7..d81e42e6a7 100644 --- a/modules/src/clients/ics07_tendermint/client_def.rs +++ b/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() diff --git a/modules/src/clients/ics07_tendermint/client_state.rs b/modules/src/clients/ics07_tendermint/client_state.rs index 5205b13687..76ca250e6a 100644 --- a/modules/src/clients/ics07_tendermint/client_state.rs +++ b/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 { + 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 { - 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 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 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 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 { diff --git a/modules/src/clients/ics07_tendermint/error.rs b/modules/src/clients/ics07_tendermint/error.rs index 0ef3b9f29c..d6d1c3d24b 100644 --- a/modules/src/clients/ics07_tendermint/error.rs +++ b/modules/src/clients/ics07_tendermint/error.rs @@ -81,8 +81,8 @@ define_error! { MissingLatestHeight |_| { "missing latest height" }, - MissingFrozenHeight - |_| { "missing frozen height" }, + InvalidFrozenHeight + |_| { "invalid frozen height" }, InvalidChainId { raw_value: String } @@ -174,9 +174,9 @@ define_error! { }, InvalidHeaderHeight - { height: Height } + { height: u64 } | e | { - format_args!("header height = {0} is invalid", e.height) + format_args!("header revision height = {0} is invalid", e.height) }, InvalidTrustedHeaderHeight diff --git a/modules/src/clients/ics07_tendermint/header.rs b/modules/src/clients/ics07_tendermint/header.rs index 8fe32105a5..8aaf361a0a 100644 --- a/modules/src/clients/ics07_tendermint/header.rs +++ b/modules/src/clients/ics07_tendermint/header.rs @@ -40,6 +40,7 @@ impl Header { ChainId::chain_version(self.signed_header.header.chain_id.as_str()), u64::from(self.signed_header.header.height), ) + .expect("malformed tendermint header domain type has an illegal height of 0") } pub fn compatible_with(&self, other_header: &Header) -> bool { @@ -104,8 +105,8 @@ impl TryFrom for Header { .map_err(Error::invalid_raw_header)?, trusted_height: raw .trusted_height - .ok_or_else(Error::missing_trusted_height)? - .into(), + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(Error::missing_trusted_height)?, trusted_validator_set: raw .trusted_validators .ok_or_else(Error::missing_trusted_validator_set)? @@ -113,10 +114,10 @@ impl TryFrom for Header { .map_err(Error::invalid_raw_header)?, }; - if header.height().revision_number != header.trusted_height.revision_number { + if header.height().revision_number() != header.trusted_height.revision_number() { return Err(Error::mismatched_revisions( - header.trusted_height.revision_number, - header.height().revision_number, + header.trusted_height.revision_number(), + header.height().revision_number(), )); } @@ -199,7 +200,7 @@ pub mod test_util { Header { signed_header: shdr, validator_set: vs.clone(), - trusted_height: Height::new(0, 1), + trusted_height: Height::new(0, 1).unwrap(), trusted_validator_set: vs, } } diff --git a/modules/src/core/ics02_client/client_consensus.rs b/modules/src/core/ics02_client/client_consensus.rs index a300b3d5f4..12434e0c0d 100644 --- a/modules/src/core/ics02_client/client_consensus.rs +++ b/modules/src/core/ics02_client/client_consensus.rs @@ -129,7 +129,10 @@ impl TryFrom for AnyConsensusStateWithHeight { .ok_or_else(Error::empty_consensus_state_response)?; Ok(AnyConsensusStateWithHeight { - height: value.height.ok_or_else(Error::missing_height)?.into(), + height: value + .height + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(Error::missing_height)?, consensus_state: state, }) } diff --git a/modules/src/core/ics02_client/error.rs b/modules/src/core/ics02_client/error.rs index fee8717689..a2f2f3047c 100644 --- a/modules/src/core/ics02_client/error.rs +++ b/modules/src/core/ics02_client/error.rs @@ -158,6 +158,9 @@ define_error! { [ HeightError ] | e | { format_args!("String {0} cannnot be converted to height", e.value) }, + InvalidHeight + | _ | { "revision height cannot be zero" }, + InvalidHeightResult | _ | { "height cannot end up zero or negative" }, diff --git a/modules/src/core/ics02_client/events.rs b/modules/src/core/ics02_client/events.rs index f858a7549f..56c576d903 100644 --- a/modules/src/core/ics02_client/events.rs +++ b/modules/src/core/ics02_client/events.rs @@ -131,10 +131,10 @@ pub struct Attributes { impl Default for Attributes { fn default() -> Self { Attributes { - height: Height::default(), + height: Height::new(0, 1).unwrap(), client_id: Default::default(), client_type: ClientType::Tendermint, - consensus_height: Height::default(), + consensus_height: Height::new(0, 1).unwrap(), } } } @@ -374,7 +374,7 @@ mod tests { #[test] fn client_event_to_abci_event() { - let height = Height::new(1, 1); + let height = Height::new(1, 1).unwrap(); let attributes = Attributes { height, client_id: "test_client".parse().unwrap(), diff --git a/modules/src/core/ics02_client/handler/create_client.rs b/modules/src/core/ics02_client/handler/create_client.rs index 6175147694..b428591a39 100644 --- a/modules/src/core/ics02_client/handler/create_client.rs +++ b/modules/src/core/ics02_client/handler/create_client.rs @@ -97,7 +97,7 @@ mod tests { fn test_create_client_ok() { let ctx = MockContext::default(); let signer = get_dummy_account_id(); - let height = Height::new(0, 42); + let height = Height::new(0, 42).unwrap(); let msg = MsgCreateAnyClient::new( MockClientState::new(MockHeader::new(height)).into(), @@ -141,50 +141,28 @@ mod tests { fn test_create_client_ok_multiple() { let existing_client_id = ClientId::default(); let signer = get_dummy_account_id(); - let height = Height::new(0, 80); + let height_1 = Height::new(0, 80).unwrap(); + let height_2 = Height::new(0, 42).unwrap(); + let height_3 = Height::new(0, 50).unwrap(); - let ctx = MockContext::default().with_client(&existing_client_id, height); + let ctx = MockContext::default().with_client(&existing_client_id, height_1); let create_client_msgs: Vec = vec![ MsgCreateAnyClient::new( - MockClientState::new(MockHeader::new(Height { - revision_height: 42, - ..height - })) - .into(), - MockConsensusState::new(MockHeader::new(Height { - revision_height: 42, - ..height - })) - .into(), + MockClientState::new(MockHeader::new(height_2)).into(), + MockConsensusState::new(MockHeader::new(height_2)).into(), signer.clone(), ) .unwrap(), MsgCreateAnyClient::new( - MockClientState::new(MockHeader::new(Height { - revision_height: 42, - ..height - })) - .into(), - MockConsensusState::new(MockHeader::new(Height { - revision_height: 42, - ..height - })) - .into(), + MockClientState::new(MockHeader::new(height_2)).into(), + MockConsensusState::new(MockHeader::new(height_2)).into(), signer.clone(), ) .unwrap(), MsgCreateAnyClient::new( - MockClientState::new(MockHeader::new(Height { - revision_height: 50, - ..height - })) - .into(), - MockConsensusState::new(MockHeader::new(Height { - revision_height: 50, - ..height - })) - .into(), + MockClientState::new(MockHeader::new(height_3)).into(), + MockConsensusState::new(MockHeader::new(height_3)).into(), signer, ) .unwrap(), @@ -243,7 +221,7 @@ mod tests { Duration::from_secs(64000), Duration::from_secs(128000), Duration::from_millis(3000), - Height::new(0, u64::from(tm_header.height)), + Height::new(0, u64::from(tm_header.height)).unwrap(), ProofSpecs::default(), vec!["".to_string()], AllowUpdate { diff --git a/modules/src/core/ics02_client/handler/update_client.rs b/modules/src/core/ics02_client/handler/update_client.rs index fd4e7a5fb3..b6695eb190 100644 --- a/modules/src/core/ics02_client/handler/update_client.rs +++ b/modules/src/core/ics02_client/handler/update_client.rs @@ -135,10 +135,10 @@ mod tests { let timestamp = Timestamp::now(); - let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42)); + let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42).unwrap()); let msg = MsgUpdateAnyClient { client_id: client_id.clone(), - header: MockHeader::new(Height::new(0, 46)) + header: MockHeader::new(Height::new(0, 46).unwrap()) .with_timestamp(timestamp) .into(), signer, @@ -184,11 +184,11 @@ mod tests { let client_id = ClientId::from_str("mockclient1").unwrap(); let signer = get_dummy_account_id(); - let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42)); + let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42).unwrap()); let msg = MsgUpdateAnyClient { client_id: ClientId::from_str("nonexistingclient").unwrap(), - header: MockHeader::new(Height::new(0, 46)).into(), + header: MockHeader::new(Height::new(0, 46).unwrap()).into(), signer, }; @@ -212,8 +212,8 @@ mod tests { ClientId::from_str("mockclient3").unwrap(), ]; let signer = get_dummy_account_id(); - let initial_height = Height::new(0, 45); - let update_height = Height::new(0, 49); + let initial_height = Height::new(0, 45).unwrap(); + let update_height = Height::new(0, 49).unwrap(); let mut ctx = MockContext::default(); @@ -254,14 +254,14 @@ mod tests { #[test] fn test_update_synthetic_tendermint_client_adjacent_ok() { let client_id = ClientId::new(ClientType::Tendermint, 0).unwrap(); - let client_height = Height::new(1, 20); - let update_height = Height::new(1, 21); + let client_height = Height::new(1, 20).unwrap(); + let update_height = Height::new(1, 21).unwrap(); let ctx = MockContext::new( ChainId::new("mockgaiaA".to_string(), 1), HostType::Mock, 5, - Height::new(1, 1), + Height::new(1, 1).unwrap(), ) .with_client_parametrized( &client_id, @@ -330,14 +330,14 @@ mod tests { #[test] fn test_update_synthetic_tendermint_client_non_adjacent_ok() { let client_id = ClientId::new(ClientType::Tendermint, 0).unwrap(); - let client_height = Height::new(1, 20); - let update_height = Height::new(1, 21); + let client_height = Height::new(1, 20).unwrap(); + let update_height = Height::new(1, 21).unwrap(); let ctx = MockContext::new( ChainId::new("mockgaiaA".to_string(), 1), HostType::Mock, 5, - Height::new(1, 1), + Height::new(1, 1).unwrap(), ) .with_client_parametrized_history( &client_id, @@ -358,7 +358,7 @@ mod tests { let block_ref = ctx_b.host_block(update_height); let mut latest_header: AnyHeader = block_ref.cloned().map(Into::into).unwrap(); - let trusted_height = client_height.clone().sub(1).unwrap_or_default(); + let trusted_height = client_height.clone().sub(1).unwrap(); latest_header = match latest_header { AnyHeader::Tendermint(mut theader) => { @@ -408,9 +408,9 @@ mod tests { #[test] fn test_update_synthetic_tendermint_client_duplicate_ok() { let client_id = ClientId::new(ClientType::Tendermint, 0).unwrap(); - let client_height = Height::new(1, 20); + let client_height = Height::new(1, 20).unwrap(); - let chain_start_height = Height::new(1, 11); + let chain_start_height = Height::new(1, 11).unwrap(); let ctx = MockContext::new( ChainId::new("mockgaiaA".to_string(), 1), @@ -440,7 +440,7 @@ mod tests { let cons_state = ctx.latest_consensus_states(&client_id, &client_height); if let AnyConsensusState::Tendermint(tcs) = cons_state { theader.signed_header.header.time = tcs.timestamp; - theader.trusted_height = Height::new(1, 11) + theader.trusted_height = Height::new(1, 11).unwrap() } AnyHeader::Tendermint(theader) } @@ -488,11 +488,11 @@ mod tests { #[test] fn test_update_synthetic_tendermint_client_lower_height() { let client_id = ClientId::new(ClientType::Tendermint, 0).unwrap(); - let client_height = Height::new(1, 20); + let client_height = Height::new(1, 20).unwrap(); - let client_update_height = Height::new(1, 19); + let client_update_height = Height::new(1, 19).unwrap(); - let chain_start_height = Height::new(1, 11); + let chain_start_height = Height::new(1, 11).unwrap(); let ctx = MockContext::new( ChainId::new("mockgaiaA".to_string(), 1), diff --git a/modules/src/core/ics02_client/handler/upgrade_client.rs b/modules/src/core/ics02_client/handler/upgrade_client.rs index 54e2d5caa6..c1a7e5bb5b 100644 --- a/modules/src/core/ics02_client/handler/upgrade_client.rs +++ b/modules/src/core/ics02_client/handler/upgrade_client.rs @@ -100,12 +100,13 @@ mod tests { let client_id = ClientId::default(); let signer = get_dummy_account_id(); - let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42)); + let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42).unwrap()); let msg = MsgUpgradeAnyClient { client_id: client_id.clone(), - client_state: MockClientState::new(MockHeader::new(Height::new(1, 26))).into(), - consensus_state: MockConsensusState::new(MockHeader::new(Height::new(1, 26))).into(), + client_state: MockClientState::new(MockHeader::new(Height::new(1, 26).unwrap())).into(), + consensus_state: MockConsensusState::new(MockHeader::new(Height::new(1, 26).unwrap())) + .into(), proof_upgrade_client: Default::default(), proof_upgrade_consensus_state: Default::default(), signer, @@ -146,12 +147,13 @@ mod tests { let client_id = ClientId::from_str("mockclient1").unwrap(); let signer = get_dummy_account_id(); - let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42)); + let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42).unwrap()); let msg = MsgUpgradeAnyClient { client_id: ClientId::from_str("nonexistingclient").unwrap(), - client_state: MockClientState::new(MockHeader::new(Height::new(1, 26))).into(), - consensus_state: MockConsensusState::new(MockHeader::new(Height::new(1, 26))).into(), + client_state: MockClientState::new(MockHeader::new(Height::new(1, 26).unwrap())).into(), + consensus_state: MockConsensusState::new(MockHeader::new(Height::new(1, 26).unwrap())) + .into(), proof_upgrade_client: Default::default(), proof_upgrade_consensus_state: Default::default(), signer, @@ -174,12 +176,13 @@ mod tests { let client_id = ClientId::default(); let signer = get_dummy_account_id(); - let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42)); + let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42).unwrap()); let msg = MsgUpgradeAnyClient { client_id, - client_state: MockClientState::new(MockHeader::new(Height::new(0, 26))).into(), - consensus_state: MockConsensusState::new(MockHeader::new(Height::new(0, 26))).into(), + client_state: MockClientState::new(MockHeader::new(Height::new(0, 26).unwrap())).into(), + consensus_state: MockConsensusState::new(MockHeader::new(Height::new(0, 26).unwrap())) + .into(), proof_upgrade_client: Default::default(), proof_upgrade_consensus_state: Default::default(), signer, @@ -189,7 +192,7 @@ mod tests { match output { Err(Error(ErrorDetail::LowUpgradeHeight(e), _)) => { - assert_eq!(e.upgraded_height, Height::new(0, 42)); + assert_eq!(e.upgraded_height, Height::new(0, 42).unwrap()); assert_eq!(e.client_height, msg.client_state.latest_height()); } _ => { diff --git a/modules/src/core/ics02_client/height.rs b/modules/src/core/ics02_client/height.rs index e7feae2028..dc84b3ffad 100644 --- a/modules/src/core/ics02_client/height.rs +++ b/modules/src/core/ics02_client/height.rs @@ -15,29 +15,30 @@ use crate::core::ics02_client::error::Error; #[derive(Copy, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] pub struct Height { /// Previously known as "epoch" - pub revision_number: u64, + revision_number: u64, /// The height of a block - pub revision_height: u64, + revision_height: u64, } impl Height { - pub fn new(revision_number: u64, revision_height: u64) -> Self { - Self { + pub fn new(revision_number: u64, revision_height: u64) -> Result { + if revision_height == 0 { + return Err(Error::invalid_height()); + } + + Ok(Self { revision_number, revision_height, - } + }) } - pub fn zero() -> Height { - Self { - revision_number: 0, - revision_height: 0, - } + pub fn revision_number(&self) -> u64 { + self.revision_number } - pub fn is_zero(&self) -> bool { - self.revision_height == 0 + pub fn revision_height(&self) -> u64 { + self.revision_height } pub fn add(&self, delta: u64) -> Height { @@ -65,19 +66,6 @@ impl Height { pub fn decrement(&self) -> Result { self.sub(1) } - - pub fn with_revision_height(self, revision_height: u64) -> Height { - Height { - revision_height, - ..self - } - } -} - -impl Default for Height { - fn default() -> Self { - Self::zero() - } } impl PartialOrd for Height { @@ -104,12 +92,11 @@ impl Ord for Height { impl Protobuf for Height {} -impl From for Height { - fn from(raw: RawHeight) -> Self { - Height { - revision_number: raw.revision_number, - revision_height: raw.revision_height, - } +impl TryFrom for Height { + type Error = Error; + + fn try_from(raw_height: RawHeight) -> Result { + Height::new(raw_height.revision_number, raw_height.revision_height) } } @@ -148,6 +135,8 @@ define_error! { format_args!("cannot convert into a `Height` type from string {0}", e.height) }, + ZeroHeight + |_| { "attempted to parse an invalid zero height" } } } @@ -156,20 +145,21 @@ impl TryFrom<&str> for Height { fn try_from(value: &str) -> Result { let split: Vec<&str> = value.split('-').collect(); - Ok(Height { - revision_number: split[0] - .parse::() - .map_err(|e| HeightError::height_conversion(value.to_owned(), e))?, - revision_height: split[1] - .parse::() - .map_err(|e| HeightError::height_conversion(value.to_owned(), e))?, - }) + + let revision_number = split[0] + .parse::() + .map_err(|e| HeightError::height_conversion(value.to_owned(), e))?; + let revision_height = split[1] + .parse::() + .map_err(|e| HeightError::height_conversion(value.to_owned(), e))?; + + Height::new(revision_number, revision_height).map_err(|_| HeightError::zero_height()) } } impl From for String { fn from(height: Height) -> Self { - format!("{}-{}", height.revision_number, height.revision_number) + format!("{}-{}", height.revision_number, height.revision_height) } } diff --git a/modules/src/core/ics02_client/msgs/upgrade_client.rs b/modules/src/core/ics02_client/msgs/upgrade_client.rs index 34ed86ea83..6f5b0626de 100644 --- a/modules/src/core/ics02_client/msgs/upgrade_client.rs +++ b/modules/src/core/ics02_client/msgs/upgrade_client.rs @@ -186,7 +186,7 @@ mod tests { let client_id: ClientId = "tendermint".parse().unwrap(); let signer = get_dummy_account_id(); - let height = Height::new(1, 1); + let height = Height::new(1, 1).unwrap(); let client_state = AnyClientState::Mock(MockClientState::new(MockHeader::new(height))); let consensus_state = diff --git a/modules/src/core/ics03_connection/events.rs b/modules/src/core/ics03_connection/events.rs index 702747c147..8d05d02527 100644 --- a/modules/src/core/ics03_connection/events.rs +++ b/modules/src/core/ics03_connection/events.rs @@ -71,7 +71,7 @@ fn extract_attributes_from_tx(event: &tendermint::abci::Event) -> Result, @@ -80,6 +80,18 @@ pub struct Attributes { pub counterparty_client_id: ClientId, } +impl Default for Attributes { + fn default() -> Self { + Self { + height: Height::new(0, 1).unwrap(), + connection_id: Default::default(), + client_id: Default::default(), + counterparty_connection_id: Default::default(), + counterparty_client_id: Default::default(), + } + } +} + /// Convert attributes to Tendermint ABCI tags /// /// # Note @@ -290,7 +302,7 @@ mod test { #[test] fn connection_event_to_abci_event() { - let height = Height::new(1, 1); + let height = Height::new(1, 1).unwrap(); let attributes = Attributes { height, connection_id: Some("test_connection".parse().unwrap()), diff --git a/modules/src/core/ics03_connection/handler/conn_open_ack.rs b/modules/src/core/ics03_connection/handler/conn_open_ack.rs index 086a8a9d12..3ea5ad33bc 100644 --- a/modules/src/core/ics03_connection/handler/conn_open_ack.rs +++ b/modules/src/core/ics03_connection/handler/conn_open_ack.rs @@ -19,8 +19,11 @@ pub(crate) fn process( ) -> HandlerResult { let mut output = HandlerOutput::builder(); - // Check the client's (consensus state) proof height. - check_client_consensus_height(ctx, msg.consensus_height())?; + // If a consensus proof is present, check that the consensus height (for + // client proof) in the message is not too advanced nor too old. + if let Some(consensus_height) = msg.consensus_height() { + check_client_consensus_height(ctx, consensus_height)?; + } // Validate the connection end. let mut conn_end = ctx.connection_end(&msg.connection_id)?; @@ -137,7 +140,7 @@ mod tests { let latest_height = proof_height.increment(); let max_history_size = 5; let default_context = MockContext::new( - ChainId::new("mockgaia".to_string(), latest_height.revision_number), + ChainId::new("mockgaia".to_string(), latest_height.revision_number()), HostType::Mock, max_history_size, latest_height, diff --git a/modules/src/core/ics03_connection/handler/conn_open_confirm.rs b/modules/src/core/ics03_connection/handler/conn_open_confirm.rs index 3f32cc3fad..f8ae51f90a 100644 --- a/modules/src/core/ics03_connection/handler/conn_open_confirm.rs +++ b/modules/src/core/ics03_connection/handler/conn_open_confirm.rs @@ -132,7 +132,7 @@ mod tests { name: "Processing fails due to connections mismatch (incorrect state)".to_string(), ctx: context .clone() - .with_client(&client_id, Height::new(0, 10)) + .with_client(&client_id, Height::new(0, 10).unwrap()) .with_connection(msg_confirm.connection_id.clone(), incorrect_conn_end_state), msg: ConnectionMsg::ConnectionOpenConfirm(msg_confirm.clone()), want_pass: false, @@ -140,7 +140,7 @@ mod tests { Test { name: "Processing successful".to_string(), ctx: context - .with_client(&client_id, Height::new(0, 10)) + .with_client(&client_id, Height::new(0, 10).unwrap()) .with_connection(msg_confirm.connection_id.clone(), correct_conn_end), msg: ConnectionMsg::ConnectionOpenConfirm(msg_confirm), want_pass: true, diff --git a/modules/src/core/ics03_connection/handler/conn_open_init.rs b/modules/src/core/ics03_connection/handler/conn_open_init.rs index 4cfd3d59ae..cd00d6ab76 100644 --- a/modules/src/core/ics03_connection/handler/conn_open_init.rs +++ b/modules/src/core/ics03_connection/handler/conn_open_init.rs @@ -108,9 +108,10 @@ mod tests { ..msg_conn_init_default.clone() }; let default_context = MockContext::default(); - let good_context = default_context - .clone() - .with_client(&msg_conn_init_default.client_id, Height::new(0, 10)); + let good_context = default_context.clone().with_client( + &msg_conn_init_default.client_id, + Height::new(0, 10).unwrap(), + ); let tests: Vec = vec![ Test { diff --git a/modules/src/core/ics03_connection/handler/conn_open_try.rs b/modules/src/core/ics03_connection/handler/conn_open_try.rs index b9b2132029..b06a0e8505 100644 --- a/modules/src/core/ics03_connection/handler/conn_open_try.rs +++ b/modules/src/core/ics03_connection/handler/conn_open_try.rs @@ -20,8 +20,11 @@ pub(crate) fn process( ) -> HandlerResult { let mut output = HandlerOutput::builder(); - // Check that consensus height (for client proof) in message is not too advanced nor too old. - check_client_consensus_height(ctx, msg.consensus_height())?; + // If a consensus proof is present, check that the consensus height (for + // client proof) in the message is not too advanced nor too old. + if let Some(consensus_height) = msg.consensus_height() { + check_client_consensus_height(ctx, consensus_height)?; + } // Unwrap the old connection end (if any) and its identifier. let (mut new_connection_end, conn_id) = match &msg.previous_connection_id { @@ -147,7 +150,7 @@ mod tests { want_pass: bool, } - let host_chain_height = Height::new(0, 35); + let host_chain_height = Height::new(0, 35).unwrap(); let max_history_size = 5; let context = MockContext::new( ChainId::new("mockgaia".to_string(), 0), @@ -159,20 +162,20 @@ mod tests { let msg_conn_try = MsgConnectionOpenTry::try_from(get_dummy_raw_msg_conn_open_try( client_consensus_state_height, - host_chain_height.revision_height, + host_chain_height.revision_height(), )) .unwrap(); // The proof targets a height that does not exist (i.e., too advanced) on destination chain. let msg_height_advanced = MsgConnectionOpenTry::try_from(get_dummy_raw_msg_conn_open_try( client_consensus_state_height, - host_chain_height.increment().revision_height, + host_chain_height.increment().revision_height(), )) .unwrap(); let pruned_height = host_chain_height .sub(max_history_size as u64 + 1) .unwrap() - .revision_height; + .revision_height(); // The consensus proof targets a missing height (pruned) on destination chain. let msg_height_old = MsgConnectionOpenTry::try_from(get_dummy_raw_msg_conn_open_try( client_consensus_state_height, @@ -184,7 +187,7 @@ mod tests { let msg_proof_height_missing = MsgConnectionOpenTry::try_from(get_dummy_raw_msg_conn_open_try( client_consensus_state_height - 1, - host_chain_height.revision_height, + host_chain_height.revision_height(), )) .unwrap(); @@ -209,19 +212,19 @@ mod tests { }, Test { name: "Processing fails because the client misses the consensus state targeted by the proof".to_string(), - ctx: context.clone().with_client(&msg_proof_height_missing.client_id, Height::new(0, client_consensus_state_height)), + ctx: context.clone().with_client(&msg_proof_height_missing.client_id, Height::new(0, client_consensus_state_height).unwrap()), msg: ConnectionMsg::ConnectionOpenTry(Box::new(msg_proof_height_missing)), want_pass: false, }, Test { name: "Good parameters but has previous_connection_id".to_string(), - ctx: context.clone().with_client(&msg_conn_try.client_id, Height::new(0, client_consensus_state_height)), + ctx: context.clone().with_client(&msg_conn_try.client_id, Height::new(0, client_consensus_state_height).unwrap()), msg: ConnectionMsg::ConnectionOpenTry(Box::new(msg_conn_try.clone())), want_pass: false, }, Test { name: "Good parameters".to_string(), - ctx: context.with_client(&msg_conn_try.client_id, Height::new(0, client_consensus_state_height)), + ctx: context.with_client(&msg_conn_try.client_id, Height::new(0, client_consensus_state_height).unwrap()), msg: ConnectionMsg::ConnectionOpenTry(Box::new(msg_conn_try.with_previous_connection_id(None))), want_pass: true, }, diff --git a/modules/src/core/ics03_connection/msgs/conn_open_ack.rs b/modules/src/core/ics03_connection/msgs/conn_open_ack.rs index 60a42712ff..42cb51106a 100644 --- a/modules/src/core/ics03_connection/msgs/conn_open_ack.rs +++ b/modules/src/core/ics03_connection/msgs/conn_open_ack.rs @@ -27,13 +27,10 @@ pub struct MsgConnectionOpenAck { } impl MsgConnectionOpenAck { - /// Getter for accessing the `consensus_height` field from this message. Returns the special - /// value `Height(0)` if this field is not set. - pub fn consensus_height(&self) -> Height { - match self.proofs.consensus_proof() { - None => Height::zero(), - Some(p) => p.height(), - } + /// Getter for accessing the `consensus_height` field from this message. + /// Returns `None` if this field is not set. + pub fn consensus_height(&self) -> Option { + self.proofs.consensus_proof().map(|proof| proof.height()) } } @@ -58,8 +55,8 @@ impl TryFrom for MsgConnectionOpenAck { fn try_from(msg: RawMsgConnectionOpenAck) -> Result { let consensus_height = msg .consensus_height - .ok_or_else(Error::missing_consensus_height)? - .into(); + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(Error::missing_consensus_height)?; let consensus_proof_obj = ConsensusProof::new( msg.proof_consensus .try_into() @@ -70,8 +67,8 @@ impl TryFrom for MsgConnectionOpenAck { let proof_height = msg .proof_height - .ok_or_else(Error::missing_proof_height)? - .into(); + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(Error::missing_proof_height)?; let client_proof = CommitmentProofBytes::try_from(msg.proof_client).map_err(Error::invalid_proof)?; diff --git a/modules/src/core/ics03_connection/msgs/conn_open_confirm.rs b/modules/src/core/ics03_connection/msgs/conn_open_confirm.rs index 8a15be9860..4c876d22e8 100644 --- a/modules/src/core/ics03_connection/msgs/conn_open_confirm.rs +++ b/modules/src/core/ics03_connection/msgs/conn_open_confirm.rs @@ -43,8 +43,8 @@ impl TryFrom for MsgConnectionOpenConfirm { fn try_from(msg: RawMsgConnectionOpenConfirm) -> Result { let proof_height = msg .proof_height - .ok_or_else(Error::missing_proof_height)? - .into(); + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(Error::missing_proof_height)?; Ok(Self { connection_id: msg diff --git a/modules/src/core/ics03_connection/msgs/conn_open_try.rs b/modules/src/core/ics03_connection/msgs/conn_open_try.rs index 4709733489..ad315ae461 100644 --- a/modules/src/core/ics03_connection/msgs/conn_open_try.rs +++ b/modules/src/core/ics03_connection/msgs/conn_open_try.rs @@ -38,13 +38,10 @@ pub struct MsgConnectionOpenTry { } impl MsgConnectionOpenTry { - /// Getter for accessing the `consensus_height` field from this message. Returns the special - /// value `0` if this field is not set. - pub fn consensus_height(&self) -> Height { - match self.proofs.consensus_proof() { - None => Height::zero(), - Some(p) => p.height(), - } + /// Getter for accessing the `consensus_height` field from this message. + /// Returns `None` if this field is not set. + pub fn consensus_height(&self) -> Option { + self.proofs.consensus_proof().map(|proof| proof.height()) } } @@ -75,8 +72,8 @@ impl TryFrom for MsgConnectionOpenTry { let consensus_height = msg .consensus_height - .ok_or_else(Error::missing_consensus_height)? - .into(); + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(Error::missing_consensus_height)?; let consensus_proof_obj = ConsensusProof::new( msg.proof_consensus @@ -88,8 +85,8 @@ impl TryFrom for MsgConnectionOpenTry { let proof_height = msg .proof_height - .ok_or_else(Error::missing_proof_height)? - .into(); + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(Error::missing_proof_height)?; let client_proof = CommitmentProofBytes::try_from(msg.proof_client).map_err(Error::invalid_proof)?; diff --git a/modules/src/core/ics04_channel/context.rs b/modules/src/core/ics04_channel/context.rs index bbd6d9090a..3102d817bb 100644 --- a/modules/src/core/ics04_channel/context.rs +++ b/modules/src/core/ics04_channel/context.rs @@ -19,6 +19,7 @@ use crate::timestamp::Timestamp; use crate::Height; use super::packet::{PacketResult, Sequence}; +use super::timeout::TimeoutHeight; /// A context supplying all the necessary read-only dependencies for processing any `ChannelMsg`. pub trait ChannelReader { @@ -67,20 +68,29 @@ pub trait ChannelReader { key: &(PortId, ChannelId, Sequence), ) -> Result; + /// Compute the commitment for a packet. + /// Note that the absence of `timeout_height` is treated as + /// `{revision_number: 0, revision_height: 0}` to be consistent with ibc-go, + /// where this value is used to mean "no timeout height": + /// fn packet_commitment( &self, packet_data: Vec, - timeout_height: Height, + timeout_height: TimeoutHeight, timeout_timestamp: Timestamp, ) -> PacketCommitment { - let mut input = timeout_timestamp.nanoseconds().to_be_bytes().to_vec(); - let revision_number = timeout_height.revision_number.to_be_bytes(); - input.append(&mut revision_number.to_vec()); - let revision_height = timeout_height.revision_height.to_be_bytes(); - input.append(&mut revision_height.to_vec()); - let data = self.hash(packet_data); - input.append(&mut data.to_vec()); - self.hash(input).into() + let mut hash_input = timeout_timestamp.nanoseconds().to_be_bytes().to_vec(); + + let revision_number = timeout_height.commitment_revision_number().to_be_bytes(); + hash_input.append(&mut revision_number.to_vec()); + + let revision_height = timeout_height.commitment_revision_height().to_be_bytes(); + hash_input.append(&mut revision_height.to_vec()); + + let packet_data_hash = self.hash(packet_data); + hash_input.append(&mut packet_data_hash.to_vec()); + + self.hash(hash_input).into() } fn ack_commitment(&self, ack: Acknowledgement) -> AcknowledgementCommitment { diff --git a/modules/src/core/ics04_channel/error.rs b/modules/src/core/ics04_channel/error.rs index 18bf45aa97..67a465ca79 100644 --- a/modules/src/core/ics04_channel/error.rs +++ b/modules/src/core/ics04_channel/error.rs @@ -1,4 +1,5 @@ use super::packet::Sequence; +use super::timeout::TimeoutHeight; use crate::core::ics02_client::error as client_error; use crate::core::ics03_connection::error as connection_error; use crate::core::ics04_channel::channel::State; @@ -82,9 +83,6 @@ define_error! { ZeroPacketData | _ | { "packet data bytes cannot be empty" }, - ZeroPacketTimeout - | _ | { "packet timeout height and packet timeout timestamp cannot both be 0" }, - InvalidTimeoutHeight | _ | { "invalid timeout height for the packet" }, @@ -201,7 +199,7 @@ define_error! { LowPacketHeight { chain_height: Height, - timeout_height: Height + timeout_height: TimeoutHeight } | e | { format_args!( @@ -211,7 +209,7 @@ define_error! { PacketTimeoutHeightNotReached { - timeout_height: Height, + timeout_height: TimeoutHeight, chain_height: Height, } | e | { diff --git a/modules/src/core/ics04_channel/events.rs b/modules/src/core/ics04_channel/events.rs index 35b4865ef6..49ee2d308f 100644 --- a/modules/src/core/ics04_channel/events.rs +++ b/modules/src/core/ics04_channel/events.rs @@ -14,6 +14,8 @@ use crate::events::{ }; use crate::prelude::*; +use super::packet::parse_timeout_height; + /// Channel event attribute keys const HEIGHT_ATTRIBUTE_KEY: &str = "height"; const CONNECTION_ID_ATTRIBUTE_KEY: &str = "connection_id"; @@ -23,15 +25,15 @@ const COUNTERPARTY_CHANNEL_ID_ATTRIBUTE_KEY: &str = "counterparty_channel_id"; const COUNTERPARTY_PORT_ID_ATTRIBUTE_KEY: &str = "counterparty_port_id"; /// Packet event attribute keys -const PKT_SEQ_ATTRIBUTE_KEY: &str = "packet_sequence"; -const PKT_DATA_ATTRIBUTE_KEY: &str = "packet_data"; -const PKT_SRC_PORT_ATTRIBUTE_KEY: &str = "packet_src_port"; -const PKT_SRC_CHANNEL_ATTRIBUTE_KEY: &str = "packet_src_channel"; -const PKT_DST_PORT_ATTRIBUTE_KEY: &str = "packet_dst_port"; -const PKT_DST_CHANNEL_ATTRIBUTE_KEY: &str = "packet_dst_channel"; -const PKT_TIMEOUT_HEIGHT_ATTRIBUTE_KEY: &str = "packet_timeout_height"; -const PKT_TIMEOUT_TIMESTAMP_ATTRIBUTE_KEY: &str = "packet_timeout_timestamp"; -const PKT_ACK_ATTRIBUTE_KEY: &str = "packet_ack"; +pub const PKT_SEQ_ATTRIBUTE_KEY: &str = "packet_sequence"; +pub const PKT_DATA_ATTRIBUTE_KEY: &str = "packet_data"; +pub const PKT_SRC_PORT_ATTRIBUTE_KEY: &str = "packet_src_port"; +pub const PKT_SRC_CHANNEL_ATTRIBUTE_KEY: &str = "packet_src_channel"; +pub const PKT_DST_PORT_ATTRIBUTE_KEY: &str = "packet_dst_port"; +pub const PKT_DST_CHANNEL_ATTRIBUTE_KEY: &str = "packet_dst_channel"; +pub const PKT_TIMEOUT_HEIGHT_ATTRIBUTE_KEY: &str = "packet_timeout_height"; +pub const PKT_TIMEOUT_TIMESTAMP_ATTRIBUTE_KEY: &str = "packet_timeout_timestamp"; +pub const PKT_ACK_ATTRIBUTE_KEY: &str = "packet_ack"; pub fn try_from_tx(event: &tendermint::abci::Event) -> Option { match event.type_str.parse() { @@ -71,7 +73,7 @@ pub fn try_from_tx(event: &tendermint::abci::Event) -> Option { // This event should not have a write ack. debug_assert_eq!(write_ack.len(), 0); IbcEvent::SendPacket(SendPacket { - height: Default::default(), + height: Height::new(0, 1).unwrap(), packet, }) }) @@ -80,7 +82,7 @@ pub fn try_from_tx(event: &tendermint::abci::Event) -> Option { Ok(IbcEventType::WriteAck) => extract_packet_and_write_ack_from_tx(event) .map(|(packet, write_ack)| { IbcEvent::WriteAcknowledgement(WriteAcknowledgement { - height: Default::default(), + height: Height::new(0, 1).unwrap(), packet, ack: write_ack, }) @@ -92,7 +94,7 @@ pub fn try_from_tx(event: &tendermint::abci::Event) -> Option { // This event should not have a write ack. debug_assert_eq!(write_ack.len(), 0); IbcEvent::AcknowledgePacket(AcknowledgePacket { - height: Default::default(), + height: Height::new(0, 1).unwrap(), packet, }) }) @@ -104,7 +106,7 @@ pub fn try_from_tx(event: &tendermint::abci::Event) -> Option { // This event should not have a write ack. debug_assert_eq!(write_ack.len(), 0); IbcEvent::TimeoutPacket(TimeoutPacket { - height: Default::default(), + height: Height::new(0, 1).unwrap(), packet, }) }) @@ -169,8 +171,7 @@ fn extract_packet_and_write_ack_from_tx( .into() } PKT_TIMEOUT_HEIGHT_ATTRIBUTE_KEY => { - packet.timeout_height = - value.parse().map_err(|_| Error::invalid_timeout_height())?; + packet.timeout_height = parse_timeout_height(value)?; } PKT_TIMEOUT_TIMESTAMP_ATTRIBUTE_KEY => { packet.timeout_timestamp = value.parse().unwrap(); @@ -213,7 +214,7 @@ fn extract_attributes(object: &RawObject<'_>, namespace: &str) -> Result Self { + Self { + height: Height::new(0, 1).unwrap(), + port_id: Default::default(), + channel_id: Default::default(), + connection_id: Default::default(), + counterparty_port_id: Default::default(), + counterparty_channel_id: Default::default(), + } + } +} + /// Convert attributes to Tendermint ABCI tags /// /// # Note @@ -320,7 +334,7 @@ impl TryFrom for Vec { attributes.push(sequence); let timeout_height = Tag { key: PKT_TIMEOUT_HEIGHT_ATTRIBUTE_KEY.parse().unwrap(), - value: p.timeout_height.to_string().parse().unwrap(), + value: p.timeout_height.into(), }; attributes.push(timeout_height); let timeout_timestamp = Tag { @@ -1156,7 +1170,7 @@ mod tests { #[test] fn channel_event_to_abci_event() { let attributes = Attributes { - height: Height::default(), + height: Height::new(0, 1).unwrap(), port_id: "test_port".parse().unwrap(), channel_id: Some("channel-0".parse().unwrap()), connection_id: "test_connection".parse().unwrap(), @@ -1214,28 +1228,28 @@ mod tests { destination_port: "b_test_port".parse().unwrap(), destination_channel: "channel-1".parse().unwrap(), data: "test_data".as_bytes().to_vec(), - timeout_height: Height::new(1, 10), + timeout_height: Height::new(1, 10).unwrap().into(), timeout_timestamp: Timestamp::now(), }; let mut abci_events = vec![]; let send_packet = SendPacket { - height: Height::default(), + height: Height::new(0, 1).unwrap(), packet: packet.clone(), }; abci_events.push(AbciEvent::try_from(send_packet.clone()).unwrap()); let write_ack = WriteAcknowledgement { - height: Height::default(), + height: Height::new(0, 1).unwrap(), packet: packet.clone(), ack: "test_ack".as_bytes().to_vec(), }; abci_events.push(AbciEvent::try_from(write_ack.clone()).unwrap()); let ack_packet = AcknowledgePacket { - height: Height::default(), + height: Height::new(0, 1).unwrap(), packet: packet.clone(), }; abci_events.push(AbciEvent::try_from(ack_packet.clone()).unwrap()); let timeout_packet = TimeoutPacket { - height: Height::default(), + height: Height::new(0, 1).unwrap(), packet, }; abci_events.push(AbciEvent::try_from(timeout_packet.clone()).unwrap()); diff --git a/modules/src/core/ics04_channel/handler/acknowledgement.rs b/modules/src/core/ics04_channel/handler/acknowledgement.rs index 5246953b8d..dccf8c947d 100644 --- a/modules/src/core/ics04_channel/handler/acknowledgement.rs +++ b/modules/src/core/ics04_channel/handler/acknowledgement.rs @@ -149,10 +149,10 @@ mod tests { let context = MockContext::default(); - let client_height = Height::new(0, Height::default().revision_height + 2); + let client_height = Height::new(0, 2).unwrap(); let msg = MsgAcknowledgement::try_from(get_dummy_raw_msg_acknowledgement( - client_height.revision_height, + client_height.revision_height(), )) .unwrap(); let packet = msg.packet.clone(); diff --git a/modules/src/core/ics04_channel/handler/chan_close_confirm.rs b/modules/src/core/ics04_channel/handler/chan_close_confirm.rs index fb47d41ac0..60cee080f9 100644 --- a/modules/src/core/ics04_channel/handler/chan_close_confirm.rs +++ b/modules/src/core/ics04_channel/handler/chan_close_confirm.rs @@ -137,7 +137,7 @@ mod tests { ); let msg_chan_close_confirm = MsgChannelCloseConfirm::try_from( - get_dummy_raw_msg_chan_close_confirm(client_consensus_state_height.revision_height), + get_dummy_raw_msg_chan_close_confirm(client_consensus_state_height.revision_height()), ) .unwrap(); diff --git a/modules/src/core/ics04_channel/handler/chan_open_ack.rs b/modules/src/core/ics04_channel/handler/chan_open_ack.rs index e883694e3e..e3fe9adc19 100644 --- a/modules/src/core/ics04_channel/handler/chan_open_ack.rs +++ b/modules/src/core/ics04_channel/handler/chan_open_ack.rs @@ -145,7 +145,7 @@ mod tests { } let proof_height = 10; let client_consensus_state_height = 10; - let host_chain_height = Height::new(0, 35); + let host_chain_height = Height::new(0, 35).unwrap(); let context = MockContext::default(); @@ -181,7 +181,7 @@ mod tests { let msg_conn_try = MsgConnectionOpenTry::try_from(get_dummy_raw_msg_conn_open_try( client_consensus_state_height, - host_chain_height.revision_height, + host_chain_height.revision_height(), )) .unwrap(); @@ -220,7 +220,7 @@ mod tests { .clone() .with_client( &msg_conn_try.client_id, - Height::new(0, client_consensus_state_height), + Height::new(0, client_consensus_state_height).unwrap(), ) .with_channel( msg_chan_ack.port_id.clone(), @@ -236,7 +236,7 @@ mod tests { .clone() .with_client( &msg_conn_try.client_id, - Height::new(0, client_consensus_state_height), + Height::new(0, client_consensus_state_height).unwrap(), ) .with_channel( msg_chan_ack.port_id.clone(), @@ -264,7 +264,7 @@ mod tests { ctx: context // .clone() .with_client( &msg_conn_try.client_id, - Height::new(0, client_consensus_state_height), + Height::new(0, client_consensus_state_height).unwrap(), ) .with_connection(cid, conn_end) .with_channel( diff --git a/modules/src/core/ics04_channel/handler/chan_open_confirm.rs b/modules/src/core/ics04_channel/handler/chan_open_confirm.rs index 23420c4fa5..37b3d1744a 100644 --- a/modules/src/core/ics04_channel/handler/chan_open_confirm.rs +++ b/modules/src/core/ics04_channel/handler/chan_open_confirm.rs @@ -138,7 +138,7 @@ mod tests { let client_id = ClientId::new(ClientType::Mock, 24).unwrap(); let conn_id = ConnectionId::new(2); let context = MockContext::default(); - let client_consensus_state_height = context.host_current_height().revision_height; + let client_consensus_state_height = context.host_current_height().revision_height(); // The connection underlying the channel we're trying to open. let conn_end = ConnectionEnd::new( @@ -168,7 +168,10 @@ mod tests { let tests: Vec = vec![Test { name: "Good parameters".to_string(), ctx: context - .with_client(&client_id, Height::new(0, client_consensus_state_height)) + .with_client( + &client_id, + Height::new(0, client_consensus_state_height).unwrap(), + ) .with_connection(conn_id, conn_end) .with_channel( msg_chan_confirm.port_id.clone(), diff --git a/modules/src/core/ics04_channel/handler/chan_open_try.rs b/modules/src/core/ics04_channel/handler/chan_open_try.rs index 126a7ad8e9..d4bba28f5c 100644 --- a/modules/src/core/ics04_channel/handler/chan_open_try.rs +++ b/modules/src/core/ics04_channel/handler/chan_open_try.rs @@ -363,7 +363,7 @@ mod tests { name: "Processing is successful".to_string(), ctx: context .clone() - .with_client(&client_id, Height::new(0, proof_height)) + .with_client(&client_id, Height::new(0, proof_height).unwrap()) .with_connection(conn_id.clone(), conn_end.clone()) .with_channel(msg.port_id.clone(), chan_id, correct_chan_end), msg: ChannelMsg::ChannelOpenTry(msg), @@ -374,7 +374,7 @@ mod tests { name: "Processing is successful against an empty context (no preexisting channel)" .to_string(), ctx: context - .with_client(&client_id, Height::new(0, proof_height)) + .with_client(&client_id, Height::new(0, proof_height).unwrap()) .with_connection(conn_id, conn_end), msg: ChannelMsg::ChannelOpenTry(msg_vanilla), want_pass: true, diff --git a/modules/src/core/ics04_channel/handler/recv_packet.rs b/modules/src/core/ics04_channel/handler/recv_packet.rs index 973c163bcd..37c3db9011 100644 --- a/modules/src/core/ics04_channel/handler/recv_packet.rs +++ b/modules/src/core/ics04_channel/handler/recv_packet.rs @@ -10,7 +10,6 @@ use crate::core::ics24_host::identifier::{ChannelId, PortId}; use crate::events::IbcEvent; use crate::handler::{HandlerOutput, HandlerResult}; use crate::timestamp::Expiry; -use crate::Height; #[derive(Clone, Debug)] pub enum RecvPacketResult { @@ -61,7 +60,7 @@ pub fn process(ctx: &dyn ChannelReader, msg: &MsgRecvPacket) -> HandlerResult HandlerResult HandlerResult { output.emit(IbcEvent::ReceivePacket(ReceivePacket { - height: Height::zero(), + height: ctx.host_height(), packet: msg.packet.clone(), })); return Ok(output.with_result(PacketResult::Recv(RecvPacketResult::NoOp))); @@ -182,9 +181,10 @@ mod tests { let client_height = host_height.increment(); - let msg = - MsgRecvPacket::try_from(get_dummy_raw_msg_recv_packet(client_height.revision_height)) - .unwrap(); + let msg = MsgRecvPacket::try_from(get_dummy_raw_msg_recv_packet( + client_height.revision_height(), + )) + .unwrap(); let packet = msg.packet.clone(); @@ -195,7 +195,7 @@ mod tests { destination_port: PortId::default(), destination_channel: ChannelId::default(), data: Vec::new(), - timeout_height: client_height, + timeout_height: client_height.into(), timeout_timestamp: Timestamp::from_nanoseconds(1).unwrap(), }; diff --git a/modules/src/core/ics04_channel/handler/send_packet.rs b/modules/src/core/ics04_channel/handler/send_packet.rs index 00e573aef2..83e4540f8a 100644 --- a/modules/src/core/ics04_channel/handler/send_packet.rs +++ b/modules/src/core/ics04_channel/handler/send_packet.rs @@ -55,7 +55,7 @@ pub fn send_packet(ctx: &dyn ChannelReader, packet: Packet) -> HandlerResult = vec![ Test { @@ -182,7 +199,7 @@ mod tests { name: "Good parameters".to_string(), ctx: context .clone() - .with_client(&ClientId::default(), Height::default()) + .with_client(&ClientId::default(), client_height) .with_connection(ConnectionId::default(), connection_end.clone()) .with_channel(PortId::default(), ChannelId::default(), channel_end.clone()) .with_send_sequence(PortId::default(), ChannelId::default(), 1.into()), @@ -190,13 +207,35 @@ mod tests { want_pass: true, }, Test { - name: "Packet timeout".to_string(), + name: "Packet timeout height same as destination chain height".to_string(), + ctx: context + .clone() + .with_client(&ClientId::default(), client_height) + .with_connection(ConnectionId::default(), connection_end.clone()) + .with_channel(PortId::default(), ChannelId::default(), channel_end.clone()) + .with_send_sequence(PortId::default(), ChannelId::default(), 1.into()), + packet: packet_timeout_equal_client_height, + want_pass: true, + }, + Test { + name: "Packet timeout height one more than destination chain height".to_string(), + ctx: context + .clone() + .with_client(&ClientId::default(), client_height) + .with_connection(ConnectionId::default(), connection_end.clone()) + .with_channel(PortId::default(), ChannelId::default(), channel_end.clone()) + .with_send_sequence(PortId::default(), ChannelId::default(), 1.into()), + packet: packet_timeout_one_before_client_height, + want_pass: false, + }, + Test { + name: "Packet timeout due to timestamp".to_string(), ctx: context .with_client(&ClientId::default(), client_height) .with_connection(ConnectionId::default(), connection_end) .with_channel(PortId::default(), ChannelId::default(), channel_end) .with_send_sequence(PortId::default(), ChannelId::default(), 1.into()), - packet: packet_old, + packet: packet_with_timestamp_old, want_pass: false, }, ] diff --git a/modules/src/core/ics04_channel/handler/timeout.rs b/modules/src/core/ics04_channel/handler/timeout.rs index c6153aa3cb..d1ebe5cb3a 100644 --- a/modules/src/core/ics04_channel/handler/timeout.rs +++ b/modules/src/core/ics04_channel/handler/timeout.rs @@ -21,6 +21,11 @@ pub struct TimeoutPacketResult { pub channel: Option, } +/// TimeoutPacket is called by a module which originally attempted to send a +/// packet to a counterparty module, where the timeout height has passed on the +/// counterparty chain without the packet being committed, to prove that the +/// packet can no longer be executed and to allow the calling module to safely +/// perform appropriate state transitions. pub fn process(ctx: &dyn ChannelReader, msg: &MsgTimeout) -> HandlerResult { let mut output = HandlerOutput::builder(); @@ -51,9 +56,8 @@ pub fn process(ctx: &dyn ChannelReader, msg: &MsgTimeout) -> HandlerResult proof_height { + if packet.timeout_height.has_expired(proof_height) { return Err(Error::packet_timeout_height_not_reached( packet.timeout_height, proof_height, @@ -170,13 +174,18 @@ mod tests { let context = MockContext::default(); - let height = Height::default().revision_height + 2; + let msg_proof_height = 2; + let msg_timeout_height = 5; let timeout_timestamp = 5; - let client_height = Height::new(0, Height::default().revision_height + 2); + let client_height = Height::new(0, 2).unwrap(); - let msg = - MsgTimeout::try_from(get_dummy_raw_msg_timeout(height, timeout_timestamp)).unwrap(); + let msg = MsgTimeout::try_from(get_dummy_raw_msg_timeout( + msg_proof_height, + msg_timeout_height, + timeout_timestamp, + )) + .unwrap(); let packet = msg.packet.clone(); let mut msg_ok = msg.clone(); diff --git a/modules/src/core/ics04_channel/handler/timeout_on_close.rs b/modules/src/core/ics04_channel/handler/timeout_on_close.rs index 04a7550ec6..d0cf3b677d 100644 --- a/modules/src/core/ics04_channel/handler/timeout_on_close.rs +++ b/modules/src/core/ics04_channel/handler/timeout_on_close.rs @@ -163,10 +163,10 @@ mod tests { let context = MockContext::default(); - let height = Height::default().revision_height + 2; + let height = 2; let timeout_timestamp = 5; - let client_height = Height::new(0, Height::default().revision_height + 2); + let client_height = Height::new(0, 2).unwrap(); let msg = MsgTimeoutOnClose::try_from(get_dummy_raw_msg_timeout_on_close( height, diff --git a/modules/src/core/ics04_channel/handler/write_acknowledgement.rs b/modules/src/core/ics04_channel/handler/write_acknowledgement.rs index 3bf30c57c0..61bbc2824d 100644 --- a/modules/src/core/ics04_channel/handler/write_acknowledgement.rs +++ b/modules/src/core/ics04_channel/handler/write_acknowledgement.rs @@ -104,7 +104,7 @@ mod tests { let context = MockContext::default(); - let client_height = Height::new(0, 1); + let client_height = Height::new(0, 1).unwrap(); let mut packet: Packet = get_dummy_raw_packet(1, 6).try_into().unwrap(); packet.sequence = 1.into(); @@ -159,7 +159,7 @@ mod tests { Test { name: "Zero ack".to_string(), ctx: context - .with_client(&ClientId::default(), Height::default()) + .with_client(&ClientId::default(), Height::new(0, 1).unwrap()) .with_connection(ConnectionId::default(), connection_end) .with_channel(PortId::default(), ChannelId::default(), dest_channel_end), packet, diff --git a/modules/src/core/ics04_channel/mod.rs b/modules/src/core/ics04_channel/mod.rs index 35a8ef9e4c..d6b23e13bc 100644 --- a/modules/src/core/ics04_channel/mod.rs +++ b/modules/src/core/ics04_channel/mod.rs @@ -9,6 +9,7 @@ pub mod events; pub mod handler; pub mod msgs; pub mod packet; +pub mod timeout; pub mod commitment; mod version; diff --git a/modules/src/core/ics04_channel/msgs/acknowledgement.rs b/modules/src/core/ics04_channel/msgs/acknowledgement.rs index 95bf93279f..f266999a89 100644 --- a/modules/src/core/ics04_channel/msgs/acknowledgement.rs +++ b/modules/src/core/ics04_channel/msgs/acknowledgement.rs @@ -102,8 +102,8 @@ impl TryFrom for MsgAcknowledgement { None, raw_msg .proof_height - .ok_or_else(Error::missing_height)? - .into(), + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(Error::missing_height)?, ) .map_err(Error::invalid_proof)?; diff --git a/modules/src/core/ics04_channel/msgs/chan_close_confirm.rs b/modules/src/core/ics04_channel/msgs/chan_close_confirm.rs index 2dc6bd27dc..f075a4c0b2 100644 --- a/modules/src/core/ics04_channel/msgs/chan_close_confirm.rs +++ b/modules/src/core/ics04_channel/msgs/chan_close_confirm.rs @@ -64,8 +64,8 @@ impl TryFrom for MsgChannelCloseConfirm { None, raw_msg .proof_height - .ok_or_else(Error::missing_height)? - .into(), + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(Error::missing_height)?, ) .map_err(Error::invalid_proof)?; diff --git a/modules/src/core/ics04_channel/msgs/chan_open_ack.rs b/modules/src/core/ics04_channel/msgs/chan_open_ack.rs index 14cd6d85c6..25b4ae1088 100644 --- a/modules/src/core/ics04_channel/msgs/chan_open_ack.rs +++ b/modules/src/core/ics04_channel/msgs/chan_open_ack.rs @@ -70,8 +70,8 @@ impl TryFrom for MsgChannelOpenAck { None, raw_msg .proof_height - .ok_or_else(Error::missing_height)? - .into(), + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(Error::missing_height)?, ) .map_err(Error::invalid_proof)?; diff --git a/modules/src/core/ics04_channel/msgs/chan_open_confirm.rs b/modules/src/core/ics04_channel/msgs/chan_open_confirm.rs index 7ad004adb3..a37bc84c8e 100644 --- a/modules/src/core/ics04_channel/msgs/chan_open_confirm.rs +++ b/modules/src/core/ics04_channel/msgs/chan_open_confirm.rs @@ -59,8 +59,8 @@ impl TryFrom for MsgChannelOpenConfirm { None, raw_msg .proof_height - .ok_or_else(Error::missing_height)? - .into(), + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(Error::missing_height)?, ) .map_err(Error::invalid_proof)?; diff --git a/modules/src/core/ics04_channel/msgs/chan_open_try.rs b/modules/src/core/ics04_channel/msgs/chan_open_try.rs index 5af004d529..358c0c979d 100644 --- a/modules/src/core/ics04_channel/msgs/chan_open_try.rs +++ b/modules/src/core/ics04_channel/msgs/chan_open_try.rs @@ -84,8 +84,8 @@ impl TryFrom for MsgChannelOpenTry { None, raw_msg .proof_height - .ok_or_else(ChannelError::missing_height)? - .into(), + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(ChannelError::missing_height)?, ) .map_err(ChannelError::invalid_proof)?; diff --git a/modules/src/core/ics04_channel/msgs/recv_packet.rs b/modules/src/core/ics04_channel/msgs/recv_packet.rs index 7aad7c6eec..30b8522b3d 100644 --- a/modules/src/core/ics04_channel/msgs/recv_packet.rs +++ b/modules/src/core/ics04_channel/msgs/recv_packet.rs @@ -61,8 +61,8 @@ impl TryFrom for MsgRecvPacket { None, raw_msg .proof_height - .ok_or_else(Error::missing_height)? - .into(), + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(Error::missing_height)?, ) .map_err(Error::invalid_proof)?; diff --git a/modules/src/core/ics04_channel/msgs/timeout.rs b/modules/src/core/ics04_channel/msgs/timeout.rs index a2887e1013..966fdd80a2 100644 --- a/modules/src/core/ics04_channel/msgs/timeout.rs +++ b/modules/src/core/ics04_channel/msgs/timeout.rs @@ -68,8 +68,8 @@ impl TryFrom for MsgTimeout { None, raw_msg .proof_height - .ok_or_else(Error::missing_height)? - .into(), + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(Error::missing_height)?, ) .map_err(Error::invalid_proof)?; @@ -109,13 +109,17 @@ pub mod test_util { /// Returns a dummy `RawMsgTimeout`, for testing only! /// The `height` parametrizes both the proof height as well as the timeout height. - pub fn get_dummy_raw_msg_timeout(height: u64, timeout_timestamp: u64) -> RawMsgTimeout { + pub fn get_dummy_raw_msg_timeout( + proof_height: u64, + timeout_height: u64, + timeout_timestamp: u64, + ) -> RawMsgTimeout { RawMsgTimeout { - packet: Some(get_dummy_raw_packet(height, timeout_timestamp)), + packet: Some(get_dummy_raw_packet(timeout_height, timeout_timestamp)), proof_unreceived: get_dummy_proof(), proof_height: Some(RawHeight { revision_number: 0, - revision_height: height, + revision_height: proof_height, }), next_sequence_recv: 1, signer: get_dummy_bech32_account(), @@ -144,9 +148,11 @@ mod test { want_pass: bool, } - let height = 50; + let proof_height = 50; + let timeout_height = proof_height; let timeout_timestamp = 0; - let default_raw_msg = get_dummy_raw_msg_timeout(height, timeout_timestamp); + let default_raw_msg = + get_dummy_raw_msg_timeout(proof_height, timeout_height, timeout_timestamp); let tests: Vec = vec![ Test { @@ -204,7 +210,7 @@ mod test { #[test] fn to_and_from() { - let raw = get_dummy_raw_msg_timeout(15, 0); + let raw = get_dummy_raw_msg_timeout(15, 20, 0); let msg = MsgTimeout::try_from(raw.clone()).unwrap(); let raw_back = RawMsgTimeout::from(msg.clone()); let msg_back = MsgTimeout::try_from(raw_back.clone()).unwrap(); diff --git a/modules/src/core/ics04_channel/msgs/timeout_on_close.rs b/modules/src/core/ics04_channel/msgs/timeout_on_close.rs index 05fda53a54..029aeb623b 100644 --- a/modules/src/core/ics04_channel/msgs/timeout_on_close.rs +++ b/modules/src/core/ics04_channel/msgs/timeout_on_close.rs @@ -67,8 +67,8 @@ impl TryFrom for MsgTimeoutOnClose { None, raw_msg .proof_height - .ok_or_else(Error::missing_height)? - .into(), + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(Error::missing_height)?, ) .map_err(Error::invalid_proof)?; diff --git a/modules/src/core/ics04_channel/packet.rs b/modules/src/core/ics04_channel/packet.rs index 3d6a49886b..6efc76a7ea 100644 --- a/modules/src/core/ics04_channel/packet.rs +++ b/modules/src/core/ics04_channel/packet.rs @@ -1,3 +1,4 @@ +use crate::core::ics02_client::height::HeightErrorDetail; use crate::prelude::*; use core::str::FromStr; @@ -12,10 +13,16 @@ use crate::events::{extract_attribute, Error as EventError, RawObject}; use crate::timestamp::{Expiry::Expired, Timestamp}; use crate::Height; +use super::events::{ + PKT_DST_CHANNEL_ATTRIBUTE_KEY, PKT_DST_PORT_ATTRIBUTE_KEY, PKT_SEQ_ATTRIBUTE_KEY, + PKT_SRC_CHANNEL_ATTRIBUTE_KEY, PKT_SRC_PORT_ATTRIBUTE_KEY, PKT_TIMEOUT_HEIGHT_ATTRIBUTE_KEY, + PKT_TIMEOUT_TIMESTAMP_ATTRIBUTE_KEY, +}; use super::handler::{ acknowledgement::AckPacketResult, recv_packet::RecvPacketResult, send_packet::SendPacketResult, timeout::TimeoutPacketResult, write_acknowledgement::WriteAckPacketResult, }; +use super::timeout::TimeoutHeight; /// Enumeration of proof carrying ICS4 message, helper for relayer. #[derive(Clone, Debug, PartialEq, Eq)] @@ -106,7 +113,7 @@ pub struct Packet { pub destination_channel: ChannelId, #[serde(serialize_with = "crate::serializers::ser_hex_upper")] pub data: Vec, - pub timeout_height: Height, + pub timeout_height: TimeoutHeight, pub timeout_timestamp: Timestamp, } @@ -165,9 +172,12 @@ impl Packet { /// instead of the common-case where it results in /// [`MsgRecvPacket`](crate::core::ics04_channel::msgs::recv_packet::MsgRecvPacket). pub fn timed_out(&self, dst_chain_ts: &Timestamp, dst_chain_height: Height) -> bool { - (self.timeout_height != Height::zero() && self.timeout_height < dst_chain_height) - || (self.timeout_timestamp != Timestamp::none() - && dst_chain_ts.check_expiry(&self.timeout_timestamp) == Expired) + let height_timed_out = self.timeout_height.has_expired(dst_chain_height); + + let timestamp_timed_out = self.timeout_timestamp != Timestamp::none() + && dst_chain_ts.check_expiry(&self.timeout_timestamp) == Expired; + + height_timed_out || timestamp_timed_out } } @@ -188,6 +198,20 @@ impl core::fmt::Display for Packet { } } +/// Parse a string into a timeout height expected to be stored in +/// `Packet.timeout_height`. We need to parse the timeout height differently +/// because of a quirk introduced in ibc-go. See comment in +/// `TryFrom for Packet`. +pub fn parse_timeout_height(s: &str) -> Result { + match s.parse::() { + Ok(height) => Ok(TimeoutHeight::from(height)), + Err(e) => match e.into_detail() { + HeightErrorDetail::ZeroHeight(_) => Ok(TimeoutHeight::no_timeout()), + _ => Err(Error::invalid_timeout_height()), + }, + } +} + impl TryFrom for Packet { type Error = Error; @@ -195,14 +219,20 @@ impl TryFrom for Packet { if Sequence::from(raw_pkt.sequence).is_zero() { return Err(Error::zero_packet_sequence()); } - let packet_timeout_height: Height = raw_pkt + + // Note: ibc-go currently (July 2022) incorrectly treats the timeout + // heights `{revision_number : >0, revision_height: 0}` as valid + // timeouts. However, heights with `revision_height == 0` are invalid in + // Tendermint. We explicitly reject these values because they go against + // the Tendermint spec, and shouldn't be used. To timeout on the next + // revision_number as soon as the chain starts, + // `{revision_number: old_rev + 1, revision_height: 1}` + // should be used. + let packet_timeout_height: TimeoutHeight = raw_pkt .timeout_height - .ok_or_else(Error::missing_height)? - .into(); + .try_into() + .map_err(|_| Error::invalid_timeout_height())?; - if packet_timeout_height.is_zero() && raw_pkt.timeout_timestamp == 0 { - return Err(Error::zero_packet_timeout()); - } if raw_pkt.data.is_empty() { return Err(Error::zero_packet_data()); } @@ -233,34 +263,47 @@ impl TryFrom> for Packet { type Error = EventError; fn try_from(obj: RawObject<'_>) -> Result { Ok(Packet { - sequence: extract_attribute(&obj, &format!("{}.packet_sequence", obj.action))? - .parse() - .map_err(EventError::channel)?, - source_port: extract_attribute(&obj, &format!("{}.packet_src_port", obj.action))? - .parse() - .map_err(EventError::parse)?, - source_channel: extract_attribute(&obj, &format!("{}.packet_src_channel", obj.action))? - .parse() - .map_err(EventError::parse)?, - destination_port: extract_attribute(&obj, &format!("{}.packet_dst_port", obj.action))? - .parse() - .map_err(EventError::parse)?, - destination_channel: extract_attribute( + sequence: extract_attribute( + &obj, + &format!("{}.{}", obj.action, PKT_SEQ_ATTRIBUTE_KEY), + )? + .parse() + .map_err(EventError::channel)?, + source_port: extract_attribute( &obj, - &format!("{}.packet_dst_channel", obj.action), + &format!("{}.{}", obj.action, PKT_SRC_PORT_ATTRIBUTE_KEY), )? .parse() .map_err(EventError::parse)?, - data: vec![], - timeout_height: extract_attribute( + source_channel: extract_attribute( &obj, - &format!("{}.packet_timeout_height", obj.action), + &format!("{}.{}", obj.action, PKT_SRC_CHANNEL_ATTRIBUTE_KEY), )? .parse() - .map_err(EventError::height)?, + .map_err(EventError::parse)?, + destination_port: extract_attribute( + &obj, + &format!("{}.{}", obj.action, PKT_DST_PORT_ATTRIBUTE_KEY), + )? + .parse() + .map_err(EventError::parse)?, + destination_channel: extract_attribute( + &obj, + &format!("{}.{}", obj.action, PKT_DST_CHANNEL_ATTRIBUTE_KEY), + )? + .parse() + .map_err(EventError::parse)?, + data: vec![], + timeout_height: { + let timeout_height_str = extract_attribute( + &obj, + &format!("{}.{}", obj.action, PKT_TIMEOUT_HEIGHT_ATTRIBUTE_KEY), + )?; + parse_timeout_height(&timeout_height_str).map_err(|_| EventError::height())? + }, timeout_timestamp: extract_attribute( &obj, - &format!("{}.packet_timeout_timestamp", obj.action), + &format!("{}.{}", obj.action, PKT_TIMEOUT_TIMESTAMP_ATTRIBUTE_KEY), )? .parse() .map_err(EventError::timestamp)?, @@ -277,7 +320,7 @@ impl From for RawPacket { destination_port: packet.destination_port.to_string(), destination_channel: packet.destination_channel.to_string(), data: packet.data, - timeout_height: Some(packet.timeout_height.into()), + timeout_height: packet.timeout_height.into(), timeout_timestamp: packet.timeout_timestamp.nanoseconds(), } } @@ -316,6 +359,7 @@ mod tests { use test_log::test; use ibc_proto::ibc::core::channel::v1::Packet as RawPacket; + use ibc_proto::ibc::core::client::v1::Height as RawHeight; use crate::core::ics04_channel::packet::test_utils::get_dummy_raw_packet; use crate::core::ics04_channel::packet::Packet; @@ -329,19 +373,38 @@ mod tests { } let proof_height = 10; - let default_raw_msg = get_dummy_raw_packet(proof_height, 0); + let default_raw_packet = get_dummy_raw_packet(proof_height, 0); + let raw_packet_no_timeout_or_timestamp = get_dummy_raw_packet(0, 0); + + let mut raw_packet_invalid_timeout_height = get_dummy_raw_packet(0, 0); + raw_packet_invalid_timeout_height.timeout_height = Some(RawHeight { + revision_number: 1, + revision_height: 0, + }); let tests: Vec = vec![ Test { name: "Good parameters".to_string(), - raw: default_raw_msg.clone(), + raw: default_raw_packet.clone(), want_pass: true, }, + Test { + // Note: ibc-go currently (July 2022) incorrectly rejects this + // case, even though it is allowed in ICS-4. + name: "Packet with no timeout of timestamp".to_string(), + raw: raw_packet_no_timeout_or_timestamp, + want_pass: true, + }, + Test { + name: "Packet with invalid timeout height".to_string(), + raw: raw_packet_invalid_timeout_height, + want_pass: false, + }, Test { name: "Src port validation: correct".to_string(), raw: RawPacket { source_port: "srcportp34".to_string(), - ..default_raw_msg.clone() + ..default_raw_packet.clone() }, want_pass: true, }, @@ -349,7 +412,7 @@ mod tests { name: "Bad src port, name too short".to_string(), raw: RawPacket { source_port: "p".to_string(), - ..default_raw_msg.clone() + ..default_raw_packet.clone() }, want_pass: false, }, @@ -357,7 +420,7 @@ mod tests { name: "Bad src port, name too long".to_string(), raw: RawPacket { source_port: "abcdefghijasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfadgasgasdfasdfasdfasdfaklmnopqrstuabcdefghijasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfadgasgasdfasdfasdfasdfaklmnopqrstu".to_string(), - ..default_raw_msg.clone() + ..default_raw_packet.clone() }, want_pass: false, }, @@ -365,7 +428,7 @@ mod tests { name: "Dst port validation: correct".to_string(), raw: RawPacket { destination_port: "destportsrcp34".to_string(), - ..default_raw_msg.clone() + ..default_raw_packet.clone() }, want_pass: true, }, @@ -373,7 +436,7 @@ mod tests { name: "Bad dst port, name too short".to_string(), raw: RawPacket { destination_port: "p".to_string(), - ..default_raw_msg.clone() + ..default_raw_packet.clone() }, want_pass: false, }, @@ -381,7 +444,7 @@ mod tests { name: "Bad dst port, name too long".to_string(), raw: RawPacket { destination_port: "abcdefghijasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfadgasgasdfasdfasdfasdfaklmnopqrstuabcdefghijasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfadgasgasdfas".to_string(), - ..default_raw_msg.clone() + ..default_raw_packet.clone() }, want_pass: false, }, @@ -389,7 +452,7 @@ mod tests { name: "Src channel validation: correct".to_string(), raw: RawPacket { source_channel: "channel-1".to_string(), - ..default_raw_msg.clone() + ..default_raw_packet.clone() }, want_pass: true, }, @@ -397,7 +460,7 @@ mod tests { name: "Bad src channel, name too short".to_string(), raw: RawPacket { source_channel: "p".to_string(), - ..default_raw_msg.clone() + ..default_raw_packet.clone() }, want_pass: false, }, @@ -405,7 +468,7 @@ mod tests { name: "Bad src channel, name too long".to_string(), raw: RawPacket { source_channel: "channel-12839128379182739812739879".to_string(), - ..default_raw_msg.clone() + ..default_raw_packet.clone() }, want_pass: false, }, @@ -413,7 +476,7 @@ mod tests { name: "Dst channel validation: correct".to_string(), raw: RawPacket { destination_channel: "channel-34".to_string(), - ..default_raw_msg.clone() + ..default_raw_packet.clone() }, want_pass: true, }, @@ -421,7 +484,7 @@ mod tests { name: "Bad dst channel, name too short".to_string(), raw: RawPacket { destination_channel: "p".to_string(), - ..default_raw_msg.clone() + ..default_raw_packet.clone() }, want_pass: false, }, @@ -429,17 +492,24 @@ mod tests { name: "Bad dst channel, name too long".to_string(), raw: RawPacket { destination_channel: "channel-12839128379182739812739879".to_string(), - ..default_raw_msg.clone() + ..default_raw_packet.clone() }, want_pass: false, }, + // Note: `timeout_height == None` is a quirk of protobuf. In + // `proto3` syntax, all structs are "nullable" by default and are + // represented as `Option`. `ibc-go` defines the protobuf file + // with the extension option `gogoproto.nullable = false`, which + // means that they will always generate a field. It is left + // unspecified what a `None` value means. In this case, I believe it + // is best to assume the obvious semantic of "no timeout". Test { name: "Missing timeout height".to_string(), raw: RawPacket { timeout_height: None, - ..default_raw_msg + ..default_raw_packet }, - want_pass: false, + want_pass: true, }, ]; diff --git a/modules/src/core/ics04_channel/timeout.rs b/modules/src/core/ics04_channel/timeout.rs new file mode 100644 index 0000000000..ed4a5641f5 --- /dev/null +++ b/modules/src/core/ics04_channel/timeout.rs @@ -0,0 +1,185 @@ +use core::fmt::Display; + +use serde::{Deserialize, Serialize}; + +use ibc_proto::ibc::core::client::v1::Height as RawHeight; +use tendermint::abci::tag::Value as TagValue; + +use crate::core::ics02_client::{error::Error as ICS2Error, height::Height}; +use crate::prelude::*; + +/// Indicates a consensus height on the destination chain after which the packet +/// will no longer be processed, and will instead count as having timed-out. +/// +/// `TimeoutHeight` is treated differently from other heights because +/// +/// `RawHeight.timeout_height == {revision_number: 0, revision_height = 0}` +/// +/// is legal and meaningful, even though the Tendermint spec rejects this height +/// as invalid. Thus, it must be parsed specially, where this special case means +/// "no timeout". +#[derive(Clone, Copy, Debug, Hash, Eq, PartialEq)] +pub enum TimeoutHeight { + Never, + At(Height), +} + +impl TimeoutHeight { + pub fn no_timeout() -> Self { + Self::Never + } + + /// Revision number to be used in packet commitment computation + pub fn commitment_revision_number(&self) -> u64 { + match self { + Self::At(height) => height.revision_number(), + Self::Never => 0, + } + } + + /// Revision height to be used in packet commitment computation + pub fn commitment_revision_height(&self) -> u64 { + match self { + Self::At(height) => height.revision_height(), + Self::Never => 0, + } + } + + /// Check if a height is *stricly past* the timeout height, and thus is + /// deemed expired. + pub fn has_expired(&self, height: Height) -> bool { + match self { + Self::At(timeout_height) => height > *timeout_height, + // When there's no timeout, heights are never expired + Self::Never => false, + } + } +} + +impl Default for TimeoutHeight { + fn default() -> Self { + Self::Never + } +} + +impl TryFrom for TimeoutHeight { + type Error = ICS2Error; + + // Note: it is important for `revision_number` to also be `0`, otherwise + // packet commitment proofs will be incorrect (proof construction in + // `ChannelReader::packet_commitment()` uses both `revision_number` and + // `revision_height`). Note also that ibc-go conforms to this convention. + fn try_from(raw_height: RawHeight) -> Result { + if raw_height.revision_number == 0 && raw_height.revision_height == 0 { + Ok(TimeoutHeight::Never) + } else { + let height: Height = raw_height.try_into()?; + Ok(TimeoutHeight::At(height)) + } + } +} + +impl TryFrom> for TimeoutHeight { + type Error = ICS2Error; + + fn try_from(maybe_raw_height: Option) -> Result { + match maybe_raw_height { + Some(raw_height) => Self::try_from(raw_height), + None => Ok(TimeoutHeight::Never), + } + } +} +/// We map "no timeout height" to `Some(RawHeight::zero)` due to a quirk +/// in ICS-4. See . +impl From for Option { + fn from(timeout_height: TimeoutHeight) -> Self { + let raw_height = match timeout_height { + TimeoutHeight::At(height) => height.into(), + TimeoutHeight::Never => RawHeight { + revision_number: 0, + revision_height: 0, + }, + }; + + Some(raw_height) + } +} + +impl From for TimeoutHeight { + fn from(height: Height) -> Self { + Self::At(height) + } +} + +impl From for TagValue { + fn from(timeout_height: TimeoutHeight) -> Self { + match timeout_height { + TimeoutHeight::At(height) => height.to_string().parse().unwrap(), + TimeoutHeight::Never => "0-0".parse().unwrap(), + } + } +} + +impl Display for TimeoutHeight { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + match self { + TimeoutHeight::At(timeout_height) => write!(f, "{}", timeout_height), + TimeoutHeight::Never => write!(f, "no timeout"), + } + } +} + +impl Serialize for TimeoutHeight { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + // When there is no timeout, we cannot construct an ICS02 Height with + // revision number and height at zero, so we have to define an + // isomorphic struct to serialize it as if it were an ICS02 height. + #[derive(Serialize)] + struct Height { + revision_number: u64, + revision_height: u64, + } + + match self { + // If there is no timeout, we use our ad-hoc struct above + TimeoutHeight::Never => { + let zero = Height { + revision_number: 0, + revision_height: 0, + }; + + zero.serialize(serializer) + } + // Otherwise we can directly serialize the underlying height + TimeoutHeight::At(height) => height.serialize(serializer), + } + } +} + +impl<'de> Deserialize<'de> for TimeoutHeight { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + use crate::core::ics02_client::height::Height as Ics02Height; + + // Here we have to use a bespoke struct as well in order to deserialize + // a height which may have a revision height equal to zero. + #[derive(Deserialize)] + struct Height { + revision_number: u64, + revision_height: u64, + } + + Height::deserialize(deserializer).map(|height| { + Ics02Height::new(height.revision_number, height.revision_height) + // If it's a valid height with a non-zero revision height, then we have a timeout + .map(TimeoutHeight::At) + // Otherwise, no timeout + .unwrap_or(TimeoutHeight::Never) + }) + } +} diff --git a/modules/src/core/ics26_routing/handler.rs b/modules/src/core/ics26_routing/handler.rs index 659eb877be..94f6d64e0c 100644 --- a/modules/src/core/ics26_routing/handler.rs +++ b/modules/src/core/ics26_routing/handler.rs @@ -129,6 +129,7 @@ where #[cfg(test)] mod tests { + use crate::core::ics04_channel::timeout::TimeoutHeight; use crate::prelude::*; use test_log::test; @@ -212,15 +213,15 @@ mod tests { } let default_signer = get_dummy_account_id(); let client_height = 5; - let start_client_height = Height::new(0, client_height); - let update_client_height = Height::new(0, 34); - let update_client_height_after_send = Height::new(0, 35); + let start_client_height = Height::new(0, client_height).unwrap(); + let update_client_height = Height::new(0, 34).unwrap(); + let update_client_height_after_send = Height::new(0, 35).unwrap(); - let update_client_height_after_second_send = Height::new(0, 36); + let update_client_height_after_second_send = Height::new(0, 36).unwrap(); - let upgrade_client_height = Height::new(1, 2); + let upgrade_client_height = Height::new(1, 2).unwrap(); - let upgrade_client_height_second = Height::new(1, 1); + let upgrade_client_height_second = Height::new(1, 1).unwrap(); let transfer_module_id: ModuleId = MODULE_ID_STR.parse().unwrap(); @@ -291,14 +292,18 @@ mod tests { MsgChannelCloseConfirm::try_from(get_dummy_raw_msg_chan_close_confirm(client_height)) .unwrap(); - let msg_transfer = get_dummy_msg_transfer(35); - let msg_transfer_two = get_dummy_msg_transfer(36); + let msg_transfer = get_dummy_msg_transfer(Height::new(0, 35).unwrap().into(), None); + let msg_transfer_two = get_dummy_msg_transfer(Height::new(0, 36).unwrap().into(), None); + let msg_transfer_no_timeout = get_dummy_msg_transfer(TimeoutHeight::no_timeout(), None); + let msg_transfer_no_timeout_or_timestamp = get_dummy_msg_transfer( + TimeoutHeight::no_timeout(), + Some(Timestamp::from_nanoseconds(0).unwrap()), + ); let mut msg_to_on_close = MsgTimeoutOnClose::try_from(get_dummy_raw_msg_timeout_on_close(36, 5)).unwrap(); msg_to_on_close.packet.sequence = 2.into(); - msg_to_on_close.packet.timeout_height = - msg_transfer_two.timeout_height.unwrap_or_else(Height::zero); + msg_to_on_close.packet.timeout_height = msg_transfer_two.timeout_height; msg_to_on_close.packet.timeout_timestamp = msg_transfer_two.timeout_timestamp; let denom = msg_transfer_two.token.denom.clone(); @@ -471,6 +476,17 @@ mod tests { .into(), want_pass: true, }, + // Timeout packets + Test { + name: "Transfer message no timeout".to_string(), + msg: msg_transfer_no_timeout.into(), + want_pass: true, + }, + Test { + name: "Transfer message no timeout nor timestamp".to_string(), + msg: msg_transfer_no_timeout_or_timestamp.into(), + want_pass: true, + }, //ICS04-close channel Test { name: "Channel close init succeeds".to_string(), diff --git a/modules/src/events.rs b/modules/src/events.rs index 58e6d54e2b..60a74e337a 100644 --- a/modules/src/events.rs +++ b/modules/src/events.rs @@ -12,7 +12,6 @@ use tendermint::abci::Event as AbciEvent; use crate::core::ics02_client::error as client_error; use crate::core::ics02_client::events as ClientEvents; use crate::core::ics02_client::events::NewBlock; -use crate::core::ics02_client::height::HeightError; use crate::core::ics03_connection::events as ConnectionEvents; use crate::core::ics03_connection::events::Attributes as ConnectionAttributes; use crate::core::ics04_channel::error as channel_error; @@ -27,7 +26,6 @@ use crate::Height; define_error! { Error { Height - [ HeightError ] | _ | { "error parsing height" }, Parse diff --git a/modules/src/mock/client_def.rs b/modules/src/mock/client_def.rs index 427d7f3e29..2a0d27b201 100644 --- a/modules/src/mock/client_def.rs +++ b/modules/src/mock/client_def.rs @@ -62,8 +62,8 @@ impl ClientDef for MockClient { ) -> Result<(), Error> { let client_prefixed_path = Path::ClientConsensusState(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(), }) .to_string(); diff --git a/modules/src/mock/context.rs b/modules/src/mock/context.rs index 07ed127984..644a059cbd 100644 --- a/modules/src/mock/context.rs +++ b/modules/src/mock/context.rs @@ -85,7 +85,7 @@ impl Default for MockContext { ChainId::new("mockgaia".to_string(), 0), HostType::Mock, 5, - Height::new(0, 5), + Height::new(0, 5).unwrap(), ) } } @@ -129,16 +129,17 @@ impl MockContext { ); assert_ne!( - latest_height.revision_height, 0, + latest_height.revision_height(), + 0, "The chain must have a non-zero revision_height" ); // Compute the number of blocks to store. - let n = min(max_history_size as u64, latest_height.revision_height); + let n = min(max_history_size as u64, latest_height.revision_height()); assert_eq!( host_id.version(), - latest_height.revision_number, + latest_height.revision_number(), "The version in the chain identifier must match the version in the latest height" ); @@ -156,7 +157,7 @@ impl MockContext { HostBlock::generate_block( host_id.clone(), host_type, - latest_height.sub(i).unwrap().revision_height, + latest_height.sub(i).unwrap().revision_height(), next_block_timestamp .sub(Duration::from_secs(DEFAULT_BLOCK_TIME_SECS * (i + 1))) .unwrap(), @@ -202,7 +203,7 @@ impl MockContext { ClientType::Tendermint => { let light_block = HostBlock::generate_tm_block( self.host_chain_id.clone(), - cs_height.revision_height, + cs_height.revision_height(), Timestamp::now(), ); @@ -254,7 +255,7 @@ impl MockContext { ClientType::Tendermint => { let light_block = HostBlock::generate_tm_block( self.host_chain_id.clone(), - cs_height.revision_height, + cs_height.revision_height(), now, ); @@ -274,7 +275,7 @@ impl MockContext { ClientType::Tendermint => { let light_block = HostBlock::generate_tm_block( self.host_chain_id.clone(), - prev_cs_height.revision_height, + prev_cs_height.revision_height(), now.sub(self.block_time).unwrap(), ); AnyConsensusState::from(light_block) @@ -369,16 +370,16 @@ impl MockContext { pub fn with_height(self, target_height: Height) -> Self { let latest_height = self.latest_height(); - if target_height.revision_number > latest_height.revision_number { + if target_height.revision_number() > latest_height.revision_number() { unimplemented!() - } else if target_height.revision_number < latest_height.revision_number { + } else if target_height.revision_number() < latest_height.revision_number() { panic!("Cannot rewind history of the chain to a smaller revision number!") - } else if target_height.revision_height < latest_height.revision_height { + } else if target_height.revision_height() < latest_height.revision_height() { panic!("Cannot rewind history of the chain to a smaller revision height!") - } else if target_height.revision_height > latest_height.revision_height { + } else if target_height.revision_height() > latest_height.revision_height() { // Repeatedly advance the host chain height till we hit the desired height let mut ctx = MockContext { ..self }; - while ctx.latest_height().revision_height < target_height.revision_height { + while ctx.latest_height().revision_height() < target_height.revision_height() { ctx.advance_host_chain_height() } ctx @@ -408,8 +409,8 @@ impl MockContext { /// Accessor for a block of the local (host) chain from this context. /// Returns `None` if the block at the requested height does not exist. pub fn host_block(&self, target_height: Height) -> Option<&HostBlock> { - let target = target_height.revision_height as usize; - let latest = self.latest_height().revision_height as usize; + let target = target_height.revision_height() as usize; + let latest = self.latest_height().revision_height() as usize; // Check that the block is not too advanced, nor has it been pruned. if (target > latest) || (target <= latest - self.history.len()) { @@ -425,7 +426,7 @@ impl MockContext { let new_block = HostBlock::generate_block( self.host_chain_id.clone(), self.host_chain_type, - latest_block.height().increment().revision_height, + latest_block.height().increment().revision_height(), latest_block.timestamp().add(self.block_time).unwrap(), ); @@ -1170,7 +1171,7 @@ impl ClientReader for MockContext { } fn pending_host_consensus_state(&self) -> Result { - Err(Ics02Error::missing_local_consensus_state(Height::zero())) + Err(Ics02Error::implementation_specific()) } fn client_counter(&self) -> Result { @@ -1345,7 +1346,7 @@ mod tests { ChainId::new("mockgaia".to_string(), cv), HostType::Mock, 2, - Height::new(cv, 1), + Height::new(cv, 1).unwrap(), ), }, Test { @@ -1354,7 +1355,7 @@ mod tests { ChainId::new("mocksgaia".to_string(), cv), HostType::SyntheticTendermint, 2, - Height::new(cv, 1), + Height::new(cv, 1).unwrap(), ), }, Test { @@ -1363,7 +1364,7 @@ mod tests { ChainId::new("mockgaia".to_string(), cv), HostType::Mock, 30, - Height::new(cv, 2), + Height::new(cv, 2).unwrap(), ), }, Test { @@ -1372,7 +1373,7 @@ mod tests { ChainId::new("mocksgaia".to_string(), cv), HostType::SyntheticTendermint, 30, - Height::new(cv, 2), + Height::new(cv, 2).unwrap(), ), }, Test { @@ -1381,7 +1382,7 @@ mod tests { ChainId::new("mockgaia".to_string(), cv), HostType::Mock, 3, - Height::new(cv, 30), + Height::new(cv, 30).unwrap(), ), }, Test { @@ -1390,7 +1391,7 @@ mod tests { ChainId::new("mockgaia".to_string(), cv), HostType::SyntheticTendermint, 3, - Height::new(cv, 30), + Height::new(cv, 30).unwrap(), ), }, Test { @@ -1399,7 +1400,7 @@ mod tests { ChainId::new("mockgaia".to_string(), cv), HostType::Mock, 3, - Height::new(cv, 2), + Height::new(cv, 2).unwrap(), ), }, Test { @@ -1408,7 +1409,7 @@ mod tests { ChainId::new("mockgaia".to_string(), cv), HostType::SyntheticTendermint, 3, - Height::new(cv, 2), + Height::new(cv, 2).unwrap(), ), }, Test { @@ -1417,7 +1418,7 @@ mod tests { ChainId::new("mockgaia".to_string(), cv), HostType::Mock, 50, - Height::new(cv, 2000), + Height::new(cv, 2000).unwrap(), ), }, Test { @@ -1426,7 +1427,7 @@ mod tests { ChainId::new("mockgaia".to_string(), cv), HostType::SyntheticTendermint, 50, - Height::new(cv, 2000), + Height::new(cv, 2000).unwrap(), ), }, ]; @@ -1458,15 +1459,14 @@ mod tests { "failed while increasing height for context {:?}", test.ctx ); - if current_height > Height::new(cv, 0) { - assert_eq!( - test.ctx.host_block(current_height).unwrap().height(), - current_height, - "failed while fetching height {:?} of context {:?}", - current_height, - test.ctx - ); - } + + assert_eq!( + test.ctx.host_block(current_height).unwrap().height(), + current_height, + "failed while fetching height {:?} of context {:?}", + current_height, + test.ctx + ); } } @@ -1550,7 +1550,7 @@ mod tests { ChainId::new("mockgaia".to_string(), 1), HostType::Mock, 1, - Height::new(1, 1), + Height::new(1, 1).unwrap(), ) .with_router(r); diff --git a/modules/src/mock/header.rs b/modules/src/mock/header.rs index 381975ae48..a03482d208 100644 --- a/modules/src/mock/header.rs +++ b/modules/src/mock/header.rs @@ -12,12 +12,21 @@ use crate::mock::client_state::MockConsensusState; use crate::timestamp::Timestamp; use crate::Height; -#[derive(Copy, Clone, Default, Debug, Deserialize, PartialEq, Eq, Serialize)] +#[derive(Copy, Clone, Debug, Deserialize, PartialEq, Eq, Serialize)] pub struct MockHeader { pub height: Height, pub timestamp: Timestamp, } +impl Default for MockHeader { + fn default() -> Self { + Self { + height: Height::new(0, 1).unwrap(), + timestamp: Default::default(), + } + } +} + impl Protobuf for MockHeader {} impl TryFrom for MockHeader { @@ -25,7 +34,10 @@ impl TryFrom for MockHeader { fn try_from(raw: RawMockHeader) -> Result { Ok(MockHeader { - height: raw.height.ok_or_else(Error::missing_raw_header)?.into(), + height: raw + .height + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(Error::missing_raw_header)?, timestamp: Timestamp::from_nanoseconds(raw.timestamp) .map_err(Error::invalid_packet_timestamp)?, @@ -95,7 +107,7 @@ mod tests { #[test] fn encode_any() { - let header = MockHeader::new(Height::new(1, 10)).with_timestamp(Timestamp::none()); + let header = MockHeader::new(Height::new(1, 10).unwrap()).with_timestamp(Timestamp::none()); let bytes = header.wrap_any().encode_vec().unwrap(); assert_eq!( diff --git a/modules/src/mock/host.rs b/modules/src/mock/host.rs index d94b90f893..cc3a813f93 100644 --- a/modules/src/mock/host.rs +++ b/modules/src/mock/host.rs @@ -40,7 +40,8 @@ impl HostBlock { HostBlock::SyntheticTendermint(light_block) => Height::new( ChainId::chain_version(light_block.signed_header.header.chain_id.as_str()), light_block.signed_header.header.height.value(), - ), + ) + .unwrap(), } } @@ -63,7 +64,7 @@ impl HostBlock { ) -> HostBlock { match chain_type { HostType::Mock => HostBlock::Mock(MockHeader { - height: Height::new(chain_id.version(), height), + height: Height::new(chain_id.version(), height).unwrap(), timestamp, }), HostType::SyntheticTendermint => HostBlock::SyntheticTendermint(Box::new( @@ -117,7 +118,7 @@ impl From for TMHeader { TMHeader { signed_header: light_block.signed_header, validator_set: light_block.validators, - trusted_height: Default::default(), + trusted_height: Height::new(0, 1).unwrap(), trusted_validator_set: light_block.next_validators, } } diff --git a/modules/src/proofs.rs b/modules/src/proofs.rs index 30e84aa726..dead167490 100644 --- a/modules/src/proofs.rs +++ b/modules/src/proofs.rs @@ -37,10 +37,6 @@ impl Proofs { other_proof: Option, height: Height, ) -> Result { - if height.is_zero() { - return Err(ProofError::zero_height()); - } - Ok(Self { object_proof, client_proof, @@ -89,10 +85,6 @@ impl ConsensusProof { consensus_proof: CommitmentProofBytes, consensus_height: Height, ) -> Result { - if consensus_height.is_zero() { - return Err(ProofError::zero_height()); - } - Ok(Self { proof: consensus_proof, height: consensus_height, diff --git a/modules/src/relayer/ics18_relayer/utils.rs b/modules/src/relayer/ics18_relayer/utils.rs index 34cd79cd7a..8afa5df569 100644 --- a/modules/src/relayer/ics18_relayer/utils.rs +++ b/modules/src/relayer/ics18_relayer/utils.rs @@ -67,10 +67,10 @@ mod tests { /// Implements a "ping pong" of client update messages, so that two chains repeatedly /// process a client update message and update their height in succession. fn client_update_ping_pong() { - let chain_a_start_height = Height::new(1, 11); - let chain_b_start_height = Height::new(1, 20); - let client_on_b_for_a_height = Height::new(1, 10); // Should be smaller than `chain_a_start_height` - let client_on_a_for_b_height = Height::new(1, 20); // Should be smaller than `chain_b_start_height` + let chain_a_start_height = Height::new(1, 11).unwrap(); + let chain_b_start_height = Height::new(1, 20).unwrap(); + let client_on_b_for_a_height = Height::new(1, 10).unwrap(); // Should be smaller than `chain_a_start_height` + let client_on_a_for_b_height = Height::new(1, 20).unwrap(); // Should be smaller than `chain_b_start_height` let num_iterations = 4; let client_on_a_for_b = ClientId::new(ClientType::Tendermint, 0).unwrap(); diff --git a/modules/src/test_utils.rs b/modules/src/test_utils.rs index 9dbbe0596b..d8be3ac7d0 100644 --- a/modules/src/test_utils.rs +++ b/modules/src/test_utils.rs @@ -343,7 +343,7 @@ impl ChannelReader for DummyTransferModule { } fn host_height(&self) -> Height { - Height::zero() + Height::new(0, 1).unwrap() } fn host_consensus_state(&self, _height: Height) -> Result { diff --git a/modules/tests/runner/mod.rs b/modules/tests/runner/mod.rs index 061b1ef81f..0fc772b711 100644 --- a/modules/tests/runner/mod.rs +++ b/modules/tests/runner/mod.rs @@ -122,7 +122,7 @@ impl IbcTestRunner { } pub fn chain_id(chain_id: String, height: Height) -> ChainId { - ChainId::new(chain_id, height.revision_number) + ChainId::new(chain_id, height.revision_number()) } pub fn version() -> Version { @@ -143,7 +143,7 @@ impl IbcTestRunner { } pub fn height(height: Height) -> Height { - Height::new(height.revision_number, height.revision_height) + Height::new(height.revision_number(), height.revision_height()).unwrap() } fn mock_header(height: Height) -> MockHeader { diff --git a/relayer-cli/src/commands/keys/list.rs b/relayer-cli/src/commands/keys/list.rs index 7bd09db4eb..b19ac785d8 100644 --- a/relayer-cli/src/commands/keys/list.rs +++ b/relayer-cli/src/commands/keys/list.rs @@ -1,4 +1,5 @@ use alloc::collections::btree_map::BTreeMap as HashMap; +use core::fmt::Write; use abscissa_core::clap::Parser; use abscissa_core::{Command, Runnable}; @@ -52,7 +53,7 @@ impl Runnable for KeysListCmd { Ok(keys) => { let mut msg = String::new(); for (name, key) in keys { - msg.push_str(&format!("\n- {} ({})", name, key.account)); + let _ = write!(msg, "\n- {} ({})", name, key.account); } Output::success_msg(msg).exit() } diff --git a/relayer-cli/src/commands/query/channel.rs b/relayer-cli/src/commands/query/channel.rs index a30c29ef69..5245d9d8b6 100644 --- a/relayer-cli/src/commands/query/channel.rs +++ b/relayer-cli/src/commands/query/channel.rs @@ -60,7 +60,10 @@ impl Runnable for QueryChannelEndCmd { port_id: self.port_id.clone(), channel_id: self.channel_id, height: self.height.map_or(QueryHeight::Latest, |revision_height| { - QueryHeight::Specific(ibc::Height::new(chain.id().version(), revision_height)) + QueryHeight::Specific( + ibc::Height::new(chain.id().version(), revision_height) + .unwrap_or_else(exit_with_unrecoverable_error), + ) }), }, IncludeProof::No, diff --git a/relayer-cli/src/commands/query/channel_ends.rs b/relayer-cli/src/commands/query/channel_ends.rs index c667dcf6c4..414e4d518c 100644 --- a/relayer-cli/src/commands/query/channel_ends.rs +++ b/relayer-cli/src/commands/query/channel_ends.rs @@ -14,7 +14,7 @@ use ibc_relayer::chain::requests::{ }; use ibc_relayer::registry::Registry; -use crate::conclude::Output; +use crate::conclude::{exit_with_unrecoverable_error, Output}; use crate::prelude::*; #[derive(Clone, Command, Debug, Parser)] @@ -96,7 +96,9 @@ fn do_run(cmd: &QueryChannelEndsCmd) -> Result<(), Box Height::new(chain.id().version(), height), + Some(height) => { + Height::new(chain.id().version(), height).unwrap_or_else(exit_with_unrecoverable_error) + } None => chain.query_latest_height()?, }; diff --git a/relayer-cli/src/commands/query/client.rs b/relayer-cli/src/commands/query/client.rs index 9614085ea8..21038efa1a 100644 --- a/relayer-cli/src/commands/query/client.rs +++ b/relayer-cli/src/commands/query/client.rs @@ -59,7 +59,10 @@ impl Runnable for QueryClientStateCmd { QueryClientStateRequest { client_id: self.client_id.clone(), height: self.height.map_or(QueryHeight::Latest, |revision_height| { - QueryHeight::Specific(ibc::Height::new(chain.id().version(), revision_height)) + QueryHeight::Specific( + ibc::Height::new(chain.id().version(), revision_height) + .unwrap_or_else(exit_with_unrecoverable_error), + ) }), }, IncludeProof::No, @@ -135,7 +138,8 @@ impl Runnable for QueryClientConsensusCmd { match self.consensus_height { Some(cs_height) => { - let consensus_height = ibc::Height::new(counterparty_chain.version(), cs_height); + let consensus_height = ibc::Height::new(counterparty_chain.version(), cs_height) + .unwrap_or_else(exit_with_unrecoverable_error); let res = chain .query_consensus_state( @@ -145,10 +149,10 @@ impl Runnable for QueryClientConsensusCmd { query_height: self.height.map_or( QueryHeight::Latest, |revision_height| { - QueryHeight::Specific(ibc::Height::new( - chain.id().version(), - revision_height, - )) + QueryHeight::Specific( + ibc::Height::new(chain.id().version(), revision_height) + .unwrap_or_else(exit_with_unrecoverable_error), + ) }, ), }, @@ -212,7 +216,7 @@ pub struct QueryClientHeaderCmd { #[clap( long = "height", value_name = "HEIGHT", - help = "The chain height context for the query" + help = "The chain height context for the query. Leave unspecified for latest height." )] height: Option, } @@ -244,12 +248,14 @@ impl Runnable for QueryClientHeaderCmd { }; let consensus_height = - ibc::Height::new(counterparty_chain.version(), self.consensus_height); + ibc::Height::new(counterparty_chain.version(), self.consensus_height) + .unwrap_or_else(exit_with_unrecoverable_error); let query_height = match self.height { - Some(revision_height) => { - QueryHeight::Specific(Height::new(chain.id().version(), revision_height)) - } + Some(revision_height) => QueryHeight::Specific( + Height::new(chain.id().version(), revision_height) + .unwrap_or_else(exit_with_unrecoverable_error), + ), None => QueryHeight::Latest, }; diff --git a/relayer-cli/src/commands/query/connection.rs b/relayer-cli/src/commands/query/connection.rs index bce0250934..6e6fb60ef1 100644 --- a/relayer-cli/src/commands/query/connection.rs +++ b/relayer-cli/src/commands/query/connection.rs @@ -38,7 +38,7 @@ pub struct QueryConnectionEndCmd { #[clap( long = "height", value_name = "HEIGHT", - help = "Height of the state to query" + help = "Height of the state to query. Leave unspecified for latest height." )] height: Option, } @@ -57,7 +57,10 @@ impl Runnable for QueryConnectionEndCmd { QueryConnectionRequest { connection_id: self.connection_id.clone(), height: self.height.map_or(QueryHeight::Latest, |revision_height| { - QueryHeight::Specific(ibc::Height::new(chain.id().version(), revision_height)) + QueryHeight::Specific( + ibc::Height::new(chain.id().version(), revision_height) + .unwrap_or_else(exit_with_unrecoverable_error), + ) }), }, IncludeProof::No, diff --git a/relayer-cli/src/commands/query/packet/ack.rs b/relayer-cli/src/commands/query/packet/ack.rs index a7acd62e39..b255970e52 100644 --- a/relayer-cli/src/commands/query/packet/ack.rs +++ b/relayer-cli/src/commands/query/packet/ack.rs @@ -8,7 +8,7 @@ use ibc::core::ics24_host::identifier::{ChainId, ChannelId, PortId}; use ibc_relayer::chain::handle::ChainHandle; use crate::cli_utils::spawn_chain_runtime; -use crate::conclude::Output; +use crate::conclude::{exit_with_unrecoverable_error, Output}; use crate::error::Error; use crate::prelude::*; @@ -51,7 +51,7 @@ pub struct QueryPacketAcknowledgmentCmd { #[clap( long = "height", value_name = "HEIGHT", - help = "Height of the state to query" + help = "Height of the state to query. Leave unspecified for latest height." )] height: Option, } @@ -71,10 +71,10 @@ impl QueryPacketAcknowledgmentCmd { channel_id: self.channel_id, sequence: self.sequence, height: self.height.map_or(QueryHeight::Latest, |revision_height| { - QueryHeight::Specific(ibc::Height::new( - chain.id().version(), - revision_height, - )) + QueryHeight::Specific( + ibc::Height::new(chain.id().version(), revision_height) + .unwrap_or_else(exit_with_unrecoverable_error), + ) }), }, IncludeProof::No, diff --git a/relayer-cli/src/commands/query/packet/commitment.rs b/relayer-cli/src/commands/query/packet/commitment.rs index 3df8986f3c..64535352a2 100644 --- a/relayer-cli/src/commands/query/packet/commitment.rs +++ b/relayer-cli/src/commands/query/packet/commitment.rs @@ -10,7 +10,7 @@ use ibc::Height; use ibc_relayer::chain::handle::ChainHandle; use crate::cli_utils::spawn_chain_runtime; -use crate::conclude::Output; +use crate::conclude::{exit_with_unrecoverable_error, Output}; use crate::error::Error; use crate::prelude::*; @@ -59,7 +59,7 @@ pub struct QueryPacketCommitmentCmd { #[clap( long = "height", value_name = "HEIGHT", - help = "Height of the state to query" + help = "Height of the state to query. Leave unspecified for latest height." )] height: Option, } @@ -79,10 +79,10 @@ impl QueryPacketCommitmentCmd { channel_id: self.channel_id, sequence: self.sequence, height: self.height.map_or(QueryHeight::Latest, |revision_height| { - QueryHeight::Specific(ibc::Height::new( - chain.id().version(), - revision_height, - )) + QueryHeight::Specific( + ibc::Height::new(chain.id().version(), revision_height) + .unwrap_or_else(exit_with_unrecoverable_error), + ) }), }, IncludeProof::No, diff --git a/relayer-cli/src/commands/tx/client.rs b/relayer-cli/src/commands/tx/client.rs index 54745f1cf9..459eec5d48 100644 --- a/relayer-cli/src/commands/tx/client.rs +++ b/relayer-cli/src/commands/tx/client.rs @@ -122,14 +122,14 @@ pub struct TxUpdateClientCmd { #[clap( long = "height", value_name = "REFERENCE_HEIGHT", - help = "The target height of the client update" + help = "The target height of the client update. Leave unspecified for latest height." )] target_height: Option, #[clap( long = "trusted-height", value_name = "REFERENCE_TRUSTED_HEIGHT", - help = "The trusted height of the client update" + help = "The trusted height of the client update. Leave unspecified for latest height." )] trusted_height: Option, } @@ -166,12 +166,16 @@ impl Runnable for TxUpdateClientCmd { }; let target_height = self.target_height.map_or(QueryHeight::Latest, |height| { - QueryHeight::Specific(Height::new(src_chain.id().version(), height)) + QueryHeight::Specific( + Height::new(src_chain.id().version(), height) + .unwrap_or_else(exit_with_unrecoverable_error), + ) }); - let trusted_height = self - .trusted_height - .map(|height| Height::new(src_chain.id().version(), height)); + let trusted_height = self.trusted_height.map(|height| { + Height::new(src_chain.id().version(), height) + .unwrap_or_else(exit_with_unrecoverable_error) + }); let client = ForeignClient::find(src_chain, dst_chain, &self.dst_client_id) .unwrap_or_else(exit_with_unrecoverable_error); diff --git a/relayer-cli/src/commands/tx/transfer.rs b/relayer-cli/src/commands/tx/transfer.rs index a8baa44ba4..387774c7f6 100644 --- a/relayer-cli/src/commands/tx/transfer.rs +++ b/relayer-cli/src/commands/tx/transfer.rs @@ -142,14 +142,6 @@ impl TxIcs20MsgTransferCmd { return Err("number of messages should be greater than zero".into()); } - if self.timeout_height_offset == 0 && self.timeout_seconds == 0 { - return Err( - "packet timeout height and packet timeout timestamp cannot both be 0, \ - please specify either --timeout-height-offset or --timeout-seconds" - .into(), - ); - } - let opts = TransferOptions { packet_src_port_id: self.src_port_id.clone(), packet_src_channel_id: self.src_channel_id, diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index d1c6f3219a..9ec187bb88 100644 --- a/relayer/src/chain/cosmos.rs +++ b/relayer/src/chain/cosmos.rs @@ -173,7 +173,7 @@ impl CosmosSdkChain { } // Get the latest height and convert to tendermint Height - let latest_height = TmHeight::try_from(self.query_chain_latest_height()?.revision_height) + let latest_height = TmHeight::try_from(self.query_chain_latest_height()?.revision_height()) .map_err(Error::invalid_height)?; // Check on the configured max_tx_size against the consensus parameters at latest height @@ -347,7 +347,7 @@ impl CosmosSdkChain { &self.config.rpc_addr, path, Path::Upgrade(query_data).to_string(), - TmHeight::try_from(query_height.revision_height).map_err(Error::invalid_height)?, + TmHeight::try_from(query_height.revision_height()).map_err(Error::invalid_height)?, true, ))?; @@ -712,10 +712,11 @@ impl ChainEndpoint for CosmosSdkChain { .block_metas; return if let Some(latest_app_block) = blocks.first() { - let height = ICSHeight { - revision_number: ChainId::chain_version(latest_app_block.header.chain_id.as_str()), - revision_height: u64::from(abci_info.last_block_height), - }; + let height = ICSHeight::new( + ChainId::chain_version(latest_app_block.header.chain_id.as_str()), + u64::from(abci_info.last_block_height), + ) + .map_err(|_| Error::invalid_height_no_source())?; let timestamp = latest_app_block.header.time.into(); Ok(ChainStatus { height, timestamp }) @@ -799,7 +800,7 @@ impl ChainEndpoint for CosmosSdkChain { .map_err(|_| Error::invalid_height_no_source())?; let (upgraded_client_state_raw, proof) = self.query_client_upgrade_state( - ClientUpgradePath::UpgradedClientState(upgrade_height.revision_height), + ClientUpgradePath::UpgradedClientState(upgrade_height.revision_height()), query_height, )?; @@ -823,7 +824,7 @@ impl ChainEndpoint for CosmosSdkChain { // Fetch the consensus state and its proof. let (upgraded_consensus_state_raw, proof) = self.query_client_upgrade_state( - ClientUpgradePath::UpgradedClientConsensusState(upgrade_height.revision_height), + ClientUpgradePath::UpgradedClientConsensusState(upgrade_height.revision_height()), query_height, )?; @@ -876,8 +877,8 @@ impl ChainEndpoint for CosmosSdkChain { let res = self.query( ClientConsensusStatePath { client_id: request.client_id.clone(), - epoch: request.consensus_height.revision_number, - height: request.consensus_height.revision_height, + epoch: request.consensus_height.revision_number(), + height: request.consensus_height.revision_height(), }, request.query_height, matches!(include_proof, IncludeProof::Yes), @@ -1223,8 +1224,8 @@ impl ChainEndpoint for CosmosSdkChain { let height = response .height - .ok_or_else(|| Error::grpc_response_param("height".to_string()))? - .into(); + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(|| Error::grpc_response_param("height".to_string()))?; Ok((commitment_sequences, height)) } @@ -1341,8 +1342,8 @@ impl ChainEndpoint for CosmosSdkChain { let height = response .height - .ok_or_else(|| Error::grpc_response_param("height".to_string()))? - .into(); + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(|| Error::grpc_response_param("height".to_string()))?; Ok((acks_sequences, height)) } @@ -1480,7 +1481,8 @@ impl ChainEndpoint for CosmosSdkChain { if let Some(block) = response.blocks.first().map(|first| &first.block) { let response_height = - ICSHeight::new(self.id().version(), u64::from(block.header.height)); + ICSHeight::new(self.id().version(), u64::from(block.header.height)) + .map_err(|_| Error::invalid_height_no_source())?; if let QueryHeight::Specific(query_height) = request.height { if response_height > query_height { @@ -1523,7 +1525,7 @@ impl ChainEndpoint for CosmosSdkChain { let height = match request.height { QueryHeight::Latest => TmHeight::from(0u32), QueryHeight::Specific(ibc_height) => { - TmHeight::try_from(ibc_height.revision_height).map_err(Error::invalid_height)? + TmHeight::try_from(ibc_height.revision_height()).map_err(Error::invalid_height)? } }; @@ -1748,15 +1750,21 @@ mod tests { let mut clients: Vec = vec![ IdentifiedAnyClientState::new( ClientId::new(ClientType::Tendermint, 4).unwrap(), - AnyClientState::Mock(MockClientState::new(MockHeader::new(Height::new(0, 0)))), + AnyClientState::Mock(MockClientState::new(MockHeader::new( + Height::new(0, 1).unwrap(), + ))), ), IdentifiedAnyClientState::new( ClientId::new(ClientType::Tendermint, 1).unwrap(), - AnyClientState::Mock(MockClientState::new(MockHeader::new(Height::new(0, 0)))), + AnyClientState::Mock(MockClientState::new(MockHeader::new( + Height::new(0, 1).unwrap(), + ))), ), IdentifiedAnyClientState::new( ClientId::new(ClientType::Tendermint, 7).unwrap(), - AnyClientState::Mock(MockClientState::new(MockHeader::new(Height::new(0, 0)))), + AnyClientState::Mock(MockClientState::new(MockHeader::new( + Height::new(0, 1).unwrap(), + ))), ), ]; clients.sort_by_cached_key(|c| client_id_suffix(&c.client_id).unwrap_or(0)); diff --git a/relayer/src/chain/cosmos/query.rs b/relayer/src/chain/cosmos/query.rs index 087f5d963b..18052561a5 100644 --- a/relayer/src/chain/cosmos/query.rs +++ b/relayer/src/chain/cosmos/query.rs @@ -59,7 +59,8 @@ pub fn header_query(request: &QueryClientEventRequest) -> Query { format!("{}.consensus_height", request.event_id.as_str()), format!( "{}-{}", - request.consensus_height.revision_number, request.consensus_height.revision_height + request.consensus_height.revision_number(), + request.consensus_height.revision_height() ), ) } diff --git a/relayer/src/chain/cosmos/query/status.rs b/relayer/src/chain/cosmos/query/status.rs index 2978f4ff43..c0bd82130d 100644 --- a/relayer/src/chain/cosmos/query/status.rs +++ b/relayer/src/chain/cosmos/query/status.rs @@ -28,10 +28,11 @@ pub async fn query_status( let time = response.sync_info.latest_block_time; - let height = Height { - revision_number: ChainId::chain_version(response.node_info.network.as_str()), - revision_height: u64::from(response.sync_info.latest_block_height), - }; + let height = Height::new( + ChainId::chain_version(response.node_info.network.as_str()), + u64::from(response.sync_info.latest_block_height), + ) + .map_err(|_| Error::invalid_height_no_source())?; Ok(ChainStatus { height, diff --git a/relayer/src/chain/cosmos/query/tx.rs b/relayer/src/chain/cosmos/query/tx.rs index 500986bafb..8a1f3b64f5 100644 --- a/relayer/src/chain/cosmos/query/tx.rs +++ b/relayer/src/chain/cosmos/query/tx.rs @@ -67,7 +67,7 @@ pub async fn query_txs( &request, *seq, response.txs[0].clone(), - ) { + )? { result.push(event); } } @@ -105,7 +105,7 @@ pub async fn query_txs( ); let tx = response.txs.remove(0); - let event = update_client_from_tx_search_response(chain_id, &request, tx); + let event = update_client_from_tx_search_response(chain_id, &request, tx)?; Ok(event.into_iter().collect()) } @@ -144,16 +144,17 @@ fn update_client_from_tx_search_response( chain_id: &ChainId, request: &QueryClientEventRequest, response: ResultTx, -) -> Option { - let height = ICSHeight::new(chain_id.version(), u64::from(response.height)); +) -> Result, Error> { + let height = ICSHeight::new(chain_id.version(), u64::from(response.height)) + .map_err(|_| Error::invalid_height_no_source())?; if let QueryHeight::Specific(specific_query_height) = request.query_height { if height > specific_query_height { - return None; + return Ok(None); } }; - response + Ok(response .tx_result .events .into_iter() @@ -170,7 +171,7 @@ fn update_client_from_tx_search_response( update.common.client_id == request.client_id && update.common.consensus_height == request.consensus_height }) - .map(IbcEvent::UpdateClient) + .map(IbcEvent::UpdateClient)) } // Extract the packet events from the query_txs RPC response. For any given @@ -185,19 +186,21 @@ fn packet_from_tx_search_response( request: &QueryPacketEventDataRequest, seq: Sequence, response: ResultTx, -) -> Option { - let height = ICSHeight::new(chain_id.version(), u64::from(response.height)); +) -> Result, Error> { + let height = ICSHeight::new(chain_id.version(), u64::from(response.height)) + .map_err(|_| Error::invalid_height_no_source())?; + if let QueryHeight::Specific(query_height) = request.height { if height > query_height { - return None; + return Ok(None); } } - response + Ok(response .tx_result .events .into_iter() - .find_map(|ev| filter_matching_event(ev, request, seq)) + .find_map(|ev| filter_matching_event(ev, request, seq))) } fn filter_matching_event( @@ -236,7 +239,7 @@ fn filter_matching_event( } fn all_ibc_events_from_tx_search_response(chain_id: &ChainId, response: ResultTx) -> Vec { - let height = ICSHeight::new(chain_id.version(), u64::from(response.height)); + let height = ICSHeight::new(chain_id.version(), u64::from(response.height)).unwrap(); let deliver_tx_result = response.tx_result; if deliver_tx_result.code.is_err() { return vec![IbcEvent::ChainError(format!( diff --git a/relayer/src/chain/mock.rs b/relayer/src/chain/mock.rs index 544a207c6c..fe47ba9841 100644 --- a/relayer/src/chain/mock.rs +++ b/relayer/src/chain/mock.rs @@ -90,7 +90,7 @@ impl ChainEndpoint for MockChain { config.id.clone(), HostType::SyntheticTendermint, 50, - Height::new(config.id.version(), 20), + Height::new(config.id.version(), 20).unwrap(), ), _event_sender: sender, event_receiver: receiver, diff --git a/relayer/src/chain/requests.rs b/relayer/src/chain/requests.rs index 705e9d9935..648d11df22 100644 --- a/relayer/src/chain/requests.rs +++ b/relayer/src/chain/requests.rs @@ -46,7 +46,7 @@ impl TryFrom for TMBlockHeight { fn try_from(height_query: QueryHeight) -> Result { let height = match height_query { QueryHeight::Latest => 0u64, - QueryHeight::Specific(height) => height.revision_height, + QueryHeight::Specific(height) => height.revision_height(), }; Self::try_from(height).map_err(Error::invalid_height) @@ -59,7 +59,7 @@ impl TryFrom for AsciiMetadataValue { fn try_from(height_query: QueryHeight) -> Result { let height = match height_query { QueryHeight::Latest => 0u64, - QueryHeight::Specific(height) => height.revision_height, + QueryHeight::Specific(height) => height.revision_height(), }; str::parse(&height.to_string()).map_err(Error::invalid_metadata) diff --git a/relayer/src/event/rpc.rs b/relayer/src/event/rpc.rs index 561e078a44..f3807bcd7c 100644 --- a/relayer/src/event/rpc.rs +++ b/relayer/src/event/rpc.rs @@ -128,7 +128,8 @@ pub fn get_all_events( let height = Height::new( ChainId::chain_version(chain_id.to_string().as_str()), u64::from(block.as_ref().ok_or("tx.height")?.header.height), - ); + ) + .map_err(|_| String::from("tx.height: invalid header height of 0"))?; vals.push((height, ClientEvents::NewBlock::new(height).into())); vals.append(&mut extract_block_events(height, &events)); @@ -137,7 +138,8 @@ pub fn get_all_events( let height = Height::new( ChainId::chain_version(chain_id.to_string().as_str()), tx_result.height as u64, - ); + ) + .map_err(|_| String::from("tx_result.height: invalid header height of 0"))?; for abci_event in &tx_result.result.events { if query == queries::ibc_client().to_string() { diff --git a/relayer/src/light_client/mock.rs b/relayer/src/light_client/mock.rs index de056e340e..4dd484c532 100644 --- a/relayer/src/light_client/mock.rs +++ b/relayer/src/light_client/mock.rs @@ -29,7 +29,7 @@ impl LightClient { /// Returns a LightBlock at the requested height `h`. fn light_block(&self, h: Height) -> TmLightBlock { - HostBlock::generate_tm_block(self.chain_id.clone(), h.revision_height, Timestamp::now()) + HostBlock::generate_tm_block(self.chain_id.clone(), h.revision_height(), Timestamp::now()) } } diff --git a/relayer/src/light_client/tendermint.rs b/relayer/src/light_client/tendermint.rs index 2e6ca90377..2e745d6f08 100644 --- a/relayer/src/light_client/tendermint.rs +++ b/relayer/src/light_client/tendermint.rs @@ -62,7 +62,7 @@ impl super::LightClient for LightClient { trace!(%trusted, %target, "light client verification"); let target_height = - TMHeight::try_from(target.revision_height).map_err(Error::invalid_height)?; + TMHeight::try_from(target.revision_height()).map_err(Error::invalid_height)?; let client = self.prepare_client(client_state)?; let mut state = self.prepare_state(trusted)?; @@ -89,7 +89,7 @@ impl super::LightClient for LightClient { fn fetch(&mut self, height: ibc::Height) -> Result { trace!(%height, "fetching header"); - let height = TMHeight::try_from(height.revision_height).map_err(Error::invalid_height)?; + let height = TMHeight::try_from(height.revision_height()).map_err(Error::invalid_height)?; self.fetch_light_block(AtHeight::At(height)) } @@ -122,7 +122,8 @@ impl super::LightClient for LightClient { let latest_chain_block = self.fetch_light_block(AtHeight::Highest)?; let latest_chain_height = - ibc::Height::new(self.chain_id.version(), latest_chain_block.height().into()); + ibc::Height::new(self.chain_id.version(), latest_chain_block.height().into()) + .map_err(|_| Error::invalid_height_no_source())?; // set the target height to the minimum between the update height and latest chain height let target_height = core::cmp::min(update.consensus_height(), latest_chain_height); @@ -211,7 +212,7 @@ impl LightClient { fn prepare_state(&self, trusted: ibc::Height) -> Result { let trusted_height = - TMHeight::try_from(trusted.revision_height).map_err(Error::invalid_height)?; + TMHeight::try_from(trusted.revision_height()).map_err(Error::invalid_height)?; let trusted_block = self.fetch_light_block(AtHeight::At(trusted_height))?; diff --git a/relayer/src/link/operational_data.rs b/relayer/src/link/operational_data.rs index f136f17d89..b2d94de0b6 100644 --- a/relayer/src/link/operational_data.rs +++ b/relayer/src/link/operational_data.rs @@ -360,8 +360,8 @@ impl ConnectionDelay { if latest_height >= acceptable_height { 0 } else { - debug_assert!(acceptable_height.revision_number == latest_height.revision_number); - acceptable_height.revision_height - latest_height.revision_height + debug_assert!(acceptable_height.revision_number() == latest_height.revision_number()); + acceptable_height.revision_height() - latest_height.revision_height() } } diff --git a/relayer/src/link/relay_path.rs b/relayer/src/link/relay_path.rs index a69c9e39ab..9bce02cf54 100644 --- a/relayer/src/link/relay_path.rs +++ b/relayer/src/link/relay_path.rs @@ -1738,7 +1738,7 @@ impl RelayPath { IbcEvent::SendPacket(send_packet_ev) => { ibc_telemetry::global().record_send_history( send_packet_ev.packet.sequence.into(), - send_packet_ev.height().revision_height, + send_packet_ev.height().revision_height(), &self.src_chain().id(), self.src_channel_id(), self.src_port_id(), @@ -1748,7 +1748,7 @@ impl RelayPath { IbcEvent::WriteAcknowledgement(write_ack_ev) => { ibc_telemetry::global().record_ack_history( write_ack_ev.packet.sequence.into(), - write_ack_ev.height().revision_height, + write_ack_ev.height().revision_height(), &self.dst_chain().id(), self.dst_channel_id(), self.dst_port_id(), @@ -1765,7 +1765,7 @@ impl RelayPath { IbcEvent::SendPacket(send_packet_ev) => { ibc_telemetry::global().send_packet_count( send_packet_ev.packet.sequence.into(), - send_packet_ev.height().revision_height, + send_packet_ev.height().revision_height(), &self.src_chain().id(), self.src_channel_id(), self.src_port_id(), @@ -1773,7 +1773,7 @@ impl RelayPath { ); ibc_telemetry::global().cleared_count( send_packet_ev.packet.sequence.into(), - send_packet_ev.height().revision_height, + send_packet_ev.height().revision_height(), &self.src_chain().id(), self.src_channel_id(), self.src_port_id(), @@ -1783,7 +1783,7 @@ impl RelayPath { IbcEvent::WriteAcknowledgement(write_ack_ev) => { ibc_telemetry::global().acknowledgement_count( write_ack_ev.packet.sequence.into(), - write_ack_ev.height().revision_height, + write_ack_ev.height().revision_height(), &self.dst_chain().id(), self.src_channel_id(), self.src_port_id(), diff --git a/relayer/src/supervisor.rs b/relayer/src/supervisor.rs index 679245f6b1..2ea8144678 100644 --- a/relayer/src/supervisor.rs +++ b/relayer/src/supervisor.rs @@ -656,7 +656,7 @@ fn process_batch( IbcEvent::SendPacket(send_packet_ev) => { ibc_telemetry::global().send_packet_count( send_packet_ev.packet.sequence.into(), - send_packet_ev.height().revision_height, + send_packet_ev.height().revision_height(), &src.id(), &_path.src_channel_id, &_path.src_port_id, @@ -666,7 +666,7 @@ fn process_batch( IbcEvent::WriteAcknowledgement(write_ack_ev) => { ibc_telemetry::global().acknowledgement_count( write_ack_ev.packet.sequence.into(), - write_ack_ev.height().revision_height, + write_ack_ev.height().revision_height(), &dst.id(), &_path.src_channel_id, &_path.src_port_id, diff --git a/relayer/src/transfer.rs b/relayer/src/transfer.rs index 041f2baa1e..89c7222c73 100644 --- a/relayer/src/transfer.rs +++ b/relayer/src/transfer.rs @@ -4,12 +4,12 @@ use flex_error::{define_error, DetailOnly}; use ibc::applications::transfer::error::Error as Ics20Error; use ibc::applications::transfer::msgs::transfer::MsgTransfer; use ibc::applications::transfer::Amount; +use ibc::core::ics04_channel::timeout::TimeoutHeight; use ibc::core::ics24_host::identifier::{ChainId, ChannelId, PortId}; use ibc::events::IbcEvent; use ibc::signer::Signer; use ibc::timestamp::{Timestamp, TimestampOverflowError}; use ibc::tx_msg::Msg; -use ibc::Height; use ibc_proto::cosmos::base::v1beta1::Coin; use ibc_proto::google::protobuf::Any; @@ -65,7 +65,7 @@ define_error! { #[derive(Copy, Clone)] pub struct TransferTimeout { - pub timeout_height: Option, + pub timeout_height: TimeoutHeight, pub timeout_timestamp: Timestamp, } @@ -86,9 +86,12 @@ impl TransferTimeout { destination_chain_status: &ChainStatus, ) -> Result { let timeout_height = if timeout_height_offset == 0 { - None + TimeoutHeight::no_timeout() } else { - Some(destination_chain_status.height.add(timeout_height_offset)) + destination_chain_status + .height + .add(timeout_height_offset) + .into() }; let timeout_timestamp = if timeout_duration == Duration::ZERO { @@ -124,7 +127,7 @@ pub fn build_transfer_message( denom: String, sender: Signer, receiver: Signer, - timeout_height: Option, + timeout_height: TimeoutHeight, timeout_timestamp: Timestamp, ) -> Any { let msg = MsgTransfer { diff --git a/relayer/src/upgrade_chain.rs b/relayer/src/upgrade_chain.rs index 6d0855c877..ee8db31819 100644 --- a/relayer/src/upgrade_chain.rs +++ b/relayer/src/upgrade_chain.rs @@ -106,7 +106,7 @@ pub fn build_and_send_ibc_upgrade_proposal( upgraded_client_state: Some(Any::from(upgraded_client_state.wrap_any())), plan: Some(Plan { name: opts.upgrade_plan_name.clone(), - height: upgrade_height.revision_height as i64, + height: upgrade_height.revision_height() as i64, info: "".to_string(), ..Default::default() // deprecated fields - time & upgraded_client_state }), diff --git a/relayer/src/worker/map.rs b/relayer/src/worker/map.rs index ad426f5b80..0347d1806a 100644 --- a/relayer/src/worker/map.rs +++ b/relayer/src/worker/map.rs @@ -58,7 +58,7 @@ impl WorkerMap { "waiting for worker loop to end" ); - let _ = handle.join(); + handle.join(); trace!( worker.id = %id, worker.object = %object.short_name(), @@ -109,7 +109,7 @@ impl WorkerMap { for worker in self.to_notify(src_chain_id) { // Ignore send error if the worker task handling // NewBlock cmd has been terminated. - let _ = worker.send_new_block(height, new_block); + worker.send_new_block(height, new_block); } } diff --git a/relayer/src/worker/packet.rs b/relayer/src/worker/packet.rs index ff4d5e4530..685741c935 100644 --- a/relayer/src/worker/packet.rs +++ b/relayer/src/worker/packet.rs @@ -158,7 +158,7 @@ fn handle_packet_cmd( /// If the specified height is reached, then packets are cleared if `clear_interval` /// is not `0` and if we have reached the interval. fn should_clear_packets(clear_interval: u64, height: Height) -> bool { - clear_interval != 0 && height.revision_height % clear_interval == 0 + clear_interval != 0 && height.revision_height() % clear_interval == 0 } fn handle_update_schedule( diff --git a/tools/test-framework/src/relayer/transfer.rs b/tools/test-framework/src/relayer/transfer.rs index 0eec47b973..5ab1bd0573 100644 --- a/tools/test-framework/src/relayer/transfer.rs +++ b/tools/test-framework/src/relayer/transfer.rs @@ -7,6 +7,7 @@ use core::ops::Add; use core::time::Duration; use ibc::applications::transfer::error::Error as Ics20Error; +use ibc::core::ics04_channel::timeout::TimeoutHeight; use ibc::timestamp::Timestamp; use ibc_proto::google::protobuf::Any; use ibc_relayer::chain::cosmos::types::config::TxConfig; @@ -52,7 +53,7 @@ pub fn build_transfer_message( denom.to_string(), sender, receiver, - None, + TimeoutHeight::no_timeout(), timeout_timestamp, )) }