Prysm: ComputeCommittee effeciency depends on separated cache fill

[protolambda]

Description

ComputeCommittee in the beacon core helpers package uses a cached array of active validator indices, but the actual shuffling is not cached.
When all active indices are shuffled, you get all the committees for that epoch. No need to re-shuffle only to get the next slice out of the shuffling result.

Has this worked before in a previous version?

It's not a bug, just an optimization that is not utilized correctly. Causing slowness, with potential side effects.

Sorry for using the bug template, the templates/labels are limited, what do I use for issues that are good enough for potential DOS or performance penalty, but not strictly a bug?

🔬 Minimal Reproduction

1. BeaconCommitteeFromState calls ActiveValidatorIndices which caches active indices.
2. BeaconCommitteeFromState calls BeaconCommittee which seems to do a second unnecessary cache lookup (minor issue)
3. BeaconCommittee calls ComputeCommittee to get the committee
4. ComputeCommittee shuffles all active validators, but then only slices out a single committee, and forgets about the other results.

Caching all resulting committees would improve the committee computation a lot. Every other client is already doing this, afaik. Small step to cache the shuffling, alike to caching the active indices.

🔥 Error

x

🌍 Your Environment

x

Edit: so there is a way of caching the full shuffling, but it's a different code path to fill that cache. It would be nice to just fill it when you have the chance to do so anyway, and get rid of the complexities to fill the cache elsewhere.

[terencechain]

I dont think we re-run shuffling for every committee. At least I haven't seen that in a flame in a while. The shuffled version of committee is cached here:

https://github.com/prysmaticlabs/prysm/blob/master/beacon-chain/cache/committee.go#L40

[protolambda]

This is where I think it happens: https://github.com/prysmaticlabs/prysm/blob/80539d9028db7a94db0da64b9ed47fb9393d6773/beacon-chain/core/helpers/committee.go#L140

My estimate is that it costs you about a second per epoch at 30k vals. Likely spread across the usage of the committees. But it scales linearly, so it may become problematic with higher counts. And API users may hit it, if they quickly query more committees in the same epoch.

And I'm confused by your link, it seems like a duplicate code path. It looks much better there, but having both options doesn't really help.

[terencechain]

We won't reach that line if there's already a committee cached via seed, slot and committee_id:

https://github.com/prysmaticlabs/prysm/blob/master/beacon-chain/core/helpers/committee.go#L102

[protolambda]

Great :), but why is it even there then? Just filling the cache when you can, and not having to manage it so closely, is less error prone.

[terencechain]

I agree on separating the caching layer and the core consensus helper layer! Will use this issue to track that.

[protolambda]

Is anyone looking at this issue still? I think it might still affect the lookup times of API calls.

[terencechain]

Not that I'm aware of. I think we are still working on urgent bug fixes and validator account revamp. Anyone is more than welcome to pick this up if they are interested!