solana-ibc: restrict port identifier and use more compact representation

solana/solana-ibc/programs/solana-ibc/src/storage/ids.rs

@@ -1,4 +1,7 @@
+use core::str::FromStr;
+
 use anchor_lang::prelude::borsh;
+use base64::engine::{general_purpose, Engine};
 
 use super::ibc;
 
@@ -125,7 +128,7 @@ impl TryFrom<&ibc::ConnectionId> for ConnectionIdx {
     borsh::BorshDeserialize,
 )]
 pub struct PortChannelPK {
-    pub(super) port_id: ibc::PortId,
+    pub(super) port_key: PortKey,
     pub(super) channel_idx: u32,
 }
 
@@ -141,16 +144,28 @@ impl PortChannelPK {
         port_id: impl MaybeOwned<ibc::PortId>,
         channel_id: impl MaybeOwned<ibc::ChannelId>,
     ) -> Result<Self, ibc::ChannelError> {
-        let channel_str = channel_id.as_ref().as_str();
-        match parse_sans_prefix(Self::CHANNEL_IBC_PREFIX, channel_str) {
-            Some(channel_idx) => {
-                Ok(Self { port_id: port_id.into_owned(), channel_idx })
-            }
-            None => Err(ibc::ChannelError::ChannelNotFound {
-                port_id: port_id.into_owned(),
-                channel_id: channel_id.into_owned(),
-            }),
-        }
+        (|| {
+            let channel = channel_id.as_ref().as_str();
+            Some(Self {
+                port_key: PortKey::try_from(port_id.as_ref()).ok()?,
+                channel_idx: parse_sans_prefix(
+                    Self::CHANNEL_IBC_PREFIX,
+                    channel,
+                )?,
+            })
+        })()
+        .ok_or_else(|| ibc::ChannelError::ChannelNotFound {
+            port_id: port_id.into_owned(),
+            channel_id: channel_id.into_owned(),
+        })
+    }
+
+    #[allow(dead_code)]
+    pub fn port_id(&self) -> ibc::PortId { ibc::PortId::from(&self.port_key) }
+
+    #[allow(dead_code)]
+    pub fn channel_id(&self) -> ibc::ChannelId {
+        ibc::ChannelId::new(self.channel_idx.into())
     }
 }
 
@@ -170,6 +185,113 @@ impl<T> MaybeOwned<T> for T {
 }
 
 
