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

Fix blob's gossip subscriptions #8896

Merged
merged 3 commits into from
Dec 9, 2024

Conversation

tbenr
Copy link
Contributor

@tbenr tbenr commented Dec 6, 2024

We are incorrectly calculating relevant blobs topics for upcoming forks.

When new specConfig has been introduced, an assumption has been broken in Eth2GossipTopicFilter, which was passing the current milestone to getAllTopics even when calculating topics for the future forks.
It was fine because getAllTopics is only getting the config and from the spec, which in previous specConfig design was always the "latest defined milestone config".
With recent changes, it is no more the case, so we need to pick config from the correct milestone.

Unit test was broken because it was passing the wrong milestone (the next fork milestone, instead of the current one)

I fixed the bug and the tests.

Moreover the Electra blobs bump would have break getAllTopics in blob topics calculation. Thanks to new dynamic spec config, that works transparently now.

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@tbenr tbenr changed the title Fix gossip subscriptions Fix blob's gossip subscriptions Dec 7, 2024
Copy link
Contributor

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

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

LGTM

@tbenr tbenr enabled auto-merge (squash) December 9, 2024 08:34
@tbenr tbenr merged commit 4ac3018 into Consensys:master Dec 9, 2024
17 checks passed
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