From 2c629cd902e63e8def07b5cde1211a6b101a922b Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Thu, 4 Feb 2021 12:44:53 +0100 Subject: [PATCH 1/4] Fix light requests handler error handling and logging target --- .../src/light_client_requests/handler.rs | 33 +++++++++++-------- client/network/src/service.rs | 3 ++ 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/client/network/src/light_client_requests/handler.rs b/client/network/src/light_client_requests/handler.rs index 08de99a0a5de4..9945a00ad77ac 100644 --- a/client/network/src/light_client_requests/handler.rs +++ b/client/network/src/light_client_requests/handler.rs @@ -159,6 +159,7 @@ impl LightClientRequestHandler { request: &schema::v1::light::RemoteCallRequest, ) -> Result { log::trace!( + target: LOG_TARGET, "Remote call request from {} ({} at {:?}).", peer, request.method, request.block, ); @@ -172,10 +173,11 @@ impl LightClientRequestHandler { Ok((_, proof)) => proof, Err(e) => { log::trace!( + target: LOG_TARGET, "remote call request from {} ({} at {:?}) failed with: {}", peer, request.method, request.block, e, ); - StorageProof::empty() + return Err(HandleRequestError::NotFulfillable); } }; @@ -193,11 +195,12 @@ impl LightClientRequestHandler { request: &schema::v1::light::RemoteReadRequest, ) -> Result { if request.keys.is_empty() { - log::debug!("Invalid remote read request sent by {}.", peer); + log::debug!(target: LOG_TARGET, "Invalid remote read request sent by {}.", peer); return Err(HandleRequestError::BadRequest("Remote read request without keys.")) } log::trace!( + target: LOG_TARGET, "Remote read request from {} ({} at {:?}).", peer, fmt_keys(request.keys.first(), request.keys.last()), request.block, ); @@ -211,10 +214,11 @@ impl LightClientRequestHandler { Ok(proof) => proof, Err(error) => { log::trace!( + target: LOG_TARGET, "remote read request from {} ({} at {:?}) failed with: {}", peer, fmt_keys(request.keys.first(), request.keys.last()), request.block, error, ); - StorageProof::empty() + return Err(HandleRequestError::NotFulfillable); } }; @@ -232,11 +236,12 @@ impl LightClientRequestHandler { request: &schema::v1::light::RemoteReadChildRequest, ) -> Result { if request.keys.is_empty() { - log::debug!("Invalid remote child read request sent by {}.", peer); + log::debug!(target: LOG_TARGET, "Invalid remote child read request sent by {}.", peer); return Err(HandleRequestError::BadRequest("Remove read child request without keys.")) } log::trace!( + target: LOG_TARGET, "Remote read child request from {} ({} {} at {:?}).", peer, HexDisplay::from(&request.storage_key), @@ -259,6 +264,7 @@ impl LightClientRequestHandler { Ok(proof) => proof, Err(error) => { log::trace!( + target: LOG_TARGET, "remote read child request from {} ({} {} at {:?}) failed with: {}", peer, HexDisplay::from(&request.storage_key), @@ -266,7 +272,7 @@ impl LightClientRequestHandler { request.block, error, ); - StorageProof::empty() + return Err(HandleRequestError::NotFulfillable); } }; @@ -283,17 +289,18 @@ impl LightClientRequestHandler { peer: &PeerId, request: &schema::v1::light::RemoteHeaderRequest, ) -> Result { - log::trace!("Remote header proof request from {} ({:?}).", peer, request.block); + log::trace!(target: LOG_TARGET, "Remote header proof request from {} ({:?}).", peer, request.block); let block = Decode::decode(&mut request.block.as_ref())?; let (header, proof) = match self.client.header_proof(&BlockId::Number(block)) { Ok((header, proof)) => (header.encode(), proof), Err(error) => { log::trace!( + target: LOG_TARGET, "Remote header proof request from {} ({:?}) failed with: {}.", peer, request.block, error ); - (Default::default(), StorageProof::empty()) + return Err(HandleRequestError::NotFulfillable); } }; @@ -311,6 +318,7 @@ impl LightClientRequestHandler { request: &schema::v1::light::RemoteChangesRequest, ) -> Result { log::trace!( + target: LOG_TARGET, "Remote changes proof request from {} for key {} ({:?}..{:?}).", peer, if !request.storage_key.is_empty() { @@ -337,6 +345,7 @@ impl LightClientRequestHandler { Ok(proof) => proof, Err(error) => { log::trace!( + target: LOG_TARGET, "Remote changes proof request from {} for key {} ({:?}..{:?}) failed with: {}.", peer, format!("{} : {}", HexDisplay::from(&request.storage_key), HexDisplay::from(&key.0)), @@ -345,12 +354,7 @@ impl LightClientRequestHandler { error, ); - light::ChangesProof:: { - max_block: Zero::zero(), - proof: Vec::new(), - roots: BTreeMap::new(), - roots_proof: StorageProof::empty(), - } + return Err(HandleRequestError::NotFulfillable); } }; @@ -381,6 +385,9 @@ enum HandleRequestError { /// A bad request has been received. #[display(fmt = "bad request: {}", _0)] BadRequest(&'static str), + /// Received request concerns blocks that aren't available, or a similar client-side problem. + #[display(fmt = "Request cannot be fulfilled")] + NotFulfillable, /// Encoding or decoding of some data failed. #[display(fmt = "codec error: {}", _0)] Codec(codec::Error), diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 39eaa606d0066..c6310bd9d7ade 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -1528,6 +1528,7 @@ impl Future for NetworkWorker { } { let mut peers_notifications_sinks = this.peers_notifications_sinks.lock(); + println!("peers_notifications_sinks.insert({:?}, {:?})", remote, protocol); peers_notifications_sinks.insert((remote.clone(), protocol.clone()), notifications_sink); } this.event_streams.send(Event::NotificationStreamOpened { @@ -1540,6 +1541,7 @@ impl Future for NetworkWorker { remote, protocol, notifications_sink })) => { let mut peers_notifications_sinks = this.peers_notifications_sinks.lock(); + println!("peers_notifications_sinks.replace({:?}, {:?})", remote, protocol); if let Some(s) = peers_notifications_sinks.get_mut(&(remote, protocol)) { *s = notifications_sink; } else { @@ -1581,6 +1583,7 @@ impl Future for NetworkWorker { }); { let mut peers_notifications_sinks = this.peers_notifications_sinks.lock(); + println!("peers_notifications_sinks.remove({:?}, {:?})", remote, protocol); peers_notifications_sinks.remove(&(remote.clone(), protocol)); } }, From d869cde5bbf8fddd4319661c343eb6b58831f219 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Thu, 4 Feb 2021 13:04:31 +0100 Subject: [PATCH 2/4] Revert service.rs --- client/network/src/service.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index c6310bd9d7ade..39eaa606d0066 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -1528,7 +1528,6 @@ impl Future for NetworkWorker { } { let mut peers_notifications_sinks = this.peers_notifications_sinks.lock(); - println!("peers_notifications_sinks.insert({:?}, {:?})", remote, protocol); peers_notifications_sinks.insert((remote.clone(), protocol.clone()), notifications_sink); } this.event_streams.send(Event::NotificationStreamOpened { @@ -1541,7 +1540,6 @@ impl Future for NetworkWorker { remote, protocol, notifications_sink })) => { let mut peers_notifications_sinks = this.peers_notifications_sinks.lock(); - println!("peers_notifications_sinks.replace({:?}, {:?})", remote, protocol); if let Some(s) = peers_notifications_sinks.get_mut(&(remote, protocol)) { *s = notifications_sink; } else { @@ -1583,7 +1581,6 @@ impl Future for NetworkWorker { }); { let mut peers_notifications_sinks = this.peers_notifications_sinks.lock(); - println!("peers_notifications_sinks.remove({:?}, {:?})", remote, protocol); peers_notifications_sinks.remove(&(remote.clone(), protocol)); } }, From 2614d43b93b4e5c1d5a594b4ecd728617a61359e Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Thu, 4 Feb 2021 13:20:24 +0100 Subject: [PATCH 3/4] Fix unused imports --- client/network/src/light_client_requests/handler.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/client/network/src/light_client_requests/handler.rs b/client/network/src/light_client_requests/handler.rs index 9945a00ad77ac..a91876c87d99f 100644 --- a/client/network/src/light_client_requests/handler.rs +++ b/client/network/src/light_client_requests/handler.rs @@ -31,23 +31,16 @@ use crate::{ use crate::request_responses::{IncomingRequest, OutgoingResponse, ProtocolConfig}; use futures::{channel::mpsc, prelude::*}; use prost::Message; -use sc_client_api::{ - StorageProof, - light -}; use sc_peerset::ReputationChange; use sp_core::{ storage::{ChildInfo, ChildType,StorageKey, PrefixedStorageKey}, hexdisplay::HexDisplay, }; use sp_runtime::{ - traits::{Block, Zero}, + traits::Block, generic::BlockId, }; -use std::{ - collections::{BTreeMap}, - sync::Arc, -}; +use std::sync::Arc; use log::debug; const LOG_TARGET: &str = "light-client-request-handler"; From 8b85cb3c5ea03e88861ad3ae341ab95a0b748c83 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Fri, 5 Feb 2021 08:20:06 +0100 Subject: [PATCH 4/4] Some test fix --- client/network/src/light_client_requests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/network/src/light_client_requests.rs b/client/network/src/light_client_requests.rs index f859a35f45b24..148c95fde2a0d 100644 --- a/client/network/src/light_client_requests.rs +++ b/client/network/src/light_client_requests.rs @@ -51,7 +51,7 @@ pub fn generate_protocol_config(protocol_id: &ProtocolId) -> ProtocolConfig { #[cfg(test)] mod tests { use super::*; - use crate::request_responses::IncomingRequest; + use crate::request_responses::{IncomingRequest, RequestFailure}; use crate::config::ProtocolId; use assert_matches::assert_matches; @@ -207,7 +207,7 @@ mod tests { pending_response: tx, })).unwrap(); pool.spawner().spawn_obj(async move { - pending_response.send(Ok(rx.await.unwrap().result.unwrap())).unwrap(); + pending_response.send(rx.await.unwrap().result.map_err(|()| RequestFailure::Refused)).unwrap(); }.boxed().into()).unwrap(); pool.spawner().spawn_obj(sender.for_each(|_| future::ready(())).boxed().into()).unwrap();