Skip to content

feat: update beacon content types to support Pectra #1801

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
merged 5 commits into from
May 6, 2025

Conversation

morph-dev
Copy link
Collaborator

@morph-dev morph-dev commented May 3, 2025

What was wrong?

The Beacon content types should be updated in order to support Pectra.
See ethereum/portal-network-specs#397 for details.

How was it fixed?

Modified existing type to support Pectra.

I had to update and remove all usage of the old type. That involved rewriting tests (I also added more test cases), which was tricky at times we now have different test data (I updated portal-spec-tests submodule)

Followup work

  1. Update beacon bridge to be able to create and gossip new type as well
    • This requires some refactoring in how we fetch BeaconState and I want to do it in a separate PR
  2. Investigate if something else should be updated (either in light-client or trin-beacon crates)

To-Do

@morph-dev morph-dev requested review from KolbyML and ogenev May 3, 2025 19:46
@morph-dev morph-dev self-assigned this May 3, 2025
@morph-dev morph-dev force-pushed the beacon_types_pectra branch from 6ce6ad7 to 40e7ba0 Compare May 4, 2025 08:10
@KolbyML
Copy link
Member

KolbyML commented May 4, 2025

  • I want to match it with ForkDigest (and ForkName), so Deneb makes it consistent

I am not sure I understand this reasoning as the HistoricalSummariesWithProof format isn't likely to change in the future past Electra. So HistoricalSummariesWithProofElectra would encompass all future forks, hence I don't get the need to call HistoricalSummariesWithProof HistoricalSummariesWithProofDeneb when it isn't specific to Deneb and there isn't a consistent trend in future forks which tie these types 1 for 1 with ForkDigest/ForkName, Other then HistoricalSummariesWithProofElectra being where the last change and breaking change from the previous format occurred

@KolbyML
Copy link
Member

KolbyML commented May 4, 2025

Isn't Electra hitting mainnet on May 7th so in 3 days? Shouldn't we just remove support for HistoricalSummariesProofDeneb right now, as we will need to remove it regardless on Wednesday?

I don't see a reason to add support for both types, when we will just be removing the old type on Wednesday, we might as only only support HistoricalSummariesProofElectra in this PR, as unlike other types HistoricalSummariesProof is ephemeral

Worth mentioning that the old type will most likely go away from the spec after the fork as we won't use it anymore.
^ which was also stated in the PR's description

@KolbyML
Copy link
Member

KolbyML commented May 4, 2025

I think we should just replace our current HistoricalSummariesWithProofDeneb with the new format HistoricalSummariesWithProofElectra and call it a day, I don't see much value in us supporting both types for 2-3 days. Adding complexity to handle both cases just to remove it in 2-3 days, doesn't make much sense to me.

@KolbyML
Copy link
Member

KolbyML commented May 5, 2025

image

shouldn't we enable pectra for all the other RPC endpoints? or do you still plan to make a separate PR for this?

@morph-dev
Copy link
Collaborator Author

morph-dev commented May 5, 2025

I think we should just replace our current HistoricalSummariesWithProofDeneb with the new format HistoricalSummariesWithProofElectra and call it a day, I don't see much value in us supporting both types for 2-3 days. Adding complexity to handle both cases just to remove it in 2-3 days, doesn't make much sense to me.

I did as suggested.

It did cause complexity in refactoring tests. I removed BeaconState that we used for tests before and now I'm using test vector that I added in ethereum/portal-spec-tests#51.

Also updated PR description

@morph-dev
Copy link
Collaborator Author

shouldn't we enable pectra for all the other RPC endpoints? or do you still plan to make a separate PR for this?

Separate PR

Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

Most of the PR looks good. I was am a little confused why we changed the code to encode the fork_digest using ssz, I thought we were supposed to should set it as the pre-fix. I didn't see anything in the PR description about there being a bug with how we did it before.

So I am a little confused? Maybe it encodes to the same output? Maybe I am missing something?

@morph-dev
Copy link
Collaborator Author

Most of the PR looks good. I was am a little confused why we changed the code to encode the fork_digest using ssz, I thought we were supposed to should set it as the pre-fix. I didn't see anything in the PR description about there being a bug with how we did it before.

So I am a little confused? Maybe it encodes to the same output? Maybe I am missing something?

The ssz encoding and actual ForkDigest are the same, so it doesn't make a difference.
But you are right, it shouldn't be ssz encoded/decoded (even if that doesn't do anything).

I can see how it can easily cause confusion, so I removed it.

Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

:shipit: looks good

Comment on lines 524 to 529
let fork_digest = ForkDigest::try_from(fork_digest_bytes)
.map_err(|err| DecodeError::BytesInvalid(err.to_string()))?;
let fork_name = match ForkName::try_from(fork_digest) {
Ok(fork_name) => fork_name,
Err(err) => return Err(DecodeError::BytesInvalid(err.to_string())),
};
Copy link
Member

Choose a reason for hiding this comment

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

It might be nice to be consistent with match/map_err usage here

@morph-dev morph-dev merged commit 5daabd4 into ethereum:master May 6, 2025
16 of 18 checks passed
@morph-dev morph-dev deleted the beacon_types_pectra branch May 6, 2025 08:29
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.

2 participants