From 39d029ba3d760040c7b32ea5668b0deb7a984669 Mon Sep 17 00:00:00 2001 From: David Salami Date: Mon, 5 Sep 2022 18:25:54 +0100 Subject: [PATCH] fix timeout processing --- modules/src/clients/ics11_beefy/header.rs | 116 +++++++++--------- modules/src/core/ics04_channel/error.rs | 13 ++ .../handler/chan_close_confirm.rs | 2 +- .../ics04_channel/handler/chan_open_ack.rs | 2 +- .../handler/chan_open_confirm.rs | 2 +- .../ics04_channel/handler/chan_open_try.rs | 2 +- .../src/core/ics04_channel/handler/timeout.rs | 18 +-- .../ics04_channel/handler/timeout_on_close.rs | 5 +- .../src/core/ics04_channel/handler/verify.rs | 7 +- .../ics04_channel/msgs/timeout_on_close.rs | 7 +- modules/src/core/ics04_channel/packet.rs | 28 ++++- modules/src/timestamp.rs | 6 +- 12 files changed, 127 insertions(+), 81 deletions(-) diff --git a/modules/src/clients/ics11_beefy/header.rs b/modules/src/clients/ics11_beefy/header.rs index 56ad47a947..9e8280fb60 100644 --- a/modules/src/clients/ics11_beefy/header.rs +++ b/modules/src/clients/ics11_beefy/header.rs @@ -82,68 +82,74 @@ impl TryFrom for BeefyHeader { type Error = Error; fn try_from(raw_header: RawBeefyHeader) -> Result { - let headers_with_proof = raw_header.consensus_state.map(|consensus_update| { - let parachain_headers = consensus_update - .parachain_headers - .into_iter() - .map(|raw_para_header| { - let mmr_partial_leaf = raw_para_header - .mmr_leaf_partial - .ok_or_else(Error::invalid_raw_header)?; - let parent_hash = - H256::decode(&mut mmr_partial_leaf.parent_hash.as_slice()).unwrap(); - let beefy_next_authority_set = - if let Some(next_set) = mmr_partial_leaf.beefy_next_authority_set { - BeefyNextAuthoritySet { - id: next_set.id, - len: next_set.len, - root: H256::decode(&mut next_set.authority_root.as_slice()) - .map_err(|e| Error::invalid_mmr_update(e.to_string()))?, - } - } else { - Default::default() - }; - Ok(ParachainHeader { - parachain_header: decode_parachain_header(raw_para_header.parachain_header) + let headers_with_proof = raw_header + .consensus_state + .map(|consensus_update| { + let parachain_headers = consensus_update + .parachain_headers + .into_iter() + .map(|raw_para_header| { + let mmr_partial_leaf = raw_para_header + .mmr_leaf_partial + .ok_or_else(Error::invalid_raw_header)?; + let parent_hash = + H256::decode(&mut mmr_partial_leaf.parent_hash.as_slice()).unwrap(); + let beefy_next_authority_set = + if let Some(next_set) = mmr_partial_leaf.beefy_next_authority_set { + BeefyNextAuthoritySet { + id: next_set.id, + len: next_set.len, + root: H256::decode(&mut next_set.authority_root.as_slice()) + .map_err(|e| Error::invalid_mmr_update(e.to_string()))?, + } + } else { + Default::default() + }; + Ok(ParachainHeader { + parachain_header: decode_parachain_header( + raw_para_header.parachain_header, + ) .map_err(|_| Error::invalid_raw_header())?, - partial_mmr_leaf: PartialMmrLeaf { - version: { - let (major, minor) = split_leaf_version( - mmr_partial_leaf.version.saturated_into::(), - ); - MmrLeafVersion::new(major, minor) + partial_mmr_leaf: PartialMmrLeaf { + version: { + let (major, minor) = split_leaf_version( + mmr_partial_leaf.version.saturated_into::(), + ); + MmrLeafVersion::new(major, minor) + }, + parent_number_and_hash: ( + mmr_partial_leaf.parent_number, + parent_hash, + ), + beefy_next_authority_set, }, - parent_number_and_hash: (mmr_partial_leaf.parent_number, parent_hash), - beefy_next_authority_set, - }, - parachain_heads_proof: raw_para_header - .parachain_heads_proof - .into_iter() - .map(|item| { - let mut dest = [0u8; 32]; - if item.len() != 32 { - return Err(Error::invalid_raw_header()); - } - dest.copy_from_slice(&*item); - Ok(dest) - }) - .collect::, Error>>()?, - heads_leaf_index: raw_para_header.heads_leaf_index, - heads_total_count: raw_para_header.heads_total_count, - extrinsic_proof: raw_para_header.extrinsic_proof, - timestamp_extrinsic: raw_para_header.timestamp_extrinsic, + parachain_heads_proof: raw_para_header + .parachain_heads_proof + .into_iter() + .map(|item| { + let mut dest = [0u8; 32]; + if item.len() != 32 { + return Err(Error::invalid_raw_header()); + } + dest.copy_from_slice(&*item); + Ok(dest) + }) + .collect::, Error>>()?, + heads_leaf_index: raw_para_header.heads_leaf_index, + heads_total_count: raw_para_header.heads_total_count, + extrinsic_proof: raw_para_header.extrinsic_proof, + timestamp_extrinsic: raw_para_header.timestamp_extrinsic, + }) }) - }) - .collect::, Error>>() - .ok(); - parachain_headers.map(|parachain_headers| { - ParachainHeadersWithProof { + .collect::, Error>>() + .ok(); + parachain_headers.map(|parachain_headers| ParachainHeadersWithProof { headers: parachain_headers, mmr_proofs: consensus_update.mmr_proofs, mmr_size: consensus_update.mmr_size, - } + }) }) - }).flatten(); + .flatten(); let mmr_update_proof = if let Some(mmr_update) = raw_header.client_state { let commitment = mmr_update diff --git a/modules/src/core/ics04_channel/error.rs b/modules/src/core/ics04_channel/error.rs index b4473966e3..9a73862d1b 100644 --- a/modules/src/core/ics04_channel/error.rs +++ b/modules/src/core/ics04_channel/error.rs @@ -72,6 +72,9 @@ define_error! { MissingHeight | _ | { "invalid proof: missing height" }, + MissingChannelProof + | _ | { "invalid proof: missing channel proof" }, + MissingNextRecvSeq { port_channel_id: (PortId, ChannelId) } | e | { @@ -212,6 +215,16 @@ define_error! { "Receiving chain block height {0} >= packet timeout height {1}", e.chain_height, e.timeout_height) }, + PacketTimeoutNotReached + { + timeout_height: Height, + chain_height: Height, + timeout_timestamp: Timestamp, + chain_timestamp: Timestamp, + } + | e | { format_args!( + "Packet timeout not satisified for either packet height or timestamp, Packet timeout height {0}, chain height {1}, Packet timeout timestamp {2}, chain timestamp {3}", + e.timeout_height, e.chain_height, e.timeout_timestamp, e.chain_timestamp) }, PacketTimeoutHeightNotReached { 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 4c54788afe..0a54903a29 100644 --- a/modules/src/core/ics04_channel/handler/chan_close_confirm.rs +++ b/modules/src/core/ics04_channel/handler/chan_close_confirm.rs @@ -71,7 +71,7 @@ pub(crate) fn process( &channel_end, &conn, &expected_channel_end, - &msg.proofs, + &msg.proofs.object_proof(), )?; output.log("success: channel close confirm "); 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 34f50d9f40..041d1f00c3 100644 --- a/modules/src/core/ics04_channel/handler/chan_open_ack.rs +++ b/modules/src/core/ics04_channel/handler/chan_open_ack.rs @@ -78,7 +78,7 @@ pub(crate) fn process( &channel_end, &conn, &expected_channel_end, - &msg.proofs, + &msg.proofs.object_proof(), )?; output.log("success: channel open ack "); 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 e2c9c58873..1e362caef5 100644 --- a/modules/src/core/ics04_channel/handler/chan_open_confirm.rs +++ b/modules/src/core/ics04_channel/handler/chan_open_confirm.rs @@ -73,7 +73,7 @@ pub(crate) fn process( &channel_end, &conn, &expected_channel_end, - &msg.proofs, + &msg.proofs.object_proof(), ) .map_err(Error::chan_open_confirm_proof_verification)?; 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 71f2351d8b..081b1ec0f9 100644 --- a/modules/src/core/ics04_channel/handler/chan_open_try.rs +++ b/modules/src/core/ics04_channel/handler/chan_open_try.rs @@ -97,7 +97,7 @@ pub(crate) fn process( &new_channel_end, &conn, &expected_channel_end, - &msg.proofs, + &msg.proofs.object_proof(), )?; output.log("success: channel open try "); diff --git a/modules/src/core/ics04_channel/handler/timeout.rs b/modules/src/core/ics04_channel/handler/timeout.rs index 5f2e7b4480..ea771b0902 100644 --- a/modules/src/core/ics04_channel/handler/timeout.rs +++ b/modules/src/core/ics04_channel/handler/timeout.rs @@ -13,7 +13,6 @@ use crate::core::ics26_routing::context::ReaderContext; use crate::events::IbcEvent; use crate::handler::{HandlerOutput, HandlerResult}; use crate::prelude::*; -use crate::timestamp::Expiry; use core::fmt::Debug; #[derive(Clone, Debug)] @@ -59,14 +58,6 @@ pub fn process( // check that timeout height or timeout timestamp has passed on the other end let proof_height = msg.proofs.height(); - let packet_height = packet.timeout_height; - - if (!packet.timeout_height.is_zero()) && packet_height > proof_height { - return Err(Error::packet_timeout_height_not_reached( - packet.timeout_height, - proof_height, - )); - } let consensus_state = ctx .consensus_state(&client_id, proof_height) @@ -74,10 +65,11 @@ pub fn process( let proof_timestamp = consensus_state.timestamp(); - let packet_timestamp = packet.timeout_timestamp; - if let Expiry::Expired = packet_timestamp.check_expiry(&proof_timestamp) { - return Err(Error::packet_timeout_timestamp_not_reached( - packet_timestamp, + if !packet.timed_out(&proof_timestamp, proof_height) { + return Err(Error::packet_timeout_not_reached( + packet.timeout_height, + proof_height, + packet.timeout_timestamp, proof_timestamp, )); } 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 3b194a395f..6f46deb180 100644 --- a/modules/src/core/ics04_channel/handler/timeout_on_close.rs +++ b/modules/src/core/ics04_channel/handler/timeout_on_close.rs @@ -81,7 +81,10 @@ pub fn process( &source_channel_end, &connection_end, &expected_channel_end, - &msg.proofs, + msg.proofs + .other_proof() + .as_ref() + .ok_or_else(|| Error::missing_channel_proof())?, )?; let result = if source_channel_end.order_matches(&Order::Ordered) { diff --git a/modules/src/core/ics04_channel/handler/verify.rs b/modules/src/core/ics04_channel/handler/verify.rs index 7e2f2e9ea9..c9be117dce 100644 --- a/modules/src/core/ics04_channel/handler/verify.rs +++ b/modules/src/core/ics04_channel/handler/verify.rs @@ -7,6 +7,7 @@ use crate::core::ics04_channel::channel::ChannelEnd; use crate::core::ics04_channel::error::Error; use crate::core::ics04_channel::msgs::acknowledgement::Acknowledgement; use crate::core::ics04_channel::packet::{Packet, Sequence}; +use crate::core::ics23_commitment::commitment::CommitmentProofBytes; use crate::core::ics26_routing::context::ReaderContext; use crate::prelude::*; use crate::proofs::Proofs; @@ -19,7 +20,7 @@ pub fn verify_channel_proofs( channel_end: &ChannelEnd, connection_end: &ConnectionEnd, expected_chan: &ChannelEnd, - proofs: &Proofs, + proof: &CommitmentProofBytes, ) -> Result<(), Error> { // This is the client which will perform proof verification. let client_id = connection_end.client_id().clone(); @@ -32,7 +33,7 @@ pub fn verify_channel_proofs( } let consensus_state = ctx - .consensus_state(&client_id, proofs.height()) + .consensus_state(&client_id, height) .map_err(|_| Error::error_invalid_consensus_state())?; let client_def = AnyClient::::from_client_type(client_state.client_type()); @@ -46,7 +47,7 @@ pub fn verify_channel_proofs( &client_state, height, connection_end.counterparty().prefix(), - proofs.object_proof(), + &proof, consensus_state.root(), channel_end.counterparty().port_id(), channel_end.counterparty().channel_id().unwrap(), diff --git a/modules/src/core/ics04_channel/msgs/timeout_on_close.rs b/modules/src/core/ics04_channel/msgs/timeout_on_close.rs index 05fda53a54..f13a0104db 100644 --- a/modules/src/core/ics04_channel/msgs/timeout_on_close.rs +++ b/modules/src/core/ics04_channel/msgs/timeout_on_close.rs @@ -64,7 +64,12 @@ impl TryFrom for MsgTimeoutOnClose { .map_err(Error::invalid_proof)?, None, None, - None, + Some( + raw_msg + .proof_close + .try_into() + .map_err(Error::invalid_proof)?, + ), raw_msg .proof_height .ok_or_else(Error::missing_height)? diff --git a/modules/src/core/ics04_channel/packet.rs b/modules/src/core/ics04_channel/packet.rs index b8745e8f8e..6e9b985235 100644 --- a/modules/src/core/ics04_channel/packet.rs +++ b/modules/src/core/ics04_channel/packet.rs @@ -153,6 +153,12 @@ impl core::fmt::Debug for Packet { } } +pub enum TimeoutVariant { + Height, + Timestamp, + Both, +} + impl Packet { /// Checks whether a packet from a /// [`SendPacket`](crate::core::ics04_channel::events::SendPacket) @@ -168,10 +174,30 @@ 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_height != Height::zero() && self.timeout_height <= dst_chain_height) || (self.timeout_timestamp != Timestamp::none() && dst_chain_ts.check_expiry(&self.timeout_timestamp) == Expired) } + + pub fn timeout_variant( + &self, + dst_chain_ts: &Timestamp, + dst_chain_height: Height, + ) -> Option { + let height_timeout = + self.timeout_height != Height::zero() && self.timeout_height < dst_chain_height; + let timestamp_timeout = self.timeout_timestamp != Timestamp::none() + && (dst_chain_ts.check_expiry(&self.timeout_timestamp) == Expired); + if height_timeout && !timestamp_timeout { + Some(TimeoutVariant::Height) + } else if timestamp_timeout && !height_timeout { + Some(TimeoutVariant::Timestamp) + } else if timestamp_timeout && height_timeout { + Some(TimeoutVariant::Both) + } else { + None + } + } } /// Custom debug output to omit the packet data diff --git a/modules/src/timestamp.rs b/modules/src/timestamp.rs index 477afe43fa..99a1dd511a 100644 --- a/modules/src/timestamp.rs +++ b/modules/src/timestamp.rs @@ -144,7 +144,7 @@ impl Timestamp { pub fn check_expiry(&self, other: &Timestamp) -> Expiry { match (self.time, other.time) { (Some(time1), Some(time2)) => { - if time1 > time2 { + if time1 >= time2 { Expiry::Expired } else { Expiry::NotExpired @@ -271,8 +271,8 @@ mod tests { assert!(Timestamp::from_nanoseconds(i64::MAX.try_into().unwrap()).is_ok()); assert_eq!(timestamp1.check_expiry(×tamp2), Expiry::NotExpired); - assert_eq!(timestamp1.check_expiry(×tamp1), Expiry::NotExpired); - assert_eq!(timestamp2.check_expiry(×tamp2), Expiry::NotExpired); + assert_eq!(timestamp1.check_expiry(×tamp1), Expiry::Expired); + assert_eq!(timestamp2.check_expiry(×tamp2), Expiry::Expired); assert_eq!(timestamp2.check_expiry(×tamp1), Expiry::Expired); assert_eq!( timestamp1.check_expiry(&nil_timestamp),