Conversation
| discard await node.router.routeExecutionPayloadEnvelope( | ||
| signedEnvelope, checkValidator = false) | ||
| # Send payload to local EL before broadcasting | ||
| payloadStatus = await node.elManager.newExecutionPayload( |
There was a problem hiding this comment.
Generally, we don't necessarily want this in the critical path for getting the payload out at all, i.e. right now this sequences events as:
- gossip-broadcast block
- newPayload the envelope/payload
- gossip-broadcast the payload
in that order.
Effectively, if the payload isn't accepted, that's a bug, either in Nimbus or in the EL, but it's better to try and get the payload out than to wait on the EL to do so in case. If it's not valid, it's not as if there's some fallback here. Better to just broadcast.
newExecutionPayload can have a timeout of 8s. Nimbus should not wait 8s before proceeding with envelope broadcast, and probably the newExecutionPayload shouldn't occur in proposeBlockAux at all, or indeed beacon_validators.nim, but rather https://github.com/status-im/nimbus-eth2/blob/unstable/beacon_chain/gossip_processing/block_processor.nim
In particular, block_processor centralizes as much as feasible all block validation, from untrusted -> trusted. So what should happen is that we broadcast/route the envelope, and the routing also triggers the block processor to assess the changed block + envelope.
| # for self-builds since the envelope's state_root is | ||
| # computed before `process_execution_payload` runs, | ||
| # so it wouldn't match the post-envelope state root | ||
| discard node[].dag.addHeadExecutionPayload( |
There was a problem hiding this comment.
Is this special case necessary? I'd note that there's no addHeadBlock here either, broadly for similar reasons as the newPayload call: that's the domain of block_processor/block_clearance. routeSignedBeaconBlock and routeExecutionPayloadEnvelope alone should ensure block_processor/block_clearance pick this up.
| let isSelfBuild = signedEnvelope.message.builder_index == | ||
| BUILDER_INDEX_SELF_BUILD | ||
|
|
||
| # Verify with state transition function. | ||
| process_execution_payload( | ||
| dag.cfg, | ||
| dag.clearanceState.forky(consensusFork), | ||
| signedEnvelope, | ||
| func(_: deneb.ExecutionPayload): bool = true, | ||
| cache, | ||
| verify = not isSelfBuild | ||
| ).isOkOr: | ||
| assign(dag.clearanceState, dag.headState) | ||
| info "Envelope transition failed", msg = error |
There was a problem hiding this comment.
When envelope is self-build, I believe they could be from ourselves or others. And in devnet-0, all blocks should be self-build. It seems we would not be able to verify with clearance state effectively?
There was a problem hiding this comment.
I believe "self-build" in this context refers to an envelope that was built locally by this node (i.e., we were the proposer and constructed the payload ourselves)
There was a problem hiding this comment.
Yes so my question is - how do we set verify = true for the received envelopes that are self-build by others?
|
Merge conflict in |
3 participants
When a validator proposes a Gloas block and builds the execution payload locally (self-build), the envelope's state_root field is set at the creation time. This is the root after the beacon state root after
process_execution_payload_bidruns but beforeprocess_execution_payloadruns.When
addHeadExecutionPayloadlater callsprocess_execution_payloadthe additional state transitions insideprocess_execution_payloadupdates thelatest_block_hash. The resulting state root no longer matches thestate_rootstored in the envelope that causes verification to fail with :process_execution_payload: state root mismatchthis passes the verify=false to
process_execution_payloadfor self-built envelopes