From 58c56edd64d0b1e6ce288f8e291f9769ed899313 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 20 Sep 2022 20:05:48 +0200 Subject: [PATCH] Remove unwrap() in runtime_lock (#2764) Fix https://github.com/paritytech/smoldot/issues/2763 This PR gives a proper error type to the `state_trie_root_hash` function (which retrieves the state trie root hash of a block from the network), and makes `runtime_lock` properly propagates the error instead of unwrapping. --- bin/light-base/src/json_rpc_service.rs | 51 ++++++++++++++----- .../src/json_rpc_service/state_chain.rs | 8 +-- bin/wasm-node/CHANGELOG.md | 1 + 3 files changed, 44 insertions(+), 16 deletions(-) diff --git a/bin/light-base/src/json_rpc_service.rs b/bin/light-base/src/json_rpc_service.rs index 4cfbd2fabf..0aa54b1823 100644 --- a/bin/light-base/src/json_rpc_service.rs +++ b/bin/light-base/src/json_rpc_service.rs @@ -532,7 +532,11 @@ struct Cache { /// trie root hash multiple, we store these hashes in this LRU cache. block_state_root_hashes_numbers: lru::LruCache< [u8; 32], - future::MaybeDone>>>, + future::MaybeDone< + future::Shared< + future::BoxFuture<'static, Result<([u8; 32], u64), StateTrieRootHashError>>, + >, + >, fnv::FnvBuildHasher, >, } @@ -1206,8 +1210,10 @@ impl Background { /// Obtain the state trie root hash and number of the given block, and make sure to put it /// in cache. - // TODO: better error return type - async fn state_trie_root_hash(&self, hash: &[u8; 32]) -> Result<([u8; 32], u64), ()> { + async fn state_trie_root_hash( + &self, + hash: &[u8; 32], + ) -> Result<([u8; 32], u64), StateTrieRootHashError> { let fetch = { // Try to find an existing entry in cache, and if not create one. let mut cache_lock = self.cache.lock().await; @@ -1219,7 +1225,7 @@ impl Background { .map(|h| header::decode(h, self.sync_service.block_number_bytes())) { Some(Ok(header)) => return Ok((*header.state_root, header.number)), - Some(Err(_)) => return Err(()), + Some(Err(err)) => return Err(StateTrieRootHashError::HeaderDecodeError(err)), // TODO: can this actually happen? unclear None => {} } @@ -1228,8 +1234,14 @@ impl Background { Some(future::MaybeDone::Done(Ok(val))) => return Ok(*val), Some(future::MaybeDone::Future(f)) => f.clone(), Some(future::MaybeDone::Gone) => unreachable!(), // We never use `Gone`. - Some(future::MaybeDone::Done(Err(()))) | None => { - // TODO: filter by error ^ ; invalid header for example should be returned immediately + Some(future::MaybeDone::Done(Err( + err @ StateTrieRootHashError::HeaderDecodeError(_), + ))) => { + // In case of a fatal error, return immediately. + return Err(err.clone()); + } + Some(future::MaybeDone::Done(Err(StateTrieRootHashError::NetworkQueryError))) + | None => { // No existing cache entry. Create the future that will perform the fetch // but do not actually start doing anything now. let fetch = { @@ -1266,7 +1278,8 @@ impl Background { .unwrap(); Ok((*decoded.state_root, decoded.number)) } else { - Err(()) + // TODO: better error details? + Err(StateTrieRootHashError::NetworkQueryError) } } }; @@ -1297,7 +1310,7 @@ impl Background { let (state_trie_root_hash, block_number) = self .state_trie_root_hash(&hash) .await - .map_err(|()| StorageQueryError::FindStorageRootHashError)?; + .map_err(StorageQueryError::FindStorageRootHashError)?; let result = self .sync_service @@ -1350,8 +1363,10 @@ impl Background { // In order to grab the runtime code and perform the call network request, we need // to know the state trie root hash and the height of the block. - let (state_trie_root_hash, block_number) = - self.state_trie_root_hash(block_hash).await.unwrap(); // TODO: don't unwrap + let (state_trie_root_hash, block_number) = self + .state_trie_root_hash(block_hash) + .await + .map_err(RuntimeCallError::FindStorageRootHashError)?; // Download the runtime of this block. This takes a long time as the runtime is rather // big (around 1MiB in general). @@ -1485,8 +1500,8 @@ impl Background { #[derive(Debug, derive_more::Display)] enum StorageQueryError { /// Error while finding the storage root hash of the requested block. - #[display(fmt = "Unknown block")] - FindStorageRootHashError, + #[display(fmt = "Failed to obtain block state trie root: {}", _0)] + FindStorageRootHashError(StateTrieRootHashError), /// Error while retrieving the storage item from other nodes. #[display(fmt = "{}", _0)] StorageRetrieval(sync_service::StorageQueryError), @@ -1495,8 +1510,20 @@ enum StorageQueryError { // TODO: doc and properly derive Display #[derive(Debug, derive_more::Display, Clone)] enum RuntimeCallError { + /// Error while finding the storage root hash of the requested block. + #[display(fmt = "Failed to obtain block state trie root: {}", _0)] + FindStorageRootHashError(StateTrieRootHashError), Call(runtime_service::RuntimeCallError), StartError(host::StartErr), ReadOnlyRuntime(read_only_runtime_host::ErrorDetail), NextKeyForbidden, } + +/// Error potentially returned by [`Background::state_trie_root_hash`]. +#[derive(Debug, derive_more::Display, Clone)] +enum StateTrieRootHashError { + /// Failed to decode block header. + HeaderDecodeError(header::Error), + /// Error while fetching block header from network. + NetworkQueryError, +} diff --git a/bin/light-base/src/json_rpc_service/state_chain.rs b/bin/light-base/src/json_rpc_service/state_chain.rs index 846b3ceb74..d8008f72fc 100644 --- a/bin/light-base/src/json_rpc_service/state_chain.rs +++ b/bin/light-base/src/json_rpc_service/state_chain.rs @@ -960,7 +960,7 @@ impl Background { // This is necessary to perform network storage queries. let (state_root, block_number) = match self.state_trie_root_hash(&hash).await { Ok(v) => v, - Err(()) => { + Err(err) => { self.requests_subscriptions .respond( &state_machine_request_id, @@ -968,7 +968,7 @@ impl Background { request_id, json_rpc::parse::ErrorResponse::ServerError( -32000, - &"Failed to fetch block information", + &format!("Failed to fetch block information: {}", err), ), None, ), @@ -1031,7 +1031,7 @@ impl Background { // This is necessary to perform network storage queries. let (state_root, block_number) = match self.state_trie_root_hash(&hash).await { Ok(v) => v, - Err(()) => { + Err(err) => { self.requests_subscriptions .respond( &state_machine_request_id, @@ -1039,7 +1039,7 @@ impl Background { request_id, json_rpc::parse::ErrorResponse::ServerError( -32000, - &"Failed to fetch block information", + &format!("Failed to fetch block information: {}", err), ), None, ), diff --git a/bin/wasm-node/CHANGELOG.md b/bin/wasm-node/CHANGELOG.md index 9667b90d29..67b1fb773f 100644 --- a/bin/wasm-node/CHANGELOG.md +++ b/bin/wasm-node/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed - Fix potential infinite loop in networking connection task. ([#2751](https://github.com/paritytech/smoldot/pull/2751)) +- Fix panic when trying to perform a runtime call on an old block while having no networking connection. ([#2764](https://github.com/paritytech/smoldot/pull/2764)) ### Changed