Check block requests response in higher level code instead

lib/src/sync/optimistic.rs

@@ -602,6 +602,8 @@ impl<TRq, TSrc, TBl> OptimisticSync<TRq, TSrc, TBl> {
             return (user_data, FinishRequestOutcome::Obsolete);
         }
 
+        // TODO: important /!\ should check whether the block bodies match the extrinsics root in the headers, in order to differentiate invalid blocks from malicious peers
+
         let ((_, user_data), source_id) = self
             .inner
             .verification_queue

light-base/src/json_rpc_service/background/chain_head.rs

@@ -877,12 +877,14 @@ impl<TPlat: PlatformRef> ChainHeadFollowTask<TPlat> {
             unreachable!()
         };
 
-        // Determine whether the requested block hash is valid, and if yes its number.
-        let block_number = {
+        // Determine whether the requested block hash is valid, and if yes its number and
+        // extrinsics trie root. The extrinsics trie root is used to verify whether the body we
+        // download is correct.
+        let (block_number, extrinsics_root) = {
             if let Some(header) = self.pinned_blocks_headers.get(&hash.0) {
                 let decoded =
                     header::decode(header, self.sync_service.block_number_bytes()).unwrap(); // TODO: unwrap?
-                decoded.number
+                (decoded.number, *decoded.extrinsics_root)
             } else {
                 // Block isn't pinned. Request is invalid.
                 request.fail(json_rpc::parse::ErrorResponse::InvalidParams);
@@ -949,20 +951,33 @@ impl<TPlat: PlatformRef> ChainHeadFollowTask<TPlat> {
                         None => return, // JSON-RPC client has unsubscribed in the meanwhile.
                     };
 
-                    match outcome {
-                        Ok(block_data) => {
+                    // We must check whether the body is present in the response and valid.
+                    // TODO: should try the request again with a different peer instead of failing immediately
+                    let body = match outcome {
+                        Ok(outcome) => {
+                            if let Some(body) = outcome.body {
+                                if header::extrinsics_root(&body) == extrinsics_root {
+                                    Ok(body)
+                                } else {
+                                    Err(())
+                                }
+                            } else {
+                                Err(())
+                            }
+                        }
+                        Err(err) => Err(err),
+                    };
+
+                    // Send back the response.
+                    match body {
+                        Ok(body) => {
                             let _ = to_main_task
                                 .send(OperationEvent {
                                     operation_id: operation_id.clone(),
                                     is_done: true,
                                     notification: methods::FollowEvent::OperationBodyDone {
                                         operation_id: operation_id.clone().into(),
-                                        value: block_data
-                                            .body
-                                            .unwrap()
-                                            .into_iter()
-                                            .map(methods::HexString)
-                                            .collect(),
+                                        value: body.into_iter().map(methods::HexString).collect(),
                                     },
                                 })
                                 .await;

light-base/src/json_rpc_service/background/state_chain.rs

@@ -123,8 +123,8 @@ impl<TPlat: PlatformRef> Background<TPlat> {
             rx.await.unwrap()
         };
 
-        // Block bodies and justifications aren't stored locally. Ask the network.
-        let result = if let Some(block_number) = block_number {
+        // Block bodies and headers aren't stored locally. Ask the network.
+        let mut result = if let Some(block_number) = block_number {
             self.sync_service
                 .clone()
                 .block_query(
@@ -133,7 +133,7 @@ impl<TPlat: PlatformRef> Background<TPlat> {
                     protocol::BlocksRequestFields {
                         header: true,
                         body: true,
-                        justifications: true,
+                        justifications: false,
                     },
                     3,
                     Duration::from_secs(8),
@@ -148,7 +148,7 @@ impl<TPlat: PlatformRef> Background<TPlat> {
                     protocol::BlocksRequestFields {
                         header: true,
                         body: true,
-                        justifications: true,
+                        justifications: false,
                     },
                     3,
                     Duration::from_secs(8),
@@ -157,9 +157,32 @@ impl<TPlat: PlatformRef> Background<TPlat> {
                 .await
         };
 
-        // The `block_query` function guarantees that the header and body are present and
-        // are correct.
+        // Check whether the header and body are present and valid.
+        // TODO: try the request again with a different peerin case the response is invalid, instead of returning null
+        if let Ok(block) = &result {
+            if let (Some(header), Some(body)) = (&block.header, &block.body) {
+                if header::hash_from_scale_encoded_header(header) == hash {
+                    if let Ok(decoded) =
+                        header::decode(header, self.sync_service.block_number_bytes())
+                    {
+                        if header::extrinsics_root(&body) != *decoded.extrinsics_root {
+                            result = Err(());
+                        }
+                    } else {
+                        // Note that if the header is undecodable it doesn't necessarily mean
+                        // that the header and/or body is bad, but given that we have no way to
+                        // check this we return an error.
+                        result = Err(());
+                    }
+                } else {
+                    result = Err(());
+                }
+            } else {
+                result = Err(());
+            }
+        }
 
+        // Return the response.
         if let Ok(block) = result {
             request.respond(methods::Response::chain_getBlock(methods::Block {
                 extrinsics: block
@@ -173,11 +196,9 @@ impl<TPlat: PlatformRef> Background<TPlat> {
                     self.sync_service.block_number_bytes(),
                 )
                 .unwrap(),
-                justifications: block.justifications.map(|list| {
-                    list.into_iter()
-                        .map(|j| (j.engine_id, j.justification))
-                        .collect()
-                }),
+                // There's no way to verify the correctness of the justifications, consequently
+                // we always return an empty list.
+                justifications: None,
             }))
         } else {
             request.respond_null()

light-base/src/transactions_service.rs

@@ -601,17 +601,20 @@ async fn background_task<TPlat: PlatformRef>(mut config: BackgroundTaskConfig<TP
                         block_hash,
                         protocol::BlocksRequestFields {
                             body: true,
-                            header: true, // TODO: must be true in order for the body to be verified; fix the sync_service to not require that
+                            header: true, // TODO: must be true in order to avoid an error being generated, fix this in sync service
                             justifications: false,
                         },
                         3,
                         Duration::from_secs(8),
                         NonZeroU32::new(3).unwrap(),
                     );
 
-                    Box::pin(
-                        async move { (block_hash, download_future.await.map(|b| b.body.unwrap())) },
-                    )
+                    Box::pin(async move {
+                        (
+                            block_hash,
+                            download_future.await.and_then(|b| b.body.ok_or(())),
+                        )
+                    })
                 });
 
                 worker
@@ -701,7 +704,7 @@ async fn background_task<TPlat: PlatformRef>(mut config: BackgroundTaskConfig<TP
 
                 download = worker.block_downloads.select_next_some() => {
                     // A block body download has finished, successfully or not.
-                    let (block_hash, block_body) = download;
+                    let (block_hash, mut block_body) = download;
 
                     let block = match worker.pending_transactions.block_user_data_mut(&block_hash) {
                         Some(b) => b,
@@ -714,8 +717,14 @@ async fn background_task<TPlat: PlatformRef>(mut config: BackgroundTaskConfig<TP
 
                     debug_assert!(block.downloading);
                     block.downloading = false;
-                    if block_body.is_err() {
-                        block.failed_downloads = block.failed_downloads.saturating_add(1);
+
+                    // Make sure that the downloaded body is the one of this block, otherwise
+                    // we consider the download as failed.
+                    if let Ok(body) = &block_body {
+                        // TODO: unwrap the decoding?! is that correct?
+                        if header::extrinsics_root(body) != *header::decode(&block.scale_encoded_header, worker.sync_service.block_number_bytes()).unwrap().extrinsics_root {
+                            block_body = Err(());
+                        }
                     }
 
                     if let Ok(block_body) = block_body {
@@ -743,6 +752,7 @@ async fn background_task<TPlat: PlatformRef>(mut config: BackgroundTaskConfig<TP
                         }
 
                     } else {
+                        block.failed_downloads = block.failed_downloads.saturating_add(1);
                         log::debug!(
                             target: &config.log_target,
                             "BlockDownloads => Failed(block={})",

wasm-node/CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+### Changed
+
+- The `chain_getBlock` JSON-RPC function now always returns an empty list of justifications, because there is no (reasonable) way for smoldot to verify whether the justifications sent by full nodes are valid. ([#1238](https://github.com/smol-dot/smoldot/pull/1238))
+
 ## 2.0.6 - 2023-10-13
 
 ### Fixed