From 7e5ce3f6e1f46cadaeb16537d844c5620236243e Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 22 Nov 2022 09:56:00 +0100 Subject: [PATCH] Backport changes about the light client protocol (#3018) Fix https://github.com/paritytech/smoldot/issues/2975 Backports https://github.com/paritytech/substrate/pull/8046 See also https://github.com/paritytech/substrate/pull/12732 --- bin/light-base/src/runtime_service.rs | 2 -- bin/light-base/src/sync_service.rs | 7 ++--- bin/wasm-node/CHANGELOG.md | 4 +++ src/network/protocol/storage_call_proof.rs | 19 +++++++----- src/network/service/requests_responses.rs | 35 +++++++++++++--------- 5 files changed, 39 insertions(+), 28 deletions(-) diff --git a/bin/light-base/src/runtime_service.rs b/bin/light-base/src/runtime_service.rs index 9cdd810e6a..bce834eb96 100644 --- a/bin/light-base/src/runtime_service.rs +++ b/bin/light-base/src/runtime_service.rs @@ -961,8 +961,6 @@ impl RuntimeCallError { pub fn is_network_problem(&self) -> bool { match self { RuntimeCallError::InvalidRuntime(_) => false, - // TODO: as a temporary hack, we consider `TrieRootNotFound` as the remote not knowing about the requested block; see https://github.com/paritytech/substrate/pull/8046 - RuntimeCallError::StorageRetrieval(proof_decode::Error::TrieRootNotFound) => true, RuntimeCallError::StorageRetrieval(_) => false, RuntimeCallError::MissingProofEntry => false, RuntimeCallError::CallProof(err) => err.is_network_problem(), diff --git a/bin/light-base/src/sync_service.rs b/bin/light-base/src/sync_service.rs index 51d924aef4..99f13cad2d 100644 --- a/bin/light-base/src/sync_service.rs +++ b/bin/light-base/src/sync_service.rs @@ -613,7 +613,8 @@ impl StorageQueryError { self.errors.iter().all(|err| match err { StorageQueryErrorDetail::Network( network_service::StorageProofRequestError::Request( - service::StorageProofRequestError::Request(_), + service::StorageProofRequestError::Request(_) + | service::StorageProofRequestError::RemoteCouldntAnswer, ), ) | StorageQueryErrorDetail::Network( @@ -624,10 +625,6 @@ impl StorageQueryError { service::StorageProofRequestError::Decode(_), ), ) => false, - // TODO: as a temporary hack, we consider `TrieRootNotFound` as the remote not knowing about the requested block; see https://github.com/paritytech/substrate/pull/8046 - StorageQueryErrorDetail::ProofVerification(proof_decode::Error::TrieRootNotFound) => { - true - } StorageQueryErrorDetail::ProofVerification(_) | StorageQueryErrorDetail::MissingProofEntry => false, }) diff --git a/bin/wasm-node/CHANGELOG.md b/bin/wasm-node/CHANGELOG.md index 2ee2c2ec3e..032032ba76 100644 --- a/bin/wasm-node/CHANGELOG.md +++ b/bin/wasm-node/CHANGELOG.md @@ -7,6 +7,10 @@ - In earlier versions of smoldot, `setTimeout(callback, 0)` was frequently used in order split execution of CPU-intensive tasks in multiple smaller ones while still giving back control to the execution environment (such as NodeJS or the browser). Unfortunately, when a web page is in the background, browsers set a minimum delay of one second for `setTimeout`. For this reason, the usage of ̀`setTimeout` has now been reduced to the strict minimum, except when the environment is browser and `document.visibilityState` is equal to `visible`. ([#2999](https://github.com/paritytech/smoldot/pull/2999)) - Optimize the Merkle proof verification. The complexity has been reduced from `O(n^2)` to `O(n * log n)`. ([#3013](https://github.com/paritytech/smoldot/pull/3013)) +### Fixed + +- Fix `ProtobufDecode` errors appearing while the Grandpa warp syncing is still in progress. ([#3018](https://github.com/paritytech/smoldot/pull/3018)) + ## 0.7.7 - 2022-11-11 ### Added diff --git a/src/network/protocol/storage_call_proof.rs b/src/network/protocol/storage_call_proof.rs index ab04462946..cfc1b85724 100644 --- a/src/network/protocol/storage_call_proof.rs +++ b/src/network/protocol/storage_call_proof.rs @@ -94,29 +94,34 @@ pub fn build_call_proof_request<'a>( /// Decodes a response to a storage proof request or a call proof request. /// -/// On success, contains a list of Merkle proof entries. +/// On success, contains a list of Merkle proof entries, or `None` if the remote couldn't answer +/// the request. pub fn decode_storage_or_call_proof_response( ty: StorageOrCallProof, response_bytes: &[u8], -) -> Result, DecodeStorageCallProofResponseError> { +) -> Result>, DecodeStorageCallProofResponseError> { let field_num = match ty { StorageOrCallProof::CallProof => 1, StorageOrCallProof::StorageProof => 2, }; + // TODO: while the `proof` field is correctly optional, the `response` field isn't supposed to be optional; make it `#[required]` again once https://github.com/paritytech/substrate/pull/12732 has been merged and released + let mut parser = nom::combinator::all_consuming::<_, _, nom::error::Error<&[u8]>, _>( nom::combinator::complete(protobuf::message_decode! { - #[required] response = field_num => protobuf::message_tag_decode(protobuf::message_decode!{ - #[required] proof = 2 => protobuf::bytes_tag_decode + #[optional] response = field_num => protobuf::message_tag_decode(protobuf::message_decode!{ + #[optional] proof = 2 => protobuf::bytes_tag_decode }), }), ); - let proof: &[u8] = match nom::Finish::finish(parser(response_bytes)) { - Ok((_, out)) => out.response.proof, + let proof = match nom::Finish::finish(parser(response_bytes)) { + Ok((_, out)) => out.response.and_then(|r| r.proof), Err(_) => return Err(DecodeStorageCallProofResponseError::ProtobufDecode), }; + let Some(proof) = proof else { return Ok(None) }; + // The proof itself is a SCALE-encoded `Vec>`. let (_, decoded) = nom::combinator::all_consuming(nom::combinator::flat_map( crate::util::nom_scale_compact_usize, @@ -126,7 +131,7 @@ pub fn decode_storage_or_call_proof_response( DecodeStorageCallProofResponseError::ProofDecodeError })?; - Ok(decoded) + Ok(Some(decoded)) } /// Error potentially returned by [`decode_storage_or_call_proof_response`]. diff --git a/src/network/service/requests_responses.rs b/src/network/service/requests_responses.rs index 158acfd9ca..d5d344c5dd 100644 --- a/src/network/service/requests_responses.rs +++ b/src/network/service/requests_responses.rs @@ -186,16 +186,16 @@ where let response = response .map_err(StorageProofRequestError::Request) .and_then(|payload| { - if let Err(err) = protocol::decode_storage_or_call_proof_response( + match protocol::decode_storage_or_call_proof_response( protocol::StorageOrCallProof::StorageProof, &payload, ) { - Err(StorageProofRequestError::Decode(err)) - } else { - Ok(EncodedMerkleProof( + Err(err) => Err(StorageProofRequestError::Decode(err)), + Ok(None) => Err(StorageProofRequestError::RemoteCouldntAnswer), + Ok(Some(_)) => Ok(EncodedMerkleProof( payload, protocol::StorageOrCallProof::StorageProof, - )) + )), } }); @@ -208,19 +208,19 @@ where let response = response .map_err(CallProofRequestError::Request) - .and_then(|payload| { - if let Err(err) = protocol::decode_storage_or_call_proof_response( + .and_then( + |payload| match protocol::decode_storage_or_call_proof_response( protocol::StorageOrCallProof::CallProof, &payload, ) { - Err(CallProofRequestError::Decode(err)) - } else { - Ok(EncodedMerkleProof( + Err(err) => Err(CallProofRequestError::Decode(err)), + Ok(None) => Err(CallProofRequestError::RemoteCouldntAnswer), + Ok(Some(_)) => Ok(EncodedMerkleProof( payload, protocol::StorageOrCallProof::CallProof, - )) - } - }); + )), + }, + ); Event::RequestResult { request_id, @@ -909,7 +909,9 @@ pub struct EncodedMerkleProof(Vec, protocol::StorageOrCallProof); impl EncodedMerkleProof { /// Returns the decoded version of the proof. pub fn decode(&self) -> Vec<&[u8]> { - protocol::decode_storage_or_call_proof_response(self.1, &self.0).unwrap() + protocol::decode_storage_or_call_proof_response(self.1, &self.0) + .unwrap() + .unwrap() } } @@ -1040,6 +1042,8 @@ pub enum StorageProofRequestError { Request(peers::RequestError), #[display(fmt = "Response decoding error: {}", _0)] Decode(protocol::DecodeStorageCallProofResponseError), + /// The remote is incapable of answering this specific request. + RemoteCouldntAnswer, } /// Error returned by [`ChainNetwork::start_call_proof_request`]. @@ -1049,6 +1053,8 @@ pub enum CallProofRequestError { Request(peers::RequestError), #[display(fmt = "Response decoding error: {}", _0)] Decode(protocol::DecodeStorageCallProofResponseError), + /// The remote is incapable of answering this specific request. + RemoteCouldntAnswer, } impl CallProofRequestError { @@ -1058,6 +1064,7 @@ impl CallProofRequestError { match self { CallProofRequestError::Request(_) => true, CallProofRequestError::Decode(_) => false, + CallProofRequestError::RemoteCouldntAnswer => true, } } }