-
Notifications
You must be signed in to change notification settings - Fork 893
Skip serializing blob_schedule before Fulu #7779
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
Skip serializing blob_schedule before Fulu #7779
Conversation
Some required checks have failed. Could you please take a look @michaelsproul? 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, fixed the test and merged unstable
Actually this is quite annoying to deal with in the tests because a roundtrip ser/de isn't consistent. i.e. this test would fail #[test]
fn chain_spec_roundtrip() {
let yamlconfig =
ConfigAndPreset::from_chain_spec::<MainnetEthSpec>(&ChainSpec::mainnet(), None);
let serialized = serde_yaml::to_string(&yamlconfig).unwrap();
let deserialized: ConfigAndPreset = serde_yaml::from_str(&serialized).unwrap();
assert_eq!(yamlconfig.config().blob_schedule, deserialized.config().blob_schedule);
} Not sure of a clean way to handle this other than setting fulu_fork_epoch all places where we do this kind of roundtrip. |
Why doesn't it round trip? Is it because of the serialisation field? Maybe we should ignore that in the PartialEq impl of BlobSchedule |
Yeah that's right lighthouse/validator_client/http_api/src/test_utils.rs Lines 257 to 265 in 257d270
Yeah i think ignoring that field in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh ignoring it in the partialeq impl is very clean, nice!
Issue Addressed
Alternative to:
Proposed Changes
Serve the
blob_schedule
field on/eth/v1/config/spec
only when Fulu is enabled. If the blob schedule is empty, we will still serve it as[]
, so long as Fulu is enabled.Additional Info
This touches the same code as:
But this PR is higher prio for Fusaka testing.