-
Notifications
You must be signed in to change notification settings - Fork 997
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
Attnet revamp: Subnet backbone structure based on beacon nodes #3312
Conversation
Just to give some numbers, since I was wondering about that:
That's assuming you want dLow in every subnet (to be able to publish). In nimbus we seek dHigh peers to be safe, so that would be even more. And if you want to subscribe-all-subnets, that's |
Yep, good point and important to highlight. If we want to maintain these gossip parameters without discovery, we're probably looking at these numbers. I'm of the opinion that our D_lo and D_high numbers are too high, so in Lighthouse, I'd be targeting D_lo. Also we'd gain extra subscriptions from validators aggregating so will bring the average number of peers subscribed to a subnet up slightly. I imagine we probably want to keep I agree this change will require higher peer counts than we had before as the network will be less centralized around beacon nodes with lots of validators. It might make sense to tune gossipsub if we are seeing issues around this (perhaps increasing the heartbeat etc). I was hoping that we could keep lower stable peers on subnets, i.e around 3 or 4 and then use discovery a little more as we can target specific subnet peers to find if needed. We could also set |
One thing to be wary of with relying on discovery is that there's a ridiculous number of nodes that don't accept inbound connections and (partly because of that) nodes with open ports often are at their peer limit so can be hard to get a stable connection to. While this change will make it easier to search the DHT for peers that should be on the subnet you need, it won't make it easier to actually connect to them so discovery still may not be a very effective option. You probably have better data on this than I do, but figured I'd mention it. |
On the other hand, the less peer you publish to, the more you are susceptible to sybil "black holes" attacks Personally I would err on the side of more connected peers, a "parked" peer shouldn't cost much resources anyway |
Yeah I agree with everything here. I have no data, but I'm hoping we can improve the connection issue by increasing peer counts and also removing the desire for everyone to connect to a smallish set of nodes that have a lot of subnets. My thinking was to maintain some low set of peers (3/4) per subnet and potentially also do discovery requests to target 6 or something as an optional extra (if we are not at the target). But it probably is smarter to just have a larger set of "parked" peers and reduce any extra resources in maintaining them. In the end, I think we'll be fine tuning this if we agree this is a good approach and each client can figure out what works best for them. edit: To add to the connection issue, there's some NAT hole-punching work being added to discovery and I'm hoping to translate that into our TCP/libp2p stack which should also help with connectivity, i'd imagine |
Just to confirm, nodes running with the new backbone structure would be inserting the predetermined subnets into the current attnets field in their enr , correct ?
If we do simply start bumping
This seems vulnerable to possible participation drops across the network in that epoch. If you do have all validators on long-lived subnets suddenly changing to a new one at the same time, it becomes a lot more chaotic for that particular node to find peers for that subnet simply for the fact that every node in the network is doing it at the same time. You could see large spikes in inbound connection attempts, and peers getting kicked off due to the fact that a node is 'full' . Is it possible to stagger these updates in the shuffling for subnets via the |
Correct. Just as they do now.
I'd imagine we'd see a drop in bandwidth as there would be less nodes per subnet than there currently are. There is a chance that we make this too low and it becomes hard to find and maintain peers on subnets. This can be corrected by either increasing
I couldn't find an easy way to stagger the transition, whilst maintaining a uniform distribution and have it be general of subnet count. In this PR I suggest that nodes don't transition on the exact epoch. Instead they subscribe earlier and unsubscribe later, so there is a period of overlap. The exact epoch where the logic switches, is just about the enforceability property. You have to be subscribed at that epoch, but you can and should be subscribed earlier. |
I'm curious as to the issue with the proposal I made a few days ago in this respect. With the current numbers 1/256 of the nodes would move subnet each epoch, would this skew the distribution compared to them all moving at the same time? |
Avoiding the sudden rotation issue with subscribing to next subnet a bit early and unsubscribing from prev subnet a bit latter is nice solution. AFAIK most clients already implement something like this around forks when they rotate all topics to the next fork. |
Yeah this could work, I think. The absolute worst case would be that 1/256 of the nodes (25% of a subnet) all belonged to the same subnet. Then each epoch 25% of a subnet could transition to another epoch. Although it is unlikely (assuming node-ids are uniformly distributed), it is possible that an entire subnet transitions to another subnet and we have no nodes on a given subnet. The odds of this are very low, perhaps its worth calculating them. The other change this will make will be that our peers will be constantly shifting long-lived subnets every epoch. Because this is deterministic, it would be manageable from a client perspective as everyone would know in advance which peer goes to which subnet, but it could make peer management a bit trickier. Other than that, it seems like a pretty good approach to get nodes to transition throughout the epochs. |
That makes sense, in that case I would have less of a concern as it would just be a matter of doing a lookahead to the transition epoch and subscribing beforehand. We do the same exact thing for sync committee subnet subscriptions, so it shouldn't be an issue as long as there is a sufficient amount of lookahead for individual nodes. This has the same effect as a staggered transition, so the proposal looks good for this case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! I'm really excited to get this out.
The biggest downside I see right now is the suggestion of 2 rather than 1 for those stakers currently with 1 validator. I think 2 is the proper number for now, a conservative choice for this critical component. just need to get our node count up...
specs/phase0/validator.md
Outdated
def compute_subnet(node_id: int, epoch: Epoch, index: int) -> int: | ||
node_id_prefix = node_id >> (256 - ATTESTATION_SUBNET_PREFIX_BITS) | ||
permutation_seed = hash(uint_to_bytes(epoch // EPOCHS_PER_SUBNET_SUBSCRIPTION)) | ||
permutated_prefix = compute_shuffled_index(node_id_prefix, 1 << ATTESTATION_SUBNET_PREFIX_BITS, permutation_seed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth putting an historic randao mix (that can be retrieved from state) in here?
It would define how long before a node_id can be grinded to do some attack.
To not run into issues with potential trailing finality windows, you'd probably need this to be on the order of a month or more. which then degrades the utility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to add this complexity in if we feel it warrants it. We had a small discussion on this in the issue.
My current thinking is that if someone wants to attack a subnet in this manner now, its pretty easy to do without having to generate node-ids. You could just make a bunch of nodes and set your ENR field to a subnet (I realize that the argument that the vulnerability currently exists is not a good argument to not fix it 😅 )
In the new form, I guess if we modify our discovery to start searching for these peer-ids, maybe there is a greater chance of being eclipsed as we are in effect splitting the DHT into smaller sub-DHTs. I think the security guarantees are the same, in that if there are some honest nodes discovery should find them also.
Mixing in RANDAO then ties in fork-choice (as you've pointed out). If it's too new and peers are temporarily on different forks, I'd imagine we'd start penalizing our peers for not being on the right subnets etc.
Do you have a suggestion for mixing in the RANDO. Like maybe current_epoch - 20
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with using anything moderately recent is that a chain split past that depth (non-finality period) would segment the p2p network because people (from your perspective) wouldn't be on the correct subnets and yuo would downscore. In practice, if there is actually a split, there is likely some sort of partition anyway.
But it does begin to seem dangerous for recent windows
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree.
My thoughts on this is to leave it deterministic for the time being. Perhaps we could add it in in a later PR. I imagine if we decide to set ATTESTATION_SUBNET_EXTRA_BITS to something non-zero, we could add it in then with that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a suggestion for mixing in the RANDO. Like maybe current_epoch - 20 or something?
The number 20 doesn't have to be fixed. I think using the randao mix of the finalized_checkpoint of the head block is the best idea now, since it's unlikely to be reverted.
specs/phase0/validator.md
Outdated
return [compute_subnet(node_id, epoch, idx) for idx in range(SUBNETS_PER_NODE)] | ||
``` | ||
|
||
*Note*: Nodes should subscribe to new subnets and remain subscribed to old subnets for at least one epoch. Nodes should pick a random duration to unsubscribe from old subnets to smooth the transition on the exact epoch boundary of which the shuffling changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to stagger subnet subscriptions to avoid the network churning all at once (and to avoid the issue of having to double-up on subnets for some amount of time).
If the node-id dictated the epoch to churn -- essentially defining a (256, 512] period in which you keep your subscription, then we'd not have the churn all at once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The random local offset could work too if it's spaced enough but then we lose the determinism (and penalization) in that period
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, seems this is an active convo on the pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added Jim's suggestion for now.
It seems the boundary rotation was of concern to a few people. I've added in @mcdee's suggestion, where every epoch 1/256 of the nodes do their transition. Curious about other's opinions on this solution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The staggering looks good -- one quick question on groupings
UPD: |
This is in a "LAST CALL" state., with the intention of merging on May 4 if no further issues emerge. Note, this will be merged without the downscoring option to ensure that it can be rolled out iteratively across clients. Options to downscore if not on appropriate peer-id attnets will be added at a subsequent hard fork |
specs/phase0/validator.md
Outdated
node_id_prefix = node_id >> (256 - int(ATTESTATION_SUBNET_PREFIX_BITS)) | ||
node_offset = node_id % EPOCHS_PER_SUBNET_SUBSCRIPTION | ||
permutation_seed = hash(uint_to_bytes(uint64((epoch + node_offset) // EPOCHS_PER_SUBNET_SUBSCRIPTION))) | ||
permutated_prefix = compute_shuffled_index( | ||
node_id_prefix, | ||
1 << int(ATTESTATION_SUBNET_PREFIX_BITS), | ||
permutation_seed, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AgeManning I added three castings (int()
and uint64()
) since remerkleable
doesn't allow some mix int
& unit
operations.
specs/phase0/validator.md
Outdated
*Note*: Short lived beacon committee assignments should not be added in into the ENR `attnets` entry. | ||
```python | ||
def compute_subscribed_subnet(node_id: int, epoch: Epoch, index: int) -> int: | ||
node_id_prefix = node_id >> (256 - int(ATTESTATION_SUBNET_PREFIX_BITS)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about adding assert spec.ATTESTATION_SUBNET_PREFIX_BITS <= 256
in test_config_invariants.py
unittest. But before that, should 256
be parameterized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to add NODE_ID_SIZE = 32
and derive bit count from it?
specs/phase0/validator.md
Outdated
|
||
*Note*: Short lived beacon committee assignments should not be added in into the ENR `attnets` entry. | ||
```python | ||
def compute_subscribed_subnet(node_id: int, epoch: Epoch, index: int) -> int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the maximum size of node_id
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it was assumed UInt256
. Not sure though what's the most appropriate type for pyspec
Some updates:
|
Sounds good. Thanks @hwwhww. The node-id is a 32-byte hash so fixing it to 256 bits is fine :). |
In the process of releasing this thing. I was thinking potentially having a different value of But perhaps we want to keep it low for testnets to detect any cracks before we hit mainnet (the testnets have lower node counts). |
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.
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.
I assumed this was well-known (ie it has been discussed before), but it seems it was not per comments on the latest consensus call: Regarding the point that nodes can use this structure enforce participation in subnet traffic, this PR does not help: a node can simply subscribe to a topic and not join the mesh to achieve a similar effect to not subscribing at all (there is still no way to detect if a node has a full mesh already and therefore legitimately is pruning) - if we start enforcing this in clients, it's likely we'll see larger subscription tables achieving a small increased book-keeping cost but still no more nodes carrying traffic. |
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.
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.
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.
This is in a "LAST CALL" state., with the intention of merging on May 4 if no further issues emerge.
Note, this will be merged without the downscoring option to ensure that it can be rolled out iteratively across clients. Options to downscore if not on appropriate peer-id attnets will be added at a subsequent hard fork
Overview
This PR is for #2749
The issue discussion highlights the main motivations for this change. The fundamental premise is that there currently is no way to enforce validators to subscribe to long-lived subnets, we may be oversubscribed to subnets and this results in significant bandwidth use.
The proposed solution is to get beacon nodes to subscribe to long-lived subnets based on their node-id (can think peer-id). Some benefits of this are:
SUBNETS_PER_NODE
parameterattnet
field in the ENR (improving validator privacy) and in the future potentially remove themetadata
RPC method if a similar approach can be found for the sync committee subnets.SUBNETS_PER_NODE
to counter issues.Some downsides are:
SUBNETS_PER_NODE
subnets.Useful statistics
I ran though the DHT to get an idea of subnet peer density and what it would look like after this change.
I measured 4.2k nodes on mainnet.
There are currently around 500 nodes per subnet. That is, if you search for a specific subnet, there should be around 500 distinct nodes to connect to (many nodes can subscribe to more than 1 subnet)
This change (with
SUBNETS_PER_NODE
==2 ) would reduce the density just under 200 nodes per subnet.This would increase the total number of nodes subscribed to subnets from 2.3k to the full 4.2k (as all nodes now need to subscribe) and I'd expect it to be easier to find connections to peers on subnets.
If we were to go with
SUBNETS_PER_NODE
==1 there would be just under 100 nodes per subnet.I've opted for 2 just to be conservative.
Additional Notes
I've currently kept
ATTESTATION_SUBNET_EXTRA_BITS
at 0 to avoid complexity in the initial stages to see how discovery functions and debugging (but we can set this to higher now or later).I've left most of the current logic around the
attnets
field as is, as this should be kept for the transitional period. I've not touched the sync subnets.Personally, I'm in favour of this and am keen to push this along. However as always am open to feedback/suggestions and everyone's thoughts on whether we should actually do this.