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

Simple Subnet Management #6146

Merged
merged 19 commits into from
Nov 26, 2024
Merged

Simple Subnet Management #6146

merged 19 commits into from
Nov 26, 2024

Conversation

AgeManning
Copy link
Member

Overview

This is an implementation of ethereum/consensus-specs#3735

Essentially, rather than having Lighthouse rotate every 24 hours around long-lived subnets, it will stay fixed on subnets for the duration of its run (and it's lifetime until a user manually deletes the network.key in the lighthouse directory).

The rotations have a dubious practical importance and adds quite a bit of complexity in our code base. By removing the rotations I was able to simplify the code. This also helps with debugging and keeping tracking of subnets and ENRs (our ENRs will also be stable, as we no longer need to rotate the subnet ids there).

While I was in there, I took the liberty to combine the SyncCommitteeSubnets with the AttestationSubnets into a single SubnetService which I think is a bit cleaner and easier to reason about.

Because this PR changes a bit of code and if there are bugs will immediately impact the performance of validators, we should take caution in merging this before significant testing.

@AgeManning AgeManning added the v6.0.0 New major release for hierarchical state diffs label Jul 22, 2024
@AgeManning AgeManning added the ready-for-review The code is ready for review label Jul 22, 2024
@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 Jul 29, 2024
@AgeManning AgeManning mentioned this pull request Sep 8, 2024
Closed
@AgeManning
Copy link
Member Author

Thanks for the review @jimmygchen - I think i've addressed all of these

@AgeManning AgeManning 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 Sep 9, 2024
@jimmygchen jimmygchen added under-review A reviewer has only partially completed a review. and removed ready-for-review The code is ready for review labels Oct 4, 2024
jimmygchen
jimmygchen requested changes Oct 14, 2024
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.

I've reviewed the PR and it looks much cleaner! I've added a few comments / questions.
I think this will simplify implementing PeerDAS validator custody (subscriptions) too.

beacon_node/network/src/subnet_service/mod.rs Show resolved Hide resolved
beacon_node/network/src/subnet_service/mod.rs Show resolved Hide resolved
beacon_node/network/src/subnet_service/mod.rs Outdated Show resolved Hide resolved
beacon_node/network/src/subnet_service/mod.rs Outdated Show resolved Hide resolved
beacon_node/network/src/subnet_service/mod.rs Show resolved Hide resolved
beacon_node/network/src/subnet_service/mod.rs Outdated Show resolved Hide resolved
beacon_node/network/src/subnet_service/mod.rs Outdated Show resolved Hide resolved
beacon_node/network/src/subnet_service/mod.rs Show resolved Hide resolved
beacon_node/network/src/subnet_service/mod.rs Show resolved Hide resolved
@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed under-review A reviewer has only partially completed a review. labels Oct 14, 2024
@AgeManning AgeManning 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 Oct 30, 2024
jimmygchen
jimmygchen approved these changes Oct 31, 2024
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.

Nice, looks good to me now! 👍

michaelsproul added a commit that referenced this pull request Nov 25, 2024
@michaelsproul
Simple Subnet Management (#6146)
3e77aad 
Squashed commit of the following:

commit 1beb678
Merge: 4968d36 6e1945f
Author: Age Manning <[email protected]>
Date:   Mon Nov 25 10:25:40 2024 +1100

    Merge latest unstable

commit 4968d36
Merge: 7c27978 1126058
Author: Jimmy Chen <[email protected]>
Date:   Thu Oct 31 23:26:19 2024 +1100

    Merge branch 'unstable' into simple-peer-mapping

commit 7c27978
Merge: 0130b97 7105442
Author: Age Manning <[email protected]>
Date:   Thu Oct 31 09:22:40 2024 +1100

    Merge branch 'unstable' into simple-peer-mapping

commit 0130b97
Merge: 0d319f1 48aa353
Author: Age Manning <[email protected]>
Date:   Tue Oct 29 18:00:55 2024 +1100

    Merge branch 'unstable' into simple-peer-mapping

commit 0d319f1
Merge: 8c7ba30 fe889c6
Author: Age Manning <[email protected]>
Date:   Tue Oct 29 15:33:16 2024 +1100

    Remove sync subnets from ENR on unsubscribe

commit 8c7ba30
Author: Age Manning <[email protected]>
Date:   Mon Oct 28 20:34:42 2024 +1100

    Reviewers comments

commit 65fe3b5
Merge: 6ca4a02 8188e03
Author: Age Manning <[email protected]>
Date:   Mon Oct 28 19:56:38 2024 +1100

    Merge latest unstable

commit 6ca4a02
Author: Age Manning <[email protected]>
Date:   Thu Sep 19 12:06:06 2024 +1000

    Fix tests

commit d96075e
Merge: 0a20547 8b085dd
Author: Age Manning <[email protected]>
Date:   Thu Sep 19 11:39:09 2024 +1000

    Merge latest unstable

commit 0a20547
Author: Age Manning <[email protected]>
Date:   Thu Sep 19 11:38:21 2024 +1000

    Fix lints

commit 1b6833f
Author: Age Manning <[email protected]>
Date:   Mon Sep 9 12:42:23 2024 +1000

    Missed a comment, corrected it

commit 03b4049
Author: Age Manning <[email protected]>
Date:   Mon Sep 9 12:39:10 2024 +1000

    Fix errors

commit b0ac85e
Merge: 1a89fb4 873748d
Author: Age Manning <[email protected]>
Date:   Mon Sep 9 12:33:34 2024 +1000

    Merge latest unstable

commit 1a89fb4
Author: Age Manning <[email protected]>
Date:   Mon Sep 9 12:27:19 2024 +1000

    Correct comments and reviewers comments

commit 6bd0f75
Author: Age Manning <[email protected]>
Date:   Mon Jul 22 16:15:49 2024 +1000

    Update tests for new version

commit bd5a753
Author: Age Manning <[email protected]>
Date:   Tue Jul 16 16:51:22 2024 +1000

    First draft without tests

commit 0dd4e05
Merge: 2c42e45 4cfdd82
Author: Age Manning <[email protected]>
Date:   Wed Jul 10 14:22:21 2024 +1000

    Merge latest unstable

commit 2c42e45
Author: Age Manning <[email protected]>
Date:   Mon May 13 10:10:40 2024 +0300

    Initial temp commit
@michaelsproul michaelsproul mentioned this pull request Nov 25, 2024
Merged
michaelsproul added a commit that referenced this pull request Nov 25, 2024
@michaelsproul
Simple Subnet Management (#6146)
8154f5a 
Squashed commit of the following:

commit 51cbf1c
Author: Michael Sproul <[email protected]>
Date:   Mon Nov 25 15:56:54 2024 +1100

    Prevent clash with pin of rust_eth_kzg

commit 1beb678
Merge: 4968d36 6e1945f
Author: Age Manning <[email protected]>
Date:   Mon Nov 25 10:25:40 2024 +1100

    Merge latest unstable

commit 4968d36
Merge: 7c27978 1126058
Author: Jimmy Chen <[email protected]>
Date:   Thu Oct 31 23:26:19 2024 +1100

    Merge branch 'unstable' into simple-peer-mapping

commit 7c27978
Merge: 0130b97 7105442
Author: Age Manning <[email protected]>
Date:   Thu Oct 31 09:22:40 2024 +1100

    Merge branch 'unstable' into simple-peer-mapping

commit 0130b97
Merge: 0d319f1 48aa353
Author: Age Manning <[email protected]>
Date:   Tue Oct 29 18:00:55 2024 +1100

    Merge branch 'unstable' into simple-peer-mapping

commit 0d319f1
Merge: 8c7ba30 fe889c6
Author: Age Manning <[email protected]>
Date:   Tue Oct 29 15:33:16 2024 +1100

    Remove sync subnets from ENR on unsubscribe

commit 8c7ba30
Author: Age Manning <[email protected]>
Date:   Mon Oct 28 20:34:42 2024 +1100

    Reviewers comments

commit 65fe3b5
Merge: 6ca4a02 8188e03
Author: Age Manning <[email protected]>
Date:   Mon Oct 28 19:56:38 2024 +1100

    Merge latest unstable

commit 6ca4a02
Author: Age Manning <[email protected]>
Date:   Thu Sep 19 12:06:06 2024 +1000

    Fix tests

commit d96075e
Merge: 0a20547 8b085dd
Author: Age Manning <[email protected]>
Date:   Thu Sep 19 11:39:09 2024 +1000

    Merge latest unstable

commit 0a20547
Author: Age Manning <[email protected]>
Date:   Thu Sep 19 11:38:21 2024 +1000

    Fix lints

commit 1b6833f
Author: Age Manning <[email protected]>
Date:   Mon Sep 9 12:42:23 2024 +1000

    Missed a comment, corrected it

commit 03b4049
Author: Age Manning <[email protected]>
Date:   Mon Sep 9 12:39:10 2024 +1000

    Fix errors

commit b0ac85e
Merge: 1a89fb4 873748d
Author: Age Manning <[email protected]>
Date:   Mon Sep 9 12:33:34 2024 +1000

    Merge latest unstable

commit 1a89fb4
Author: Age Manning <[email protected]>
Date:   Mon Sep 9 12:27:19 2024 +1000

    Correct comments and reviewers comments

commit 6bd0f75
Author: Age Manning <[email protected]>
Date:   Mon Jul 22 16:15:49 2024 +1000

    Update tests for new version

commit bd5a753
Author: Age Manning <[email protected]>
Date:   Tue Jul 16 16:51:22 2024 +1000

    First draft without tests

commit 0dd4e05
Merge: 2c42e45 4cfdd82
Author: Age Manning <[email protected]>
Date:   Wed Jul 10 14:22:21 2024 +1000

    Merge latest unstable

commit 2c42e45
Author: Age Manning <[email protected]>
Date:   Mon May 13 10:10:40 2024 +0300

    Initial temp commit
@michaelsproul
Copy link
Member

@mergify queue

@mergify Mergify
Copy link

mergify bot commented Nov 26, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 08e8b92

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Nov 26, 2024
@mergify mergify bot merged commit 08e8b92 into sigp:unstable Nov 26, 2024
29 checks passed
michaelsproul
michaelsproul reviewed Nov 26, 2024
View reviewed changes
consensus/types/src/chain_spec.rs
@@ -1701,9 +1669,6 @@ impl Config {
maximum_gossip_clock_disparity_millis: spec.maximum_gossip_clock_disparity_millis,
message_domain_invalid_snappy: spec.message_domain_invalid_snappy,
message_domain_valid_snappy: spec.message_domain_valid_snappy,
attestation_subnet_extra_bits: spec.attestation_subnet_extra_bits,
Copy link
Member

Choose a reason for hiding this comment

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

The removal of these fields may break some third-party HTTP API consumers. I will flag this PR as backwards-incompat so that this is mentioned in the release notes.

@michaelsproul michaelsproul added backwards-incompat Backwards-incompatible API change HTTP-API labels Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelsproul michaelsproul michaelsproul left review comments

@jimmygchen jimmygchen jimmygchen approved these changes

@jxs jxs Awaiting requested review from jxs jxs is a code owner

@pawanjay176 pawanjay176 Awaiting requested review from pawanjay176

Assignees
No one assigned
Labels
backwards-incompat Backwards-incompatible API change HTTP-API ready-for-merge This PR is ready to merge. v6.0.0 New major release for hierarchical state diffs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AgeManning @michaelsproul @jimmygchen