From d20ac5b37378dfe980fe7db70646879132742de7 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Fri, 24 Jun 2022 09:29:40 -0400 Subject: [PATCH 01/43] From for String fix --- modules/src/core/ics02_client/height.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/src/core/ics02_client/height.rs b/modules/src/core/ics02_client/height.rs index e7feae2028..172d77d627 100644 --- a/modules/src/core/ics02_client/height.rs +++ b/modules/src/core/ics02_client/height.rs @@ -169,7 +169,7 @@ impl TryFrom<&str> for Height { impl From for String { fn from(height: Height) -> Self { - format!("{}-{}", height.revision_number, height.revision_number) + format!("{}-{}", height.revision_number, height.revision_height) } } From aae757ba54be27bbd844428191ca52e517c16b35 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Fri, 24 Jun 2022 14:16:45 -0400 Subject: [PATCH 02/43] Use Option instead of Height::zero for consensus_height --- .../core/ics03_connection/handler/conn_open_ack.rs | 7 +++++-- .../core/ics03_connection/handler/conn_open_try.rs | 7 +++++-- .../src/core/ics03_connection/msgs/conn_open_ack.rs | 7 ++----- .../src/core/ics03_connection/msgs/conn_open_try.rs | 11 ++++------- 4 files changed, 16 insertions(+), 16 deletions(-) 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..a714f92e6b 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)?; 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..f0b0965615 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 { 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..567e6f84e6 100644 --- a/modules/src/core/ics03_connection/msgs/conn_open_ack.rs +++ b/modules/src/core/ics03_connection/msgs/conn_open_ack.rs @@ -29,11 +29,8 @@ 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(), - } + pub fn consensus_height(&self) -> Option { + self.proofs.consensus_proof().map(|proof| proof.height()) } } 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..02cd934f95 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()) } } From 00957d2284f5499bc5e02f242e79b4699fb0df1a Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Fri, 24 Jun 2022 14:19:04 -0400 Subject: [PATCH 03/43] recv_packet still had Height::zero() in events --- modules/src/core/ics04_channel/handler/recv_packet.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/modules/src/core/ics04_channel/handler/recv_packet.rs b/modules/src/core/ics04_channel/handler/recv_packet.rs index 973c163bcd..09cb95b336 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 { @@ -89,7 +88,7 @@ pub fn process(ctx: &dyn ChannelReader, msg: &MsgRecvPacket) -> 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))); From 047727f3d6e4e6675ebf1a5e35faaabdba871649 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Mon, 27 Jun 2022 13:56:12 -0400 Subject: [PATCH 04/43] TryFrom for Height --- .../applications/transfer/msgs/transfer.rs | 4 ++++ .../clients/ics07_tendermint/client_state.rs | 17 ++++++++--------- modules/src/clients/ics07_tendermint/error.rs | 4 ++-- .../src/clients/ics07_tendermint/header.rs | 4 ++-- .../src/core/ics02_client/client_consensus.rs | 5 ++++- modules/src/core/ics02_client/error.rs | 3 +++ modules/src/core/ics02_client/height.rs | 19 ++++++++++++++----- .../ics03_connection/msgs/conn_open_ack.rs | 8 ++++---- .../msgs/conn_open_confirm.rs | 4 ++-- .../ics03_connection/msgs/conn_open_try.rs | 8 ++++---- .../ics04_channel/msgs/acknowledgement.rs | 4 ++-- .../ics04_channel/msgs/chan_close_confirm.rs | 4 ++-- .../core/ics04_channel/msgs/chan_open_ack.rs | 4 ++-- .../ics04_channel/msgs/chan_open_confirm.rs | 4 ++-- .../core/ics04_channel/msgs/chan_open_try.rs | 4 ++-- .../core/ics04_channel/msgs/recv_packet.rs | 4 ++-- .../src/core/ics04_channel/msgs/timeout.rs | 4 ++-- .../ics04_channel/msgs/timeout_on_close.rs | 4 ++-- modules/src/core/ics04_channel/packet.rs | 8 ++++++-- modules/src/mock/header.rs | 5 ++++- relayer/src/chain/cosmos.rs | 8 ++++---- 21 files changed, 77 insertions(+), 52 deletions(-) diff --git a/modules/src/applications/transfer/msgs/transfer.rs b/modules/src/applications/transfer/msgs/transfer.rs index bb52dbf414..7cd761be11 100644 --- a/modules/src/applications/transfer/msgs/transfer.rs +++ b/modules/src/applications/transfer/msgs/transfer.rs @@ -63,6 +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))?; + // FIXME: With "Height zero" representation gone, this conversion will fail (return Error) with + // `timeout_height == 0` + // Do not merge without this comment addressed. let timeout_height: Option = raw_msg .timeout_height .map(|raw_height| raw_height.try_into()) @@ -143,6 +146,7 @@ pub mod test_util { Height, }; + // FIXME (BEFORE MERGE): Add at least 1 test that uses `timeout_height: None` // Returns a dummy ICS20 `MsgTransfer`, for testing only! pub fn get_dummy_msg_transfer(height: u64) -> MsgTransfer { let address: Signer = get_dummy_bech32_account().as_str().parse().unwrap(); diff --git a/modules/src/clients/ics07_tendermint/client_state.rs b/modules/src/clients/ics07_tendermint/client_state.rs index 5205b13687..0598e18859 100644 --- a/modules/src/clients/ics07_tendermint/client_state.rs +++ b/modules/src/clients/ics07_tendermint/client_state.rs @@ -264,14 +264,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 +294,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 { diff --git a/modules/src/clients/ics07_tendermint/error.rs b/modules/src/clients/ics07_tendermint/error.rs index 0ef3b9f29c..23496e7ea3 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 } diff --git a/modules/src/clients/ics07_tendermint/header.rs b/modules/src/clients/ics07_tendermint/header.rs index 8fe32105a5..e7f7a669dd 100644 --- a/modules/src/clients/ics07_tendermint/header.rs +++ b/modules/src/clients/ics07_tendermint/header.rs @@ -104,8 +104,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)? 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/height.rs b/modules/src/core/ics02_client/height.rs index 172d77d627..7cdd8312cc 100644 --- a/modules/src/core/ics02_client/height.rs +++ b/modules/src/core/ics02_client/height.rs @@ -104,11 +104,17 @@ 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 { + if raw_height.revision_height == 0 { + Err(Error::invalid_height()) + } else { + Ok(Height::new( + raw_height.revision_number, + raw_height.revision_height, + )) } } } @@ -155,6 +161,9 @@ impl TryFrom<&str> for Height { type Error = HeightError; fn try_from(value: &str) -> Result { + // FIXME: REJECT `revision_height == 0` + // Note: we might have stored a height with `revision_height == 0` + // in the case of `Packet.timeout_height` (it's a valid height). let split: Vec<&str> = value.split('-').collect(); Ok(Height { revision_number: split[0] 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 567e6f84e6..19437f50fc 100644 --- a/modules/src/core/ics03_connection/msgs/conn_open_ack.rs +++ b/modules/src/core/ics03_connection/msgs/conn_open_ack.rs @@ -55,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() @@ -67,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 02cd934f95..ad315ae461 100644 --- a/modules/src/core/ics03_connection/msgs/conn_open_try.rs +++ b/modules/src/core/ics03_connection/msgs/conn_open_try.rs @@ -72,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 @@ -85,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/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..866c69abc8 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)?; 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..7e5d508774 100644 --- a/modules/src/core/ics04_channel/packet.rs +++ b/modules/src/core/ics04_channel/packet.rs @@ -195,10 +195,14 @@ impl TryFrom for Packet { if Sequence::from(raw_pkt.sequence).is_zero() { return Err(Error::zero_packet_sequence()); } + + // FIXME: Currently buggy, because `timeout_height.revision_height == 0` + // is the only place where it's a valid value. + // Introduce a new type `TimeoutHeight` to properly model this. let packet_timeout_height: Height = raw_pkt .timeout_height - .ok_or_else(Error::missing_height)? - .into(); + .and_then(|raw_height| raw_height.try_into().ok()) + .ok_or_else(Error::missing_height)?; if packet_timeout_height.is_zero() && raw_pkt.timeout_timestamp == 0 { return Err(Error::zero_packet_timeout()); diff --git a/modules/src/mock/header.rs b/modules/src/mock/header.rs index 381975ae48..c354405537 100644 --- a/modules/src/mock/header.rs +++ b/modules/src/mock/header.rs @@ -25,7 +25,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)?, diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index d1c6f3219a..eb83e8396d 100644 --- a/relayer/src/chain/cosmos.rs +++ b/relayer/src/chain/cosmos.rs @@ -1223,8 +1223,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 +1341,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)) } From 444f2c39762f9aa29c3c26a81aea2baa1c078c46 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Mon, 27 Jun 2022 16:35:15 -0400 Subject: [PATCH 05/43] pre height parse changes --- .../transfer/relay/send_transfer.rs | 3 +- modules/src/core/ics04_channel/context.rs | 31 ++++++--- modules/src/core/ics04_channel/events.rs | 2 +- .../core/ics04_channel/handler/recv_packet.rs | 11 ++-- .../core/ics04_channel/handler/send_packet.rs | 9 ++- .../src/core/ics04_channel/handler/timeout.rs | 12 ++-- modules/src/core/ics04_channel/packet.rs | 64 +++++++++++++++---- modules/src/core/ics26_routing/handler.rs | 3 +- 8 files changed, 91 insertions(+), 44 deletions(-) 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/core/ics04_channel/context.rs b/modules/src/core/ics04_channel/context.rs index bbd6d9090a..32416b4f7d 100644 --- a/modules/src/core/ics04_channel/context.rs +++ b/modules/src/core/ics04_channel/context.rs @@ -67,20 +67,33 @@ 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": + /// https://github.com/cosmos/ibc-go/blob/04791984b3d6c83f704c4f058e6ca0038d155d91/modules/core/04-channel/keeper/packet.go#L206 fn packet_commitment( &self, packet_data: Vec, - timeout_height: Height, + timeout_height: Option, 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, revision_height) = match timeout_height { + Some(height) => ( + height.revision_number.to_be_bytes(), + height.revision_height.to_be_bytes(), + ), + None => (0u64.to_be_bytes(), 0u64.to_be_bytes()), + }; + hash_input.append(&mut revision_number.to_vec()); + 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/events.rs b/modules/src/core/ics04_channel/events.rs index 35b4865ef6..42e475baa6 100644 --- a/modules/src/core/ics04_channel/events.rs +++ b/modules/src/core/ics04_channel/events.rs @@ -1214,7 +1214,7 @@ 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: Some(Height::new(1, 10)), timeout_timestamp: Timestamp::now(), }; let mut abci_events = vec![]; diff --git a/modules/src/core/ics04_channel/handler/recv_packet.rs b/modules/src/core/ics04_channel/handler/recv_packet.rs index 09cb95b336..353d99f629 100644 --- a/modules/src/core/ics04_channel/handler/recv_packet.rs +++ b/modules/src/core/ics04_channel/handler/recv_packet.rs @@ -60,11 +60,10 @@ pub fn process(ctx: &dyn ChannelReader, msg: &MsgRecvPacket) -> HandlerResult HandlerResult HandlerResult proof_height { - return Err(Error::packet_timeout_height_not_reached( - packet.timeout_height, - proof_height, - )); + if let Some(timeout_height) = packet.timeout_height { + if timeout_height > proof_height { + return Err(Error::packet_timeout_height_not_reached( + timeout_height, + proof_height, + )); + } } let consensus_state = ctx.client_consensus_state(&client_id, proof_height)?; diff --git a/modules/src/core/ics04_channel/packet.rs b/modules/src/core/ics04_channel/packet.rs index 7e5d508774..b4104f6dc3 100644 --- a/modules/src/core/ics04_channel/packet.rs +++ b/modules/src/core/ics04_channel/packet.rs @@ -5,6 +5,7 @@ use core::str::FromStr; use serde_derive::{Deserialize, Serialize}; 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::error::Error; use crate::core::ics24_host::identifier::{ChannelId, PortId}; @@ -106,7 +107,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: Option, pub timeout_timestamp: Timestamp, } @@ -165,15 +166,25 @@ 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 + .map_or(false, |timeout_height| timeout_height < 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 } } /// Custom debug output to omit the packet data impl core::fmt::Display for Packet { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> Result<(), core::fmt::Error> { + let timeout_height_display: String = match self.timeout_height { + Some(timeout_height) => format!("{}", timeout_height), + None => "None".into(), + }; + write!( f, "seq:{}, path:{}/{}->{}/{}, toh:{}, tos:{})", @@ -182,7 +193,7 @@ impl core::fmt::Display for Packet { self.source_port, self.destination_channel, self.destination_port, - self.timeout_height, + timeout_height_display, self.timeout_timestamp ) } @@ -196,15 +207,32 @@ impl TryFrom for Packet { return Err(Error::zero_packet_sequence()); } - // FIXME: Currently buggy, because `timeout_height.revision_height == 0` - // is the only place where it's a valid value. - // Introduce a new type `TimeoutHeight` to properly model this. - let packet_timeout_height: Height = raw_pkt + // `RawPacket.timeout_height` is treated differently 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". + // + // Note: it is important for `revision_number` to also be `0`, otherwise + // packet commitment proofs will be incorrect (see proof construction in + // `ChannelReader::packet_commitment()`). Note also that ibc-go conforms + // to this. + let packet_timeout_height: Option = raw_pkt .timeout_height - .and_then(|raw_height| raw_height.try_into().ok()) - .ok_or_else(Error::missing_height)?; - - if packet_timeout_height.is_zero() && raw_pkt.timeout_timestamp == 0 { + .and_then(|raw_height| { + if raw_height.revision_number == 0 && raw_height.revision_height == 0 { + None + } else { + Some(raw_height) + } + }) + .map(|raw_height| raw_height.try_into()) + .transpose() + .map_err(|_| Error::invalid_timeout_height())?; + + if packet_timeout_height.is_none() && raw_pkt.timeout_timestamp == 0 { return Err(Error::zero_packet_timeout()); } if raw_pkt.data.is_empty() { @@ -281,7 +309,15 @@ 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()), + // We map "no timeout height" to `Some(RawHeight::zero)` due to a quirk + // in ICS-4. See https://github.com/cosmos/ibc/issues/776. + timeout_height: match packet.timeout_height { + Some(height) => Some(height.into()), + None => Some(RawHeight { + revision_number: 0, + revision_height: 0, + }), + }, timeout_timestamp: packet.timeout_timestamp.nanoseconds(), } } diff --git a/modules/src/core/ics26_routing/handler.rs b/modules/src/core/ics26_routing/handler.rs index 659eb877be..b5d5362ec5 100644 --- a/modules/src/core/ics26_routing/handler.rs +++ b/modules/src/core/ics26_routing/handler.rs @@ -297,8 +297,7 @@ mod tests { 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(); From 0b19d5813bae397c7db2c14b0c9eb61e878a0281 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Mon, 27 Jun 2022 17:30:29 -0400 Subject: [PATCH 06/43] Packet.timeout_height is now an Option --- modules/src/core/ics02_client/height.rs | 26 ++++++++++-------- modules/src/core/ics04_channel/events.rs | 10 ++++--- .../src/core/ics04_channel/handler/timeout.rs | 1 - modules/src/core/ics04_channel/packet.rs | 27 ++++++++++++++----- modules/src/events.rs | 2 -- 5 files changed, 43 insertions(+), 23 deletions(-) diff --git a/modules/src/core/ics02_client/height.rs b/modules/src/core/ics02_client/height.rs index 7cdd8312cc..4aa0d12378 100644 --- a/modules/src/core/ics02_client/height.rs +++ b/modules/src/core/ics02_client/height.rs @@ -154,6 +154,8 @@ define_error! { format_args!("cannot convert into a `Height` type from string {0}", e.height) }, + ZeroHeight + |_| { "attempted to parse an invalid zero height" } } } @@ -161,18 +163,20 @@ impl TryFrom<&str> for Height { type Error = HeightError; fn try_from(value: &str) -> Result { - // FIXME: REJECT `revision_height == 0` - // Note: we might have stored a height with `revision_height == 0` - // in the case of `Packet.timeout_height` (it's a valid height). 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))?; + + if revision_height == 0 { + return Err(HeightError::zero_height()); + } + + Ok(Height::new(revision_number, revision_height)) } } diff --git a/modules/src/core/ics04_channel/events.rs b/modules/src/core/ics04_channel/events.rs index 42e475baa6..4e4ebaa02f 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"; @@ -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(); @@ -320,7 +321,10 @@ 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: match p.timeout_height { + Some(timeout_height) => timeout_height.to_string().parse().unwrap(), + None => "0-0".parse().unwrap(), + }, }; attributes.push(timeout_height); let timeout_timestamp = Tag { diff --git a/modules/src/core/ics04_channel/handler/timeout.rs b/modules/src/core/ics04_channel/handler/timeout.rs index 20a24540b4..9f00154b5e 100644 --- a/modules/src/core/ics04_channel/handler/timeout.rs +++ b/modules/src/core/ics04_channel/handler/timeout.rs @@ -51,7 +51,6 @@ pub fn process(ctx: &dyn ChannelReader, msg: &MsgTimeout) -> HandlerResult proof_height { diff --git a/modules/src/core/ics04_channel/packet.rs b/modules/src/core/ics04_channel/packet.rs index b4104f6dc3..172cb24e94 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; @@ -199,6 +200,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, Error> { + match s.parse::() { + Ok(height) => Ok(Some(height)), + Err(e) => match e.into_detail() { + HeightErrorDetail::ZeroHeight(_) => Ok(None), + _ => Err(Error::invalid_timeout_height()), + }, + } +} + impl TryFrom for Packet { type Error = Error; @@ -264,6 +279,7 @@ impl TryFrom for Packet { impl TryFrom> for Packet { type Error = EventError; fn try_from(obj: RawObject<'_>) -> Result { + // FIXME: Use ABCI constants instead of these hardcoded strings Ok(Packet { sequence: extract_attribute(&obj, &format!("{}.packet_sequence", obj.action))? .parse() @@ -284,12 +300,11 @@ impl TryFrom> for Packet { .parse() .map_err(EventError::parse)?, data: vec![], - timeout_height: extract_attribute( - &obj, - &format!("{}.packet_timeout_height", obj.action), - )? - .parse() - .map_err(EventError::height)?, + timeout_height: { + let timeout_height_str = + extract_attribute(&obj, &format!("{}.packet_timeout_height", obj.action))?; + parse_timeout_height(&timeout_height_str).map_err(|_| EventError::height())? + }, timeout_timestamp: extract_attribute( &obj, &format!("{}.packet_timeout_timestamp", obj.action), 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 From bf6192d10ebdf529cc545d08aec97dd6d463c8e6 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 28 Jun 2022 08:25:01 -0400 Subject: [PATCH 07/43] rustdoc --- modules/src/core/ics04_channel/context.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/src/core/ics04_channel/context.rs b/modules/src/core/ics04_channel/context.rs index 32416b4f7d..4dd0f27681 100644 --- a/modules/src/core/ics04_channel/context.rs +++ b/modules/src/core/ics04_channel/context.rs @@ -71,7 +71,7 @@ pub trait ChannelReader { /// 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": - /// https://github.com/cosmos/ibc-go/blob/04791984b3d6c83f704c4f058e6ca0038d155d91/modules/core/04-channel/keeper/packet.go#L206 + /// fn packet_commitment( &self, packet_data: Vec, From 2e47253a7c75c8c923f4976820453f2fcaa7e2e2 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 28 Jun 2022 08:29:42 -0400 Subject: [PATCH 08/43] Remove Height::with_revision_height --- modules/src/clients/ics07_tendermint/client_state.rs | 8 ++++---- modules/src/core/ics02_client/height.rs | 7 ------- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/modules/src/clients/ics07_tendermint/client_state.rs b/modules/src/clients/ics07_tendermint/client_state.rs index 0598e18859..832925458f 100644 --- a/modules/src/clients/ics07_tendermint/client_state.rs +++ b/modules/src/clients/ics07_tendermint/client_state.rs @@ -118,11 +118,11 @@ impl ClientState { } 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)), + latest_height: Height::new( + self.latest_height.revision_number, + h.signed_header.header.height.into(), + ), ..self } } diff --git a/modules/src/core/ics02_client/height.rs b/modules/src/core/ics02_client/height.rs index 4aa0d12378..5af661f3fe 100644 --- a/modules/src/core/ics02_client/height.rs +++ b/modules/src/core/ics02_client/height.rs @@ -65,13 +65,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 { From 56378b5da00021562a3fa908b1d0625e0426977d Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 28 Jun 2022 13:39:21 -0400 Subject: [PATCH 09/43] commit pre-modifying cli --- .../clients/ics07_tendermint/client_def.rs | 4 +- .../clients/ics07_tendermint/client_state.rs | 38 ++++++++++--------- modules/src/clients/ics07_tendermint/error.rs | 4 +- .../src/clients/ics07_tendermint/header.rs | 3 +- modules/src/core/ics02_client/events.rs | 2 +- .../ics02_client/handler/create_client.rs | 6 +-- .../ics02_client/handler/update_client.rs | 36 +++++++++--------- .../ics02_client/handler/upgrade_client.rs | 23 ++++++----- modules/src/core/ics02_client/height.rs | 25 +++++------- .../core/ics02_client/msgs/upgrade_client.rs | 2 +- modules/src/core/ics03_connection/events.rs | 2 +- .../handler/conn_open_confirm.rs | 4 +- .../handler/conn_open_init.rs | 7 ++-- .../ics03_connection/handler/conn_open_try.rs | 8 ++-- modules/src/core/ics04_channel/events.rs | 2 +- .../ics04_channel/handler/acknowledgement.rs | 2 +- .../ics04_channel/handler/chan_open_ack.rs | 8 ++-- .../handler/chan_open_confirm.rs | 5 ++- .../ics04_channel/handler/chan_open_try.rs | 4 +- .../core/ics04_channel/handler/send_packet.rs | 2 +- .../src/core/ics04_channel/handler/timeout.rs | 4 +- .../ics04_channel/handler/timeout_on_close.rs | 2 +- .../handler/write_acknowledgement.rs | 2 +- modules/src/core/ics26_routing/handler.rs | 12 +++--- modules/src/mock/context.rs | 26 ++++++------- modules/src/mock/header.rs | 2 +- modules/src/mock/host.rs | 5 ++- modules/src/relayer/ics18_relayer/utils.rs | 8 ++-- modules/tests/runner/mod.rs | 2 +- relayer-cli/src/commands/query/channel.rs | 5 ++- .../src/commands/query/channel_ends.rs | 6 ++- relayer/src/chain/cosmos.rs | 15 ++++++-- relayer/src/chain/cosmos/query/tx.rs | 29 +++++++------- relayer/src/chain/mock.rs | 2 +- relayer/src/event/rpc.rs | 6 ++- relayer/src/light_client/tendermint.rs | 3 +- 36 files changed, 170 insertions(+), 146 deletions(-) diff --git a/modules/src/clients/ics07_tendermint/client_def.rs b/modules/src/clients/ics07_tendermint/client_def.rs index 9f81f1c2d7..1f5963728b 100644 --- a/modules/src/clients/ics07_tendermint/client_def.rs +++ b/modules/src/clients/ics07_tendermint/client_def.rs @@ -92,7 +92,7 @@ impl ClientDef for TendermintClient { .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), )) } diff --git a/modules/src/clients/ics07_tendermint/client_state.rs b/modules/src/clients/ics07_tendermint/client_state.rs index 832925458f..c32c0ab580 100644 --- a/modules/src/clients/ics07_tendermint/client_state.rs +++ b/modules/src/clients/ics07_tendermint/client_state.rs @@ -117,14 +117,15 @@ impl ClientState { self.latest_height } - pub fn with_header(self, h: Header) -> Self { - ClientState { + 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 { @@ -378,7 +379,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 { @@ -501,9 +502,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, }, @@ -513,9 +514,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, }, @@ -525,9 +526,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, }, @@ -565,7 +566,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 { @@ -584,21 +585,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, }, @@ -659,7 +662,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 23496e7ea3..d6d1c3d24b 100644 --- a/modules/src/clients/ics07_tendermint/error.rs +++ b/modules/src/clients/ics07_tendermint/error.rs @@ -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 e7f7a669dd..db4898c3d2 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 { @@ -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/events.rs b/modules/src/core/ics02_client/events.rs index f858a7549f..2949eba523 100644 --- a/modules/src/core/ics02_client/events.rs +++ b/modules/src/core/ics02_client/events.rs @@ -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..04343acaa2 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,7 +141,7 @@ 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 = Height::new(0, 80).unwrap(); let ctx = MockContext::default().with_client(&existing_client_id, height); @@ -243,7 +243,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..6d50f190da 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, @@ -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 5af661f3fe..8d355863e9 100644 --- a/modules/src/core/ics02_client/height.rs +++ b/modules/src/core/ics02_client/height.rs @@ -22,11 +22,15 @@ pub struct Height { } 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 { @@ -101,14 +105,7 @@ impl TryFrom for Height { type Error = Error; fn try_from(raw_height: RawHeight) -> Result { - if raw_height.revision_height == 0 { - Err(Error::invalid_height()) - } else { - Ok(Height::new( - raw_height.revision_number, - raw_height.revision_height, - )) - } + Height::new(raw_height.revision_number, raw_height.revision_height) } } @@ -165,11 +162,7 @@ impl TryFrom<&str> for Height { .parse::() .map_err(|e| HeightError::height_conversion(value.to_owned(), e))?; - if revision_height == 0 { - return Err(HeightError::zero_height()); - } - - Ok(Height::new(revision_number, revision_height)) + Height::new(revision_number, revision_height).map_err(|_| HeightError::zero_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..f56a4d65a0 100644 --- a/modules/src/core/ics03_connection/events.rs +++ b/modules/src/core/ics03_connection/events.rs @@ -290,7 +290,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_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 f0b0965615..e4d2b64d2e 100644 --- a/modules/src/core/ics03_connection/handler/conn_open_try.rs +++ b/modules/src/core/ics03_connection/handler/conn_open_try.rs @@ -150,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), @@ -212,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/ics04_channel/events.rs b/modules/src/core/ics04_channel/events.rs index 4e4ebaa02f..fae02cb400 100644 --- a/modules/src/core/ics04_channel/events.rs +++ b/modules/src/core/ics04_channel/events.rs @@ -1218,7 +1218,7 @@ mod tests { destination_port: "b_test_port".parse().unwrap(), destination_channel: "channel-1".parse().unwrap(), data: "test_data".as_bytes().to_vec(), - timeout_height: Some(Height::new(1, 10)), + timeout_height: Some(Height::new(1, 10).unwrap()), timeout_timestamp: Timestamp::now(), }; let mut abci_events = vec![]; diff --git a/modules/src/core/ics04_channel/handler/acknowledgement.rs b/modules/src/core/ics04_channel/handler/acknowledgement.rs index 5246953b8d..60f62d6049 100644 --- a/modules/src/core/ics04_channel/handler/acknowledgement.rs +++ b/modules/src/core/ics04_channel/handler/acknowledgement.rs @@ -149,7 +149,7 @@ 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, 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..14cceb6cf3 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(); @@ -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..e43e971376 100644 --- a/modules/src/core/ics04_channel/handler/chan_open_confirm.rs +++ b/modules/src/core/ics04_channel/handler/chan_open_confirm.rs @@ -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/send_packet.rs b/modules/src/core/ics04_channel/handler/send_packet.rs index 6fd90a9e98..d08517ab7e 100644 --- a/modules/src/core/ics04_channel/handler/send_packet.rs +++ b/modules/src/core/ics04_channel/handler/send_packet.rs @@ -168,7 +168,7 @@ mod tests { packet_old.sequence = 1.into(); packet_old.data = vec![0]; - let client_height = Height::new(0, Height::default().revision_height + 1); + let client_height = Height::new(0, 1).unwrap(); let tests: Vec = vec![ Test { diff --git a/modules/src/core/ics04_channel/handler/timeout.rs b/modules/src/core/ics04_channel/handler/timeout.rs index 9f00154b5e..09267dd9a4 100644 --- a/modules/src/core/ics04_channel/handler/timeout.rs +++ b/modules/src/core/ics04_channel/handler/timeout.rs @@ -171,10 +171,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 = MsgTimeout::try_from(get_dummy_raw_msg_timeout(height, timeout_timestamp)).unwrap(); 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..8a26a32cdc 100644 --- a/modules/src/core/ics04_channel/handler/timeout_on_close.rs +++ b/modules/src/core/ics04_channel/handler/timeout_on_close.rs @@ -166,7 +166,7 @@ mod tests { let height = Height::default().revision_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..26528e2341 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(); diff --git a/modules/src/core/ics26_routing/handler.rs b/modules/src/core/ics26_routing/handler.rs index b5d5362ec5..ffdb0bac6d 100644 --- a/modules/src/core/ics26_routing/handler.rs +++ b/modules/src/core/ics26_routing/handler.rs @@ -212,15 +212,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(); diff --git a/modules/src/mock/context.rs b/modules/src/mock/context.rs index 07ed127984..8f9c1d5b9e 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(), ) } } @@ -1345,7 +1345,7 @@ mod tests { ChainId::new("mockgaia".to_string(), cv), HostType::Mock, 2, - Height::new(cv, 1), + Height::new(cv, 1).unwrap(), ), }, Test { @@ -1354,7 +1354,7 @@ mod tests { ChainId::new("mocksgaia".to_string(), cv), HostType::SyntheticTendermint, 2, - Height::new(cv, 1), + Height::new(cv, 1).unwrap(), ), }, Test { @@ -1363,7 +1363,7 @@ mod tests { ChainId::new("mockgaia".to_string(), cv), HostType::Mock, 30, - Height::new(cv, 2), + Height::new(cv, 2).unwrap(), ), }, Test { @@ -1372,7 +1372,7 @@ mod tests { ChainId::new("mocksgaia".to_string(), cv), HostType::SyntheticTendermint, 30, - Height::new(cv, 2), + Height::new(cv, 2).unwrap(), ), }, Test { @@ -1381,7 +1381,7 @@ mod tests { ChainId::new("mockgaia".to_string(), cv), HostType::Mock, 3, - Height::new(cv, 30), + Height::new(cv, 30).unwrap(), ), }, Test { @@ -1390,7 +1390,7 @@ mod tests { ChainId::new("mockgaia".to_string(), cv), HostType::SyntheticTendermint, 3, - Height::new(cv, 30), + Height::new(cv, 30).unwrap(), ), }, Test { @@ -1399,7 +1399,7 @@ mod tests { ChainId::new("mockgaia".to_string(), cv), HostType::Mock, 3, - Height::new(cv, 2), + Height::new(cv, 2).unwrap(), ), }, Test { @@ -1408,7 +1408,7 @@ mod tests { ChainId::new("mockgaia".to_string(), cv), HostType::SyntheticTendermint, 3, - Height::new(cv, 2), + Height::new(cv, 2).unwrap(), ), }, Test { @@ -1417,7 +1417,7 @@ mod tests { ChainId::new("mockgaia".to_string(), cv), HostType::Mock, 50, - Height::new(cv, 2000), + Height::new(cv, 2000).unwrap(), ), }, Test { @@ -1426,7 +1426,7 @@ mod tests { ChainId::new("mockgaia".to_string(), cv), HostType::SyntheticTendermint, 50, - Height::new(cv, 2000), + Height::new(cv, 2000).unwrap(), ), }, ]; @@ -1458,7 +1458,7 @@ mod tests { "failed while increasing height for context {:?}", test.ctx ); - if current_height > Height::new(cv, 0) { + if current_height > Height::new(cv, 0).unwrap() { assert_eq!( test.ctx.host_block(current_height).unwrap().height(), current_height, @@ -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 c354405537..5ede69ad62 100644 --- a/modules/src/mock/header.rs +++ b/modules/src/mock/header.rs @@ -98,7 +98,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..ff76fd6fe0 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( 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/tests/runner/mod.rs b/modules/tests/runner/mod.rs index 061b1ef81f..892f5e8bbc 100644 --- a/modules/tests/runner/mod.rs +++ b/modules/tests/runner/mod.rs @@ -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/query/channel.rs b/relayer-cli/src/commands/query/channel.rs index 44e748d0f9..744037fe1d 100644 --- a/relayer-cli/src/commands/query/channel.rs +++ b/relayer-cli/src/commands/query/channel.rs @@ -40,7 +40,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 ecde0dc54c..8f3838e3e1 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)] @@ -77,7 +77,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/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index eb83e8396d..a659dc75f8 100644 --- a/relayer/src/chain/cosmos.rs +++ b/relayer/src/chain/cosmos.rs @@ -1480,7 +1480,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 { @@ -1748,15 +1749,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/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 1d930d86bb..9f87408b04 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/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/tendermint.rs b/relayer/src/light_client/tendermint.rs index 2e6ca90377..1ed8b301cf 100644 --- a/relayer/src/light_client/tendermint.rs +++ b/relayer/src/light_client/tendermint.rs @@ -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); From 036da49940d570f9153db82c6240e6ef888aae0f Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 28 Jun 2022 13:55:16 -0400 Subject: [PATCH 10/43] Finish Height::new() returns Result --- relayer-cli/src/commands/query/client.rs | 28 +++++++++++-------- relayer-cli/src/commands/query/connection.rs | 7 +++-- relayer-cli/src/commands/query/packet/ack.rs | 12 ++++---- .../src/commands/query/packet/commitment.rs | 12 ++++---- relayer-cli/src/commands/tx/client.rs | 16 +++++++---- 5 files changed, 44 insertions(+), 31 deletions(-) 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); From 362dab327c4d45f7f4a6dc23e97a964eb26bced0 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 28 Jun 2022 13:57:50 -0400 Subject: [PATCH 11/43] remove Height::is_zero() --- modules/src/core/ics02_client/height.rs | 4 ---- modules/src/proofs.rs | 8 -------- 2 files changed, 12 deletions(-) diff --git a/modules/src/core/ics02_client/height.rs b/modules/src/core/ics02_client/height.rs index 8d355863e9..ebb1539e2a 100644 --- a/modules/src/core/ics02_client/height.rs +++ b/modules/src/core/ics02_client/height.rs @@ -40,10 +40,6 @@ impl Height { } } - pub fn is_zero(&self) -> bool { - self.revision_height == 0 - } - pub fn add(&self, delta: u64) -> Height { Height { revision_number: self.revision_number, 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, From d1f9e452df75b6a8cb53fcacf729b3ac1fb1fb51 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 28 Jun 2022 14:09:35 -0400 Subject: [PATCH 12/43] Remove `Height::zero()` --- .../clients/ics07_tendermint/client_state.rs | 30 +++++-------------- modules/src/core/ics02_client/height.rs | 9 +----- modules/src/mock/context.rs | 2 +- modules/src/test_utils.rs | 2 +- 4 files changed, 11 insertions(+), 32 deletions(-) diff --git a/modules/src/clients/ics07_tendermint/client_state.rs b/modules/src/clients/ics07_tendermint/client_state.rs index c32c0ab580..5d79b6d780 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 { @@ -129,11 +123,6 @@ impl ClientState { } 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 @@ -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, } } } @@ -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 { diff --git a/modules/src/core/ics02_client/height.rs b/modules/src/core/ics02_client/height.rs index ebb1539e2a..3e480811df 100644 --- a/modules/src/core/ics02_client/height.rs +++ b/modules/src/core/ics02_client/height.rs @@ -33,13 +33,6 @@ impl Height { }) } - pub fn zero() -> Height { - Self { - revision_number: 0, - revision_height: 0, - } - } - pub fn add(&self, delta: u64) -> Height { Height { revision_number: self.revision_number, @@ -69,7 +62,7 @@ impl Height { impl Default for Height { fn default() -> Self { - Self::zero() + Height::new(0, 1).unwrap() } } diff --git a/modules/src/mock/context.rs b/modules/src/mock/context.rs index 8f9c1d5b9e..2e1cf029d1 100644 --- a/modules/src/mock/context.rs +++ b/modules/src/mock/context.rs @@ -1170,7 +1170,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 { 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 { From 52941d496a8eb06c43abcb45d3e5d25e43dd875d Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 28 Jun 2022 17:05:36 -0400 Subject: [PATCH 13/43] Remove Height::default() --- modules/src/core/ics02_client/events.rs | 4 +-- .../ics02_client/handler/update_client.rs | 2 +- modules/src/core/ics02_client/height.rs | 6 ---- modules/src/core/ics03_connection/events.rs | 14 +++++++- modules/src/core/ics04_channel/events.rs | 33 +++++++++++++------ .../core/ics04_channel/handler/send_packet.rs | 2 +- .../ics04_channel/handler/timeout_on_close.rs | 2 +- .../handler/write_acknowledgement.rs | 2 +- modules/src/mock/header.rs | 11 ++++++- modules/src/mock/host.rs | 2 +- 10 files changed, 53 insertions(+), 25 deletions(-) diff --git a/modules/src/core/ics02_client/events.rs b/modules/src/core/ics02_client/events.rs index 2949eba523..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(), } } } diff --git a/modules/src/core/ics02_client/handler/update_client.rs b/modules/src/core/ics02_client/handler/update_client.rs index 6d50f190da..b6695eb190 100644 --- a/modules/src/core/ics02_client/handler/update_client.rs +++ b/modules/src/core/ics02_client/handler/update_client.rs @@ -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) => { diff --git a/modules/src/core/ics02_client/height.rs b/modules/src/core/ics02_client/height.rs index 3e480811df..99b983ebf8 100644 --- a/modules/src/core/ics02_client/height.rs +++ b/modules/src/core/ics02_client/height.rs @@ -60,12 +60,6 @@ impl Height { } } -impl Default for Height { - fn default() -> Self { - Height::new(0, 1).unwrap() - } -} - impl PartialOrd for Height { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) diff --git a/modules/src/core/ics03_connection/events.rs b/modules/src/core/ics03_connection/events.rs index f56a4d65a0..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 diff --git a/modules/src/core/ics04_channel/events.rs b/modules/src/core/ics04_channel/events.rs index fae02cb400..5b5014fd43 100644 --- a/modules/src/core/ics04_channel/events.rs +++ b/modules/src/core/ics04_channel/events.rs @@ -73,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, }) }) @@ -82,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, }) @@ -94,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, }) }) @@ -106,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, }) }) @@ -214,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 @@ -1160,7 +1173,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(), @@ -1223,23 +1236,23 @@ mod tests { }; 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/send_packet.rs b/modules/src/core/ics04_channel/handler/send_packet.rs index d08517ab7e..f1b602288d 100644 --- a/modules/src/core/ics04_channel/handler/send_packet.rs +++ b/modules/src/core/ics04_channel/handler/send_packet.rs @@ -181,7 +181,7 @@ mod tests { name: "Good parameters".to_string(), ctx: context .clone() - .with_client(&ClientId::default(), Height::default()) + .with_client(&ClientId::default(), Height::new(0, 1).unwrap()) .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()), 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 8a26a32cdc..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,7 +163,7 @@ 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, 2).unwrap(); diff --git a/modules/src/core/ics04_channel/handler/write_acknowledgement.rs b/modules/src/core/ics04_channel/handler/write_acknowledgement.rs index 26528e2341..61bbc2824d 100644 --- a/modules/src/core/ics04_channel/handler/write_acknowledgement.rs +++ b/modules/src/core/ics04_channel/handler/write_acknowledgement.rs @@ -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/mock/header.rs b/modules/src/mock/header.rs index 5ede69ad62..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 { diff --git a/modules/src/mock/host.rs b/modules/src/mock/host.rs index ff76fd6fe0..cc3a813f93 100644 --- a/modules/src/mock/host.rs +++ b/modules/src/mock/host.rs @@ -118,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, } } From 7507bfda5998a8bce264bad06529857275d474de Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 28 Jun 2022 17:12:19 -0400 Subject: [PATCH 14/43] Height fields accessors --- modules/src/core/ics02_client/height.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/modules/src/core/ics02_client/height.rs b/modules/src/core/ics02_client/height.rs index 99b983ebf8..4e44cf78a2 100644 --- a/modules/src/core/ics02_client/height.rs +++ b/modules/src/core/ics02_client/height.rs @@ -33,6 +33,14 @@ impl Height { }) } + pub fn revision_number(&self) -> u64 { + self.revision_number + } + + pub fn revision_height(&self) -> u64 { + self.revision_height + } + pub fn add(&self, delta: u64) -> Height { Height { revision_number: self.revision_number, From 2004958c28b630aa92ae9c346f3c30641ea40a23 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 28 Jun 2022 17:15:17 -0400 Subject: [PATCH 15/43] use revision_height accessor --- .../clients/ics07_tendermint/client_def.rs | 6 ++--- .../ics03_connection/handler/conn_open_try.rs | 8 +++--- modules/src/core/ics04_channel/context.rs | 2 +- .../ics04_channel/handler/acknowledgement.rs | 2 +- .../handler/chan_close_confirm.rs | 2 +- .../ics04_channel/handler/chan_open_ack.rs | 2 +- .../handler/chan_open_confirm.rs | 2 +- .../core/ics04_channel/handler/recv_packet.rs | 7 +++--- modules/src/mock/client_def.rs | 2 +- modules/src/mock/context.rs | 25 ++++++++++--------- modules/tests/runner/mod.rs | 2 +- relayer/src/chain/cosmos.rs | 12 ++++----- relayer/src/chain/cosmos/query.rs | 3 ++- relayer/src/chain/requests.rs | 4 +-- relayer/src/light_client/mock.rs | 2 +- relayer/src/light_client/tendermint.rs | 6 ++--- relayer/src/link/operational_data.rs | 2 +- relayer/src/upgrade_chain.rs | 2 +- relayer/src/worker/packet.rs | 2 +- 19 files changed, 48 insertions(+), 45 deletions(-) diff --git a/modules/src/clients/ics07_tendermint/client_def.rs b/modules/src/clients/ics07_tendermint/client_def.rs index 1f5963728b..8989886768 100644 --- a/modules/src/clients/ics07_tendermint/client_def.rs +++ b/modules/src/clients/ics07_tendermint/client_def.rs @@ -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.revision_height, + header.trusted_height.revision_height(), )) })?, next_validators: &header.trusted_validator_set, @@ -206,7 +206,7 @@ impl ClientDef for TendermintClient { let path = ClientConsensusStatePath { client_id: client_id.clone(), epoch: consensus_height.revision_number, - height: consensus_height.revision_height, + height: consensus_height.revision_height(), }; let value = expected_consensus_state .encode_vec() 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 e4d2b64d2e..b06a0e8505 100644 --- a/modules/src/core/ics03_connection/handler/conn_open_try.rs +++ b/modules/src/core/ics03_connection/handler/conn_open_try.rs @@ -162,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, @@ -187,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(); diff --git a/modules/src/core/ics04_channel/context.rs b/modules/src/core/ics04_channel/context.rs index 4dd0f27681..e435933fef 100644 --- a/modules/src/core/ics04_channel/context.rs +++ b/modules/src/core/ics04_channel/context.rs @@ -83,7 +83,7 @@ pub trait ChannelReader { let (revision_number, revision_height) = match timeout_height { Some(height) => ( height.revision_number.to_be_bytes(), - height.revision_height.to_be_bytes(), + height.revision_height().to_be_bytes(), ), None => (0u64.to_be_bytes(), 0u64.to_be_bytes()), }; diff --git a/modules/src/core/ics04_channel/handler/acknowledgement.rs b/modules/src/core/ics04_channel/handler/acknowledgement.rs index 60f62d6049..dccf8c947d 100644 --- a/modules/src/core/ics04_channel/handler/acknowledgement.rs +++ b/modules/src/core/ics04_channel/handler/acknowledgement.rs @@ -152,7 +152,7 @@ mod tests { 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 14cceb6cf3..e3fe9adc19 100644 --- a/modules/src/core/ics04_channel/handler/chan_open_ack.rs +++ b/modules/src/core/ics04_channel/handler/chan_open_ack.rs @@ -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(); 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 e43e971376..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( diff --git a/modules/src/core/ics04_channel/handler/recv_packet.rs b/modules/src/core/ics04_channel/handler/recv_packet.rs index 353d99f629..9e7bdd171d 100644 --- a/modules/src/core/ics04_channel/handler/recv_packet.rs +++ b/modules/src/core/ics04_channel/handler/recv_packet.rs @@ -180,9 +180,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(); diff --git a/modules/src/mock/client_def.rs b/modules/src/mock/client_def.rs index 427d7f3e29..f3d8aba922 100644 --- a/modules/src/mock/client_def.rs +++ b/modules/src/mock/client_def.rs @@ -63,7 +63,7 @@ impl ClientDef for MockClient { let client_prefixed_path = Path::ClientConsensusState(ClientConsensusStatePath { client_id: client_id.clone(), epoch: consensus_height.revision_number, - height: consensus_height.revision_height, + height: consensus_height.revision_height(), }) .to_string(); diff --git a/modules/src/mock/context.rs b/modules/src/mock/context.rs index 2e1cf029d1..b878454de2 100644 --- a/modules/src/mock/context.rs +++ b/modules/src/mock/context.rs @@ -129,12 +129,13 @@ 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(), @@ -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) @@ -373,12 +374,12 @@ impl MockContext { unimplemented!() } 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(), ); diff --git a/modules/tests/runner/mod.rs b/modules/tests/runner/mod.rs index 892f5e8bbc..0bf4763629 100644 --- a/modules/tests/runner/mod.rs +++ b/modules/tests/runner/mod.rs @@ -143,7 +143,7 @@ impl IbcTestRunner { } pub fn height(height: Height) -> Height { - Height::new(height.revision_number, height.revision_height).unwrap() + Height::new(height.revision_number, height.revision_height()).unwrap() } fn mock_header(height: Height) -> MockHeader { diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index a659dc75f8..ea0e494dcc 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, ))?; @@ -799,7 +799,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 +823,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, )?; @@ -877,7 +877,7 @@ impl ChainEndpoint for CosmosSdkChain { ClientConsensusStatePath { client_id: request.client_id.clone(), epoch: request.consensus_height.revision_number, - height: request.consensus_height.revision_height, + height: request.consensus_height.revision_height(), }, request.query_height, matches!(include_proof, IncludeProof::Yes), @@ -1524,7 +1524,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)? } }; diff --git a/relayer/src/chain/cosmos/query.rs b/relayer/src/chain/cosmos/query.rs index 087f5d963b..05e3d534c1 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/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/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 1ed8b301cf..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)) } @@ -212,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..34c624b22f 100644 --- a/relayer/src/link/operational_data.rs +++ b/relayer/src/link/operational_data.rs @@ -361,7 +361,7 @@ impl ConnectionDelay { 0 } else { debug_assert!(acceptable_height.revision_number == latest_height.revision_number); - acceptable_height.revision_height - latest_height.revision_height + acceptable_height.revision_height() - latest_height.revision_height() } } 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/packet.rs b/relayer/src/worker/packet.rs index 936ffadd6d..25f1073087 100644 --- a/relayer/src/worker/packet.rs +++ b/relayer/src/worker/packet.rs @@ -165,7 +165,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( From c2418c17b54fadee939a4719095014f207b5494e Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 28 Jun 2022 17:16:17 -0400 Subject: [PATCH 16/43] use revision_number accessor --- modules/src/clients/ics07_tendermint/client_def.rs | 6 +++--- modules/src/clients/ics07_tendermint/client_state.rs | 2 +- modules/src/clients/ics07_tendermint/header.rs | 6 +++--- modules/src/core/ics03_connection/handler/conn_open_ack.rs | 2 +- modules/src/core/ics04_channel/context.rs | 2 +- modules/src/mock/client_def.rs | 2 +- modules/src/mock/context.rs | 6 +++--- modules/tests/runner/mod.rs | 4 ++-- relayer/src/chain/cosmos.rs | 2 +- relayer/src/chain/cosmos/query.rs | 2 +- relayer/src/link/operational_data.rs | 2 +- 11 files changed, 18 insertions(+), 18 deletions(-) diff --git a/modules/src/clients/ics07_tendermint/client_def.rs b/modules/src/clients/ics07_tendermint/client_def.rs index 8989886768..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(), ), )); } @@ -205,7 +205,7 @@ impl ClientDef for TendermintClient { let path = ClientConsensusStatePath { client_id: client_id.clone(), - epoch: consensus_height.revision_number, + epoch: consensus_height.revision_number(), height: consensus_height.revision_height(), }; let value = expected_consensus_state diff --git a/modules/src/clients/ics07_tendermint/client_state.rs b/modules/src/clients/ics07_tendermint/client_state.rs index 5d79b6d780..76ca250e6a 100644 --- a/modules/src/clients/ics07_tendermint/client_state.rs +++ b/modules/src/clients/ics07_tendermint/client_state.rs @@ -114,7 +114,7 @@ impl ClientState { pub fn with_header(self, h: Header) -> Result { Ok(ClientState { latest_height: Height::new( - self.latest_height.revision_number, + self.latest_height.revision_number(), h.signed_header.header.height.into(), ) .map_err(|_| Error::invalid_header_height(h.signed_header.header.height.value()))?, diff --git a/modules/src/clients/ics07_tendermint/header.rs b/modules/src/clients/ics07_tendermint/header.rs index db4898c3d2..8aaf361a0a 100644 --- a/modules/src/clients/ics07_tendermint/header.rs +++ b/modules/src/clients/ics07_tendermint/header.rs @@ -114,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(), )); } 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 a714f92e6b..3ea5ad33bc 100644 --- a/modules/src/core/ics03_connection/handler/conn_open_ack.rs +++ b/modules/src/core/ics03_connection/handler/conn_open_ack.rs @@ -140,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/ics04_channel/context.rs b/modules/src/core/ics04_channel/context.rs index e435933fef..dd4579d849 100644 --- a/modules/src/core/ics04_channel/context.rs +++ b/modules/src/core/ics04_channel/context.rs @@ -82,7 +82,7 @@ pub trait ChannelReader { let (revision_number, revision_height) = match timeout_height { Some(height) => ( - height.revision_number.to_be_bytes(), + height.revision_number().to_be_bytes(), height.revision_height().to_be_bytes(), ), None => (0u64.to_be_bytes(), 0u64.to_be_bytes()), diff --git a/modules/src/mock/client_def.rs b/modules/src/mock/client_def.rs index f3d8aba922..2a0d27b201 100644 --- a/modules/src/mock/client_def.rs +++ b/modules/src/mock/client_def.rs @@ -62,7 +62,7 @@ impl ClientDef for MockClient { ) -> Result<(), Error> { let client_prefixed_path = Path::ClientConsensusState(ClientConsensusStatePath { client_id: client_id.clone(), - epoch: consensus_height.revision_number, + 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 b878454de2..d3287a4a46 100644 --- a/modules/src/mock/context.rs +++ b/modules/src/mock/context.rs @@ -139,7 +139,7 @@ impl MockContext { 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" ); @@ -370,9 +370,9 @@ 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() { panic!("Cannot rewind history of the chain to a smaller revision height!") diff --git a/modules/tests/runner/mod.rs b/modules/tests/runner/mod.rs index 0bf4763629..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()).unwrap() + Height::new(height.revision_number(), height.revision_height()).unwrap() } fn mock_header(height: Height) -> MockHeader { diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index ea0e494dcc..9fa5dcf0c0 100644 --- a/relayer/src/chain/cosmos.rs +++ b/relayer/src/chain/cosmos.rs @@ -876,7 +876,7 @@ impl ChainEndpoint for CosmosSdkChain { let res = self.query( ClientConsensusStatePath { client_id: request.client_id.clone(), - epoch: request.consensus_height.revision_number, + epoch: request.consensus_height.revision_number(), height: request.consensus_height.revision_height(), }, request.query_height, diff --git a/relayer/src/chain/cosmos/query.rs b/relayer/src/chain/cosmos/query.rs index 05e3d534c1..18052561a5 100644 --- a/relayer/src/chain/cosmos/query.rs +++ b/relayer/src/chain/cosmos/query.rs @@ -59,7 +59,7 @@ 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_number(), request.consensus_height.revision_height() ), ) diff --git a/relayer/src/link/operational_data.rs b/relayer/src/link/operational_data.rs index 34c624b22f..b2d94de0b6 100644 --- a/relayer/src/link/operational_data.rs +++ b/relayer/src/link/operational_data.rs @@ -360,7 +360,7 @@ impl ConnectionDelay { if latest_height >= acceptable_height { 0 } else { - debug_assert!(acceptable_height.revision_number == latest_height.revision_number); + debug_assert!(acceptable_height.revision_number() == latest_height.revision_number()); acceptable_height.revision_height() - latest_height.revision_height() } } From 97fd83295bbee082d7d98b7597ab627bfae28c73 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 28 Jun 2022 17:19:50 -0400 Subject: [PATCH 17/43] make Height fields private (WIP) --- modules/src/applications/transfer/msgs/transfer.rs | 5 +---- modules/src/core/ics02_client/handler/create_client.rs | 6 +----- modules/src/core/ics02_client/height.rs | 4 ++-- 3 files changed, 4 insertions(+), 11 deletions(-) diff --git a/modules/src/applications/transfer/msgs/transfer.rs b/modules/src/applications/transfer/msgs/transfer.rs index 7cd761be11..56b999b2eb 100644 --- a/modules/src/applications/transfer/msgs/transfer.rs +++ b/modules/src/applications/transfer/msgs/transfer.rs @@ -161,10 +161,7 @@ pub mod test_util { 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_height: Some(Height::new(0, height).unwrap()), } } } diff --git a/modules/src/core/ics02_client/handler/create_client.rs b/modules/src/core/ics02_client/handler/create_client.rs index 04343acaa2..d0f5041e30 100644 --- a/modules/src/core/ics02_client/handler/create_client.rs +++ b/modules/src/core/ics02_client/handler/create_client.rs @@ -147,11 +147,7 @@ mod tests { let create_client_msgs: Vec = vec![ MsgCreateAnyClient::new( - MockClientState::new(MockHeader::new(Height { - revision_height: 42, - ..height - })) - .into(), + MockClientState::new(MockHeader::new(Height::new(0, 42).unwrap())).into(), MockConsensusState::new(MockHeader::new(Height { revision_height: 42, ..height diff --git a/modules/src/core/ics02_client/height.rs b/modules/src/core/ics02_client/height.rs index 4e44cf78a2..dc84b3ffad 100644 --- a/modules/src/core/ics02_client/height.rs +++ b/modules/src/core/ics02_client/height.rs @@ -15,10 +15,10 @@ 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 { From fca5c9edb947f0e61e0ab9310f24525270a0255b Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 28 Jun 2022 17:26:05 -0400 Subject: [PATCH 18/43] compile error fixes --- .../ics02_client/handler/create_client.rs | 38 +++++-------------- relayer/src/chain/cosmos.rs | 9 +++-- relayer/src/chain/cosmos/query/status.rs | 9 +++-- 3 files changed, 20 insertions(+), 36 deletions(-) diff --git a/modules/src/core/ics02_client/handler/create_client.rs b/modules/src/core/ics02_client/handler/create_client.rs index d0f5041e30..b428591a39 100644 --- a/modules/src/core/ics02_client/handler/create_client.rs +++ b/modules/src/core/ics02_client/handler/create_client.rs @@ -141,46 +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).unwrap(); + 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::new(0, 42).unwrap())).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(), diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index 9fa5dcf0c0..9ec187bb88 100644 --- a/relayer/src/chain/cosmos.rs +++ b/relayer/src/chain/cosmos.rs @@ -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 }) 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, From 7730de80fb441349940ac91f57dbc6d0a0afdac2 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 28 Jun 2022 17:38:27 -0400 Subject: [PATCH 19/43] FIXME in transfer.rs addressed --- .../applications/transfer/msgs/transfer.rs | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/modules/src/applications/transfer/msgs/transfer.rs b/modules/src/applications/transfer/msgs/transfer.rs index 56b999b2eb..130f02b2bb 100644 --- a/modules/src/applications/transfer/msgs/transfer.rs +++ b/modules/src/applications/transfer/msgs/transfer.rs @@ -63,11 +63,27 @@ 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))?; - // FIXME: With "Height zero" representation gone, this conversion will fail (return Error) with - // `timeout_height == 0` - // Do not merge without this comment addressed. + // `RawPacket.timeout_height` is treated differently 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". + // + // Note: it is important for `revision_number` to also be `0`, otherwise + // packet commitment proofs will be incorrect (see proof construction in + // `ChannelReader::packet_commitment()`). Note also that ibc-go conforms + // to this. let timeout_height: Option = raw_msg .timeout_height + .and_then(|raw_height| { + if raw_height.revision_number == 0 && raw_height.revision_height == 0 { + None + } else { + Some(raw_height) + } + }) .map(|raw_height| raw_height.try_into()) .transpose() .map_err(|e| { From b10a70325057aa25086fd4bf38f9caf941fef5ae Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 28 Jun 2022 17:42:55 -0400 Subject: [PATCH 20/43] Use existing constants instead of hardcoded strings --- modules/src/core/ics04_channel/events.rs | 18 ++++---- modules/src/core/ics04_channel/packet.rs | 52 ++++++++++++++++-------- 2 files changed, 44 insertions(+), 26 deletions(-) diff --git a/modules/src/core/ics04_channel/events.rs b/modules/src/core/ics04_channel/events.rs index 5b5014fd43..e838ffcbce 100644 --- a/modules/src/core/ics04_channel/events.rs +++ b/modules/src/core/ics04_channel/events.rs @@ -25,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() { diff --git a/modules/src/core/ics04_channel/packet.rs b/modules/src/core/ics04_channel/packet.rs index 172cb24e94..c5df425944 100644 --- a/modules/src/core/ics04_channel/packet.rs +++ b/modules/src/core/ics04_channel/packet.rs @@ -14,6 +14,11 @@ 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, @@ -279,35 +284,48 @@ impl TryFrom for Packet { impl TryFrom> for Packet { type Error = EventError; fn try_from(obj: RawObject<'_>) -> Result { - // FIXME: Use ABCI constants instead of these hardcoded strings 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)?, + sequence: extract_attribute( + &obj, + &format!("{}.{}", obj.action, PKT_SEQ_ATTRIBUTE_KEY), + )? + .parse() + .map_err(EventError::channel)?, + source_port: extract_attribute( + &obj, + &format!("{}.{}", obj.action, PKT_SRC_PORT_ATTRIBUTE_KEY), + )? + .parse() + .map_err(EventError::parse)?, + source_channel: extract_attribute( + &obj, + &format!("{}.{}", obj.action, PKT_SRC_CHANNEL_ATTRIBUTE_KEY), + )? + .parse() + .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!("{}.packet_dst_channel", obj.action), + &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!("{}.packet_timeout_height", obj.action))?; + 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)?, From 1bc18b65d10ae91030913e40b04fc87fa710a3ed Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 28 Jun 2022 17:46:08 -0400 Subject: [PATCH 21/43] changelog --- .../unreleased/improvements/ibc/1009-height-zero-complete.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/ibc/1009-height-zero-complete.md 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 From 5660a999ac192a0fc29d1de1797161ac212782c3 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 29 Jun 2022 14:55:03 -0400 Subject: [PATCH 22/43] TimeoutHeight newtype --- modules/src/core/ics04_channel/mod.rs | 1 + modules/src/core/ics04_channel/packet.rs | 34 ++---- modules/src/core/ics04_channel/timeout.rs | 124 ++++++++++++++++++++++ 3 files changed, 135 insertions(+), 24 deletions(-) create mode 100644 modules/src/core/ics04_channel/timeout.rs 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/packet.rs b/modules/src/core/ics04_channel/packet.rs index c5df425944..458ff13c03 100644 --- a/modules/src/core/ics04_channel/packet.rs +++ b/modules/src/core/ics04_channel/packet.rs @@ -23,6 +23,7 @@ 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)] @@ -113,7 +114,7 @@ pub struct Packet { pub destination_channel: ChannelId, #[serde(serialize_with = "crate::serializers::ser_hex_upper")] pub data: Vec, - pub timeout_height: Option, + pub timeout_height: TimeoutHeight, pub timeout_timestamp: Timestamp, } @@ -172,9 +173,7 @@ 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 { - let height_timed_out = self - .timeout_height - .map_or(false, |timeout_height| timeout_height < dst_chain_height); + 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; @@ -186,11 +185,6 @@ impl Packet { /// Custom debug output to omit the packet data impl core::fmt::Display for Packet { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> Result<(), core::fmt::Error> { - let timeout_height_display: String = match self.timeout_height { - Some(timeout_height) => format!("{}", timeout_height), - None => "None".into(), - }; - write!( f, "seq:{}, path:{}/{}->{}/{}, toh:{}, tos:{})", @@ -199,7 +193,7 @@ impl core::fmt::Display for Packet { self.source_port, self.destination_channel, self.destination_port, - timeout_height_display, + self.timeout_height, self.timeout_timestamp ) } @@ -209,11 +203,11 @@ impl core::fmt::Display for Packet { /// `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, Error> { +pub fn parse_timeout_height(s: &str) -> Result { match s.parse::() { - Ok(height) => Ok(Some(height)), + Ok(height) => Ok(TimeoutHeight::from(height)), Err(e) => match e.into_detail() { - HeightErrorDetail::ZeroHeight(_) => Ok(None), + HeightErrorDetail::ZeroHeight(_) => Ok(TimeoutHeight::no_timeout()), _ => Err(Error::invalid_timeout_height()), }, } @@ -239,20 +233,12 @@ impl TryFrom for Packet { // packet commitment proofs will be incorrect (see proof construction in // `ChannelReader::packet_commitment()`). Note also that ibc-go conforms // to this. - let packet_timeout_height: Option = raw_pkt + let packet_timeout_height: TimeoutHeight = raw_pkt .timeout_height - .and_then(|raw_height| { - if raw_height.revision_number == 0 && raw_height.revision_height == 0 { - None - } else { - Some(raw_height) - } - }) - .map(|raw_height| raw_height.try_into()) - .transpose() + .try_into() .map_err(|_| Error::invalid_timeout_height())?; - if packet_timeout_height.is_none() && raw_pkt.timeout_timestamp == 0 { + if packet_timeout_height.has_timeout() == false && raw_pkt.timeout_timestamp == 0 { return Err(Error::zero_packet_timeout()); } if raw_pkt.data.is_empty() { diff --git a/modules/src/core/ics04_channel/timeout.rs b/modules/src/core/ics04_channel/timeout.rs new file mode 100644 index 0000000000..d72bddffd2 --- /dev/null +++ b/modules/src/core/ics04_channel/timeout.rs @@ -0,0 +1,124 @@ +use crate::prelude::*; + +use core::fmt::Display; + +use crate::{core::ics02_client::error::Error as ICS2Error, Height}; + +use ibc_proto::ibc::core::client::v1::Height as RawHeight; +use serde::{Deserialize, Serialize}; +use tendermint::abci::tag::Value as TagValue; + +/// Represents a height to be used to timeout packets. +/// +/// `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, Default, Hash, Eq, PartialEq, Serialize, Deserialize)] +pub struct TimeoutHeight(Option); + +impl TimeoutHeight { + pub fn no_timeout() -> Self { + Self(None) + } + + /// Revision number to be used in packet commitment computation + pub fn commitment_revision_number(&self) -> u64 { + match self.0 { + Some(height) => height.revision_number(), + None => 0, + } + } + + /// Revision height to be used in packet commitment computation + pub fn commitment_revision_height(&self) -> u64 { + match self.0 { + Some(height) => height.revision_height(), + None => 0, + } + } + + /// Check if a height is past the timeout height, and thus is expired. + pub fn has_expired(&self, height: &Height) -> bool { + match self.0 { + Some(timeout_height) => timeout_height >= *height, + // When there's no timeout, heights are never expired + None => false, + } + } + + // FIXME: Find better name + pub fn has_timeout(&self) -> bool { + self.0.is_some() + } +} + +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(None)) + } else { + let height: Height = raw_height.try_into()?; + Ok(TimeoutHeight(Some(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(None)), + } + } +} +/// We map "no timeout height" to `Some(RawHeight::zero)` due to a quirk +/// in ICS-4. See https://github.com/cosmos/ibc/issues/776. +impl From for Option { + fn from(timeout_height: TimeoutHeight) -> Self { + let raw_height = match timeout_height.0 { + Some(height) => height.into(), + None => RawHeight { + revision_number: 0, + revision_height: 0, + }, + }; + + Some(raw_height) + } +} + +impl From for TimeoutHeight { + fn from(height: Height) -> Self { + Self(Some(height)) + } +} + +impl From for TagValue { + fn from(timeout_height: TimeoutHeight) -> Self { + match timeout_height.0 { + Some(height) => height.to_string().parse().unwrap(), + None => "0-0".parse().unwrap(), + } + } +} + +impl Display for TimeoutHeight { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + match self.0 { + Some(timeout_height) => write!(f, "{}", timeout_height), + None => write!(f, "no timeout"), + } + } +} From 69234fbe7d3ee615f5dcb3dbca37203ba4aec30d Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 29 Jun 2022 16:26:15 -0400 Subject: [PATCH 23/43] Use TimeoutHeight everywhere --- .../applications/transfer/msgs/transfer.rs | 37 ++++--------------- modules/src/core/ics04_channel/context.rs | 13 +++---- modules/src/core/ics04_channel/error.rs | 5 ++- modules/src/core/ics04_channel/events.rs | 7 +--- .../core/ics04_channel/handler/recv_packet.rs | 11 +++--- .../core/ics04_channel/handler/send_packet.rs | 9 +++-- .../src/core/ics04_channel/handler/timeout.rs | 12 +++--- modules/src/core/ics04_channel/packet.rs | 13 +------ relayer/src/transfer.rs | 13 ++++--- tools/test-framework/src/relayer/transfer.rs | 3 +- 10 files changed, 45 insertions(+), 78 deletions(-) diff --git a/modules/src/applications/transfer/msgs/transfer.rs b/modules/src/applications/transfer/msgs/transfer.rs index 130f02b2bb..7c967ed71c 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,32 +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))?; - // `RawPacket.timeout_height` is treated differently 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". - // - // Note: it is important for `revision_number` to also be `0`, otherwise - // packet commitment proofs will be incorrect (see proof construction in - // `ChannelReader::packet_commitment()`). Note also that ibc-go conforms - // to this. - let timeout_height: Option = raw_msg - .timeout_height - .and_then(|raw_height| { - if raw_height.revision_number == 0 && raw_height.revision_height == 0 { - None - } else { - Some(raw_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 @@ -116,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(), } } @@ -177,7 +154,7 @@ pub mod test_util { sender: address.clone(), receiver: address, timeout_timestamp: Timestamp::now().add(Duration::from_secs(10)).unwrap(), - timeout_height: Some(Height::new(0, height).unwrap()), + timeout_height: Height::new(0, height).unwrap().into(), } } } diff --git a/modules/src/core/ics04_channel/context.rs b/modules/src/core/ics04_channel/context.rs index dd4579d849..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 { @@ -75,19 +76,15 @@ pub trait ChannelReader { fn packet_commitment( &self, packet_data: Vec, - timeout_height: Option, + timeout_height: TimeoutHeight, timeout_timestamp: Timestamp, ) -> PacketCommitment { let mut hash_input = timeout_timestamp.nanoseconds().to_be_bytes().to_vec(); - let (revision_number, revision_height) = match timeout_height { - Some(height) => ( - height.revision_number().to_be_bytes(), - height.revision_height().to_be_bytes(), - ), - None => (0u64.to_be_bytes(), 0u64.to_be_bytes()), - }; + 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); diff --git a/modules/src/core/ics04_channel/error.rs b/modules/src/core/ics04_channel/error.rs index 18bf45aa97..3505d25306 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; @@ -201,7 +202,7 @@ define_error! { LowPacketHeight { chain_height: Height, - timeout_height: Height + timeout_height: TimeoutHeight } | e | { format_args!( @@ -211,7 +212,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 e838ffcbce..49ee2d308f 100644 --- a/modules/src/core/ics04_channel/events.rs +++ b/modules/src/core/ics04_channel/events.rs @@ -334,10 +334,7 @@ impl TryFrom for Vec { attributes.push(sequence); let timeout_height = Tag { key: PKT_TIMEOUT_HEIGHT_ATTRIBUTE_KEY.parse().unwrap(), - value: match p.timeout_height { - Some(timeout_height) => timeout_height.to_string().parse().unwrap(), - None => "0-0".parse().unwrap(), - }, + value: p.timeout_height.into(), }; attributes.push(timeout_height); let timeout_timestamp = Tag { @@ -1231,7 +1228,7 @@ mod tests { destination_port: "b_test_port".parse().unwrap(), destination_channel: "channel-1".parse().unwrap(), data: "test_data".as_bytes().to_vec(), - timeout_height: Some(Height::new(1, 10).unwrap()), + timeout_height: Height::new(1, 10).unwrap().into(), timeout_timestamp: Timestamp::now(), }; let mut abci_events = vec![]; diff --git a/modules/src/core/ics04_channel/handler/recv_packet.rs b/modules/src/core/ics04_channel/handler/recv_packet.rs index 9e7bdd171d..312932a369 100644 --- a/modules/src/core/ics04_channel/handler/recv_packet.rs +++ b/modules/src/core/ics04_channel/handler/recv_packet.rs @@ -60,10 +60,11 @@ pub fn process(ctx: &dyn ChannelReader, msg: &MsgRecvPacket) -> HandlerResult HandlerResult HandlerResult proof_height { - return Err(Error::packet_timeout_height_not_reached( - timeout_height, - proof_height, - )); - } + if packet.timeout_height.has_expired(&proof_height) { + return Err(Error::packet_timeout_height_not_reached( + packet.timeout_height, + proof_height, + )); } let consensus_state = ctx.client_consensus_state(&client_id, proof_height)?; diff --git a/modules/src/core/ics04_channel/packet.rs b/modules/src/core/ics04_channel/packet.rs index 458ff13c03..cff4c1aeb6 100644 --- a/modules/src/core/ics04_channel/packet.rs +++ b/modules/src/core/ics04_channel/packet.rs @@ -6,7 +6,6 @@ use core::str::FromStr; use serde_derive::{Deserialize, Serialize}; 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::error::Error; use crate::core::ics24_host::identifier::{ChannelId, PortId}; @@ -238,7 +237,7 @@ impl TryFrom for Packet { .try_into() .map_err(|_| Error::invalid_timeout_height())?; - if packet_timeout_height.has_timeout() == false && raw_pkt.timeout_timestamp == 0 { + if !packet_timeout_height.has_timeout() && raw_pkt.timeout_timestamp == 0 { return Err(Error::zero_packet_timeout()); } if raw_pkt.data.is_empty() { @@ -328,15 +327,7 @@ impl From for RawPacket { destination_port: packet.destination_port.to_string(), destination_channel: packet.destination_channel.to_string(), data: packet.data, - // We map "no timeout height" to `Some(RawHeight::zero)` due to a quirk - // in ICS-4. See https://github.com/cosmos/ibc/issues/776. - timeout_height: match packet.timeout_height { - Some(height) => Some(height.into()), - None => Some(RawHeight { - revision_number: 0, - revision_height: 0, - }), - }, + timeout_height: packet.timeout_height.into(), timeout_timestamp: packet.timeout_timestamp.nanoseconds(), } } 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/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, )) } From bb8192e6e40b73cea94110a405aea64863623f57 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 29 Jun 2022 16:40:09 -0400 Subject: [PATCH 24/43] Fixes after merging with master --- Cargo.lock | 1 + relayer/src/link/relay_path.rs | 10 +++++----- relayer/src/supervisor.rs | 4 ++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ba2f747e9b..86fc725736 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1636,6 +1636,7 @@ dependencies = [ "opentelemetry-prometheus", "prometheus", "rouille", + "tendermint", "uuid 1.1.2", ] 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 f8189defc7..7c9699fda9 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, From 78eb9f178b94c00b2182f67a60263ec506a90865 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 29 Jun 2022 17:13:07 -0400 Subject: [PATCH 25/43] has_expired() fix --- modules/src/core/ics04_channel/timeout.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/src/core/ics04_channel/timeout.rs b/modules/src/core/ics04_channel/timeout.rs index d72bddffd2..1d7763b601 100644 --- a/modules/src/core/ics04_channel/timeout.rs +++ b/modules/src/core/ics04_channel/timeout.rs @@ -44,7 +44,7 @@ impl TimeoutHeight { /// Check if a height is past the timeout height, and thus is expired. pub fn has_expired(&self, height: &Height) -> bool { match self.0 { - Some(timeout_height) => timeout_height >= *height, + Some(timeout_height) => *height >= timeout_height, // When there's no timeout, heights are never expired None => false, } From 1178a2eb1dac444235ebb2b6f2d701a226065cf9 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 29 Jun 2022 17:19:25 -0400 Subject: [PATCH 26/43] doc url --- modules/src/core/ics04_channel/timeout.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/src/core/ics04_channel/timeout.rs b/modules/src/core/ics04_channel/timeout.rs index 1d7763b601..99e4c76435 100644 --- a/modules/src/core/ics04_channel/timeout.rs +++ b/modules/src/core/ics04_channel/timeout.rs @@ -84,7 +84,7 @@ impl TryFrom> for TimeoutHeight { } } /// We map "no timeout height" to `Some(RawHeight::zero)` due to a quirk -/// in ICS-4. See https://github.com/cosmos/ibc/issues/776. +/// in ICS-4. See . impl From for Option { fn from(timeout_height: TimeoutHeight) -> Self { let raw_height = match timeout_height.0 { From a1bd379c260b6aa5f72e49bda3572c0952ba1671 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 30 Jun 2022 12:50:53 -0400 Subject: [PATCH 27/43] fix send_packet test --- .../core/ics04_channel/handler/send_packet.rs | 31 ++++++++++++------- modules/src/core/ics04_channel/packet.rs | 12 ------- 2 files changed, 19 insertions(+), 24 deletions(-) diff --git a/modules/src/core/ics04_channel/handler/send_packet.rs b/modules/src/core/ics04_channel/handler/send_packet.rs index 36bcc4350a..83308dbc07 100644 --- a/modules/src/core/ics04_channel/handler/send_packet.rs +++ b/modules/src/core/ics04_channel/handler/send_packet.rs @@ -137,14 +137,6 @@ mod tests { let context = MockContext::default(); - let timestamp = Timestamp::now().add(Duration::from_secs(10)); - //CD:TODO remove unwrap - let mut packet: Packet = get_dummy_raw_packet(1, timestamp.unwrap().nanoseconds()) - .try_into() - .unwrap(); - packet.sequence = 1.into(); - packet.data = vec![0]; - let channel_end = ChannelEnd::new( State::TryOpen, Order::default(), @@ -165,11 +157,26 @@ mod tests { ZERO_DURATION, ); - let mut packet_old: Packet = get_dummy_raw_packet(1, 1).try_into().unwrap(); + let timestamp_future = Timestamp::now().add(Duration::from_secs(10)); + let timestamp_ns_past = 1; + + // FIXME: Test with old timeout too + let timeout_height = 10; + //CD:TODO remove unwrap + let mut packet: Packet = + get_dummy_raw_packet(timeout_height, timestamp_future.unwrap().nanoseconds()) + .try_into() + .unwrap(); + packet.sequence = 1.into(); + packet.data = vec![0]; + + let mut packet_old: Packet = get_dummy_raw_packet(timeout_height, timestamp_ns_past) + .try_into() + .unwrap(); packet_old.sequence = 1.into(); packet_old.data = vec![0]; - let client_height = Height::new(0, 1).unwrap(); + let client_height_no_packet_timeout = Height::new(0, 5).unwrap(); let tests: Vec = vec![ Test { @@ -182,7 +189,7 @@ mod tests { name: "Good parameters".to_string(), ctx: context .clone() - .with_client(&ClientId::default(), Height::new(0, 1).unwrap()) + .with_client(&ClientId::default(), client_height_no_packet_timeout) .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()), @@ -192,7 +199,7 @@ mod tests { Test { name: "Packet timeout".to_string(), ctx: context - .with_client(&ClientId::default(), client_height) + .with_client(&ClientId::default(), client_height_no_packet_timeout) .with_connection(ConnectionId::default(), connection_end) .with_channel(PortId::default(), ChannelId::default(), channel_end) .with_send_sequence(PortId::default(), ChannelId::default(), 1.into()), diff --git a/modules/src/core/ics04_channel/packet.rs b/modules/src/core/ics04_channel/packet.rs index cff4c1aeb6..e1014960af 100644 --- a/modules/src/core/ics04_channel/packet.rs +++ b/modules/src/core/ics04_channel/packet.rs @@ -220,18 +220,6 @@ impl TryFrom for Packet { return Err(Error::zero_packet_sequence()); } - // `RawPacket.timeout_height` is treated differently 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". - // - // Note: it is important for `revision_number` to also be `0`, otherwise - // packet commitment proofs will be incorrect (see proof construction in - // `ChannelReader::packet_commitment()`). Note also that ibc-go conforms - // to this. let packet_timeout_height: TimeoutHeight = raw_pkt .timeout_height .try_into() From 6e2cf233db56ead99156886092965af045c7854b Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 30 Jun 2022 13:51:15 -0400 Subject: [PATCH 28/43] Remove timeout_height == 0 && timestamp == 0 check --- modules/src/core/ics04_channel/error.rs | 3 --- modules/src/core/ics04_channel/packet.rs | 3 --- relayer-cli/src/commands/tx/transfer.rs | 8 -------- 3 files changed, 14 deletions(-) diff --git a/modules/src/core/ics04_channel/error.rs b/modules/src/core/ics04_channel/error.rs index 3505d25306..67a465ca79 100644 --- a/modules/src/core/ics04_channel/error.rs +++ b/modules/src/core/ics04_channel/error.rs @@ -83,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" }, diff --git a/modules/src/core/ics04_channel/packet.rs b/modules/src/core/ics04_channel/packet.rs index e1014960af..f142db07f7 100644 --- a/modules/src/core/ics04_channel/packet.rs +++ b/modules/src/core/ics04_channel/packet.rs @@ -225,9 +225,6 @@ impl TryFrom for Packet { .try_into() .map_err(|_| Error::invalid_timeout_height())?; - if !packet_timeout_height.has_timeout() && raw_pkt.timeout_timestamp == 0 { - return Err(Error::zero_packet_timeout()); - } if raw_pkt.data.is_empty() { return Err(Error::zero_packet_data()); } 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, From 87cbfed19bde2f868299d3d405dcef3eb772a599 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 30 Jun 2022 13:52:40 -0400 Subject: [PATCH 29/43] Remove `has_timeout()` --- modules/src/core/ics04_channel/timeout.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/modules/src/core/ics04_channel/timeout.rs b/modules/src/core/ics04_channel/timeout.rs index 99e4c76435..cc9b48c4cf 100644 --- a/modules/src/core/ics04_channel/timeout.rs +++ b/modules/src/core/ics04_channel/timeout.rs @@ -49,11 +49,6 @@ impl TimeoutHeight { None => false, } } - - // FIXME: Find better name - pub fn has_timeout(&self) -> bool { - self.0.is_some() - } } impl TryFrom for TimeoutHeight { From 95eb992deb7a32d06c44b41215601aff9311f3f8 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 30 Jun 2022 14:08:16 -0400 Subject: [PATCH 30/43] Change TimeoutHeight impl --- .../core/ics04_channel/handler/recv_packet.rs | 2 +- .../core/ics04_channel/handler/send_packet.rs | 2 +- .../src/core/ics04_channel/handler/timeout.rs | 2 +- modules/src/core/ics04_channel/packet.rs | 2 +- modules/src/core/ics04_channel/timeout.rs | 61 +++++++++++-------- 5 files changed, 39 insertions(+), 30 deletions(-) diff --git a/modules/src/core/ics04_channel/handler/recv_packet.rs b/modules/src/core/ics04_channel/handler/recv_packet.rs index 312932a369..37c3db9011 100644 --- a/modules/src/core/ics04_channel/handler/recv_packet.rs +++ b/modules/src/core/ics04_channel/handler/recv_packet.rs @@ -60,7 +60,7 @@ pub fn process(ctx: &dyn ChannelReader, msg: &MsgRecvPacket) -> HandlerResult HandlerResult HandlerResult bool { - let height_timed_out = self.timeout_height.has_expired(&dst_chain_height); + 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; diff --git a/modules/src/core/ics04_channel/timeout.rs b/modules/src/core/ics04_channel/timeout.rs index cc9b48c4cf..d3f7e81002 100644 --- a/modules/src/core/ics04_channel/timeout.rs +++ b/modules/src/core/ics04_channel/timeout.rs @@ -17,40 +17,49 @@ use tendermint::abci::tag::Value as TagValue; /// 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, Default, Hash, Eq, PartialEq, Serialize, Deserialize)] -pub struct TimeoutHeight(Option); +#[derive(Clone, Copy, Debug, Hash, Eq, PartialEq, Serialize, Deserialize)] +pub enum TimeoutHeight { + Never, + At(Height), +} impl TimeoutHeight { pub fn no_timeout() -> Self { - Self(None) + Self::Never } /// Revision number to be used in packet commitment computation pub fn commitment_revision_number(&self) -> u64 { - match self.0 { - Some(height) => height.revision_number(), - None => 0, + 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.0 { - Some(height) => height.revision_height(), - None => 0, + match self { + Self::At(height) => height.revision_height(), + Self::Never => 0, } } /// Check if a height is past the timeout height, and thus is expired. - pub fn has_expired(&self, height: &Height) -> bool { - match self.0 { - Some(timeout_height) => *height >= timeout_height, + 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 - None => false, + Self::Never => false, } } } +impl Default for TimeoutHeight { + fn default() -> Self { + Self::Never + } +} + impl TryFrom for TimeoutHeight { type Error = ICS2Error; @@ -60,10 +69,10 @@ impl TryFrom for TimeoutHeight { // `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(None)) + Ok(TimeoutHeight::Never) } else { let height: Height = raw_height.try_into()?; - Ok(TimeoutHeight(Some(height))) + Ok(TimeoutHeight::At(height)) } } } @@ -74,7 +83,7 @@ impl TryFrom> for TimeoutHeight { fn try_from(maybe_raw_height: Option) -> Result { match maybe_raw_height { Some(raw_height) => Self::try_from(raw_height), - None => Ok(TimeoutHeight(None)), + None => Ok(TimeoutHeight::Never), } } } @@ -82,9 +91,9 @@ impl TryFrom> for TimeoutHeight { /// in ICS-4. See . impl From for Option { fn from(timeout_height: TimeoutHeight) -> Self { - let raw_height = match timeout_height.0 { - Some(height) => height.into(), - None => RawHeight { + let raw_height = match timeout_height { + TimeoutHeight::At(height) => height.into(), + TimeoutHeight::Never => RawHeight { revision_number: 0, revision_height: 0, }, @@ -96,24 +105,24 @@ impl From for Option { impl From for TimeoutHeight { fn from(height: Height) -> Self { - Self(Some(height)) + Self::At(height) } } impl From for TagValue { fn from(timeout_height: TimeoutHeight) -> Self { - match timeout_height.0 { - Some(height) => height.to_string().parse().unwrap(), - None => "0-0".parse().unwrap(), + 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.0 { - Some(timeout_height) => write!(f, "{}", timeout_height), - None => write!(f, "no timeout"), + match self { + TimeoutHeight::At(timeout_height) => write!(f, "{}", timeout_height), + TimeoutHeight::Never => write!(f, "no timeout"), } } } From b3d424820df66f760287eace5101bfbc39683d0d Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 30 Jun 2022 14:50:49 -0400 Subject: [PATCH 31/43] docstrings --- modules/src/core/ics04_channel/handler/timeout.rs | 10 ++++++++-- modules/src/core/ics04_channel/timeout.rs | 14 ++++++++------ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/modules/src/core/ics04_channel/handler/timeout.rs b/modules/src/core/ics04_channel/handler/timeout.rs index 69f4ca5e2c..1e18816ebd 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(); @@ -169,13 +174,14 @@ mod tests { let context = MockContext::default(); - let height = 2; + let msg_proof_height = 2; + let msg_timeout_height = 5; let timeout_timestamp = 5; let client_height = Height::new(0, 2).unwrap(); let msg = - MsgTimeout::try_from(get_dummy_raw_msg_timeout(height, timeout_timestamp)).unwrap(); + 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/timeout.rs b/modules/src/core/ics04_channel/timeout.rs index d3f7e81002..3e44eec175 100644 --- a/modules/src/core/ics04_channel/timeout.rs +++ b/modules/src/core/ics04_channel/timeout.rs @@ -8,15 +8,16 @@ use ibc_proto::ibc::core::client::v1::Height as RawHeight; use serde::{Deserialize, Serialize}; use tendermint::abci::tag::Value as TagValue; -/// Represents a height to be used to timeout packets. +/// 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". +/// 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, Serialize, Deserialize)] pub enum TimeoutHeight { Never, @@ -44,10 +45,11 @@ impl TimeoutHeight { } } - /// Check if a height is past the timeout height, and thus is expired. + /// 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, + Self::At(timeout_height) => height > *timeout_height, // When there's no timeout, heights are never expired Self::Never => false, } From 6ed145b855b87c84db7aceacbd3ebc35d4e9ed83 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 30 Jun 2022 14:51:12 -0400 Subject: [PATCH 32/43] tests fix --- .../src/core/ics04_channel/handler/timeout.rs | 8 ++++++-- modules/src/core/ics04_channel/msgs/timeout.rs | 18 ++++++++++++------ modules/src/core/ics04_channel/packet.rs | 9 ++++++++- 3 files changed, 26 insertions(+), 9 deletions(-) diff --git a/modules/src/core/ics04_channel/handler/timeout.rs b/modules/src/core/ics04_channel/handler/timeout.rs index 1e18816ebd..d1ebe5cb3a 100644 --- a/modules/src/core/ics04_channel/handler/timeout.rs +++ b/modules/src/core/ics04_channel/handler/timeout.rs @@ -180,8 +180,12 @@ mod tests { let client_height = Height::new(0, 2).unwrap(); - let msg = - MsgTimeout::try_from(get_dummy_raw_msg_timeout(msg_proof_height, 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/msgs/timeout.rs b/modules/src/core/ics04_channel/msgs/timeout.rs index 866c69abc8..966fdd80a2 100644 --- a/modules/src/core/ics04_channel/msgs/timeout.rs +++ b/modules/src/core/ics04_channel/msgs/timeout.rs @@ -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/packet.rs b/modules/src/core/ics04_channel/packet.rs index 8d26b9fce3..44ff740afb 100644 --- a/modules/src/core/ics04_channel/packet.rs +++ b/modules/src/core/ics04_channel/packet.rs @@ -468,13 +468,20 @@ mod tests { }, 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 }, - want_pass: false, + want_pass: true, }, ]; From a2ec5aae66da486963ee8cb565adfab1fe7a417f Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 30 Jun 2022 14:55:49 -0400 Subject: [PATCH 33/43] fix history manipulation test --- modules/src/mock/context.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/modules/src/mock/context.rs b/modules/src/mock/context.rs index d3287a4a46..644a059cbd 100644 --- a/modules/src/mock/context.rs +++ b/modules/src/mock/context.rs @@ -1459,15 +1459,14 @@ mod tests { "failed while increasing height for context {:?}", test.ctx ); - if current_height > Height::new(cv, 0).unwrap() { - 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 + ); } } From 8eff6d6aa443ac36a3bc5b1d508665d2fb8f32e8 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 30 Jun 2022 15:43:44 -0400 Subject: [PATCH 34/43] transfer test: no timeout --- modules/src/applications/transfer/msgs/transfer.rs | 6 +++--- modules/src/core/ics26_routing/handler.rs | 11 +++++++++-- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/modules/src/applications/transfer/msgs/transfer.rs b/modules/src/applications/transfer/msgs/transfer.rs index 7c967ed71c..7b86ed9559 100644 --- a/modules/src/applications/transfer/msgs/transfer.rs +++ b/modules/src/applications/transfer/msgs/transfer.rs @@ -130,18 +130,18 @@ 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, }; // FIXME (BEFORE MERGE): Add at least 1 test that uses `timeout_height: None` // Returns a dummy ICS20 `MsgTransfer`, for testing only! - pub fn get_dummy_msg_transfer(height: u64) -> MsgTransfer { + pub fn get_dummy_msg_transfer(timeout_height: TimeoutHeight) -> MsgTransfer { let address: Signer = get_dummy_bech32_account().as_str().parse().unwrap(); MsgTransfer { source_port: PortId::default(), @@ -154,7 +154,7 @@ pub mod test_util { sender: address.clone(), receiver: address, timeout_timestamp: Timestamp::now().add(Duration::from_secs(10)).unwrap(), - timeout_height: Height::new(0, height).unwrap().into(), + timeout_height, } } } diff --git a/modules/src/core/ics26_routing/handler.rs b/modules/src/core/ics26_routing/handler.rs index ffdb0bac6d..0d812c1991 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; @@ -291,8 +292,9 @@ 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()); + let msg_transfer_two = get_dummy_msg_transfer(Height::new(0, 36).unwrap().into()); + let msg_transfer_no_timeout = get_dummy_msg_transfer(TimeoutHeight::no_timeout()); let mut msg_to_on_close = MsgTimeoutOnClose::try_from(get_dummy_raw_msg_timeout_on_close(36, 5)).unwrap(); @@ -470,6 +472,11 @@ mod tests { .into(), want_pass: true, }, + Test { + name: "Transfer message no timeout".to_string(), + msg: msg_transfer_no_timeout.into(), + want_pass: true, + }, //ICS04-close channel Test { name: "Channel close init succeeds".to_string(), From 62cad52d9a8457b3d9f980874d7194b192ff8e3d Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 30 Jun 2022 15:50:43 -0400 Subject: [PATCH 35/43] msgtransfer test --- .../src/applications/transfer/msgs/transfer.rs | 12 ++++++++---- modules/src/core/ics26_routing/handler.rs | 16 +++++++++++++--- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/modules/src/applications/transfer/msgs/transfer.rs b/modules/src/applications/transfer/msgs/transfer.rs index 7b86ed9559..7d92972ce8 100644 --- a/modules/src/applications/transfer/msgs/transfer.rs +++ b/modules/src/applications/transfer/msgs/transfer.rs @@ -139,9 +139,12 @@ pub mod test_util { timestamp::Timestamp, }; - // FIXME (BEFORE MERGE): Add at least 1 test that uses `timeout_height: None` - // Returns a dummy ICS20 `MsgTransfer`, for testing only! - pub fn get_dummy_msg_transfer(timeout_height: TimeoutHeight) -> 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(), @@ -153,7 +156,8 @@ pub mod test_util { .into(), sender: address.clone(), receiver: address, - timeout_timestamp: Timestamp::now().add(Duration::from_secs(10)).unwrap(), + timeout_timestamp: timeout_timestamp + .unwrap_or_else(|| Timestamp::now().add(Duration::from_secs(10)).unwrap()), timeout_height, } } diff --git a/modules/src/core/ics26_routing/handler.rs b/modules/src/core/ics26_routing/handler.rs index 0d812c1991..94f6d64e0c 100644 --- a/modules/src/core/ics26_routing/handler.rs +++ b/modules/src/core/ics26_routing/handler.rs @@ -292,9 +292,13 @@ mod tests { MsgChannelCloseConfirm::try_from(get_dummy_raw_msg_chan_close_confirm(client_height)) .unwrap(); - let msg_transfer = get_dummy_msg_transfer(Height::new(0, 35).unwrap().into()); - let msg_transfer_two = get_dummy_msg_transfer(Height::new(0, 36).unwrap().into()); - let msg_transfer_no_timeout = get_dummy_msg_transfer(TimeoutHeight::no_timeout()); + 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(); @@ -472,11 +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(), From 0d3d2f26dd36ad55b33e9dd9c2c27358741df645 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 30 Jun 2022 15:54:30 -0400 Subject: [PATCH 36/43] packet no timeout or timestamp test --- modules/src/core/ics04_channel/packet.rs | 38 ++++++++++++++---------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/modules/src/core/ics04_channel/packet.rs b/modules/src/core/ics04_channel/packet.rs index 44ff740afb..d1f8aef99d 100644 --- a/modules/src/core/ics04_channel/packet.rs +++ b/modules/src/core/ics04_channel/packet.rs @@ -364,19 +364,27 @@ 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 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 (06/30/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: "Src port validation: correct".to_string(), raw: RawPacket { source_port: "srcportp34".to_string(), - ..default_raw_msg.clone() + ..default_raw_packet.clone() }, want_pass: true, }, @@ -384,7 +392,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, }, @@ -392,7 +400,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, }, @@ -400,7 +408,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, }, @@ -408,7 +416,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, }, @@ -416,7 +424,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, }, @@ -424,7 +432,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, }, @@ -432,7 +440,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, }, @@ -440,7 +448,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, }, @@ -448,7 +456,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, }, @@ -456,7 +464,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, }, @@ -464,7 +472,7 @@ 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, }, @@ -479,7 +487,7 @@ mod tests { name: "Missing timeout height".to_string(), raw: RawPacket { timeout_height: None, - ..default_raw_msg + ..default_raw_packet }, want_pass: true, }, From c1a2a8646d101856432f449b5c84ff06daabd634 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 30 Jun 2022 16:05:59 -0400 Subject: [PATCH 37/43] send_packet timeout height equal destination chain height --- .../core/ics04_channel/handler/send_packet.rs | 54 +++++++++++++------ 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/modules/src/core/ics04_channel/handler/send_packet.rs b/modules/src/core/ics04_channel/handler/send_packet.rs index 3694862ed8..5ae45adb37 100644 --- a/modules/src/core/ics04_channel/handler/send_packet.rs +++ b/modules/src/core/ics04_channel/handler/send_packet.rs @@ -157,26 +157,35 @@ mod tests { ZERO_DURATION, ); - let timestamp_future = Timestamp::now().add(Duration::from_secs(10)); + let timestamp_future = Timestamp::now().add(Duration::from_secs(10)).unwrap(); let timestamp_ns_past = 1; // FIXME: Test with old timeout too - let timeout_height = 10; - //CD:TODO remove unwrap - let mut packet: Packet = - get_dummy_raw_packet(timeout_height, timestamp_future.unwrap().nanoseconds()) - .try_into() - .unwrap(); + let timeout_height_future = 10; + + let mut packet: Packet = get_dummy_raw_packet( + timeout_height_future, + timestamp_future.clone().nanoseconds(), + ) + .try_into() + .unwrap(); packet.sequence = 1.into(); packet.data = vec![0]; - let mut packet_old: Packet = get_dummy_raw_packet(timeout_height, timestamp_ns_past) - .try_into() - .unwrap(); - packet_old.sequence = 1.into(); - packet_old.data = vec![0]; + let mut packet_with_timestamp_old: Packet = + get_dummy_raw_packet(timeout_height_future, timestamp_ns_past) + .try_into() + .unwrap(); + packet_with_timestamp_old.sequence = 1.into(); + packet_with_timestamp_old.data = vec![0]; + + let client_raw_height = 5; + let packet_timeout_equal_client_height = + get_dummy_raw_packet(client_raw_height, timestamp_future.nanoseconds()) + .try_into() + .unwrap(); - let client_height_no_packet_timeout = Height::new(0, 5).unwrap(); + let client_height = Height::new(0, client_raw_height).unwrap(); let tests: Vec = vec![ Test { @@ -189,7 +198,7 @@ mod tests { name: "Good parameters".to_string(), ctx: context .clone() - .with_client(&ClientId::default(), client_height_no_packet_timeout) + .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()), @@ -197,13 +206,24 @@ 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 due to timestamp".to_string(), ctx: context - .with_client(&ClientId::default(), client_height_no_packet_timeout) + .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, }, ] From a8a958c89324c5e8a47a5bce64ba96958e90b15e Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 30 Jun 2022 16:23:39 -0400 Subject: [PATCH 38/43] send_packet test with timeout = height - 1 --- .../core/ics04_channel/handler/send_packet.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/modules/src/core/ics04_channel/handler/send_packet.rs b/modules/src/core/ics04_channel/handler/send_packet.rs index 5ae45adb37..258b77de33 100644 --- a/modules/src/core/ics04_channel/handler/send_packet.rs +++ b/modules/src/core/ics04_channel/handler/send_packet.rs @@ -160,7 +160,6 @@ mod tests { let timestamp_future = Timestamp::now().add(Duration::from_secs(10)).unwrap(); let timestamp_ns_past = 1; - // FIXME: Test with old timeout too let timeout_height_future = 10; let mut packet: Packet = get_dummy_raw_packet( @@ -180,10 +179,14 @@ mod tests { packet_with_timestamp_old.data = vec![0]; let client_raw_height = 5; - let packet_timeout_equal_client_height = + let packet_timeout_equal_client_height: Packet = get_dummy_raw_packet(client_raw_height, timestamp_future.nanoseconds()) .try_into() .unwrap(); + let packet_timeout_one_before_client_height: Packet = + get_dummy_raw_packet(client_raw_height - 1, timestamp_future.nanoseconds()) + .try_into() + .unwrap(); let client_height = Height::new(0, client_raw_height).unwrap(); @@ -216,6 +219,17 @@ mod tests { 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 From c0fafdbbf9052699c8ed5747c5956fd7e3de46d4 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 30 Jun 2022 16:43:50 -0400 Subject: [PATCH 39/43] test invalid timeout height --- modules/src/core/ics04_channel/packet.rs | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/modules/src/core/ics04_channel/packet.rs b/modules/src/core/ics04_channel/packet.rs index d1f8aef99d..6efc76a7ea 100644 --- a/modules/src/core/ics04_channel/packet.rs +++ b/modules/src/core/ics04_channel/packet.rs @@ -220,6 +220,14 @@ impl TryFrom for Packet { return Err(Error::zero_packet_sequence()); } + // 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 .try_into() @@ -351,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; @@ -367,6 +376,12 @@ mod tests { 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(), @@ -374,12 +389,17 @@ mod tests { want_pass: true, }, Test { - // Note: ibc-go currently (06/30/2022) incorrectly rejects this + // 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 { From 4a9dca7bf1fba72aa4f25806a9442616a07c1c22 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 30 Jun 2022 16:48:16 -0400 Subject: [PATCH 40/43] fix docstring --- modules/src/core/ics03_connection/msgs/conn_open_ack.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 19437f50fc..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,8 +27,8 @@ 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. + /// 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()) } From 6a58582837b738a08267f9eaabe2f1748299b1fc Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 30 Jun 2022 17:02:43 -0400 Subject: [PATCH 41/43] clippy --- modules/src/core/ics04_channel/handler/send_packet.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/modules/src/core/ics04_channel/handler/send_packet.rs b/modules/src/core/ics04_channel/handler/send_packet.rs index 258b77de33..83e4540f8a 100644 --- a/modules/src/core/ics04_channel/handler/send_packet.rs +++ b/modules/src/core/ics04_channel/handler/send_packet.rs @@ -162,12 +162,10 @@ mod tests { let timeout_height_future = 10; - let mut packet: Packet = get_dummy_raw_packet( - timeout_height_future, - timestamp_future.clone().nanoseconds(), - ) - .try_into() - .unwrap(); + let mut packet: Packet = + get_dummy_raw_packet(timeout_height_future, timestamp_future.nanoseconds()) + .try_into() + .unwrap(); packet.sequence = 1.into(); packet.data = vec![0]; From e262c3c9a5eed679b7ba8529bbe746e6603022c7 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Fri, 1 Jul 2022 11:59:38 +0200 Subject: [PATCH 42/43] Fix clippy warnings introduced in Rust 1.62 --- relayer-cli/src/commands/keys/list.rs | 3 ++- relayer/src/worker/map.rs | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) 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/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); } } From f5c3f67f7f6ebca1a275bb479407a7f6e0601be4 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Fri, 1 Jul 2022 17:06:21 +0200 Subject: [PATCH 43/43] Fix serialization of `TimeoutHeight` to match the format used by `Height` --- e2e/e2e/common.py | 7 +++ e2e/e2e/packet.py | 2 +- modules/src/core/ics04_channel/timeout.rs | 65 +++++++++++++++++++++-- 3 files changed, 68 insertions(+), 6 deletions(-) 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/core/ics04_channel/timeout.rs b/modules/src/core/ics04_channel/timeout.rs index 3e44eec175..ed4a5641f5 100644 --- a/modules/src/core/ics04_channel/timeout.rs +++ b/modules/src/core/ics04_channel/timeout.rs @@ -1,13 +1,13 @@ -use crate::prelude::*; - use core::fmt::Display; -use crate::{core::ics02_client::error::Error as ICS2Error, Height}; +use serde::{Deserialize, Serialize}; use ibc_proto::ibc::core::client::v1::Height as RawHeight; -use serde::{Deserialize, Serialize}; 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. /// @@ -18,7 +18,7 @@ use tendermint::abci::tag::Value as TagValue; /// 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, Serialize, Deserialize)] +#[derive(Clone, Copy, Debug, Hash, Eq, PartialEq)] pub enum TimeoutHeight { Never, At(Height), @@ -128,3 +128,58 @@ impl Display for TimeoutHeight { } } } + +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) + }) + } +}