Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eip4844 schema definitions #6441

Merged
merged 10 commits into from
Nov 15, 2022
Merged

Conversation

zilm13
Copy link
Contributor

@zilm13 zilm13 commented Nov 14, 2022

PR Description

  • api.schema.eip4844 structures will be added in separate PR to reduce size of this one
  • please check carefully 2 fixes in Capella files 6f685e1 205e7c4

Fixed Issue(s)

Fixes #6388

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good generally. I think we've missed some Capella fields in the BeaconState. Probably worth someone more familiar with the 4844 schemas to cast an eye over it as well.

Comment on lines 51 to 54
return Stream.concat(
BeaconStateSchemaAltair.getUniqueFields(specConfig).stream(),
Stream.of(latestExecutionPayloadHeaderField))
.collect(Collectors.toList());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this will be missing all the new fields added in Capella? It may be worth just getting all the capella fields and then replacing the latest execution payload header field. Would probably be quite useful to have a utility method around that can do that replacement.

Copy link
Contributor Author

@zilm13 zilm13 Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I copied it from Capella and it's the same bug there. Looking for a way to cover it in a test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need SpecLogic for tests, postpone

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it's not bug, @tbenr helped me to figure out

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to introduce an override pattern like this (this is for capella). To always build on top of the previous and just apply the differences.

  private static List<SszField> getUniqueFields(final SpecConfig specConfig) {
    final SszField nextWithdrawalIndexField =
        new SszField(
            NEXT_WITHDRAWAL_INDEX,
            BeaconStateFields.NEXT_WITHDRAWAL_INDEX,
            () -> SszPrimitiveSchemas.UINT64_SCHEMA);
    final SszField nextWithdrawalValidatorIndexField =
        new SszField(
            NEXT_WITHDRAWAL_VALIDATOR_INDEX,
            BeaconStateFields.NEXT_WITHDRAWAL_VALIDATOR_INDEX,
            () -> SszPrimitiveSchemas.UINT64_SCHEMA);

    final Stream<SszField> overriddenBellatrixFields =
        BeaconStateSchemaBellatrix.getUniqueFields(specConfig).stream()
            .map(
                sszField -> {
                  if (sszField.getIndex() == LATEST_EXECUTION_PAYLOAD_HEADER_FIELD_INDEX) {
                    return new SszField(
                        LATEST_EXECUTION_PAYLOAD_HEADER_FIELD_INDEX,
                        BeaconStateFields.LATEST_EXECUTION_PAYLOAD_HEADER,
                        () ->
                            new ExecutionPayloadHeaderSchemaCapella(
                                SpecConfigCapella.required(specConfig)));
                  } else {
                    return sszField;
                  }
                });
    return Stream.concat(
            overriddenBellatrixFields,
            Stream.of(nextWithdrawalIndexField, nextWithdrawalValidatorIndexField))
        .collect(Collectors.toList());
  }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise we could end up being forced to always build on top of Altair and add keep adding everything from there for each additional fork

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And we can filter out fields when we will start deleting stuff (if we ever will)

Copy link
Contributor Author

@zilm13 zilm13 Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this? needs tests and refactoring, just to show an idea 9052835
Also weird place for this logic only in BeaconStateSchema

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

decided to left it for a future PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's worth nothing that Capella didn't inherit any of the fields from Bellatrix - the execution payload is a different schema in Capella so needed to be replaced. 4844 is the first point where we're wanting to inherit fields unchanged that aren't common to all forks.

Copy link
Contributor

@tbenr tbenr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zilm13 zilm13 merged commit c4837e1 into Consensys:master Nov 15, 2022
@zilm13 zilm13 deleted the eip4844-schema-definitions branch November 15, 2022 14:34
tbenr pushed a commit to tbenr/teku that referenced this pull request Nov 15, 2022
* Fix duplicate of ExecutionPayload in toString() for Capella

* Fix BeaconBlockBody creation for Capella

* Add EIP-4844 schema definitions

* Added Blob, BlobsSidecar, SignedBeaconBlockAndBlobsSidecar schema getters from EIP-4844 schema definitions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

expose schema definitions for Eip4844 fork
3 participants