+/// An internal port identifier.
+///
+/// We’re restricting valid port identifiers to be at most 12 alphanumeric
+/// characters.
+///
+/// We pad the id with slash characters (which are invalid in IBC identifiers)
+/// and then parse them using base64 to get a 9-byte buffer which represents the
+/// identifier.
+#[derive(
+    Clone,
+    PartialEq,
+    Eq,
+    PartialOrd,
+    Ord,
+    Hash,
+    borsh::BorshSerialize,
+    // TODO(mina86): Verify value is valid when deserialising.  There are bit
+    // patterns which don’t correspond to valid port keys.
+    borsh::BorshDeserialize,
+)]
+pub struct PortKey([u8; 9]);
+
+impl PortKey {
+    #[inline]
+    pub fn as_bytes(&self) -> &[u8; 9] { &self.0 }
+
+    fn write_into<'a>(&self, buf: &'a mut [u8; 12]) -> &'a str {
+        let mut len = general_purpose::STANDARD
+            .encode_slice(self.as_bytes(), &mut buf[..])
+            .unwrap();
+        debug_assert_eq!(buf.len(), len);
+
+        while len > 0 && buf[len - 1] == b'/' {
+            len -= 1;
+        }
+
+        // SAFETY: base64 outputs ASCII characters.
+        unsafe { core::str::from_utf8_unchecked(&buf[..len]) }
+    }
+}
+
+impl TryFrom<ibc::PortId> for PortKey {
+    type Error = ();
+    fn try_from(port_id: ibc::PortId) -> Result<Self, Self::Error> {
+        Self::try_from(&port_id)
+    }
+}
+
+impl TryFrom<&ibc::PortId> for PortKey {
+    type Error = ();
+
+    fn try_from(port_id: &ibc::PortId) -> Result<Self, Self::Error> {
+        let port_id = port_id.as_bytes();
+        // We allow alphanumeric characters only in the port id.  We need to
+        // filter out pluses and slashes since those are valid base64 characters
+        // and base64 decoder won’t error out on those.
+        //
+        // We technically shouldn’t need to check for slashes since IBC should
+        // guarantee that the identifier has no slash.  However, just to make
+        // sure also filter slashes out.
+        if port_id.iter().any(|byte| *byte == b'+' || *byte == b'/') {
+            return Err(());
+        }
+
+        // Pad the identifier with slashes.  Observe that slash is a valid
+        // base64 character so we can treat the entire 12-character long string
+        // as base64-encoded value.
+        let mut buf = [b'/'; 12];
+        buf.get_mut(..port_id.len()).ok_or(())?.copy_from_slice(port_id);
+
+        // Decode into 9-byte buffer.
+        let mut this = Self([0; 9]);
+        let len = general_purpose::STANDARD
+            .decode_slice_unchecked(&buf[..], &mut this.0[..])
+            .map_err(|_| ())?;
+        debug_assert_eq!(this.0.len(), len);
+
+        Ok(this)
+    }
+}
+
+impl From<PortKey> for ibc::PortId {
+    fn from(port_key: PortKey) -> Self { Self::from(&port_key) }
+}
+
+impl From<&PortKey> for ibc::PortId {
+    fn from(port_key: &PortKey) -> Self {
+        let mut buf = [0; 12];
+        Self::from_str(port_key.write_into(&mut buf)).unwrap()
+    }
+}
+
+impl core::fmt::Display for PortKey {
+    #[inline]
+    fn fmt(&self, fmtr: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        let mut buf = [0; 12];
+        fmtr.write_str(self.write_into(&mut buf))
+    }
+}
+
+impl core::fmt::Debug for PortKey {
+    fn fmt(&self, fmtr: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        core::fmt::Display::fmt(self, fmtr)
+    }
+}
+
+
 /// Strips `prefix` from `data` and parses it to get `u32`.  Panics if data
 /// doesn’t start with the prefix or parsing fails.
 fn parse_sans_prefix(prefix: &'static str, data: &str) -> Option<u32> {

solana/solana-ibc/programs/solana-ibc/src/storage/trie_key.rs

@@ -15,13 +15,13 @@ use crate::storage::{ibc, ids};
 ///     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 },
-///     NextSequenceRecv { port_id: String, channel_id: u32 },
-///     NextSequenceAck  { port_id: String, channel_id: u32 },
-///     Commitment       { port_id: String, channel_id: u32, sequence: u64 },
-///     Receipts         { port_id: String, channel_id: u32, sequence: u64 },
-///     Acks             { port_id: String, channel_id: u32, sequence: u64 },
+///     ChannelEnd       { port_id: [u8; 9], channel_id: u32 },
+///     NextSequenceSend { port_id: [u8; 9], channel_id: u32 },
+///     NextSequenceRecv { port_id: [u8; 9], channel_id: u32 },
+///     NextSequenceAck  { port_id: [u8; 9], channel_id: u32 },
+///     Commitment       { port_id: [u8; 9], channel_id: u32, sequence: u64 },
+///     Receipts         { port_id: [u8; 9], channel_id: u32, sequence: u64 },
+///     Acks             { port_id: [u8; 9], channel_id: u32, sequence: u64 },
 /// }
 /// ```
 ///
@@ -83,24 +83,34 @@ impl TrieKey {
     }
 
     /// Constructs a new key for a channel end path.
+    ///
+    /// The hash stored under the key is `hash(borsh(channel_end))`.
     pub fn for_channel_end(port_channel: &ids::PortChannelPK) -> Self {
         Self::for_channel_path(Tag::ChannelEnd, port_channel)
     }
 
+    /// Constructs a new key for next sequence counters.
+    ///
+    /// The hash stored under the key is built by `SequenceTriple::hash` method
+    /// and directly encodes next send, receive and ack sequence numbers.
     pub fn for_next_sequence(port_channel: &ids::PortChannelPK) -> Self {
         Self::for_channel_path(Tag::NextSequence, port_channel)
     }
 
     /// Constructs a new key for a `(port_id, channel_id)` path.
     ///
-    /// Panics if `channel_id` is invalid.
+    /// This is internal method used by other public-facing methods which use
+    /// only (port, channel) tuple as the key component.
     fn for_channel_path(tag: Tag, port_channel: &ids::PortChannelPK) -> Self {
         new_key_impl!(tag, port_channel)
     }
 
     /// Constructs a new key for a `(port_id, channel_id, sequence)` path.
     ///
-    /// Panics if `channel_id` is invalid.
+    /// Returns an error if `port_id` or `channel_id` is invalid.
+    ///
+    /// This is internal method used by other public-facing interfaces which use
+    /// only (port, channel, sequence) tuple as the key component.
     fn try_for_sequence_path(
         tag: Tag,
         port_id: &ibc::PortId,
@@ -222,18 +232,12 @@ cast_component!(ids::ClientIdx as u32);
 cast_component!(ids::ConnectionIdx as u32);
 cast_component!(ibc::Sequence as u64);
 
-// TODO(#35): Investigate more compact ways of representing port identifier or
-// enforcing restrictions on it
 impl AsComponent for ids::PortChannelPK {
     fn key_len(&self) -> usize {
-        let port_id_len = self.port_id.as_bytes().len();
-        assert!(port_id_len <= usize::from(u8::MAX));
-        1 + port_id_len + self.channel_idx.key_len()
+        self.port_key.as_bytes().len() + self.channel_idx.key_len()
     }
     fn append_into(&self, dest: &mut Vec<u8>) {
-        let port_id = self.port_id.as_bytes();
-        dest.push(port_id.len() as u8);
-        dest.extend(port_id);
+        self.port_key.as_bytes().append_into(dest);
         self.channel_idx.append_into(dest);
     }
 }
@@ -249,13 +253,18 @@ impl AsComponent for ibc::Height {
 impl AsComponent for u32 {
     fn key_len(&self) -> usize { core::mem::size_of_val(self) }
     fn append_into(&self, dest: &mut Vec<u8>) {
-        dest.extend(&self.to_be_bytes()[..]);
+        self.to_be_bytes().append_into(dest)
     }
 }
 
 impl AsComponent for u64 {
     fn key_len(&self) -> usize { core::mem::size_of_val(self) }
     fn append_into(&self, dest: &mut Vec<u8>) {
-        dest.extend(&self.to_be_bytes()[..]);
+        self.to_be_bytes().append_into(dest)
     }
 }
+
+impl<const N: usize> AsComponent for [u8; N] {
+    fn key_len(&self) -> usize { N }
+    fn append_into(&self, dest: &mut Vec<u8>) { dest.extend_from_slice(self) }
+}