From a91c4f24d64084a8e55371a667996c6fd778003a Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Sat, 18 Nov 2023 17:06:07 +0100 Subject: [PATCH] solana-ibc: map IBC client ids into u32 and refactor client state storage (#98) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IBC client ids have ‘-’ format where counter is a global sequential number. Take advantage of that by converting client ids into u32 including just the counter. Since we’re effectively ignoring the client-type part of the id, keep counter → client id map in private storage and use it to verify that id we were given is the one we know about. Furthermore, rather than storing client information in a map keep it in a vector which is more compact and faster to index. At the same time, keep just a single vector for all per-client data rather than having separate maps for each piece of information. Issue: https://github.com/ComposableFi/emulated-light-client/issues/35 --- .../programs/solana-ibc/src/client_state.rs | 19 +- .../solana-ibc/src/execution_context.rs | 97 +++++----- .../programs/solana-ibc/src/storage.rs | 169 ++++++++++++++++-- .../programs/solana-ibc/src/trie_key.rs | 55 +++--- .../solana-ibc/src/validation_context.rs | 41 ++--- 5 files changed, 253 insertions(+), 128 deletions(-) diff --git a/solana/solana-ibc/programs/solana-ibc/src/client_state.rs b/solana/solana-ibc/programs/solana-ibc/src/client_state.rs index 7464fe48..a9a6f57f 100644 --- a/solana/solana-ibc/programs/solana-ibc/src/client_state.rs +++ b/solana/solana-ibc/programs/solana-ibc/src/client_state.rs @@ -458,17 +458,15 @@ impl ibc::clients::ics07_tendermint::CommonContext for IbcStorage<'_, '_> { &self, client_id: &ClientId, ) -> Result, ContextError> { - let low = (client_id.to_string(), Height::min(0)); - let high = - (client_id.to_string(), Height::new(u64::MAX, u64::MAX).unwrap()); - let heights = self + Ok(self .borrow() .private + .client(client_id)? + .1 .consensus_states - .range(low..=high) - .map(|((_client, height), _value)| *height) - .collect(); - Ok(heights) + .keys() + .copied() + .collect()) } fn host_timestamp(&self) -> Result { @@ -534,7 +532,7 @@ impl IbcStorage<'_, '_> { ) -> Result, ContextError> { use core::ops::Bound; - let pivot = Bound::Excluded((client_id.to_string(), *height)); + let pivot = Bound::Excluded(*height); let range = if dir == Direction::Next { (pivot, Bound::Unbounded) } else { @@ -542,7 +540,8 @@ impl IbcStorage<'_, '_> { }; let store = self.borrow(); - let mut range = store.private.consensus_states.range(range); + let (_index, client) = store.private.client(client_id)?; + let mut range = client.consensus_states.range(range); if dir == Direction::Next { range.next() } else { range.next_back() } .map(|(_, data)| data.get()) .transpose() diff --git a/solana/solana-ibc/programs/solana-ibc/src/execution_context.rs b/solana/solana-ibc/programs/solana-ibc/src/execution_context.rs index d6e60868..279d7d86 100644 --- a/solana/solana-ibc/programs/solana-ibc/src/execution_context.rs +++ b/solana/solana-ibc/programs/solana-ibc/src/execution_context.rs @@ -40,12 +40,11 @@ impl ClientExecutionContext for IbcStorage<'_, '_> { state: Self::AnyClientState, ) -> Result { msg!("store_client_state({}, {:?})", path, state); - self.borrow_mut().store_serialised_proof( - |private| &mut private.clients, - path.0.to_string(), - &TrieKey::from(&path), - &state, - ) + let mut store = self.borrow_mut(); + let (client_idx, client) = store.private.client_mut(&path.0, true)?; + let hash = client.client_state.set(&state)?.digest(); + let key = TrieKey::for_client_state(client_idx); + store.provable.set(&key, &hash).map_err(error) } fn store_consensus_state( @@ -55,12 +54,15 @@ impl ClientExecutionContext for IbcStorage<'_, '_> { ) -> Result { msg!("store_consensus_state({}, {:?})", path, state); let height = Height::new(path.epoch, path.height)?; - self.borrow_mut().store_serialised_proof( - |private| &mut private.consensus_states, - (path.client_id.to_string(), height), - &TrieKey::from(&path), - &state, - ) + let mut store = self.borrow_mut(); + let (client_idx, client) = + store.private.client_mut(&path.client_id, false)?; + let serialised = storage::Serialised::new(&state)?; + let hash = serialised.digest(); + client.consensus_states.insert(height, serialised); + let trie_key = TrieKey::for_consensus_state(client_idx, height); + store.provable.set(&trie_key, &hash).map_err(error)?; + Ok(()) } fn delete_consensus_state( @@ -69,10 +71,12 @@ impl ClientExecutionContext for IbcStorage<'_, '_> { ) -> Result<(), ContextError> { msg!("delete_consensus_state({})", path); let height = Height::new(path.epoch, path.height)?; - let key = (path.client_id.to_string(), height); let mut store = self.borrow_mut(); - store.private.consensus_states.remove(&key); - store.provable.del(&TrieKey::from(&path)).unwrap(); + let (client_idx, client) = + store.private.client_mut(&path.client_id, false)?; + client.consensus_states.remove(&height); + let key = TrieKey::for_consensus_state(client_idx, height); + store.provable.del(&key).map_err(error)?; Ok(()) } @@ -84,9 +88,10 @@ impl ClientExecutionContext for IbcStorage<'_, '_> { ) -> Result<(), ContextError> { self.borrow_mut() .private - .client_processed_heights - .get_mut(client_id.as_str()) - .and_then(|processed_times| processed_times.remove(&height)); + .client_mut(&client_id, false)? + .1 + .processed_heights + .remove(&height); Ok(()) } @@ -97,9 +102,10 @@ impl ClientExecutionContext for IbcStorage<'_, '_> { ) -> Result<(), ContextError> { self.borrow_mut() .private - .client_processed_times - .get_mut(client_id.as_str()) - .and_then(|processed_times| processed_times.remove(&height)); + .client_mut(&client_id, false)? + .1 + .processed_times + .remove(&height); Ok(()) } @@ -112,9 +118,9 @@ impl ClientExecutionContext for IbcStorage<'_, '_> { msg!("store_update_time({}, {}, {})", client_id, height, timestamp); self.borrow_mut() .private - .client_processed_times - .entry(client_id.to_string()) - .or_default() + .client_mut(&client_id, false)? + .1 + .processed_times .insert(height, timestamp.nanoseconds()); Ok(()) } @@ -128,25 +134,16 @@ impl ClientExecutionContext for IbcStorage<'_, '_> { msg!("store_update_height({}, {}, {})", client_id, height, host_height); self.borrow_mut() .private - .client_processed_heights - .entry(client_id.to_string()) - .or_default() + .client_mut(&client_id, false)? + .1 + .processed_heights .insert(height, host_height); Ok(()) } } impl ExecutionContext for IbcStorage<'_, '_> { - fn increase_client_counter(&mut self) -> Result { - let mut store = self.borrow_mut(); - store.private.client_counter = - store.private.client_counter.checked_add(1).unwrap(); - msg!( - "client_counter has increased to: {}", - store.private.client_counter - ); - Ok(()) - } + fn increase_client_counter(&mut self) -> Result { Ok(()) } fn store_connection( &mut self, @@ -164,19 +161,12 @@ impl ExecutionContext for IbcStorage<'_, '_> { fn store_connection_to_client( &mut self, - client_connection_path: &ClientConnectionPath, + path: &ClientConnectionPath, conn_id: ConnectionId, ) -> Result { - msg!( - "store_connection_to_client: path: {}, connection_id: {:?}", - client_connection_path, - conn_id - ); - let mut store = self.borrow_mut(); - store - .private - .client_to_connection - .insert(client_connection_path.0.to_string(), conn_id.to_string()); + msg!("store_connection_to_client({}, {:?})", path, conn_id); + self.borrow_mut().private.client_mut(&path.0, false)?.1.connection_id = + Some(conn_id); Ok(()) } @@ -335,8 +325,7 @@ impl ExecutionContext for IbcStorage<'_, '_> { } fn emit_ibc_event(&mut self, event: IbcEvent) -> Result { - crate::events::emit(event) - .map_err(|description| ClientError::Other { description }.into()) + crate::events::emit(event).map_err(error) } fn log_message(&mut self, message: String) -> Result { @@ -383,9 +372,7 @@ impl storage::IbcStorageInner<'_, '_> { value: &V, ) -> Result { let serialised = storage::Serialised::new(value)?; - self.provable - .set(trie_key, &serialised.digest()) - .map_err(|e| ClientError::Other { description: e.to_string() })?; + self.provable.set(trie_key, &serialised.digest()).map_err(error)?; get_map(self.private).insert(key, serialised); Ok(()) } @@ -425,3 +412,7 @@ fn delete_packet_sequence( } } } + +fn error(description: impl ToString) -> ContextError { + ClientError::Other { description: description.to_string() }.into() +} diff --git a/solana/solana-ibc/programs/solana-ibc/src/storage.rs b/solana/solana-ibc/programs/solana-ibc/src/storage.rs index 577ec759..83d4b811 100644 --- a/solana/solana-ibc/programs/solana-ibc/src/storage.rs +++ b/solana/solana-ibc/programs/solana-ibc/src/storage.rs @@ -6,23 +6,22 @@ use anchor_lang::prelude::*; use borsh::maybestd::io; use lib::hash::CryptoHash; +type Result = core::result::Result; + use crate::client_state::AnyClientState; use crate::consensus_state::AnyConsensusState; mod ibc { pub use ibc::core::ics02_client::error::ClientError; - pub use ibc::core::ics02_client::height::Height; pub use ibc::core::ics03_connection::connection::ConnectionEnd; pub use ibc::core::ics04_channel::channel::ChannelEnd; pub use ibc::core::ics04_channel::msgs::PacketMsg; pub use ibc::core::ics04_channel::packet::Sequence; + pub use ibc::core::ics24_host::identifier::{ClientId, ConnectionId}; + pub use ibc::Height; } -type Result = core::result::Result; - -pub(crate) type HostHeight = ibc::Height; pub(crate) type SolanaTimestamp = u64; -pub(crate) type InnerClientId = String; pub(crate) type InnerConnectionId = String; pub(crate) type InnerPortId = String; pub(crate) type InnerChannelId = String; @@ -80,6 +79,72 @@ impl SequenceTriple { } } +/// An index used as unique identifier for a client. +/// +/// IBC client id uses `-` format. This index is +/// constructed from a client id by stripping the client type. Since counter is +/// unique within an IBC module, the index is enough to identify a known client. +/// +/// To avoid confusing identifiers with the same counter but different client +/// type (which may be crafted by an attacker), we always check that client type +/// matches one we know. Because of this check, to get `ClientIdx` +/// [`PrivateStorage::client`] needs to be used. +/// +/// The index is guaranteed to fit `u32` and `usize`. +#[derive(Clone, Copy, PartialEq, Eq, derive_more::From, derive_more::Into)] +pub struct ClientIdx(u32); + +impl From for usize { + #[inline] + fn from(index: ClientIdx) -> usize { index.0 as usize } +} + +impl core::str::FromStr for ClientIdx { + type Err = core::num::ParseIntError; + + #[inline] + fn from_str(value: &str) -> Result { + if core::mem::size_of::() < 4 { + usize::from_str(value).map(|index| Self(index as u32)) + } else { + u32::from_str(value).map(Self) + } + } +} + +impl PartialEq for ClientIdx { + #[inline] + fn eq(&self, rhs: &usize) -> bool { + u32::try_from(*rhs).ok().filter(|rhs| self.0 == *rhs).is_some() + } +} + + +/// Per-client private storage. +#[derive(Clone, Debug, borsh::BorshSerialize, borsh::BorshDeserialize)] +pub(crate) struct ClientStore { + pub client_id: ibc::ClientId, + pub connection_id: Option, + + pub client_state: Serialised, + pub consensus_states: BTreeMap>, + pub processed_times: BTreeMap, + pub processed_heights: BTreeMap, +} + +impl ClientStore { + fn new(client_id: ibc::ClientId) -> Self { + Self { + client_id, + connection_id: Default::default(), + client_state: Serialised::empty(), + consensus_states: Default::default(), + processed_times: Default::default(), + processed_heights: Default::default(), + } + } +} + #[account] #[derive(Debug)] pub struct IbcPackets(pub Vec); @@ -89,21 +154,13 @@ pub struct IbcPackets(pub Vec); /// All the structs from IBC are stored as String since they dont implement /// AnchorSerialize and AnchorDeserialize pub(crate) struct PrivateStorage { - pub clients: BTreeMap>, - pub client_counter: u64, - pub client_processed_times: - BTreeMap>, - pub client_processed_heights: - BTreeMap>, - pub consensus_states: - BTreeMap<(InnerClientId, ibc::Height), Serialised>, + clients: Vec, + pub connection_counter: u64, pub connections: BTreeMap>, pub channel_ends: BTreeMap<(InnerPortId, InnerChannelId), Serialised>, - // Contains the client id corresponding to the connectionId - pub client_to_connection: BTreeMap, pub channel_counter: u64, /// The sequence numbers of the packet commitments. @@ -121,6 +178,81 @@ pub(crate) struct PrivateStorage { pub next_sequence: BTreeMap<(InnerPortId, InnerChannelId), SequenceTriple>, } +impl PrivateStorage { + /// Returns number of known clients; or counter for the next client. + pub fn client_counter(&self) -> u64 { + u64::try_from(self.clients.len()).unwrap() + } + + /// Returns state for an existing client. + /// + /// Client ids use `-` format where is + /// sequential. We take advantage of that by extracting the and + /// using it as index in client states. + pub fn client( + &self, + client_id: &ibc::ClientId, + ) -> Result<(ClientIdx, &ClientStore), ibc::ClientError> { + self.client_index(client_id) + .and_then(|idx| { + self.clients + .get(usize::from(idx)) + .filter(|state| state.client_id == *client_id) + .map(|state| (idx, state)) + }) + .ok_or_else(|| ibc::ClientError::ClientStateNotFound { + client_id: client_id.clone(), + }) + } + + /// Returns state for an existing client. + /// + /// Client ids use `-` format where is + /// sequential. We take advantage of that by extracting the and + /// using it as index in client states. + /// + /// If `create` argument is true, creates a new client if the index equals + /// current count of clients (that is if the index is the next available + /// index). + pub fn client_mut( + &mut self, + client_id: &ibc::ClientId, + create: bool, + ) -> Result<(ClientIdx, &mut ClientStore), ibc::ClientError> { + self.client_mut_impl(client_id, create).ok_or_else(|| { + ibc::ClientError::ClientStateNotFound { + client_id: client_id.clone(), + } + }) + } + + fn client_mut_impl( + &mut self, + client_id: &ibc::ClientId, + create: bool, + ) -> Option<(ClientIdx, &mut ClientStore)> { + use core::cmp::Ordering; + + let idx = self.client_index(client_id)?; + let pos = usize::from(idx); + match pos.cmp(&self.clients.len()) { + Ordering::Less => Some((idx, &mut self.clients[pos])), + Ordering::Equal if create => { + self.clients.push(ClientStore::new(client_id.clone())); + self.clients.last_mut().map(|client| (idx, client)) + } + _ => None, + } + } + + fn client_index(&self, client_id: &ibc::ClientId) -> Option { + client_id + .as_str() + .rsplit_once('-') + .and_then(|(_, index)| core::str::FromStr::from_str(index).ok()) + } +} + /// Provable storage, i.e. the trie, held in an account. pub type AccountTrie<'a, 'b> = solana_trie::AccountTrie>; @@ -220,6 +352,8 @@ impl<'a, 'b> IbcStorage<'a, 'b> { pub(crate) struct Serialised(Vec, core::marker::PhantomData); impl Serialised { + pub fn empty() -> Self { Self(Vec::new(), core::marker::PhantomData) } + pub fn digest(&self) -> CryptoHash { CryptoHash::digest(self.0.as_slice()) } fn make_err(err: io::Error) -> ibc::ClientError { @@ -233,6 +367,11 @@ impl Serialised { .map(|data| Self(data, core::marker::PhantomData)) .map_err(Self::make_err) } + + pub fn set(&mut self, value: &T) -> Result<&mut Self, ibc::ClientError> { + *self = Self::new(value)?; + Ok(self) + } } impl Serialised { diff --git a/solana/solana-ibc/programs/solana-ibc/src/trie_key.rs b/solana/solana-ibc/programs/solana-ibc/src/trie_key.rs index 45132014..a2999b84 100644 --- a/solana/solana-ibc/programs/solana-ibc/src/trie_key.rs +++ b/solana/solana-ibc/programs/solana-ibc/src/trie_key.rs @@ -1,15 +1,15 @@ use ibc::core::ics04_channel::packet::Sequence; use ibc::core::ics24_host::identifier::{ChannelId, PortId}; use ibc::core::ics24_host::path::{ - AckPath, ChannelEndPath, ClientConsensusStatePath, ClientStatePath, - CommitmentPath, ConnectionPath, ReceiptPath, SeqAckPath, SeqRecvPath, - SeqSendPath, + AckPath, ChannelEndPath, CommitmentPath, ConnectionPath, ReceiptPath, + SeqAckPath, SeqRecvPath, SeqSendPath, }; // Note: We’re not using ChannelId::prefix() and ConnectionId::prefix() because // those return the prefix without trailing `-` and we want constants which also // include that hyphen. use super::{CHANNEL_ID_PREFIX, CONNECTION_ID_PREFIX}; +use crate::storage::ClientIdx; /// A key used for indexing entries in the provable storage. /// @@ -19,8 +19,8 @@ use super::{CHANNEL_ID_PREFIX, CONNECTION_ID_PREFIX}; /// /// ```ignore /// enum TrieKey { -/// ClientState { client_id: String }, -/// ConsensusState { client_id: String, epoch: u64, height: u64 }, +/// ClientState { client_id: u32 }, +/// ConsensusState { client_id: u32, epoch: u64, height: u64 }, /// Connection { connection_id: u32 }, /// ChannelEnd { port_id: String, channel_id: u32 }, /// NextSequenceSend { port_id: String, channel_id: u32 }, @@ -62,6 +62,18 @@ macro_rules! new_key_impl { } impl TrieKey { + /// Constructs a new key for a client state path for client with given + /// counter. + pub fn for_client_state(client: ClientIdx) -> Self { + new_key_impl!(Tag::ClientState, client) + } + + /// Constructs a new key for a consensus state path for client with given + /// counter and specified height. + pub fn for_consensus_state(client: ClientIdx, height: ibc::Height) -> Self { + new_key_impl!(Tag::ConsensusState, client, height) + } + /// Constructs a new key for a `(port_id, channel_id)` path. /// /// Panics if `channel_id` is invalid. @@ -91,23 +103,6 @@ impl core::ops::Deref for TrieKey { fn deref(&self) -> &[u8] { self.0.as_slice() } } -impl From<&ClientStatePath> for TrieKey { - fn from(path: &ClientStatePath) -> Self { - new_key_impl!(Tag::ClientState, path.0) - } -} - -impl From<&ClientConsensusStatePath> for TrieKey { - fn from(path: &ClientConsensusStatePath) -> Self { - new_key_impl!( - Tag::ConsensusState, - path.client_id, - path.epoch, - path.height - ) - } -} - impl From<&ConnectionPath> for TrieKey { fn from(path: &ConnectionPath) -> Self { new_key_impl!(Tag::Connection, path.0) @@ -206,12 +201,10 @@ trait AsComponent { fn append_into(&self, dest: &mut Vec); } -// TODO(#35): Investigate weather we can impose restrictions on client -// identifiers, e.g. `client-`. -impl AsComponent for ibc::core::ics24_host::identifier::ClientId { - fn key_len(&self) -> usize { self.as_str().key_len() } +impl AsComponent for ClientIdx { + fn key_len(&self) -> usize { 0_u32.key_len() } fn append_into(&self, dest: &mut Vec) { - self.as_str().append_into(dest) + u32::from(*self).append_into(dest) } } @@ -238,6 +231,14 @@ impl AsComponent for ibc::core::ics24_host::identifier::ChannelId { } } +impl AsComponent for ibc::Height { + fn key_len(&self) -> usize { 2 * 0_u64.key_len() } + fn append_into(&self, dest: &mut Vec) { + self.revision_number().append_into(dest); + self.revision_height().append_into(dest); + } +} + impl AsComponent for ibc::core::ics04_channel::packet::Sequence { fn key_len(&self) -> usize { 0_u64.key_len() } fn append_into(&self, dest: &mut Vec) { diff --git a/solana/solana-ibc/programs/solana-ibc/src/validation_context.rs b/solana/solana-ibc/programs/solana-ibc/src/validation_context.rs index 0dc85ceb..62f5bf59 100644 --- a/solana/solana-ibc/programs/solana-ibc/src/validation_context.rs +++ b/solana/solana-ibc/programs/solana-ibc/src/validation_context.rs @@ -39,15 +39,7 @@ impl ValidationContext for IbcStorage<'_, '_> { &self, client_id: &ClientId, ) -> Result { - self.borrow() - .private - .clients - .get(client_id.as_str()) - .ok_or_else(|| ClientError::ClientStateNotFound { - client_id: client_id.clone(), - })? - .get() - .map_err(Into::into) + Ok(self.borrow().private.client(client_id)?.1.client_state.get()?) } fn decode_client_state( @@ -64,14 +56,17 @@ impl ValidationContext for IbcStorage<'_, '_> { let height = Height::new(path.epoch, path.height)?; self.borrow() .private + .client(&path.client_id)? + .1 .consensus_states - .get(&(path.client_id.to_string(), height)) + .get(&height) + .cloned() .ok_or_else(|| ClientError::ConsensusStateNotFound { client_id: path.client_id.clone(), height, - })? - .get() - .map_err(Into::into) + }) + .and_then(|data| data.get()) + .map_err(ibc::core::ContextError::from) } fn host_height(&self) -> Result { @@ -94,8 +89,7 @@ impl ValidationContext for IbcStorage<'_, '_> { } fn client_counter(&self) -> Result { - let store = self.borrow(); - Ok(store.private.client_counter) + Ok(self.borrow().private.client_counter()) } fn connection_end(&self, conn_id: &ConnectionId) -> Result { @@ -284,9 +278,10 @@ impl ibc::core::ics02_client::ClientValidationContext for IbcStorage<'_, '_> { let store = self.borrow(); store .private - .client_processed_times - .get(client_id.as_str()) - .and_then(|processed_times| processed_times.get(height)) + .client(client_id)? + .1 + .processed_times + .get(height) .map(|ts| Timestamp::from_nanoseconds(*ts).unwrap()) .ok_or_else(|| { ContextError::ClientError(ClientError::Other { @@ -304,12 +299,12 @@ impl ibc::core::ics02_client::ClientValidationContext for IbcStorage<'_, '_> { client_id: &ClientId, height: &Height, ) -> Result { - let store = self.borrow(); - store + self.borrow() .private - .client_processed_heights - .get(client_id.as_str()) - .and_then(|processed_heights| processed_heights.get(height)) + .client(client_id)? + .1 + .processed_heights + .get(height) .copied() .ok_or_else(|| { ContextError::ClientError(ClientError::Other {