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

Implement Subnet Sampling for PeerDAS #6410

Merged
merged 7 commits into from
Oct 4, 2024
Merged

Implement Subnet Sampling for PeerDAS #6410

merged 7 commits into from
Oct 4, 2024

Conversation

jimmygchen
Copy link
Member

Issue Addressed

Addresses #6409

Proposed Changes

  • Add SAMPLES_PER_SLOT: 8 config.
  • Rename sampling module to peer_sampling.
  • Implement subnet sampling.

Additional Info

This PR is basically a bunch of renames, because we basically treat sampling the same as custody:

  • we need all samples for making block available
  • we require all samples during sync

With the exception that sampling subnets are not advertised in metadata and ENR. The additional sampling data may not need to be stored, however this is not currently defined in spec and will be clarified later.

@jimmygchen jimmygchen requested a review from dapplion September 18, 2024 06:19
@jimmygchen jimmygchen added ready-for-review The code is ready for review das Data Availability Sampling labels Sep 18, 2024
@jimmygchen
Merge branch 'unstable' into subnet-sampling
07afa6c 
# Conflicts:
#	beacon_node/beacon_chain/src/data_availability_checker.rs
#	beacon_node/http_api/src/publish_blocks.rs
#	beacon_node/lighthouse_network/src/types/globals.rs
#	beacon_node/network/src/sync/manager.rs
jimmygchen added a commit that referenced this pull request Oct 2, 2024
@jimmygchen
Implement Subnet Sampling for PeerDAS (#6410)
be78d29 
Squashed commit of the following:

commit 07afa6c
Merge: 35f683c 50d8375
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Thu Sep 26 17:31:37 2024 +1000

    Merge branch 'unstable' into subnet-sampling

    # Conflicts:
    #	beacon_node/beacon_chain/src/data_availability_checker.rs
    #	beacon_node/http_api/src/publish_blocks.rs
    #	beacon_node/lighthouse_network/src/types/globals.rs
    #	beacon_node/network/src/sync/manager.rs

commit 35f683c
Merge: 8a095f1 46e0d66
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Fri Sep 20 09:50:08 2024 +1000

    Merge branch 'unstable' into subnet-sampling

commit 8a095f1
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Wed Sep 18 17:07:55 2024 +1000

    Update lookup test.

commit ea46893
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Wed Sep 18 16:09:33 2024 +1000

    Implement subnet sampling.

commit 85e274f
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Wed Sep 18 14:32:43 2024 +1000

    Rename `sampling` module to `peer_sampling`

commit 13a81e8
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Wed Sep 18 12:16:19 2024 +1000

    Add `SAMPLES_PER_SLOT` config.
@dapplion
Copy link
Collaborator

dapplion commented Oct 3, 2024

I'm missing where in the code we filter out the sampled columns and only store the custody requirement.

Plus the additional sampled columns can be not random? We are always fetching the same subnets every slot and then throwing them away.

@jimmygchen
Copy link
Member Author

I'm missing where in the code we filter out the sampled columns and only store the custody requirement.

This hasn't been implemented yet, as it requires more changes and still needs to be clarified in spec (comment earlier)

Plus the additional sampled columns can be not random? We are always fetching the same subnets every slot and then throwing them away.

Yes that's right - this is the latest version of subnet sampling (#3870). I asked a similar question before about randomness, and Francesco doesn't think it matters for the network as a whole (see his explanation on discord here). And In the future clients can switch over to peer sampling whenever we're ready.

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Oct 3, 2024
@jimmygchen
Copy link
Member Author

@mergify queue

@mergify Mergify
Copy link

mergify bot commented Oct 4, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at f3a5e25

@mergify mergify bot merged commit f3a5e25 into sigp:unstable Oct 4, 2024
28 checks passed
@jimmygchen jimmygchen deleted the subnet-sampling branch October 4, 2024 00:27
This was referenced Oct 4, 2024
Open
Closed
chong-he pushed a commit to chong-he/lighthouse that referenced this pull request Nov 26, 2024
@jimmygchen @chong-he
Implement Subnet Sampling for PeerDAS (sigp#6410)
ddfa920 
* Add `SAMPLES_PER_SLOT` config.

* Rename `sampling` module to `peer_sampling`

* Implement subnet sampling.

* Update lookup test.

* Merge branch 'unstable' into subnet-sampling

* Merge branch 'unstable' into subnet-sampling

# Conflicts:
#	beacon_node/beacon_chain/src/data_availability_checker.rs
#	beacon_node/http_api/src/publish_blocks.rs
#	beacon_node/lighthouse_network/src/types/globals.rs
#	beacon_node/network/src/sync/manager.rs

* Merge branch 'unstable' into subnet-sampling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dapplion dapplion dapplion approved these changes

Assignees
No one assigned
Labels
das Data Availability Sampling ready-for-merge This PR is ready to merge.
Projects
None yet
Milestone
No milestone
Development

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