Skip to content

Conversation

@jimmygchen
Copy link
Member

Issue Addressed

Addresses this comment: #8254 (comment)

We're currently using subscribe_all_data_column_subnets here to subscribe to all subnets

if fork_name.fulu_enabled() {
if opts.subscribe_all_data_column_subnets {
for i in 0..spec.data_column_sidecar_subnet_count {
topics.push(GossipKind::DataColumnSidecar(i.into()));
}
} else {
for subnet in &opts.sampling_subnets {
topics.push(GossipKind::DataColumnSidecar(*subnet));
}
}
}

But its unnecessary because the else path also works for supernode (uses sampling_subnets instead)

The big diffs will disappear once #8254 is merged.

@jimmygchen jimmygchen requested a review from jxs as a code owner October 22, 2025 01:06
@jimmygchen jimmygchen added code-quality ready-for-review The code is ready for review v8.1.0 Post-Fulu release labels Oct 22, 2025
@jimmygchen jimmygchen force-pushed the semi-supernode-simple-cleanup branch from f3a3e2a to 029ba13 Compare October 23, 2025 05:10
@mergify
Copy link

mergify bot commented Oct 23, 2025

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

@mergify mergify bot 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 Oct 23, 2025
Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

LGTM!

@jimmygchen this commit fixes the failing test 4084d11

Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

LGTM, just one nit

@jimmygchen jimmygchen force-pushed the semi-supernode-simple-cleanup branch from fe778b3 to c19e963 Compare October 30, 2025 02:25
@jimmygchen
Copy link
Member Author

@dapplion thanks for the fix, I've pushed your changes.

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Oct 30, 2025
@mergify mergify bot added the queued label Oct 30, 2025
mergify bot added a commit that referenced this pull request Oct 30, 2025
@mergify mergify bot merged commit 30094f0 into sigp:unstable Oct 30, 2025
37 checks passed
@mergify mergify bot removed the queued label Oct 30, 2025
eserilev pushed a commit to eserilev/lighthouse that referenced this pull request Oct 30, 2025
…rk (sigp#8259)

Addresses this comment: sigp#8254 (comment)

We're currently using `subscribe_all_data_column_subnets` here to subscribe to all subnets
https://github.com/sigp/lighthouse/blob/522bd9e9c6ac167f2231525e937c9ebbcb86cf6e/beacon_node/lighthouse_network/src/types/topics.rs#L82-L92

But its unnecessary because the else path also works for supernode (uses `sampling_subnets` instead)

The big diffs will disappear once sigp#8254 is merged.


  


Co-Authored-By: Jimmy Chen <[email protected]>
eserilev pushed a commit to eserilev/lighthouse that referenced this pull request Oct 30, 2025
…rk (sigp#8259)

Addresses this comment: sigp#8254 (comment)

We're currently using `subscribe_all_data_column_subnets` here to subscribe to all subnets
https://github.com/sigp/lighthouse/blob/522bd9e9c6ac167f2231525e937c9ebbcb86cf6e/beacon_node/lighthouse_network/src/types/topics.rs#L82-L92

But its unnecessary because the else path also works for supernode (uses `sampling_subnets` instead)

The big diffs will disappear once sigp#8254 is merged.


  


Co-Authored-By: Jimmy Chen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-quality ready-for-merge This PR is ready to merge. v8.1.0 Post-Fulu release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants