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

Fullnodes to publish data columns from EL getBlobs #7258

Merged
merged 4 commits into from
Apr 8, 2025

Conversation

jimmygchen
Copy link
Member

Issue Addressed

Previously only supernode contributes to data column publishing in Lighthouse.

Recently we've updated the spec to have full nodes publishing data columns as well, to ensure all nodes contributes to propagation.

This also prevents already imported data columns from being imported again (because we don't "observe" them), and ensures columns that are observed in the gossip seen cache are forwarded to its peers, rather than being ignored.

@jimmygchen jimmygchen requested a review from jxs as a code owner April 4, 2025 05:18
@jimmygchen jimmygchen added ready-for-review The code is ready for review das Data Availability Sampling labels Apr 4, 2025
@jimmygchen jimmygchen requested a review from pawanjay176 April 4, 2025 12:50
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.

Nice!

Comment on lines 939 to 943
let columns_to_publish = columns
.into_iter()
.filter(|c| custody_columns.contains(&c.index))
.collect();
self_cloned.publish_data_columns_gradually(columns_to_publish, block_root);
Copy link
Member

Choose a reason for hiding this comment

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

One small optimization here might be to skip the filtering if self.network_globals.is_supernode()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 0c60a36

I was a bit tempted to remove the is_supernode function - with the introduction of validator custody, some full nodes now performs the same tasks as supernodes - including distributed blob publishing and reconstruction, but I'll leave this change out of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

With validator custody, this becomes a bit complicated - the node should be publishing the columns that it imported into its database - so we need to be using the custody_group_count tied to the Rpc or gossip verified beacon block instead of using the NetworkGlobals, because CGC is now dynamic:

However, there are a few moving pieces and this wiring hasn't been fully set up, I think it's best to wait until the following PR is completed:

@jimmygchen jimmygchen force-pushed the distributed-publishing-full-nodes branch from 8a8adb3 to 0c60a36 Compare April 7, 2025 00:14
Copy link
Collaborator

@eserilev eserilev 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 jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Apr 8, 2025
mergify bot added a commit that referenced this pull request Apr 8, 2025
@mergify mergify bot merged commit e924264 into sigp:unstable Apr 8, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
das Data Availability Sampling ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants