Repository: paradigmxyz/reth (language: rust)

Issue:
Return RecoveredBlock in ensure_well_formed_payload
### Describe the feature

This currently only returns the sealedblock:

https://github.com/paradigmxyz/reth/blob/81cff4a90b00cd5fbbb1ee122611266c14e86ed4/crates/engine/primitives/src/lib.rs#L145-L148

it would be nice if this returns the recoveredblock right away:

which would make this function redundant:

https://github.com/paradigmxyz/reth/blob/81cff4a90b00cd5fbbb1ee122611266c14e86ed4/crates/engine/tree/src/tree/mod.rs#L2353-L2361

## TODO
* change `SealedBlock` to `RecoveredBlock` by using `block.try_recover()`
* simplify callsite, remove `insert_block_without_senders` and use https://github.com/paradigmxyz/reth/blob/81cff4a90b00cd5fbbb1ee122611266c14e86ed4/crates/engine/tree/src/tree/mod.rs#L2363-L2363 directly

### Additional context

_No response_


Discussion / hints:
Can I take this one?
@mattsse can i take this one?

assigned @programskillforverification 

@VeerChaurasia I have a few more issues coming, perhaps #14617 ?

Fix patch:
diff --git a/crates/engine/primitives/src/lib.rs b/crates/engine/primitives/src/lib.rs
index ed67a491c390..e8d07df753dc 100644
--- a/crates/engine/primitives/src/lib.rs
+++ b/crates/engine/primitives/src/lib.rs
@@ -21,7 +21,7 @@ use reth_payload_primitives::{
     EngineObjectValidationError, InvalidPayloadAttributesError, NewPayloadError, PayloadAttributes,
     PayloadOrAttributes, PayloadTypes,
 };
-use reth_primitives::{NodePrimitives, SealedBlock};
+use reth_primitives::{NodePrimitives, RecoveredBlock, SealedBlock};
 use reth_primitives_traits::Block;
 use serde::{de::DeserializeOwned, Serialize};
 
@@ -145,7 +145,7 @@ pub trait PayloadValidator: fmt::Debug + Send + Sync + Unpin + 'static {
     fn ensure_well_formed_payload(
         &self,
         payload: Self::ExecutionData,
-    ) -> Result<SealedBlock<Self::Block>, NewPayloadError>;
+    ) -> Result<RecoveredBlock<Self::Block>, NewPayloadError>;
 }
 
 /// Type that validates the payloads processed by the engine.
