refactor(app): Extract payload validation utilities and reduce code duplication #199
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| pub mod app; | ||
| mod metrics; | ||
| pub mod node; | ||
| mod payload; | ||
| pub mod state; | ||
| mod store; | ||
| mod streaming; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,157 @@ | ||
| //! Execution payload utilities for validation, caching, and manipulation. | ||
|
|
||
| use alloy_rpc_types_engine::{ExecutionPayloadV1, ExecutionPayloadV2, ExecutionPayloadV3}; | ||
| use bytes::Bytes; | ||
| use caches::lru::AdaptiveCache; | ||
| use caches::Cache; | ||
| use color_eyre::eyre::{self, eyre}; | ||
| use malachitebft_app_channel::app::types::core::{Round, Validity}; | ||
| use malachitebft_eth_engine::engine::Engine; | ||
| use malachitebft_eth_engine::json_structures::ExecutionPayloadBodyV1; | ||
| use malachitebft_eth_types::{Block, BlockHash, Height, RetryConfig}; | ||
| use ssz::Decode; | ||
| use tracing::{debug, error, warn}; | ||
|
|
||
| /// Cache for tracking recently validated execution payloads to avoid redundant validation. | ||
| /// Stores both the block hash and its validity result (Valid or Invalid). | ||
| pub struct ValidatedPayloadCache { | ||
| cache: AdaptiveCache<BlockHash, Validity>, | ||
| } | ||
|
|
||
| impl ValidatedPayloadCache { | ||
| pub fn new(max_size: usize) -> Self { | ||
| Self { | ||
| cache: AdaptiveCache::new(max_size) | ||
| .expect("Failed to create AdaptiveCache: invalid cache size"), | ||
| } | ||
| } | ||
|
|
||
| /// Check if a block hash has been validated and return its cached validity | ||
| pub fn get(&mut self, block_hash: &BlockHash) -> Option<Validity> { | ||
| self.cache.get(block_hash).copied() | ||
| } | ||
|
|
||
| /// Insert a block hash and its validity result into the cache | ||
| pub fn insert(&mut self, block_hash: BlockHash, validity: Validity) { | ||
| self.cache.put(block_hash, validity); | ||
| } | ||
| } | ||
|
|
||
| /// Validates execution payload bytes with the execution engine. | ||
| /// Decodes the payload, extracts versioned hashes, and validates. | ||
| /// Uses cache to avoid duplicate validation calls. | ||
| /// | ||
| /// Returns `Ok(Validity::Invalid)` if decoding fails or payload is invalid, | ||
| /// `Ok(Validity::Valid)` if valid, or `Err` for engine communication failures. | ||
| pub async fn validate_execution_payload( | ||
| cache: &mut ValidatedPayloadCache, | ||
| data: &Bytes, | ||
| height: Height, | ||
| round: Round, | ||
| engine: &Engine, | ||
| retry_config: &RetryConfig, | ||
| ) -> eyre::Result<Validity> { | ||
| // Decode execution payload | ||
| let execution_payload = match ExecutionPayloadV3::from_ssz_bytes(data) { | ||
| Ok(payload) => payload, | ||
| Err(e) => { | ||
| warn!( | ||
| height = %height, | ||
| round = %round, | ||
| error = ?e, | ||
| "Proposal has invalid ExecutionPayloadV3 encoding" | ||
| ); | ||
| return Ok(Validity::Invalid); | ||
| } | ||
| }; | ||
|
|
||
| let block_hash = execution_payload.payload_inner.payload_inner.block_hash; | ||
|
|
||
| // Check if we've already validated this block | ||
| if let Some(cached_validity) = cache.get(&block_hash) { | ||
| debug!( | ||
| %height, %round, %block_hash, validity = ?cached_validity, | ||
| "Skipping duplicate newPayload call, returning cached result" | ||
| ); | ||
| return Ok(cached_validity); | ||
| } | ||
|
|
||
| // Extract versioned hashes for blob transactions | ||
| let block: Block = match execution_payload.clone().try_into_block() { | ||
| Ok(block) => block, | ||
| Err(e) => { | ||
| warn!( | ||
| height = %height, | ||
| round = %round, | ||
| error = ?e, | ||
| "Failed to convert ExecutionPayloadV3 to Block" | ||
| ); | ||
| return Ok(Validity::Invalid); | ||
| } | ||
| }; | ||
| let versioned_hashes: Vec<BlockHash> = | ||
| block.body.blob_versioned_hashes_iter().copied().collect(); | ||
|
|
||
| // Validate with execution engine | ||
| let payload_status = engine | ||
| .notify_new_block_with_retry(execution_payload, versioned_hashes, retry_config) | ||
| .await | ||
| .map_err(|e| { | ||
| eyre!( | ||
| "Execution client stuck in SYNCING for {:?} at height {}: {}", | ||
| retry_config.max_elapsed_time, | ||
| height, | ||
| e | ||
| ) | ||
| })?; | ||
|
|
||
| let validity = if payload_status.status.is_valid() { | ||
| Validity::Valid | ||
| } else { | ||
| // INVALID or ACCEPTED - both are treated as invalid | ||
| // INVALID: malicious block | ||
| // ACCEPTED: Non-canonical payload - should not happen with instant finality | ||
| error!(%height, %round, "Block validation failed: {}", payload_status.status); | ||
| Validity::Invalid | ||
| }; | ||
|
|
||
| cache.insert(block_hash, validity); | ||
|
Member
There was a problem hiding this comment. we call I would prefer to manage cache outside this method.
Contributor
Author
There was a problem hiding this comment. I think this is just an optimization that would avoid caching the validation result for the proposer. We could add a param to indicate if the validation result should be cached, but I think it just adds complexity for very little benefit. |
||
| Ok(validity) | ||
| } | ||
|
|
||
| /// Extracts a block header from an ExecutionPayloadV3 by removing transactions and withdrawals. | ||
| /// | ||
| /// Returns an ExecutionPayloadV3 with empty transactions and withdrawals vectors, | ||
| /// containing only the block header fields. | ||
| pub fn extract_block_header(payload: &ExecutionPayloadV3) -> ExecutionPayloadV3 { | ||
| ExecutionPayloadV3 { | ||
| payload_inner: ExecutionPayloadV2 { | ||
| payload_inner: ExecutionPayloadV1 { | ||
| transactions: vec![], | ||
| ..payload.payload_inner.payload_inner.clone() | ||
| }, | ||
| withdrawals: vec![], | ||
| }, | ||
| ..payload.clone() | ||
| } | ||
| } | ||
|
|
||
| /// Reconstructs a complete ExecutionPayloadV3 from a block header and payload body. | ||
| /// | ||
| /// Takes a header (ExecutionPayloadV3 with empty transactions/withdrawals) and combines it | ||
| /// with the transactions and withdrawals from an ExecutionPayloadBodyV1 to create a full payload. | ||
| pub fn reconstruct_execution_payload( | ||
| header: ExecutionPayloadV3, | ||
| body: ExecutionPayloadBodyV1, | ||
| ) -> ExecutionPayloadV3 { | ||
| ExecutionPayloadV3 { | ||
| payload_inner: ExecutionPayloadV2 { | ||
| payload_inner: ExecutionPayloadV1 { | ||
| transactions: body.transactions, | ||
| ..header.payload_inner.payload_inner | ||
| }, | ||
| withdrawals: body.withdrawals.unwrap_or_default(), | ||
| }, | ||
| ..header | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also add back the hash ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really as we no longer have the block hash. We could get it by extracting the execution payload from the block_bytes and then use
execution_payload.payload_inner.payload_inner.block_hash, but I don't see the benefit of logging the block hash in a debug message.