[Merge] add merge spec datastructures #4503
Conversation
tbenr
force-pushed
the
merge_add_merge_spec_datastructures
branch
from
042d394
to
7a5ba83
Compare
...src/test/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactoryTest.java
Outdated
Show resolved
Hide resolved
...main/java/tech/pegasys/teku/spec/datastructures/blocks/blockbody/BeaconBlockBodyBuilder.java
Show resolved
Hide resolved
.../teku/spec/datastructures/blocks/blockbody/versions/altair/BeaconBlockBodyBuilderAltair.java
Outdated
Show resolved
Hide resolved
.../teku/spec/datastructures/blocks/blockbody/versions/altair/BeaconBlockBodyBuilderAltair.java
Outdated
Show resolved
Hide resolved
...pegasys/teku/spec/datastructures/state/beaconstate/versions/altair/ValidatorStatsAltair.java
Outdated
Show resolved
Hide resolved
...ch/pegasys/teku/spec/datastructures/state/beaconstate/versions/phase0/BeaconStatePhase0.java
Outdated
Show resolved
Hide resolved
...gasys/teku/spec/datastructures/blocks/blockbody/versions/merge/BeaconBlockBodyMergeImpl.java
Outdated
Show resolved
Hide resolved
| @@ -125,8 +126,7 @@ protected void applyAdditionalFields(final MutableBeaconState state) { | |||
| beaconStateAltair -> { | |||
| final tech.pegasys.teku.spec.datastructures.state.SyncCommittee.SyncCommitteeSchema | |||
| syncCommitteeSchema = | |||
| beaconStateAltair | |||
| .getBeaconStateSchemaAltair() | |||
| ((BeaconStateSchemaAltair) beaconStateAltair.getBeaconStateSchema()) | |||
There was a problem hiding this comment.
@ajsutton going down to that path I found this. do you think we should then implement the toVersion<Milestone> pattern in BeaconStateSchema too?
There was a problem hiding this comment.
I suspect it's more likely a required kind of pattern where it just throws an exception if it's the wrong type (with useful detail). Code like this won't be able to handle getting an empty Optional back. But yes that's definitely better than casting.
| public BeaconBlockBody createBlockBody(final Consumer<BeaconBlockBodyBuilder> builderConsumer) { | ||
| final BeaconBlockBodyBuilderMerge builder = new BeaconBlockBodyBuilderMerge().schema(this); | ||
| // Provide a default empty sync aggregate | ||
| builder.syncAggregate(getSyncAggregateSchema()::createEmpty); |
There was a problem hiding this comment.
@ajsutton i'd like to discuss this too. I'm incline to say that the Empty\ZERO\Default values should be always provided by the builder, but I'd don't have a clear reasoning why in this particular case we are ensuring that there is at least an empty syncAggregate.
There was a problem hiding this comment.
I'm unclear on that as well. I don't think we want to create an empty SyncAggregate only to throw it away. It's not expensive but block creation is a very time sensitive process and SSZ objects aren't a simple POJO so not free.
But Altair/Merge block creation should always wind up setting a sync aggregate so I'm not sure why this was required. I wonder if it was added just to save adding it into a lot of tests? If so, pushing it into the builder is probably better.
PR Description
puts
Mergedatastructures atopAltairdatastructuresadds:
spec.datastructures.blocks.blockbody.versions.merge.*classesspec.datastructures.state.beaconstate.versions.merge.*classesSchemaDefinitionsMergerevisit
BeaconBlockBodyBuilderclasses hierarchyrelated to #4504
Documentation
documentationlabel to this PR if updates are required.Changelog