Move parent_beacon_block_root in execution payload and update verification

[g11tech]

Want to propose a simplification which allows execution block to be reconstructed entirely based on execution payload

• add parent_beacon_block_root to execution payload since its already part of execution header
• corresponding change for execution aps - Move parentBeaconBlockRoot to execution payload execution-apis#441

Till 4788 addition execution payload (from beacon bloc) had one to one correspondence with execution block and hence was possible to construct execution block with payload (and relatively straight forward debugging). However the payload from beacon bloc directly can't be used without massaging with parent

Ethereumjs has utility methods to construct block from execution payload

TODO

• if this change is acceptable to community then can update any other post deneb references

[mkalinin]

It would be good to know whether other clients have a preference of parent_beacon_block_root to be a part of ExecutionPayload to justify this change. Alternatively, can this utility method be extended with the parent_beacon_block_root parameter? Will it be a breaking change?

[g11tech]

It would be good to know whether other clients have a preference of parent_beacon_block_root to be a part of ExecutionPayload to justify this change. Alternatively, can this utility method be extended with the parent_beacon_block_root parameter? Will it be a breaking change?

it can be extended for sure but the problem is all the payload data being handy and available in one place. currently parent_beacon_block_root not in payload messes up the UX imo. (we have seen several instances of debugging on testnets where we used payload to construct and debug the blocs)

and yea would love to see opinions from other clients 🙂

[djrtwo]

There was a moderate amount of discussion on this when the initial EIP PR was up. I was in favor of putting it in there to just more easily mirror what's on CL and EL and for simple access patterns. I also thought it might help with lightclient proofs but was convinced that when moving from CL into EL proofs, that you'd use native EL structures at that point. So the argument came down mainly to aesthetic reasons vs de-duplication of data.

If the aesthetic reasons have elevated to practical reasons due to debugging issues, I'm very okay with patching this in.

Want to hear from other CL devs first

EDIT: previous discussions that lead to not being in payload are here -- #3421 (comment)

[djrtwo]

Need to shuffle this around a bit, see comments.

Plus need a few tests to show success and failure

[tbenr]

I'm mildly against it (due to data duplication) unless there are other EL devs agreeing that this would improve significantly their DevX. I'm generally not a fan of adding extra data on-chain just for easier testing\debugging.

[lightclient]

I don't really see what the advantage of having it in the execution payload vs. having it as an equal-level parameter in the engine api. Of course our architecture is very EL-CL split right now, but I don't think we should let that leak into our actual consensus objects more than it already is.

[terencechain]

I'm against it unless it brings significant improvement on the EL side. Same reason as @tbenr

[mkalinin]

I want to echo @lightclient's point. From the perspective of a client software running CL and EL under the same hood this field in the CL's payload structure and the corresponding check in the state transition look ugly. Engine API is the place where we should aim encapsulating such redundancies. Although, we already have precedents like the block hash check it would be great not to extend the surface of EL dependencies leaked into the beacon chain structures and validity rules unless there is a strong reason for that.

[g11tech]

because this field is just not a check but something that impacts EL state and with the same decoupling argument it should belong in payload standalone by itself.

check can still be moved to EL with new payload also sending block's parent hash as it does currently but check performed on EL side similar to versioned hashes)

Not sure how to value such cleanliness and debuggng exp against havng a duplcated field

[Giulio2002]

I am against it as well for the same reason as @tbenr, I also do not see any significant improvements for the EL side, and I think it has the potential to only add protocol debt.

[Giulio2002]

Ok, I misread, this is good because since EIP4788 is in Cancun, having it present in the header makes it consistent

[g11tech]

closing the PR as per discussion in the ACD CL call jul 27