feat(sidecar): fallback block builder refactor #512
Conversation
merklefruit
force-pushed
the
feat/nethermind-fallback
branch
from
a4b390a to
1b25ad4
Compare
merklefruit
force-pushed
the
feat/nethermind-fallback
branch
from
d8e7713 to
c878559
Compare
| let validation_error = match payload_status.status { | ||
| PayloadStatusEnum::Valid => return Ok(EngineApiHint::ValidPayload), | ||
| PayloadStatusEnum::Invalid { validation_error } => validation_error, | ||
| PayloadStatusEnum::Syncing | PayloadStatusEnum::Accepted => { |
There was a problem hiding this comment.
Why is Accepted an error?
There was a problem hiding this comment.
Because we don't want the EL to accept this block. According to the engine specs: https://github.com/ethereum/execution-apis/blob/main/src/engine/paris.md#engine_newpayloadv1 the ACCEPTED code MUST be returned only if the following conditions are met:
- all transactions have non-zero length
- the blockHash of the payload is valid
- the payload doesn't extend the canonical chain
- the payload hasn't been fully validated
- ancestors of a payload are known and comprise a well-formed chain
in our case our block extends the canonical chain so it will be marked as VALID.
| // This should not happen, but in case it does, it doesn't break the flow. | ||
| EngineApiHint::BlockHash(hash) => self.block_hash = Some(hash), | ||
|
|
||
| // For regular hint types, set the value and reset the block hash |
There was a problem hiding this comment.
Why do we reset the block hash hint here every time?
There was a problem hiding this comment.
It really depends on how the EL validation is structured, but me and @thedevbirb found that sometimes Geth would return a mismatching hash error after populating some extra field (e.g. state root) which means we can't get more information out of it (as it kept returning mismatching block hash and no other hints). So when this happens we set the block hash to the one hinted by Geth and then proceed with the next hints.
As the comment mentions, this shouldn't happen and it's more of an edge case
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This PR contains a refactor to the bolt-sidecar
buildermodule to generalize over the EL client used for fallback block building. The refactor also comes with:clientmodule:execution,beacon,engineandconstraints