diff --git a/enclave-modules/ecall-handler/src/light_client/errors.rs b/enclave-modules/ecall-handler/src/light_client/errors.rs index c1b4644d..a8c495fc 100644 --- a/enclave-modules/ecall-handler/src/light_client/errors.rs +++ b/enclave-modules/ecall-handler/src/light_client/errors.rs @@ -15,6 +15,14 @@ define_error! { SealedEnclaveKeyNotFound |_| { "Sealed EnclaveKey not found" }, + ClientAlreadyExists + { + client_id: String + } + |e| { + format_args!("client already exists: client_id={}", e.client_id) + }, + LightClient [light_client::Error] |_| { "LightClient error" }, diff --git a/enclave-modules/ecall-handler/src/light_client/init_client.rs b/enclave-modules/ecall-handler/src/light_client/init_client.rs index 817e0931..b4ec6226 100644 --- a/enclave-modules/ecall-handler/src/light_client/init_client.rs +++ b/enclave-modules/ecall-handler/src/light_client/init_client.rs @@ -21,12 +21,15 @@ pub fn init_client( let ek = ctx.get_enclave_key(); let res = lc.create_client(ctx, any_client_state.clone(), any_consensus_state.clone())?; let client_type = lc.client_type(); - let client_id = gen_client_id(client_type.clone(), ctx.client_counter()?)?; + let client_id = ClientId::from_str(&input.client_id)?; + client_id.validate(&client_type)?; + if ctx.client_exists(&client_id) { + return Err(Error::client_already_exists(client_id.to_string())); + } ctx.store_client_type(client_id.clone(), client_type)?; ctx.store_any_client_state(client_id.clone(), any_client_state)?; ctx.store_any_consensus_state(client_id.clone(), res.height, any_consensus_state)?; - ctx.increase_client_counter(); let proof = if res.prove { prove_commitment(ek, input.signer, res.message)? @@ -34,11 +37,6 @@ pub fn init_client( CommitmentProof::new_with_no_signature(res.message.to_bytes()) }; Ok(LightClientResponse::InitClient(InitClientResponse { - client_id, proof, })) } - -fn gen_client_id(client_type: String, counter: u64) -> Result { - Ok(ClientId::from_str(&format!("{}-{}", client_type, counter))?) -} diff --git a/enclave-modules/ecall-handler/src/light_client/query.rs b/enclave-modules/ecall-handler/src/light_client/query.rs index 710a8b5a..41952bf7 100644 --- a/enclave-modules/ecall-handler/src/light_client/query.rs +++ b/enclave-modules/ecall-handler/src/light_client/query.rs @@ -10,13 +10,21 @@ pub fn query_client( ctx: &mut Context, input: QueryClientInput, ) -> Result { - let lc = get_light_client_by_client_id(ctx, &input.client_id)?; - let any_client_state = ctx.client_state(&input.client_id)?; - let any_consensus_state = - ctx.consensus_state(&input.client_id, &lc.latest_height(ctx, &input.client_id)?)?; - - Ok(LightClientResponse::QueryClient(QueryClientResponse { - any_client_state, - any_consensus_state, - })) + if ctx.client_exists(&input.client_id) { + let lc = get_light_client_by_client_id(ctx, &input.client_id)?; + let any_client_state = ctx.client_state(&input.client_id)?; + let any_consensus_state = + ctx.consensus_state(&input.client_id, &lc.latest_height(ctx, &input.client_id)?)?; + Ok(LightClientResponse::QueryClient(QueryClientResponse { + found: true, + any_client_state: Some(any_client_state), + any_consensus_state: Some(any_consensus_state), + })) + } else { + Ok(LightClientResponse::QueryClient(QueryClientResponse { + found: false, + any_client_state: None, + any_consensus_state: None, + })) + } } diff --git a/modules/ecall-commands/src/light_client.rs b/modules/ecall-commands/src/light_client.rs index 71a43aaf..893607db 100644 --- a/modules/ecall-commands/src/light_client.rs +++ b/modules/ecall-commands/src/light_client.rs @@ -41,6 +41,7 @@ impl EnclaveKeySelector for LightClientCommand { #[derive(Serialize, Deserialize, Debug)] pub struct InitClientInput { + pub client_id: String, pub any_client_state: Any, pub any_consensus_state: Any, pub current_timestamp: Time, @@ -105,7 +106,6 @@ pub enum LightClientResponse { #[derive(Serialize, Deserialize, Debug)] pub struct InitClientResponse { - pub client_id: ClientId, pub proof: CommitmentProof, } @@ -123,6 +123,7 @@ pub struct VerifyNonMembershipResponse(pub CommitmentProof); #[derive(Serialize, Deserialize, Debug)] pub struct QueryClientResponse { - pub any_client_state: Any, - pub any_consensus_state: Any, + pub found: bool, + pub any_client_state: Option, + pub any_consensus_state: Option, } diff --git a/modules/ecall-commands/src/msgs.rs b/modules/ecall-commands/src/msgs.rs index 9a7bbc80..8efe8a36 100644 --- a/modules/ecall-commands/src/msgs.rs +++ b/modules/ecall-commands/src/msgs.rs @@ -23,6 +23,7 @@ impl TryFrom for InitClientInput { .ok_or_else(|| Error::invalid_argument("consensus_state must be non-nil".into()))? .into(); Ok(Self { + client_id: msg.client_id, any_client_state, any_consensus_state, current_timestamp: Time::now(), @@ -116,7 +117,6 @@ impl TryFrom for QueryClientInput { impl From for MsgCreateClientResponse { fn from(res: InitClientResponse) -> Self { Self { - client_id: res.client_id.to_string(), message: res.proof.message, signer: res.proof.signer.into(), signature: res.proof.signature, @@ -167,8 +167,9 @@ impl From for MsgVerifyNonMembershipResponse { impl From for MsgQueryClientResponse { fn from(res: QueryClientResponse) -> Self { Self { - client_state: Some(res.any_client_state.into()), - consensus_state: Some(res.any_consensus_state.into()), + found: res.found, + client_state: res.any_client_state.map(Into::into), + consensus_state: res.any_consensus_state.map(Into::into), } } } diff --git a/modules/enclave-api/src/api/proto.rs b/modules/enclave-api/src/api/proto.rs index f7199fa7..465dbf03 100644 --- a/modules/enclave-api/src/api/proto.rs +++ b/modules/enclave-api/src/api/proto.rs @@ -11,10 +11,11 @@ use store::transaction::CommitStore; pub trait EnclaveProtoAPI: EnclaveCommandAPI { fn proto_create_client(&self, msg: MsgCreateClient) -> Result { + let client_id = msg.client_id.clone(); let res = self.init_client(msg.try_into()?)?; info!( "create_client: client_id={} message={{{}}}", - res.client_id, + client_id, res.proof.message()? ); Ok(res.into()) diff --git a/modules/light-client/src/context.rs b/modules/light-client/src/context.rs index a7711950..2ce4dbbd 100644 --- a/modules/light-client/src/context.rs +++ b/modules/light-client/src/context.rs @@ -1,7 +1,7 @@ use crate::types::{Any, ClientId, Height, Time}; use crate::{ errors::Error, - path::{ClientConsensusStatePath, ClientStatePath, ClientTypePath, NEXT_CLIENT_SEQUENCE}, + path::{ClientConsensusStatePath, ClientStatePath, ClientTypePath}, prelude::*, }; use store::KVStore; @@ -12,6 +12,12 @@ pub trait HostContext { } pub trait ClientReader: KVStore { + /// Returns `true` if the client exists in the store. + fn client_exists(&self, client_id: &ClientId) -> bool { + self.get(format!("{}", ClientTypePath::new(client_id)).as_bytes()) + .is_some() + } + /// Returns the ClientType for the given identifier `client_id`. fn client_type(&self, client_id: &ClientId) -> Result { let value = self.get(format!("{}", ClientTypePath::new(client_id)).as_bytes()); @@ -54,19 +60,6 @@ pub trait ClientReader: KVStore { .0, ) } - - /// Returns a natural number, counting how many clients have been created thus far. - /// The value of this counter should increase only via method `ClientKeeper::increase_client_counter`. - fn client_counter(&self) -> Result { - match self.get(NEXT_CLIENT_SEQUENCE.as_bytes()) { - Some(bz) => { - let mut b: [u8; 8] = Default::default(); - b.copy_from_slice(&bz); - Ok(u64::from_be_bytes(b)) - } - None => Ok(0), - } - } } pub trait ClientKeeper: ClientReader { @@ -106,17 +99,6 @@ pub trait ClientKeeper: ClientReader { self.set(format!("{}", path).into_bytes(), bz); Ok(()) } - - /// Called upon client creation. - /// Increases the counter which keeps track of how many clients have been created. - /// Should never fail. - fn increase_client_counter(&mut self) { - let next_counter = ::client_counter(self).unwrap() + 1; - self.set( - NEXT_CLIENT_SEQUENCE.as_bytes().to_vec(), - next_counter.to_be_bytes().to_vec(), - ); - } } pub trait HostClientReader: HostContext + ClientReader {} diff --git a/modules/light-client/src/ibc.rs b/modules/light-client/src/ibc.rs index 6e71b721..ab555b42 100644 --- a/modules/light-client/src/ibc.rs +++ b/modules/light-client/src/ibc.rs @@ -138,11 +138,7 @@ impl<'a, ClientState: Ics02ClientState, ConsensusState: Ics02ConsensusState> Val } fn client_counter(&self) -> Result { - self.parent.client_counter().map_err(|e| { - ContextError::ClientError(ClientError::ClientSpecific { - description: e.to_string(), - }) - }) + unimplemented!() } fn connection_end( diff --git a/modules/light-client/src/path.rs b/modules/light-client/src/path.rs index 13dcebb6..c3034c8f 100644 --- a/modules/light-client/src/path.rs +++ b/modules/light-client/src/path.rs @@ -1,8 +1,6 @@ use crate::types::{ClientId, Height}; use derive_more::Display; -pub static NEXT_CLIENT_SEQUENCE: &str = "nextClientSequence"; - #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Display)] #[display(fmt = "clients/{_0}/clientType")] pub struct ClientTypePath(pub ClientId); diff --git a/modules/types/src/errors.rs b/modules/types/src/errors.rs index aafaeddf..74cba744 100644 --- a/modules/types/src/errors.rs +++ b/modules/types/src/errors.rs @@ -46,6 +46,26 @@ define_error! { |_| { "identifier cannot be empty" }, + ClientIdInvalidFormat + { id: String } + |e| { + format_args!("identifier `{}` must be in the format `{{client_type}}-{{counter}}`", e.id) + }, + ClientIdInvalidClientType + { id: String, client_type: String } + |e| { + format_args!("identifier `{}` must have client type `{}`", e.id, e.client_type) + }, + ClientIdInvalidCounter + { id: String } + |e| { + format_args!("identifier `{}` must have a valid counter", e.id) + }, + ClientIdInvalidCounterParseIntError + { id: String, e: core::num::ParseIntError } + |e| { + format_args!("identifier `{}` counter parse error: {}", e.id, e.e) + }, MrenclaveBytesConversion { bz: Vec, diff --git a/modules/types/src/host.rs b/modules/types/src/host.rs index 9b7f67fc..2a06c407 100644 --- a/modules/types/src/host.rs +++ b/modules/types/src/host.rs @@ -29,6 +29,35 @@ impl ClientId { pub fn as_bytes(&self) -> &[u8] { self.0.as_bytes() } + + /// Validate the client identifier + pub fn validate(&self, client_type: &str) -> Result<(), TypeError> { + validate_client_identifier(self.0.as_str())?; + self.0.rfind('-').map_or( + Err(TypeError::client_id_invalid_format(self.0.clone())), + |pos| { + let (client_type_, prefixed_counter) = self.0.split_at(pos); + if client_type_ != client_type { + return Err(TypeError::client_id_invalid_client_type( + self.0.clone(), + client_type.to_string(), + )); + } + match prefixed_counter.strip_prefix('-') { + None => Err(TypeError::client_id_invalid_counter(self.0.clone())), + Some(counter) if counter.starts_with('0') && counter.len() > 1 => { + Err(TypeError::client_id_invalid_counter(self.0.clone())) + } + Some(counter) => { + counter.parse::().map_err(|e| { + TypeError::client_id_invalid_counter_parse_int_error(self.0.clone(), e) + })?; + Ok(()) + } + } + }, + ) + } } /// This implementation provides a `to_string` method. @@ -116,3 +145,46 @@ fn validate_identifier(id: &str, min: usize, max: usize) -> Result<(), TypeError // All good! Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_valid_client_id() { + let cases = vec![ + ("testclient", "testclient-0"), + ("testclient", "testclient-1"), + ("testclient", "testclient-10"), + ("07-tendermint", "07-tendermint-0"), + ("07-tendermint", "07-tendermint-1"), + ("07-tendermint", "07-tendermint-10"), + ("testclient", "testclient-12345678901234567890"), + ]; + for (i, (client_type, c)) in cases.iter().enumerate() { + let cl = ClientId::from_str(c).unwrap(); + let res = cl.validate(client_type); + assert!(res.is_ok(), "case: {}, error: {:?}", i, res); + } + } + + #[test] + fn test_invalid_client_id() { + let cases = vec![ + ("testclient", "testclient"), + ("testclient", "testclient1"), + ("07-tendermint", "07-tendermint"), + ("07-tendermint", "07-tendermint0"), + ("07-tendermint", "07-tendermint1"), + ("07-tendermint", "07-tendermint-01"), + ("client", "client-0"), + ("", ""), + ("", "07-tendermint"), + ("testclient", "testclient-123456789012345678901"), + ]; + for (i, (client_type, c)) in cases.iter().enumerate() { + let res = ClientId::from_str(c).and_then(|cl| cl.validate(client_type)); + assert!(res.is_err(), "case: {}, error: {:?}", i, res); + } + } +} diff --git a/proto/definitions/lcp/service/elc/v1/query.proto b/proto/definitions/lcp/service/elc/v1/query.proto index ff4beeff..56a3b0b2 100644 --- a/proto/definitions/lcp/service/elc/v1/query.proto +++ b/proto/definitions/lcp/service/elc/v1/query.proto @@ -24,9 +24,11 @@ message QueryClientResponse { option (gogoproto.equal) = false; option (gogoproto.goproto_getters) = false; + // if false, the client_state and consensus_state fields will be empty + bool found = 1; // light client state - google.protobuf.Any client_state = 1 [(gogoproto.moretags) = "yaml:\"client_state\""]; + google.protobuf.Any client_state = 2 [(gogoproto.moretags) = "yaml:\"client_state\""]; // consensus state associated with the client that corresponds to a given // height. - google.protobuf.Any consensus_state = 2 [(gogoproto.moretags) = "yaml:\"consensus_state\""]; + google.protobuf.Any consensus_state = 3 [(gogoproto.moretags) = "yaml:\"consensus_state\""]; } diff --git a/proto/definitions/lcp/service/elc/v1/tx.proto b/proto/definitions/lcp/service/elc/v1/tx.proto index 0b916a90..9d9bc976 100644 --- a/proto/definitions/lcp/service/elc/v1/tx.proto +++ b/proto/definitions/lcp/service/elc/v1/tx.proto @@ -31,13 +31,14 @@ message MsgCreateClient { option (gogoproto.equal) = false; option (gogoproto.goproto_getters) = false; + string client_id = 1 [(gogoproto.moretags) = "yaml:\"client_id\""]; // light client state - google.protobuf.Any client_state = 1 [(gogoproto.moretags) = "yaml:\"client_state\""]; + google.protobuf.Any client_state = 2 [(gogoproto.moretags) = "yaml:\"client_state\""]; // consensus state associated with the client that corresponds to a given // height. - google.protobuf.Any consensus_state = 2 [(gogoproto.moretags) = "yaml:\"consensus_state\""]; + google.protobuf.Any consensus_state = 3 [(gogoproto.moretags) = "yaml:\"consensus_state\""]; // enclave key for signing - bytes signer = 3; + bytes signer = 4; } // MsgCreateClientResponse defines the Msg/CreateClient response type. @@ -45,10 +46,9 @@ message MsgCreateClientResponse { option (gogoproto.equal) = false; option (gogoproto.goproto_getters) = false; - string client_id = 1; - bytes message = 2; - bytes signer = 3; - bytes signature = 4; + bytes message = 1; + bytes signer = 2; + bytes signature = 3; } // MsgUpdateClient defines an sdk.Msg to update a IBC client state using diff --git a/proto/src/descriptor.bin b/proto/src/descriptor.bin index 1eddd20f..2dc55d1d 100644 Binary files a/proto/src/descriptor.bin and b/proto/src/descriptor.bin differ diff --git a/proto/src/prost/lcp.service.elc.v1.rs b/proto/src/prost/lcp.service.elc.v1.rs index dc8d0241..a1838678 100644 --- a/proto/src/prost/lcp.service.elc.v1.rs +++ b/proto/src/prost/lcp.service.elc.v1.rs @@ -9,14 +9,17 @@ pub struct QueryClientRequest { #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct QueryClientResponse { + /// if false, the client_state and consensus_state fields will be empty + #[prost(bool, tag = "1")] + pub found: bool, /// light client state - #[prost(message, optional, tag = "1")] + #[prost(message, optional, tag = "2")] pub client_state: ::core::option::Option< super::super::super::super::google::protobuf::Any, >, /// consensus state associated with the client that corresponds to a given /// height. - #[prost(message, optional, tag = "2")] + #[prost(message, optional, tag = "3")] pub consensus_state: ::core::option::Option< super::super::super::super::google::protobuf::Any, >, @@ -266,19 +269,21 @@ pub mod query_server { #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct MsgCreateClient { + #[prost(string, tag = "1")] + pub client_id: ::prost::alloc::string::String, /// light client state - #[prost(message, optional, tag = "1")] + #[prost(message, optional, tag = "2")] pub client_state: ::core::option::Option< super::super::super::super::google::protobuf::Any, >, /// consensus state associated with the client that corresponds to a given /// height. - #[prost(message, optional, tag = "2")] + #[prost(message, optional, tag = "3")] pub consensus_state: ::core::option::Option< super::super::super::super::google::protobuf::Any, >, /// enclave key for signing - #[prost(bytes = "vec", tag = "3")] + #[prost(bytes = "vec", tag = "4")] pub signer: ::prost::alloc::vec::Vec, } /// MsgCreateClientResponse defines the Msg/CreateClient response type. @@ -286,13 +291,11 @@ pub struct MsgCreateClient { #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct MsgCreateClientResponse { - #[prost(string, tag = "1")] - pub client_id: ::prost::alloc::string::String, - #[prost(bytes = "vec", tag = "2")] + #[prost(bytes = "vec", tag = "1")] pub message: ::prost::alloc::vec::Vec, - #[prost(bytes = "vec", tag = "3")] + #[prost(bytes = "vec", tag = "2")] pub signer: ::prost::alloc::vec::Vec, - #[prost(bytes = "vec", tag = "4")] + #[prost(bytes = "vec", tag = "3")] pub signature: ::prost::alloc::vec::Vec, } /// MsgUpdateClient defines an sdk.Msg to update a IBC client state using diff --git a/tests/integration/src/lib.rs b/tests/integration/src/lib.rs index 21333825..6ddef752 100644 --- a/tests/integration/src/lib.rs +++ b/tests/integration/src/lib.rs @@ -34,7 +34,7 @@ mod tests { }; use keymanager::EnclaveKeyManager; use lcp_proto::protobuf::Protobuf; - use lcp_types::{Height, Time}; + use lcp_types::{ClientId, Height, Time}; use log::*; use std::sync::{Arc, RwLock}; use std::{ops::Add, str::FromStr, time::Duration}; @@ -157,16 +157,17 @@ mod tests { initial_height, client_state, consensus_state ); + let client_id = "07-tendermint-0".to_string(); let res = enclave.init_client(InitClientInput { + client_id: client_id.clone(), any_client_state: client_state, any_consensus_state: consensus_state, current_timestamp: Time::now(), signer, })?; assert!(!res.proof.is_proven()); - let client_id = res.client_id; - (client_id, initial_height) + (ClientId::from_str(&client_id)?, initial_height) }; info!("generated client: id={} height={}", client_id, last_height);