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

Start prototyping superstruct features #5610

Open
wants to merge 11 commits into
base: unstable
Choose a base branch
from

Conversation

michaelsproul
Copy link
Member

Proposed Changes

Start using superstruct's burgeoning support for features & fork ordering:

Additional Info

Current limitations:

  • The superstruct features branch is out of date with main (doesn't support flatten or meta_variants)
  • Only files in the same crate (types) can use the FORK_ORDER. We may need some hacks to correct this, as presently the data is stored between macro invocations in a crate-specific part of the OUT_DIR.

@michaelsproul michaelsproul added code-quality electra Required for the Electra/Prague fork labels Apr 19, 2024
@dapplion
Copy link
Collaborator

Once a fork has landed, is the plan to consolidate all features into the same fork variant?

@michaelsproul
Copy link
Member Author

@dapplion I guess we could if we want to simplify the code, but I don't see any strong reason to. The main reason the existing forks are combined is that it would be unnecessary to split them up.

@macladson
Copy link
Member

I've updated a decent amount of boilerplate to use the has_feature() function instead of matching on the type variant.

There are a few things of note:

1. Superstruct quirks.

Currently, compiling crates depending on the new superstruct features are kinda broken. There exists some kind of race condition where the FORK_ORDER.json and FEATURE_DEPENDENCIES.json files are required before they are created. This only affects the first build. Subsequent builds should work normally since the files now exist. This also applies any time you change the fork ordering or feature dependencies, or you run a cargo clean. I saw some initial success with using cargo check -j 1 but this stopped working as the number of types utilizing the new features increased.

This basically means in it's current state, it will never pass CI. We'll need a solution to this in the long term. I think @michaelsproul might have some ideas here.

2. Removing match's

Removing a match means the compiler will not complain when a new fork is added. While this is useful for adding new/empty forks, it goes against a long-standing policy in Lighthouse of using match explicitly against every type (avoiding wildcards). The reason this was done in the first place, is so that when adding a new fork, the compiler will direct you to an area of code which might need to be updated/changed to support the new fork. While this is helpful in some cases, in my view, this process generally just involves adding the new ForkName without any further consideration required, which just slows down dev time. There will be cases where we should leave it as an explicit match, but committing to the ideas in this PR will necessarily mean we break this policy in most cases.

3. Remaining Boilerplate

The codebase is still bloated with a large amount of match blocks that follow similar patterns where every variant has it's own unique code path. For example:

match self {
    Fork::Base => Base::some_function(),
    Fork::Altair => Altair::some_function(),
    ...
}

or converting one type to another:

match body.fork() {
    Fork::Base => Block::Base(body),
    Fork::Altair  => Block::Altair(body),
    ...
}

These patterns frequently come up when dealing with SSZ and deserialization.
Generally adding a new fork to these patterns doesn't require any real additional logic, but they are currently required due to the type system. It's probably out of scope for this PR, but being able to deal with these basic patterns in a cleaner way would remove a significant amount of the remaining boilerplate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-quality electra Required for the Electra/Prague fork
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants