Skip to content

Conversation

@pawanjay176
Copy link
Member

@pawanjay176 pawanjay176 commented Jul 17, 2025

Issue Addressed

N/A

Proposed Changes

Serializes the blob_schedule in ascending order to match other clients. This is needed to keep the output of eth/v1/config/spec http endpoint consistent across clients.
cc @barnabasbusa

@pawanjay176 pawanjay176 added ready-for-review The code is ready for review fulu Required for the upcoming Fulu hard fork labels Jul 17, 2025
@pawanjay176 pawanjay176 requested a review from jimmygchen July 17, 2025 18:39
@mergify
Copy link

mergify bot commented Jul 17, 2025

Some required checks have failed. Could you please take a look @pawanjay176? 🙏

@mergify mergify bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. ready-for-review The code is ready for review and removed ready-for-review The code is ready for review waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jul 17, 2025
// Check that serialization is in ascending order
let yaml = serde_yaml::to_string(&spec.blob_schedule).expect("should serialize");

// Deserialize back to Vec<BlobParameters> to check order
Copy link
Member

@jimmygchen jimmygchen Jul 18, 2025

Choose a reason for hiding this comment

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

It's a little weird that a serde round trip result in different ordering. Maybe we should reverse again in deserialize?

It currently don't cause any issue, but if it gets serialized -> deseralized -> serailzied again we'll end up having descending order - might be confusing if this functionality gets used by downstream projects

Copy link
Member

Choose a reason for hiding this comment

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

Ah nevermind, this is done in the constructor

pub fn new(mut vec: Vec<BlobParameters>) -> Self {
// reverse sort by epoch
vec.sort_by(|a, b| b.epoch.cmp(&a.epoch));
Self(vec)
}

Copy link
Member

@jimmygchen jimmygchen 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, thanks!

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jul 18, 2025
mergify bot added a commit that referenced this pull request Jul 18, 2025
@mergify mergify bot merged commit 1046dfb into sigp:unstable Jul 18, 2025
56 of 57 checks passed
danielrachi1 pushed a commit to danielrachi1/lighthouse that referenced this pull request Jul 18, 2025
N/A


  Serializes the blob_schedule in ascending order to match other clients. This is needed to keep the output of `eth/v1/config/spec` http endpoint consistent across clients.
cc @barnabasbusa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fulu Required for the upcoming Fulu hard fork ready-for-merge This PR is ready to merge.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants