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.

Sign up for GitHub

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

Jump to bottom

Unsubscribe blob topics at Fulu fork #6932

Open
wants to merge 2 commits into
base: unstable
Choose a base branch
from
Open

Unsubscribe blob topics at Fulu fork #6932

wants to merge 2 commits into from

Conversation

dapplion
Copy link
Collaborator

@dapplion dapplion commented Feb 6, 2025 •
edited by jimmygchen
Loading

Issue Addressed

Addresses #6854.

PeerDAS requires unsubscribing a Gossip topic at a fork boundary. This is not possible with our current topic machinery.

Proposed Changes

Instead of defining which topics have to be added at a given fork, we define the complete set of topics at a given fork. The new start of the show and key function is:

pub fn core_topics_to_subscribe<E: EthSpec>(
    fork_name: ForkName,
    opts: &TopicConfig,
    spec: &ChainSpec,
) -> Vec<GossipKind> {
    // ...
    if fork_name.deneb_enabled() && !fork_name.fulu_enabled() {
        // All of deneb blob topics are core topics
        for i in 0..spec.blob_sidecar_subnet_count(fork_name) {
            topics.push(GossipKind::BlobSidecar(i));
        }
    }
    // ...
}

core_topics_to_subscribe only returns the blob topics if fork < Fulu. Then at the fork boundary, we subscribe with the new fork digest to core_topics_to_subscribe(next_fork), which excludes the blob topics.

I added is_fork_non_core_topic to carry on to the next fork the aggregator topics for attestations and sync committee messages. This approach is future-proof if those topics ever become fork-dependent.

Closes #6854

@dapplion dapplion requested a review from jxs as a code owner February 6, 2025 22:22
@dapplion dapplion requested review from pawanjay176 and jimmygchen and removed request for jxs February 6, 2025 22:22
@dapplion dapplion added ready-for-review The code is ready for review das Data Availability Sampling labels Feb 6, 2025
jimmygchen
jimmygchen reviewed Feb 7, 2025
View reviewed changes
beacon_node/lighthouse_network/src/service/mod.rs
}
});

let max_topics_by_fork = current_and_future_forks
Copy link
Member

Choose a reason for hiding this comment

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

There's no max usage here, can we call this all_topics_for_forks?

jimmygchen
jimmygchen reviewed Feb 7, 2025
View reviewed changes
beacon_node/lighthouse_network/src/service/mod.rs
.iter()
.map(|topics| topics.len())
.max()
.unwrap_or(0);
Copy link
Member

Choose a reason for hiding this comment

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

This should be unreachable but I think it's safe to do an expect here and exit
If we have 0 topics here something is fundamentally wrong.

jimmygchen
jimmygchen reviewed Feb 7, 2025
View reviewed changes
beacon_node/lighthouse_network/src/service/mod.rs
&self.network_globals.as_topic_config(),
&self.fork_context.spec,
) {
let topic = GossipTopic::new(kind, GossipEncoding::default(), new_fork_digest);
self.subscribe(topic);
}

// TODO(das): unsubscribe from blob topics at the Fulu fork
Copy link
Member

Choose a reason for hiding this comment

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

This TODO can be removed

jimmygchen
jimmygchen reviewed Feb 7, 2025
View reviewed changes
beacon_node/lighthouse_network/src/types/topics.rs
/// Returns the core topics associated with each fork that are new to the previous fork
pub fn fork_core_topics<E: EthSpec>(
fork_name: &ForkName,
/// Returns all the topics the node should subscribe at `epoch`
Copy link
Member

Choose a reason for hiding this comment

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

at ForkName

jimmygchen
jimmygchen reviewed Feb 7, 2025
View reviewed changes
beacon_node/lighthouse_network/src/types/topics.rs
}

#[test]
fn base_topics_are_always_active() {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a test to cover all topics for the latest fork? Similar to what the old test does.

jimmygchen
jimmygchen requested changes Feb 7, 2025
View reviewed changes
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 to me, just a few comments and questions

@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimmygchen jimmygchen jimmygchen requested changes

@pawanjay176 pawanjay176 Awaiting requested review from pawanjay176

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
das Data Availability Sampling waiting-on-author The reviewer has suggested changes and awaits thier implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dapplion @jimmygchen