Conversation
| return Ok(self.forkchoice_response_invalid()); | ||
| } | ||
|
|
||
| if !self.is_descendant(self.last_valid_fcs.finalized_block_hash, finalized) { |
There was a problem hiding this comment.
Isn't this logic that the CL should be doing? It seems like we are redoing logic related to forks that we probably can just get from the CLs fork choice impl for free
There was a problem hiding this comment.
As far as I can see, there are no execution block hash ancestry checks in the CL specs or code. These checks are delegated to the engineAPI. I think we could make some changes to the specs to mitigate this requirement, but that would be more invasive (spec changes) than using the existing decoupling and separation of concerns. I would propose deferring those changes to the mandatory proofs hardfork.
There was a problem hiding this comment.
My assumption here is that the CL has forking logic that is used to select the "correct" beacon block and every beacon block corresponds to an execution block hash -- can this not be re-used or are you solving a different problem?
| get_blobs_v1: false, | ||
| get_blobs_v2: false, |
There was a problem hiding this comment.
May be good to add a comment on this and or make it conditional for optional proofs, since normal CLs will have an EL attached
There was a problem hiding this comment.
From what I can see, DEFAULT_ENGINE_CAPABILITIES is primarily used in MockServer, which does not support get_blobs_* methods.
| current_slot: Slot, | ||
| ) { | ||
| // TODO: Optimise this so we don't have to clone. | ||
| let beacon_block = Arc::unwrap_or_clone(signed_block.clone()); |
There was a problem hiding this comment.
ack on comment; since we are now cloning on every block import
There was a problem hiding this comment.
I will try to refactor this code to remove this requirement.
| }; | ||
|
|
||
| let proof_engine_result = if let Some(proof_engine) = self.proof_engine() { | ||
| Some(Ok(proof_engine.new_payload(&new_payload_request).await?)) |
There was a problem hiding this comment.
Even if execution engine doesn't error, wouldn't this ? cause an early return if proof_engine errors, so we never get to:
let result = engine_result
.or(proof_engine_result)
.expect("at least one of engine or proof engine must be present");?
There was a problem hiding this comment.
Good catch. I was using ? for ergonomic proof conversion, but you are correct that this results in early return. I will update.
2 participants
Issue Addressed
Which issue # does this PR address?
Proposed Changes
Please list or describe the changes introduced by this PR.
Additional Info
Please provide any additional information. For example, future considerations
or information useful for reviewers.