Fork-specific consensus-types interfaces #13948
Conversation
| WithdrawalRequests() ([]*enginev1.ExecutionLayerWithdrawalRequest, error) | ||
| } | ||
|
|
||
| type ExecutionDataElectra interface { |
There was a problem hiding this comment.
Shouldn't there be ExecutionDataCapella and ExecutionDataDeneb too? That will allow us to remove unnecessary methods from previous forks.
There was a problem hiding this comment.
This is an incremental step, but yes I would like us to add interfaces for each fork.
| // the Electra hard fork. If the value is for an earlier fork (based on comparing its Version() to the electra version) | ||
| // an error will be returned. Callers that want to conditionally process electra data can check for this condition | ||
| // and safely ignore it like `if err != nil && errors.Is(interfaces.ErrInvalidCast) {` | ||
| func AsROBlockBodyElectra(in ReadOnlyBeaconBlockBody) (ROBlockBodyElectra, error) { |
There was a problem hiding this comment.
Same as with execution data, shouldn't we have equivalent functions for all other forks?
There was a problem hiding this comment.
Yes, once we backport to the other forks.
| epe, postElectra := data.(interfaces.ExecutionDataElectra) | ||
| if postElectra { | ||
| drs := epe.DepositReceipts() |
There was a problem hiding this comment.
This pattern assumes that every interfaces.ExecutionData in future forks, that also satisfies ExecutionDataElectra will have to be handled in the same way. Say for example that interfaces.ExecutionDataEpbs satisfies the Electra interface, it requires different handling of deposits but it requires the same handling of 7002 withdrawals, etc. If we explicitly change it to not satisfy ExecutionDataElectra and satisfy a different ExectutionDataEpbs then we would have to copy paste the withdrawal handling. If we keep it satisfying ExecutionDataElectra then we would have to deal with deposits with an if-else data.(interfaces.ExecutionDataEpbs) within this block right?
There was a problem hiding this comment.
Copy/paste from team chat:
If the ssz is the same, then it's fine for ExecutionDataEpbs to satisfy ExecutionDataElectra. Satisfying the interface only dictates what kind of value are available, it does not dictate how the data should be processed. That should always be based on the slot (or version, which is based on the slot).
SSZ changes in nested types is similar to what we just went through with Attestations. In that case our solution was to move attestations to an interface. A benefit of this kind of composition is that we would only need to implement the logic to deal with the interface in 2 places.If we do want to actually deprecate (remove) a field, then yes one option would be to break the chain of interface composition. Another would be to backport an error to that fields signature. Another would be to allow the getter to return a nil value and rely on the caller to be fork-aware. Another option is to rely less on these big interfaces and move processing functions to accept narrower interfaces. For example, epbs could take:
type EpbsWithdrawalGetter interface {
GetWithdrawals() *eth.PBSWithdrawal
}
whereas other methods would take:
type WithdrawalsGetter interface {
GetWithdrawals() []byte
}
and then perform assertions on those instead of the whole big type. A problem in both cases is that now if we want block to have something like an ExecutionData type that the container could use to abstractly carry the payload. This type would have to become /increasingly/ abstract (some kind of ConsensusType interface that would have ssz methods etc, which is asserted downstream to narrower interfaces). Otherwise we would also break the chain of composition in the container (block body).
TL;DR this approach handles all the previous fork changes pretty well, with attestations being the only notable exception. If we see more extreme changes across forks I think the best way to deal with that is probably more abstract interfaces for nested types, and narrow interfaces used by consumers of those fields. We'll need to keep iterating on it to see what feels most maintainable in practice.
* fork-specific interface for electra * add electra to wrapped payload switch * use electra body in block factory * deepsource * rm pb getters from electra payload --------- Co-authored-by: Kasey Kirkham <kasey@users.noreply.github.com>
What type of PR is this?
Other
What does this PR do? Why is it needed?
This brings back the changes from #13928 that got lost in the shuffle of various electra branches.
The goal of this PR is to deprecate the following patterns:
In place of these patterns, we can use interface composition to add a new interface at each fork. The ExecutionData version for a particular fork can be accessed with a vanilla go type assertion. The BeaconBlockBody equivalent requires a little more wiring, because there is only one underlying concrete type which satisfies the interfaces of all forks. So we can add safe cast helpers to
consensus-types/interfaces/cast.go.