Fix edge cases in response handling (#59)

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<H: IsmpHost>(host: &H) -> IntermediateState {
@@ -225,8 +226,9 @@ pub fn timeout_post_processing_check<H: IsmpHost>(
     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::<H>(&request);
+    let res = host.request_commitment(commitment);
+    assert!(matches!(res, Err(..)));
     Ok(())
 }
 
@@ -236,8 +238,8 @@ pub fn timeout_post_processing_check<H: IsmpHost>(
 
 /// 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<H: IsmpHost>(
+    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::<H>(&request);
+    host.request_commitment(commitment)
         .map_err(|_| "Expected Request commitment to be found in storage")?;
     let post = Post {
         source_chain: StateMachine::Kusama(2000),

ismp-testsuite/src/mocks.rs

@@ -164,12 +164,11 @@ impl IsmpHost for Host {
         Ok(())
     }
 
-    fn request_commitment(&self, req: &Request) -> Result<H256, Error> {
-        let hash = hash_request::<Self>(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::<Self>(res);
+    fn response_receipt(&self, res: &Request) -> Option<()> {
+        let hash = hash_request::<Self>(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::<Self>(res);
+    fn store_response_receipt(&self, res: &Request) -> Result<(), Error> {
+        let hash = hash_request::<Self>(res);
         self.receipts.borrow_mut().insert(hash, ());
         Ok(())
     }

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,

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::<H>(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::<H>(&request);
+                    host.request_commitment(commitment).is_ok() &&
+                        host.response_receipt(&request).is_none()
+                })
+                .collect::<Vec<_>>();
             // 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,25 +70,32 @@ 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::<Result<Vec<_>, _>>()?
         }
         ResponseMessage::Get { requests, proof } => {
+            let requests = requests
+                .into_iter()
+                .filter(|request| {
+                    let commitment = hash_request::<H>(request);
+                    host.request_commitment(commitment).is_ok() &&
+                        host.response_receipt(request).is_none()
+                })
+                .collect::<Vec<_>>();
             // Ensure the proof height is greater than each retrieval height specified in the Get
             // requests
             sufficient_proof_height(&requests, &proof)?;
             // Since each get request can  contain multiple storage keys, we should handle them
             // 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::<Result<Vec<_>, _>>()?

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::<H>(request) {
-                    return Err(Error::RequestCommitmentNotFound {
-                        nonce: request.nonce(),
-                        source: request.source_chain(),
-                        dest: request.dest_chain(),
-                    })
-                }
+                let commitment = hash_request::<H>(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::<H>(request) {
-                    return Err(Error::RequestCommitmentNotFound {
-                        nonce: request.nonce(),
-                        source: request.source_chain(),
-                        dest: request.dest_chain(),
-                    })
-                }
+                let commitment = hash_request::<H>(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(),

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<H256, Error>;
+    /// 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<u8>) -> 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<Box<dyn ConsensusClient>, Error>;