From 3e2880d6c85a47db74005e33d7c5b5f2656faa18 Mon Sep 17 00:00:00 2001 From: David Salami <31099392+Wizdave97@users.noreply.github.com> Date: Fri, 23 Jun 2023 13:05:33 +0100 Subject: [PATCH] Fix edge cases in response handling (#59) --- ismp-testsuite/src/lib.rs | 13 ++++++----- ismp-testsuite/src/mocks.rs | 13 +++++------ ismp/src/handlers/request.rs | 2 +- ismp/src/handlers/response.rs | 41 ++++++++++++++++++++--------------- ismp/src/handlers/timeout.rs | 24 +++++--------------- ismp/src/host.rs | 17 +++++++-------- 6 files changed, 52 insertions(+), 58 deletions(-) diff --git a/ismp-testsuite/src/lib.rs b/ismp-testsuite/src/lib.rs index 2e1a1d0da..686b80108 100644 --- a/ismp-testsuite/src/lib.rs +++ b/ismp-testsuite/src/lib.rs @@ -30,6 +30,7 @@ use ismp::{ router::{ DispatchPost, DispatchRequest, IsmpDispatcher, Post, PostResponse, Request, Response, }, + util::hash_request, }; fn setup_mock_client(host: &H) -> IntermediateState { @@ -225,8 +226,9 @@ pub fn timeout_post_processing_check( handle_incoming_message(host, timeout_message).unwrap(); // Assert that request commitment was deleted - let commitment = host.request_commitment(&request); - assert!(matches!(commitment, Err(..))); + let commitment = hash_request::(&request); + let res = host.request_commitment(commitment); + assert!(matches!(res, Err(..))); Ok(()) } @@ -236,8 +238,8 @@ pub fn timeout_post_processing_check( /// Check that dispatcher stores commitments for outgoing requests and responses and rejects /// duplicate responses -pub fn write_outgoing_commitments( - host: &dyn IsmpHost, +pub fn write_outgoing_commitments( + host: &H, dispatcher: &dyn IsmpDispatcher, ) -> Result<(), &'static str> { let post = DispatchPost { @@ -263,7 +265,8 @@ pub fn write_outgoing_commitments( data: vec![0u8; 64], }; let request = Request::Post(post); - host.request_commitment(&request) + let commitment = hash_request::(&request); + host.request_commitment(commitment) .map_err(|_| "Expected Request commitment to be found in storage")?; let post = Post { source_chain: StateMachine::Kusama(2000), diff --git a/ismp-testsuite/src/mocks.rs b/ismp-testsuite/src/mocks.rs index 07abbef6a..0e06207e1 100644 --- a/ismp-testsuite/src/mocks.rs +++ b/ismp-testsuite/src/mocks.rs @@ -164,12 +164,11 @@ impl IsmpHost for Host { Ok(()) } - fn request_commitment(&self, req: &Request) -> Result { - let hash = hash_request::(req); + fn request_commitment(&self, hash: H256) -> Result<(), Error> { self.requests .borrow() .contains(&hash) - .then_some(hash) + .then_some(()) .ok_or_else(|| Error::ImplementationSpecific("Request commitment not found".into())) } @@ -184,8 +183,8 @@ impl IsmpHost for Host { self.receipts.borrow().get(&hash).map(|_| ()) } - fn response_receipt(&self, res: &Response) -> Option<()> { - let hash = hash_response::(res); + fn response_receipt(&self, res: &Request) -> Option<()> { + let hash = hash_request::(res); self.receipts.borrow().get(&hash).map(|_| ()) } @@ -238,8 +237,8 @@ impl IsmpHost for Host { Ok(()) } - fn store_response_receipt(&self, res: &Response) -> Result<(), Error> { - let hash = hash_response::(res); + fn store_response_receipt(&self, res: &Request) -> Result<(), Error> { + let hash = hash_request::(res); self.receipts.borrow_mut().insert(hash, ()); Ok(()) } diff --git a/ismp/src/handlers/request.rs b/ismp/src/handlers/request.rs index eb34cf5e1..9d55b4d8d 100644 --- a/ismp/src/handlers/request.rs +++ b/ismp/src/handlers/request.rs @@ -60,7 +60,7 @@ where nonce: request.nonce, }) .map_err(|e| DispatchError { - msg: format!("{:?}", e), + msg: format!("{e:?}"), nonce: request.nonce, source_chain: request.source_chain, dest_chain: request.dest_chain, diff --git a/ismp/src/handlers/response.rs b/ismp/src/handlers/response.rs index e5de6d5fc..6e158e23c 100644 --- a/ismp/src/handlers/response.rs +++ b/ismp/src/handlers/response.rs @@ -32,23 +32,22 @@ where H: IsmpHost, { let state_machine = validate_state_machine(host, msg.proof().height)?; - for request in &msg.requests() { - // For a response to be valid a request commitment must be present in storage - let commitment = host.request_commitment(request)?; - - if commitment != hash_request::(request) { - return Err(Error::RequestCommitmentNotFound { - nonce: request.nonce(), - source: request.source_chain(), - dest: request.dest_chain(), - }) - } - } let state = host.state_machine_commitment(msg.proof().height)?; let result = match msg { ResponseMessage::Post { responses, proof } => { + // For a response to be valid a request commitment must be present in storage + // Also we must not have received a response for this request + let responses = responses + .into_iter() + .filter(|response| { + let request = response.request(); + let commitment = hash_request::(&request); + host.request_commitment(commitment).is_ok() && + host.response_receipt(&request).is_none() + }) + .collect::>(); // Verify membership proof state_machine.verify_membership( host, @@ -61,7 +60,6 @@ where responses .into_iter() - .filter(|res| host.response_receipt(res).is_none()) .map(|response| { let cb = router.module_for_id(response.destination_module())?; let res = cb @@ -72,17 +70,25 @@ where nonce: response.nonce(), }) .map_err(|e| DispatchError { - msg: format!("{:?}", e), + msg: format!("{e:?}"), nonce: response.nonce(), source_chain: response.source_chain(), dest_chain: response.dest_chain(), }); - host.store_response_receipt(&response)?; + host.store_response_receipt(&response.request())?; Ok(res) }) .collect::, _>>()? } ResponseMessage::Get { requests, proof } => { + let requests = requests + .into_iter() + .filter(|request| { + let commitment = hash_request::(request); + host.request_commitment(commitment).is_ok() && + host.response_receipt(request).is_none() + }) + .collect::>(); // Ensure the proof height is greater than each retrieval height specified in the Get // requests sufficient_proof_height(&requests, &proof)?; @@ -90,7 +96,6 @@ where // individually requests .into_iter() - .filter(|req| host.request_receipt(req).is_none()) .map(|request| { let keys = request.keys().ok_or_else(|| { Error::ImplementationSpecific("Missing keys for get request".to_string()) @@ -110,12 +115,12 @@ where nonce: request.nonce(), }) .map_err(|e| DispatchError { - msg: format!("{:?}", e), + msg: format!("{e:?}"), nonce: request.nonce(), source_chain: request.source_chain(), dest_chain: request.dest_chain(), }); - host.store_request_receipt(&request)?; + host.store_response_receipt(&request)?; Ok(res) }) .collect::, _>>()? diff --git a/ismp/src/handlers/timeout.rs b/ismp/src/handlers/timeout.rs index a48e08a34..a6b3a964a 100644 --- a/ismp/src/handlers/timeout.rs +++ b/ismp/src/handlers/timeout.rs @@ -36,14 +36,8 @@ where let state = host.state_machine_commitment(timeout_proof.height)?; for request in &requests { // Ensure a commitment exists for all requests in the batch - let commitment = host.request_commitment(request)?; - if commitment != hash_request::(request) { - return Err(Error::RequestCommitmentNotFound { - nonce: request.nonce(), - source: request.source_chain(), - dest: request.dest_chain(), - }) - } + let commitment = hash_request::(request); + host.request_commitment(commitment)?; if !request.timed_out(state.timestamp()) { Err(Error::RequestTimeoutNotElapsed { @@ -77,7 +71,7 @@ where nonce: request.nonce(), }) .map_err(|e| DispatchError { - msg: format!("{:?}", e), + msg: format!("{e:?}"), nonce: request.nonce(), source_chain: request.source_chain(), dest_chain: request.dest_chain(), @@ -89,14 +83,8 @@ where } TimeoutMessage::Get { requests } => { for request in &requests { - let commitment = host.request_commitment(request)?; - if commitment != hash_request::(request) { - return Err(Error::RequestCommitmentNotFound { - nonce: request.nonce(), - source: request.source_chain(), - dest: request.dest_chain(), - }) - } + let commitment = hash_request::(request); + host.request_commitment(commitment)?; // Ensure the get timeout has elapsed on the host if !request.timed_out(host.timestamp()) { @@ -122,7 +110,7 @@ where nonce: request.nonce(), }) .map_err(|e| DispatchError { - msg: format!("{:?}", e), + msg: format!("{e:?}"), nonce: request.nonce(), source_chain: request.source_chain(), dest_chain: request.dest_chain(), diff --git a/ismp/src/host.rs b/ismp/src/host.rs index d43d9be2a..26eeffa24 100644 --- a/ismp/src/host.rs +++ b/ismp/src/host.rs @@ -21,7 +21,7 @@ use crate::{ }, error::Error, prelude::Vec, - router::{IsmpRouter, Request, Response}, + router::{IsmpRouter, Request}, }; use alloc::{ boxed::Box, @@ -63,17 +63,17 @@ pub trait IsmpHost { /// Checks if a state machine is frozen at the provided height fn is_consensus_client_frozen(&self, client: ConsensusClientId) -> Result<(), Error>; - /// Fetch commitment of an outgoing request from storage - fn request_commitment(&self, req: &Request) -> Result; + /// Should return an error if request commitment does not exist in storage + fn request_commitment(&self, req: H256) -> Result<(), Error>; /// Increment and return the next available nonce for an outgoing request. fn next_nonce(&self) -> u64; - /// Get an incoming request receipt, should return Some(()) if a receipt exists in storage + /// Should return Some(()) if a receipt for this request exists in storage fn request_receipt(&self, req: &Request) -> Option<()>; - /// Get an incoming response receipt, should return Some(()) if a receipt exists in storage - fn response_receipt(&self, res: &Response) -> Option<()>; + /// Should return Some(()) if a response has been received for the given request + fn response_receipt(&self, res: &Request) -> Option<()>; /// Store an encoded consensus state fn store_consensus_state(&self, id: ConsensusClientId, state: Vec) -> Result<(), Error>; @@ -108,9 +108,8 @@ pub trait IsmpHost { /// Prevents duplicate incoming requests from being processed. fn store_request_receipt(&self, req: &Request) -> Result<(), Error>; - /// Stores a receipt for an incoming response after it is successfully routed to a module. - /// Prevents duplicate incoming responses from being processed. - fn store_response_receipt(&self, req: &Response) -> Result<(), Error>; + /// Stores a receipt that shows that the given request has received a response + fn store_response_receipt(&self, req: &Request) -> Result<(), Error>; /// Should return a handle to the consensus client based on the id fn consensus_client(&self, id: ConsensusClientId) -> Result, Error>;