[Merged by Bors] - Shift subnet backbone structure (attnets revamp)

[AgeManning]

This PR address the following spec change: ethereum/consensus-specs#3312

Instead of subscribing to a long-lived subnet for every attached validator to a beacon node, all beacon nodes will subscribe to SUBNETS_PER_NODE long-lived subnets. This is currently set to 2 for mainnet.

This PR does not include any scoring or advanced discovery mechanisms. A future PR will improve discovery and we can implement scoring after the next hard fork when we expect all client teams and all implementations to respect this spec change.

This will be a significant change in the subnet network structure for consensus clients and we will likely have to monitor and tweak our peer management logic.

[divagant-martian]

this was private since it's a calculated field. Why make it public?

[AgeManning]

The extra bits is no longer a calculated field. The attestation_subnet_prefix_bits is now calculated from attestation_subnet_extra_bits and attestation_subnet_count.

With that said, I'm not sure we reference it explicitly anywhere, so made it pub more for consistency.

[divagant-martian]

if the epoch is 0 and the offset is 1 for a subscription duration of 10 this means the offset is backwards?
Here the valid_until_epoch is 0 + 10 - (0 + 1) = 9 I would have expected this to be 1. Not claiming this to be wrong, just making sure this is the intended use

[AgeManning]

Yeah had me worried here for a bit. Intuitively I'd agree with you.
After a bit of thought, it works like this:
The node-offset is always added to the epoch to shift the transition. It shouldn't be thought of as the epoch at which the subnet changes. i.e it wont shift at epoch 1 (like we intuitively expect).

Note that we shift epochs once: (epoch + node_offset) % subscription_duration == 0.

So in your example, we have one subnet at ( 0 + 1) % 10.
Then it will take another 9 more epochs before we hit that threshold and have to change subnets.

A better way to visualize is, is to set the node_offset to 0 and epoch to 1. We get the same result, have to wait 9 epochs.

[divagant-martian]

Not sure how hard it is to craft an easy test case, but I think it would be beneficial to test the validity of valid_until_epoch. This means testing that the range [valid_until_epoch-subscription_duration, valid_until_epoch) produce the same subnets as the expected subnets of the tested epoch

[AgeManning]

I have a few tests, where i tested against the pyspec. It's not in the rust code, but what I did was, create a for-loop in python and just go through every epoch and return the epoch when the pyspec tells us that we should be on a new spec. The test compares this fixed python result, with the calculated version in rust.

[divagant-martian]

Ok I did the tests in the range [epoch, valid_until_epoch) checking the valid_epoch doesn't change and the subnets don't change either. Also tested that in valid_during_epoch the results change. I'm therefore convinced this is legit

[divagant-martian]

I'm convinced this is fine plus it's great to finally see the legacy code removed 🚀

[divagant-martian]

This is for later, since I don't want to delay merging this PR
There is #3960, could you later test this with

diff --git a/beacon_node/network/src/subnet_service/tests/mod.rs b/beacon_node/network/src/subnet_service/tests/mod.rs
index 3b8c89a44..e9b91f3bf 100644
--- a/beacon_node/network/src/subnet_service/tests/mod.rs
+++ b/beacon_node/network/src/subnet_service/tests/mod.rs
@@ -176,7 +176,6 @@ async fn get_events<S: Stream<Item = SubnetServiceMessage> + Unpin>(
 
 mod attestation_service {
 
-    #[cfg(not(windows))]
     use crate::subnet_service::attestation_subnets::MIN_PEER_DISCOVERY_SLOT_LOOK_AHEAD;
 
     use super::*;
@@ -291,7 +290,6 @@ mod attestation_service {
     }
 
     /// Test to verify that we are not unsubscribing to a subnet before a required subscription.
-    #[cfg(not(windows))]
     #[tokio::test]
     async fn test_same_subnet_unsubscription() {
         // subscription config
@@ -536,7 +534,6 @@ mod attestation_service {
         assert_eq!(unexpected_msg_count, 0);
     }
 
-    #[cfg(not(windows))]
     #[tokio::test]
     async fn test_subscribe_same_subnet_several_slots_apart() {
         // subscription config
@@ -814,4 +811,4 @@ mod sync_committee_service {
         // Should be unsubscribed at the end.
         assert_eq!(sync_committee_service.subscription_count(), 1);
     }
-}
+}
\ No newline at end of file

[divagant-martian]

@AgeManning just noticed the feature flag is still declared in the Cargo.toml
You'll find it with a quick rg deterministic_long_lived_attnets

[michaelsproul]

bors r+

[bors]

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• arbitrary-check
• beacon-chain-tests
• cargo-audit
• cargo-fmt
• cargo-udeps
• cargo-vendor
• check-benchmarks
• check-msrv
• clippy
• compile-with-beta-compiler
• debug-tests-ubuntu
• dockerfile-ubuntu
• doppelganger-protection-test
• ef-tests-ubuntu
• eth1-simulator-ubuntu
• execution-engine-integration-ubuntu
• merge-transition-ubuntu
• no-eth1-simulator-ubuntu
• op-pool-tests
• release-tests-ubuntu
• release-tests-windows
• slasher-tests
• state-transition-vectors-ubuntu
• syncing-simulator-ubuntu