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

[Merged by Bors] - Serve Bellatrix preset in BN API #3425

Closed
wants to merge 1 commit into from

Conversation

michaelsproul
Copy link
Member

@michaelsproul michaelsproul commented Aug 4, 2022

Issue Addressed

Resolves #3388
Resolves #2638

Proposed Changes

  • Return the BellatrixPreset on /eth/v1/config/spec by default.
  • Allow users to opt out of this by providing --http-spec-fork=altair (unless there's a Bellatrix fork epoch set).
  • Add the Altair constants from /eth/v1/config/spec does not return Altair constants #2638 and make serving the constants non-optional (the http-disable-legacy-spec flag is deprecated).
  • Modify the VC to only read the Config and not to log extra fields. This prevents it from having to muck around parsing the ConfigAndPreset fields it doesn't need.

Additional Info

This change is backwards-compatible for the VC and the BN, but is marked as a breaking change for the removal of --http-disable-legacy-spec.

I tried making Config a superstruct too, but getting the automatic decoding to work was a huge pain and was going to require a lot of hacks, so I gave up in favour of keeping the default-based approach we have now.

@michaelsproul michaelsproul added ready-for-review The code is ready for review backwards-incompat Backwards-incompatible API change bellatrix Required to support the Bellatrix Upgrade v2.6.0 labels Aug 4, 2022
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

LGTM! This looks really nice, it's a shame that API endpoint has become so complex!

beacon_node/src/config.rs Show resolved Hide resolved
@paulhauner paulhauner added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Aug 10, 2022
@paulhauner
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Aug 10, 2022
## Issue Addressed

Resolves #3388
Resolves #2638

## Proposed Changes

- Return the `BellatrixPreset` on `/eth/v1/config/spec` by default.
- Allow users to opt out of this by providing `--http-spec-fork=altair` (unless there's a Bellatrix fork epoch set).
- Add the Altair constants from #2638 and make serving the constants non-optional (the `http-disable-legacy-spec` flag is deprecated).
- Modify the VC to only read the `Config` and not to log extra fields. This prevents it from having to muck around parsing the `ConfigAndPreset` fields it doesn't need.

## Additional Info

This change is backwards-compatible for the VC and the BN, but is marked as a breaking change for the removal of `--http-disable-legacy-spec`.

I tried making `Config` a `superstruct` too, but getting the automatic decoding to work was a huge pain and was going to require a lot of hacks, so I gave up in favour of keeping the default-based approach we have now.
@bors bors bot changed the title Serve Bellatrix preset in BN API [Merged by Bors] - Serve Bellatrix preset in BN API Aug 10, 2022
@bors bors bot closed this Aug 10, 2022
@michaelsproul michaelsproul deleted the bellatrix-preset branch August 10, 2022 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-incompat Backwards-incompatible API change bellatrix Required to support the Bellatrix Upgrade ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants