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

Add plumbing for PeerDAS supernodes (#5050, #5409, #5570, #5966) #6216

Merged
merged 9 commits into from
Aug 12, 2024
Merged

Add plumbing for PeerDAS supernodes (#5050, #5409, #5570, #5966) #6216

merged 9 commits into from
Aug 12, 2024

Conversation

jimmygchen
Copy link
Member

Issue Addressed

Part of #6072.

Add plumbing for peerdas supernodes (#5050, #5409, #5570, #5966)

  • add cli option --subscribe-to-all-data-columns
  • add custody subnet count to ENR, only if PeerDAS is scheduled
  • subscribe to data column topics, only if PeerDAS is scheduled

Upstream PRs:

@jimmygchen @jacobkaufmann
Add plumbing for peerdas supernodes (sigp#5050, sigp#5409, sigp#5570, s…
364417c 
…igp#5966)

- add cli option `--subscribe-to-all-data-columns`
- add custody subnet count to ENR, only if PeerDAS is scheduled
- subscribe to data column topics, only if PeerDAS is scheduled

Co-authored-by: Jacob Kaufmann <jacobkaufmann18@gmail.com>
@jimmygchen jimmygchen added ready-for-review The code is ready for review das Data Availability Sampling das-unstable-merge labels Aug 2, 2024
@jimmygchen jimmygchen mentioned this pull request Aug 2, 2024
Closed
dapplion
dapplion reviewed Aug 4, 2024
View reviewed changes
book/src/help_bn.md Outdated
@@ -585,6 +585,10 @@ Flags:
server on localhost:5052 and import deposit logs from the execution
node. This is equivalent to `--http` on merge-ready networks, or
`--http --eth1` pre-merge
--subscribe-all-data-column-subnets
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are going to commit to this flag, is this the best name? Another angle is to have a --supernode flag. I don't have a strong opinion but now is the time to think it twice

Copy link
Member

Choose a reason for hiding this comment

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

I like --supernode better

Copy link
Member Author

Choose a reason for hiding this comment

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

Just dropping some thoughts here - I thought about a few alternatives when originally picking this name

  • --custody-subnet-count 12: number of column subnets to custody, this is flexible but we don't really have a use case, and isn't really compatible with the upcoming validator custody and custody group changes.
  • --subscribe-all-data-column-subnets: this name is consistent with the attestation subnet flag, but thinking about this now, it's actually a bit different, because it means subscribe all subnets + import all data columns + reconstruction + distributed blob building etc. so --supernode does feel a bit more suitable.
  • --min-data-column-custody <number|all>: with the upcoming validator custody change, the number of custody columns may change during runtime, depending on the number of validators registered. Note I dropped the subnet here because of the upcoming change to decouple subnets from custody.

We've used the supernode term a lot between implementers, but feels a bit vague for people without the PeerDAS context, e.g. would a regular user think it subscribes to all attestation subnets and import all attestations too, or would it have a higher --network-load value?

The downside is that it is a bit rigid and we may need to add more flags, if one wants to run a lower bandwidth and lower storage supernode, they may want to subscribe to just 50% of the subnets - just to be able to locally perform reconstruction without having to query peers when having to serve blob_sidecars over HTTP.

Btw all other clients have adopted the same thing so far:

participants:
# Supernodes
  - cl_type: prysm
    cl_image: ethpandaops/prysm-beacon-chain:peerdas-initial-sync
    cl_extra_params: [--subscribe-all-subnets]
  - cl_type: lighthouse
    cl_image: ethpandaops/lighthouse:das-devnet-2
    cl_extra_params: [--subscribe-all-data-column-subnets]
  - cl_type: teku
    cl_image: ethpandaops/teku:nashatyrev-das
    cl_extra_params: [--p2p-subscribe-all-custody-subnets-enabled]
  - cl_type: nimbus
    cl_image: ethpandaops/nimbus-eth2:peerdas-p2p
    cl_extra_params: [--subscribe-all-subnets=true]

Copy link
Member Author

Choose a reason for hiding this comment

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

we should probably hide all peerdas flags at this stage?

Copy link
Member Author

Choose a reason for hiding this comment

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

btw I also don’t have a strong opinion, happy to switch to supernode, and I think we can still change our mind before go live

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 help and made this a hidden flag.

realbigsean
realbigsean previously requested changes Aug 5, 2024
View reviewed changes
beacon_node/network/src/service.rs Outdated Show resolved Hide resolved
beacon_node/lighthouse_network/src/discovery/enr.rs Outdated Show resolved Hide resolved
@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 Aug 6, 2024
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Aug 7, 2024
dapplion
dapplion approved these changes Aug 8, 2024
View reviewed changes
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.

Ok to keep the name, but we should either hide it or note it that it is experimental and may break

dapplion
dapplion approved these changes Aug 10, 2024
View reviewed changes
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 jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Aug 12, 2024
@jimmygchen
Copy link
Member Author

@mergify queue

@mergify Mergify
Copy link

mergify bot commented Aug 12, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at f2fdbe7

mergify bot added a commit that referenced this pull request Aug 12, 2024
@mergify
Merge of #6216
abb1802
@mergify mergify bot mentioned this pull request Aug 12, 2024
Closed
8 tasks
@mergify mergify bot mentioned this pull request Aug 12, 2024
Merged
@mergify mergify bot merged commit f2fdbe7 into sigp:unstable Aug 12, 2024
28 checks passed
ThreeHrSleep pushed a commit to ThreeHrSleep/lighthouse that referenced this pull request Aug 12, 2024
@jimmygchen @ThreeHrSleep
Add plumbing for PeerDAS supernodes (sigp#5050, sigp#5409, sigp#5570, s…
d2de306 
…igp#5966) (sigp#6216)

* Add plumbing for peerdas supernodes (sigp#5050, sigp#5409, sigp#5570, sigp#5966)
- add cli option `--subscribe-to-all-data-columns`
- add custody subnet count to ENR, only if PeerDAS is scheduled
- subscribe to data column topics, only if PeerDAS is scheduled

Co-authored-by: Jacob Kaufmann <jacobkaufmann18@gmail.com>

* Merge branch 'unstable' into das-supernode

* Update CLI docs.

* Merge branch 'unstable' into das-supernode

* Fix fork epoch comparison with `FAR_FUTURE_EPOCH`.

* Merge branch 'unstable' into das-supernode

* Hide `--subscribe-all-data-column-subnets` flag and update help.

* Fix docs only

* Merge branch 'unstable' into das-supernode
AgeManning pushed a commit to AgeManning/lighthouse that referenced this pull request Sep 3, 2024
@jimmygchen @AgeManning
Add plumbing for PeerDAS supernodes (sigp#5050, sigp#5409, sigp#5570, s…
c6857ae 
…igp#5966) (sigp#6216)

* Add plumbing for peerdas supernodes (sigp#5050, sigp#5409, sigp#5570, sigp#5966)
- add cli option `--subscribe-to-all-data-columns`
- add custody subnet count to ENR, only if PeerDAS is scheduled
- subscribe to data column topics, only if PeerDAS is scheduled

Co-authored-by: Jacob Kaufmann <jacobkaufmann18@gmail.com>

* Merge branch 'unstable' into das-supernode

* Update CLI docs.

* Merge branch 'unstable' into das-supernode

* Fix fork epoch comparison with `FAR_FUTURE_EPOCH`.

* Merge branch 'unstable' into das-supernode

* Hide `--subscribe-all-data-column-subnets` flag and update help.

* Fix docs only

* Merge branch 'unstable' into das-supernode
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

@realbigsean realbigsean Awaiting requested review from realbigsean

Assignees
No one assigned
Labels
das Data Availability Sampling das-unstable-merge 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.
3 participants
@jimmygchen @realbigsean @dapplion