diff --git a/crates/engine/tree/src/tree/mod.rs b/crates/engine/tree/src/tree/mod.rs
index ca36a69a96bf..0155bc852def 100644
--- a/crates/engine/tree/src/tree/mod.rs
+++ b/crates/engine/tree/src/tree/mod.rs
@@ -919,7 +919,7 @@ where
         let status = if self.backfill_sync_state.is_idle() {
             let mut latest_valid_hash = None;
             let num_hash = block.num_hash();
-            match self.insert_block_without_senders(block) {
+            match self.insert_block(block) {
                 Ok(status) => {
                     let status = match status {
                         InsertPayloadOk::Inserted(BlockStatus::Valid) => {
@@ -942,7 +942,7 @@ where
                 }
                 Err(error) => self.on_insert_block_error(error)?,
             }
-        } else if let Err(error) = self.buffer_block_without_senders(block) {
+        } else if let Err(error) = self.buffer_block(block) {
             self.on_insert_block_error(error)?
         } else {
             PayloadStatus::from_status(PayloadStatusEnum::Syncing)
@@ -1991,19 +1991,6 @@ where
         Ok(())
     }
 
-    /// Attempts to recover the block's senders and then buffers it.
-    ///
-    /// Returns an error if sender recovery failed or inserting into the buffer failed.
-    fn buffer_block_without_senders(
-        &mut self,
-        block: SealedBlock<N::Block>,
-    ) -> Result<(), InsertBlockError<N::Block>> {
-        match block.try_recover() {
-            Ok(block) => self.buffer_block(block),
-            Err(err) => Err(InsertBlockError::sender_recovery_error(err.into_inner())),
-        }
-    }
-
     /// Pre-validates the block and inserts it into the buffer.
     fn buffer_block(
         &mut self,
@@ -2350,16 +2337,6 @@ where
         self.most_recent_cache.take_if(|cache| cache.executed_block_hash() == parent_hash)
     }
 
-    fn insert_block_without_senders(
-        &mut self,
-        block: SealedBlock<N::Block>,
-    ) -> Result<InsertPayloadOk, InsertBlockError<N::Block>> {
-        match block.try_recover() {
-            Ok(block) => self.insert_block(block),
-            Err(err) => Err(InsertBlockError::sender_recovery_error(err.into_inner())),
-        }
-    }
-
     fn insert_block(
         &mut self,
         block: RecoveredBlock<N::Block>,
@@ -3833,11 +3810,11 @@ mod tests {
         let s = include_str!("../../test-data/holesky/2.rlp");
         let data = Bytes::from_str(s).unwrap();
         let block = Block::decode(&mut data.as_ref()).unwrap();
-        let sealed = block.seal_slow();
+        let sealed = block.seal_slow().try_recover().unwrap();
 
         let mut test_harness = TestHarness::new(HOLESKY.clone());
 
-        let outcome = test_harness.tree.insert_block_without_senders(sealed.clone()).unwrap();
+        let outcome = test_harness.tree.insert_block(sealed.clone()).unwrap();
         assert_eq!(
             outcome,
             InsertPayloadOk::Inserted(BlockStatus::Disconnected {
diff --git a/crates/ethereum/engine-primitives/src/lib.rs b/crates/ethereum/engine-primitives/src/lib.rs
index a8eec97bd3d3..644da880acd0 100644
--- a/crates/ethereum/engine-primitives/src/lib.rs
+++ b/crates/ethereum/engine-primitives/src/lib.rs
@@ -26,7 +26,7 @@ use reth_payload_primitives::{
     EngineObjectValidationError, NewPayloadError, PayloadOrAttributes, PayloadTypes,
 };
 use reth_payload_validator::ExecutionPayloadValidator;
-use reth_primitives::{Block, NodePrimitives, SealedBlock};
+use reth_primitives::{Block, NodePrimitives, RecoveredBlock, SealedBlock};
 
 /// The types used in the default mainnet ethereum beacon consensus engine.
 #[derive(Debug, Default, Clone, serde::Deserialize, serde::Serialize)]
@@ -104,8 +104,9 @@ impl PayloadValidator for EthereumEngineValidator {
     fn ensure_well_formed_payload(
         &self,
         payload: ExecutionData,
-    ) -> Result<SealedBlock, NewPayloadError> {
-        Ok(self.inner.ensure_well_formed_payload(payload)?)
+    ) -> Result<RecoveredBlock<Self::Block>, NewPayloadError> {
+        let sealed_block = self.inner.ensure_well_formed_payload(payload)?;
+        sealed_block.try_recover().map_err(|e| NewPayloadError::Other(e.into()))
     }
 }
 
diff --git a/crates/optimism/node/src/engine.rs b/crates/optimism/node/src/engine.rs
index 9fb9fc3c5de5..4b12008cd8c2 100644
--- a/crates/optimism/node/src/engine.rs
+++ b/crates/optimism/node/src/engine.rs
@@ -19,7 +19,7 @@ use reth_optimism_forks::{OpHardfork, OpHardforks};
 use reth_optimism_payload_builder::{OpBuiltPayload, OpPayloadBuilderAttributes};
 use reth_optimism_primitives::{OpBlock, OpPrimitives};
 use reth_payload_validator::ExecutionPayloadValidator;
-use reth_primitives::SealedBlock;
+use reth_primitives::{RecoveredBlock, SealedBlock};
 use std::sync::Arc;
 
 /// The types used in the optimism beacon consensus engine.
@@ -97,8 +97,9 @@ impl PayloadValidator for OpEngineValidator {
     fn ensure_well_formed_payload(
         &self,
         payload: ExecutionData,
-    ) -> Result<SealedBlock<Self::Block>, NewPayloadError> {
-        Ok(self.inner.ensure_well_formed_payload(payload)?)
+    ) -> Result<RecoveredBlock<Self::Block>, NewPayloadError> {
+        let sealed_block = self.inner.ensure_well_formed_payload(payload)?;
+        sealed_block.try_recover().map_err(|e| NewPayloadError::Other(e.into()))
     }
 }
 
diff --git a/crates/rpc/rpc/src/validation.rs b/crates/rpc/rpc/src/validation.rs
index d1791984deae..76d9ade3a780 100644
--- a/crates/rpc/rpc/src/validation.rs
+++ b/crates/rpc/rpc/src/validation.rs
@@ -356,17 +356,13 @@ where
         &self,
         request: BuilderBlockValidationRequestV3,
     ) -> Result<(), ValidationApiError> {
-        let block = self
-            .payload_validator
-            .ensure_well_formed_payload(ExecutionData {
-                payload: ExecutionPayload::V3(request.request.execution_payload),
-                sidecar: ExecutionPayloadSidecar::v3(CancunPayloadFields {
-                    parent_beacon_block_root: request.parent_beacon_block_root,
-                    versioned_hashes: self.validate_blobs_bundle(request.request.blobs_bundle)?,
-                }),
-            })?
-            .try_recover()
-            .map_err(|_| ValidationApiError::InvalidTransactionSignature)?;
+        let block = self.payload_validator.ensure_well_formed_payload(ExecutionData {
+            payload: ExecutionPayload::V3(request.request.execution_payload),
+            sidecar: ExecutionPayloadSidecar::v3(CancunPayloadFields {
+                parent_beacon_block_root: request.parent_beacon_block_root,
+                versioned_hashes: self.validate_blobs_bundle(request.request.blobs_bundle)?,
+            }),
+        })?;
 
         self.validate_message_against_block(
             block,
@@ -381,25 +377,20 @@ where
         &self,
         request: BuilderBlockValidationRequestV4,
     ) -> Result<(), ValidationApiError> {
-        let block = self
-            .payload_validator
-            .ensure_well_formed_payload(ExecutionData {
-                payload: ExecutionPayload::V3(request.request.execution_payload),
-                sidecar: ExecutionPayloadSidecar::v4(
-                    CancunPayloadFields {
-                        parent_beacon_block_root: request.parent_beacon_block_root,
-                        versioned_hashes: self
-                            .validate_blobs_bundle(request.request.blobs_bundle)?,
-                    },
-                    PraguePayloadFields {
-                        requests: RequestsOrHash::Requests(
-                            request.request.execution_requests.to_requests(),
-                        ),
-                    },
-                ),
-            })?
-            .try_recover()
-            .map_err(|_| ValidationApiError::InvalidTransactionSignature)?;
+        let block = self.payload_validator.ensure_well_formed_payload(ExecutionData {
+            payload: ExecutionPayload::V3(request.request.execution_payload),
+            sidecar: ExecutionPayloadSidecar::v4(
+                CancunPayloadFields {
+                    parent_beacon_block_root: request.parent_beacon_block_root,
+                    versioned_hashes: self.validate_blobs_bundle(request.request.blobs_bundle)?,
+                },
+                PraguePayloadFields {
+                    requests: RequestsOrHash::Requests(
+                        request.request.execution_requests.to_requests(),
+                    ),
+                },
+            ),
+        })?;
 
         self.validate_message_against_block(
             block,
diff --git a/examples/custom-engine-types/src/main.rs b/examples/custom-engine-types/src/main.rs
index 50bc60f801ea..efaa8de207da 100644
--- a/examples/custom-engine-types/src/main.rs
+++ b/examples/custom-engine-types/src/main.rs
@@ -38,7 +38,7 @@ use reth::{
     },
     network::NetworkHandle,
     payload::ExecutionPayloadValidator,
-    primitives::{Block, EthPrimitives, SealedBlock, TransactionSigned},
+    primitives::{Block, EthPrimitives, RecoveredBlock, SealedBlock, TransactionSigned},
     providers::{EthStorage, StateProviderFactory},
     rpc::{eth::EthApi, types::engine::ExecutionPayload},
     tasks::TaskManager,
@@ -205,8 +205,9 @@ impl PayloadValidator for CustomEngineValidator {
     fn ensure_well_formed_payload(
         &self,
         payload: ExecutionData,
-    ) -> Result<SealedBlock<Self::Block>, NewPayloadError> {
-        Ok(self.inner.ensure_well_formed_payload(payload)?)
+    ) -> Result<RecoveredBlock<Self::Block>, NewPayloadError> {
+        let sealed_block = self.inner.ensure_well_formed_payload(payload)?;
+        sealed_block.try_recover().map_err(|e| NewPayloadError::Other(e.into()))
     }
 }
 


Test patch: