[Merged by Bors] - Reduce attestation subscription spam from VC #4806
Conversation
michaelsproul
added
val-client
|
Initial data from Holesky looks good. No more errors about subscriptions failing. Previously we had lots of:
After the change was deployed this is gone:
Additionally upload bandwidth from the VC is reduced by about half:
And the time for the subscription service to run is also reduced:
These numbers are from a Holesky VC with 10k validators, connected to a single BN. |
There was a problem hiding this comment.
Looks good to me.
The main reason for sending every slot was just to handle edge cases of just learning about the duties and on start up.
The decaying subscription time makes a lot more sense.
I noticed the queue you have which records when we have sent a sub can have gaps/holes indicating we haven't sent subscriptions, but this doesn't matter because your searches are short circuiting on the latest ones.
So lgtm.
| let num_expected_subscriptions = std::cmp::max( | ||
| 1, | ||
| 5 * local_pubkeys.len() * ATTESTATION_SUBSCRIPTION_OFFSETS.len() | ||
| / (4 * E::slots_per_epoch() as usize), |
There was a problem hiding this comment.
Not really that important, but I didn't follow this logic. Why 5x keys? Also the division by 4?
Naively i would expect 50% of keys to do ATTESTATION_SUBSCRIPTION_OFFSET.len() -1 and they lie in one half of the epoch.
It doesn't really matter tho, we're just trying to save some allocation, the larger the better, probably
There was a problem hiding this comment.
The 5/4 is so the vec is 1/4 larger than it should be.
Other than the 5/4 the bound is quite tight because we are looking at 2 epochs total (current and next), there are 2 * local_pubkeys.len() attestation duties, and each duty gets sent at ATTESTATION_SUBSCRIPTION_OFFSETS.len() slots. So there are 2 * local_pubkeys.len() * ATTESTATION_SUBSCRIPTION_OFFSETS.len() subscription-slots to be distributed. They're approximately distributed over 2 epochs (current and next) but with a slight offset from the epoch start, e.g. some of the subscription-slots for the current epoch were in the previous epoch, but this is balanced out by some of the subscription-slots for the N + 2 epoch being in the next epoch (we just don't know it yet). Therefore the total number of subscription-slots per slot is: (2 * local_pubkeys.len() * ATTESTATION_SUBSCRIPTION_OFFSETS.len()) / (2 * E::slots_per_epoch()), and we can cancel the factor of 2.
Experimentally the number of subscriptions per slot is very close to this. For 10k validators, we'd expect 2.5k subscriptions without the 5/4 factor, and in practice we're very close to this:
DEBG Sent attestation subscriptions expected: 3124, count: 2512
DEBG Sent attestation subscriptions expected: 3124, count: 2560
DEBG Sent attestation subscriptions expected: 3124, count: 2535
DEBG Sent attestation subscriptions expected: 3124, count: 2483
|
LFG bors r+ |
michaelsproul
added
ready-for-merge
## Proposed Changes Instead of sending every attestation subscription every slot to every BN: - Send subscriptions 32, 16, 8, 7, 6, 5, 4, 3 slots before they occur. - Track whether each subscription is sent successfully and retry it in subsequent slots if necessary. ## Additional Info - [x] Add unit tests for `SubscriptionSlots`. - [x] Test on Holesky. - [x] Based on #4774 for testing.
|
Build failed: |
|
bors retry |
## Proposed Changes Instead of sending every attestation subscription every slot to every BN: - Send subscriptions 32, 16, 8, 7, 6, 5, 4, 3 slots before they occur. - Track whether each subscription is sent successfully and retry it in subsequent slots if necessary. ## Additional Info - [x] Add unit tests for `SubscriptionSlots`. - [x] Test on Holesky. - [x] Based on #4774 for testing.
|
Build failed: |
|
bors retry |
## Proposed Changes Instead of sending every attestation subscription every slot to every BN: - Send subscriptions 32, 16, 8, 7, 6, 5, 4, 3 slots before they occur. - Track whether each subscription is sent successfully and retry it in subsequent slots if necessary. ## Additional Info - [x] Add unit tests for `SubscriptionSlots`. - [x] Test on Holesky. - [x] Based on #4774 for testing.
|
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. If you want to switch to GitHub's built-in merge queue, visit their help page.
|
bors
bot
changed the title
## Proposed Changes Instead of sending every attestation subscription every slot to every BN: - Send subscriptions 32, 16, 8, 7, 6, 5, 4, 3 slots before they occur. - Track whether each subscription is sent successfully and retry it in subsequent slots if necessary. ## Additional Info - [x] Add unit tests for `SubscriptionSlots`. - [x] Test on Holesky. - [x] Based on sigp#4774 for testing.
## Proposed Changes Instead of sending every attestation subscription every slot to every BN: - Send subscriptions 32, 16, 8, 7, 6, 5, 4, 3 slots before they occur. - Track whether each subscription is sent successfully and retry it in subsequent slots if necessary. ## Additional Info - [x] Add unit tests for `SubscriptionSlots`. - [x] Test on Holesky. - [x] Based on sigp#4774 for testing.
## Proposed Changes Instead of sending every attestation subscription every slot to every BN: - Send subscriptions 32, 16, 8, 7, 6, 5, 4, 3 slots before they occur. - Track whether each subscription is sent successfully and retry it in subsequent slots if necessary. ## Additional Info - [x] Add unit tests for `SubscriptionSlots`. - [x] Test on Holesky. - [x] Based on sigp#4774 for testing.
Proposed Changes
Instead of sending every attestation subscription every slot to every BN:
Additional Info
SubscriptionSlots.