feat: self validation of built payload #11359
Conversation
rkrasiuk
force-pushed
the
rkrasiuk/built-payload-self-validation
branch
from
53825f0
to
53a87d4
Compare
rkrasiuk
changed the title
rkrasiuk
added
C-enhancement
rkrasiuk
force-pushed
the
rkrasiuk/built-payload-self-validation
branch
from
53a87d4
to
56a9e40
Compare
| let mut new_payload_responses = futures::stream::FuturesUnordered::default(); | ||
| loop { | ||
| tokio::select! { | ||
| payload = built_payloads.select_next_some() => { | ||
| if let Some(executed_block) = payload.executed_block() { | ||
| debug!(target: "reth::cli", block=?executed_block.block().num_hash(), "inserting built payload"); | ||
| eth_service.orchestrator_mut().handler_mut().handler_mut().on_event(EngineApiRequest::InsertExecutedBlock(executed_block).into()); | ||
| let request = if payload_building_validation_enabled { | ||
| debug!(target: "reth::cli", block=?executed_block.block().num_hash(), "inserting built payload with validation"); | ||
| let (tx, rx) = oneshot::channel(); | ||
| new_payload_responses.push(rx); | ||
|
|
||
| let message = BeaconEngineMessage::NewPayload { |
There was a problem hiding this comment.
not a fan of this massive select arm, this blows up the entire thing and is hard to parse, this loop was intended to be easy to understand.
this feeds data from one select arm into another, which means this can be converted into a helper type instead
| /// Flag indicating whether the node should send locally-produced payload | ||
| /// back to consensus engine for validation. | ||
| #[arg(long = "debug.payload-building-validation", help_heading = "Debug")] | ||
| pub payload_building_validation: bool, |
There was a problem hiding this comment.
opt-in makes sense for eth, but for op this should always be enabled
There was a problem hiding this comment.
Forwarding payload is always enabled, this flag only controls whether the payload will be validated or not
| #[must_use = "futures do nothing unless you `.await` or poll them"] | ||
| pub(crate) struct EngineServiceTask<N, Client, E, Network> | ||
| where | ||
| N: EngineNodeTypes, | ||
| Client: BlockClient + 'static, | ||
| E: BlockExecutorProvider + 'static, | ||
| { |
There was a problem hiding this comment.
can we document this a bit, that this is basically the node's main loop
| } | ||
| } | ||
|
|
||
| fn on_chain_event(&self, event: ChainEvent<BeaconConsensusEngineEvent>) -> eyre::Result<()> { |
There was a problem hiding this comment.
I would also like some docs here that this is for reporting
| Ok(()) | ||
| } | ||
|
|
||
| fn on_built_executed_block(&mut self, executed_block: ExecutedBlock) { |
There was a problem hiding this comment.
docs :) and what this does
| let (tx, rx) = oneshot::channel(); | ||
| self.new_payload_responses.push(rx); | ||
|
|
||
| let message = BeaconEngineMessage::NewPayload { | ||
| payload: block_to_payload(executed_block.block.as_ref().clone()), | ||
| cancun_fields: executed_block.block.parent_beacon_block_root.map( | ||
| |parent_beacon_block_root| CancunPayloadFields { | ||
| parent_beacon_block_root, | ||
| versioned_hashes: executed_block | ||
| .block | ||
| .blob_versioned_hashes_iter() | ||
| .copied() | ||
| .collect(), | ||
| }, | ||
| ), | ||
| tx, | ||
| }; | ||
| EngineApiRequest::Beacon(message) |
There was a problem hiding this comment.
this is the only thing that changed right?
we add it as a different request variant and log the response
imo we could make this a lot simpler if we do this with the main version and simply spawn a new task that awaits this response and logs it, because I really prefer the loop select! for this
github-actions
bot
added
the
S-stale
github-actions
bot
removed
the
S-stale
Description
Introduce new CLI argument
--debug.payload-building-validationthat enables forwarding built payloads back to consensus engine for validation.