Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Fix timeout processing #59

Merged
merged 1 commit into from
Sep 5, 2022
Merged

Fix timeout processing #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 61 additions & 55 deletions modules/src/clients/ics11_beefy/header.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,68 +82,74 @@ impl TryFrom<RawBeefyHeader> for BeefyHeader {
type Error = Error;

fn try_from(raw_header: RawBeefyHeader) -> Result<Self, Self::Error> {
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::<u8>(),
);
MmrLeafVersion::new(major, minor)
partial_mmr_leaf: PartialMmrLeaf {
version: {
let (major, minor) = split_leaf_version(
mmr_partial_leaf.version.saturated_into::<u8>(),
);
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::<Result<Vec<_>, 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::<Result<Vec<_>, 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::<Result<Vec<_>, Error>>()
.ok();
parachain_headers.map(|parachain_headers| {
ParachainHeadersWithProof {
.collect::<Result<Vec<_>, 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
Expand Down
13 changes: 13 additions & 0 deletions modules/src/core/ics04_channel/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ define_error! {
MissingHeight
| _ | { "invalid proof: missing height" },

MissingChannelProof
| _ | { "invalid proof: missing channel proof" },

MissingNextRecvSeq
{ port_channel_id: (PortId, ChannelId) }
| e | {
Expand Down Expand Up @@ -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
{
Expand Down
2 changes: 1 addition & 1 deletion modules/src/core/ics04_channel/handler/chan_close_confirm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ pub(crate) fn process<HostFunctions: HostFunctionsProvider>(
&channel_end,
&conn,
&expected_channel_end,
&msg.proofs,
&msg.proofs.object_proof(),
)?;

output.log("success: channel close confirm ");
Expand Down
2 changes: 1 addition & 1 deletion modules/src/core/ics04_channel/handler/chan_open_ack.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ pub(crate) fn process<HostFunctions: HostFunctionsProvider>(
&channel_end,
&conn,
&expected_channel_end,
&msg.proofs,
&msg.proofs.object_proof(),
)?;

output.log("success: channel open ack ");
Expand Down
2 changes: 1 addition & 1 deletion modules/src/core/ics04_channel/handler/chan_open_confirm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ pub(crate) fn process<HostFunctions: HostFunctionsProvider>(
&channel_end,
&conn,
&expected_channel_end,
&msg.proofs,
&msg.proofs.object_proof(),
)
.map_err(Error::chan_open_confirm_proof_verification)?;

Expand Down
2 changes: 1 addition & 1 deletion modules/src/core/ics04_channel/handler/chan_open_try.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ pub(crate) fn process<HostFunctions: HostFunctionsProvider>(
&new_channel_end,
&conn,
&expected_channel_end,
&msg.proofs,
&msg.proofs.object_proof(),
)?;

output.log("success: channel open try ");
Expand Down
18 changes: 5 additions & 13 deletions modules/src/core/ics04_channel/handler/timeout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -59,25 +58,18 @@ pub fn process<HostFunctions: HostFunctionsProvider>(

// 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)
.map_err(|_| Error::error_invalid_consensus_state())?;

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) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we check that the timestamp isn't zero before doing this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zero timestamp is represented as None internally and
packet.timed_out function checks that with this self.timeout_timestamp != Timestamp::none()

return Err(Error::packet_timeout_not_reached(
packet.timeout_height,
proof_height,
packet.timeout_timestamp,
proof_timestamp,
));
}
Expand Down
5 changes: 4 additions & 1 deletion modules/src/core/ics04_channel/handler/timeout_on_close.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,10 @@ pub fn process<HostFunctions: HostFunctionsProvider>(
&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) {
Expand Down
7 changes: 4 additions & 3 deletions modules/src/core/ics04_channel/handler/verify.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -19,7 +20,7 @@ pub fn verify_channel_proofs<HostFunctions: HostFunctionsProvider>(
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();
Expand All @@ -32,7 +33,7 @@ pub fn verify_channel_proofs<HostFunctions: HostFunctionsProvider>(
}

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::<HostFunctions>::from_client_type(client_state.client_type());
Expand All @@ -46,7 +47,7 @@ pub fn verify_channel_proofs<HostFunctions: HostFunctionsProvider>(
&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(),
Expand Down
7 changes: 6 additions & 1 deletion modules/src/core/ics04_channel/msgs/timeout_on_close.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,12 @@ impl TryFrom<RawMsgTimeoutOnClose> 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)?
Expand Down
28 changes: 27 additions & 1 deletion modules/src/core/ics04_channel/packet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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<TimeoutVariant> {
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
Expand Down
6 changes: 3 additions & 3 deletions modules/src/timestamp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -271,8 +271,8 @@ mod tests {
assert!(Timestamp::from_nanoseconds(i64::MAX.try_into().unwrap()).is_ok());

assert_eq!(timestamp1.check_expiry(&timestamp2), Expiry::NotExpired);
assert_eq!(timestamp1.check_expiry(&timestamp1), Expiry::NotExpired);
assert_eq!(timestamp2.check_expiry(&timestamp2), Expiry::NotExpired);
assert_eq!(timestamp1.check_expiry(&timestamp1), Expiry::Expired);
assert_eq!(timestamp2.check_expiry(&timestamp2), Expiry::Expired);
assert_eq!(timestamp2.check_expiry(&timestamp1), Expiry::Expired);
assert_eq!(
timestamp1.check_expiry(&nil_timestamp),
Expand Down