From ac7abacc9bb09503d6fd6a396bc0b6850079084e Mon Sep 17 00:00:00 2001 From: noot <36753753+noot@users.noreply.github.com> Date: Thu, 5 Dec 2024 05:26:28 +0900 Subject: [PATCH] ibc: handle height in gRPC request metadata if specified (#4905) ## Describe your changes Check if height is specified in the metadata for relevant IBC gRPC queries and get the respective snapshot if it is provided. If the height is not specified in the metadata, the latest snapshot is used, which is the existing behaviour. Reasoning: since astria uses only gRPC queries for all the IBC queries in the hermes impl, but the gRPC proto messages don't contain the query height, we implemented a fix by putting the height in the metadata. Without this fix, our hermes impl would submit unexpected proofs on occasion, as the proofs would be off-by-one height than that was expected. I opened a PR to update the protos, but the response by the maintainer of ibc-go suggests to me that they intend for having the height in the header be the actual fix (https://github.com/cosmos/ibc-go/pull/7303). ## Issue ticket number and link n/a ## Checklist before requesting a review - [ ] I have added guiding text to explain how a reviewer should test these changes. - [ ] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > only affects the IBC gRPC server, and does not affect existing behaviour --------- Co-authored-by: Richard Janis Goldschmidt --- .../core/component/ibc/src/component/rpc.rs | 1 + .../ibc/src/component/rpc/client_query.rs | 17 +- .../ibc/src/component/rpc/connection_query.rs | 10 +- .../ibc/src/component/rpc/consensus_query.rs | 23 ++- .../component/ibc/src/component/rpc/utils.rs | 160 ++++++++++++++++++ 5 files changed, 205 insertions(+), 6 deletions(-) create mode 100644 crates/core/component/ibc/src/component/rpc/utils.rs diff --git a/crates/core/component/ibc/src/component/rpc.rs b/crates/core/component/ibc/src/component/rpc.rs index 4d125b1350..4b54f52105 100644 --- a/crates/core/component/ibc/src/component/rpc.rs +++ b/crates/core/component/ibc/src/component/rpc.rs @@ -6,6 +6,7 @@ use super::HostInterface; mod client_query; mod connection_query; mod consensus_query; +mod utils; use std::marker::PhantomData; diff --git a/crates/core/component/ibc/src/component/rpc/client_query.rs b/crates/core/component/ibc/src/component/rpc/client_query.rs index 3c7fa28fb3..4933f11090 100644 --- a/crates/core/component/ibc/src/component/rpc/client_query.rs +++ b/crates/core/component/ibc/src/component/rpc/client_query.rs @@ -26,6 +26,7 @@ use crate::component::{ClientStateReadExt, HostInterface}; use crate::prefix::MerklePrefixExt; use crate::IBC_COMMITMENT_PREFIX; +use super::utils::determine_snapshot_from_metadata; use super::IbcQuery; #[async_trait] @@ -34,7 +35,13 @@ impl ClientQuery for IbcQuery { &self, request: tonic::Request, ) -> std::result::Result, Status> { - let snapshot = self.storage.latest_snapshot(); + let snapshot = match determine_snapshot_from_metadata(self.storage.clone(), request.metadata()) { + Err(err) => return Err(tonic::Status::aborted( + format!("could not determine the correct snapshot to open given the `\"height\"` header of the request: {err:#}") + )), + Ok(snapshot) => snapshot, + }; + let client_id = ClientId::from_str(&request.get_ref().client_id) .map_err(|e| tonic::Status::invalid_argument(format!("invalid client id: {e}")))?; @@ -111,7 +118,13 @@ impl ClientQuery for IbcQuery { &self, request: tonic::Request, ) -> std::result::Result, tonic::Status> { - let snapshot = self.storage.latest_snapshot(); + let snapshot = match determine_snapshot_from_metadata(self.storage.clone(), request.metadata()) { + Err(err) => return Err(tonic::Status::aborted( + format!("could not determine the correct snapshot to open given the `\"height\"` header of the request: {err:#}") + )), + Ok(snapshot) => snapshot, + }; + let client_id = ClientId::from_str(&request.get_ref().client_id) .map_err(|e| tonic::Status::invalid_argument(format!("invalid client id: {e}")))?; let height = if request.get_ref().latest_height { diff --git a/crates/core/component/ibc/src/component/rpc/connection_query.rs b/crates/core/component/ibc/src/component/rpc/connection_query.rs index 70ed5dbd2e..60614405fa 100644 --- a/crates/core/component/ibc/src/component/rpc/connection_query.rs +++ b/crates/core/component/ibc/src/component/rpc/connection_query.rs @@ -19,6 +19,7 @@ use ibc_types::DomainType; use prost::Message; use std::str::FromStr; +use crate::component::rpc::utils::determine_snapshot_from_metadata; use crate::component::{ConnectionStateReadExt, HostInterface}; use crate::prefix::MerklePrefixExt; use crate::IBC_COMMITMENT_PREFIX; @@ -33,7 +34,14 @@ impl ConnectionQuery for IbcQuery request: tonic::Request, ) -> std::result::Result, tonic::Status> { tracing::debug!("querying connection {:?}", request); - let snapshot = self.storage.latest_snapshot(); + + let snapshot = match determine_snapshot_from_metadata(self.storage.clone(), request.metadata()) { + Err(err) => return Err(tonic::Status::aborted( + format!("could not determine the correct snapshot to open given the `\"height\"` header of the request: {err:#}") + )), + Ok(snapshot) => snapshot, + }; + let connection_id = &ConnectionId::from_str(&request.get_ref().connection_id) .map_err(|e| tonic::Status::aborted(format!("invalid connection id: {e}")))?; diff --git a/crates/core/component/ibc/src/component/rpc/consensus_query.rs b/crates/core/component/ibc/src/component/rpc/consensus_query.rs index 4d67297416..4202dd1540 100644 --- a/crates/core/component/ibc/src/component/rpc/consensus_query.rs +++ b/crates/core/component/ibc/src/component/rpc/consensus_query.rs @@ -31,6 +31,7 @@ use std::str::FromStr; use crate::component::{ChannelStateReadExt, ConnectionStateReadExt, HostInterface}; +use super::utils::determine_snapshot_from_metadata; use super::IbcQuery; #[async_trait] @@ -364,7 +365,12 @@ impl ConsensusQuery for IbcQuery &self, request: tonic::Request, ) -> std::result::Result, tonic::Status> { - let snapshot = self.storage.latest_snapshot(); + let snapshot = match determine_snapshot_from_metadata(self.storage.clone(), request.metadata()) { + Err(err) => return Err(tonic::Status::aborted( + format!("could not determine the correct snapshot to open given the `\"height\"` header of the request: {err:#}") + )), + Ok(snapshot) => snapshot, + }; let port_id = PortId::from_str(&request.get_ref().port_id) .map_err(|e| tonic::Status::aborted(format!("invalid port id: {e}")))?; @@ -473,7 +479,12 @@ impl ConsensusQuery for IbcQuery &self, request: tonic::Request, ) -> std::result::Result, tonic::Status> { - let snapshot = self.storage.latest_snapshot(); + let snapshot = match determine_snapshot_from_metadata(self.storage.clone(), request.metadata()) { + Err(err) => return Err(tonic::Status::aborted( + format!("could not determine the correct snapshot to open given the `\"height\"` header of the request: {err:#}") + )), + Ok(snapshot) => snapshot, + }; let port_id = PortId::from_str(&request.get_ref().port_id) .map_err(|e| tonic::Status::aborted(format!("invalid port id: {e}")))?; @@ -513,7 +524,13 @@ impl ConsensusQuery for IbcQuery request: tonic::Request, ) -> std::result::Result, tonic::Status> { - let snapshot = self.storage.latest_snapshot(); + let snapshot = match determine_snapshot_from_metadata(self.storage.clone(), request.metadata()) { + Err(err) => return Err(tonic::Status::aborted( + format!("could not determine the correct snapshot to open given the `\"height\"` header of the request: {err:#}") + )), + Ok(snapshot) => snapshot, + }; + let channel_id = ChannelId::from_str(request.get_ref().channel_id.as_str()) .map_err(|e| tonic::Status::aborted(format!("invalid channel id: {e}")))?; let port_id = PortId::from_str(request.get_ref().port_id.as_str()) diff --git a/crates/core/component/ibc/src/component/rpc/utils.rs b/crates/core/component/ibc/src/component/rpc/utils.rs new file mode 100644 index 0000000000..642d4bcff2 --- /dev/null +++ b/crates/core/component/ibc/src/component/rpc/utils.rs @@ -0,0 +1,160 @@ +use std::str::FromStr; + +use anyhow::bail; +use anyhow::Context as _; +use cnidarium::Snapshot; +use cnidarium::Storage; +use ibc_proto::ibc::core::client::v1::Height; +use tracing::debug; +use tracing::instrument; + +type Type = tonic::metadata::MetadataMap; + +/// Determines which state snapshot to open given the height header in a [`MetadataMap`]. +/// +/// Returns the latest snapshot if the height header is 0, 0-0, or absent. +#[instrument(skip_all, level = "debug")] +pub(in crate::component::rpc) fn determine_snapshot_from_metadata( + storage: Storage, + metadata: &Type, +) -> anyhow::Result { + let height = determine_height_from_metadata(metadata) + .context("failed to determine height from metadata")?; + if height.revision_height == 0 { + Ok(storage.latest_snapshot()) + } else { + storage + .snapshot(height.revision_height) + .context("failed to create state snapshot from IBC height in height header") + } +} + +#[instrument(skip_all, level = "debug")] +fn determine_height_from_metadata( + metadata: &tonic::metadata::MetadataMap, +) -> anyhow::Result { + match metadata.get("height") { + None => { + debug!("height header was missing; assuming a height of 0"); + Ok(TheHeight::zero().into_inner()) + } + Some(entry) => entry + .to_str() + .context("height header was present but its entry was not ASCII") + .and_then(parse_as_ibc_height) + .context("failed to parse height header as IBC height"), + } +} + +/// Newtype wrapper around [`Height`] to implement [`FromStr`]. +#[derive(Debug)] +struct TheHeight(Height); + +impl TheHeight { + fn zero() -> Self { + Self(Height { + revision_number: 0, + revision_height: 0, + }) + } + fn into_inner(self) -> Height { + self.0 + } +} + +impl FromStr for TheHeight { + type Err = anyhow::Error; + + fn from_str(input: &str) -> Result { + const FORM: &str = "input was not of the form '0' or '-'"; + + let mut parts = input.split('-'); + + let revision_number = parts + .next() + .context(FORM)? + .parse::() + .context("failed to parse revision number as u64")?; + let revision_height = match parts.next() { + None if revision_number == 0 => return Ok(Self::zero()), + None => bail!(FORM), + Some(rev_height) => rev_height + .parse::() + .context("failed to parse revision height as u64")?, + }; + + Ok(TheHeight(Height { + revision_number, + revision_height, + })) + } +} + +fn parse_as_ibc_height(input: &str) -> anyhow::Result { + let height = input + .trim() + .parse::() + .context("failed to parse as IBC height")? + .into_inner(); + + Ok(height) +} + +#[cfg(test)] +mod tests { + use ibc_proto::ibc::core::client::v1::Height; + use tonic::metadata::MetadataMap; + + use crate::component::rpc::utils::determine_height_from_metadata; + + use super::TheHeight; + + fn zero() -> Height { + Height { + revision_number: 0, + revision_height: 0, + } + } + + fn height(revision_number: u64, revision_height: u64) -> Height { + Height { + revision_number, + revision_height, + } + } + + #[track_caller] + fn assert_ibc_height_is_parsed_correctly(input: &str, expected: Height) { + let actual = input.parse::().unwrap().into_inner(); + assert_eq!(expected, actual); + } + + #[test] + fn parse_ibc_height() { + assert_ibc_height_is_parsed_correctly("0", zero()); + assert_ibc_height_is_parsed_correctly("0-0", zero()); + assert_ibc_height_is_parsed_correctly("0-1", height(0, 1)); + assert_ibc_height_is_parsed_correctly("1-0", height(1, 0)); + assert_ibc_height_is_parsed_correctly("1-1", height(1, 1)); + } + + #[track_caller] + fn assert_ibc_height_is_determined_correctly(input: Option<&str>, expected: Height) { + let mut metadata = MetadataMap::new(); + if let Some(input) = input { + metadata.insert("height", input.parse().unwrap()); + } + let actual = determine_height_from_metadata(&metadata).unwrap(); + assert_eq!(expected, actual); + } + + #[test] + fn determine_ibc_height_from_metadata() { + assert_ibc_height_is_determined_correctly(None, zero()); + assert_ibc_height_is_determined_correctly(Some("0"), zero()); + assert_ibc_height_is_determined_correctly(Some("0-0"), zero()); + assert_ibc_height_is_determined_correctly(Some("0-1"), height(0, 1)); + assert_ibc_height_is_determined_correctly(Some("1-0"), height(1, 0)); + assert_ibc_height_is_determined_correctly(Some("1-1"), height(1, 1)); + } +}